Re: [PATCH 4/4] fetch-pack: mention server version with verbose output

From: Jeff King <peff@peff.net>
Date: Aug 13, 2012, 20:54 UTC
Message-ID: <20120813205423.GA31630@sigill.intra.peff.net>
In-Reply-To: <7v7gt2ehl4.fsf_-_@alter.siamese.dyndns.org>

On Mon, Aug 13, 2012 at 12:07:35PM -0700, Junio C Hamano wrote:

Show 18 quoted lines

>  * And this is your 4 adjusted for the previous one, releaving the
>    caller from having to figure out where the capability string
>    ends.
> [...]
> @@ -829,8 +831,15 @@ static struct ref *do_fetch_pack(int fd[2],
>  			fprintf(stderr, "Server supports ofs-delta\n");
>  	} else
>  		prefer_ofs_delta = 0;
> -	if (server_supports("agent"))
> +
> +	if ((agent_feature = server_feature("agent", &agent_len)) != NULL &&
> +	    5 < agent_len && agent_feature[5] == '=') {
>  		agent_supported = 1;
> +		if (args.verbose) {
> +			fprintf(stderr, "Server version is %.*s\n",
> +				agent_len - 6, agent_feature + 6);
> +		}
> +	}

Yeah, this is exactly the kind of ugliness I was trying to avoid with my allocating wrapper. Still, there is only one call site, so I do not care overly much (and I as I've already said, I'm lukewarm on the final two patches, anyway).

There is one difference in your code and mine. With mine, the server can say simply "agent" to tell the client that it understands the extension but does not wish to disclose its version. That might be considered unfriendly (why would the client show theirs if the server is not willing to do the same?), but it may be a practical decision (e.g., security policies may say that servers are higher-risk targets[1]). Of course, a server can also say "agent=git/none-of-your-business"; this is just a syntactic question.

-Peff

[1] I think you and I both agreed earlier that the "sharing versions is
    a security risk" line of argument is not that compelling, but that
    does not mean it is not made all the time.

-Peff