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

6 messages, 2 authors, 2017-03-31 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2017-03-30 17:30:46

Michael J Gruber [off-list ref] writes:

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

quoted

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.

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

From: Michael J Gruber <hidden>
Date: 2017-03-31 13:51:50

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

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

From: Michael J Gruber <hidden>
Date: 2017-03-31 13:51:52

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

[PATCH v3 0/4] name-rev sanity

From: Michael J Gruber <hidden>
Date: 2017-03-31 13:51:54

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

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

From: Michael J Gruber <hidden>
Date: 2017-03-31 13:51:59

From: Junio C Hamano <redacted>

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 <redacted>
Signed-off-by: Michael J Gruber <redacted>
---
 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

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

From: Michael J Gruber <hidden>
Date: 2017-03-31 13:52:05

From: Junio C Hamano <redacted>

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

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help