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

[Junio C Hamano <gitster@pobox.com>]

cornelius.weig@tngtech.com writes:

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

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

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

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