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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 8, 2017, 21:28 UTC
Message-ID: <xmqqshnov0c4.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170206222416.28720-2-cornelius.weig@tngtech.com>

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.