threads / patch / 10607

patchReuse previous annotation when overwriting a tag

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

## tl;dr

16 messages between Nov 3, 2007 and Nov 7, 2007. Diffs are folded; open one to read it.

replies: 15people: 3as markdown or json

Mike Hommey· Nov 3, 2007, 09:31 UTC · lore

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(-)
Show changes to builtin-tag.c +44 −3
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
Johannes Schindelin· Nov 3, 2007, 11:54 UTC · re: Mike Hommey · lore

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

Hi,
On Sat, 3 Nov 2007, Mike Hommey wrote:
Show 9 quoted lines
> 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".
Show 11 quoted lines
> +{
> +	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.
Show 7 quoted lines
> +		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.

Show 12 quoted lines
> +	/* 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");'.

Show 6 quoted lines
> +}
> +
>  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.
Show 9 quoted lines
> @@ -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?
Show 21 quoted lines
> @@ -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

Mike Hommey· Nov 3, 2007, 12:10 UTC · re: Johannes Schindelin · lore

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

On Sat, Nov 03, 2007 at 11:54:38AM +0000, Johannes Schindelin wrote:
Show 13 quoted lines
> > +{
> > +	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.
Show 8 quoted lines
> > +}
> > +
> >  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.
Show 11 quoted lines
> > @@ -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
Johannes Schindelin· Nov 3, 2007, 12:23 UTC · re: Mike Hommey · lore

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

Hi,
On Sat, 3 Nov 2007, Mike Hommey wrote:
Show 16 quoted lines
> 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.
Show 9 quoted lines
> (...)
> > 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.

Show 6 quoted lines
> (...)
> > 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

Mike Hommey· Nov 3, 2007, 13:08 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to builtin-tag.c +38 −3
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
Mike Hommey· Nov 3, 2007, 13:08 UTC · re: Mike Hommey · lore

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

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 builtin-tag.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)
Show changes to builtin-tag.c +4 −5
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
Mike Hommey· Nov 7, 2007, 21:51 UTC · re: Mike Hommey · lore

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

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:
Show 36 quoted lines
> 
> 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
Junio C Hamano· Nov 3, 2007, 18:47 UTC · re: Mike Hommey · lore

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

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?
Mike Hommey· Nov 3, 2007, 19:55 UTC · re: Junio C Hamano · lore

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

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 ;)
(...)
Show 5 quoted lines
> 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
Mike Hommey· Nov 4, 2007, 00:11 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to builtin-tag.c +35 −3
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
Mike Hommey· Nov 4, 2007, 00:11 UTC · re: Mike Hommey · lore

[PATCH 2/2] Add tests for git tag

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(-)
Show changes to t/t7004-tag.sh +16 −0
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
Mike Hommey· Nov 3, 2007, 12:27 UTC · re: Johannes Schindelin · lore

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

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
Johannes Schindelin· Nov 3, 2007, 12:36 UTC · re: Mike Hommey · lore

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

Hi,
On Sat, 3 Nov 2007, Mike Hommey wrote:
Show 10 quoted lines
> 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

Mike Hommey· Nov 3, 2007, 13:10 UTC · re: Johannes Schindelin · lore

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

On Sat, Nov 03, 2007 at 12:36:36PM +0000, Johannes Schindelin wrote:
Show 16 quoted lines
> 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
Johannes Schindelin· Nov 3, 2007, 13:22 UTC · re: Mike Hommey · lore

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

Hi,
On Sat, 3 Nov 2007, Mike Hommey wrote:
Show 20 quoted lines
> 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

Mike Hommey· Nov 3, 2007, 13:59 UTC · re: Johannes Schindelin · lore

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

On Sat, Nov 03, 2007 at 01:22:44PM +0000, Johannes Schindelin wrote:
Show 30 quoted lines
> 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

← back to recent threads