threads / patch / 45385

patch, 3 partsdescribe --contains sanity

Subject: [PATCH 0/3] describe --contains sanity

## tl;dr

46 messages between Mar 15, 2017 and May 20, 2017. Diffs are folded; open one to read it.

replies: 45people: 5as markdown or json

Michael J Gruber· Mar 15, 2017, 13:15 UTC · lore

2 patches and 1 RFD around describe (--contains). They are technically independent, but happened along the same stroll in that area when I tried to match documentation, my expectations, and reality.

1 and 2 should be no-brainers.
3 is something to ponder for a while.
Michael J Gruber (3):
  describe: debug is incompatible with contains
  git-prompt: add a describe style for any tags
  name-rev: Allow lightweight tags and branch refs
 Documentation/git-describe.txt   | 2 +-
 builtin/describe.c               | 2 ++
 builtin/name-rev.c               | 2 ++
 contrib/completion/git-prompt.sh | 3 +++
 t/t9903-bash-prompt.sh           | 2 +-
 5 files changed, 9 insertions(+), 2 deletions(-)
-- 
2.12.0.384.g157040b11f.dirty
Michael J Gruber· Mar 15, 2017, 13:15 UTC · re: Michael J Gruber · lore

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

When name-rev constructs possible names for a rev, it assigns the taggerdate to an annotated tag and ULONG_MAX to other names such as lightweight tags and branch names. Practically, this rules out even naming a tagged commit by the tag if that is lightweight and there is another possible indirect name (e.g. foo~5) coming from an annotated tag.

Instead, assign the commit date to lightweight tags or branch refs so that they get their fair chance of being picked up.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---

Originally, I didn't even think of submitting this as is until I noticed that all tests succeed with it (bar one for git-prompt.h which looked strange anyways); rather, I thought about another switch "--lightweight" or so.

I still submit it as RFD. This origin of my foray into name-rev came from git describe, where a fair expectation should be that "--contains" mode is the same as the odinary mode, albeit with a different direction in time/traversal. (Technically, describe --contains is name-rev.)

Consider the following:

git init echo past >tense git add tense git commit -m past git tag -m past -a past echo present >tense git commit -am present git tag present echo future >tense git commit -am future

"git describe past present future" gives

past past-1-g5ad942f future

because (as documented) it does not consider lightweight tags, and thus has to describe present from the past.

"git describe --tags past present future" gives

past present future

because (as documented) it does consider lightweight tags.
"git describe --contains past present future" gives

past^0 future~1 future^0

and I have a hard time matching that with the documentation (describe doc claims that --tags is automatic; name-rev doc does not distinguish between tag types). "--tags" does not make any difference here, nor does "--all" (besides fully qualifying the tags).

"git describe --contains past present future" gives

past^0 present future^0

with this patch (I'm tempted to say: with the present patch), which is what I would expect.

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(-)
Show changes to 2 files +3 −1

builtin/name-rev.c, t/t9903-bash-prompt.sh

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);
 	}
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 97c9b32c2e..d467d5957d 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -107,7 +107,7 @@ test_expect_success 'prompt - describe detached head - contains' '
 '
 
 test_expect_success 'prompt - describe detached head - branch' '
-	printf " ((tags/t2~1))" >expected &&
+	printf " ((b1~1))" >expected &&
 	git checkout b1^ &&
 	test_when_finished "git checkout master" &&
 	(
-- 
2.12.0.384.g157040b11f.dirty
Junio C Hamano· Mar 15, 2017, 16:14 UTC · re: Michael J Gruber · lore

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

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 24 quoted lines
> Consider the following:
> ...
> "git describe past present future" gives
>
> past
> past-1-g5ad942f
> future
>
> because (as documented) it does not consider lightweight tags, and thus
> has to describe present from the past.
>
> "git describe --tags past present future" gives
>
> past
> present
> future
> 
> because (as documented) it does consider lightweight tags.
>
> "git describe --contains past present future" gives
>
> past^0
> future~1
> future^0

Nice illustration to convince anybody that "git describe --tags --contains" should give "past present future" (or "past^0 present future^0"). I am not sure if it is a good idea in general to change the output to include "present" when "--tags" is not given, though.

For _this_ example, it does give us a more useful result, but the example was to constructed to illustrate the case where the new behaviour is more useful, and does not serve to convince us that it won't have negative impact X-<.

Junio C Hamano· Mar 15, 2017, 19:50 UTC · re: Michael J Gruber · lore

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

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.

Junio C Hamano· Mar 15, 2017, 20:51 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> 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.
> ...
> 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.

I'll leave "only do so under --tags" as an exercise to the reader, but if we want to use the timestamp for tie-breaking between names based on two branches, a patch to do so may look like this one.

I am presenting it as a single ball of wax, but in the real history I have (which I am not publishing, as I haven't decided if this is a good direction to go in the first place), this is a two-patch series, the first one that factors out is_better_name() without doing anything else, followed by a commit that adds "from_tag" and passes it throughout the callpath to allow is_better_name() to use it to:

 - always favor names based on a tag;
 - between two names based on tags, favor name based on an older tag
   and tiebreak with distance;
 - between two names based on non-tags, favor name with shorter hops
   and tiebreak with age.
 builtin/name-rev.c | 43 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)
Show changes to builtin/name-rev.c +36 −7
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index cd89d48b65..ba30c9ca23 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -13,6 +13,7 @@ typedef struct rev_name {
 	unsigned long taggerdate;
 	int generation;
 	int distance;
+	int from_tag;
 } rev_name;
 
 static long cutoff = LONG_MAX;
@@ -20,9 +21,31 @@ static long cutoff = LONG_MAX;
 /* How many generations are maximally preferred over _one_ merge traversal? */
 #define MERGE_TRAVERSAL_WEIGHT 65535
 
+static int is_better_name(struct rev_name *name,
+			  const char *tip_name,
+			  unsigned long taggerdate,
+			  int generation,
+			  int distance,
+			  int from_tag)
+{
+	if (name->from_tag > from_tag)
+		return 0;
+	else if (name->from_tag < from_tag)
+		return 1;
+
+	if (from_tag)
+		return (name->taggerdate > taggerdate ||
+			(name->taggerdate == taggerdate &&
+			 name->distance > distance));
+	else
+		return (name->distance > distance ||
+			(name->distance == distance &&
+			 name->taggerdate > taggerdate));
+}
+
 static void name_rev(struct commit *commit,
 		const char *tip_name, unsigned long taggerdate,
-		int generation, int distance,
+		int generation, int distance, int from_tag,
 		int deref)
 {
 	struct rev_name *name = (struct rev_name *)commit->util;
@@ -45,14 +68,14 @@ static void name_rev(struct commit *commit,
 		name = xmalloc(sizeof(rev_name));
 		commit->util = name;
 		goto copy_data;
-	} else if (name->taggerdate > taggerdate ||
-			(name->taggerdate == taggerdate &&
-			 name->distance > distance)) {
+	} else if (is_better_name(name, tip_name, taggerdate,
+				  generation, distance, from_tag)) {
 copy_data:
 		name->tip_name = tip_name;
 		name->taggerdate = taggerdate;
 		name->generation = generation;
 		name->distance = distance;
+		name->from_tag = from_tag;
 	} else
 		return;
 
@@ -72,10 +95,12 @@ static void name_rev(struct commit *commit,
 						   parent_number);
 
 			name_rev(parents->item, new_name, taggerdate, 0,
-				distance + MERGE_TRAVERSAL_WEIGHT, 0);
+				 distance + MERGE_TRAVERSAL_WEIGHT,
+				 from_tag, 0);
 		} else {
 			name_rev(parents->item, tip_name, taggerdate,
-				generation + 1, distance + 1, 0);
+				 generation + 1, distance + 1,
+				 from_tag, 0);
 		}
 	}
 }
@@ -174,9 +199,13 @@ 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;
+		int from_tag = starts_with(path, "refs/tags/");
 
+		if (taggerdate == ULONG_MAX)
+			taggerdate = ((struct commit *)o)->date;
 		path = name_ref_abbrev(path, can_abbreviate_output);
-		name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref);
+		name_rev(commit, xstrdup(path), taggerdate, 0, 0,
+			 from_tag, deref);
 	}
 	return 0;
 }
Junio C Hamano· Mar 15, 2017, 22:50 UTC · re: Junio C Hamano · lore

[PATCH 0/2] Teach name-rev to pay more attention to lightweight tags

Here is a reroll of what I did in
    http://public-inbox.org/git/xmqqd1die00j.fsf@gitster.mtv.corp.google.com/
Junio C Hamano (2):
  name-rev: refactor logic to see if a new candidate is a better name
  name-rev: favor describing with tags and use committer date to tiebreak
 builtin/name-rev.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++------
 t/t4202-log.sh     |  2 +-
 2 files changed, 57 insertions(+), 8 deletions(-)
-- 
2.12.0-306-g4a9b9b32d4
Junio C Hamano· Mar 15, 2017, 22:50 UTC · re: Junio C Hamano · lore

[PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak

"git name-rev" assigned a phony "far in the future" date to tips of refs that are not pointing at tag objects, and favored names based on a ref with the oldest date. This made it almost impossible for an unannotated tags and branches to be counted as a viable base, which was especially problematic when the command is run with the "--tags" option. If an unannotated tag that points at an ancient commit and an annotated tag that points at a much newer commit reaches the commit that is being named, the old unannotated tag was ignored.

Update the "taggerdate" field of the rev-name structure, which is initialized from the tip of ref, to have the committer date if the object at the tip of ref is a commit, not a tag, so that we can optionally take it into account when doing "is this name better?" comparison logic.

When "name-rev" is run without the "--tags" option, the general expectation is still to name the commit based on a tag if possible, but use non-tag refs as fallback, and tiebreak among these non-tag refs by favoring names with shorter hops from the tip. The use of a phony "far in the future" date in the original code was an effective way to ensure this expectation is held: a non-tag tip gets the same "far in the future" timestamp, giving precedence to tags, and among non-tag tips, names with shorter hops are preferred over longer hops, without taking the "taggerdate" into account. As we are taking over the "taggerdate" field to store the committer date for tips with commits:

 (1) keep the original logic when comparing names based on two refs
     both of which are from refs/tags/;
 (2) favoring a name based on a ref in refs/tags/ hierarchy over
     a ref outside the hierarchy;
 (3) between two names based on a ref both outside refs/tags/, give
     precedence to a name with shorter hops and use "taggerdate"
     only to tie-break.

A change to t4202 is a natural consequence. The test creates a commit on a branch "side" and points at it with an unannotated tag "refs/tags/side-2". The original code couldn't decide which one to favor at all, and gave a name based on a branch (simply because refs/heads/side sorts earlier than refs/tags/side-2). Because the updated logic is taught to favor refs in refs/tags/ hierarchy, the the test is updated to expect to see tags/side-2 instead.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * I am sure others can add better heurisitics in is_better_name()
   for comparing names based on non-tag refs, and I do not mind
   people disagreeing with the logic that this patch happens to
   implement.  This is done primarily to illustrate the value of
   using a separate helper function is_better_name() instead of
   open-coding the selection logic in name_rev() function.
---
 builtin/name-rev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++---------
 t/t4202-log.sh     |  2 +-
 2 files changed, 49 insertions(+), 10 deletions(-)
Show changes to 2 files +49 −10

builtin/name-rev.c, t/t4202-log.sh

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index f64c71d9bc..eac0180c62 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -13,6 +13,7 @@ typedef struct rev_name {
 	unsigned long taggerdate;
 	int generation;
 	int distance;
+	int from_tag;
 } rev_name;
 
 static long cutoff = LONG_MAX;
@@ -24,16 +25,47 @@ static int is_better_name(struct rev_name *name,
 			  const char *tip_name,
 			  unsigned long taggerdate,
 			  int generation,
-			  int distance)
+			  int distance,
+			  int from_tag)
 {
-	return (name->taggerdate > taggerdate ||
-		(name->taggerdate == taggerdate &&
-		 name->distance > distance));
+	/*
+	 * When comparing names based on tags, prefer names
+	 * based on the older tag, even if it is farther away.
+	 */
+	if (from_tag && name->from_tag)
+		return (name->taggerdate > taggerdate ||
+			(name->taggerdate == taggerdate &&
+			 name->distance > distance));
+
+#define COMPARE(attribute, smaller_is_better)	 \
+	if (name->attribute > attribute) \
+		return smaller_is_better; \
+	if (name->attribute < attribute) \
+		return !smaller_is_better
+
+	/*
+	 * We know that at least one of them is a non-tag at this point.
+	 * favor a tag over a non-tag.
+	 */
+	COMPARE(from_tag, 0);
+
+	/*
+	 * We are now looking at two non-tags.  Tiebreak to favor
+	 * shorter hops.
+	 */
+	COMPARE(distance, 1);
+
+	/* ... or tiebreak to favor older date */
+	COMPARE(taggerdate, 1);
+
+	/* keep the current one if we cannot decide */
+	return 0;
+#undef COMPARE
 }
 
 static void name_rev(struct commit *commit,
 		const char *tip_name, unsigned long taggerdate,
-		int generation, int distance,
+		int generation, int distance, int from_tag,
 		int deref)
 {
 	struct rev_name *name = (struct rev_name *)commit->util;
@@ -57,12 +89,13 @@ static void name_rev(struct commit *commit,
 		commit->util = name;
 		goto copy_data;
 	} else if (is_better_name(name, tip_name, taggerdate,
-				  generation, distance)) {
+				  generation, distance, from_tag)) {
 copy_data:
 		name->tip_name = tip_name;
 		name->taggerdate = taggerdate;
 		name->generation = generation;
 		name->distance = distance;
+		name->from_tag = from_tag;
 	} else
 		return;
 
@@ -82,10 +115,12 @@ static void name_rev(struct commit *commit,
 						   parent_number);
 
 			name_rev(parents->item, new_name, taggerdate, 0,
-				distance + MERGE_TRAVERSAL_WEIGHT, 0);
+				 distance + MERGE_TRAVERSAL_WEIGHT,
+				 from_tag, 0);
 		} else {
 			name_rev(parents->item, tip_name, taggerdate,
-				generation + 1, distance + 1, 0);
+				 generation + 1, distance + 1,
+				 from_tag, 0);
 		}
 	}
 }
@@ -184,9 +219,13 @@ 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;
+		int from_tag = starts_with(path, "refs/tags/");
 
+		if (taggerdate == ULONG_MAX)
+			taggerdate = ((struct commit *)o)->date;
 		path = name_ref_abbrev(path, can_abbreviate_output);
-		name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref);
+		name_rev(commit, xstrdup(path), taggerdate, 0, 0,
+			 from_tag, deref);
 	}
 	return 0;
 }
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 48b55bfd27..038911f230 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -398,7 +398,7 @@ cat > expect <<\EOF
 | |
 | |     Merge branch 'side'
 | |
-| * commit side
+| * commit tags/side-2
 | | Author: A U Thor <author@example.com>
 | |
 | |     side-2
-- 
2.12.0-306-g4a9b9b32d4
Lars Schneider· Mar 17, 2017, 04:07 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak

Show 50 quoted lines
> On 16 Mar 2017, at 06:50, Junio C Hamano <gitster@pobox.com> wrote:
> 
> "git name-rev" assigned a phony "far in the future" date to tips of
> refs that are not pointing at tag objects, and favored names based
> on a ref with the oldest date.  This made it almost impossible for
> an unannotated tags and branches to be counted as a viable base,
> which was especially problematic when the command is run with the
> "--tags" option.  If an unannotated tag that points at an ancient
> commit and an annotated tag that points at a much newer commit
> reaches the commit that is being named, the old unannotated tag was
> ignored.
> 
> Update the "taggerdate" field of the rev-name structure, which is
> initialized from the tip of ref, to have the committer date if the
> object at the tip of ref is a commit, not a tag, so that we can
> optionally take it into account when doing "is this name better?"
> comparison logic.
> 
> When "name-rev" is run without the "--tags" option, the general
> expectation is still to name the commit based on a tag if possible,
> but use non-tag refs as fallback, and tiebreak among these non-tag
> refs by favoring names with shorter hops from the tip.  The use of a
> phony "far in the future" date in the original code was an effective
> way to ensure this expectation is held: a non-tag tip gets the same
> "far in the future" timestamp, giving precedence to tags, and among
> non-tag tips, names with shorter hops are preferred over longer
> hops, without taking the "taggerdate" into account.  As we are
> taking over the "taggerdate" field to store the committer date for
> tips with commits:
> 
> (1) keep the original logic when comparing names based on two refs
>     both of which are from refs/tags/;
> 
> (2) favoring a name based on a ref in refs/tags/ hierarchy over
>     a ref outside the hierarchy;
> 
> (3) between two names based on a ref both outside refs/tags/, give
>     precedence to a name with shorter hops and use "taggerdate"
>     only to tie-break.
> 
> A change to t4202 is a natural consequence.  The test creates a
> commit on a branch "side" and points at it with an unannotated tag
> "refs/tags/side-2".  The original code couldn't decide which one to
> favor at all, and gave a name based on a branch (simply because
> refs/heads/side sorts earlier than refs/tags/side-2).  Because the
> updated logic is taught to favor refs in refs/tags/ hierarchy, the
> the test is updated to expect to see tags/side-2 instead.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

A quick bisect indicates that this patch might break t9807-git-p4-submit.sh 8 and 13. I haven't looked into it further, yet.

https://travis-ci.org/git/git/jobs/211948406#L2152 https://travis-ci.org/git/git/jobs/211948406#L2460

Non-JS: https://s3.amazonaws.com/archive.travis-ci.org/jobs/211948406/log.txt
- Lars
Junio C Hamano· Mar 17, 2017, 05:45 UTC · re: Lars Schneider · lore

Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak

Lars Schneider <larsxschneider@gmail.com> writes:
> A quick bisect indicates that this patch might break 
> t9807-git-p4-submit.sh 8 and 13. I haven't looked into
> it further, yet.

As I do not do P4, help in diagnosing why it breaks is appreciated. If the test script expects a commit, that can be described in two more more ways, is named one way (e.g. named after a branch) and the new "favor tags even if they are unannotated" behaviour breaks its expectation, and the only thing the test _should_ care about is what commit the result is, then clearly the test script needs to be fixed. On the other hand, if git-p4 command internally uses name-rev and it is not prepared to properly handle commits that can be named in more than one way, the problem would be deeper, as it would mean it can misbehave even without the change to name-rev when multiple branches point at the same commit.

Thanks.
Junio C Hamano· Mar 17, 2017, 05:56 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> Lars Schneider <larsxschneider@gmail.com> writes:
>
>> A quick bisect indicates that this patch might break 
>> t9807-git-p4-submit.sh 8 and 13. I haven't looked into
>> it further, yet.
>
> As I do not do P4, help in diagnosing why it breaks is appreciated.
> If the test script expects...
> On the other hand, if git-p4 command internally uses name-rev and it
> is not prepared to properly handle commits that can be named in more
> than one way, the problem would be deeper, as it would mean it can
> misbehave even without the change to name-rev when multiple branches
> point at the same commit.
Yikes.  Perhaps something along this line?  

This function seems to want to learn which branch we are on, and running "name-rev HEAD" is *NEVER* the right way to do so. If you are on branch B which happens to point at the same commit as branch A, "name-rev HEAD" can say either A or B (and it is likely it would say A simply because it sorts earlier, and the logic seems to favor the one that was discovered earlier when all else being equal).

 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to git-p4.py +1 −1
diff --git a/git-p4.py b/git-p4.py
index eab319d76e..351d1ab58e 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -582,7 +582,7 @@ def currentGitBranch():
         # on a detached head
         return None
     else:
-        return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
+        return read_pipe(["git", "symbolic-ref", "HEAD"]).strip()[11:]
 
 def isValidGitDir(path):
     return git_dir(path) != None
Lars Schneider· Mar 17, 2017, 17:09 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak

Show 43 quoted lines
> On 17 Mar 2017, at 13:56, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Lars Schneider <larsxschneider@gmail.com> writes:
>> 
>>> A quick bisect indicates that this patch might break 
>>> t9807-git-p4-submit.sh 8 and 13. I haven't looked into
>>> it further, yet.
>> 
>> As I do not do P4, help in diagnosing why it breaks is appreciated.
>> If the test script expects...
>> On the other hand, if git-p4 command internally uses name-rev and it
>> is not prepared to properly handle commits that can be named in more
>> than one way, the problem would be deeper, as it would mean it can
>> misbehave even without the change to name-rev when multiple branches
>> point at the same commit.
> 
> Yikes.  Perhaps something along this line?  
> 
> This function seems to want to learn which branch we are on, and
> running "name-rev HEAD" is *NEVER* the right way to do so.  If you
> are on branch B which happens to point at the same commit as branch
> A, "name-rev HEAD" can say either A or B (and it is likely it would
> say A simply because it sorts earlier, and the logic seems to favor
> the one that was discovered earlier when all else being equal).
> 
> git-p4.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git-p4.py b/git-p4.py
> index eab319d76e..351d1ab58e 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -582,7 +582,7 @@ def currentGitBranch():
>         # on a detached head
>         return None
>     else:
> -        return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
> +        return read_pipe(["git", "symbolic-ref", "HEAD"]).strip()[11:]
> 
> def isValidGitDir(path):
>     return git_dir(path) != None

Following your explanation this patch looks good to me and this fixes the test failure. TBH I never thought about the difference of these commands before. "rev" and "ref" sound so similar although they denote completely different things.

Thanks, Lars

Junio C Hamano· Mar 17, 2017, 17:17 UTC · re: Lars Schneider · lore

Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak

Lars Schneider <larsxschneider@gmail.com> writes:
Show 21 quoted lines
>> git-p4.py | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/git-p4.py b/git-p4.py
>> index eab319d76e..351d1ab58e 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -582,7 +582,7 @@ def currentGitBranch():
>>         # on a detached head
>>         return None
>>     else:
>> -        return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
>> +        return read_pipe(["git", "symbolic-ref", "HEAD"]).strip()[11:]
>> 
>> def isValidGitDir(path):
>>     return git_dir(path) != None
>
> Following your explanation this patch looks good to me and this fixes the
> test failure. TBH I never thought about the difference of these commands
> before. "rev" and "ref" sound so similar although they denote completely 
> different things.
Thanks for testing.

The above was done merely to point out the problematic place and a possible solution. As I am not familiar with the code in git-p4.py, I didn't even try to check if the code already has a helper function that strips "refs/heads/" from the beginning of the string (iow, I am not happy with the [11:]). I didn't and don't like the fact that this function now runs "symbolic-ref HEAD" twice but I didn't try to see if there are more suitable and idiomatic ways to do this with a single invocation.

Hence, I would really prefer not to commit mine myself. I'd rather see somebody from git-p4 circle to come up with a version that is more in line with the way things are done in the existing code and send a tested version for me to apply.

Thanks.
Junio C Hamano· Mar 17, 2017, 22:43 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> Lars Schneider <larsxschneider@gmail.com> writes:
> ...
>> Following your explanation this patch looks good to me and this fixes the
>> test failure. TBH I never thought about the difference of these commands
>> before. "rev" and "ref" sound so similar although they denote completely 
>> different things.
>
> Thanks for testing.
> ...
> Hence, I would really prefer not to commit mine myself.  I'd rather
> see somebody from git-p4 circle to come up with a version that is
> more in line with the way things are done in the existing code and
> send a tested version for me to apply.

Tentatively I queued my hack together with the name-rev thing on 'pu', and Travis seems to be OK with the result.

It would be nice if we can "fix" the use of "name-rev HEAD" in "git p4" sooner rather than later. If the code truly uses it to figure out what branch we are currently on and acts on it, as the name of that function suggests, it may be easy to construct a testcase where the code without the fix does a wrong thing (e.g. have two branches pointing at the same commit, and tell "git p4" to act on the one that sorts later in the "git branch --list" output; the command would act as if it were working on the other branch), and shows that the patch fixes that problem. That would be a bug worth fixing quickly regardless of the "name-rev" change michael wanted to make.

Thanks.
Michael J Gruber· Mar 29, 2017, 14:39 UTC · re: Junio C Hamano · lore

[PATCH v2 2/3] name-rev: favor describing with tags and use committer date to tiebreak

From: Junio C Hamano <gitster@pobox.com>

"git name-rev" assigned a phony "far in the future" date to tips of refs that are not pointing at tag objects, and favored names based on a ref with the oldest date. This made it almost impossible for an unannotated tags and branches to be counted as a viable base, which was especially problematic when the command is run with the "--tags" option. If an unannotated tag that points at an ancient commit and an annotated tag that points at a much newer commit reaches the commit that is being named, the old unannotated tag was ignored.

Update the "taggerdate" field of the rev-name structure, which is initialized from the tip of ref, to have the committer date if the object at the tip of ref is a commit, not a tag, so that we can optionally take it into account when doing "is this name better?" comparison logic.

When "name-rev" is run without the "--tags" option, the general expectation is still to name the commit based on a tag if possible, but use non-tag refs as fallback, and tiebreak among these non-tag refs by favoring names with shorter hops from the tip. The use of a phony "far in the future" date in the original code was an effective way to ensure this expectation is held: a non-tag tip gets the same "far in the future" timestamp, giving precedence to tags, and among non-tag tips, names with shorter hops are preferred over longer hops, without taking the "taggerdate" into account. As we are taking over the "taggerdate" field to store the committer date for tips with commits:

 (1) keep the original logic when comparing names based on two refs
     both of which are from refs/tags/;
 (2) favoring a name based on a ref in refs/tags/ hierarchy over
     a ref outside the hierarchy;
 (3) between two names based on a ref both outside refs/tags/, give
     precedence to a name with shorter hops and use "taggerdate"
     only to tie-break.

A change to t4202 is a natural consequence. The test creates a commit on a branch "side" and points at it with an unannotated tag "refs/tags/side-2". The original code couldn't decide which one to favor at all, and gave a name based on a branch (simply because refs/heads/side sorts earlier than refs/tags/side-2). Because the updated logic is taught to favor refs in refs/tags/ hierarchy, the the test is updated to expect to see tags/side-2 instead.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * I am sure others can add better heurisitics in is_better_name()
   for comparing names based on non-tag refs, and I do not mind
   people disagreeing with the logic that this patch happens to
   implement.  This is done primarily to illustrate the value of
   using a separate helper function is_better_name() instead of
   open-coding the selection logic in name_rev() function.
Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 builtin/name-rev.c | 53 ++++++++++++++++++++++++++++++++++++++++++++---------
 t/t4202-log.sh     |  2 +-
 2 files changed, 45 insertions(+), 10 deletions(-)
Show changes to 2 files +45 −10

builtin/name-rev.c, t/t4202-log.sh

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 7890f826ce..bf7ed015ae 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -13,6 +13,7 @@ typedef struct rev_name {
 	unsigned long taggerdate;
 	int generation;
 	int distance;
+	int from_tag;
 } rev_name;
 
 static long cutoff = LONG_MAX;
@@ -24,16 +25,43 @@ static int is_better_name(struct rev_name *name,
 			  const char *tip_name,
 			  unsigned long taggerdate,
 			  int generation,
-			  int distance)
+			  int distance,
+			  int from_tag)
 {
-	return (name->taggerdate > taggerdate ||
-		(name->taggerdate == taggerdate &&
-		 name->distance > distance));
+	/*
+	 * When comparing names based on tags, prefer names
+	 * based on the older tag, even if it is farther away.
+	 */
+	if (from_tag && name->from_tag)
+		return (name->taggerdate > taggerdate ||
+			(name->taggerdate == taggerdate &&
+			 name->distance > distance));
+
+	/*
+	 * We know that at least one of them is a non-tag at this point.
+	 * favor a tag over a non-tag.
+	 */
+	if (name->from_tag != from_tag)
+		return from_tag;
+
+	/*
+	 * We are now looking at two non-tags.  Tiebreak to favor
+	 * shorter hops.
+	 */
+	if (name->distance != distance)
+		return name->distance > distance;
+
+	/* ... or tiebreak to favor older date */
+	if (name->taggerdate != taggerdate)
+		return name->taggerdate > taggerdate;
+
+	/* keep the current one if we cannot decide */
+	return 0;
 }
 
 static void name_rev(struct commit *commit,
 		const char *tip_name, unsigned long taggerdate,
-		int generation, int distance,
+		int generation, int distance, int from_tag,
 		int deref)
 {
 	struct rev_name *name = (struct rev_name *)commit->util;
@@ -57,12 +85,13 @@ static void name_rev(struct commit *commit,
 		commit->util = name;
 		goto copy_data;
 	} else if (is_better_name(name, tip_name, taggerdate,
-				  generation, distance)) {
+				  generation, distance, from_tag)) {
 copy_data:
 		name->tip_name = tip_name;
 		name->taggerdate = taggerdate;
 		name->generation = generation;
 		name->distance = distance;
+		name->from_tag = from_tag;
 	} else
 		return;
 
@@ -82,10 +111,12 @@ static void name_rev(struct commit *commit,
 						   parent_number);
 
 			name_rev(parents->item, new_name, taggerdate, 0,
-				distance + MERGE_TRAVERSAL_WEIGHT, 0);
+				 distance + MERGE_TRAVERSAL_WEIGHT,
+				 from_tag, 0);
 		} else {
 			name_rev(parents->item, tip_name, taggerdate,
-				generation + 1, distance + 1, 0);
+				 generation + 1, distance + 1,
+				 from_tag, 0);
 		}
 	}
 }
@@ -216,9 +247,13 @@ 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;
+		int from_tag = starts_with(path, "refs/tags/");
 
+		if (taggerdate == ULONG_MAX)
+			taggerdate = ((struct commit *)o)->date;
 		path = name_ref_abbrev(path, can_abbreviate_output);
-		name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref);
+		name_rev(commit, xstrdup(path), taggerdate, 0, 0,
+			 from_tag, deref);
 	}
 	return 0;
 }
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index f577990716..4afd8135cd 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -399,7 +399,7 @@ cat > expect <<\EOF
 | |
 | |     Merge branch 'side'
 | |
-| * commit side
+| * commit tags/side-2
 | | Author: A U Thor <author@example.com>
 | |
 | |     side-2
-- 
2.12.2.712.gba4c48c431
Michael J Gruber· Mar 29, 2017, 14:39 UTC · re: Junio C Hamano · lore

[PATCH v2 0/3] name-rev sanity

So here is v2 of the name-rev series, the result of our discussions being:
Junio C Hamano (2):
  name-rev: refactor logic to see if a new candidate is a better name
  name-rev: favor describing with tags and use committer date to
    tiebreak
That second patch is slighty changed as discussed, but still mostly Junio's
Michael J Gruber (1):
  name-rev: provide debug output
This replaces the patch which documented that --debug does not work with --contains :)
 builtin/describe.c |   2 +
 builtin/name-rev.c | 117 +++++++++++++++++++++++++++++++++++++++++++++--------
 t/t4202-log.sh     |   2 +-
 3 files changed, 103 insertions(+), 18 deletions(-)
-- 
2.12.2.712.gba4c48c431
Junio C Hamano· Mar 29, 2017, 17:15 UTC · re: Michael J Gruber · lore

Re: [PATCH v2 0/3] name-rev sanity

Michael J Gruber <git@grubix.eu> writes:
Show 18 quoted lines
> So here is v2 of the name-rev series, the result of our discussions being:
>
> Junio C Hamano (2):
>   name-rev: refactor logic to see if a new candidate is a better name
>   name-rev: favor describing with tags and use committer date to
>     tiebreak
>
> That second patch is slighty changed as discussed, but still mostly Junio's
>
> Michael J Gruber (1):
>   name-rev: provide debug output
>
> This replaces the patch which documented that --debug does not work with --contains :)
>
>  builtin/describe.c |   2 +
>  builtin/name-rev.c | 117 +++++++++++++++++++++++++++++++++++++++++++++--------
>  t/t4202-log.sh     |   2 +-
>  3 files changed, 103 insertions(+), 18 deletions(-)

The first two applies cleanly to the same base as jc/name-rev that the first two of these patches are meant to replace, but the third one doesn't apply on top. Are you depending on something newer?

Junio C Hamano· Mar 29, 2017, 17:43 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 0/3] name-rev sanity

Junio C Hamano <gitster@pobox.com> writes:
> The first two applies cleanly to the same base as jc/name-rev that
> the first two of these patches are meant to replace, but the third
> one doesn't apply on top.  Are you depending on something newer?

Ah, of course, you are depending on your other topic ;-) I'll wiggle these in.

Thanks.
Michael J Gruber· Mar 30, 2017, 13:48 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 0/3] name-rev sanity

Junio C Hamano venit, vidit, dixit 29.03.2017 19:43:
Show 11 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> The first two applies cleanly to the same base as jc/name-rev that
>> the first two of these patches are meant to replace, but the third
>> one doesn't apply on top.  Are you depending on something newer?
> 
> Ah, of course, you are depending on your other topic ;-)
> I'll wiggle these in.
> 
> Thanks.
> 
Yes, sorry, isn't that in next already? I should have meantioned it anyways.

Also, I should split patch 3 into name-rev and describe related parts and update the name-rev documentation. We don't have tests for describe --debug, that should be ok.

Mihael
Junio C Hamano· Mar 30, 2017, 17:30 UTC · re: Michael J Gruber · lore

Re: [PATCH v2 0/3] name-rev sanity

Michael J Gruber <git@grubix.eu> writes:
Show 8 quoted lines
> Junio C Hamano venit, vidit, dixit 29.03.2017 19:43:
> ...
>> Ah, of course, you are depending on your other topic ;-)
>> I'll wiggle these in.
>> 
>> Thanks.
>
> Yes, sorry, isn't that in next already? I should have meantioned it anyways.
No worries.  jc/name-rev was meant to be replaced.
> Also, I should split patch 3 into name-rev and describe related parts
> and update the name-rev documentation. We don't have tests for describe
> --debug, that should be ok.

OK. I do not think these three patches are in 'next' yet, so feel free to further replace the last one (or for that matter the first two as necessary). I do not think we want to cast the debugging information in stone, but it would not hurt if you made sure that "describe --debug [--contains]" does not dump core ;-)

Thanks.
Michael J Gruber· Mar 31, 2017, 13:51 UTC · re: Junio C Hamano · lore

[PATCH v3 3/4] name-rev: provide debug output

Currently, `git describe --contains --debug` does not create any debug output because it does not pass the flag down to `git name-rev`, which does not know that flag.

Teach the latter that flag, so that the former can pass it down (in the following commit).

The output is patterned after that of `git describe --debug`, with the following differences:

describe loops over all args to describe, then over all possible descriptions; name-rev does it the other way round. Therefore, we need to amend each possible description by the arg that it is for (and we leave out the "searching to describe" header).

The date cut-off for name-rev kicks in way more often than the candidate number cut-off of describe, so we do not clutter the output with the cut-off.

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 Documentation/git-name-rev.txt |  5 ++++
 builtin/name-rev.c             | 64 +++++++++++++++++++++++++++++++++---------
 2 files changed, 56 insertions(+), 13 deletions(-)
Show changes to 2 files +56 −13

Documentation/git-name-rev.txt, builtin/name-rev.c

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index e8e68f528c..fd78ee86e8 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -42,6 +42,11 @@ OPTIONS
 --all::
 	List all commits reachable from all refs
 
+--debug::
+	Verbosely display information about the searching strategy
+	being employed to standard error.  The symbolic name will still
+	be printed to standard out.
+
 --stdin::
 	Transform stdin by substituting all the 40-character SHA-1
 	hexes (say $hex) with "$hex ($rev_name)".  When used with
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index bf7ed015ae..51302a79ba 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -18,6 +18,10 @@ typedef struct rev_name {
 
 static long cutoff = LONG_MAX;
 
+static const char *prio_names[] = {
+	N_("head"), N_("lightweight"), N_("annotated"),
+};
+
 /* How many generations are maximally preferred over _one_ merge traversal? */
 #define MERGE_TRAVERSAL_WEIGHT 65535
 
@@ -59,10 +63,19 @@ static int is_better_name(struct rev_name *name,
 	return 0;
 }
 
+struct name_ref_data {
+	int tags_only;
+	int name_only;
+	int debug;
+	struct string_list ref_filters;
+	struct string_list exclude_filters;
+	struct object_array *revs;
+};
+
 static void name_rev(struct commit *commit,
 		const char *tip_name, unsigned long taggerdate,
 		int generation, int distance, int from_tag,
-		int deref)
+		int deref, struct name_ref_data *data)
 {
 	struct rev_name *name = (struct rev_name *)commit->util;
 	struct commit_list *parents;
@@ -75,6 +88,7 @@ static void name_rev(struct commit *commit,
 
 	if (deref) {
 		tip_name = xstrfmt("%s^0", tip_name);
+		from_tag += 1;
 
 		if (generation)
 			die("generation: %d, but deref?", generation);
@@ -87,6 +101,36 @@ static void name_rev(struct commit *commit,
 	} else if (is_better_name(name, tip_name, taggerdate,
 				  generation, distance, from_tag)) {
 copy_data:
+		if (data->debug) {
+			int i;
+			static int label_width = -1;
+			static int name_width = -1;
+			if (label_width < 0) {
+				int w;
+				for (i = 0; i < ARRAY_SIZE(prio_names); i++) {
+					w = strlen(_(prio_names[i]));
+					if (label_width < w)
+						label_width = w;
+				}
+			}
+			if (name_width < 0) {
+				int w;
+				for (i = 0; i < data->revs->nr; i++) {
+					w = strlen(data->revs->objects[i].name);
+					if (name_width < w)
+						name_width = w;
+				}
+			}
+			for (i = 0; i < data->revs->nr; i++)
+				if (!oidcmp(&commit->object.oid,
+					    &data->revs->objects[i].item->oid))
+					fprintf(stderr, " %-*s %8d %-*s %s~%d\n",
+						label_width,
+						_(prio_names[from_tag]),
+						distance, name_width,
+						data->revs->objects[i].name,
+						tip_name, generation);
+		}
 		name->tip_name = tip_name;
 		name->taggerdate = taggerdate;
 		name->generation = generation;
@@ -112,11 +156,11 @@ static void name_rev(struct commit *commit,
 
 			name_rev(parents->item, new_name, taggerdate, 0,
 				 distance + MERGE_TRAVERSAL_WEIGHT,
-				 from_tag, 0);
+				 from_tag, 0, data);
 		} else {
 			name_rev(parents->item, tip_name, taggerdate,
 				 generation + 1, distance + 1,
-				 from_tag, 0);
+				 from_tag, 0, data);
 		}
 	}
 }
@@ -146,13 +190,6 @@ static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)
 	return refname;
 }
 
-struct name_ref_data {
-	int tags_only;
-	int name_only;
-	struct string_list ref_filters;
-	struct string_list exclude_filters;
-};
-
 static struct tip_table {
 	struct tip_table_entry {
 		unsigned char sha1[20];
@@ -236,7 +273,6 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
 	}
 
 	add_to_tip_table(oid->hash, path, can_abbreviate_output);
-
 	while (o && o->type == OBJ_TAG) {
 		struct tag *t = (struct tag *) o;
 		if (!t->tagged)
@@ -253,7 +289,7 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
 			taggerdate = ((struct commit *)o)->date;
 		path = name_ref_abbrev(path, can_abbreviate_output);
 		name_rev(commit, xstrdup(path), taggerdate, 0, 0,
-			 from_tag, deref);
+			 from_tag, deref, data);
 	}
 	return 0;
 }
@@ -383,7 +419,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 {
 	struct object_array revs = OBJECT_ARRAY_INIT;
 	int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
-	struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP };
+	struct name_ref_data data = { 0, 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP };
 	struct option opts[] = {
 		OPT_BOOL(0, "name-only", &data.name_only, N_("print only names (no SHA-1)")),
 		OPT_BOOL(0, "tags", &data.tags_only, N_("only use tags to name the commits")),
@@ -393,6 +429,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 				   N_("ignore refs matching <pattern>")),
 		OPT_GROUP(""),
 		OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
+		OPT_BOOL(0, "debug", &data.debug, N_("debug search strategy on stderr")),
 		OPT_BOOL(0, "stdin", &transform_stdin, N_("read from stdin")),
 		OPT_BOOL(0, "undefined", &allow_undefined, N_("allow to print `undefined` names (default)")),
 		OPT_BOOL(0, "always",     &always,
@@ -458,6 +495,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 
 	if (cutoff)
 		cutoff = cutoff - CUTOFF_DATE_SLOP;
+	data.revs = &revs;
 	for_each_ref(name_ref, &data);
 
 	if (transform_stdin) {
-- 
2.12.2.739.gfc3eb97820
Junio C Hamano· Mar 31, 2017, 16:52 UTC · re: Michael J Gruber · lore

Re: [PATCH v3 3/4] name-rev: provide debug output

Michael J Gruber <git@grubix.eu> writes:
Show 21 quoted lines
> Currently, `git describe --contains --debug` does not create any debug
> output because it does not pass the flag down to `git name-rev`, which
> does not know that flag.
>
> Teach the latter that flag, so that the former can pass it down (in
> the following commit).
>
> The output is patterned after that of `git describe --debug`, with the
> following differences:
>
> describe loops over all args to describe, then over all possible
> descriptions; name-rev does it the other way round. Therefore, we need
> to amend each possible description by the arg that it is for (and we
> leave out the "searching to describe" header).
>
> The date cut-off for name-rev kicks in way more often than the candidate
> number cut-off of describe, so we do not clutter the output with the
> cut-off.
>
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
Show 13 quoted lines
>  static void name_rev(struct commit *commit,
>  		const char *tip_name, unsigned long taggerdate,
>  		int generation, int distance, int from_tag,
> -		int deref)
> +		int deref, struct name_ref_data *data)
>  {
>  	struct rev_name *name = (struct rev_name *)commit->util;
>  	struct commit_list *parents;
> @@ -75,6 +88,7 @@ static void name_rev(struct commit *commit,
>  
>  	if (deref) {
>  		tip_name = xstrfmt("%s^0", tip_name);
> +		from_tag += 1;

Why this change? I didn't see it explained in the proposed log message. "deref" is true only when our immediate caller is the one that inspected the object at the tip and found it to be a tag object (i.e. not a lightweight tag or a branch). from_tag is about "is the tip within refs/tags/ hierarchy? Yes/No?" and such a caller will set it appropriately when calling us. This function just passes it down when it recursively calls itself.

We shouldn't be mucking with that value ourselves here, should we?

The only case that this change may make a difference I can think of is when you have a tag object pointed at from outside refs/tags (e.g. refs/heads/foo is a tag object); if you are trying to change the definition of "from_tag" from the current "Is the tip inside refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag object anywhere?", that may be a good change (I didn't think things through, though), but that shouldn't be hidden inside a commit that claims to only add support for debugging.

What problem are you solving?  
Show 8 quoted lines
> @@ -236,7 +273,6 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
>  	}
>  
>  	add_to_tip_table(oid->hash, path, can_abbreviate_output);
> -
>  	while (o && o->type == OBJ_TAG) {
>  		struct tag *t = (struct tag *) o;
>  		if (!t->tagged)
This is a patch noise we can do without.
Thanks.
Junio C Hamano· Mar 31, 2017, 16:55 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 3/4] name-rev: provide debug output

Junio C Hamano <gitster@pobox.com> writes:
Show 8 quoted lines
> The only case that this change may make a difference I can think of
> is when you have a tag object pointed at from outside refs/tags
> (e.g. refs/heads/foo is a tag object); if you are trying to change
> the definition of "from_tag" from the current "Is the tip inside
> refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag
> object anywhere?", that may be a good change (I didn't think things
> through, though), but that shouldn't be hidden inside a commit that
> claims to only add support for debugging.

And if that "a tag object outside refs/tags/" is what you are solving, I think a better place to do it is in name_ref().

Instead of saying "from_tag is true iff it starts with refs/tags/", you'd say "... or deref is set to true, because we know that the original was a tag object in that case".

Show 14 quoted lines
> What problem are you solving?  
>
>> @@ -236,7 +273,6 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
>>  	}
>>  
>>  	add_to_tip_table(oid->hash, path, can_abbreviate_output);
>> -
>>  	while (o && o->type == OBJ_TAG) {
>>  		struct tag *t = (struct tag *) o;
>>  		if (!t->tagged)
>
> This is a patch noise we can do without.
>
> Thanks.
Michael J Gruber· Mar 31, 2017, 18:02 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 3/4] name-rev: provide debug output

Am 31. März 2017 18:52:16 MESZ schrieb Junio C Hamano <gitster@pobox.com>:
Show 63 quoted lines
>Michael J Gruber <git@grubix.eu> writes:
>
>> Currently, `git describe --contains --debug` does not create any
>debug
>> output because it does not pass the flag down to `git name-rev`,
>which
>> does not know that flag.
>>
>> Teach the latter that flag, so that the former can pass it down (in
>> the following commit).
>>
>> The output is patterned after that of `git describe --debug`, with
>the
>> following differences:
>>
>> describe loops over all args to describe, then over all possible
>> descriptions; name-rev does it the other way round. Therefore, we
>need
>> to amend each possible description by the arg that it is for (and we
>> leave out the "searching to describe" header).
>>
>> The date cut-off for name-rev kicks in way more often than the
>candidate
>> number cut-off of describe, so we do not clutter the output with the
>> cut-off.
>>
>> Signed-off-by: Michael J Gruber <git@grubix.eu>
>> ---
>
>>  static void name_rev(struct commit *commit,
>>  		const char *tip_name, unsigned long taggerdate,
>>  		int generation, int distance, int from_tag,
>> -		int deref)
>> +		int deref, struct name_ref_data *data)
>>  {
>>  	struct rev_name *name = (struct rev_name *)commit->util;
>>  	struct commit_list *parents;
>> @@ -75,6 +88,7 @@ static void name_rev(struct commit *commit,
>>  
>>  	if (deref) {
>>  		tip_name = xstrfmt("%s^0", tip_name);
>> +		from_tag += 1;
>
>Why this change?  I didn't see it explained in the proposed log
>message.  "deref" is true only when our immediate caller is the one
>that inspected the object at the tip and found it to be a tag object
>(i.e. not a lightweight tag or a branch).  from_tag is about "is the
>tip within refs/tags/ hierarchy?  Yes/No?" and such a caller will
>set it appropriately when calling us.  This function just passes it
>down when it recursively calls itself.  
>
>We shouldn't be mucking with that value ourselves here, should we?
>
>The only case that this change may make a difference I can think of
>is when you have a tag object pointed at from outside refs/tags
>(e.g. refs/heads/foo is a tag object); if you are trying to change
>the definition of "from_tag" from the current "Is the tip inside
>refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag
>object anywhere?", that may be a good change (I didn't think things
>through, though), but that shouldn't be hidden inside a commit that
>claims to only add support for debugging.
>
>What problem are you solving?  
Sorry, I forgot about that change and failed to mention it.
It makes no difference in the non-debug case which cares about the Boolean only. In the debug case, I want to distinguish between annotated and lightweight tags, just like describe --debug does. By adding 1 via deref and passing this down, I know that an annotated tag gets the value 2, a lightweight tag 1 and everything else 0, just like describe --tags.
Show 13 quoted lines
>> @@ -236,7 +273,6 @@ static int name_ref(const char *path, const
>struct object_id *oid, int flags, vo
>>  	}
>>  
>>  	add_to_tip_table(oid->hash, path, can_abbreviate_output);
>> -
>>  	while (o && o->type == OBJ_TAG) {
>>  		struct tag *t = (struct tag *) o;
>>  		if (!t->tagged)
>
>This is a patch noise we can do without.
>
>Thanks.
Junio C Hamano· Mar 31, 2017, 18:06 UTC · re: Michael J Gruber · lore

Re: [PATCH v3 3/4] name-rev: provide debug output

Michael J Gruber <git@grubix.eu> writes:
Show 19 quoted lines
>>The only case that this change may make a difference I can think of
>>is when you have a tag object pointed at from outside refs/tags
>>(e.g. refs/heads/foo is a tag object); if you are trying to change
>>the definition of "from_tag" from the current "Is the tip inside
>>refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag
>>object anywhere?", that may be a good change (I didn't think things
>>through, though), but that shouldn't be hidden inside a commit that
>>claims to only add support for debugging.
>>
>>What problem are you solving?  
>
> Sorry, I forgot about that change and failed to mention it.
>
> It makes no difference in the non-debug case which cares about the
> Boolean only. In the debug case, I want to distinguish between
> annotated and lightweight tags, just like describe --debug does. By
> adding 1 via deref and passing this down, I know that an annotated tag
> gets the value 2, a lightweight tag 1 and everything else 0, just like
> describe --tags.

So it sounds like you meant to do something else, and the implementation is wrong for that something else (i.e. it wouldn't do the right thing for a tag object outside refs/tags/, with or without the "--debug" option passed).

Show 13 quoted lines
>>> @@ -236,7 +273,6 @@ static int name_ref(const char *path, const
>>struct object_id *oid, int flags, vo
>>>  	}
>>>  
>>>  	add_to_tip_table(oid->hash, path, can_abbreviate_output);
>>> -
>>>  	while (o && o->type == OBJ_TAG) {
>>>  		struct tag *t = (struct tag *) o;
>>>  		if (!t->tagged)
>>
>>This is a patch noise we can do without.
>>
>>Thanks.
Junio C Hamano· Mar 31, 2017, 18:33 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 3/4] name-rev: provide debug output

Junio C Hamano <gitster@pobox.com> writes:
Show 26 quoted lines
> Michael J Gruber <git@grubix.eu> writes:
>
>>>The only case that this change may make a difference I can think of
>>>is when you have a tag object pointed at from outside refs/tags
>>>(e.g. refs/heads/foo is a tag object); if you are trying to change
>>>the definition of "from_tag" from the current "Is the tip inside
>>>refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag
>>>object anywhere?", that may be a good change (I didn't think things
>>>through, though), but that shouldn't be hidden inside a commit that
>>>claims to only add support for debugging.
>>>
>>>What problem are you solving?  
>>
>> Sorry, I forgot about that change and failed to mention it.
>>
>> It makes no difference in the non-debug case which cares about the
>> Boolean only. In the debug case, I want to distinguish between
>> annotated and lightweight tags, just like describe --debug does. By
>> adding 1 via deref and passing this down, I know that an annotated tag
>> gets the value 2, a lightweight tag 1 and everything else 0, just like
>> describe --tags.
>
> So it sounds like you meant to do something else, and the
> implementation is wrong for that something else (i.e. it wouldn't do
> the right thing for a tag object outside refs/tags/, with or without
> the "--debug" option passed).
The damage seems worse, but I may be misreading the code.

is_better_name() compares name->from_tag and from_tag numerically, because it was designed to take a boolean view of that variable. Now, an artificially bumped 2 gets compared with name->from_tag that may be 1 and gets different priority. That artificially inflated value may be propagated to name->from_tag when the current tip is judged as a better basis for naming the object.

If this change is only for debugging, perhaps inside if(data->debug) you added, instead of looking at from_tag, you can look at both from_tag and deref to choose which prio-nmes to show, without butchering the value in from_tag variable to affect the existing code that is exercised with or without --debug?

Michael J Gruber· Apr 3, 2017, 14:46 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 3/4] name-rev: provide debug output

Junio C Hamano venit, vidit, dixit 31.03.2017 20:33:
Show 37 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Michael J Gruber <git@grubix.eu> writes:
>>
>>>> The only case that this change may make a difference I can think of
>>>> is when you have a tag object pointed at from outside refs/tags
>>>> (e.g. refs/heads/foo is a tag object); if you are trying to change
>>>> the definition of "from_tag" from the current "Is the tip inside
>>>> refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag
>>>> object anywhere?", that may be a good change (I didn't think things
>>>> through, though), but that shouldn't be hidden inside a commit that
>>>> claims to only add support for debugging.
>>>>
>>>> What problem are you solving?  
>>>
>>> Sorry, I forgot about that change and failed to mention it.
>>>
>>> It makes no difference in the non-debug case which cares about the
>>> Boolean only. In the debug case, I want to distinguish between
>>> annotated and lightweight tags, just like describe --debug does. By
>>> adding 1 via deref and passing this down, I know that an annotated tag
>>> gets the value 2, a lightweight tag 1 and everything else 0, just like
>>> describe --tags.
>>
>> So it sounds like you meant to do something else, and the
>> implementation is wrong for that something else (i.e. it wouldn't do
>> the right thing for a tag object outside refs/tags/, with or without
>> the "--debug" option passed).
> 
> The damage seems worse, but I may be misreading the code.
> 
> is_better_name() compares name->from_tag and from_tag numerically,
> because it was designed to take a boolean view of that variable.
> Now, an artificially bumped 2 gets compared with name->from_tag that
> may be 1 and gets different priority.  That artificially inflated
> value may be propagated to name->from_tag when the current tip is
> judged as a better basis for naming the object.
No, I checked not to change the existing behaviour.

If you look at the comment above that then you see that one of the sides of the comparison is a non-tag, so we compare 0 to 1 or 2, the boolean outcome being the same.

Show 5 quoted lines
> If this change is only for debugging, perhaps inside if(data->debug)
> you added, instead of looking at from_tag, you can look at both
> from_tag and deref to choose which prio-nmes to show, without
> butchering the value in from_tag variable to affect the existing
> code that is exercised with or without --debug?

What I did overlook, though, was that name-rev uses the notion "under refs/tags" for "being a tag".

In fact, it's puzzling how different describe and name-rev proceed and weigh the alternatives. I didn't mean to change that.

In retrospect, displaying the "same" debug information for the two commands doesn't make too much sense as long as they use different information. name-rev does-not distinguish between tag types, so why even display it?

I think I should change 3/4 to display exactly those bits that name-rev actually uses for weighing different possible descriptions; they are differents from the "describe-bits". So please withhold 3/4 and 4/4.

Michael
Junio C Hamano· Apr 3, 2017, 17:07 UTC · re: Michael J Gruber · lore

Re: [PATCH v3 3/4] name-rev: provide debug output

Michael J Gruber <git@grubix.eu> writes:
Show 5 quoted lines
> No, I checked not to change the existing behaviour.
>
> If you look at the comment above that then you see that one of the sides
> of the comparison is a non-tag, so we compare 0 to 1 or 2, the boolean
> outcome being the same.

That one I understand, but if you compare 1 and 2 (one side being a lightweight, the other being an annotated tag), they no longer compare equal, no?

Junio C Hamano· May 20, 2017, 05:19 UTC · re: Michael J Gruber · lore

Re: [PATCH v3 3/4] name-rev: provide debug output

Michael J Gruber <git@grubix.eu> writes:
> I think I should change 3/4 to display exactly those bits that name-rev
> actually uses for weighing different possible descriptions; they are
> differents from the "describe-bits". So please withhold 3/4 and 4/4.

OK, I think 1&2/4 from this series can progress before that as it is an end-user visible improvement. While looking at it, I also found a variable that recent "timestamp_t" series didn't notice and update, so perhaps 1&2/4 needs to be rebased on a fix for that anyway ;-)

Thanks.  Will hold 3&4/4 for now.
Junio C Hamano· Mar 31, 2017, 19:10 UTC · re: Michael J Gruber · lore

Re: [PATCH v3 3/4] name-rev: provide debug output

Michael J Gruber <git@grubix.eu> writes:
Show 10 quoted lines
>>What problem are you solving?  
>
> Sorry, I forgot about that change and failed to mention it.
>
> It makes no difference in the non-debug case which cares about the
> Boolean only. In the debug case, I want to distinguish between
> annotated and lightweight tags, just like describe --debug
> does. By adding 1 via deref and passing this down, I know that an
> annotated tag gets the value 2, a lightweight tag 1 and everything
> else 0, just like describe --tags.

If you want to only affect debug display, perhaps you should start with a patch like the attached, before adding any debug code as a preparatory step. Then add your debugging thing, _WITHOUT_ the increment in name_rev(), that uses from_tag to choose between lightweight and annotated as a separate step.

When we decide that it would make sense to give precedence to annotated ones over lightweight ones in is_better_name(), the comparison can be further tweaked to actually compare values of the from_tag thing in *name and the current candidate. That would have to be a separate step, as it changes the semantics (I suspect it would be a better change but it may not be).

How does that sound?
-- >8 --
Subject: name-rev: allow to tell annotated and lightweight tags apart

We do not use this feature yet, but from_tag that is passed around and kept in the rev_name structure now takes three values, instead of a boolean "did this come from refs/tags/ hierarchy?". A new value '2' is "this is an annotated tag that came from refs/tags/ hierarchy".

---
 builtin/name-rev.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Show changes to builtin/name-rev.c +6 −3
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index bf7ed015ae..fe2d306e7c 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -41,7 +41,7 @@ static int is_better_name(struct rev_name *name,
 	 * We know that at least one of them is a non-tag at this point.
 	 * favor a tag over a non-tag.
 	 */
-	if (name->from_tag != from_tag)
+	if (!!name->from_tag != !!from_tag)
 		return from_tag;
 
 	/*
@@ -247,8 +247,11 @@ 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;
-		int from_tag = starts_with(path, "refs/tags/");
-
+		int from_tag;
+		if (starts_with(path, "refs/tags/"))
+			from_tag = 1 + deref;
+		else
+			from_tag = 0;
 		if (taggerdate == ULONG_MAX)
 			taggerdate = ((struct commit *)o)->date;
 		path = name_ref_abbrev(path, can_abbreviate_output);
Michael J Gruber· Mar 31, 2017, 13:51 UTC · re: Junio C Hamano · lore

[PATCH v3 4/4] describe: pass --debug down to name-rev

Now that name-rev knows --debug, pass that flag down to name-rev when we call it for doing describe --contains.

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 builtin/describe.c | 2 ++
 1 file changed, 2 insertions(+)
Show changes to builtin/describe.c +2 −0
diff --git a/builtin/describe.c b/builtin/describe.c
index a5cd8c513f..30196793f0 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -486,6 +486,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 				 NULL);
 		if (always)
 			argv_array_push(&args, "--always");
+		if (debug)
+			argv_array_push(&args, "--debug");
 		if (!all) {
 			argv_array_push(&args, "--tags");
 			for_each_string_list_item(item, &patterns)
-- 
2.12.2.739.gfc3eb97820
Michael J Gruber· Mar 31, 2017, 13:51 UTC · re: Junio C Hamano · lore

[PATCH v3 0/4] name-rev sanity

v3 splits the old 3/3 into name-rev and describe related parts and adds documentation.

No core dumps encountered so far ;)
Did I mention this is on top of mg/describe-debug-l10n (next)?
Junio C Hamano (2):
  name-rev: refactor logic to see if a new candidate is a better name
  name-rev: favor describing with tags and use committer date to
    tiebreak
Michael J Gruber (2):
  name-rev: provide debug output
  describe: pass --debug down to name-rev
 Documentation/git-name-rev.txt |   5 ++
 builtin/describe.c             |   2 +
 builtin/name-rev.c             | 117 +++++++++++++++++++++++++++++++++++------
 t/t4202-log.sh                 |   2 +-
 4 files changed, 108 insertions(+), 18 deletions(-)
-- 
2.12.2.739.gfc3eb97820
Michael J Gruber· Mar 31, 2017, 13:51 UTC · re: Junio C Hamano · lore

[PATCH v3 1/4] name-rev: refactor logic to see if a new candidate is a better name

From: Junio C Hamano <gitster@pobox.com>

When we encounter a new ref that could describe the commit we are looking at, we compare the name that is formed using that ref and the name we found so far and pick a better one.

Factor the comparison logic out to a separate helper function, while keeping the current logic the same (i.e. a name that is based on an older tag is better, and if two tags of the same age can reach the commit, the one with fewer number of hops to reach the commit is better).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 builtin/name-rev.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
Show changes to builtin/name-rev.c +13 −3
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 8bdc3eaa6f..7890f826ce 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -20,6 +20,17 @@ static long cutoff = LONG_MAX;
 /* How many generations are maximally preferred over _one_ merge traversal? */
 #define MERGE_TRAVERSAL_WEIGHT 65535
 
+static int is_better_name(struct rev_name *name,
+			  const char *tip_name,
+			  unsigned long taggerdate,
+			  int generation,
+			  int distance)
+{
+	return (name->taggerdate > taggerdate ||
+		(name->taggerdate == taggerdate &&
+		 name->distance > distance));
+}
+
 static void name_rev(struct commit *commit,
 		const char *tip_name, unsigned long taggerdate,
 		int generation, int distance,
@@ -45,9 +56,8 @@ static void name_rev(struct commit *commit,
 		name = xmalloc(sizeof(rev_name));
 		commit->util = name;
 		goto copy_data;
-	} else if (name->taggerdate > taggerdate ||
-			(name->taggerdate == taggerdate &&
-			 name->distance > distance)) {
+	} else if (is_better_name(name, tip_name, taggerdate,
+				  generation, distance)) {
 copy_data:
 		name->tip_name = tip_name;
 		name->taggerdate = taggerdate;
-- 
2.12.2.739.gfc3eb97820
Michael J Gruber· Mar 31, 2017, 13:51 UTC · re: Junio C Hamano · lore

[PATCH v3 2/4] name-rev: favor describing with tags and use committer date to tiebreak

From: Junio C Hamano <gitster@pobox.com>

"git name-rev" assigned a phony "far in the future" date to tips of refs that are not pointing at tag objects, and favored names based on a ref with the oldest date. This made it almost impossible for an unannotated tags and branches to be counted as a viable base, which was especially problematic when the command is run with the "--tags" option. If an unannotated tag that points at an ancient commit and an annotated tag that points at a much newer commit reaches the commit that is being named, the old unannotated tag was ignored.

Update the "taggerdate" field of the rev-name structure, which is initialized from the tip of ref, to have the committer date if the object at the tip of ref is a commit, not a tag, so that we can optionally take it into account when doing "is this name better?" comparison logic.

When "name-rev" is run without the "--tags" option, the general expectation is still to name the commit based on a tag if possible, but use non-tag refs as fallback, and tiebreak among these non-tag refs by favoring names with shorter hops from the tip. The use of a phony "far in the future" date in the original code was an effective way to ensure this expectation is held: a non-tag tip gets the same "far in the future" timestamp, giving precedence to tags, and among non-tag tips, names with shorter hops are preferred over longer hops, without taking the "taggerdate" into account. As we are taking over the "taggerdate" field to store the committer date for tips with commits:

 (1) keep the original logic when comparing names based on two refs
     both of which are from refs/tags/;
 (2) favoring a name based on a ref in refs/tags/ hierarchy over
     a ref outside the hierarchy;
 (3) between two names based on a ref both outside refs/tags/, give
     precedence to a name with shorter hops and use "taggerdate"
     only to tie-break.

A change to t4202 is a natural consequence. The test creates a commit on a branch "side" and points at it with an unannotated tag "refs/tags/side-2". The original code couldn't decide which one to favor at all, and gave a name based on a branch (simply because refs/heads/side sorts earlier than refs/tags/side-2). Because the updated logic is taught to favor refs in refs/tags/ hierarchy, the the test is updated to expect to see tags/side-2 instead.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * I am sure others can add better heurisitics in is_better_name()
   for comparing names based on non-tag refs, and I do not mind
   people disagreeing with the logic that this patch happens to
   implement.  This is done primarily to illustrate the value of
   using a separate helper function is_better_name() instead of
   open-coding the selection logic in name_rev() function.
Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 builtin/name-rev.c | 53 ++++++++++++++++++++++++++++++++++++++++++++---------
 t/t4202-log.sh     |  2 +-
 2 files changed, 45 insertions(+), 10 deletions(-)
Show changes to 2 files +45 −10

builtin/name-rev.c, t/t4202-log.sh

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 7890f826ce..bf7ed015ae 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -13,6 +13,7 @@ typedef struct rev_name {
 	unsigned long taggerdate;
 	int generation;
 	int distance;
+	int from_tag;
 } rev_name;
 
 static long cutoff = LONG_MAX;
@@ -24,16 +25,43 @@ static int is_better_name(struct rev_name *name,
 			  const char *tip_name,
 			  unsigned long taggerdate,
 			  int generation,
-			  int distance)
+			  int distance,
+			  int from_tag)
 {
-	return (name->taggerdate > taggerdate ||
-		(name->taggerdate == taggerdate &&
-		 name->distance > distance));
+	/*
+	 * When comparing names based on tags, prefer names
+	 * based on the older tag, even if it is farther away.
+	 */
+	if (from_tag && name->from_tag)
+		return (name->taggerdate > taggerdate ||
+			(name->taggerdate == taggerdate &&
+			 name->distance > distance));
+
+	/*
+	 * We know that at least one of them is a non-tag at this point.
+	 * favor a tag over a non-tag.
+	 */
+	if (name->from_tag != from_tag)
+		return from_tag;
+
+	/*
+	 * We are now looking at two non-tags.  Tiebreak to favor
+	 * shorter hops.
+	 */
+	if (name->distance != distance)
+		return name->distance > distance;
+
+	/* ... or tiebreak to favor older date */
+	if (name->taggerdate != taggerdate)
+		return name->taggerdate > taggerdate;
+
+	/* keep the current one if we cannot decide */
+	return 0;
 }
 
 static void name_rev(struct commit *commit,
 		const char *tip_name, unsigned long taggerdate,
-		int generation, int distance,
+		int generation, int distance, int from_tag,
 		int deref)
 {
 	struct rev_name *name = (struct rev_name *)commit->util;
@@ -57,12 +85,13 @@ static void name_rev(struct commit *commit,
 		commit->util = name;
 		goto copy_data;
 	} else if (is_better_name(name, tip_name, taggerdate,
-				  generation, distance)) {
+				  generation, distance, from_tag)) {
 copy_data:
 		name->tip_name = tip_name;
 		name->taggerdate = taggerdate;
 		name->generation = generation;
 		name->distance = distance;
+		name->from_tag = from_tag;
 	} else
 		return;
 
@@ -82,10 +111,12 @@ static void name_rev(struct commit *commit,
 						   parent_number);
 
 			name_rev(parents->item, new_name, taggerdate, 0,
-				distance + MERGE_TRAVERSAL_WEIGHT, 0);
+				 distance + MERGE_TRAVERSAL_WEIGHT,
+				 from_tag, 0);
 		} else {
 			name_rev(parents->item, tip_name, taggerdate,
-				generation + 1, distance + 1, 0);
+				 generation + 1, distance + 1,
+				 from_tag, 0);
 		}
 	}
 }
@@ -216,9 +247,13 @@ 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;
+		int from_tag = starts_with(path, "refs/tags/");
 
+		if (taggerdate == ULONG_MAX)
+			taggerdate = ((struct commit *)o)->date;
 		path = name_ref_abbrev(path, can_abbreviate_output);
-		name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref);
+		name_rev(commit, xstrdup(path), taggerdate, 0, 0,
+			 from_tag, deref);
 	}
 	return 0;
 }
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index f577990716..4afd8135cd 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -399,7 +399,7 @@ cat > expect <<\EOF
 | |
 | |     Merge branch 'side'
 | |
-| * commit side
+| * commit tags/side-2
 | | Author: A U Thor <author@example.com>
 | |
 | |     side-2
-- 
2.12.2.739.gfc3eb97820
Michael J Gruber· Mar 29, 2017, 14:39 UTC · re: Junio C Hamano · lore

[PATCH v2 1/3] name-rev: refactor logic to see if a new candidate is a better name

From: Junio C Hamano <gitster@pobox.com>

When we encounter a new ref that could describe the commit we are looking at, we compare the name that is formed using that ref and the name we found so far and pick a better one.

Factor the comparison logic out to a separate helper function, while keeping the current logic the same (i.e. a name that is based on an older tag is better, and if two tags of the same age can reach the commit, the one with fewer number of hops to reach the commit is better).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 builtin/name-rev.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
Show changes to builtin/name-rev.c +13 −3
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 8bdc3eaa6f..7890f826ce 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -20,6 +20,17 @@ static long cutoff = LONG_MAX;
 /* How many generations are maximally preferred over _one_ merge traversal? */
 #define MERGE_TRAVERSAL_WEIGHT 65535
 
+static int is_better_name(struct rev_name *name,
+			  const char *tip_name,
+			  unsigned long taggerdate,
+			  int generation,
+			  int distance)
+{
+	return (name->taggerdate > taggerdate ||
+		(name->taggerdate == taggerdate &&
+		 name->distance > distance));
+}
+
 static void name_rev(struct commit *commit,
 		const char *tip_name, unsigned long taggerdate,
 		int generation, int distance,
@@ -45,9 +56,8 @@ static void name_rev(struct commit *commit,
 		name = xmalloc(sizeof(rev_name));
 		commit->util = name;
 		goto copy_data;
-	} else if (name->taggerdate > taggerdate ||
-			(name->taggerdate == taggerdate &&
-			 name->distance > distance)) {
+	} else if (is_better_name(name, tip_name, taggerdate,
+				  generation, distance)) {
 copy_data:
 		name->tip_name = tip_name;
 		name->taggerdate = taggerdate;
-- 
2.12.2.712.gba4c48c431
Michael J Gruber· Mar 29, 2017, 14:39 UTC · re: Junio C Hamano · lore

[PATCH v2 3/3] name-rev: provide debug output

Currently, `git describe --contains --debug` does not create any debug output because it does not pass the flag down to `git name-rev`, which does not know that flag.

Teach the latter that flag and the former to pass it down.

The output is patterned after that of `git describe --debug`, with the following differences:

describe loops over all args to describe, then over all possible descriptions; name-rev does it the other way round. Therefore, we need to amend each possible description by the arg that it is for (and we leave out the "searching to describe" header).

The date cut-off for name-rev kicks in way more often than the candidate number cut-off of describe, so we do not clutter the output with the cut-off.

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 builtin/describe.c |  2 ++
 builtin/name-rev.c | 64 +++++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 53 insertions(+), 13 deletions(-)
Show changes to 2 files +53 −13

builtin/describe.c, builtin/name-rev.c

diff --git a/builtin/describe.c b/builtin/describe.c
index a5cd8c513f..30196793f0 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -486,6 +486,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 				 NULL);
 		if (always)
 			argv_array_push(&args, "--always");
+		if (debug)
+			argv_array_push(&args, "--debug");
 		if (!all) {
 			argv_array_push(&args, "--tags");
 			for_each_string_list_item(item, &patterns)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index bf7ed015ae..51302a79ba 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -18,6 +18,10 @@ typedef struct rev_name {
 
 static long cutoff = LONG_MAX;
 
+static const char *prio_names[] = {
+	N_("head"), N_("lightweight"), N_("annotated"),
+};
+
 /* How many generations are maximally preferred over _one_ merge traversal? */
 #define MERGE_TRAVERSAL_WEIGHT 65535
 
@@ -59,10 +63,19 @@ static int is_better_name(struct rev_name *name,
 	return 0;
 }
 
+struct name_ref_data {
+	int tags_only;
+	int name_only;
+	int debug;
+	struct string_list ref_filters;
+	struct string_list exclude_filters;
+	struct object_array *revs;
+};
+
 static void name_rev(struct commit *commit,
 		const char *tip_name, unsigned long taggerdate,
 		int generation, int distance, int from_tag,
-		int deref)
+		int deref, struct name_ref_data *data)
 {
 	struct rev_name *name = (struct rev_name *)commit->util;
 	struct commit_list *parents;
@@ -75,6 +88,7 @@ static void name_rev(struct commit *commit,
 
 	if (deref) {
 		tip_name = xstrfmt("%s^0", tip_name);
+		from_tag += 1;
 
 		if (generation)
 			die("generation: %d, but deref?", generation);
@@ -87,6 +101,36 @@ static void name_rev(struct commit *commit,
 	} else if (is_better_name(name, tip_name, taggerdate,
 				  generation, distance, from_tag)) {
 copy_data:
+		if (data->debug) {
+			int i;
+			static int label_width = -1;
+			static int name_width = -1;
+			if (label_width < 0) {
+				int w;
+				for (i = 0; i < ARRAY_SIZE(prio_names); i++) {
+					w = strlen(_(prio_names[i]));
+					if (label_width < w)
+						label_width = w;
+				}
+			}
+			if (name_width < 0) {
+				int w;
+				for (i = 0; i < data->revs->nr; i++) {
+					w = strlen(data->revs->objects[i].name);
+					if (name_width < w)
+						name_width = w;
+				}
+			}
+			for (i = 0; i < data->revs->nr; i++)
+				if (!oidcmp(&commit->object.oid,
+					    &data->revs->objects[i].item->oid))
+					fprintf(stderr, " %-*s %8d %-*s %s~%d\n",
+						label_width,
+						_(prio_names[from_tag]),
+						distance, name_width,
+						data->revs->objects[i].name,
+						tip_name, generation);
+		}
 		name->tip_name = tip_name;
 		name->taggerdate = taggerdate;
 		name->generation = generation;
@@ -112,11 +156,11 @@ static void name_rev(struct commit *commit,
 
 			name_rev(parents->item, new_name, taggerdate, 0,
 				 distance + MERGE_TRAVERSAL_WEIGHT,
-				 from_tag, 0);
+				 from_tag, 0, data);
 		} else {
 			name_rev(parents->item, tip_name, taggerdate,
 				 generation + 1, distance + 1,
-				 from_tag, 0);
+				 from_tag, 0, data);
 		}
 	}
 }
@@ -146,13 +190,6 @@ static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)
 	return refname;
 }
 
-struct name_ref_data {
-	int tags_only;
-	int name_only;
-	struct string_list ref_filters;
-	struct string_list exclude_filters;
-};
-
 static struct tip_table {
 	struct tip_table_entry {
 		unsigned char sha1[20];
@@ -236,7 +273,6 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
 	}
 
 	add_to_tip_table(oid->hash, path, can_abbreviate_output);
-
 	while (o && o->type == OBJ_TAG) {
 		struct tag *t = (struct tag *) o;
 		if (!t->tagged)
@@ -253,7 +289,7 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
 			taggerdate = ((struct commit *)o)->date;
 		path = name_ref_abbrev(path, can_abbreviate_output);
 		name_rev(commit, xstrdup(path), taggerdate, 0, 0,
-			 from_tag, deref);
+			 from_tag, deref, data);
 	}
 	return 0;
 }
@@ -383,7 +419,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 {
 	struct object_array revs = OBJECT_ARRAY_INIT;
 	int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
-	struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP };
+	struct name_ref_data data = { 0, 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP };
 	struct option opts[] = {
 		OPT_BOOL(0, "name-only", &data.name_only, N_("print only names (no SHA-1)")),
 		OPT_BOOL(0, "tags", &data.tags_only, N_("only use tags to name the commits")),
@@ -393,6 +429,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 				   N_("ignore refs matching <pattern>")),
 		OPT_GROUP(""),
 		OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
+		OPT_BOOL(0, "debug", &data.debug, N_("debug search strategy on stderr")),
 		OPT_BOOL(0, "stdin", &transform_stdin, N_("read from stdin")),
 		OPT_BOOL(0, "undefined", &allow_undefined, N_("allow to print `undefined` names (default)")),
 		OPT_BOOL(0, "always",     &always,
@@ -458,6 +495,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 
 	if (cutoff)
 		cutoff = cutoff - CUTOFF_DATE_SLOP;
+	data.revs = &revs;
 	for_each_ref(name_ref, &data);
 
 	if (transform_stdin) {
-- 
2.12.2.712.gba4c48c431
Michael J Gruber· Mar 17, 2017, 11:25 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak

Show 23 quoted lines
> hops, without taking the "taggerdate" into account.  As we are
> taking over the "taggerdate" field to store the committer date for
> tips with commits:
> 
>  (1) keep the original logic when comparing names based on two refs
>      both of which are from refs/tags/;
> 
>  (2) favoring a name based on a ref in refs/tags/ hierarchy over
>      a ref outside the hierarchy;
> 
>  (3) between two names based on a ref both outside refs/tags/, give
>      precedence to a name with shorter hops and use "taggerdate"
>      only to tie-break.
> 
> A change to t4202 is a natural consequence.  The test creates a
> commit on a branch "side" and points at it with an unannotated tag
> "refs/tags/side-2".  The original code couldn't decide which one to
> favor at all, and gave a name based on a branch (simply because
> refs/heads/side sorts earlier than refs/tags/side-2).  Because the
> updated logic is taught to favor refs in refs/tags/ hierarchy, the
> the test is updated to expect to see tags/side-2 instead.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

I think that strategy is fine and works as I expected in all tested cases. Thanks!

Show 50 quoted lines
> ---
> 
>  * I am sure others can add better heurisitics in is_better_name()
>    for comparing names based on non-tag refs, and I do not mind
>    people disagreeing with the logic that this patch happens to
>    implement.  This is done primarily to illustrate the value of
>    using a separate helper function is_better_name() instead of
>    open-coding the selection logic in name_rev() function.
> ---
>  builtin/name-rev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++---------
>  t/t4202-log.sh     |  2 +-
>  2 files changed, 49 insertions(+), 10 deletions(-)
> 
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index f64c71d9bc..eac0180c62 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -13,6 +13,7 @@ typedef struct rev_name {
>  	unsigned long taggerdate;
>  	int generation;
>  	int distance;
> +	int from_tag;
>  } rev_name;
>  
>  static long cutoff = LONG_MAX;
> @@ -24,16 +25,47 @@ static int is_better_name(struct rev_name *name,
>  			  const char *tip_name,
>  			  unsigned long taggerdate,
>  			  int generation,
> -			  int distance)
> +			  int distance,
> +			  int from_tag)
>  {
> -	return (name->taggerdate > taggerdate ||
> -		(name->taggerdate == taggerdate &&
> -		 name->distance > distance));
> +	/*
> +	 * When comparing names based on tags, prefer names
> +	 * based on the older tag, even if it is farther away.
> +	 */
> +	if (from_tag && name->from_tag)
> +		return (name->taggerdate > taggerdate ||
> +			(name->taggerdate == taggerdate &&
> +			 name->distance > distance));
> +
> +#define COMPARE(attribute, smaller_is_better)	 \
> +	if (name->attribute > attribute) \
> +		return smaller_is_better; \
> +	if (name->attribute < attribute) \
> +		return !smaller_is_better

I find that define pretty confusing. On first reading, the "==" case seems to be missing, but that is basically "pass" as one can see in the later code.

Also, the comparison ">" and "<" is used to switch between the cases "tag vs. non-tag" and "non-tag vs. tag" which happen to be encoded by the from_tag attribute beeing "1 vs. 0" resp "0 vs. 1" in the following:

Show 7 quoted lines
> +
> +	/*
> +	 * We know that at least one of them is a non-tag at this point.
> +	 * favor a tag over a non-tag.
> +	 */
> +	COMPARE(from_tag, 0);
> +

But in the next two instances you use it to do "actual" comparisons between distances and dates:

Show 13 quoted lines
> +	/*
> +	 * We are now looking at two non-tags.  Tiebreak to favor
> +	 * shorter hops.
> +	 */
> +	COMPARE(distance, 1);
> +
> +	/* ... or tiebreak to favor older date */
> +	COMPARE(taggerdate, 1);
> +
> +	/* keep the current one if we cannot decide */
> +	return 0;
> +#undef COMPARE
>  }

So, while I do understand that now, I'm wondering whether I will next time ;)

How about something like
/* favor tag over non-tag */
if (name->from_tag != from_tag)
	return from_tag;
at least for the first instance and possibly
/* favor shorter hops for non-tags */
if (name->distance != distance)
	return name->distance > distance;
/* tie-break by date */
if (name->taggerdate != taggerdate)
	return name->taggerdate > taggerdate;
Show 67 quoted lines
>  
>  static void name_rev(struct commit *commit,
>  		const char *tip_name, unsigned long taggerdate,
> -		int generation, int distance,
> +		int generation, int distance, int from_tag,
>  		int deref)
>  {
>  	struct rev_name *name = (struct rev_name *)commit->util;
> @@ -57,12 +89,13 @@ static void name_rev(struct commit *commit,
>  		commit->util = name;
>  		goto copy_data;
>  	} else if (is_better_name(name, tip_name, taggerdate,
> -				  generation, distance)) {
> +				  generation, distance, from_tag)) {
>  copy_data:
>  		name->tip_name = tip_name;
>  		name->taggerdate = taggerdate;
>  		name->generation = generation;
>  		name->distance = distance;
> +		name->from_tag = from_tag;
>  	} else
>  		return;
>  
> @@ -82,10 +115,12 @@ static void name_rev(struct commit *commit,
>  						   parent_number);
>  
>  			name_rev(parents->item, new_name, taggerdate, 0,
> -				distance + MERGE_TRAVERSAL_WEIGHT, 0);
> +				 distance + MERGE_TRAVERSAL_WEIGHT,
> +				 from_tag, 0);
>  		} else {
>  			name_rev(parents->item, tip_name, taggerdate,
> -				generation + 1, distance + 1, 0);
> +				 generation + 1, distance + 1,
> +				 from_tag, 0);
>  		}
>  	}
>  }
> @@ -184,9 +219,13 @@ 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;
> +		int from_tag = starts_with(path, "refs/tags/");
>  
> +		if (taggerdate == ULONG_MAX)
> +			taggerdate = ((struct commit *)o)->date;
>  		path = name_ref_abbrev(path, can_abbreviate_output);
> -		name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref);
> +		name_rev(commit, xstrdup(path), taggerdate, 0, 0,
> +			 from_tag, deref);
>  	}
>  	return 0;
>  }
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 48b55bfd27..038911f230 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -398,7 +398,7 @@ cat > expect <<\EOF
>  | |
>  | |     Merge branch 'side'
>  | |
> -| * commit side
> +| * commit tags/side-2
>  | | Author: A U Thor <author@example.com>
>  | |
>  | |     side-2
> 
Junio C Hamano· Mar 17, 2017, 16:03 UTC · re: Michael J Gruber · lore

Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 16 quoted lines
>> +#define COMPARE(attribute, smaller_is_better)	 \
>> +	if (name->attribute > attribute) \
>> +		return smaller_is_better; \
>> +	if (name->attribute < attribute) \
>> +		return !smaller_is_better
>
> I find that define pretty confusing. On first reading, the "==" case
> seems to be missing, but that is basically "pass" as one can see in the
> later code.
>
> Also, the comparison ">"  and "<" is used to switch between the cases
> "tag vs. non-tag" and "non-tag vs. tag" which happen to be encoded by
> the from_tag attribute beeing "1 vs. 0" resp "0 vs. 1" in the following:
> ...
> But in the next two instances you use it to do "actual" comparisons
> between distances and dates:

That is pretty much deliberate. The macro is designed to compare the attribute and return if one side is strictly better than the other side, and fall through to tie-breaker that follow (i.e. lack of "==" is very deliberate). Also it is not "return if one side is strictly smaller"---the second parameter decides if smaller means better or bigger means better (i.e. "from_tag" being 1 for a name based on tag and 0 for a name based on non-tag can use the macro by telling that the side that is strictly bigger is better).

> How about something like
> ...

I did a sequence of COMPARE() macros that cascade to implement tie-breaking logic because I thought it was the easiest to read and understand, and I did "tag vs non-tag first decides, and then tiebreak to favor shorter hops and then tiebreak on date, while paying attention to committer dates even when we are comparing two commits", because I thought that would be one of the easier logic to explain to the users.

But this topic is not my itch, and quite honestly I'd be happier if you took it over, improving both the implementation and the semantics it implements, following your best judgment. You are equipped with better motivation than I am to make these decisions.

This patch has turned up a bug in git-p4 in that it seems to have misunderstood that "name-rev HEAD" is a good way to learn which branch we are currently on (it never is); as far as I am concerned, it has served its purpose ;-)

Thanks.
Junio C Hamano· Mar 15, 2017, 22:50 UTC · re: Junio C Hamano · lore

[PATCH 1/2] name-rev: refactor logic to see if a new candidate is a better name

When we encounter a new ref that could describe the commit we are looking at, we compare the name that is formed using that ref and the name we found so far and pick a better one.

Factor the comparison logic out to a separate helper function, while keeping the current logic the same (i.e. a name that is based on an older tag is better, and if two tags of the same age can reach the commit, the one with fewer number of hops to reach the commit is better).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/name-rev.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
Show changes to builtin/name-rev.c +13 −3
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index cd89d48b65..f64c71d9bc 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -20,6 +20,17 @@ static long cutoff = LONG_MAX;
 /* How many generations are maximally preferred over _one_ merge traversal? */
 #define MERGE_TRAVERSAL_WEIGHT 65535
 
+static int is_better_name(struct rev_name *name,
+			  const char *tip_name,
+			  unsigned long taggerdate,
+			  int generation,
+			  int distance)
+{
+	return (name->taggerdate > taggerdate ||
+		(name->taggerdate == taggerdate &&
+		 name->distance > distance));
+}
+
 static void name_rev(struct commit *commit,
 		const char *tip_name, unsigned long taggerdate,
 		int generation, int distance,
@@ -45,9 +56,8 @@ static void name_rev(struct commit *commit,
 		name = xmalloc(sizeof(rev_name));
 		commit->util = name;
 		goto copy_data;
-	} else if (name->taggerdate > taggerdate ||
-			(name->taggerdate == taggerdate &&
-			 name->distance > distance)) {
+	} else if (is_better_name(name, tip_name, taggerdate,
+				  generation, distance)) {
 copy_data:
 		name->tip_name = tip_name;
 		name->taggerdate = taggerdate;
-- 
2.12.0-306-g4a9b9b32d4
Stefan Beller· Mar 16, 2017, 00:14 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags

On Wed, Mar 15, 2017 at 3:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Here is a reroll of what I did in
>
>     http://public-inbox.org/git/xmqqd1die00j.fsf@gitster.mtv.corp.google.com/
>
Both patches look good to me.

Thanks, Stefan

Jacob Keller· Mar 16, 2017, 10:28 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags

On Wed, Mar 15, 2017 at 3:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> Here is a reroll of what I did in
>
>     http://public-inbox.org/git/xmqqd1die00j.fsf@gitster.mtv.corp.google.com/
>
> Junio C Hamano (2):
>   name-rev: refactor logic to see if a new candidate is a better name
>   name-rev: favor describing with tags and use committer date to tiebreak
>
>  builtin/name-rev.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++------
>  t/t4202-log.sh     |  2 +-
>  2 files changed, 57 insertions(+), 8 deletions(-)
>
> --
> 2.12.0-306-g4a9b9b32d4
>

This looks like an improvement to me. It may be that someone comes up with a better definition for is_better_name() but it's definitely much easier to do so now as it's easier to understand what the logic is when it's separated out.

Thanks, Jake

Michael J Gruber· Mar 15, 2017, 13:15 UTC · re: Michael J Gruber · lore

[PATCH 1/3] describe: debug is incompatible with contains

`git describe --contains` calls into `git name-rev` which does not have any searching to do and thus does not display any debug information.

Say so in the documentation and catch the incompatible arguments.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-describe.txt | 2 +-
 builtin/describe.c             | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)
Show changes to 2 files +3 −1

Documentation/git-describe.txt, builtin/describe.c

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index 8755f3af7b..0f9adb6e9a 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -69,7 +69,7 @@ OPTIONS
 --debug::
 	Verbosely display information about the searching strategy
 	being employed to standard error.  The tag name will still
-	be printed to standard out.
+	be printed to standard out. This is incompatible with --contains.
 
 --long::
 	Always output the long format (the tag, the number of commits
diff --git a/builtin/describe.c b/builtin/describe.c
index 76c18059bf..01a6d159a0 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -462,6 +462,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 
 	if (longformat && abbrev == 0)
 		die(_("--long is incompatible with --abbrev=0"));
+	if (contains && debug)
+		die(_("--debug is incompatible with --contains"));
 
 	if (contains) {
 		struct string_list_item *item;
-- 
2.12.0.384.g157040b11f.dirty
Junio C Hamano· Mar 15, 2017, 19:21 UTC · re: Michael J Gruber · lore

Re: [PATCH 1/3] describe: debug is incompatible with contains

Michael J Gruber <git@drmicha.warpmail.net> writes:
> `git describe --contains` calls into `git name-rev` which does not have
> any searching to do and thus does not display any debug information.
>
> Say so in the documentation and catch the incompatible arguments.

I am not sure if this is worth it. Those who are really doing the debugging would be staring at the code while running it anyway, so it is not like this new error condition would help anybody from wasting time scratching her head before viewing the source and realize that the underlying name-rev does not honor the option.

If "--debug" is truly valuable, "name-rev" can gain "--debug" later and then we can pass it down if we want.

Also, it is not like "--debug" is incompatible. It is just the "--contains" codepath is overly silent and does not give any useful information when run in the debug mode. "incompatible" is more like "would not work correctly when both are given", which is not the case here.

Show 33 quoted lines
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  Documentation/git-describe.txt | 2 +-
>  builtin/describe.c             | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
> index 8755f3af7b..0f9adb6e9a 100644
> --- a/Documentation/git-describe.txt
> +++ b/Documentation/git-describe.txt
> @@ -69,7 +69,7 @@ OPTIONS
>  --debug::
>  	Verbosely display information about the searching strategy
>  	being employed to standard error.  The tag name will still
> -	be printed to standard out.
> +	be printed to standard out. This is incompatible with --contains.
>  
>  --long::
>  	Always output the long format (the tag, the number of commits
> diff --git a/builtin/describe.c b/builtin/describe.c
> index 76c18059bf..01a6d159a0 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -462,6 +462,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
>  
>  	if (longformat && abbrev == 0)
>  		die(_("--long is incompatible with --abbrev=0"));
> +	if (contains && debug)
> +		die(_("--debug is incompatible with --contains"));
>  
>  	if (contains) {
>  		struct string_list_item *item;
Michael J Gruber· Mar 17, 2017, 10:54 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] describe: debug is incompatible with contains

Junio C Hamano venit, vidit, dixit 15.03.2017 20:21:
Show 12 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> `git describe --contains` calls into `git name-rev` which does not have
>> any searching to do and thus does not display any debug information.
>>
>> Say so in the documentation and catch the incompatible arguments.
> 
> I am not sure if this is worth it.  Those who are really doing the
> debugging would be staring at the code while running it anyway, so
> it is not like this new error condition would help anybody from
> wasting time scratching her head before viewing the source and
> realize that the underlying name-rev does not honor the option.

The story was: I tried to understand why git describe --contains did not do what I expected. The documentation said that --debug would output candidates, but it did not do anything.

So, instead of learning how git describe --contains works, I got side-tracked into understanding why git describe --contains does not do what the documentation says. That was a waste of time that we can avoid.

"viewing the source" should not be necessary to understand what is going on, should it?

Show 8 quoted lines
> If "--debug" is truly valuable, "name-rev" can gain "--debug" later
> and then we can pass it down if we want.
> 
> Also, it is not like "--debug" is incompatible.  It is just the
> "--contains" codepath is overly silent and does not give any useful
> information when run in the debug mode.  "incompatible" is more like
> "would not work correctly when both are given", which is not the
> case here.

Well, thee notion of giving debug output is certainly not incompatible. But if the "--debug" does not output anything with "--contains" then it is not working, which I would call incompatible (implementation, not concept).

Show 33 quoted lines
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>>  Documentation/git-describe.txt | 2 +-
>>  builtin/describe.c             | 2 ++
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
>> index 8755f3af7b..0f9adb6e9a 100644
>> --- a/Documentation/git-describe.txt
>> +++ b/Documentation/git-describe.txt
>> @@ -69,7 +69,7 @@ OPTIONS
>>  --debug::
>>  	Verbosely display information about the searching strategy
>>  	being employed to standard error.  The tag name will still
>> -	be printed to standard out.
>> +	be printed to standard out. This is incompatible with --contains.
>>  
>>  --long::
>>  	Always output the long format (the tag, the number of commits
>> diff --git a/builtin/describe.c b/builtin/describe.c
>> index 76c18059bf..01a6d159a0 100644
>> --- a/builtin/describe.c
>> +++ b/builtin/describe.c
>> @@ -462,6 +462,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
>>  
>>  	if (longformat && abbrev == 0)
>>  		die(_("--long is incompatible with --abbrev=0"));
>> +	if (contains && debug)
>> +		die(_("--debug is incompatible with --contains"));
>>  
>>  	if (contains) {
>>  		struct string_list_item *item;
Michael J Gruber· Mar 15, 2017, 13:15 UTC · re: Michael J Gruber · lore

[PATCH 2/3] git-prompt: add a describe style for any tags

git-prompt has various describe styles, among them "describe" (by annotated tags) and "default" (by exact match with any tag).

Add a mode "tag" that describes by any tag, annotated or not.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 contrib/completion/git-prompt.sh | 3 +++
 1 file changed, 3 insertions(+)
Show changes to contrib/completion/git-prompt.sh +3 −0
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 97eacd7832..c6cbef38c2 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -82,6 +82,7 @@
 #     contains      relative to newer annotated tag (v1.6.3.2~35)
 #     branch        relative to newer tag or branch (master~4)
 #     describe      relative to older annotated tag (v1.6.3.1-13-gdd42c2f)
+#     tag           relative to any older tag (v1.6.3.1-13-gdd42c2f)
 #     default       exactly matching tag
 #
 # If you would like a colored hint about the current dirty state, set
@@ -443,6 +444,8 @@ __git_ps1 ()
 					git describe --contains HEAD ;;
 				(branch)
 					git describe --contains --all HEAD ;;
+				(tag)
+					git describe --tags HEAD ;;
 				(describe)
 					git describe HEAD ;;
 				(* | default)
-- 
2.12.0.384.g157040b11f.dirty
Junio C Hamano· Mar 15, 2017, 19:25 UTC · re: Michael J Gruber · lore

Re: [PATCH 2/3] git-prompt: add a describe style for any tags

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 19 quoted lines
> git-prompt has various describe styles, among them "describe" (by
> annotated tags) and "default" (by exact match with any tag).
>
> Add a mode "tag" that describes by any tag, annotated or not.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  contrib/completion/git-prompt.sh | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 97eacd7832..c6cbef38c2 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -82,6 +82,7 @@
>  #     contains      relative to newer annotated tag (v1.6.3.2~35)
>  #     branch        relative to newer tag or branch (master~4)
>  #     describe      relative to older annotated tag (v1.6.3.1-13-gdd42c2f)
> +#     tag           relative to any older tag (v1.6.3.1-13-gdd42c2f)
I guess this feature makes sense.  

I just wish we had a well-known unannotated tag we can use for such an example; using v1.6.3.1 that is annotated does not help to make the distinctin between describe and tag stand out. We want to convey "both annotated one and unannotated one can be used".

I am wondering if it makes sense to do something like this instead:
#	tag 	similar to 'describe' but also allow unannotated tags
Show 12 quoted lines
>  #     default       exactly matching tag
>  #
>  # If you would like a colored hint about the current dirty state, set
> @@ -443,6 +444,8 @@ __git_ps1 ()
>  					git describe --contains HEAD ;;
>  				(branch)
>  					git describe --contains --all HEAD ;;
> +				(tag)
> +					git describe --tags HEAD ;;
>  				(describe)
>  					git describe HEAD ;;
>  				(* | default)
Michael J Gruber· Mar 17, 2017, 10:56 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] git-prompt: add a describe style for any tags

Junio C Hamano venit, vidit, dixit 15.03.2017 20:25:
Show 32 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> git-prompt has various describe styles, among them "describe" (by
>> annotated tags) and "default" (by exact match with any tag).
>>
>> Add a mode "tag" that describes by any tag, annotated or not.
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>>  contrib/completion/git-prompt.sh | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
>> index 97eacd7832..c6cbef38c2 100644
>> --- a/contrib/completion/git-prompt.sh
>> +++ b/contrib/completion/git-prompt.sh
>> @@ -82,6 +82,7 @@
>>  #     contains      relative to newer annotated tag (v1.6.3.2~35)
>>  #     branch        relative to newer tag or branch (master~4)
>>  #     describe      relative to older annotated tag (v1.6.3.1-13-gdd42c2f)
>> +#     tag           relative to any older tag (v1.6.3.1-13-gdd42c2f)
> 
> I guess this feature makes sense.  
> 
> I just wish we had a well-known unannotated tag we can use for such
> an example; using v1.6.3.1 that is annotated does not help to make
> the distinctin between describe and tag stand out.  We want to
> convey "both annotated one and unannotated one can be used".
> 
> I am wondering if it makes sense to do something like this instead:
> 
> #	tag 	similar to 'describe' but also allow unannotated tags

or inventing a local lightweight tag such as "lastgood-5-gbadbad"). Either way would be fine with me. (I guess it's in next as is now.)

Show 12 quoted lines
>>  #     default       exactly matching tag
>>  #
>>  # If you would like a colored hint about the current dirty state, set
>> @@ -443,6 +444,8 @@ __git_ps1 ()
>>  					git describe --contains HEAD ;;
>>  				(branch)
>>  					git describe --contains --all HEAD ;;
>> +				(tag)
>> +					git describe --tags HEAD ;;
>>  				(describe)
>>  					git describe HEAD ;;
>>  				(* | default)

← back to recent threads