From: Jeff King <peff@peff.net>
Date: Mon, 13 Aug 2012 21:11:10 GMT
Subject: Re: [PATCH 4/4] fetch-pack: mention server version with verbose output
Message-ID: <20120813211109.GA32688@sigill.intra.peff.net>
In-Reply-To: <7v628mcxdf.fsf@alter.siamese.dyndns.org>

On Mon, Aug 13, 2012 at 02:09:32PM -0700, Junio C Hamano wrote:

> >> +	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).
> 
> Actually, the above is vastly superiour compared to the allocating
> kind.  Be honest and think about it.  If the caller wants to
> allocate, it could, and it does not even have to count.  If the
> caller does not want to allocate, it does not have to pay the price.

My point is that the run-time allocation price is quite small, but the
readability cost of that ugly conditional with the magic "5" is
non-trivial. But they are apples and oranges, so it is hard to compare
their amounts directly.

-Peff