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

[Junio C Hamano]

Michael J Gruber  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.

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

[Junio C Hamano]

Michael J Gruber  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?

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 31.03.2017 20:33:
> Junio C Hamano  writes:
> 
>> Michael J Gruber  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

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

[Junio C Hamano]

Michael J Gruber  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);

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

[Junio C Hamano]

Junio C Hamano  writes:

> Michael J Gruber  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?

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

[Junio C Hamano]

Michael J Gruber  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.

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

[Michael J Gruber]

Am 31. März 2017 18:52:16 MESZ schrieb Junio C Hamano :
>Michael J Gruber  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 
>> ---
>
>>  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.

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

[Junio C Hamano]

Junio C Hamano  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.

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

[Junio C Hamano]

Michael J Gruber  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 
> ---

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

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

[Michael J Gruber]

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 
---
 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(>object.oid,
+   >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 @@