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

[Junio C Hamano <gitster@pobox.com>]

Mike Hommey <mh@glandium.org> writes:

> +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". ;-)

> +
> +	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/).

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