Re: [PATCH 1/2] Reuse previous annotation when overwriting a tag

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 3, 2007, 18:47 UTC
Message-ID: <7vlk9fxj1r.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1194095285-18651-1-git-send-email-mh@glandium.org>

Mike Hommey <mh@glandium.org> writes:

Show 8 quoted lines

> +static void write_tag_body(int fd, const unsigned char *sha1)
> +{
> ...
> +	sp = buf = read_sha1_file(sha1, &type, &size);
> +	if (!buf)
> +		return;
> +	/* skip header */
> +	sp = strstr(buf, "\n\n");

I was relieved to see this second assignment to "sp" here.

Why?

Because I wanted to say something about the first assignment to it, that is done this way:

> +	sp = buf = read_sha1_file(sha1, &type, &size);

The original git codebase, as it came from Linus, tends to avoid assignment to multiple variables in a single statement like this (and that style is written down in the kernel coding style document). As I do not have a strong opinion against that coding style, I've tried to follow it myself. However, I do not personaly have a strong argument to support enforcing the style to others.

But in this case, as the variable "sp" is never used before it is reassigned, I can easily say "drop the useless assignment to sp there". ;-)

Show 6 quoted lines

> +
> +	if (!sp || !size || type != OBJ_TAG) {
> +		free(buf);
> +		return;
> +	}
> +	sp += 2; /* skip the 2 CRs */

You are not skipping carriage returns. You are skipping line feeds (i.e. s/CRs/LFs/).

Show 11 quoted lines

> @@ -282,7 +313,11 @@ static void create_tag(const unsigned char *object, const char *tag,
>  		if (fd < 0)
>  			die("could not create file '%s': %s",
>  						path, strerror(errno));
> -		write_or_die(fd, tag_template, strlen(tag_template));
> +
> +		if (prev)
> +			write_tag_body(fd, prev);
> +		else
> +			write_or_die(fd, tag_template, strlen(tag_template));
>  		close(fd);

When prev is not NULL but points at a null_sha1 nobody writes anything out. Is this intended?

        In fact, the calling site always passes prev which is
        prev[] in cmd_tag() and cannot be non-NULL.

Why is there "else" in the first place?  Even if you start with
the previous tag's message, you are launching the editor for the
user to further edit it, and you would want to give some
instructions, wouldn't you?