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

Mike Hommey [off-list ref] 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/).

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?
        

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

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

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

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