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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Sat, 3 Nov 2007, Mike Hommey wrote:

> diff --git a/builtin-tag.c b/builtin-tag.c
> index 66e5a58..cfd8017 100644
> --- a/builtin-tag.c
> +++ b/builtin-tag.c
> @@ -247,9 +247,42 @@ static int git_tag_config(const char *var, const char *value)
>  	return git_default_config(var, value);
>  }
>  
> +static void write_annotation(int fd, const unsigned char *sha1)

Technically, it is the "body".

> +{
> +	int i;
> +	unsigned long size;
> +	enum object_type type;
> +	char *buf, *sp, *eol;
> +	size_t len;
> +
> +	sp = buf = read_sha1_file(sha1, &type, &size);
> +	if (!buf)
> +		return;
> +	if (!size || (type != OBJ_TAG)) {

Please lose the extra parents.

> +		free(buf);
> +		return;
> +	}
> +	/* skip header */
> +	while (sp + 1 < buf + size &&
> +			!(sp[0] == '\n' && sp[1] == '\n'))
> +		sp++;

This can be done much easier with 'sp = strstr(buf, "\n\n");'.  You can 
even do that before the previous if(), to free() && return if there is no 
body.

> +	/* strip the signature */
> +	for (i = 0, sp += 2; sp < buf + size &&
> +			prefixcmp(sp, PGP_SIGNATURE "\n");
> +			i++) {
> +		eol = memchr(sp, '\n', size - (sp - buf));
> +		len = eol ? eol - sp : size - (sp - buf);
> +		write_or_die(fd, sp, len + 1);
> +		if (!eol)
> +			break;
> +		sp = eol + 1;
> +	}
> +	free(buf);

This can be done much easier with 'eob = strstr(sp, "\n" PGP_SIGNATURE 
"\n");'.

> +}
> +
>  static void create_tag(const unsigned char *object, const char *tag,
>  		       struct strbuf *buf, int message, int sign,
> -			   unsigned char *result)
> +			unsigned char *prev, unsigned char *result)

This changes indentation.

> @@ -282,6 +315,10 @@ static void create_tag(const unsigned char *object, const char *tag,
>  		if (fd < 0)
>  			die("could not create file '%s': %s",
>  						path, strerror(errno));
> +
> +		if (prev)
> +			write_annotation(fd, prev);
> +
>  		write_or_die(fd, tag_template, strlen(tag_template));

Isn't an "else" missing before the write_or_die() here?

> @@ -308,7 +345,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  {
>  	struct strbuf buf;
>  	unsigned char object[20], prev[20];
> -	int annotate = 0, sign = 0, force = 0, lines = 0, message = 0;
> +	int annotate = 0, sign = 0, force = 0, lines = 0,
> +	    message = 0, existed = 0;
>  	char ref[PATH_MAX];
>  	const char *object_ref, *tag;
>  	int i;
> @@ -417,9 +455,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  		hashclr(prev);
>  	else if (!force)
>  		die("tag '%s' already exists", tag);
> +	else
> +		existed = 1;
>  
>  	if (annotate)
> -		create_tag(object, tag, &buf, message, sign, object);
> +		create_tag(object, tag, &buf, message, sign,
> +			   existed ? prev : NULL, object);

Why not teach write_annotations() (or write_tag_body() like I would prefer 
it to be called) to grok a null_sha1?  It's not like we care for 
performance here, but rather for readability and ease of use.

Ciao,
Dscho