Re: [PATCH 2/4] do not send client agent unless server does first

Jeff King [off-list ref] writes:

Instead, each client must record whether we saw an agent
string from the server, and respond with its agent only if
the server mentioned it first.

Just a couple of minor comments.

This is only set to false once per process invocation.  We do not
currently talk with more than one remote from the same process in
the "fetch" codepath, and we must maintain that. This fix will be
broken otherwise ("recursive submodule fetch" comes to mind; one
more reason it should do its submodule business in a separate
process).

This codepath still forgets to check if the other side advertised
"thin-pack", "no-progress", and "include-tag", no?

This is initialied to 0 per communication, so having multiple
remote.$there.pushURL configuration variables will work correctly.

On Fri, Aug 10, 2012 at 12:45:19PM -0700, Junio C Hamano wrote:

quoted

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index fe56596..bc7a0f9 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c

@@ -19,6 +19,7 @@ static int prefer_ofs_delta = 1;
 static int no_done;
 static int fetch_fsck_objects = -1;
 static int transfer_fsck_objects = -1;
+static int agent_supported;
 static struct fetch_pack_args args = {
 	/* .uploadpack = */ "git-upload-pack",
 };

This is only set to false once per process invocation.  We do not
currently talk with more than one remote from the same process in
the "fetch" codepath, and we must maintain that. This fix will be
broken otherwise ("recursive submodule fetch" comes to mind; one
more reason it should do its submodule business in a separate
process).

Right; I followed the existing options in both fetch-pack and send-pack
(use_sideband, no_done, and others here have the same issue), with the
assumption that the current code was not broken (which may not be true,
if it is simply masked by the fact that in practice everybody happens to
support those older features).

I don't know off-hand whether that is actually a trigger-able bug in the
current code or not. It's probably a good topic for a follow-on series.

quoted

@@ -328,7 +329,8 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 			if (args.no_progress)   strbuf_addstr(&c, " no-progress");
 			if (args.include_tag)   strbuf_addstr(&c, " include-tag");

This codepath still forgets to check if the other side advertised
"thin-pack", "no-progress", and "include-tag", no?

Yes. I didn't realize it until Dave mentioned the "innocuous" list. I
think cleaning them up is reasonable, but probably a separate topic.

quoted

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 5c69995..7d05064 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c

@@ -252,6 +252,7 @@ int send_pack(struct send_pack_args *args,
 	int status_report = 0;
 	int use_sideband = 0;
 	int quiet_supported = 0;
+	int agent_supported = 0;
 	unsigned cmds_sent = 0;
 	int ret;
 	struct async demux;

This is initialied to 0 per communication, so having multiple
remote.$there.pushURL configuration variables will work correctly.

Right. Not through me having thought about it, though, but by following
the existing convention.

-Peff