All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] describe --contains sanity
@ 2017-03-15 13:15 Michael J Gruber
  2017-03-15 13:15 ` [PATCH 1/3] describe: debug is incompatible with contains Michael J Gruber
                   ` (2 more replies)
  0 siblings, 3 replies; 46+ messages in thread
From: Michael J Gruber @ 2017-03-15 13:15 UTC (permalink / raw)
  To: git

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


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 1/3] describe: debug is incompatible with contains
  2017-03-15 13:15 [PATCH 0/3] describe --contains sanity Michael J Gruber
@ 2017-03-15 13:15 ` Michael J Gruber
  2017-03-15 19:21   ` Junio C Hamano
  2017-03-15 13:15 ` [PATCH 2/3] git-prompt: add a describe style for any tags Michael J Gruber
  2017-03-15 13:15 ` [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs Michael J Gruber
  2 siblings, 1 reply; 46+ messages in thread
From: Michael J Gruber @ 2017-03-15 13:15 UTC (permalink / raw)
  To: git

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

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


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 2/3] git-prompt: add a describe style for any tags
  2017-03-15 13:15 [PATCH 0/3] describe --contains sanity Michael J Gruber
  2017-03-15 13:15 ` [PATCH 1/3] describe: debug is incompatible with contains Michael J Gruber
@ 2017-03-15 13:15 ` Michael J Gruber
  2017-03-15 19:25   ` Junio C Hamano
  2017-03-15 13:15 ` [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs Michael J Gruber
  2 siblings, 1 reply; 46+ messages in thread
From: Michael J Gruber @ 2017-03-15 13:15 UTC (permalink / raw)
  To: git

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


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs
  2017-03-15 13:15 [PATCH 0/3] describe --contains sanity Michael J Gruber
  2017-03-15 13:15 ` [PATCH 1/3] describe: debug is incompatible with contains Michael J Gruber
  2017-03-15 13:15 ` [PATCH 2/3] git-prompt: add a describe style for any tags Michael J Gruber
@ 2017-03-15 13:15 ` Michael J Gruber
  2017-03-15 16:14   ` Junio C Hamano
  2017-03-15 19:50   ` Junio C Hamano
  2 siblings, 2 replies; 46+ messages in thread
From: Michael J Gruber @ 2017-03-15 13:15 UTC (permalink / raw)
  To: git

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

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


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* Re: [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs
  2017-03-15 13:15 ` [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs Michael J Gruber
@ 2017-03-15 16:14   ` Junio C Hamano
  2017-03-15 19:50   ` Junio C Hamano
  1 sibling, 0 replies; 46+ messages in thread
From: Junio C Hamano @ 2017-03-15 16:14 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git

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

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 1/3] describe: debug is incompatible with contains
  2017-03-15 13:15 ` [PATCH 1/3] describe: debug is incompatible with contains Michael J Gruber
@ 2017-03-15 19:21   ` Junio C Hamano
  2017-03-17 10:54     ` Michael J Gruber
  0 siblings, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2017-03-15 19:21 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git

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.

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 2/3] git-prompt: add a describe style for any tags
  2017-03-15 13:15 ` [PATCH 2/3] git-prompt: add a describe style for any tags Michael J Gruber
@ 2017-03-15 19:25   ` Junio C Hamano
  2017-03-17 10:56     ` Michael J Gruber
  0 siblings, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2017-03-15 19:25 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git

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

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs
  2017-03-15 13:15 ` [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs Michael J Gruber
  2017-03-15 16:14   ` Junio C Hamano
@ 2017-03-15 19:50   ` Junio C Hamano
  2017-03-15 20:51     ` Junio C Hamano
  1 sibling, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2017-03-15 19:50 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git

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

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs
  2017-03-15 19:50   ` Junio C Hamano
@ 2017-03-15 20:51     ` Junio C Hamano
  2017-03-15 22:50       ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Junio C Hamano
  0 siblings, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2017-03-15 20:51 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git

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

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

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;
 }

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags
  2017-03-15 20:51     ` Junio C Hamano
@ 2017-03-15 22:50       ` Junio C Hamano
  2017-03-15 22:50         ` [PATCH 1/2] name-rev: refactor logic to see if a new candidate is a better name Junio C Hamano
                           ` (3 more replies)
  0 siblings, 4 replies; 46+ messages in thread
From: Junio C Hamano @ 2017-03-15 22:50 UTC (permalink / raw)
  To: git; +Cc: Michael J Gruber

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


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 1/2] name-rev: refactor logic to see if a new candidate is a better name
  2017-03-15 22:50       ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Junio C Hamano
@ 2017-03-15 22:50         ` Junio C Hamano
  2017-03-15 22:50         ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Junio C Hamano
                           ` (2 subsequent siblings)
  3 siblings, 0 replies; 46+ messages in thread
From: Junio C Hamano @ 2017-03-15 22:50 UTC (permalink / raw)
  To: git; +Cc: Michael J Gruber

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

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


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak
  2017-03-15 22:50       ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Junio C Hamano
  2017-03-15 22:50         ` [PATCH 1/2] name-rev: refactor logic to see if a new candidate is a better name Junio C Hamano
@ 2017-03-15 22:50         ` Junio C Hamano
  2017-03-17  4:07           ` Lars Schneider
  2017-03-17 11:25           ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber
  2017-03-16  0:14         ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Stefan Beller
  2017-03-16 10:28         ` Jacob Keller
  3 siblings, 2 replies; 46+ messages in thread
From: Junio C Hamano @ 2017-03-15 22:50 UTC (permalink / raw)
  To: git; +Cc: Michael J Gruber

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

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


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* Re: [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags
  2017-03-15 22:50       ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Junio C Hamano
  2017-03-15 22:50         ` [PATCH 1/2] name-rev: refactor logic to see if a new candidate is a better name Junio C Hamano
  2017-03-15 22:50         ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Junio C Hamano
@ 2017-03-16  0:14         ` Stefan Beller
  2017-03-16 10:28         ` Jacob Keller
  3 siblings, 0 replies; 46+ messages in thread
From: Stefan Beller @ 2017-03-16  0:14 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Michael J Gruber

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags
  2017-03-15 22:50       ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Junio C Hamano
                           ` (2 preceding siblings ...)
  2017-03-16  0:14         ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Stefan Beller
@ 2017-03-16 10:28         ` Jacob Keller
  3 siblings, 0 replies; 46+ messages in thread
From: Jacob Keller @ 2017-03-16 10:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git mailing list, Michael J Gruber

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak
  2017-03-15 22:50         ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Junio C Hamano
@ 2017-03-17  4:07           ` Lars Schneider
  2017-03-17  5:45             ` Junio C Hamano
  2017-03-17 11:25           ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber
  1 sibling, 1 reply; 46+ messages in thread
From: Lars Schneider @ 2017-03-17  4:07 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Michael J Gruber


> 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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak
  2017-03-17  4:07           ` Lars Schneider
@ 2017-03-17  5:45             ` Junio C Hamano
  2017-03-17  5:56               ` Junio C Hamano
  0 siblings, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2017-03-17  5:45 UTC (permalink / raw)
  To: Lars Schneider; +Cc: git, Michael J Gruber

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.


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak
  2017-03-17  5:45             ` Junio C Hamano
@ 2017-03-17  5:56               ` Junio C Hamano
  2017-03-17 17:09                 ` Lars Schneider
  0 siblings, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2017-03-17  5:56 UTC (permalink / raw)
  To: Lars Schneider; +Cc: git, Michael J Gruber

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

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* Re: [PATCH 1/3] describe: debug is incompatible with contains
  2017-03-15 19:21   ` Junio C Hamano
@ 2017-03-17 10:54     ` Michael J Gruber
  0 siblings, 0 replies; 46+ messages in thread
From: Michael J Gruber @ 2017-03-17 10:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Junio C Hamano venit, vidit, dixit 15.03.2017 20:21:
> 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?

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

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 2/3] git-prompt: add a describe style for any tags
  2017-03-15 19:25   ` Junio C Hamano
@ 2017-03-17 10:56     ` Michael J Gruber
  0 siblings, 0 replies; 46+ messages in thread
From: Michael J Gruber @ 2017-03-17 10:56 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Junio C Hamano venit, vidit, dixit 15.03.2017 20:25:
> 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.)

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak
  2017-03-15 22:50         ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Junio C Hamano
  2017-03-17  4:07           ` Lars Schneider
@ 2017-03-17 11:25           ` Michael J Gruber
  2017-03-17 16:03             ` Junio C Hamano
  1 sibling, 1 reply; 46+ messages in thread
From: Michael J Gruber @ 2017-03-17 11:25 UTC (permalink / raw)
  To: Junio C Hamano, git

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

> ---
> 
>  * 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:

> +
> +	/*
> +	 * 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:

> +	/*
> +	 * 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;

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak
  2017-03-17 11:25           ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber
@ 2017-03-17 16:03             ` Junio C Hamano
  0 siblings, 0 replies; 46+ messages in thread
From: Junio C Hamano @ 2017-03-17 16:03 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git

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

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak
  2017-03-17  5:56               ` Junio C Hamano
@ 2017-03-17 17:09                 ` Lars Schneider
  2017-03-17 17:17                   ` Junio C Hamano
  0 siblings, 1 reply; 46+ messages in thread
From: Lars Schneider @ 2017-03-17 17:09 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, Michael J Gruber, Luke Diamand


> 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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak
  2017-03-17 17:09                 ` Lars Schneider
@ 2017-03-17 17:17                   ` Junio C Hamano
  2017-03-17 22:43                     ` Junio C Hamano
  0 siblings, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2017-03-17 17:17 UTC (permalink / raw)
  To: Lars Schneider; +Cc: Git Mailing List, Michael J Gruber, Luke Diamand

Lars Schneider <larsxschneider@gmail.com> writes:

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



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak
  2017-03-17 17:17                   ` Junio C Hamano
@ 2017-03-17 22:43                     ` Junio C Hamano
  2017-03-29 14:39                       ` [PATCH v2 0/3] name-rev sanity Michael J Gruber
  0 siblings, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2017-03-17 22:43 UTC (permalink / raw)
  To: Lars Schneider; +Cc: Git Mailing List, Michael J Gruber, Luke Diamand

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

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


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 0/3] name-rev sanity
  2017-03-17 22:43                     ` Junio C Hamano
@ 2017-03-29 14:39                       ` Michael J Gruber
  2017-03-29 14:39                         ` [PATCH v2 1/3] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber
                                           ` (3 more replies)
  0 siblings, 4 replies; 46+ messages in thread
From: Michael J Gruber @ 2017-03-29 14:39 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand

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


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v2 1/3] name-rev: refactor logic to see if a new candidate is a better name
  2017-03-29 14:39                       ` [PATCH v2 0/3] name-rev sanity Michael J Gruber
@ 2017-03-29 14:39                         ` Michael J Gruber
  2017-03-29 14:39                         ` [PATCH v2 2/3] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber
                                           ` (2 subsequent siblings)
  3 siblings, 0 replies; 46+ messages in thread
From: Michael J Gruber @ 2017-03-29 14:39 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand

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

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


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v2 2/3] name-rev: favor describing with tags and use committer date to tiebreak
  2017-03-29 14:39                       ` [PATCH v2 0/3] name-rev sanity Michael J Gruber
  2017-03-29 14:39                         ` [PATCH v2 1/3] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber
@ 2017-03-29 14:39                         ` Michael J Gruber
  2017-03-29 14:39                         ` [PATCH v2 3/3] name-rev: provide debug output Michael J Gruber
  2017-03-29 17:15                         ` [PATCH v2 0/3] name-rev sanity Junio C Hamano
  3 siblings, 0 replies; 46+ messages in thread
From: Michael J Gruber @ 2017-03-29 14:39 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand

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

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


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v2 3/3] name-rev: provide debug output
  2017-03-29 14:39                       ` [PATCH v2 0/3] name-rev sanity Michael J Gruber
  2017-03-29 14:39                         ` [PATCH v2 1/3] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber
  2017-03-29 14:39                         ` [PATCH v2 2/3] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber
@ 2017-03-29 14:39                         ` Michael J Gruber
  2017-03-29 17:15                         ` [PATCH v2 0/3] name-rev sanity Junio C Hamano
  3 siblings, 0 replies; 46+ messages in thread
From: Michael J Gruber @ 2017-03-29 14:39 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand

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

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


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 0/3] name-rev sanity
  2017-03-29 14:39                       ` [PATCH v2 0/3] name-rev sanity Michael J Gruber
                                           ` (2 preceding siblings ...)
  2017-03-29 14:39                         ` [PATCH v2 3/3] name-rev: provide debug output Michael J Gruber
@ 2017-03-29 17:15                         ` Junio C Hamano
  2017-03-29 17:43                           ` Junio C Hamano
  3 siblings, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2017-03-29 17:15 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand

Michael J Gruber <git@grubix.eu> writes:

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 0/3] name-rev sanity
  2017-03-29 17:15                         ` [PATCH v2 0/3] name-rev sanity Junio C Hamano
@ 2017-03-29 17:43                           ` Junio C Hamano
  2017-03-30 13:48                             ` Michael J Gruber
  0 siblings, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2017-03-29 17:43 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand

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.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 0/3] name-rev sanity
  2017-03-29 17:43                           ` Junio C Hamano
@ 2017-03-30 13:48                             ` Michael J Gruber
  2017-03-30 17:30                               ` Junio C Hamano
  0 siblings, 1 reply; 46+ messages in thread
From: Michael J Gruber @ 2017-03-30 13:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Lars Schneider, Luke Diamand

Junio C Hamano venit, vidit, dixit 29.03.2017 19:43:
> 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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 0/3] name-rev sanity
  2017-03-30 13:48                             ` Michael J Gruber
@ 2017-03-30 17:30                               ` Junio C Hamano
  2017-03-31 13:51                                 ` [PATCH v3 0/4] " Michael J Gruber
  0 siblings, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2017-03-30 17:30 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand

Michael J Gruber <git@grubix.eu> writes:

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v3 0/4] name-rev sanity
  2017-03-30 17:30                               ` Junio C Hamano
@ 2017-03-31 13:51                                 ` Michael J Gruber
  2017-03-31 13:51                                   ` [PATCH v3 1/4] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber
                                                     ` (3 more replies)
  0 siblings, 4 replies; 46+ messages in thread
From: Michael J Gruber @ 2017-03-31 13:51 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand

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


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH v3 1/4] name-rev: refactor logic to see if a new candidate is a better name
  2017-03-31 13:51                                 ` [PATCH v3 0/4] " Michael J Gruber
@ 2017-03-31 13:51                                   ` Michael J Gruber
  2017-03-31 13:51                                   ` [PATCH v3 2/4] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber
                                                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 46+ messages in thread
From: Michael J Gruber @ 2017-03-31 13:51 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand

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

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


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v3 2/4] name-rev: favor describing with tags and use committer date to tiebreak
  2017-03-31 13:51                                 ` [PATCH v3 0/4] " Michael J Gruber
  2017-03-31 13:51                                   ` [PATCH v3 1/4] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber
@ 2017-03-31 13:51                                   ` Michael J Gruber
  2017-03-31 13:51                                   ` [PATCH v3 3/4] name-rev: provide debug output Michael J Gruber
  2017-03-31 13:51                                   ` [PATCH v3 4/4] describe: pass --debug down to name-rev Michael J Gruber
  3 siblings, 0 replies; 46+ messages in thread
From: Michael J Gruber @ 2017-03-31 13:51 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand

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

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


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v3 3/4] name-rev: provide debug output
  2017-03-31 13:51                                 ` [PATCH v3 0/4] " Michael J Gruber
  2017-03-31 13:51                                   ` [PATCH v3 1/4] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber
  2017-03-31 13:51                                   ` [PATCH v3 2/4] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber
@ 2017-03-31 13:51                                   ` Michael J Gruber
  2017-03-31 16:52                                     ` Junio C Hamano
  2017-03-31 13:51                                   ` [PATCH v3 4/4] describe: pass --debug down to name-rev Michael J Gruber
  3 siblings, 1 reply; 46+ messages in thread
From: Michael J Gruber @ 2017-03-31 13:51 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand

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

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


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v3 4/4] describe: pass --debug down to name-rev
  2017-03-31 13:51                                 ` [PATCH v3 0/4] " Michael J Gruber
                                                     ` (2 preceding siblings ...)
  2017-03-31 13:51                                   ` [PATCH v3 3/4] name-rev: provide debug output Michael J Gruber
@ 2017-03-31 13:51                                   ` Michael J Gruber
  3 siblings, 0 replies; 46+ messages in thread
From: Michael J Gruber @ 2017-03-31 13:51 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand

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

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


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* Re: [PATCH v3 3/4] name-rev: provide debug output
  2017-03-31 13:51                                   ` [PATCH v3 3/4] name-rev: provide debug output Michael J Gruber
@ 2017-03-31 16:52                                     ` Junio C Hamano
  2017-03-31 16:55                                       ` Junio C Hamano
  2017-03-31 18:02                                       ` Michael J Gruber
  0 siblings, 2 replies; 46+ messages in thread
From: Junio C Hamano @ 2017-03-31 16:52 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand

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?  

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


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v3 3/4] name-rev: provide debug output
  2017-03-31 16:52                                     ` Junio C Hamano
@ 2017-03-31 16:55                                       ` Junio C Hamano
  2017-03-31 18:02                                       ` Michael J Gruber
  1 sibling, 0 replies; 46+ messages in thread
From: Junio C Hamano @ 2017-03-31 16:55 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand

Junio C Hamano <gitster@pobox.com> 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.

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


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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v3 3/4] name-rev: provide debug output
  2017-03-31 16:52                                     ` Junio C Hamano
  2017-03-31 16:55                                       ` Junio C Hamano
@ 2017-03-31 18:02                                       ` Michael J Gruber
  2017-03-31 18:06                                         ` Junio C Hamano
  2017-03-31 19:10                                         ` Junio C Hamano
  1 sibling, 2 replies; 46+ messages in thread
From: Michael J Gruber @ 2017-03-31 18:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Lars Schneider, Luke Diamand

Am 31. März 2017 18:52:16 MESZ schrieb Junio C Hamano <gitster@pobox.com>:
>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.

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


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v3 3/4] name-rev: provide debug output
  2017-03-31 18:02                                       ` Michael J Gruber
@ 2017-03-31 18:06                                         ` Junio C Hamano
  2017-03-31 18:33                                           ` Junio C Hamano
  2017-03-31 19:10                                         ` Junio C Hamano
  1 sibling, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2017-03-31 18:06 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand

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

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v3 3/4] name-rev: provide debug output
  2017-03-31 18:06                                         ` Junio C Hamano
@ 2017-03-31 18:33                                           ` Junio C Hamano
  2017-04-03 14:46                                             ` Michael J Gruber
  0 siblings, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2017-03-31 18:33 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand

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.

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?

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v3 3/4] name-rev: provide debug output
  2017-03-31 18:02                                       ` Michael J Gruber
  2017-03-31 18:06                                         ` Junio C Hamano
@ 2017-03-31 19:10                                         ` Junio C Hamano
  1 sibling, 0 replies; 46+ messages in thread
From: Junio C Hamano @ 2017-03-31 19:10 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand

Michael J Gruber <git@grubix.eu> writes:

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

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

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* Re: [PATCH v3 3/4] name-rev: provide debug output
  2017-03-31 18:33                                           ` Junio C Hamano
@ 2017-04-03 14:46                                             ` Michael J Gruber
  2017-04-03 17:07                                               ` Junio C Hamano
  2017-05-20  5:19                                               ` Junio C Hamano
  0 siblings, 2 replies; 46+ messages in thread
From: Michael J Gruber @ 2017-04-03 14:46 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Lars Schneider, Luke Diamand

Junio C Hamano venit, vidit, dixit 31.03.2017 20:33:
> 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.

> 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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v3 3/4] name-rev: provide debug output
  2017-04-03 14:46                                             ` Michael J Gruber
@ 2017-04-03 17:07                                               ` Junio C Hamano
  2017-05-20  5:19                                               ` Junio C Hamano
  1 sibling, 0 replies; 46+ messages in thread
From: Junio C Hamano @ 2017-04-03 17:07 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git

Michael J Gruber <git@grubix.eu> writes:

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v3 3/4] name-rev: provide debug output
  2017-04-03 14:46                                             ` Michael J Gruber
  2017-04-03 17:07                                               ` Junio C Hamano
@ 2017-05-20  5:19                                               ` Junio C Hamano
  1 sibling, 0 replies; 46+ messages in thread
From: Junio C Hamano @ 2017-05-20  5:19 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand

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.

^ permalink raw reply	[flat|nested] 46+ messages in thread

end of thread, other threads:[~2017-05-20  5:19 UTC | newest]

Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-15 13:15 [PATCH 0/3] describe --contains sanity Michael J Gruber
2017-03-15 13:15 ` [PATCH 1/3] describe: debug is incompatible with contains Michael J Gruber
2017-03-15 19:21   ` Junio C Hamano
2017-03-17 10:54     ` Michael J Gruber
2017-03-15 13:15 ` [PATCH 2/3] git-prompt: add a describe style for any tags Michael J Gruber
2017-03-15 19:25   ` Junio C Hamano
2017-03-17 10:56     ` Michael J Gruber
2017-03-15 13:15 ` [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs Michael J Gruber
2017-03-15 16:14   ` Junio C Hamano
2017-03-15 19:50   ` Junio C Hamano
2017-03-15 20:51     ` Junio C Hamano
2017-03-15 22:50       ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Junio C Hamano
2017-03-15 22:50         ` [PATCH 1/2] name-rev: refactor logic to see if a new candidate is a better name Junio C Hamano
2017-03-15 22:50         ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Junio C Hamano
2017-03-17  4:07           ` Lars Schneider
2017-03-17  5:45             ` Junio C Hamano
2017-03-17  5:56               ` Junio C Hamano
2017-03-17 17:09                 ` Lars Schneider
2017-03-17 17:17                   ` Junio C Hamano
2017-03-17 22:43                     ` Junio C Hamano
2017-03-29 14:39                       ` [PATCH v2 0/3] name-rev sanity Michael J Gruber
2017-03-29 14:39                         ` [PATCH v2 1/3] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber
2017-03-29 14:39                         ` [PATCH v2 2/3] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber
2017-03-29 14:39                         ` [PATCH v2 3/3] name-rev: provide debug output Michael J Gruber
2017-03-29 17:15                         ` [PATCH v2 0/3] name-rev sanity Junio C Hamano
2017-03-29 17:43                           ` Junio C Hamano
2017-03-30 13:48                             ` Michael J Gruber
2017-03-30 17:30                               ` Junio C Hamano
2017-03-31 13:51                                 ` [PATCH v3 0/4] " Michael J Gruber
2017-03-31 13:51                                   ` [PATCH v3 1/4] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber
2017-03-31 13:51                                   ` [PATCH v3 2/4] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber
2017-03-31 13:51                                   ` [PATCH v3 3/4] name-rev: provide debug output Michael J Gruber
2017-03-31 16:52                                     ` Junio C Hamano
2017-03-31 16:55                                       ` Junio C Hamano
2017-03-31 18:02                                       ` Michael J Gruber
2017-03-31 18:06                                         ` Junio C Hamano
2017-03-31 18:33                                           ` Junio C Hamano
2017-04-03 14:46                                             ` Michael J Gruber
2017-04-03 17:07                                               ` Junio C Hamano
2017-05-20  5:19                                               ` Junio C Hamano
2017-03-31 19:10                                         ` Junio C Hamano
2017-03-31 13:51                                   ` [PATCH v3 4/4] describe: pass --debug down to name-rev Michael J Gruber
2017-03-17 11:25           ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber
2017-03-17 16:03             ` Junio C Hamano
2017-03-16  0:14         ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Stefan Beller
2017-03-16 10:28         ` Jacob Keller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.