Re: [PATCH 0/4] jk/version-string and google code

[Junio C Hamano <gitster@pobox.com>]

Dave Borowitz <dborowitz@google.com> writes:

> You may also notice in that code a set of innocuous_capabilities,
> which IIRC is the complete set of capabilities, at the time of
> writing, that the C git client may send without the server advertising
> them. Such a set (painstakingly assembled, I assure you :) may be
> useful as we move further in this direction.

In builtin/fetch-pack.c, we find this:

		if (!fetching) {
			struct strbuf c = STRBUF_INIT;
			if (multi_ack == 2)     strbuf_addstr(&c, " multi_ack_detailed");
			if (multi_ack == 1)     strbuf_addstr(&c, " multi_ack");
			if (no_done)            strbuf_addstr(&c, " no-done");
			if (use_sideband == 2)  strbuf_addstr(&c, " side-band-64k");
			if (use_sideband == 1)  strbuf_addstr(&c, " side-band");
			if (args.use_thin_pack) strbuf_addstr(&c, " thin-pack");
			if (args.no_progress)   strbuf_addstr(&c, " no-progress");
			if (args.include_tag)   strbuf_addstr(&c, " include-tag");
			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
			strbuf_release(&c);
		} else
			packet_buf_write(&req_buf, "want %s\n", remote_hex);

The ones we choose to throw at the other end based on "args.foo" are
not protected by "server_supports()" at all, which is where the
hardcoded list of "innocuous capabilities" comes from.  I would say
this is a client bug.  I wish Dulwich folks didn't choose to be
silent when they added that hardcoded list as a workaround.

If a client threw a request X at a server that does not support it,
and relied on a server bug that does not reject such a request to
allow it send a pack stream that does not conform to what X asked,
and handled the pack stream assuming that the server did X, it would
be a triple bug on the client's end.  Depending on the nature of X,
the end result may be broken. Dulwich is correct to raise an
exception upon seeing agent=foo.

One could argue that from correctness standpoint, being asked to
send ofs-delta and using only ref-delta does not make a corrupt
packfile (it just makes things less efficient), but we cannot
guarantee that all protocol capabilities will be "innocuous" that
way. Longer term direction should be to reduce the "innocuous" set.

Thanks.