Re: [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 15, 2017, 19:50 UTC
Message-ID: <xmqqlgs6e2uv.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <db54f291407ef34080fe9e8c9dbdd482b4b3698d.1489581340.git.git@drmicha.warpmail.net>

Michael J Gruber <git@drmicha.warpmail.net> writes:

Show 22 quoted lines

> I'm wondering whether I'm overlooking any side-effects that our test
> suite doesn't cover, though. In any case, we may want to have
> lightweight tags allowed based on an extra flag (like the
> existing --tags for describe, which means something else for name-rev).
>
>  builtin/name-rev.c     | 2 ++
>  t/t9903-bash-prompt.sh | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index 8bdc3eaa6f..75ba7bbad5 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -207,6 +207,8 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
>  	if (o && o->type == OBJ_COMMIT) {
>  		struct commit *commit = (struct commit *)o;
>  
> +		if (taggerdate == ULONG_MAX) /* lightweight tag */
> +			taggerdate = commit->date;
>  		path = name_ref_abbrev(path, can_abbreviate_output);
>  		name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref);
>  	}

I think this comment indicates where this change will bite us ;-)

This could have been an invocation of "name-rev" without "--tags", where _any_ tip of ref can be used to name a revision, and in such a case, retaining commit->date may still be valuable, but it is probably wrong to use it for nothing more than tie-breaking.

In an extreme case, imagine that your repository does not have any tag and you have a commit that can be reached from both 'maint' and 'master' branches. Because the current code assigns the same useless taggerdate for these tips of refs, the commit is described solely based on the distance from the tip of 'maint' or 'master' with today's code.

If you assign commit date to taggerdate variable here, that date is used to trump the distance when deciding whether to name the commit based on 'maint' and 'master', and I doubt it is what you want. Perhaps I regularly advance 'master', but still add a few selected fixes to 'maint' every few days---when these two branches both are active like that, do you want the same commit that is topologically much closer to 'maint' than 'master' to be described in terms of 'maint' on some days (i.e. those days I added fixes to it) and 'master' on some other days?

I think the expectation by end-users who do not use "--tags" is that they still prefer a commit to be named in terms of the oldest tag that is fewer number of hops away, and the naming to fall back to non-tag refs only when there is no tags that reach the commit. And between two or more non-tag tips that are _known_ to be still active (iow, branch tips), they want "fewer number of hops away" to be the primary deciding factor to choose among them. The ULONG_MAX setting the code currently uses is very suitable for that purpose. It penalizes the non-tag refs, and it disables the use of timestamp for deciding among these tips and uses only the topological distance.

I think you can do this change _ONLY_ when we are operating under the "--tags" option. That would place unannotated tags at a better location in the timestamp order than the current code does, without introducing issues with refs that are actively moving.