[PATCH 0/4] Make "git name-rev $(git rev-parse v1.8.3)" work

[PATCH 0/4] Make "git name-rev $(git rev-parse v1.8.3)" work

[Junio C Hamano]

So here is a set of small preparatory steps to help the other topic
to allow "git describe -contains v1.8.3" omit trailing "^0" from its
output.  We do not want to prevent people from allowing "name-rev"
to convert object names other than commit-ishes.

The series should apply on 96ffd4ca (Merge branch
'nk/name-rev-abbreviated-refs', 2013-06-30).

Junio C Hamano (4):
  name-ref: factor out name shortening logic from name_ref()
  name-rev: allow converting the exact object name at the tip of a ref
  describe: use argv-array
  describe/name-rev: tell name-rev to peel the incoming object to commit first

 builtin/describe.c |  32 ++++++++-------
 builtin/name-rev.c | 113 ++++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 112 insertions(+), 33 deletions(-)

-- 
1.8.3.2-853-ga8cbcc9

[PATCH 1/4] name-ref: factor out name shortening logic from name_ref()

[Junio C Hamano]

The logic will be used in a new codepath for showing exact matches.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/name-rev.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 87d4854..1234ebb 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -96,6 +96,17 @@ static int subpath_matches(const char *path, const char *filter)
 	return -1;
 }
 
+static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)
+{
+	if (shorten_unambiguous)
+		refname = shorten_unambiguous_ref(refname, 0);
+	else if (!prefixcmp(refname, "refs/heads/"))
+		refname = refname + 11;
+	else if (!prefixcmp(refname, "refs/"))
+		refname = refname + 5;
+	return refname;
+}
+
 struct name_ref_data {
 	int tags_only;
 	int name_only;
@@ -134,13 +145,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
 	if (o && o->type == OBJ_COMMIT) {
 		struct commit *commit = (struct commit *)o;
 
-		if (can_abbreviate_output)
-			path = shorten_unambiguous_ref(path, 0);
-		else if (!prefixcmp(path, "refs/heads/"))
-			path = path + 11;
-		else if (!prefixcmp(path, "refs/"))
-			path = path + 5;
-
+		path = name_ref_abbrev(path, can_abbreviate_output);
 		name_rev(commit, xstrdup(path), 0, 0, deref);
 	}
 	return 0;
-- 
1.8.3.2-853-ga8cbcc9

[PATCH 2/4] name-rev: allow converting the exact object name at the tip of a ref

[Junio C Hamano]

"git name-rev" is supposed to convert 40-hex object names into
strings that name the same objects based on refs, that can be fed to
"git rev-parse" to get the same object names back, so

    $ git rev-parse v1.8.3 v1.8.3^0 | git name-rev --stdin
    8af06057d0c31a24e8737ae846ac2e116e8bafb9
    edca4152560522a431a51fc0a06147fc680b5b18 (tags/v1.8.3^0)

has to have "^0" at the end, as "edca41" is a commit, not the tag
that references it.

The command however did not bother to see if the object is at the
tip of some ref, and failed to convert a tag object.

Teach it to show this instead:

    $ git rev-parse v1.8.3 v1.8.3^0 | git name-rev --stdin
    8af06057d0c31a24e8737ae846ac2e116e8bafb9 (tags/v1.8.3)
    edca4152560522a431a51fc0a06147fc680b5b18 (tags/v1.8.3^0)

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/name-rev.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 1234ebb..29a6f56 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -4,6 +4,7 @@
 #include "tag.h"
 #include "refs.h"
 #include "parse-options.h"
+#include "sha1-lookup.h"
 
 #define CUTOFF_DATE_SLOP 86400 /* one day */
 
@@ -113,6 +114,34 @@ struct name_ref_data {
 	const char *ref_filter;
 };
 
+static struct tip_table {
+	struct tip_table_entry {
+		unsigned char sha1[20];
+		const char *refname;
+	} *table;
+	int nr;
+	int alloc;
+	int sorted;
+} tip_table;
+
+static void add_to_tip_table(const unsigned char *sha1, const char *refname,
+			     int shorten_unambiguous)
+{
+	refname = name_ref_abbrev(refname, shorten_unambiguous);
+
+	ALLOC_GROW(tip_table.table, tip_table.nr + 1, tip_table.alloc);
+	hashcpy(tip_table.table[tip_table.nr].sha1, sha1);
+	tip_table.table[tip_table.nr].refname = xstrdup(refname);
+	tip_table.nr++;
+	tip_table.sorted = 0;
+}
+
+static int tipcmp(const void *a_, const void *b_)
+{
+	const struct tip_table_entry *a = a_, *b = b_;
+	return hashcmp(a->sha1, b->sha1);
+}
+
 static int name_ref(const char *path, const unsigned char *sha1, int flags, void *cb_data)
 {
 	struct object *o = parse_object(sha1);
@@ -135,6 +164,8 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
 		}
 	}
 
+	add_to_tip_table(sha1, path, can_abbreviate_output);
+
 	while (o && o->type == OBJ_TAG) {
 		struct tag *t = (struct tag *) o;
 		if (!t->tagged)
@@ -151,6 +182,32 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
 	return 0;
 }
 
+static const unsigned char *nth_tip_table_ent(size_t ix, void *table_)
+{
+	struct tip_table_entry *table = table_;
+	return table[ix].sha1;
+}
+
+static const char *get_exact_ref_match(const struct object *o)
+{
+	int found;
+
+	if (!tip_table.table || !tip_table.nr)
+		return NULL;
+
+	if (!tip_table.sorted) {
+		qsort(tip_table.table, tip_table.nr, sizeof(*tip_table.table),
+		      tipcmp);
+		tip_table.sorted = 1;
+	}
+
+	found = sha1_pos(o->sha1, tip_table.table, tip_table.nr,
+			 nth_tip_table_ent);
+	if (0 <= found)
+		return tip_table.table[found].refname;
+	return NULL;
+}
+
 /* returns a static buffer */
 static const char *get_rev_name(const struct object *o)
 {
@@ -159,7 +216,7 @@ static const char *get_rev_name(const struct object *o)
 	struct commit *c;
 
 	if (o->type != OBJ_COMMIT)
-		return NULL;
+		return get_exact_ref_match(o);
 	c = (struct commit *) o;
 	n = c->util;
 	if (!n)
-- 
1.8.3.2-853-ga8cbcc9

[PATCH 3/4] describe: use argv-array

[Junio C Hamano]

Instead of using a hand allocated args[] array, use argv-array API
to manage the dynamically created list of arguments when invoking
name-rev.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/describe.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 4e675c3..b5434e4 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -7,6 +7,7 @@
 #include "parse-options.h"
 #include "diff.h"
 #include "hash.h"
+#include "argv-array.h"
 
 #define SEEN		(1u<<0)
 #define MAX_TAGS	(FLAG_BITS - 1)
@@ -442,24 +443,24 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		die(_("--long is incompatible with --abbrev=0"));
 
 	if (contains) {
-		const char **args = xmalloc((7 + argc) * sizeof(char *));
-		int i = 0;
-		args[i++] = "name-rev";
-		args[i++] = "--name-only";
-		args[i++] = "--no-undefined";
+		struct argv_array args;
+
+		argv_array_init(&args);
+		argv_array_push(&args, "name-rev");
+		argv_array_push(&args, "--name-only");
+		argv_array_push(&args, "--no-undefined");
 		if (always)
-			args[i++] = "--always";
+			argv_array_push(&args, "--always");
 		if (!all) {
-			args[i++] = "--tags";
-			if (pattern) {
-				char *s = xmalloc(strlen("--refs=refs/tags/") + strlen(pattern) + 1);
-				sprintf(s, "--refs=refs/tags/%s", pattern);
-				args[i++] = s;
-			}
+			argv_array_push(&args, "--tags");
+			if (pattern)
+				argv_array_pushf(&args, "--refs=refs/tags/%s", pattern);
+		}
+		while (*argv) {
+			argv_array_push(&args, *argv);
+			argv++;
 		}
-		memcpy(args + i, argv, argc * sizeof(char *));
-		args[i + argc] = NULL;
-		return cmd_name_rev(i + argc, args, prefix);
+		return cmd_name_rev(args.argc, args.argv, prefix);
 	}
 
 	init_hash(&names);
-- 
1.8.3.2-853-ga8cbcc9

[PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first

[Junio C Hamano]

With this on top of the other patches in this series, you would get:

    $ git describe --contains $(git rev-parse v1.8.3 v1.8.3^0)
    v1.8.3
    v1.8.3

while you can still differentiate tags and the commits they point at
with:

    $ git name-rev --refs=tags/\* --name-only $(git rev-parse v1.8.3 v1.8.3^0)
    v1.8.3
    v1.8.3^0

The difference in these two behaviours is achieved by adding --peel-to-commit
option to "name-rev" and using it when "describe" internally calls it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/describe.c |  1 +
 builtin/name-rev.c | 35 +++++++++++++++++++++++++----------
 2 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index b5434e4..f7adda6 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -447,6 +447,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 
 		argv_array_init(&args);
 		argv_array_push(&args, "name-rev");
+		argv_array_push(&args, "--peel-to-commit");
 		argv_array_push(&args, "--name-only");
 		argv_array_push(&args, "--no-undefined");
 		if (always)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 29a6f56..fa37731 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -15,6 +15,7 @@ typedef struct rev_name {
 } rev_name;
 
 static long cutoff = LONG_MAX;
+static int peel_to_commit;
 
 /* How many generations are maximally preferred over _one_ merge traversal? */
 #define MERGE_TRAVERSAL_WEIGHT 65535
@@ -33,7 +34,7 @@ static void name_rev(struct commit *commit,
 	if (commit->date < cutoff)
 		return;
 
-	if (deref) {
+	if (deref && !peel_to_commit) {
 		char *new_name = xmalloc(strlen(tip_name)+3);
 		strcpy(new_name, tip_name);
 		strcat(new_name, "^0");
@@ -320,6 +321,8 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN(0, "undefined", &allow_undefined, N_("allow to print `undefined` names")),
 		OPT_BOOLEAN(0, "always",     &always,
 			   N_("show abbreviated commit object as fallback")),
+		OPT_BOOLEAN(0, "peel-to-commit", &peel_to_commit,
+			    N_("peel tag object names in the input to a commmit")),
 		OPT_END(),
 	};
 
@@ -334,7 +337,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 
 	for (; argc; argc--, argv++) {
 		unsigned char sha1[20];
-		struct object *o;
+		struct object *object;
 		struct commit *commit;
 
 		if (get_sha1(*argv, sha1)) {
@@ -343,17 +346,29 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		o = deref_tag(parse_object(sha1), *argv, 0);
-		if (!o || o->type != OBJ_COMMIT) {
+		commit = NULL;
+		object = parse_object(sha1);
+		if (object) {
+			struct object *peeled = deref_tag(object, *argv, 0);
+			if (peeled && peeled->type == OBJ_COMMIT)
+				commit = (struct commit *) peeled;
+		}
+
+		if (!object) {
+			fprintf(stderr, "Could not get object for %s. Skipping.\n",
+				*argv);
+			continue;
+		}
+		if (peel_to_commit && !commit) {
 			fprintf(stderr, "Could not get commit for %s. Skipping.\n",
-					*argv);
+				*argv);
 			continue;
 		}
-
-		commit = (struct commit *)o;
-		if (cutoff > commit->date)
-			cutoff = commit->date;
-		add_object_array((struct object *)commit, *argv, &revs);
+		if (commit) {
+			if (cutoff > commit->date)
+				cutoff = commit->date;
+		}
+		add_object_array(object, *argv, &revs);
 	}
 
 	if (cutoff)
-- 
1.8.3.2-853-ga8cbcc9

Re: [PATCH 1/4] name-ref: factor out name shortening logic from name_ref()

[Michael Haggerty]

On 07/08/2013 12:33 AM, Junio C Hamano wrote:
> The logic will be used in a new codepath for showing exact matches.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/name-rev.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index 87d4854..1234ebb 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -96,6 +96,17 @@ static int subpath_matches(const char *path, const char *filter)
>  	return -1;
>  }
>  
> +static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)
> +{
> +	if (shorten_unambiguous)
> +		refname = shorten_unambiguous_ref(refname, 0);
> +	else if (!prefixcmp(refname, "refs/heads/"))
> +		refname = refname + 11;
> +	else if (!prefixcmp(refname, "refs/"))
> +		refname = refname + 5;
> +	return refname;
> +}
> +

In my opinion this would be a tad clearer if each of the branches of the
"if" returned the value directly rather than setting refname and relying
on the "return" statement that follows.  But it's probably a matter of
taste.

>  struct name_ref_data {
>  	int tags_only;
>  	int name_only;
> @@ -134,13 +145,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
>  	if (o && o->type == OBJ_COMMIT) {
>  		struct commit *commit = (struct commit *)o;
>  
> -		if (can_abbreviate_output)
> -			path = shorten_unambiguous_ref(path, 0);
> -		else if (!prefixcmp(path, "refs/heads/"))
> -			path = path + 11;
> -		else if (!prefixcmp(path, "refs/"))
> -			path = path + 5;
> -
> +		path = name_ref_abbrev(path, can_abbreviate_output);
>  		name_rev(commit, xstrdup(path), 0, 0, deref);
>  	}
>  	return 0;
> 

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 2/4] name-rev: allow converting the exact object name at the tip of a ref

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> "git name-rev" is supposed to convert 40-hex object names into
> strings that name the same objects based on refs, that can be fed to
> "git rev-parse" to get the same object names back, so
>
>     $ git rev-parse v1.8.3 v1.8.3^0 | git name-rev --stdin
>     8af06057d0c31a24e8737ae846ac2e116e8bafb9
>     edca4152560522a431a51fc0a06147fc680b5b18 (tags/v1.8.3^0)

Wait, what?

  $ git name-rev 8af060
  8af060 tags/v1.8.3^0

Isn't this a failure specific to --stdin?

> Teach it to show this instead:
>
>     $ git rev-parse v1.8.3 v1.8.3^0 | git name-rev --stdin
>     8af06057d0c31a24e8737ae846ac2e116e8bafb9 (tags/v1.8.3)
>     edca4152560522a431a51fc0a06147fc680b5b18 (tags/v1.8.3^0)

Wait, what is name-rev?

  Finds symbolic names suitable for human digestion for revisions
  given in any format parsable by git rev-parse.

It is meant to name _revisions_ (aka. commits): in that context, what
sense does it make to distinguish between tags/v1.8.3 and
tags/v1.8.3^0?

Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> With this on top of the other patches in this series, you would get:
>
>     $ git describe --contains $(git rev-parse v1.8.3 v1.8.3^0)
>     v1.8.3
>     v1.8.3
>
> while you can still differentiate tags and the commits they point at
> with:
>
>     $ git name-rev --refs=tags/\* --name-only $(git rev-parse v1.8.3 v1.8.3^0)
>     v1.8.3
>     v1.8.3^0
>
> The difference in these two behaviours is achieved by adding --peel-to-commit
> option to "name-rev" and using it when "describe" internally calls it.

Essentially a revert of [2/4] for describe-purposes, achieved by
adding an ugly command-line option to name-rev.  Before we argue any
further, let me ask: who uses name-rev (and depends strongly on its
output)?!  Our very own testsuite does not exercise it.  There are
exactly two users of describe/name-rev:

1. prompt, obviously.
2. DAG-tests, for simplification.

I really can't imagine it being useful elsewhere.

Re: [PATCH 1/4] name-ref: factor out name shortening logic from name_ref()

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 07/08/2013 12:33 AM, Junio C Hamano wrote:
>> The logic will be used in a new codepath for showing exact matches.
>> 
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  builtin/name-rev.c | 19 ++++++++++++-------
>>  1 file changed, 12 insertions(+), 7 deletions(-)
>> 
>> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
>> index 87d4854..1234ebb 100644
>> --- a/builtin/name-rev.c
>> +++ b/builtin/name-rev.c
>> @@ -96,6 +96,17 @@ static int subpath_matches(const char *path, const char *filter)
>>  	return -1;
>>  }
>>  
>> +static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)
>> +{
>> +	if (shorten_unambiguous)
>> +		refname = shorten_unambiguous_ref(refname, 0);
>> +	else if (!prefixcmp(refname, "refs/heads/"))
>> +		refname = refname + 11;
>> +	else if (!prefixcmp(refname, "refs/"))
>> +		refname = refname + 5;
>> +	return refname;
>> +}
>> +
>
> In my opinion this would be a tad clearer if each of the branches of the
> "if" returned the value directly rather than setting refname and relying
> on the "return" statement that follows.  But it's probably a matter of
> taste.

I tend to agree; this is a straight-forward code movement (with the
variable name changed to conform to your recent refs.c update to
call these things "refname"), and that was the primary reason I kept
them like so.

>
>>  struct name_ref_data {
>>  	int tags_only;
>>  	int name_only;
>> @@ -134,13 +145,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
>>  	if (o && o->type == OBJ_COMMIT) {
>>  		struct commit *commit = (struct commit *)o;
>>  
>> -		if (can_abbreviate_output)
>> -			path = shorten_unambiguous_ref(path, 0);
>> -		else if (!prefixcmp(path, "refs/heads/"))
>> -			path = path + 11;
>> -		else if (!prefixcmp(path, "refs/"))
>> -			path = path + 5;
>> -
>> +		path = name_ref_abbrev(path, can_abbreviate_output);
>>  		name_rev(commit, xstrdup(path), 0, 0, deref);
>>  	}
>>  	return 0;
>> 
>
> Michael

Re: [PATCH 2/4] name-rev: allow converting the exact object name at the tip of a ref

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>   Finds symbolic names suitable for human digestion for revisions
>   given in any format parsable by git rev-parse.
>
> It is meant to name _revisions_ (aka. commits):

That is a mistaken documentation, written based on a half-baked
implementation that conflated "the current code only can handle
commits" with "we need to only handle commit and nothing, ever".

We had to fix a similar misguided/short-sighted implementation in
the "git fetch" codepath when adding "git pull/merge $tag" (the code
peeled a tag object too early when writing FETCH_HEAD out, losing
information if what we were given was a tag or a commit).

I do not want to see us making the mistake worse unnecessarily.
When we see a commit object that is pointed by tag v1.8.3, it is the
right thing to do to return v1.8.3^0 as its string representation so
that "git rev-parse v1.8.3^0" give us the same thing back.  name-rev
that is fed a tag object v1.8.3 should give v1.8.3 (not v1.8.3^0)
back, otherwise feeding its output to "git rev-parse" will not give
us the same thing we fed as the input to name-rev.

Re: [PATCH 3/4] describe: use argv-array

[Jeff King]

On Sun, Jul 07, 2013 at 03:33:43PM -0700, Junio C Hamano wrote:

> +		argv_array_init(&args);
> +		argv_array_push(&args, "name-rev");
> +		argv_array_push(&args, "--name-only");
> +		argv_array_push(&args, "--no-undefined");
> [...]
> -		memcpy(args + i, argv, argc * sizeof(char *));
> -		args[i + argc] = NULL;
> -		return cmd_name_rev(i + argc, args, prefix);
> +		return cmd_name_rev(args.argc, args.argv, prefix);

This leaks the memory allocated by "args". The original did, too, and it
is probably not that big a deal (we exit right after anyway). The fix
would be something like:

  rc = cmd_name_rev(args.argc, args.argv, prefix);
  argv_array_clear(&args);
  return rc;

-Peff

Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first

[Jeff King]

On Sun, Jul 07, 2013 at 03:33:44PM -0700, Junio C Hamano wrote:

> With this on top of the other patches in this series, you would get:
> 
>     $ git describe --contains $(git rev-parse v1.8.3 v1.8.3^0)
>     v1.8.3
>     v1.8.3
> 
> while you can still differentiate tags and the commits they point at
> with:
> 
>     $ git name-rev --refs=tags/\* --name-only $(git rev-parse v1.8.3 v1.8.3^0)
>     v1.8.3
>     v1.8.3^0
> 
> The difference in these two behaviours is achieved by adding --peel-to-commit
> option to "name-rev" and using it when "describe" internally calls it.

I am somewhat mixed on this.

You are changing the default behavior of name-rev and adding a new
option to restore it, so I wonder who (if anyone) might be broken. The
documentation is now also out of date; not only does it not mention
"peel-to-commit", but it claims the argument to name-rev is a
committish, which is not really true without that option.

On the other hand, the new default behavior seems way more sane to me.
In general, I would expect name-rev to:

  1. Behave more or less the same between "git name-rev $sha1" and "echo
     $sha1 | git name-rev --stdin". Your patch improves that. Though I
     note that --peel-to-commit does not affect --stdin at all. Should
     it? And of course the two differ in that the command line will take
     any rev-parse expression, and --stdin only looks for full sha1s.

  2. If name-rev prints "$X $Y", I would expect "git rev-parse $X" to
     equal "git rev-parse $Y". With peeling, that is not the case, and
     you get the misleading example that Ram showed:

       $ git name-rev 8af0605
       8af0605 tags/v1.8.3^0

    or more obviously weird:

       $ git name-rev v1.8.3
       v1.8.3 tags/v1.8.3^0

So I think your series moves in a good direction, but I would just worry
that it is breaking backwards compatibility (but like I said, I am not
clear on who is affected and what it means for them).

-Peff

Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first

[Jeff King]

On Mon, Jul 08, 2013 at 06:38:32PM +0530, Ramkumar Ramachandra wrote:

> Junio C Hamano wrote:
> > With this on top of the other patches in this series, you would get:
> >
> >     $ git describe --contains $(git rev-parse v1.8.3 v1.8.3^0)
> >     v1.8.3
> >     v1.8.3
> >
> > while you can still differentiate tags and the commits they point at
> > with:
> >
> >     $ git name-rev --refs=tags/\* --name-only $(git rev-parse v1.8.3 v1.8.3^0)
> >     v1.8.3
> >     v1.8.3^0
> >
> > The difference in these two behaviours is achieved by adding --peel-to-commit
> > option to "name-rev" and using it when "describe" internally calls it.
> 
> Essentially a revert of [2/4] for describe-purposes, achieved by
> adding an ugly command-line option to name-rev.

I don't think it is a revert. The two patches complement each other.

2/4 is basically "if we have a non-commit object which is pointed at
directly by a tip, make sure we name it by that tip". But you can only
get such an object by "name-rev --stdin", since name-rev peels its
command-line arguments.

4/4 is "stop peeling command line objects, so we can find their exact
tips". IOW, it lets the command line do the same thing that --stdin was
able to do in 2/4.

> Before we argue any further, let me ask: who uses name-rev (and
> depends strongly on its output)?!  Our very own testsuite does not
> exercise it.  There are exactly two users of describe/name-rev:
> 
> 1. prompt, obviously.
> 2. DAG-tests, for simplification.

Yeah, I'm not clear on who we are breaking with the change in default
peeling behavior, nor why the "describe --contains" wrapper wants to
keep it.

-Peff

Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   1. Behave more or less the same between "git name-rev $sha1" and "echo
>      $sha1 | git name-rev --stdin". Your patch improves that. Though I
>      note that --peel-to-commit does not affect --stdin at all. Should
>      it? And of course the two differ in that the command line will take
>      any rev-parse expression, and --stdin only looks for full sha1s.

To "Should it?", I do not deeply care.  "--peel-to-commit" is an
exception that only is needed to support "describe".

I could instead have tucked "^0" at the end of each argument when
"describe" calls out to "name-rev" without adding this new option,
which is much much closer to what is really going on.

And that will alleviate your #2 below.

>   2. If name-rev prints "$X $Y", I would expect "git rev-parse $X" to
>      equal "git rev-parse $Y". With peeling, that is not the case, and
>      you get the misleading example that Ram showed:
>
>        $ git name-rev 8af0605
>        8af0605 tags/v1.8.3^0
>
>     or more obviously weird:
>
>        $ git name-rev v1.8.3
>        v1.8.3 tags/v1.8.3^0
>
> So I think your series moves in a good direction, but I would just worry
> that it is breaking backwards compatibility (but like I said, I am not
> clear on who is affected and what it means for them).

Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first

[Jeff King]

On Mon, Jul 08, 2013 at 10:33:26PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >   1. Behave more or less the same between "git name-rev $sha1" and "echo
> >      $sha1 | git name-rev --stdin". Your patch improves that. Though I
> >      note that --peel-to-commit does not affect --stdin at all. Should
> >      it? And of course the two differ in that the command line will take
> >      any rev-parse expression, and --stdin only looks for full sha1s.
> 
> To "Should it?", I do not deeply care.  "--peel-to-commit" is an
> exception that only is needed to support "describe".
> 
> I could instead have tucked "^0" at the end of each argument when
> "describe" calls out to "name-rev" without adding this new option,
> which is much much closer to what is really going on.

Yeah, I tend to think that is a more sane interface, even though it is a
little more work in git-describe.

Although I am still not clear on why it would not be up to the caller of
git-describe in the first place to decide which they wanted.

-Peff

Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Although I am still not clear on why it would not be up to the caller of
> git-describe in the first place to decide which they wanted.

Thanks for a dose of sanity.

Even though the part of the miniseries that makes sure that "X (Y)"
output from "name-rev" always satisfies that "rev-parse" on X and Y
give the same thing is an improvement, the whole thing about
"describe" is misguided and wrong, I think.

It started from the observation that these do not match:

    $ git describe $(git rev-parse v1.8.3)
    v1.8.3
    $ git describe --contains $(git rev-parse v1.8.3)
    v1.8.3^0

and the miniseries veered in a wrong direction of "fixing" the
latter to match the former.

But the thing is, what is incosistent from the rest of the world is
the describe output without "--contains" for a commit that is
exactly at a tag (i.e. the former), and there is no need to "fix"
this "inconsistency", as we see below.

The form without "--contains" in general reads like this:

    $ git describe --long $(git rev-parse v1.8.3) a717d9e
    v1.8.3-0-gedca415
    v1.8.3-2-ga717d9e

They both name a commit object, but that is sort of an afterthought;
the support for describe name came late at 7dd45e15 (sha1_name.c:
understand "describe" output as a valid object name, 2006-09-20).

The primary purpose of "git describe" without "--contains" is to
give a string that is suitable for a version number to be embedded
in an executable.  For that purpose, "v1.8.3" is more convenient
than "v1.8.3-0-gedca415".

But this convenient format breaks the consistency.  While any other
describe name for a commmit names a commit, the output for a commit
that is exactly at a tag does not (in ancient times, describe output
were not even extended SHA-1 expressions, so this inconsistency did
not matter, but the "afterthought" brought the consistency to the
foreground).  The user chooses the convenience over the consistency
by not using "--long".

And the short form cannot be "v1.8.3^0" or "v1.8.3~0" for the sake of
"consistency", as these are no more suitable as a version number
than a short and sweet "v1.8.3".

The "--contains" form does not even aim to come up with a pleasant
looking version string without using funny line noise characters, so
it is perfectly fine for it to say:

    $ git describe --contains $(git rev-parse v1.8.3) a717d9e
    v1.8.3^0
    v1.8.3.1~9

and these are internally consistent (they both roundtrip via
rev-parse).  Stripping "^0" from the former will break the
consistency, even though it may make the output look prettier, but
the "--contains" output is not even meant to be "pretty" in the
first place.

So let's drop 4/4; it is breaking the system by trying to solve a
problem that does not exist.

Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>     $ git describe $(git rev-parse v1.8.3)
>     v1.8.3
>     $ git describe --contains $(git rev-parse v1.8.3)
>     v1.8.3^0

This is a correct observation, and I've already submitted the correct
fix: "name-rev: strip trailing ^0 in when --name-only".

>     $ git describe --contains $(git rev-parse v1.8.3) a717d9e
>     v1.8.3^0
>     v1.8.3.1~9
>
> and these are internally consistent (they both roundtrip via
> rev-parse).  Stripping "^0" from the former will break the
> consistency, even though it may make the output look prettier, but
> the "--contains" output is not even meant to be "pretty" in the
> first place.

Incorrect.  The "--contains" output _is_ meant to be pretty.  That's
the whole reason name-rev --name-only was invented.

[2/4] is correct in that it fixes --stdin for annotated tags (although
the implementation could be simpler, and the commit message is
completely misleading).

Re: [PATCH 3/4] describe: use argv-array

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Jul 07, 2013 at 03:33:43PM -0700, Junio C Hamano wrote:
>
>> +		argv_array_init(&args);
>> +		argv_array_push(&args, "name-rev");
>> +		argv_array_push(&args, "--name-only");
>> +		argv_array_push(&args, "--no-undefined");
>> [...]
>> -		memcpy(args + i, argv, argc * sizeof(char *));
>> -		args[i + argc] = NULL;
>> -		return cmd_name_rev(i + argc, args, prefix);
>> +		return cmd_name_rev(args.argc, args.argv, prefix);
>
> This leaks the memory allocated by "args". The original did, too, and it
> is probably not that big a deal (we exit right after anyway). The fix
> would be something like:
>
>   rc = cmd_name_rev(args.argc, args.argv, prefix);
>   argv_array_clear(&args);
>   return rc;

Yes; this was meant as a straight rewrite and I did not bother, but
I should have cleaned it up as I meant to build on top.

Will amend, even though I do not think we need to build anything on
top.

Re: [PATCH 3/4] describe: use argv-array

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> On Sun, Jul 07, 2013 at 03:33:43PM -0700, Junio C Hamano wrote:
>>
>>> +		argv_array_init(&args);
>>> +		argv_array_push(&args, "name-rev");
>>> +		argv_array_push(&args, "--name-only");
>>> +		argv_array_push(&args, "--no-undefined");
>>> [...]
>>> -		memcpy(args + i, argv, argc * sizeof(char *));
>>> -		args[i + argc] = NULL;
>>> -		return cmd_name_rev(i + argc, args, prefix);
>>> +		return cmd_name_rev(args.argc, args.argv, prefix);
>>
>> This leaks the memory allocated by "args". The original did, too, and it
>> is probably not that big a deal (we exit right after anyway). The fix
>> would be something like:
>>
>>   rc = cmd_name_rev(args.argc, args.argv, prefix);
>>   argv_array_clear(&args);
>>   return rc;
>
> Yes; this was meant as a straight rewrite and I did not bother, but
> I should have cleaned it up as I meant to build on top.
>
> Will amend, even though I do not think we need to build anything on
> top.

Heh, you fooled me.  cmd_name_rev() uses the usual parse-options
machinery that updates args.argv[].  Dashed options that were
consumed will not remain in args.argv[] and argv_array_clear() will
not have a chance to free them, and besides, args.argc and args.argv
will be out of sync and wreaks havoc in argv_array_clear().

We could expose argv_array_push_nodup() and use it in this caller
and then free the args.argv[] but not its contents, but I do not
think it is worth it.

Re: [PATCH 3/4] describe: use argv-array

[Jeff King]

On Tue, Jul 09, 2013 at 09:00:20AM -0700, Junio C Hamano wrote:

> >>> +		return cmd_name_rev(args.argc, args.argv, prefix);
> >>
> >> This leaks the memory allocated by "args". The original did, too, and it
> >> is probably not that big a deal (we exit right after anyway). The fix
> >> would be something like:
> >>
> >>   rc = cmd_name_rev(args.argc, args.argv, prefix);
> >>   argv_array_clear(&args);
> >>   return rc;
> >
> > Yes; this was meant as a straight rewrite and I did not bother, but
> > I should have cleaned it up as I meant to build on top.
> >
> > Will amend, even though I do not think we need to build anything on
> > top.
> 
> Heh, you fooled me.  cmd_name_rev() uses the usual parse-options
> machinery that updates args.argv[].  Dashed options that were
> consumed will not remain in args.argv[] and argv_array_clear() will
> not have a chance to free them, and besides, args.argc and args.argv
> will be out of sync and wreaks havoc in argv_array_clear().

Ick, yeah, I forgot about that. Let's just leave it as a leak, then. We
are exiting immediately afterwards, anyway.

-Peff