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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 10, 2012, 19:45 UTC
Message-ID: <7vd32yjzu8.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120810075743.GB8399@sigill.intra.peff.net>

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.

Show 12 quoted lines

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

Show 16 quoted lines

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