[PATCH] Reuse previous annotation when overwriting a tag

[PATCH] Reuse previous annotation when overwriting a tag

[Mike Hommey]

When forcing to overwrite an annotated tag, there are good chances one
wants to keep the old annotation, or modify it, not start from scratch.

This is obviously only triggered for annotated tagging (-a or -s).

Signed-off-by: Mike Hommey <mh@glandium.org>
---
The write_annotation function could be made more generic and be used in
various different coming builtins such as git-commit. Also, it could be
used in show_reference() in builtin-tag.c.

 builtin-tag.c |   47 ++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 44 insertions(+), 3 deletions(-)

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)
+{
+	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)) {
+		free(buf);
+		return;
+	}
+	/* skip header */
+	while (sp + 1 < buf + size &&
+			!(sp[0] == '\n' && sp[1] == '\n'))
+		sp++;
+	/* 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);
+}
+
 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)
 {
 	enum object_type type;
 	char header_buf[1024];
@@ -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));
 		close(fd);
 
@@ -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);
 
 	lock = lock_any_ref_for_update(ref, prev, 0);
 	if (!lock)
-- 
1.5.3.5

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

[Johannes Schindelin]

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

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

[Mike Hommey]

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

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

[Johannes Schindelin]

Hi,

On Sat, 3 Nov 2007, Mike Hommey wrote:

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

Typo.  I meant the parens, and my fingers typed parents. D'oh.

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

I agree for the "\n\n"; this was my mistake (IOW it should be fixed both 
in show_reference() as well as in your code).

But for the signature, show_reference() _has_ to go line by line, because 
the user is allowed to specify a maximal line count.  This does not apply 
for your function.

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

Hehe.  This is what I really like about git's mailing list: it is a place 
where you learn something new every day.

Ciao,
Dscho

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

[Mike Hommey]

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

By the way, I think it would be much better if this function was made
more generic and would not write, but return an strbuf containing the
object body. It could also be used by e.g. git-commit --amend.

What would be the best suited place for such a function ?

Mike

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

[Johannes Schindelin]

Hi,

On Sat, 3 Nov 2007, Mike Hommey wrote:

> On Sat, Nov 03, 2007 at 11:54:38AM +0000, Johannes Schindelin wrote:
> > 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.
> 
> By the way, I think it would be much better if this function was made
> more generic and would not write, but return an strbuf containing the
> object body. It could also be used by e.g. git-commit --amend.
> 
> What would be the best suited place for such a function ?

editor.c, I'd say.

Ciao,
Dscho

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

[Mike Hommey]

When forcing to overwrite an annotated tag, there are good chances one
wants to keep the old annotation, or modify it, not start from scratch.

This is obviously only triggered for annotated tagging (-a or -s).

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 builtin-tag.c |   41 ++++++++++++++++++++++++++++++++++++++---
 1 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 66e5a58..e57f57f 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -247,9 +247,40 @@ static int git_tag_config(const char *var, const char *value)
 	return git_default_config(var, value);
 }
 
+static void write_tag_body(int fd, const unsigned char *sha1)
+{
+	unsigned long size;
+	enum object_type type;
+	char *buf, *sp, *eob;
+	size_t len;
+
+	if (is_null_sha1(sha1))
+		return;
+
+	sp = buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return;
+	/* skip header */
+	sp = strstr(buf, "\n\n");
+
+	if (!sp || !size || type != OBJ_TAG) {
+		free(buf);
+		return;
+	}
+	sp += 2; /* skip the 2 CRs */
+	eob = strstr(sp, "\n" PGP_SIGNATURE "\n");
+	if (eob)
+		len = eob - sp;
+	else
+		len = buf + size - sp;
+	write_or_die(fd, sp, len);
+
+	free(buf);
+}
+
 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)
 {
 	enum object_type type;
 	char header_buf[1024];
@@ -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);
 
 		launch_editor(path, buf);
@@ -419,7 +454,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		die("tag '%s' already exists", tag);
 
 	if (annotate)
-		create_tag(object, tag, &buf, message, sign, object);
+		create_tag(object, tag, &buf, message, sign, prev, object);
 
 	lock = lock_any_ref_for_update(ref, prev, 0);
 	if (!lock)
-- 
1.5.3.5

[PATCH 2/2] Small code readability improvement in show_reference() in builtin-tag.c

[Mike Hommey]

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 builtin-tag.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index e57f57f..3ed5759 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -84,14 +84,13 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 		sp = buf = read_sha1_file(sha1, &type, &size);
 		if (!buf)
 			return 0;
-		if (!size) {
+		/* skip header */
+		sp = strstr(buf, "\n\n");
+
+		if (!sp || !size) {
 			free(buf);
 			return 0;
 		}
-		/* skip header */
-		while (sp + 1 < buf + size &&
-				!(sp[0] == '\n' && sp[1] == '\n'))
-			sp++;
 		/* only take up to "lines" lines, and strip the signature */
 		for (i = 0, sp += 2;
 				i < filter->lines && sp < buf + size &&
-- 
1.5.3.5

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

[Mike Hommey]

On Sat, Nov 03, 2007 at 12:36:36PM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Sat, 3 Nov 2007, Mike Hommey wrote:
> 
> > On Sat, Nov 03, 2007 at 11:54:38AM +0000, Johannes Schindelin wrote:
> > > 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.
> > 
> > By the way, I think it would be much better if this function was made
> > more generic and would not write, but return an strbuf containing the
> > object body. It could also be used by e.g. git-commit --amend.
> > 
> > What would be the best suited place for such a function ?
> 
> editor.c, I'd say.

On which topic is this ?

Mike

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

[Johannes Schindelin]

Hi,

On Sat, 3 Nov 2007, Mike Hommey wrote:

> On Sat, Nov 03, 2007 at 12:36:36PM +0000, Johannes Schindelin wrote:
> 
> > On Sat, 3 Nov 2007, Mike Hommey wrote:
> > 
> > > On Sat, Nov 03, 2007 at 11:54:38AM +0000, Johannes Schindelin wrote:
> > > > 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.
> > > 
> > > By the way, I think it would be much better if this function was 
> > > made more generic and would not write, but return an strbuf 
> > > containing the object body. It could also be used by e.g. git-commit 
> > > --amend.
> > > 
> > > What would be the best suited place for such a function ?
> > 
> > editor.c, I'd say.
> 
> On which topic is this ?

On none so far.  But the plan was to move some functions used by both 
builtin-tag and builtin-commit (such as launch_editor()) into the files 
editor.[ch].

Unfortunately, that plan has not been executed by anybody.  Yet.

Ciao,
Dscho

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

[Mike Hommey]

On Sat, Nov 03, 2007 at 01:22:44PM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Sat, 3 Nov 2007, Mike Hommey wrote:
> 
> > On Sat, Nov 03, 2007 at 12:36:36PM +0000, Johannes Schindelin wrote:
> > 
> > > On Sat, 3 Nov 2007, Mike Hommey wrote:
> > > 
> > > > On Sat, Nov 03, 2007 at 11:54:38AM +0000, Johannes Schindelin wrote:
> > > > > 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.
> > > > 
> > > > By the way, I think it would be much better if this function was 
> > > > made more generic and would not write, but return an strbuf 
> > > > containing the object body. It could also be used by e.g. git-commit 
> > > > --amend.
> > > > 
> > > > What would be the best suited place for such a function ?
> > > 
> > > editor.c, I'd say.
> > 
> > On which topic is this ?
> 
> On none so far.  But the plan was to move some functions used by both 
> builtin-tag and builtin-commit (such as launch_editor()) into the files 
> editor.[ch].
> 
> Unfortunately, that plan has not been executed by anybody.  Yet.

Anyways, I took a quick glance at builtin-commit.c on pu, and it doesn't
look like it would benefit from having a shared function to get the
commit body. So I'll just forget about this idea for now.

Mike

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

[Junio C Hamano]

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?
        

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

[Mike Hommey]

On Sat, Nov 03, 2007 at 11:47:44AM -0700, Junio C Hamano wrote:
(...)
> 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". ;-)

You got me here ;)

(...)
> 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.
 
Damn, I overlooked this, and since the test suite doesn't do anything
on that, that got through. Indeed either the test can be removed, since
write_tag_body does the is_null_sha1() test, or the is_null_sha1() test
can be moved here.

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

Well, it could be true if the text was more verbose than "Write a tag
message". Anyways, as the test is now, the text is not going to appear.
:(

I'll fix this and will try to enhance the test suite to catch these
problems.

Mike

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

[Mike Hommey]

When forcing to overwrite an annotated tag, there are good chances one
wants to keep the old annotation, or modify it, not start from scratch.

This is obviously only triggered for annotated tagging (-a or -s).

Signed-off-by: Mike Hommey <mh@glandium.org>
---

You are free to remove the else that makes the tag template not show up
in the editor, but I'm not convinced it is any useful. Maybe a new
template for this annotation reuse would be sensible.

 builtin-tag.c |   38 +++++++++++++++++++++++++++++++++++---
 1 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 66e5a58..566d123 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -247,9 +247,37 @@ static int git_tag_config(const char *var, const char *value)
 	return git_default_config(var, value);
 }
 
+static void write_tag_body(int fd, const unsigned char *sha1)
+{
+	unsigned long size;
+	enum object_type type;
+	char *buf, *sp, *eob;
+	size_t len;
+
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return;
+	/* skip header */
+	sp = strstr(buf, "\n\n");
+
+	if (!sp || !size || type != OBJ_TAG) {
+		free(buf);
+		return;
+	}
+	sp += 2; /* skip the 2 LFs */
+	eob = strstr(sp, "\n" PGP_SIGNATURE "\n");
+	if (eob)
+		len = eob - sp;
+	else
+		len = buf + size - sp;
+	write_or_die(fd, sp, len);
+
+	free(buf);
+}
+
 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)
 {
 	enum object_type type;
 	char header_buf[1024];
@@ -282,7 +310,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 (!is_null_sha1(prev))
+			write_tag_body(fd, prev);
+		else
+			write_or_die(fd, tag_template, strlen(tag_template));
 		close(fd);
 
 		launch_editor(path, buf);
@@ -419,7 +451,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		die("tag '%s' already exists", tag);
 
 	if (annotate)
-		create_tag(object, tag, &buf, message, sign, object);
+		create_tag(object, tag, &buf, message, sign, prev, object);
 
 	lock = lock_any_ref_for_update(ref, prev, 0);
 	if (!lock)
-- 
1.5.3.5

[PATCH 2/2] Add tests for git tag

[Mike Hommey]

These tests check whether git-tag properly sends a comment into the
editor, and whether it reuses previous annotation when overwriting
an existing tag.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 t/t7004-tag.sh |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 0d07bc3..096fe33 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1004,4 +1004,20 @@ test_expect_failure \
 	'verify signed tag fails when public key is not present' \
 	'git-tag -v signed-tag'
 
+test_expect_success \
+	'message in editor has initial comment' '
+	GIT_EDITOR=cat git tag -a initial-comment > actual || true &&
+	test $(sed -n "/^\(#\|\$\)/p" actual | wc -l) -gt 0
+'
+
+get_tag_header reuse $commit commit $time >expect
+echo "An annotation to be reused" >> expect
+test_expect_success \
+	'overwriting an annoted tag should use its previous body' '
+	git tag -a -m "An annotation to be reused" reuse &&
+	GIT_EDITOR=true git tag -f -a reuse &&
+	get_tag_msg reuse >actual &&
+	git diff expect actual
+'
+
 test_done
-- 
1.5.3.5

Re: [PATCH 2/2] Small code readability improvement in show_reference() in builtin-tag.c

[Mike Hommey]

I think this patch got lost with my bad behaviour of sending another
series of patch not including this one.

Cheers,

Mike

On Sat, Nov 03, 2007 at 02:08:05PM +0100, Mike Hommey wrote:
> 
> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---
>  builtin-tag.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin-tag.c b/builtin-tag.c
> index e57f57f..3ed5759 100644
> --- a/builtin-tag.c
> +++ b/builtin-tag.c
> @@ -84,14 +84,13 @@ static int show_reference(const char *refname, const unsigned char *sha1,
>  		sp = buf = read_sha1_file(sha1, &type, &size);
>  		if (!buf)
>  			return 0;
> -		if (!size) {
> +		/* skip header */
> +		sp = strstr(buf, "\n\n");
> +
> +		if (!sp || !size) {
>  			free(buf);
>  			return 0;
>  		}
> -		/* skip header */
> -		while (sp + 1 < buf + size &&
> -				!(sp[0] == '\n' && sp[1] == '\n'))
> -			sp++;
>  		/* only take up to "lines" lines, and strip the signature */
>  		for (i = 0, sp += 2;
>  				i < filter->lines && sp < buf + size &&
> -- 
> 1.5.3.5
> 
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html