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

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

Jeff King <peff@peff.net> 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.

> 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).

> @@ -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?

>  			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
> -			strbuf_addf(&c, " agent=%s", git_user_agent_sanitized());
> +			if (agent_supported)    strbuf_addf(&c, " agent=%s",
> +							    git_user_agent_sanitized());
> 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.