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

[Junio C Hamano]

Cornelius Weig  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.

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

[Cornelius Weig]

On 02/08/2017 10:28 PM, Junio C Hamano wrote:
> cornelius.w...@tngtech.com writes:
> 
>> 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/test@{0}:
>>
>> Now, a reflog message is generated when creating a tag, following the
>> pattern "tag: tagging  ()". If
>> GIT_REFLOG_ACTION is set, the message becomes "$GIT_REFLOG_ACTION
>> ()" 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 
>> Reviewed-by: Junio C Hamano 
> 
> 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.

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

[Junio C Hamano]

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

> 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/test@{0}:
>
> Now, a reflog message is generated when creating a tag, following the
> pattern "tag: tagging  ()". If
> GIT_REFLOG_ACTION is set, the message becomes "$GIT_REFLOG_ACTION
> ()" 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 
> Reviewed-by: Junio C Hamano 

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.

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

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

[PATCH v4] tag: generate useful reflog message

[cornelius . weig]

From: Cornelius Weig 

Thanks for taking a look at my last version.

> 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:
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, , );
-   if (!c || !buf) {
-   die(_("commit object %s could not be read"),
-   sha1_to_hex(sha1));
+   if ((buf = read_sha1_file(sha1, , )) != NULL) {
+   subject_len = find_commit_subject(buf, _start);
+   strbuf_insert(sb, sb->len, subject_start, subject_len);
+   } else {
+   strbuf_addstr(sb, _("commit object"));
}
-   subject_len = find_commit_subject(buf, _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

[PATCH v4] 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/test@{0}:

Now, a reflog message is generated when creating a tag, following the
pattern "tag: tagging  ()". If
GIT_REFLOG_ACTION is set, the message becomes "$GIT_REFLOG_ACTION
()" 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 
Reviewed-by: Junio C Hamano 
---
 builtin/tag.c  | 54 +-
 t/t7004-tag.sh | 16 +++-
 2 files changed, 68 insertions(+), 2 deletions(-)

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, , )) != NULL) {
+   subject_len = find_commit_subject(buf, _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, _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, ) ||
+  reflog_msg.buf, ) ||
ref_transaction_commit(transaction, ))
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();
strbuf_release();
strbuf_release();
+   strbuf_release(_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 &&
-