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

[Mike Hommey <mh@glandium.org>]

On Sat, Nov 03, 2007 at 11:54:38AM +0000, Johannes Schindelin wrote:
> > +{
> > +	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.

What do you mean ?

(...)
> 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.
(...)
> This can be done much easier with 'eob = strstr(sp, "\n" PGP_SIGNATURE 
> "\n");'.

I must say I just stole most of it in show_reference() in the same file.

> > +}
> > +
> >  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.

I'll fix this.

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

You're obviously right.

(...)
> 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.

I would have if I had looked up for is_null_sha1() earlier ;)

Cheers,

Mike