From: Jeff King <peff@peff.net>
Date: Wed, 27 May 2015 07:02:02 GMT
Subject: Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number
Message-ID: <20150527070202.GG885@peff.net>
In-Reply-To: <1432677675-5118-9-git-send-email-sbeller@google.com>

On Tue, May 26, 2015 at 03:01:12PM -0700, Stefan Beller wrote:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  transport.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/transport.c b/transport.c
> index 3ef15f6..33644a6 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -496,15 +496,29 @@ static int set_git_option(struct git_transport_options *opts,
>  static int connect_setup(struct transport *transport, int for_push, int verbose)
>  {
>  	struct git_transport_data *data = transport->data;
> +	const char *remote_program;
> +	char *buf = 0;

Use NULL when you mean a NULL pointer (they're equivalent to the
compiler, but the word is easier to read).

I agree on Eric's naming this "to_free" (and I consider it idiomatic to
assign them in a chain, like "foo = to_free = xmalloc(...)", but we
don't always do that).

> +	if (transport->smart_options
> +	    && transport->smart_options->transport_version) {
> +		buf = xmalloc(strlen(remote_program) + 12);
> +		sprintf(buf, "%s-%d", remote_program,
> +			transport->smart_options->transport_version);
> +		remote_program = buf;
> +	}

Using xstrfmt can help you avoid magic numbers and repetition,
like:

  to_free = xstrfmt("%s-%d",
                    remote_program,
		    transport->smart_options->transport_version);

-Peff