describe --contains sanity

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

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.

---

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(-)

index 8bdc3eaa6f..75ba7bbad5 100644
@@ -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);
 	}

index 97c9b32c2e..d467d5957d 100755
@@ -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

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.

---
 Documentation/git-describe.txt | 2 +-
 builtin/describe.c             | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

index 8755f3af7b..0f9adb6e9a 100644
@@ -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

index 76c18059bf..01a6d159a0 100644
@@ -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

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.

---
 contrib/completion/git-prompt.sh | 3 +++
 1 file changed, 3 insertions(+)

index 97eacd7832..c6cbef38c2 100644
@@ -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

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

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.

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(-)

index cd89d48b65..ba30c9ca23 100644
@@ -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;
 }

Here is a reroll of what I did in

http://public-inbox.org/git/[email protected]/

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

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

---

* 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(-)

index f64c71d9bc..eac0180c62 100644
@@ -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;
 }

index 48b55bfd27..038911f230 100755
@@ -398,7 +398,7 @@ cat > expect <<\EOF
 | |
 | |     Merge branch 'side'
 | |
-| * commit side
+| * commit tags/side-2
 | | Author: A U Thor <[email protected]>
 | |
 | |     side-2

2.12.0-306-g4a9b9b32d4

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

---
 builtin/name-rev.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

index cd89d48b65..f64c71d9bc 100644
@@ -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

Both patches look good to me.

Thanks,
Stefan

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

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

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.

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(-)

index eab319d76e..351d1ab58e 100755
@@ -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

Junio C Hamano venit, vidit, dixit 15.03.2017 20:25:

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

Junio C Hamano venit, vidit, dixit 15.03.2017 20:21:

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?

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

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

	return from_tag;

	return name->distance > distance;

	return name->taggerdate > taggerdate;

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

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.

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

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.

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.

From: Junio C Hamano <[email protected]>

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

---

* 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 | 53 ++++++++++++++++++++++++++++++++++++++++++++---------
 t/t4202-log.sh     |  2 +-
 2 files changed, 45 insertions(+), 10 deletions(-)

index 7890f826ce..bf7ed015ae 100644
@@ -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;
 }

index f577990716..4afd8135cd 100755
@@ -399,7 +399,7 @@ cat > expect <<\EOF
 | |
 | |     Merge branch 'side'
 | |
-| * commit side
+| * commit tags/side-2
 | | Author: A U Thor <[email protected]>
 | |
 | |     side-2

2.12.2.712.gba4c48c431

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

From: Junio C Hamano <[email protected]>

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

---
 builtin/name-rev.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

index 8bdc3eaa6f..7890f826ce 100644
@@ -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

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.

---
 builtin/describe.c |  2 ++
 builtin/name-rev.c | 64 +++++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 53 insertions(+), 13 deletions(-)

index a5cd8c513f..30196793f0 100644
@@ -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.712.gba4c48c431

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.

Junio C Hamano venit, vidit, dixit 29.03.2017 19:43:

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

No worries. jc/name-rev was meant to be replaced.

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.

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.

---
 Documentation/git-name-rev.txt |  5 ++++
 builtin/name-rev.c             | 64 +++++++++++++++++++++++++++++++++---------
 2 files changed, 56 insertions(+), 13 deletions(-)

index e8e68f528c..fd78ee86e8 100644
@@ -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

2.12.2.739.gfc3eb97820

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

---
 builtin/describe.c | 2 ++
 1 file changed, 2 insertions(+)

index a5cd8c513f..30196793f0 100644
@@ -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

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

From: Junio C Hamano <[email protected]>

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

---
 builtin/name-rev.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

index 8bdc3eaa6f..7890f826ce 100644
@@ -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

From: Junio C Hamano <[email protected]>

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

---

* 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 | 53 ++++++++++++++++++++++++++++++++++++++++++++---------
 t/t4202-log.sh     |  2 +-
 2 files changed, 45 insertions(+), 10 deletions(-)

index 7890f826ce..bf7ed015ae 100644
@@ -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;
 }

index f577990716..4afd8135cd 100755
@@ -399,7 +399,7 @@ cat > expect <<\EOF
 | |
 | |     Merge branch 'side'
 | |
-| * commit side
+| * commit tags/side-2
 | | Author: A U Thor <[email protected]>
 | |
 | |     side-2

2.12.2.739.gfc3eb97820

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

Am 31. März 2017 18:52:16 MESZ schrieb Junio C Hamano <[email protected]>:

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?

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(-)

index bf7ed015ae..fe2d306e7c 100644
@@ -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);

Junio C Hamano venit, vidit, dixit 31.03.2017 20:33:

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.

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

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?

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.