Re: [PATCH] tag: generate useful reflog message

[Cornelius Weig]

On 02/06/2017 12:25 AM, Junio C Hamano wrote:
> cornelius.w...@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.

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

Re: [PATCH] tag: generate useful reflog message

[Junio C Hamano]

cornelius.w...@tngtech.com writes:

> Now, a reflog message is generated when creating a tag. The message
> follows the pattern "commit: " 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".

[PATCH] tag: generate useful reflog message

[cornelius . weig]

From: Cornelius Weig 

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: " 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 
---

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

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, , );
+   if (buf) {
+   subject_len = find_commit_subject(buf, _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, _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, ) ||
+  reflog_msg.buf, ) ||
ref_transaction_commit(transaction, ))
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();
strbuf_release();
strbuf_release();
+   strbuf_release(_msg);
return 0;
 }
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 072e6c6..0a92b2c 100755