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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 6, 2017, 19:32 UTC
Message-ID: <xmqqpoiv15ew.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170206135834.19637-1-cornelius.weig@tngtech.com>

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