threads / patch / 45054

patchtag: generate useful reflog message

Subject: [PATCH] tag: generate useful reflog message

## tl;dr

12 messages between Feb 5, 2017 and Feb 9, 2017. Diffs are folded; open one to read it.

replies: 11people: 2as markdown or json

cornelius.weig@tngtech.com· Feb 5, 2017, 21:42 UTC · lore
From: Cornelius Weig <cornelius.weig@tngtech.com>

When tags are created with `--create-reflog` or with the option `core.logAllRefUpdates` set to 'always', a reflog is created for them. So far, the description of reflog entries for tags was empty, making the reflog hard to understand. For example: "6e3a7b3 refs/tags/tag_with_reflog@{0}:"

Now, a reflog message is generated when creating a tag. The message follows the pattern "commit: <subject>" where the subject is taken from the commit the tag points to. For example: "6e3a7b3 refs/tags/tag_with_reflog@{0}: commit: Git 2.12-rc0" If the tag points to a tree/blob/tag object, the following static messages are used instead:

 - "tree object"
 - "blob object"
 - "other tag object"
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
Notes:
    While playing around with tag reflogs I also found a bug that was present
    before this patch. It manifests itself when the sha1-ref in the reflog does not
    point to a commit object but something else.
    
    For example,
    
     - when the referenced sha1 is a tag object:
    	$ git tag --create-reflog -f -m'annotated tag' tag_with_reflog
     - when the referenced sha1 is a blob object:
    	$ git tag --create-reflog -f tag_with_reflog HEAD:<filename>
     - when the referenced sha1 is a tree object:
    	$ git tag --create-reflog -f tag_with_reflog HEAD^{tree}
    
    In each case, a proper reflog entry is generated, but
    	$ git reflog tag_with_reflog
    will sometimes segfault (if it does, it does so consistently), or only show the
    first few entries. The tree/blob cases are IMHO not so important, but the
    broken reflog for annotated tags I find quite severe.
    
    I guess it's because the reflog is funneled through the log.c code, where every
    reflog-entry is assumed to be a commit object? If this is the case, a fix would
    probably be quite involved.
 builtin/tag.c  | 43 ++++++++++++++++++++++++++++++++++++++++++-
 t/t7004-tag.sh | 14 +++++++++++++-
 2 files changed, 55 insertions(+), 2 deletions(-)
Show changes to 2 files +55 −2

builtin/tag.c, t/t7004-tag.sh

diff --git a/builtin/tag.c b/builtin/tag.c
index e40c4a9..c0d9478 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -302,6 +302,43 @@ static void create_tag(const unsigned char *object, const char *tag,
 	}
 }
 
+static void create_reflog_msg(const unsigned char *object, struct strbuf *sb)
+{
+	enum object_type type;
+	char *buf;
+	unsigned long size;
+	int subject_len = 0;
+	const char *subject_start;
+
+	type = sha1_object_info(object, NULL);
+	switch (type) {
+	default:
+		strbuf_addstr(sb, "internal object");
+		break;
+	case OBJ_COMMIT:
+		strbuf_addstr(sb, "commit: ");
+		buf = read_sha1_file(object, &type, &size);
+		if (buf) {
+			subject_len = find_commit_subject(buf, &subject_start);
+			strbuf_insert(sb, 8, subject_start, subject_len);
+			free(buf);
+		} else {
+			die("commit object %s could not be read",
+				sha1_to_hex(repl));
+		}
+		break;
+	case OBJ_TREE:
+		strbuf_addstr(sb, "tree object");
+		break;
+	case OBJ_BLOB:
+		strbuf_addstr(sb, "blob object");
+		break;
+	case OBJ_TAG:
+		strbuf_addstr(sb, "other tag object");
+		break;
+	}
+}
+
 struct msg_arg {
 	int given;
 	struct strbuf buf;
@@ -335,6 +372,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf ref = STRBUF_INIT;
+	struct strbuf reflog_msg = STRBUF_INIT;
 	unsigned char object[20], prev[20];
 	const char *object_ref, *tag;
 	struct create_tag_options opt;
@@ -494,6 +532,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	else
 		die(_("Invalid cleanup mode %s"), cleanup_arg);
 
+	create_reflog_msg(object, &reflog_msg);
+
 	if (create_tag_object) {
 		if (force_sign_annotate && !annotate)
 			opt.sign = 1;
@@ -504,7 +544,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, object, prev,
 				   create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
-				   NULL, &err) ||
+				   reflog_msg.buf, &err) ||
 	    ref_transaction_commit(transaction, &err))
 		die("%s", err.buf);
 	ref_transaction_free(transaction);
@@ -514,5 +554,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	strbuf_release(&err);
 	strbuf_release(&buf);
 	strbuf_release(&ref);
+	strbuf_release(&reflog_msg);
 	return 0;
 }
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 072e6c6..0a92b2c 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -83,7 +83,19 @@ test_expect_success 'creating a tag using default HEAD should succeed' '
 test_expect_success 'creating a tag with --create-reflog should create reflog' '
 	test_when_finished "git tag -d tag_with_reflog" &&
 	git tag --create-reflog tag_with_reflog &&
-	git reflog exists refs/tags/tag_with_reflog
+	git reflog exists refs/tags/tag_with_reflog &&
+	git log -1 --format="format:commit: %s%n" > expected &&
+	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'annotated tag with --create-reflog has correct message' '
+	test_when_finished "git tag -d tag_with_reflog" &&
+	git tag -m "annotated tag" --create-reflog tag_with_reflog &&
+	git reflog exists refs/tags/tag_with_reflog &&
+	git log -1 --format="format:commit: %s%n" > expected &&
+	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
 '
 
 test_expect_success '--create-reflog does not create reflog on failure' '
-- 
2.10.2
cornelius.weig@tngtech.com· Feb 5, 2017, 23:18 UTC · re: cornelius.weig@tngtech.com · lore

[PATCH v2] tag: generate useful reflog message

From: Cornelius Weig <cornelius.weig@tngtech.com>

When tags are created with `--create-reflog` or with the option `core.logAllRefUpdates` set to 'always', a reflog is created for them. So far, the description of reflog entries for tags was empty, making the reflog hard to understand. For example: "6e3a7b3 refs/tags/test@{0}:"

Now, a reflog message is generated when creating a tag, following the pattern "<action>: <description>". Here, action is the command line with all arguments, or the value of GIT_REFLOG_ACTION if it is set. The description is the commit subject, if the tag points to a commit. For example: "6e3a7b3 refs/tags/test@{0}: tag --create-reflog test: Git 2.12-rc0" If the tag points to a tree/blob/tag object, the following static strings are taken as description:

 - "tree object"
 - "blob object"
 - "other tag object"
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
Notes:
    *This patch supersedes the version submitted a few hours earlier.*
    
    Sorry for the messup, but I realized that the pattern for the reflog message
    from my first draft did not comply to standard git behavior.
    
    Please also note my remarks from v1 (repeated here):
    
    While playing around with tag reflogs I also found a bug that was present
    before this patch. It manifests itself when the sha1-ref in the reflog does not
    point to a commit object but something else.
    
    For example,
    
     - when the referenced sha1 is a tag object:
    	$ git tag --create-reflog -f -m'annotated tag' tag_with_reflog
     - when the referenced sha1 is a blob object:
    	$ git tag --create-reflog -f tag_with_reflog HEAD:<filename>
     - when the referenced sha1 is a tree object:
    	$ git tag --create-reflog -f tag_with_reflog HEAD^{tree}
    
    In each case, a proper reflog entry is generated, but
    	$ git reflog tag_with_reflog
    will sometimes segfault (if it does, it does so consistently), or only show the
    first few entries. The tree/blob cases are IMHO not so important, but the
    broken reflog for annotated tags I find quite severe.
    
    I guess it's because the reflog is funneled through the log.c code, where every
    reflog-entry is assumed to be a commit object? If this is the case, a fix would
    probably be quite involved.
 builtin/tag.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 t/t7004-tag.sh | 16 +++++++++++++++-
 2 files changed, 69 insertions(+), 3 deletions(-)
Show changes to 2 files +69 −3

builtin/tag.c, t/t7004-tag.sh

diff --git a/builtin/tag.c b/builtin/tag.c
index e40c4a9..3d9e105 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -30,6 +30,7 @@ static const char * const git_tag_usage[] = {
 
 static unsigned int colopts;
 static int force_sign_annotate;
+static struct strbuf default_rla = STRBUF_INIT;
 
 static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
@@ -302,6 +303,48 @@ static void create_tag(const unsigned char *object, const char *tag,
 	}
 }
 
+static void create_reflog_msg(const unsigned char *object, struct strbuf *sb)
+{
+	enum object_type type;
+	char *buf;
+	unsigned long size;
+	int subject_len = 0;
+	const char *subject_start;
+
+	char *rla = getenv("GIT_REFLOG_ACTION");
+	if (!rla)
+		rla = default_rla.buf;
+
+	strbuf_addf(sb, "%s: ", rla ? rla : default_rla.buf);
+
+	type = sha1_object_info(object, NULL);
+	switch (type) {
+	default:
+		strbuf_addstr(sb, "internal object");
+		break;
+	case OBJ_COMMIT:
+		buf = read_sha1_file(object, &type, &size);
+		if (buf) {
+			subject_len = find_commit_subject(buf, &subject_start);
+			strbuf_insert(sb, sb->len, subject_start, subject_len);
+			free(buf);
+		} else {
+			die("commit object %s could not be read",
+				sha1_to_hex(object));
+		}
+		break;
+	case OBJ_TREE:
+		strbuf_addstr(sb, "tree object");
+		break;
+	case OBJ_BLOB:
+		strbuf_addstr(sb, "blob object");
+		break;
+	case OBJ_TAG:
+		strbuf_addstr(sb, "other tag object");
+		break;
+	}
+}
+
 struct msg_arg {
 	int given;
 	struct strbuf buf;
@@ -335,6 +378,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf ref = STRBUF_INIT;
+	struct strbuf reflog_msg = STRBUF_INIT;
 	unsigned char object[20], prev[20];
 	const char *object_ref, *tag;
 	struct create_tag_options opt;
@@ -349,7 +393,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct ref_filter filter;
 	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 	const char *format = NULL;
-	int icase = 0;
+	int icase = 0, i;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
@@ -391,6 +435,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	git_config(git_tag_config, sorting_tail);
 
+	/* Record the command line for the reflog */
+	strbuf_addstr(&default_rla, "tag");
+	for (i = 1; i < argc; i++)
+		strbuf_addf(&default_rla, " %s", argv[i]);
+
 	memset(&opt, 0, sizeof(opt));
 	memset(&filter, 0, sizeof(filter));
 	filter.lines = -1;
@@ -494,6 +543,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	else
 		die(_("Invalid cleanup mode %s"), cleanup_arg);
 
+	create_reflog_msg(object, &reflog_msg);
+
 	if (create_tag_object) {
 		if (force_sign_annotate && !annotate)
 			opt.sign = 1;
@@ -504,7 +555,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, object, prev,
 				   create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
-				   NULL, &err) ||
+				   reflog_msg.buf, &err) ||
 	    ref_transaction_commit(transaction, &err))
 		die("%s", err.buf);
 	ref_transaction_free(transaction);
@@ -514,5 +565,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	strbuf_release(&err);
 	strbuf_release(&buf);
 	strbuf_release(&ref);
+	strbuf_release(&reflog_msg);
 	return 0;
 }
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 072e6c6..9c80bc9 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -80,10 +80,24 @@ test_expect_success 'creating a tag using default HEAD should succeed' '
 	test_must_fail git reflog exists refs/tags/mytag
 '
 
+git log -1 > expected \
+	--format="format:tag --create-reflog tag_with_reflog: %s%n"
 test_expect_success 'creating a tag with --create-reflog should create reflog' '
 	test_when_finished "git tag -d tag_with_reflog" &&
 	git tag --create-reflog tag_with_reflog &&
-	git reflog exists refs/tags/tag_with_reflog
+	git reflog exists refs/tags/tag_with_reflog &&
+	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
+'
+
+git log -1 > expected \
+	--format='format:tag -m annotated tag --create-reflog tag_with_reflog: %s%n'
+test_expect_success 'annotated tag with --create-reflog has correct message' '
+	test_when_finished "git tag -d tag_with_reflog" &&
+	git tag -m "annotated tag" --create-reflog tag_with_reflog &&
+	git reflog exists refs/tags/tag_with_reflog &&
+	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
 '
 
 test_expect_success '--create-reflog does not create reflog on failure' '
-- 
2.10.2
Junio C Hamano· Feb 5, 2017, 23:25 UTC · re: cornelius.weig@tngtech.com · lore

Re: [PATCH] tag: generate useful reflog message

cornelius.weig@tngtech.com writes:
> Now, a reflog message is generated when creating a tag. The message
> follows the pattern "commit: <subject>" where the subject is taken from
> the commit the tag points to. For example:
> "6e3a7b3 refs/tags/tag_with_reflog@{0}: commit: Git 2.12-rc0"

Because the reflog records the actions, shouldn't it be saying that you "tagged"? The reflog for HEAD says things like "reset: moving to...", "am: $subject", and the reflog for a branch says things like "branch: created from master", "am: $subject", "rebase -i (finish)".

For a tag, I would imagine something like "tag: tagged 4e59582ff7 ("Seventh batch for 2.12", 2017-01-23)" would be more appropriate.

> Notes:
>     While playing around with tag reflogs I also found a bug that was present
>     before this patch. It manifests itself when the sha1-ref in the reflog does not
>     point to a commit object but something else.

The underlying machinery for "log" and "rev-list" is about showing a stream of commits, and most of the reflog entries point at commits.

On the other hand, the "walking reflogs to and show the sequence of the tip of refs", and there is no reason to expect the tip of refs will always be commits, but an ancient design mistake bolted the latter on top of the former (perhaps because in practice the tip of refs are almost always commits); "reflog" aka "log -g" and "rev-list --walk-reflogs" share the same issue coming from that misdesign, which needs to be corrected to solve this issue.

The exact same design mistake also makes "git reflog" to accept options like "--topo-order", even though many of the options that make sense for the "commit DAG walking" (which is what "log" and "rev-list" are about) do not make any sense when walking a reflog. And the command would give nonsense output when given such an option, because a reflog is a single strand of pearl of objects (not necessarily commits) and the order in which these objects appear in the reflog does not have anything to do with the underlying commit DAG topology. Fixing the ancient misdesign would fix this issue, too.

I think the fix would involve first ripping out the "reflog walking" code that was bolted on and stop allowing it to inject the entries taken from the reflog into the "walk the commit DAG" machinery. Then "reflog walking" code needs to be taught to have its own "now we got a single object to show, show it (using the helper functions to show a single object that is already used by 'git show')" code, instead of piggy-backing on the output codepath used by "log" and "rev-list".

cornelius.weig@tngtech.com· Feb 6, 2017, 13:58 UTC · re: Junio C Hamano · lore

[PATCH v3] tag: generate useful reflog message

From: Cornelius Weig <cornelius.weig@tngtech.com>

When tags are created with `--create-reflog` or with the option `core.logAllRefUpdates` set to 'always', a reflog is created for them. So far, the description of reflog entries for tags was empty, making the reflog hard to understand. For example: 6e3a7b3 refs/tags/test@{0}:

Now, a reflog message is generated when creating a tag, following the pattern "tag: tagging <short-sha1> (<description>)". If GIT_REFLOG_ACTION is set, the message becomes "$GIT_REFLOG_ACTION (<description>)" instead. If the tag references a commit object, the description is set to the subject line of the commit, followed by its commit date. For example: 6e3a7b3 refs/tags/test@{0}: tag: tagging 6e3a7b3398 (Git 2.12-rc0, 2017-02-03)

If the tag points to a tree/blob/tag objects, the following static strings are taken as description:

 - "tree object"
 - "blob object"
 - "other tag object"
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
 builtin/tag.c  | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 t/t7004-tag.sh | 16 +++++++++++++++-
 2 files changed, 68 insertions(+), 2 deletions(-)
Show changes to 2 files +68 −2

builtin/tag.c, t/t7004-tag.sh

diff --git a/builtin/tag.c b/builtin/tag.c
index e40c4a9..638b68e 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -302,6 +302,54 @@ static void create_tag(const unsigned char *object, const char *tag,
 	}
 }
 
+static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
+{
+	enum object_type type;
+	struct commit *c;
+	char *buf;
+	unsigned long size;
+	int subject_len = 0;
+	const char *subject_start;
+
+	char *rla = getenv("GIT_REFLOG_ACTION");
+	if (rla) {
+		strbuf_addstr(sb, rla);
+	} else {
+		strbuf_addstr(sb, _("tag: tagging "));
+		strbuf_add_unique_abbrev(sb, sha1, DEFAULT_ABBREV);
+	}
+
+	strbuf_addstr(sb, " (");
+	type = sha1_object_info(sha1, NULL);
+	switch (type) {
+	default:
+		strbuf_addstr(sb, _("internal object"));
+		break;
+	case OBJ_COMMIT:
+		c = lookup_commit_reference(sha1);
+		buf = read_sha1_file(sha1, &type, &size);
+		if (!c || !buf) {
+			die(_("commit object %s could not be read"),
+				sha1_to_hex(sha1));
+		}
+		subject_len = find_commit_subject(buf, &subject_start);
+		strbuf_insert(sb, sb->len, subject_start, subject_len);
+		strbuf_addf(sb, ", %s", show_date(c->date, 0, DATE_MODE(SHORT)));
+		free(buf);
+		break;
+	case OBJ_TREE:
+		strbuf_addstr(sb, _("tree object"));
+		break;
+	case OBJ_BLOB:
+		strbuf_addstr(sb, _("blob object"));
+		break;
+	case OBJ_TAG:
+		strbuf_addstr(sb, _("other tag object"));
+		break;
+	}
+	strbuf_addch(sb, ')');
+}
+
 struct msg_arg {
 	int given;
 	struct strbuf buf;
@@ -335,6 +383,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf ref = STRBUF_INIT;
+	struct strbuf reflog_msg = STRBUF_INIT;
 	unsigned char object[20], prev[20];
 	const char *object_ref, *tag;
 	struct create_tag_options opt;
@@ -494,6 +543,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	else
 		die(_("Invalid cleanup mode %s"), cleanup_arg);
 
+	create_reflog_msg(object, &reflog_msg);
+
 	if (create_tag_object) {
 		if (force_sign_annotate && !annotate)
 			opt.sign = 1;
@@ -504,7 +555,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, object, prev,
 				   create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
-				   NULL, &err) ||
+				   reflog_msg.buf, &err) ||
 	    ref_transaction_commit(transaction, &err))
 		die("%s", err.buf);
 	ref_transaction_free(transaction);
@@ -514,5 +565,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	strbuf_release(&err);
 	strbuf_release(&buf);
 	strbuf_release(&ref);
+	strbuf_release(&reflog_msg);
 	return 0;
 }
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 072e6c6..3c4cb58 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -80,10 +80,24 @@ test_expect_success 'creating a tag using default HEAD should succeed' '
 	test_must_fail git reflog exists refs/tags/mytag
 '
 
+git log -1 > expected \
+	--format="format:tag: tagging %h (%s, %cd)%n" --date=format:%F
 test_expect_success 'creating a tag with --create-reflog should create reflog' '
 	test_when_finished "git tag -d tag_with_reflog" &&
 	git tag --create-reflog tag_with_reflog &&
-	git reflog exists refs/tags/tag_with_reflog
+	git reflog exists refs/tags/tag_with_reflog &&
+	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
+'
+
+git log -1 > expected \
+	--format="format:tag: tagging %h (%s, %cd)%n" --date=format:%F
+test_expect_success 'annotated tag with --create-reflog has correct message' '
+	test_when_finished "git tag -d tag_with_reflog" &&
+	git tag -m "annotated tag" --create-reflog tag_with_reflog &&
+	git reflog exists refs/tags/tag_with_reflog &&
+	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
 '
 
 test_expect_success '--create-reflog does not create reflog on failure' '
-- 
2.10.2
Junio C Hamano· Feb 6, 2017, 19:32 UTC · re: cornelius.weig@tngtech.com · lore

Re: [PATCH v3] tag: generate useful reflog message

cornelius.weig@tngtech.com writes:
Show 6 quoted lines
> +	strbuf_addstr(sb, " (");
> +	type = sha1_object_info(sha1, NULL);
> +	switch (type) {
> +	default:
> +		strbuf_addstr(sb, _("internal object"));
> +		break;

The code does not even know if this is an "internal" object, does it? What it got was simply an object of an unknown type that it is not prepared to handle. It's not like you are trying to die() in this function (I see a die() upon failing to read the referent commit), so I wonder if this should be a die("BUG").

On the other hand, it's not like failing to describe the tagged commit in the reflog is such a grave error. If we can get away with being vague on a tag that points at an object of unknown type like the above code does, we could loosen the "oops, we thought we got a commit, but it turns out that we cannot read it" case below from die() to just stuffing generic _("commit object") in the reflog.

Between the two extremes above, I am leaning towards making it more lenient myself, but others may have different opinions.

Show 16 quoted lines
> +	case OBJ_COMMIT:
> +		c = lookup_commit_reference(sha1);
> +		buf = read_sha1_file(sha1, &type, &size);
> +		if (!c || !buf) {
> +			die(_("commit object %s could not be read"),
> +				sha1_to_hex(sha1));
> +		}
> +		subject_len = find_commit_subject(buf, &subject_start);
> +		strbuf_insert(sb, sb->len, subject_start, subject_len);
> +		strbuf_addf(sb, ", %s", show_date(c->date, 0, DATE_MODE(SHORT)));
> +		free(buf);
> +		break;
> +	case OBJ_TREE:
> +		strbuf_addstr(sb, _("tree object"));
> +		break;
> ...
Show 8 quoted lines
> +git log -1 > expected \
> +	--format="format:tag: tagging %h (%s, %cd)%n" --date=format:%F
>  test_expect_success 'creating a tag with --create-reflog should create reflog' '
>  	test_when_finished "git tag -d tag_with_reflog" &&
>  	git tag --create-reflog tag_with_reflog &&
> -	git reflog exists refs/tags/tag_with_reflog
> +	git reflog exists refs/tags/tag_with_reflog &&
> +	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
I'd spell that "\t" with an actual HT to make it portable [*1*].  

We have one example that uses the form in git-filter-branch documentation and a script in the contrib/ area, but otherwise do not have anything that relies on \t to be turned into HT by sed.

Show 10 quoted lines
> +	test_cmp expected actual
> +'
> +
> +git log -1 > expected \
> +	--format="format:tag: tagging %h (%s, %cd)%n" --date=format:%F
> +test_expect_success 'annotated tag with --create-reflog has correct message' '
> +	test_when_finished "git tag -d tag_with_reflog" &&
> +	git tag -m "annotated tag" --create-reflog tag_with_reflog &&
> +	git reflog exists refs/tags/tag_with_reflog &&
> +	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
Likewise.
[Reference]
*1* http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap09.html#tag_09_03
    9.3.2 BRE Ordinary Characters
    An ordinary character is a BRE that matches itself: any character in
    the supported character set, except for the BRE special characters
    listed in BRE Special Characters.
    The interpretation of an ordinary character preceded by an unescaped
    <backslash> ( '\\' ) is undefined, except for:
    - The characters ')', '(', '{', and '}'
    - The digits 1 to 9 inclusive (see BREs Matching Multiple Characters)
    - A character inside a bracket expression
cornelius.weig@tngtech.com· Feb 6, 2017, 22:24 UTC · re: Junio C Hamano · lore

[PATCH v4] tag: generate useful reflog message

From: Cornelius Weig <cornelius.weig@tngtech.com>
Thanks for taking a look at my last version.
Show 6 quoted lines
> On the other hand, it's not like failing to describe the tagged
> commit in the reflog is such a grave error.  If we can get away with
> being vague on a tag that points at an object of unknown type like
> the above code does, we could loosen the "oops, we thought we got a
> commit, but it turns out that we cannot read it" case below from
> die() to just stuffing generic _("commit object") in the reflog.

Good point. I agree that failing to create the message should be no reason to die(). As you also pointed out, "internal object" is no reliable description for unhandled object types. I changed that as well.

Changes wrt v3 (interdiff below):
 - change default message for unhandled object types
 - do not die if commit is not readable, but use default description instead
 - test: use verbatim HT character instead of \t
Cornelius Weig (1):
  tag: generate useful reflog message
 builtin/tag.c  | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 t/t7004-tag.sh | 16 +++++++++++++++-
 2 files changed, 68 insertions(+), 2 deletions(-)
Interdiff v3..v4:
Show changes to 2 files +11 −11

builtin/tag.c, t/t7004-tag.sh

diff --git a/builtin/tag.c b/builtin/tag.c
index 638b68e..9b2eabd 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -323,19 +323,19 @@ static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
 	type = sha1_object_info(sha1, NULL);
 	switch (type) {
 	default:
-		strbuf_addstr(sb, _("internal object"));
+		strbuf_addstr(sb, _("object of unknown type"));
 		break;
 	case OBJ_COMMIT:
-		c = lookup_commit_reference(sha1);
-		buf = read_sha1_file(sha1, &type, &size);
-		if (!c || !buf) {
-			die(_("commit object %s could not be read"),
-				sha1_to_hex(sha1));
+		if ((buf = read_sha1_file(sha1, &type, &size)) != NULL) {
+			subject_len = find_commit_subject(buf, &subject_start);
+			strbuf_insert(sb, sb->len, subject_start, subject_len);
+		} else {
+			strbuf_addstr(sb, _("commit object"));
 		}
-		subject_len = find_commit_subject(buf, &subject_start);
-		strbuf_insert(sb, sb->len, subject_start, subject_len);
-		strbuf_addf(sb, ", %s", show_date(c->date, 0, DATE_MODE(SHORT)));
 		free(buf);
+
+		if ((c = lookup_commit_reference(sha1)) != NULL)
+			strbuf_addf(sb, ", %s", show_date(c->date, 0, DATE_MODE(SHORT)));
 		break;
 	case OBJ_TREE:
 		strbuf_addstr(sb, _("tree object"));
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 3c4cb58..894959f 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -86,7 +86,7 @@ test_expect_success 'creating a tag with --create-reflog should create reflog' '
 	test_when_finished "git tag -d tag_with_reflog" &&
 	git tag --create-reflog tag_with_reflog &&
 	git reflog exists refs/tags/tag_with_reflog &&
-	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	sed -e "s/^.*	//" .git/logs/refs/tags/tag_with_reflog > actual &&
 	test_cmp expected actual
 '

@@ -96,7 +96,7 @@ test_expect_success 'annotated tag with --create-reflog has correct message' '
 	test_when_finished "git tag -d tag_with_reflog" &&
 	git tag -m "annotated tag" --create-reflog tag_with_reflog &&
 	git reflog exists refs/tags/tag_with_reflog &&
-	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	sed -e "s/^.*	//" .git/logs/refs/tags/tag_with_reflog > actual &&
 	test_cmp expected actual
 '
-- 
2.10.2
cornelius.weig@tngtech.com· Feb 6, 2017, 22:24 UTC · re: cornelius.weig@tngtech.com · lore

[PATCH v4] tag: generate useful reflog message

From: Cornelius Weig <cornelius.weig@tngtech.com>

When tags are created with `--create-reflog` or with the option `core.logAllRefUpdates` set to 'always', a reflog is created for them. So far, the description of reflog entries for tags was empty, making the reflog hard to understand. For example: 6e3a7b3 refs/tags/test@{0}:

Now, a reflog message is generated when creating a tag, following the pattern "tag: tagging <short-sha1> (<description>)". If GIT_REFLOG_ACTION is set, the message becomes "$GIT_REFLOG_ACTION (<description>)" instead. If the tag references a commit object, the description is set to the subject line of the commit, followed by its commit date. For example: 6e3a7b3 refs/tags/test@{0}: tag: tagging 6e3a7b3398 (Git 2.12-rc0, 2017-02-03)

If the tag points to a tree/blob/tag objects, the following static strings are taken as description:

 - "tree object"
 - "blob object"
 - "other tag object"
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
Reviewed-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/tag.c  | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 t/t7004-tag.sh | 16 +++++++++++++++-
 2 files changed, 68 insertions(+), 2 deletions(-)
Show changes to 2 files +68 −2

builtin/tag.c, t/t7004-tag.sh

diff --git a/builtin/tag.c b/builtin/tag.c
index e40c4a9..bca890f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -302,6 +302,54 @@ static void create_tag(const unsigned char *object, const char *tag,
 	}
 }
 
+static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
+{
+	enum object_type type;
+	struct commit *c;
+	char *buf;
+	unsigned long size;
+	int subject_len = 0;
+	const char *subject_start;
+
+	char *rla = getenv("GIT_REFLOG_ACTION");
+	if (rla) {
+		strbuf_addstr(sb, rla);
+	} else {
+		strbuf_addstr(sb, _("tag: tagging "));
+		strbuf_add_unique_abbrev(sb, sha1, DEFAULT_ABBREV);
+	}
+
+	strbuf_addstr(sb, " (");
+	type = sha1_object_info(sha1, NULL);
+	switch (type) {
+	default:
+		strbuf_addstr(sb, _("object of unknown type"));
+		break;
+	case OBJ_COMMIT:
+		if ((buf = read_sha1_file(sha1, &type, &size)) != NULL) {
+			subject_len = find_commit_subject(buf, &subject_start);
+			strbuf_insert(sb, sb->len, subject_start, subject_len);
+		} else {
+			strbuf_addstr(sb, _("commit object"));
+		}
+		free(buf);
+
+		if ((c = lookup_commit_reference(sha1)) != NULL)
+			strbuf_addf(sb, ", %s", show_date(c->date, 0, DATE_MODE(SHORT)));
+		break;
+	case OBJ_TREE:
+		strbuf_addstr(sb, _("tree object"));
+		break;
+	case OBJ_BLOB:
+		strbuf_addstr(sb, _("blob object"));
+		break;
+	case OBJ_TAG:
+		strbuf_addstr(sb, _("other tag object"));
+		break;
+	}
+	strbuf_addch(sb, ')');
+}
+
 struct msg_arg {
 	int given;
 	struct strbuf buf;
@@ -335,6 +383,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf ref = STRBUF_INIT;
+	struct strbuf reflog_msg = STRBUF_INIT;
 	unsigned char object[20], prev[20];
 	const char *object_ref, *tag;
 	struct create_tag_options opt;
@@ -494,6 +543,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	else
 		die(_("Invalid cleanup mode %s"), cleanup_arg);
 
+	create_reflog_msg(object, &reflog_msg);
+
 	if (create_tag_object) {
 		if (force_sign_annotate && !annotate)
 			opt.sign = 1;
@@ -504,7 +555,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, object, prev,
 				   create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
-				   NULL, &err) ||
+				   reflog_msg.buf, &err) ||
 	    ref_transaction_commit(transaction, &err))
 		die("%s", err.buf);
 	ref_transaction_free(transaction);
@@ -514,5 +565,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	strbuf_release(&err);
 	strbuf_release(&buf);
 	strbuf_release(&ref);
+	strbuf_release(&reflog_msg);
 	return 0;
 }
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 072e6c6..894959f 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -80,10 +80,24 @@ test_expect_success 'creating a tag using default HEAD should succeed' '
 	test_must_fail git reflog exists refs/tags/mytag
 '
 
+git log -1 > expected \
+	--format="format:tag: tagging %h (%s, %cd)%n" --date=format:%F
 test_expect_success 'creating a tag with --create-reflog should create reflog' '
 	test_when_finished "git tag -d tag_with_reflog" &&
 	git tag --create-reflog tag_with_reflog &&
-	git reflog exists refs/tags/tag_with_reflog
+	git reflog exists refs/tags/tag_with_reflog &&
+	sed -e "s/^.*	//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
+'
+
+git log -1 > expected \
+	--format="format:tag: tagging %h (%s, %cd)%n" --date=format:%F
+test_expect_success 'annotated tag with --create-reflog has correct message' '
+	test_when_finished "git tag -d tag_with_reflog" &&
+	git tag -m "annotated tag" --create-reflog tag_with_reflog &&
+	git reflog exists refs/tags/tag_with_reflog &&
+	sed -e "s/^.*	//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
 '
 
 test_expect_success '--create-reflog does not create reflog on failure' '
-- 
2.10.2
Junio C Hamano· Feb 8, 2017, 21:28 UTC · re: cornelius.weig@tngtech.com · lore

Re: [PATCH v4] tag: generate useful reflog message

cornelius.weig@tngtech.com writes:
Show 25 quoted lines
> From: Cornelius Weig <cornelius.weig@tngtech.com>
>
> When tags are created with `--create-reflog` or with the option
> `core.logAllRefUpdates` set to 'always', a reflog is created for them.
> So far, the description of reflog entries for tags was empty, making the
> reflog hard to understand. For example:
> 6e3a7b3 refs/tags/test@{0}:
>
> Now, a reflog message is generated when creating a tag, following the
> pattern "tag: tagging <short-sha1> (<description>)". If
> GIT_REFLOG_ACTION is set, the message becomes "$GIT_REFLOG_ACTION
> (<description>)" instead. If the tag references a commit object, the
> description is set to the subject line of the commit, followed by its
> commit date. For example:
> 6e3a7b3 refs/tags/test@{0}: tag: tagging 6e3a7b3398 (Git 2.12-rc0, 2017-02-03)
>
> If the tag points to a tree/blob/tag objects, the following static
> strings are taken as description:
>
>  - "tree object"
>  - "blob object"
>  - "other tag object"
>
> Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
> Reviewed-by: Junio C Hamano <gitster@pobox.com>

This last line is inappropriate, as I didn't review _THIS_ version, which is different from the previous one, and I haven't checked if the way the comments on the previous review were addressed in this version is agreeable.

Show 10 quoted lines
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 072e6c6..894959f 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -80,10 +80,24 @@ test_expect_success 'creating a tag using default HEAD should succeed' '
>  	test_must_fail git reflog exists refs/tags/mytag
>  '
>  
> +git log -1 > expected \
> +	--format="format:tag: tagging %h (%s, %cd)%n" --date=format:%F

We do not want to do this kind of thing outside the test_expect_success immediately below, unless there is a good reason, and in this case I do not see any.

Also write redirection operator and redirection target pathname without SP in between.

Show 8 quoted lines
>  test_expect_success 'creating a tag with --create-reflog should create reflog' '
>  	test_when_finished "git tag -d tag_with_reflog" &&
>  	git tag --create-reflog tag_with_reflog &&
> -	git reflog exists refs/tags/tag_with_reflog
> +	git reflog exists refs/tags/tag_with_reflog &&
> +	sed -e "s/^.*	//" .git/logs/refs/tags/tag_with_reflog > actual &&
> +	test_cmp expected actual
> +'
In other words, something like:
test_expect_success 'creating a tag with --create-reflog should create reflog' '
	git log -1 \
		--format="format:tag: tagging %h (%s, %cd)%n" \
		--date=format:%Y-%m-%d >expected &&
	test_when_finished "git tag -d tag_with_reflog" &&
	git tag --create-reflog tag_with_reflog &&
	git reflog exists refs/tags/tag_with_reflog &&
	sed -e "s/^.*	//" .git/logs/refs/tags/tag_with_reflog >actual &&
	test_cmp expected actual
'	

Even though %F may be shorter, spelling it out makes what we expect more explicit, and what is what I did in the above example.

Thanks.
Cornelius Weig· Feb 8, 2017, 22:28 UTC · re: Junio C Hamano · lore

Re: [PATCH v4] tag: generate useful reflog message

On 02/08/2017 10:28 PM, Junio C Hamano wrote:
Show 32 quoted lines
> cornelius.weig@tngtech.com writes:
> 
>> From: Cornelius Weig <cornelius.weig@tngtech.com>
>>
>> When tags are created with `--create-reflog` or with the option
>> `core.logAllRefUpdates` set to 'always', a reflog is created for them.
>> So far, the description of reflog entries for tags was empty, making the
>> reflog hard to understand. For example:
>> 6e3a7b3 refs/tags/test@{0}:
>>
>> Now, a reflog message is generated when creating a tag, following the
>> pattern "tag: tagging <short-sha1> (<description>)". If
>> GIT_REFLOG_ACTION is set, the message becomes "$GIT_REFLOG_ACTION
>> (<description>)" instead. If the tag references a commit object, the
>> description is set to the subject line of the commit, followed by its
>> commit date. For example:
>> 6e3a7b3 refs/tags/test@{0}: tag: tagging 6e3a7b3398 (Git 2.12-rc0, 2017-02-03)
>>
>> If the tag points to a tree/blob/tag objects, the following static
>> strings are taken as description:
>>
>>  - "tree object"
>>  - "blob object"
>>  - "other tag object"
>>
>> Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
>> Reviewed-by: Junio C Hamano <gitster@pobox.com>
> 
> This last line is inappropriate, as I didn't review _THIS_ version,
> which is different from the previous one, and I haven't checked if
> the way the comments on the previous review were addressed in this
> version is agreeable.

Sorry for that confusion. I'm still not used to when adding what sign-off is appropriate. I thought that adding you as reviewer is also a question of courtesy.

A version with revised tests will follow.
Junio C Hamano· Feb 8, 2017, 23:44 UTC · re: Cornelius Weig · lore

Re: [PATCH v4] tag: generate useful reflog message

Cornelius Weig <cornelius.weig@tngtech.com> writes:
> A version with revised tests will follow.

Thanks; I think this is clean enough. Let's queue this one and advance it to 'next' soonish.

cornelius.weig@tngtech.com· Feb 8, 2017, 22:41 UTC · re: Junio C Hamano · lore

[PATCH v5] tag: generate useful reflog message

From: Cornelius Weig <cornelius.weig@tngtech.com>

When tags are created with `--create-reflog` or with the option `core.logAllRefUpdates` set to 'always', a reflog is created for them. So far, the description of reflog entries for tags was empty, making the reflog hard to understand. For example: 6e3a7b3 refs/tags/test@{0}:

Now, a reflog message is generated when creating a tag, following the pattern "tag: tagging <short-sha1> (<description>)". If GIT_REFLOG_ACTION is set, the message becomes "$GIT_REFLOG_ACTION (<description>)" instead. If the tag references a commit object, the description is set to the subject line of the commit, followed by its commit date. For example: 6e3a7b3 refs/tags/test@{0}: tag: tagging 6e3a7b3398 (Git 2.12-rc0, 2017-02-03)

If the tag points to a tree/blob/tag objects, the following static strings are taken as description:

 - "tree object"
 - "blob object"
 - "other tag object"
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
Notes:
    Interdiff v4..v5
    diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
    index 894959f..1a3230f 100755
    --- a/t/t7004-tag.sh
    +++ b/t/t7004-tag.sh
    @@ -80,9 +80,10 @@ test_expect_success 'creating a tag using default HEAD should succeed' '
     	test_must_fail git reflog exists refs/tags/mytag
     '
    
    -git log -1 > expected \
    -	--format="format:tag: tagging %h (%s, %cd)%n" --date=format:%F
     test_expect_success 'creating a tag with --create-reflog should create reflog' '
    +	git log -1 \
    +		--format="format:tag: tagging %h (%s, %cd)%n" \
    +		--date=format:%Y-%m-%d >expected &&
     	test_when_finished "git tag -d tag_with_reflog" &&
     	git tag --create-reflog tag_with_reflog &&
     	git reflog exists refs/tags/tag_with_reflog &&
    @@ -90,9 +91,10 @@ test_expect_success 'creating a tag with --create-reflog should create reflog' '
     	test_cmp expected actual
     '
    
    -git log -1 > expected \
    -	--format="format:tag: tagging %h (%s, %cd)%n" --date=format:%F
     test_expect_success 'annotated tag with --create-reflog has correct message' '
    +	git log -1 \
    +		--format="format:tag: tagging %h (%s, %cd)%n" \
    +		--date=format:%Y-%m-%d >expected &&
     	test_when_finished "git tag -d tag_with_reflog" &&
     	git tag -m "annotated tag" --create-reflog tag_with_reflog &&
     	git reflog exists refs/tags/tag_with_reflog &&
 builtin/tag.c  | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 t/t7004-tag.sh | 18 +++++++++++++++++-
 2 files changed, 70 insertions(+), 2 deletions(-)
Show changes to 2 files +70 −2

builtin/tag.c, t/t7004-tag.sh

diff --git a/builtin/tag.c b/builtin/tag.c
index e40c4a9..bca890f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -302,6 +302,54 @@ static void create_tag(const unsigned char *object, const char *tag,
 	}
 }
 
+static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
+{
+	enum object_type type;
+	struct commit *c;
+	char *buf;
+	unsigned long size;
+	int subject_len = 0;
+	const char *subject_start;
+
+	char *rla = getenv("GIT_REFLOG_ACTION");
+	if (rla) {
+		strbuf_addstr(sb, rla);
+	} else {
+		strbuf_addstr(sb, _("tag: tagging "));
+		strbuf_add_unique_abbrev(sb, sha1, DEFAULT_ABBREV);
+	}
+
+	strbuf_addstr(sb, " (");
+	type = sha1_object_info(sha1, NULL);
+	switch (type) {
+	default:
+		strbuf_addstr(sb, _("object of unknown type"));
+		break;
+	case OBJ_COMMIT:
+		if ((buf = read_sha1_file(sha1, &type, &size)) != NULL) {
+			subject_len = find_commit_subject(buf, &subject_start);
+			strbuf_insert(sb, sb->len, subject_start, subject_len);
+		} else {
+			strbuf_addstr(sb, _("commit object"));
+		}
+		free(buf);
+
+		if ((c = lookup_commit_reference(sha1)) != NULL)
+			strbuf_addf(sb, ", %s", show_date(c->date, 0, DATE_MODE(SHORT)));
+		break;
+	case OBJ_TREE:
+		strbuf_addstr(sb, _("tree object"));
+		break;
+	case OBJ_BLOB:
+		strbuf_addstr(sb, _("blob object"));
+		break;
+	case OBJ_TAG:
+		strbuf_addstr(sb, _("other tag object"));
+		break;
+	}
+	strbuf_addch(sb, ')');
+}
+
 struct msg_arg {
 	int given;
 	struct strbuf buf;
@@ -335,6 +383,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf ref = STRBUF_INIT;
+	struct strbuf reflog_msg = STRBUF_INIT;
 	unsigned char object[20], prev[20];
 	const char *object_ref, *tag;
 	struct create_tag_options opt;
@@ -494,6 +543,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	else
 		die(_("Invalid cleanup mode %s"), cleanup_arg);
 
+	create_reflog_msg(object, &reflog_msg);
+
 	if (create_tag_object) {
 		if (force_sign_annotate && !annotate)
 			opt.sign = 1;
@@ -504,7 +555,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, object, prev,
 				   create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
-				   NULL, &err) ||
+				   reflog_msg.buf, &err) ||
 	    ref_transaction_commit(transaction, &err))
 		die("%s", err.buf);
 	ref_transaction_free(transaction);
@@ -514,5 +565,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	strbuf_release(&err);
 	strbuf_release(&buf);
 	strbuf_release(&ref);
+	strbuf_release(&reflog_msg);
 	return 0;
 }
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 072e6c6..1a3230f 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -81,9 +81,25 @@ test_expect_success 'creating a tag using default HEAD should succeed' '
 '
 
 test_expect_success 'creating a tag with --create-reflog should create reflog' '
+	git log -1 \
+		--format="format:tag: tagging %h (%s, %cd)%n" \
+		--date=format:%Y-%m-%d >expected &&
 	test_when_finished "git tag -d tag_with_reflog" &&
 	git tag --create-reflog tag_with_reflog &&
-	git reflog exists refs/tags/tag_with_reflog
+	git reflog exists refs/tags/tag_with_reflog &&
+	sed -e "s/^.*	//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'annotated tag with --create-reflog has correct message' '
+	git log -1 \
+		--format="format:tag: tagging %h (%s, %cd)%n" \
+		--date=format:%Y-%m-%d >expected &&
+	test_when_finished "git tag -d tag_with_reflog" &&
+	git tag -m "annotated tag" --create-reflog tag_with_reflog &&
+	git reflog exists refs/tags/tag_with_reflog &&
+	sed -e "s/^.*	//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
 '
 
 test_expect_success '--create-reflog does not create reflog on failure' '
-- 
2.10.2
Cornelius Weig· Feb 6, 2017, 16:54 UTC · re: Junio C Hamano · lore

Re: [PATCH] tag: generate useful reflog message

On 02/06/2017 12:25 AM, Junio C Hamano wrote:
> cornelius.weig@tngtech.com writes
> For a tag, I would imagine something like "tag: tagged 4e59582ff7
> ("Seventh batch for 2.12", 2017-01-23)" would be more appropriate.

Yes, I agree that this is much clearer. The revised version v3 implements this behavior.

Show 13 quoted lines
>> Notes:
>>     While playing around with tag reflogs I also found a bug that was present
>>     before this patch. It manifests itself when the sha1-ref in the reflog does not
>>     point to a commit object but something else.
> 
> I think the fix would involve first ripping out the "reflog walking"
> code that was bolted on and stop allowing it to inject the entries
> taken from the reflog into the "walk the commit DAG" machinery.
> Then "reflog walking" code needs to be taught to have its own "now
> we got a single object to show, show it (using the helper functions
> to show a single object that is already used by 'git show')" code,
> instead of piggy-backing on the output codepath used by "log" and
> "rev-list".

I'll start investigating how that could be done. My first glance tells me that it won't be easy. Especially because I'm not yet familiar with the git code.

Thanks for your advice!

← back to recent threads