[PATCH v4 00/18] Extending the shelf-life of "git describe" output

[PATCH v4 00/18] Extending the shelf-life of "git describe" output

[Junio C Hamano]

This is take 4.  The earlier rounds were $gmane/200165, 200387, and
200506.

Compared to the previous round, it has more patches in the clean-up
phase.  Most notably, patch 03/18 gets rid of get_sha1_with_mode_1()
and replaces the only external caller of it with a call to a more
straightforward die_on_misspelt_object_name().  The test suite added
by patch 12/18 has more patterns that we can potentially improve on.

The disambiguation logic can now be asked to pick only committish,
which can be used in places like "git commit -C deadbeef".  It also
knows that A and B in "git log A..B" can only be committishes.

Adding support for treeish, if anybody is tempted to do so, should
now be pretty straightforward.

Junio C Hamano (18):
  sha1_name.c: indentation fix
  sha1_name.c: hide get_sha1_with_context_1() ugliness
  sha1_name.c: get rid of ugly get_sha1_with_mode_1()
  sha1_name.c: get rid of get_sha1_with_mode()
  sha1_name.c: clarify what "fake" is for in find_short_object_filename()
  sha1_name.c: rename "now" to "current"
  sha1_name.c: refactor find_short_packed_object()
  sha1_name.c: correct misnamed "canonical" and "res"
  sha1_name.c: restructure disambiguation of short names
  sha1_name.c: allow get_short_sha1() to take other flags
  sha1_name.c: teach get_short_sha1() a commit-only option
  sha1_name.c: get_describe_name() by definition groks only commits
  sha1_name.c: get_sha1_1() takes lookup flags
  sha1_name.c: many short names can only be committish
  sha1_name.c: teach lookup context to get_sha1_with_context()
  sha1_name.c: introduce get_sha1_committish()
  revision.c: allow handle_revision_arg() to take other flags
  revision.c: the "log" family, except for "show", takes committish

 builtin/cat-file.c                  |   2 +-
 builtin/log.c                       |   3 +
 builtin/pack-objects.c              |   2 +-
 cache.h                             |  18 +-
 commit.c                            |   2 +-
 revision.c                          |  38 ++--
 revision.h                          |   5 +-
 setup.c                             |   8 +-
 sha1_name.c                         | 383 ++++++++++++++++++++++++------------
 t/t1512-rev-parse-disambiguation.sh | 208 ++++++++++++++++++++
 10 files changed, 509 insertions(+), 160 deletions(-)
 create mode 100755 t/t1512-rev-parse-disambiguation.sh

-- 
1.7.11.1.212.g52fe12e

[PATCH v4 01/18] sha1_name.c: indentation fix

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 03ffc2c..5b0c845 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -103,10 +103,10 @@ static int find_short_packed_object(int len, const unsigned char *match, unsigne
 		}
 		if (first < num) {
 			const unsigned char *now, *next;
-		       now = nth_packed_object_sha1(p, first);
+			now = nth_packed_object_sha1(p, first);
 			if (match_sha(len, match, now)) {
 				next = nth_packed_object_sha1(p, first+1);
-			       if (!next|| !match_sha(len, match, next)) {
+				if (!next|| !match_sha(len, match, next)) {
 					/* unique within this pack */
 					if (!found) {
 						found_sha1 = now;
-- 
1.7.11.1.212.g52fe12e

[PATCH v4 02/18] sha1_name.c: hide get_sha1_with_context_1() ugliness

[Junio C Hamano]

There is no outside caller that cares about the "only-to-die" ugliness.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h     |  6 +-----
 sha1_name.c | 31 ++++++++++++++++++-------------
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/cache.h b/cache.h
index 10afd71..9ee470c 100644
--- a/cache.h
+++ b/cache.h
@@ -817,11 +817,7 @@ static inline int get_sha1_with_mode(const char *str, unsigned char *sha1, unsig
 {
 	return get_sha1_with_mode_1(str, sha1, mode, 0, NULL);
 }
-extern int get_sha1_with_context_1(const char *name, unsigned char *sha1, struct object_context *orc, int only_to_die, const char *prefix);
-static inline int get_sha1_with_context(const char *str, unsigned char *sha1, struct object_context *orc)
-{
-	return get_sha1_with_context_1(str, sha1, orc, 0, NULL);
-}
+extern int get_sha1_with_context(const char *str, unsigned char *sha1, struct object_context *orc);
 
 /*
  * Try to read a SHA1 in hexadecimal format from the 40 characters
diff --git a/sha1_name.c b/sha1_name.c
index 5b0c845..10932bf 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -992,16 +992,6 @@ static void diagnose_invalid_index_path(int stage,
 }
 
 
-int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,
-			 int only_to_die, const char *prefix)
-{
-	struct object_context oc;
-	int ret;
-	ret = get_sha1_with_context_1(name, sha1, &oc, only_to_die, prefix);
-	*mode = oc.mode;
-	return ret;
-}
-
 static char *resolve_relative_path(const char *rel)
 {
 	if (prefixcmp(rel, "./") && prefixcmp(rel, "../"))
@@ -1019,9 +1009,9 @@ static char *resolve_relative_path(const char *rel)
 			   rel);
 }
 
-int get_sha1_with_context_1(const char *name, unsigned char *sha1,
-			    struct object_context *oc,
-			    int only_to_die, const char *prefix)
+static int get_sha1_with_context_1(const char *name, unsigned char *sha1,
+				   struct object_context *oc,
+				   int only_to_die, const char *prefix)
 {
 	int ret, bracket_depth;
 	int namelen = strlen(name);
@@ -1134,3 +1124,18 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 	}
 	return ret;
 }
+
+int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,
+			 int only_to_die, const char *prefix)
+{
+	struct object_context oc;
+	int ret;
+	ret = get_sha1_with_context_1(name, sha1, &oc, only_to_die, prefix);
+	*mode = oc.mode;
+	return ret;
+}
+
+int get_sha1_with_context(const char *str, unsigned char *sha1, struct object_context *orc)
+{
+	return get_sha1_with_context_1(str, sha1, orc, 0, NULL);
+}
-- 
1.7.11.1.212.g52fe12e

[PATCH v4 03/18] sha1_name.c: get rid of ugly get_sha1_with_mode_1()

[Junio C Hamano]

The only external caller is setup.c that tries to give a nicer error
message when an object name is misspelt (e.g. "HEAD:cashe.h").
Retire it and give the caller a dedicated and more intuitive API
function die_on_misspelt_object_name().

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h     |  7 ++-----
 setup.c     |  8 ++------
 sha1_name.c | 20 ++++++++++++++++----
 3 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/cache.h b/cache.h
index 9ee470c..2200110 100644
--- a/cache.h
+++ b/cache.h
@@ -812,11 +812,8 @@ struct object_context {
 };
 
 extern int get_sha1(const char *str, unsigned char *sha1);
-extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int only_to_die, const char *prefix);
-static inline int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
-{
-	return get_sha1_with_mode_1(str, sha1, mode, 0, NULL);
-}
+extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);
+extern void die_on_misspelt_object_name(const char *name, const char *prefix);
 extern int get_sha1_with_context(const char *str, unsigned char *sha1, struct object_context *orc);
 
 /*
diff --git a/setup.c b/setup.c
index 61c22e6..979a26b 100644
--- a/setup.c
+++ b/setup.c
@@ -55,18 +55,14 @@ int check_filename(const char *prefix, const char *arg)
 
 static void NORETURN die_verify_filename(const char *prefix, const char *arg)
 {
-	unsigned char sha1[20];
-	unsigned mode;
-
 	/*
 	 * Saying "'(icase)foo' does not exist in the index" when the
 	 * user gave us ":(icase)foo" is just stupid.  A magic pathspec
 	 * begins with a colon and is followed by a non-alnum; do not
-	 * let get_sha1_with_mode_1(only_to_die=1) to even trigger.
+	 * let die_on_misspelt_object_name() even trigger.
 	 */
 	if (!(arg[0] == ':' && !isalnum(arg[1])))
-		/* try a detailed diagnostic ... */
-		get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);
+		die_on_misspelt_object_name(arg, prefix);
 
 	/* ... or fall back the most general message. */
 	die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
diff --git a/sha1_name.c b/sha1_name.c
index 10932bf..01ed48b 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1125,12 +1125,24 @@ static int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 	return ret;
 }
 
-int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,
-			 int only_to_die, const char *prefix)
+/*
+ * Call this function when you know "name" given by the end user must
+ * name an object but it doesn't; the function _may_ die with a better
+ * diagnostic message than "no such object 'name'", e.g. "Path 'doc' does not
+ * exist in 'HEAD'" when given "HEAD:doc", or it may return in which case
+ * you have a chance to diagnose the error further.
+ */
+void die_on_misspelt_object_name(const char *name, const char *prefix)
 {
 	struct object_context oc;
-	int ret;
-	ret = get_sha1_with_context_1(name, sha1, &oc, only_to_die, prefix);
+	unsigned char sha1[20];
+	get_sha1_with_context_1(name, sha1, &oc, 1, prefix);
+}
+
+int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
+{
+	struct object_context oc;
+	int ret = get_sha1_with_context_1(str, sha1, &oc, 0, NULL);
 	*mode = oc.mode;
 	return ret;
 }
-- 
1.7.11.1.212.g52fe12e

Re: [PATCH v4 03/18] sha1_name.c: get rid of ugly get_sha1_with_mode_1()

[Matthieu Moy]

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

> +/*
> + * Call this function when you know "name" given by the end user must
> + * name an object but it doesn't; the function _may_ die with a better
> + * diagnostic message than "no such object 'name'", e.g. "Path 'doc' does not
> + * exist in 'HEAD'" when given "HEAD:doc", or it may return in which case
> + * you have a chance to diagnose the error further.
> + */
> +void die_on_misspelt_object_name(const char *name, const char *prefix)

It seems unusual to have a function named die_* that is not a noreturn
function. I'd call it die_*_maybe, or diagnose_* instead.

(but as the comment above documents the behavior, it's not terribly
important, you may ignore my comment if you whish)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v4 03/18] sha1_name.c: get rid of ugly get_sha1_with_mode_1()

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> +/*
>> + * Call this function when you know "name" given by the end user must
>> + * name an object but it doesn't; the function _may_ die with a better
>> + * diagnostic message than "no such object 'name'", e.g. "Path 'doc' does not
>> + * exist in 'HEAD'" when given "HEAD:doc", or it may return in which case
>> + * you have a chance to diagnose the error further.
>> + */
>> +void die_on_misspelt_object_name(const char *name, const char *prefix)
>
> It seems unusual to have a function named die_* that is not a noreturn
> function. I'd call it die_*_maybe, or diagnose_* instead.
>
> (but as the comment above documents the behavior, it's not terribly
> important, you may ignore my comment if you whish)

I was hoping "on" may imply "if not misspelled, ignore and keep
going", but apparently that failed.  I am not good at names.

die_if_misspelt?  maybe_die_on_misspelt?

I am fairly negative on "diagnose" as it does not say much about
what would happen after diagnosis (namely, we _die_).

Re: [PATCH v4 03/18] sha1_name.c: get rid of ugly get_sha1_with_mode_1()

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> +/*
>>> + * Call this function when you know "name" given by the end user must
>>> + * name an object but it doesn't; the function _may_ die with a better
>>> + * diagnostic message than "no such object 'name'", e.g. "Path 'doc' does not
>>> + * exist in 'HEAD'" when given "HEAD:doc", or it may return in which case
>>> + * you have a chance to diagnose the error further.
>>> + */
>>> +void die_on_misspelt_object_name(const char *name, const char *prefix)
>>
>> It seems unusual to have a function named die_* that is not a noreturn
>> function. I'd call it die_*_maybe, or diagnose_* instead.
>>
>> (but as the comment above documents the behavior, it's not terribly
>> important, you may ignore my comment if you whish)
>
> I was hoping "on" may imply "if not misspelled, ignore and keep
> going", but apparently that failed.  I am not good at names.

Ah, maybe I missed the "on". With your explanation, it makes sense.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH v4 04/18] sha1_name.c: get rid of get_sha1_with_mode()

[Junio C Hamano]

There are only two callers, and they will benefit from being able to
pass disambiguation hints to underlying get_sha1_with_context() API
once it happens.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h     |  1 -
 revision.c  | 12 ++++++------
 sha1_name.c |  8 --------
 3 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/cache.h b/cache.h
index 2200110..3066b03 100644
--- a/cache.h
+++ b/cache.h
@@ -812,7 +812,6 @@ struct object_context {
 };
 
 extern int get_sha1(const char *str, unsigned char *sha1);
-extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);
 extern void die_on_misspelt_object_name(const char *name, const char *prefix);
 extern int get_sha1_with_context(const char *str, unsigned char *sha1, struct object_context *orc);
 
diff --git a/revision.c b/revision.c
index 064e351..86a14c8 100644
--- a/revision.c
+++ b/revision.c
@@ -1097,7 +1097,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs,
 			int flags,
 			int cant_be_filename)
 {
-	unsigned mode;
+	struct object_context oc;
 	char *dotdot;
 	struct object *object;
 	unsigned char sha1[20];
@@ -1180,13 +1180,13 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs,
 		local_flags = UNINTERESTING;
 		arg++;
 	}
-	if (get_sha1_with_mode(arg, sha1, &mode))
+	if (get_sha1_with_context(arg, sha1, &oc))
 		return revs->ignore_missing ? 0 : -1;
 	if (!cant_be_filename)
 		verify_non_filename(revs->prefix, arg);
 	object = get_reference(revs, arg, sha1, flags ^ local_flags);
 	add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);
-	add_pending_object_with_mode(revs, object, arg, mode);
+	add_pending_object_with_mode(revs, object, arg, oc.mode);
 	return 0;
 }
 
@@ -1794,11 +1794,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (revs->def && !revs->pending.nr && !got_rev_arg) {
 		unsigned char sha1[20];
 		struct object *object;
-		unsigned mode;
-		if (get_sha1_with_mode(revs->def, sha1, &mode))
+		struct object_context oc;
+		if (get_sha1_with_context(revs->def, sha1, &oc))
 			die("bad default revision '%s'", revs->def);
 		object = get_reference(revs, revs->def, sha1, 0);
-		add_pending_object_with_mode(revs, object, revs->def, mode);
+		add_pending_object_with_mode(revs, object, revs->def, oc.mode);
 	}
 
 	/* Did the user ask for any diff output? Run the diff! */
diff --git a/sha1_name.c b/sha1_name.c
index 01ed48b..de41177 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1139,14 +1139,6 @@ void die_on_misspelt_object_name(const char *name, const char *prefix)
 	get_sha1_with_context_1(name, sha1, &oc, 1, prefix);
 }
 
-int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
-{
-	struct object_context oc;
-	int ret = get_sha1_with_context_1(str, sha1, &oc, 0, NULL);
-	*mode = oc.mode;
-	return ret;
-}
-
 int get_sha1_with_context(const char *str, unsigned char *sha1, struct object_context *orc)
 {
 	return get_sha1_with_context_1(str, sha1, orc, 0, NULL);
-- 
1.7.11.1.212.g52fe12e

[PATCH v4 05/18] sha1_name.c: clarify what "fake" is for in find_short_object_filename()

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/sha1_name.c b/sha1_name.c
index de41177..6eb3280 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -17,6 +17,13 @@ static int find_short_object_filename(int len, const char *name, unsigned char *
 	static struct alternate_object_database *fakeent;
 
 	if (!fakeent) {
+		/*
+		 * Create a "fake" alternate object database that
+		 * points to our own object database, to make it
+		 * easier to get a temporary working space in
+		 * alt->name/alt->base while iterating over the
+		 * object databases including our own.
+		 */
 		const char *objdir = get_object_directory();
 		int objdir_len = strlen(objdir);
 		int entlen = objdir_len + 43;
-- 
1.7.11.1.212.g52fe12e

[PATCH v4 06/18] sha1_name.c: rename "now" to "current"

[Junio C Hamano]

This variable points at the element we are currently looking at, and
does not have anything to do with the current time which the name
"now" implies.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 6eb3280..49d2403 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -93,11 +93,11 @@ static int find_short_packed_object(int len, const unsigned char *match, unsigne
 		last = num;
 		while (first < last) {
 			uint32_t mid = (first + last) / 2;
-			const unsigned char *now;
+			const unsigned char *current;
 			int cmp;
 
-			now = nth_packed_object_sha1(p, mid);
-			cmp = hashcmp(match, now);
+			current = nth_packed_object_sha1(p, mid);
+			cmp = hashcmp(match, current);
 			if (!cmp) {
 				first = mid;
 				break;
@@ -109,17 +109,17 @@ static int find_short_packed_object(int len, const unsigned char *match, unsigne
 			last = mid;
 		}
 		if (first < num) {
-			const unsigned char *now, *next;
-			now = nth_packed_object_sha1(p, first);
-			if (match_sha(len, match, now)) {
+			const unsigned char *current, *next;
+			current = nth_packed_object_sha1(p, first);
+			if (match_sha(len, match, current)) {
 				next = nth_packed_object_sha1(p, first+1);
 				if (!next|| !match_sha(len, match, next)) {
 					/* unique within this pack */
 					if (!found) {
-						found_sha1 = now;
+						found_sha1 = current;
 						found++;
 					}
-					else if (hashcmp(found_sha1, now)) {
+					else if (hashcmp(found_sha1, current)) {
 						found = 2;
 						break;
 					}
-- 
1.7.11.1.212.g52fe12e

[PATCH v4 07/18] sha1_name.c: refactor find_short_packed_object()

[Junio C Hamano]

Extract the logic to find object(s) that match a given prefix inside
a single pack into a separate helper function, and give it a bit more
comment.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c | 103 +++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 56 insertions(+), 47 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 49d2403..b78a2c3 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -78,6 +78,59 @@ static int match_sha(unsigned len, const unsigned char *a, const unsigned char *
 	return 1;
 }
 
+static int unique_in_pack(int len,
+			  const unsigned char *match,
+			  struct packed_git *p,
+			  const unsigned char **found_sha1,
+			  int seen_so_far)
+{
+	uint32_t num, last, i, first = 0;
+	const unsigned char *current = NULL;
+
+	open_pack_index(p);
+	num = p->num_objects;
+	last = num;
+	while (first < last) {
+		uint32_t mid = (first + last) / 2;
+		const unsigned char *current;
+		int cmp;
+
+		current = nth_packed_object_sha1(p, mid);
+		cmp = hashcmp(match, current);
+		if (!cmp) {
+			first = mid;
+			break;
+		}
+		if (cmp > 0) {
+			first = mid+1;
+			continue;
+		}
+		last = mid;
+	}
+
+	/*
+	 * At this point, "first" is the location of the lowest object
+	 * with an object name that could match "match".  See if we have
+	 * 0, 1 or more objects that actually match(es).
+	 */
+	for (i = first; i < num; i++) {
+		current = nth_packed_object_sha1(p, first);
+		if (!match_sha(len, match, current))
+			break;
+
+		/* current matches */
+		if (!seen_so_far) {
+			*found_sha1 = current;
+			seen_so_far++;
+		} else if (seen_so_far) {
+			/* is it the same as the one previously found elsewhere? */
+			if (hashcmp(*found_sha1, current))
+				return 2; /* definitely not unique */
+		}
+	}
+	return seen_so_far;
+}
+
 static int find_short_packed_object(int len, const unsigned char *match, unsigned char *sha1)
 {
 	struct packed_git *p;
@@ -85,53 +138,9 @@ static int find_short_packed_object(int len, const unsigned char *match, unsigne
 	int found = 0;
 
 	prepare_packed_git();
-	for (p = packed_git; p && found < 2; p = p->next) {
-		uint32_t num, last;
-		uint32_t first = 0;
-		open_pack_index(p);
-		num = p->num_objects;
-		last = num;
-		while (first < last) {
-			uint32_t mid = (first + last) / 2;
-			const unsigned char *current;
-			int cmp;
-
-			current = nth_packed_object_sha1(p, mid);
-			cmp = hashcmp(match, current);
-			if (!cmp) {
-				first = mid;
-				break;
-			}
-			if (cmp > 0) {
-				first = mid+1;
-				continue;
-			}
-			last = mid;
-		}
-		if (first < num) {
-			const unsigned char *current, *next;
-			current = nth_packed_object_sha1(p, first);
-			if (match_sha(len, match, current)) {
-				next = nth_packed_object_sha1(p, first+1);
-				if (!next|| !match_sha(len, match, next)) {
-					/* unique within this pack */
-					if (!found) {
-						found_sha1 = current;
-						found++;
-					}
-					else if (hashcmp(found_sha1, current)) {
-						found = 2;
-						break;
-					}
-				}
-				else {
-					/* not even unique within this pack */
-					found = 2;
-					break;
-				}
-			}
-		}
-	}
+	for (p = packed_git; p && found < 2; p = p->next)
+		found = unique_in_pack(len, match, p, &found_sha1, found);
+
 	if (found == 1)
 		hashcpy(sha1, found_sha1);
 	return found;
-- 
1.7.11.1.212.g52fe12e

[PATCH v4 08/18] sha1_name.c: correct misnamed "canonical" and "res"

[Junio C Hamano]

These are hexadecimal and binary representation of the short object
name given to the callchain as its input.  Rename them with _pfx
suffix to make it clear they are prefixes, and call them hex and bin
respectively.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index b78a2c3..dc20730 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -9,7 +9,7 @@
 
 static int get_sha1_oneline(const char *, unsigned char *, struct commit_list *);
 
-static int find_short_object_filename(int len, const char *name, unsigned char *sha1)
+static int find_short_object_filename(int len, const char *hex_pfx, unsigned char *sha1)
 {
 	struct alternate_object_database *alt;
 	char hex[40];
@@ -34,18 +34,18 @@ static int find_short_object_filename(int len, const char *name, unsigned char *
 	}
 	fakeent->next = alt_odb_list;
 
-	sprintf(hex, "%.2s", name);
+	sprintf(hex, "%.2s", hex_pfx);
 	for (alt = fakeent; alt && found < 2; alt = alt->next) {
 		struct dirent *de;
 		DIR *dir;
-		sprintf(alt->name, "%.2s/", name);
+		sprintf(alt->name, "%.2s/", hex_pfx);
 		dir = opendir(alt->base);
 		if (!dir)
 			continue;
 		while ((de = readdir(dir)) != NULL) {
 			if (strlen(de->d_name) != 38)
 				continue;
-			if (memcmp(de->d_name, name + 2, len - 2))
+			if (memcmp(de->d_name, hex_pfx + 2, len - 2))
 				continue;
 			if (!found) {
 				memcpy(hex + 2, de->d_name, 38);
@@ -79,7 +79,7 @@ static int match_sha(unsigned len, const unsigned char *a, const unsigned char *
 }
 
 static int unique_in_pack(int len,
-			  const unsigned char *match,
+			  const unsigned char *bin_pfx,
 			  struct packed_git *p,
 			  const unsigned char **found_sha1,
 			  int seen_so_far)
@@ -96,7 +96,7 @@ static int unique_in_pack(int len,
 		int cmp;
 
 		current = nth_packed_object_sha1(p, mid);
-		cmp = hashcmp(match, current);
+		cmp = hashcmp(bin_pfx, current);
 		if (!cmp) {
 			first = mid;
 			break;
@@ -110,12 +110,12 @@ static int unique_in_pack(int len,
 
 	/*
 	 * At this point, "first" is the location of the lowest object
-	 * with an object name that could match "match".  See if we have
+	 * with an object name that could match "bin_pfx".  See if we have
 	 * 0, 1 or more objects that actually match(es).
 	 */
 	for (i = first; i < num; i++) {
 		current = nth_packed_object_sha1(p, first);
-		if (!match_sha(len, match, current))
+		if (!match_sha(len, bin_pfx, current))
 			break;
 
 		/* current matches */
@@ -131,7 +131,7 @@ static int unique_in_pack(int len,
 	return seen_so_far;
 }
 
-static int find_short_packed_object(int len, const unsigned char *match, unsigned char *sha1)
+static int find_short_packed_object(int len, const unsigned char *bin_pfx, unsigned char *sha1)
 {
 	struct packed_git *p;
 	const unsigned char *found_sha1 = NULL;
@@ -139,7 +139,7 @@ static int find_short_packed_object(int len, const unsigned char *match, unsigne
 
 	prepare_packed_git();
 	for (p = packed_git; p && found < 2; p = p->next)
-		found = unique_in_pack(len, match, p, &found_sha1, found);
+		found = unique_in_pack(len, bin_pfx, p, &found_sha1, found);
 
 	if (found == 1)
 		hashcpy(sha1, found_sha1);
@@ -149,15 +149,15 @@ static int find_short_packed_object(int len, const unsigned char *match, unsigne
 #define SHORT_NAME_NOT_FOUND (-1)
 #define SHORT_NAME_AMBIGUOUS (-2)
 
-static int find_unique_short_object(int len, char *canonical,
-				    unsigned char *res, unsigned char *sha1)
+static int find_unique_short_object(int len, char *hex_pfx,
+				    unsigned char *bin_pfx, unsigned char *sha1)
 {
 	int has_unpacked, has_packed;
 	unsigned char unpacked_sha1[20], packed_sha1[20];
 
 	prepare_alt_odb();
-	has_unpacked = find_short_object_filename(len, canonical, unpacked_sha1);
-	has_packed = find_short_packed_object(len, res, packed_sha1);
+	has_unpacked = find_short_object_filename(len, hex_pfx, unpacked_sha1);
+	has_packed = find_short_packed_object(len, bin_pfx, packed_sha1);
 	if (!has_unpacked && !has_packed)
 		return SHORT_NAME_NOT_FOUND;
 	if (1 < has_unpacked || 1 < has_packed)
@@ -177,13 +177,13 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
 			  int quietly)
 {
 	int i, status;
-	char canonical[40];
-	unsigned char res[20];
+	char hex_pfx[40];
+	unsigned char bin_pfx[20];
 
 	if (len < MINIMUM_ABBREV || len > 40)
 		return -1;
-	hashclr(res);
-	memset(canonical, 'x', 40);
+	hashclr(bin_pfx);
+	memset(hex_pfx, 'x', 40);
 	for (i = 0; i < len ;i++) {
 		unsigned char c = name[i];
 		unsigned char val;
@@ -197,15 +197,15 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
 		}
 		else
 			return -1;
-		canonical[i] = c;
+		hex_pfx[i] = c;
 		if (!(i & 1))
 			val <<= 4;
-		res[i >> 1] |= val;
+		bin_pfx[i >> 1] |= val;
 	}
 
-	status = find_unique_short_object(i, canonical, res, sha1);
+	status = find_unique_short_object(i, hex_pfx, bin_pfx, sha1);
 	if (!quietly && (status == SHORT_NAME_AMBIGUOUS))
-		return error("short SHA1 %.*s is ambiguous.", len, canonical);
+		return error("short SHA1 %.*s is ambiguous.", len, hex_pfx);
 	return status;
 }
 
-- 
1.7.11.1.212.g52fe12e

[PATCH v4 09/18] sha1_name.c: restructure disambiguation of short names

[Junio C Hamano]

We try to find zero, one or more matches from loose objects and
packed objects independently and then decide if the given short
object name is unique across them.

Instead, introduce a "struct disambiguate_state" that keeps track of
what we have found so far, that can be one of:

 - We have seen one object that _could_ be what we are looking for;
 - We have also checked that object for additional constraints (if any),
   and found that the object satisfies it;
 - We have also checked that object for additional constraints (if any),
   and found that the object does not satisfy it; or
 - We have seen more than one objects that satisfy the constraints.

and pass it to the enumeration functions for loose and packed
objects.  The disambiguation state can optionally take a callback
function that takes a candidate object name and reports if the
object satisifies additional criteria (e.g. when the caller knows
that the short name must refer to a commit, this mechanism can be
used to check the type of the given object).

Compared to the earlier attempt, this round avoids the optional
check if there is only one candidate that matches the short name in
the first place.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c | 172 +++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 112 insertions(+), 60 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index dc20730..6c585e3 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -9,11 +9,67 @@
 
 static int get_sha1_oneline(const char *, unsigned char *, struct commit_list *);
 
-static int find_short_object_filename(int len, const char *hex_pfx, unsigned char *sha1)
+typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);
+
+struct disambiguate_state {
+	disambiguate_hint_fn fn;
+	void *cb_data;
+	unsigned char candidate[20];
+	unsigned candidate_exists:1;
+	unsigned candidate_checked:1;
+	unsigned candidate_ok:1;
+	unsigned disambiguate_fn_used:1;
+	unsigned ambiguous:1;
+};
+
+static void update_candidates(struct disambiguate_state *ds, const unsigned char *current)
+{
+	if (!ds->candidate_exists) {
+		/* this is the first candidate */
+		hashcpy(ds->candidate, current);
+		ds->candidate_exists = 1;
+		return;
+	} else if (!hashcmp(ds->candidate, current)) {
+		/* the same as what we already have seen */
+		return;
+	}
+
+	if (!ds->fn) {
+		/* cannot disambiguate between ds->candidate and current */
+		ds->ambiguous = 1;
+		return;
+	}
+
+	if (!ds->candidate_checked) {
+		ds->candidate_ok = ds->fn(ds->candidate, ds->cb_data);
+		ds->disambiguate_fn_used = 1;
+		ds->candidate_checked = 1;
+	}
+
+	if (!ds->candidate_ok) {
+		/* discard the candidate; we know it does not satisify fn */
+		hashcpy(ds->candidate, current);
+		ds->candidate_checked = 0;
+		return;
+	}
+
+	/* if we reach this point, we know ds->candidate satisfies fn */
+	if (ds->fn(current, ds->cb_data)) {
+		/*
+		 * if both current and candidate satisfy fn, we cannot
+		 * disambiguate.
+		 */
+		ds->candidate_ok = 0;
+		ds->ambiguous = 1;
+	}
+
+	/* otherwise, current can be discarded and candidate is still good */
+}
+
+static void find_short_object_filename(int len, const char *hex_pfx, struct disambiguate_state *ds)
 {
 	struct alternate_object_database *alt;
 	char hex[40];
-	int found = 0;
 	static struct alternate_object_database *fakeent;
 
 	if (!fakeent) {
@@ -35,32 +91,27 @@ static int find_short_object_filename(int len, const char *hex_pfx, unsigned cha
 	fakeent->next = alt_odb_list;
 
 	sprintf(hex, "%.2s", hex_pfx);
-	for (alt = fakeent; alt && found < 2; alt = alt->next) {
+	for (alt = fakeent; alt && !ds->ambiguous; alt = alt->next) {
 		struct dirent *de;
 		DIR *dir;
 		sprintf(alt->name, "%.2s/", hex_pfx);
 		dir = opendir(alt->base);
 		if (!dir)
 			continue;
-		while ((de = readdir(dir)) != NULL) {
+
+		while (!ds->ambiguous && (de = readdir(dir)) != NULL) {
+			unsigned char sha1[20];
+
 			if (strlen(de->d_name) != 38)
 				continue;
 			if (memcmp(de->d_name, hex_pfx + 2, len - 2))
 				continue;
-			if (!found) {
-				memcpy(hex + 2, de->d_name, 38);
-				found++;
-			}
-			else if (memcmp(hex + 2, de->d_name, 38)) {
-				found = 2;
-				break;
-			}
+			memcpy(hex + 2, de->d_name, 38);
+			if (!get_sha1_hex(hex, sha1))
+				update_candidates(ds, sha1);
 		}
 		closedir(dir);
 	}
-	if (found == 1)
-		return get_sha1_hex(hex, sha1) == 0;
-	return found;
 }
 
 static int match_sha(unsigned len, const unsigned char *a, const unsigned char *b)
@@ -78,11 +129,10 @@ static int match_sha(unsigned len, const unsigned char *a, const unsigned char *
 	return 1;
 }
 
-static int unique_in_pack(int len,
+static void unique_in_pack(int len,
 			  const unsigned char *bin_pfx,
-			  struct packed_git *p,
-			  const unsigned char **found_sha1,
-			  int seen_so_far)
+			   struct packed_git *p,
+			   struct disambiguate_state *ds)
 {
 	uint32_t num, last, i, first = 0;
 	const unsigned char *current = NULL;
@@ -113,63 +163,58 @@ static int unique_in_pack(int len,
 	 * with an object name that could match "bin_pfx".  See if we have
 	 * 0, 1 or more objects that actually match(es).
 	 */
-	for (i = first; i < num; i++) {
-		current = nth_packed_object_sha1(p, first);
+	for (i = first; i < num && !ds->ambiguous; i++) {
+		current = nth_packed_object_sha1(p, i);
 		if (!match_sha(len, bin_pfx, current))
 			break;
-
-		/* current matches */
-		if (!seen_so_far) {
-			*found_sha1 = current;
-			seen_so_far++;
-		} else if (seen_so_far) {
-			/* is it the same as the one previously found elsewhere? */
-			if (hashcmp(*found_sha1, current))
-				return 2; /* definitely not unique */
-		}
+		update_candidates(ds, current);
 	}
-	return seen_so_far;
 }
 
-static int find_short_packed_object(int len, const unsigned char *bin_pfx, unsigned char *sha1)
+static void find_short_packed_object(int len, const unsigned char *bin_pfx,
+				     struct disambiguate_state *ds)
 {
 	struct packed_git *p;
-	const unsigned char *found_sha1 = NULL;
-	int found = 0;
 
 	prepare_packed_git();
-	for (p = packed_git; p && found < 2; p = p->next)
-		found = unique_in_pack(len, bin_pfx, p, &found_sha1, found);
-
-	if (found == 1)
-		hashcpy(sha1, found_sha1);
-	return found;
+	for (p = packed_git; p && !ds->ambiguous; p = p->next)
+		unique_in_pack(len, bin_pfx, p, ds);
 }
 
 #define SHORT_NAME_NOT_FOUND (-1)
 #define SHORT_NAME_AMBIGUOUS (-2)
 
-static int find_unique_short_object(int len, char *hex_pfx,
-				    unsigned char *bin_pfx, unsigned char *sha1)
+static int finish_object_disambiguation(struct disambiguate_state *ds,
+					unsigned char *sha1)
 {
-	int has_unpacked, has_packed;
-	unsigned char unpacked_sha1[20], packed_sha1[20];
+	if (ds->ambiguous)
+		return SHORT_NAME_AMBIGUOUS;
 
-	prepare_alt_odb();
-	has_unpacked = find_short_object_filename(len, hex_pfx, unpacked_sha1);
-	has_packed = find_short_packed_object(len, bin_pfx, packed_sha1);
-	if (!has_unpacked && !has_packed)
+	if (!ds->candidate_exists)
 		return SHORT_NAME_NOT_FOUND;
-	if (1 < has_unpacked || 1 < has_packed)
-		return SHORT_NAME_AMBIGUOUS;
-	if (has_unpacked != has_packed) {
-		hashcpy(sha1, (has_packed ? packed_sha1 : unpacked_sha1));
-		return 0;
-	}
-	/* Both have unique ones -- do they match? */
-	if (hashcmp(packed_sha1, unpacked_sha1))
-		return SHORT_NAME_AMBIGUOUS;
-	hashcpy(sha1, packed_sha1);
+
+	if (!ds->candidate_checked)
+		/*
+		 * If this is the only candidate, there is no point
+		 * calling the disambiguation hint callback.
+		 *
+		 * On the other hand, if the current candidate
+		 * replaced an earlier candidate that did _not_ pass
+		 * the disambiguation hint callback, then we do have
+		 * more than one objects that match the short name
+		 * given, so we should make sure this one matches;
+		 * otherwise, if we discovered this one and the one
+		 * that we previously discarded in the reverse order,
+		 * we would end up showing different results in the
+		 * same repository!
+		 */
+		ds->candidate_ok = (!ds->disambiguate_fn_used ||
+				    ds->fn(ds->candidate, ds->cb_data));
+
+	if (!ds->candidate_ok)
+		return SHORT_NAME_NOT_FOUND;
+
+	hashcpy(sha1, ds->candidate);
 	return 0;
 }
 
@@ -179,6 +224,7 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
 	int i, status;
 	char hex_pfx[40];
 	unsigned char bin_pfx[20];
+	struct disambiguate_state ds;
 
 	if (len < MINIMUM_ABBREV || len > 40)
 		return -1;
@@ -203,7 +249,13 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
 		bin_pfx[i >> 1] |= val;
 	}
 
-	status = find_unique_short_object(i, hex_pfx, bin_pfx, sha1);
+	prepare_alt_odb();
+
+	memset(&ds, 0, sizeof(ds));
+	find_short_object_filename(len, hex_pfx, &ds);
+	find_short_packed_object(len, bin_pfx, &ds);
+	status = finish_object_disambiguation(&ds, sha1);
+
 	if (!quietly && (status == SHORT_NAME_AMBIGUOUS))
 		return error("short SHA1 %.*s is ambiguous.", len, hex_pfx);
 	return status;
-- 
1.7.11.1.212.g52fe12e

[PATCH v4 10/18] sha1_name.c: allow get_short_sha1() to take other flags

[Junio C Hamano]

Instead of a separate "int quietly" argument, make it take "unsigned
flags" so that we can pass other options to it.

The bit assignment of this flag word is exposed in cache.h because
the mechanism will be exposed to callers of the higher layer in
later commits in this series.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h     | 2 ++
 sha1_name.c | 7 ++++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 3066b03..fd5b7f6 100644
--- a/cache.h
+++ b/cache.h
@@ -811,6 +811,8 @@ struct object_context {
 	unsigned mode;
 };
 
+#define GET_SHA1_QUIETLY 01
+
 extern int get_sha1(const char *str, unsigned char *sha1);
 extern void die_on_misspelt_object_name(const char *name, const char *prefix);
 extern int get_sha1_with_context(const char *str, unsigned char *sha1, struct object_context *orc);
diff --git a/sha1_name.c b/sha1_name.c
index 6c585e3..01cce9f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -219,12 +219,13 @@ static int finish_object_disambiguation(struct disambiguate_state *ds,
 }
 
 static int get_short_sha1(const char *name, int len, unsigned char *sha1,
-			  int quietly)
+			  unsigned flags)
 {
 	int i, status;
 	char hex_pfx[40];
 	unsigned char bin_pfx[20];
 	struct disambiguate_state ds;
+	int quietly = !!(flags & GET_SHA1_QUIETLY);
 
 	if (len < MINIMUM_ABBREV || len > 40)
 		return -1;
@@ -272,7 +273,7 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
 		return hex;
 	while (len < 40) {
 		unsigned char sha1_ret[20];
-		status = get_short_sha1(hex, len, sha1_ret, 1);
+		status = get_short_sha1(hex, len, sha1_ret, GET_SHA1_QUIETLY);
 		if (exists
 		    ? !status
 		    : status == SHORT_NAME_NOT_FOUND) {
@@ -603,7 +604,7 @@ static int get_describe_name(const char *name, int len, unsigned char *sha1)
 			if (ch == 'g' && cp[-1] == '-') {
 				cp++;
 				len -= cp - name;
-				return get_short_sha1(cp, len, sha1, 1);
+				return get_short_sha1(cp, len, sha1, GET_SHA1_QUIETLY);
 			}
 		}
 	}
-- 
1.7.11.1.212.g52fe12e

[PATCH v4 11/18] sha1_name.c: teach get_short_sha1() a commit-only option

[Junio C Hamano]

When the caller knows that the parameter is meant to name a commit,
e.g. "56789a" in describe name "v1.2.3-4-g56789a", pass that as a
hint so that lower level can use it to disambiguate objects when
there is only one commit whose name begins with 56789a even if there
are objects of other types whose names share the same prefix.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h     | 1 +
 sha1_name.c | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/cache.h b/cache.h
index fd5b7f6..29f0b2a 100644
--- a/cache.h
+++ b/cache.h
@@ -812,6 +812,7 @@ struct object_context {
 };
 
 #define GET_SHA1_QUIETLY 01
+#define GET_SHA1_COMMIT 02
 
 extern int get_sha1(const char *str, unsigned char *sha1);
 extern void die_on_misspelt_object_name(const char *name, const char *prefix);
diff --git a/sha1_name.c b/sha1_name.c
index 01cce9f..be15d94 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -218,6 +218,12 @@ static int finish_object_disambiguation(struct disambiguate_state *ds,
 	return 0;
 }
 
+static int disambiguate_commit_only(const unsigned char *sha1, void *cb_data_unused)
+{
+	int kind = sha1_object_info(sha1, NULL);
+	return kind == OBJ_COMMIT;
+}
+
 static int get_short_sha1(const char *name, int len, unsigned char *sha1,
 			  unsigned flags)
 {
@@ -253,6 +259,9 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
 	prepare_alt_odb();
 
 	memset(&ds, 0, sizeof(ds));
+	if (flags & GET_SHA1_COMMIT)
+		ds.fn = disambiguate_commit_only;
+
 	find_short_object_filename(len, hex_pfx, &ds);
 	find_short_packed_object(len, bin_pfx, &ds);
 	status = finish_object_disambiguation(&ds, sha1);
-- 
1.7.11.1.212.g52fe12e

[PATCH v4 12/18] sha1_name.c: get_describe_name() by definition groks only commits

[Junio C Hamano]

Teach get_describe_name() to pass the disambiguation hint down the
callchain to get_short_sha1().

Also add tests to show various syntactic elements that we could take
advantage of the object type information to help disambiguration of
abbreviated object names.  Many of them are marked as broken, and
some of them will be fixed in later patches in this series.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c                         |   3 +-
 t/t1512-rev-parse-disambiguation.sh | 208 ++++++++++++++++++++++++++++++++++++
 2 files changed, 210 insertions(+), 1 deletion(-)
 create mode 100755 t/t1512-rev-parse-disambiguation.sh

diff --git a/sha1_name.c b/sha1_name.c
index be15d94..9af84b1 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -603,6 +603,7 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 static int get_describe_name(const char *name, int len, unsigned char *sha1)
 {
 	const char *cp;
+	unsigned flags = GET_SHA1_QUIETLY | GET_SHA1_COMMIT;
 
 	for (cp = name + len - 1; name + 2 <= cp; cp--) {
 		char ch = *cp;
@@ -613,7 +614,7 @@ static int get_describe_name(const char *name, int len, unsigned char *sha1)
 			if (ch == 'g' && cp[-1] == '-') {
 				cp++;
 				len -= cp - name;
-				return get_short_sha1(cp, len, sha1, GET_SHA1_QUIETLY);
+				return get_short_sha1(cp, len, sha1, flags);
 			}
 		}
 	}
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
new file mode 100755
index 0000000..42701ad
--- /dev/null
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -0,0 +1,208 @@
+#!/bin/sh
+
+test_description='object name disambiguation
+
+Create blobs, trees, commits and a tag that all share the same
+prefix, and make sure "git rev-parse" can take advantage of
+type information to disambiguate short object names that are
+not necessarily unique.
+
+The final history used in the test has five commits, with the bottom
+one tagged as v1.0.0.  They all have one regular file each.
+
+  +-------------------------------------------+
+  |                                           |
+  |           .-------cs60sb5----- d59np2z    |
+  |          /                   /            |
+  |  d8znjge0 ---a2dit76---b4crl15            |
+  |                                           |
+  +-------------------------------------------+
+
+'
+
+. ./test-lib.sh
+
+test_expect_success 'blob and tree' '
+	(
+		for i in 0 1 2 3 4 5 6 7 8 9
+		do
+			echo $i
+		done
+		echo
+		echo cbpkuqa
+	) >bz01t33 &&
+
+	# create one blob 1102198254
+	git add bz01t33 &&
+
+	# create one tree 1102198206
+	git write-tree
+'
+
+test_expect_failure 'disambiguate tree-ish' '
+	# feed tree-ish in an unambiguous way
+	git rev-parse --verify 1102198206:bz01t33 &&
+
+	# ambiguous at the object name level, but there is only one
+	# such tree-ish (the other is a blob)
+	git rev-parse --verify 11021982:bz01t33
+'
+
+test_expect_success 'first commit' '
+	# create one commit 1102198268
+	test_tick &&
+	git commit -m d8znjge0
+'
+
+test_expect_failure 'disambiguate commit-ish' '
+	# feed commit-ish in an unambiguous way
+	git rev-parse --verify 1102198268^{commit} &&
+
+	# ambiguous at the object name level, but there is only one
+	# such commit (the others are tree and blob)
+	git rev-parse --verify 11021982^{commit} &&
+
+	# likewise
+	git rev-parse --verify 11021982^0
+'
+
+test_expect_failure 'name1..name2 takes only commit-ishes on both ends' '
+	git log 11021982..11021982 &&
+	git log ..11021982 &&
+	git log 11021982.. &&
+	git log 11021982...11021982 &&
+	git log ...11021982 &&
+	git log 11021982...
+'
+
+test_expect_failure 'git log takes only commit-ish' '
+	git log 11021982
+'
+
+test_expect_success 'first tag' '
+	# create one tag 1102198256
+	git tag -a -m d3g97ek v1.0.0
+'
+
+test_expect_failure 'two semi-ambiguous commit-ish' '
+	# Once the parser becomes ultra-smart, it could notice that
+	# 110282 before ^{commit} name many different objects, but
+	# that only two (HEAD and v1.0.0 tag) can be peeled to commit,
+	# and that peeling them down to commit yield the same commit
+	# without ambiguity.
+	git rev-parse --verify 110282^{commit} &&
+
+	# likewise
+	git log 11021982..11021982 &&
+	git log ..11021982 &&
+	git log 11021982.. &&
+	git log 11021982...11021982 &&
+	git log ...11021982 &&
+	git log 11021982...
+'
+
+test_expect_failure 'three semi-ambiguous tree-ish' '
+	# Likewise for tree-ish.  HEAD, v1.0.0 and HEAD^{tree} share
+	# the prefix but peeling them to tree yields the same thing
+	git rev-parse --verify 110282^{tree}
+'
+
+test_expect_success 'parse describe name' '
+	# feed an unambiguous describe name
+	git rev-parse --verify v1.0.0-0-g1102198268 &&
+
+	# ambiguous at the object name level, but there is only one
+	# such commit (others are blob, tree and tag)
+	git rev-parse --verify v1.0.0-0-g11021982
+'
+
+test_expect_success 'more history' '
+	# commit 110219822
+	git mv bz01t33 b90au8x &&
+	echo bdc8xi8 >>b90au8x &&
+	git add b90au8x &&
+
+	test_tick &&
+	git commit -m a2dit76 &&
+
+	# commit 110219828
+	git mv b90au8x djintr2 &&
+	echo a2a49zt >>djintr2 &&
+	git add djintr2 &&
+
+	test_tick &&
+	git commit -m b4crl15 &&
+
+	# commit 11021982d
+	git checkout v1.0.0^0 &&
+	git mv bz01t33 ddp2qt1 &&
+
+	for i in bdc8xi8 a2a49zt c0l9aeu
+	do
+		echo $i
+	done >>ddp2qt1 &&
+	git add ddp2qt1 &&
+
+	test_tick &&
+	git commit -m cs60sb5 &&
+	side=$(git rev-parse HEAD) &&
+
+	# commit 11021982a
+	git checkout master &&
+
+	# If you use recursive, merge will fail and you will need to
+	# clean up bz01t33 as well.  If you use resolve, merge will
+	# succeed.
+	test_might_fail git merge --no-commit -s resolve $side &&
+	git rm -f ddp2qt1 djintr2 &&
+	test_might_fail git rm -f bz01t33 &&
+	(
+		git cat-file blob $side:ddp2qt1
+		echo d4o8f4f
+	) >c3q9963 &&
+	git add c3q9963 &&
+
+	test_tick &&
+	git commit -m d59np2z
+
+'
+
+test_expect_failure 'parse describe name taking advantage of generation' '
+	# ambiguous at the object name level, but there is only one
+	# such commit at generation 0
+	git rev-parse --verify v1.0.0-0-g11021982 &&
+
+	# likewise for generation 2 and 4
+	git rev-parse --verify v1.0.0-2-g11021982 &&
+	git rev-parse --verify v1.0.0-4-g11021982
+'
+
+# Note: because rev-parse does not even try to disambiguate based on
+# the generation number, this test currently succeeds for a wrong
+# reason.  When it learns to use the generation number, the previous
+# test should succeed, and also this test should fail because the
+# describe name used in the test with generation number can name two
+# commits.  Make sure that such a future enhancement does not randomly
+# pick one.
+test_expect_success 'parse describe name not ignoring ambiguity' '
+	# ambiguous at the object name level, and there are two such
+	# commits at generation 1
+	test_must_fail git rev-parse --verify v1.0.0-1-g11021982
+'
+
+test_expect_success 'ambiguous commit-ish' '
+	# Now there are many commits that begin with the
+	# common prefix, none of these should pick one at
+	# random.  They all should result in ambiguity errors.
+	test_must_fail git rev-parse --verify 110282^{commit} &&
+
+	# likewise
+	test_must_fail git log 11021982..11021982 &&
+	test_must_fail git log ..11021982 &&
+	test_must_fail git log 11021982.. &&
+	test_must_fail git log 11021982...11021982 &&
+	test_must_fail git log ...11021982 &&
+	test_must_fail git log 11021982...
+'
+
+test_done
-- 
1.7.11.1.212.g52fe12e

[PATCH v4 13/18] sha1_name.c: get_sha1_1() takes lookup flags

[Junio C Hamano]

This is to pass the disambiguation hints from the caller down the
callchain.  Nothing is changed in this step, as everybody just
passes 0 in the flag.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 9af84b1..6f6c49c 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -333,7 +333,7 @@ static inline int upstream_mark(const char *string, int len)
 	return 0;
 }
 
-static int get_sha1_1(const char *name, int len, unsigned char *sha1);
+static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);
 
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 {
@@ -370,7 +370,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		ret = interpret_branch_name(str+at, &buf);
 		if (ret > 0) {
 			/* substitute this branch name and restart */
-			return get_sha1_1(buf.buf, buf.len, sha1);
+			return get_sha1_1(buf.buf, buf.len, sha1, 0);
 		} else if (ret == 0) {
 			return -1;
 		}
@@ -440,7 +440,7 @@ static int get_parent(const char *name, int len,
 		      unsigned char *result, int idx)
 {
 	unsigned char sha1[20];
-	int ret = get_sha1_1(name, len, sha1);
+	int ret = get_sha1_1(name, len, sha1, 0);
 	struct commit *commit;
 	struct commit_list *p;
 
@@ -473,7 +473,7 @@ static int get_nth_ancestor(const char *name, int len,
 	struct commit *commit;
 	int ret;
 
-	ret = get_sha1_1(name, len, sha1);
+	ret = get_sha1_1(name, len, sha1, 0);
 	if (ret)
 		return ret;
 	commit = lookup_commit_reference(sha1);
@@ -554,7 +554,7 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 	else
 		return -1;
 
-	if (get_sha1_1(name, sp - name - 2, outer))
+	if (get_sha1_1(name, sp - name - 2, outer, 0))
 		return -1;
 
 	o = parse_object(outer);
@@ -621,7 +621,7 @@ static int get_describe_name(const char *name, int len, unsigned char *sha1)
 	return -1;
 }
 
-static int get_sha1_1(const char *name, int len, unsigned char *sha1)
+static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags)
 {
 	int ret, has_suffix;
 	const char *cp;
@@ -1098,7 +1098,7 @@ static int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 
 	memset(oc, 0, sizeof(*oc));
 	oc->mode = S_IFINVALID;
-	ret = get_sha1_1(name, namelen, sha1);
+	ret = get_sha1_1(name, namelen, sha1, 0);
 	if (!ret)
 		return ret;
 	/* sha1:path --> object name of path in ent sha1
@@ -1176,7 +1176,7 @@ static int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 			strncpy(object_name, name, cp-name);
 			object_name[cp-name] = '\0';
 		}
-		if (!get_sha1_1(name, cp-name, tree_sha1)) {
+		if (!get_sha1_1(name, cp-name, tree_sha1, 0)) {
 			const char *filename = cp+1;
 			char *new_filename = NULL;
 
-- 
1.7.11.1.212.g52fe12e

[PATCH v4 14/18] sha1_name.c: many short names can only be committish

[Junio C Hamano]

We know that the token "$name" that appear in "$name^{commit}",
"$name^4", "$name~4" etc. can only name a committish (either a
commit or a tag that peels to a commit).  Teach get_short_sha1() to
take advantage of that knowledge when disambiguating an abbreviated
SHA-1 given as an object name.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h                             |  1 +
 sha1_name.c                         | 32 ++++++++++++++++++++++++++++----
 t/t1512-rev-parse-disambiguation.sh |  2 +-
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index 29f0b2a..1acf3b4 100644
--- a/cache.h
+++ b/cache.h
@@ -813,6 +813,7 @@ struct object_context {
 
 #define GET_SHA1_QUIETLY 01
 #define GET_SHA1_COMMIT 02
+#define GET_SHA1_COMMITTISH 04
 
 extern int get_sha1(const char *str, unsigned char *sha1);
 extern void die_on_misspelt_object_name(const char *name, const char *prefix);
diff --git a/sha1_name.c b/sha1_name.c
index 6f6c49c..35ad473 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -224,6 +224,24 @@ static int disambiguate_commit_only(const unsigned char *sha1, void *cb_data_unu
 	return kind == OBJ_COMMIT;
 }
 
+static int disambiguate_committish_only(const unsigned char *sha1, void *cb_data_unused)
+{
+	struct object *obj;
+	int kind;
+
+	kind = sha1_object_info(sha1, NULL);
+	if (kind == OBJ_COMMIT)
+		return 1;
+	if (kind != OBJ_TAG)
+		return 0;
+
+	/* We need to do this the hard way... */
+	obj = deref_tag(lookup_object(sha1), NULL, 0);
+	if (obj && obj->type == OBJ_COMMIT)
+		return 1;
+	return 0;
+}
+
 static int get_short_sha1(const char *name, int len, unsigned char *sha1,
 			  unsigned flags)
 {
@@ -261,6 +279,8 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
 	memset(&ds, 0, sizeof(ds));
 	if (flags & GET_SHA1_COMMIT)
 		ds.fn = disambiguate_commit_only;
+	else if (flags & GET_SHA1_COMMITTISH)
+		ds.fn = disambiguate_committish_only;
 
 	find_short_object_filename(len, hex_pfx, &ds);
 	find_short_packed_object(len, bin_pfx, &ds);
@@ -440,9 +460,9 @@ static int get_parent(const char *name, int len,
 		      unsigned char *result, int idx)
 {
 	unsigned char sha1[20];
-	int ret = get_sha1_1(name, len, sha1, 0);
 	struct commit *commit;
 	struct commit_list *p;
+	int ret = get_sha1_1(name, len, sha1, GET_SHA1_COMMITTISH);
 
 	if (ret)
 		return ret;
@@ -473,7 +493,7 @@ static int get_nth_ancestor(const char *name, int len,
 	struct commit *commit;
 	int ret;
 
-	ret = get_sha1_1(name, len, sha1, 0);
+	ret = get_sha1_1(name, len, sha1, GET_SHA1_COMMITTISH);
 	if (ret)
 		return ret;
 	commit = lookup_commit_reference(sha1);
@@ -519,6 +539,7 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 	unsigned char outer[20];
 	const char *sp;
 	unsigned int expected_type = 0;
+	unsigned lookup_flags;
 	struct object *o;
 
 	/*
@@ -554,7 +575,10 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 	else
 		return -1;
 
-	if (get_sha1_1(name, sp - name - 2, outer, 0))
+	if (expected_type == OBJ_COMMIT)
+		lookup_flags = GET_SHA1_COMMITTISH;
+
+	if (get_sha1_1(name, sp - name - 2, outer, lookup_flags))
 		return -1;
 
 	o = parse_object(outer);
@@ -666,7 +690,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l
 	if (!ret)
 		return 0;
 
-	return get_short_sha1(name, len, sha1, 0);
+	return get_short_sha1(name, len, sha1, lookup_flags);
 }
 
 /*
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 42701ad..823e288 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -54,7 +54,7 @@ test_expect_success 'first commit' '
 	git commit -m d8znjge0
 '
 
-test_expect_failure 'disambiguate commit-ish' '
+test_expect_success 'disambiguate commit-ish' '
 	# feed commit-ish in an unambiguous way
 	git rev-parse --verify 1102198268^{commit} &&
 
-- 
1.7.11.1.212.g52fe12e

Re: [PATCH v4 14/18] sha1_name.c: many short names can only be committish

[Junio C Hamano]

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

> diff --git a/sha1_name.c b/sha1_name.c
> index 6f6c49c..35ad473 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -519,6 +539,7 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
>  	unsigned char outer[20];
>  	const char *sp;
>  	unsigned int expected_type = 0;
> +	unsigned lookup_flags;

This needs to be initialized to 0, i.e.

	unsigned lookup_flags = 0;

for the following two hunks to make sense.

> @@ -554,7 +575,10 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
>  	else
>  		return -1;
>  
> -	if (get_sha1_1(name, sp - name - 2, outer, 0))
> +	if (expected_type == OBJ_COMMIT)
> +		lookup_flags = GET_SHA1_COMMITTISH;
> +
> +	if (get_sha1_1(name, sp - name - 2, outer, lookup_flags))
>  		return -1;
>  
>  	o = parse_object(outer);
> @@ -666,7 +690,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l
>  	if (!ret)
>  		return 0;
>  
> -	return get_short_sha1(name, len, sha1, 0);
> +	return get_short_sha1(name, len, sha1, lookup_flags);
>  }
>  
>  /*

[PATCH v4 15/18] sha1_name.c: teach lookup context to get_sha1_with_context()

[Junio C Hamano]

The function takes user input string and returns the object name
(binary SHA-1) with mode bits and path when the object was looked
up in a tree.

Additionally give hints to help disambiguation of abbreviated object
names when the caller knows what it is looking for.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/cat-file.c |  2 +-
 cache.h            |  3 ++-
 revision.c         |  4 ++--
 sha1_name.c        | 22 +++++++++++++---------
 4 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 07bd984..c27268f 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -90,7 +90,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
 	unsigned long size;
 	struct object_context obj_context;
 
-	if (get_sha1_with_context(obj_name, sha1, &obj_context))
+	if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
 		die("Not a valid object name %s", obj_name);
 
 	buf = NULL;
diff --git a/cache.h b/cache.h
index 1acf3b4..d97fa2d 100644
--- a/cache.h
+++ b/cache.h
@@ -814,10 +814,11 @@ struct object_context {
 #define GET_SHA1_QUIETLY 01
 #define GET_SHA1_COMMIT 02
 #define GET_SHA1_COMMITTISH 04
+#define GET_SHA1_ONLY_TO_DIE 04000
 
 extern int get_sha1(const char *str, unsigned char *sha1);
 extern void die_on_misspelt_object_name(const char *name, const char *prefix);
-extern int get_sha1_with_context(const char *str, unsigned char *sha1, struct object_context *orc);
+extern int get_sha1_with_context(const char *str, unsigned flags, unsigned char *sha1, struct object_context *orc);
 
 /*
  * Try to read a SHA1 in hexadecimal format from the 40 characters
diff --git a/revision.c b/revision.c
index 86a14c8..7444f2e 100644
--- a/revision.c
+++ b/revision.c
@@ -1180,7 +1180,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs,
 		local_flags = UNINTERESTING;
 		arg++;
 	}
-	if (get_sha1_with_context(arg, sha1, &oc))
+	if (get_sha1_with_context(arg, 0, sha1, &oc))
 		return revs->ignore_missing ? 0 : -1;
 	if (!cant_be_filename)
 		verify_non_filename(revs->prefix, arg);
@@ -1795,7 +1795,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		unsigned char sha1[20];
 		struct object *object;
 		struct object_context oc;
-		if (get_sha1_with_context(revs->def, sha1, &oc))
+		if (get_sha1_with_context(revs->def, 0, sha1, &oc))
 			die("bad default revision '%s'", revs->def);
 		object = get_reference(revs, revs->def, sha1, 0);
 		add_pending_object_with_mode(revs, object, revs->def, oc.mode);
diff --git a/sha1_name.c b/sha1_name.c
index 35ad473..b5f0be5 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -996,7 +996,7 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 int get_sha1(const char *name, unsigned char *sha1)
 {
 	struct object_context unused;
-	return get_sha1_with_context(name, sha1, &unused);
+	return get_sha1_with_context(name, 0, sha1, &unused);
 }
 
 /* Must be called only when object_name:filename doesn't exist. */
@@ -1112,20 +1112,24 @@ static char *resolve_relative_path(const char *rel)
 			   rel);
 }
 
-static int get_sha1_with_context_1(const char *name, unsigned char *sha1,
-				   struct object_context *oc,
-				   int only_to_die, const char *prefix)
+static int get_sha1_with_context_1(const char *name,
+				   unsigned flags,
+				   const char *prefix,
+				   unsigned char *sha1,
+				   struct object_context *oc)
 {
 	int ret, bracket_depth;
 	int namelen = strlen(name);
 	const char *cp;
+	int only_to_die = flags & GET_SHA1_ONLY_TO_DIE;
 
 	memset(oc, 0, sizeof(*oc));
 	oc->mode = S_IFINVALID;
-	ret = get_sha1_1(name, namelen, sha1, 0);
+	ret = get_sha1_1(name, namelen, sha1, flags);
 	if (!ret)
 		return ret;
-	/* sha1:path --> object name of path in ent sha1
+	/*
+	 * sha1:path --> object name of path in ent sha1
 	 * :path -> object name of absolute path in index
 	 * :./path -> object name of path relative to cwd in index
 	 * :[0-3]:path -> object name of path in index at stage
@@ -1239,10 +1243,10 @@ void die_on_misspelt_object_name(const char *name, const char *prefix)
 {
 	struct object_context oc;
 	unsigned char sha1[20];
-	get_sha1_with_context_1(name, sha1, &oc, 1, prefix);
+	get_sha1_with_context_1(name, GET_SHA1_ONLY_TO_DIE, prefix, sha1, &oc);
 }
 
-int get_sha1_with_context(const char *str, unsigned char *sha1, struct object_context *orc)
+int get_sha1_with_context(const char *str, unsigned flags, unsigned char *sha1, struct object_context *orc)
 {
-	return get_sha1_with_context_1(str, sha1, orc, 0, NULL);
+	return get_sha1_with_context_1(str, flags, NULL, sha1, orc);
 }
-- 
1.7.11.1.212.g52fe12e

[PATCH v4 16/18] sha1_name.c: introduce get_sha1_committish()

[Junio C Hamano]

Many callers know that the user meant to name a committish by
syntactical positions where the object name appears.  Calling this
function allows the machinery to disambiguate shorter-than-unique
abbreviated object names between committish and others.

Note that this does NOT error out when the named object is not a
committish. It is merely to give a hint to the disambiguation
machinery.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h                             |  1 +
 commit.c                            |  2 +-
 revision.c                          |  6 +++---
 sha1_name.c                         | 21 +++++++++++++++++++--
 t/t1512-rev-parse-disambiguation.sh |  2 +-
 5 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/cache.h b/cache.h
index d97fa2d..db84eac 100644
--- a/cache.h
+++ b/cache.h
@@ -817,6 +817,7 @@ struct object_context {
 #define GET_SHA1_ONLY_TO_DIE 04000
 
 extern int get_sha1(const char *str, unsigned char *sha1);
+extern int get_sha1_committish(const char *str, unsigned char *sha1);
 extern void die_on_misspelt_object_name(const char *name, const char *prefix);
 extern int get_sha1_with_context(const char *str, unsigned flags, unsigned char *sha1, struct object_context *orc);
 
diff --git a/commit.c b/commit.c
index 35af498..8b84eff 100644
--- a/commit.c
+++ b/commit.c
@@ -67,7 +67,7 @@ struct commit *lookup_commit_reference_by_name(const char *name)
 	unsigned char sha1[20];
 	struct commit *commit;
 
-	if (get_sha1(name, sha1))
+	if (get_sha1_committish(name, sha1))
 		return NULL;
 	commit = lookup_commit_reference(sha1);
 	if (!commit || parse_commit(commit))
diff --git a/revision.c b/revision.c
index 7444f2e..c3160f2 100644
--- a/revision.c
+++ b/revision.c
@@ -979,7 +979,7 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags)
 		flags ^= UNINTERESTING;
 		arg++;
 	}
-	if (get_sha1(arg, sha1))
+	if (get_sha1_committish(arg, sha1))
 		return 0;
 	while (1) {
 		it = get_reference(revs, arg, sha1, 0);
@@ -1120,8 +1120,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs,
 			next = "HEAD";
 		if (dotdot == arg)
 			this = "HEAD";
-		if (!get_sha1(this, from_sha1) &&
-		    !get_sha1(next, sha1)) {
+		if (!get_sha1_committish(this, from_sha1) &&
+		    !get_sha1_committish(next, sha1)) {
 			struct commit *a, *b;
 			struct commit_list *exclude;
 
diff --git a/sha1_name.c b/sha1_name.c
index b5f0be5..49e6afd 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -872,7 +872,7 @@ int get_sha1_mb(const char *name, unsigned char *sha1)
 		struct strbuf sb;
 		strbuf_init(&sb, dots - name);
 		strbuf_add(&sb, name, dots - name);
-		st = get_sha1(sb.buf, sha1_tmp);
+		st = get_sha1_committish(sb.buf, sha1_tmp);
 		strbuf_release(&sb);
 	}
 	if (st)
@@ -881,7 +881,7 @@ int get_sha1_mb(const char *name, unsigned char *sha1)
 	if (!one)
 		return -1;
 
-	if (get_sha1(dots[3] ? (dots + 3) : "HEAD", sha1_tmp))
+	if (get_sha1_committish(dots[3] ? (dots + 3) : "HEAD", sha1_tmp))
 		return -1;
 	two = lookup_commit_reference_gently(sha1_tmp, 0);
 	if (!two)
@@ -999,6 +999,23 @@ int get_sha1(const char *name, unsigned char *sha1)
 	return get_sha1_with_context(name, 0, sha1, &unused);
 }
 
+/*
+ * Many callers know that the user meant to name a committish by
+ * syntactical positions where the object name appears.  Calling this
+ * function allows the machinery to disambiguate shorter-than-unique
+ * abbreviated object names between committish and others.
+ *
+ * Note that this does NOT error out when the named object is not a
+ * committish. It is merely to give a hint to the disambiguation
+ * machinery.
+ */
+int get_sha1_committish(const char *name, unsigned char *sha1)
+{
+	struct object_context unused;
+	return get_sha1_with_context(name, GET_SHA1_COMMITTISH,
+				     sha1, &unused);
+}
+
 /* Must be called only when object_name:filename doesn't exist. */
 static void diagnose_invalid_sha1_path(const char *prefix,
 				       const char *filename,
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 823e288..6ced848 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -66,7 +66,7 @@ test_expect_success 'disambiguate commit-ish' '
 	git rev-parse --verify 11021982^0
 '
 
-test_expect_failure 'name1..name2 takes only commit-ishes on both ends' '
+test_expect_success 'name1..name2 takes only commit-ishes on both ends' '
 	git log 11021982..11021982 &&
 	git log ..11021982 &&
 	git log 11021982.. &&
-- 
1.7.11.1.212.g52fe12e

[PATCH v4 17/18] revision.c: allow handle_revision_arg() to take other flags

[Junio C Hamano]

The existing "cant_be_filename" that tells the function that the
caller knows the arg is not a path (hence it does not have to be
checked for absense of the file whose name matches it) is made into
a bit in the flag word.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/pack-objects.c |  2 +-
 revision.c             | 13 +++++++------
 revision.h             |  3 ++-
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 0f2e7b8..48ccadd 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2290,7 +2290,7 @@ static void get_object_list(int ac, const char **av)
 			}
 			die("not a rev '%s'", line);
 		}
-		if (handle_revision_arg(line, &revs, flags, 1))
+		if (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))
 			die("bad revision '%s'", line);
 	}
 
diff --git a/revision.c b/revision.c
index c3160f2..929497f 100644
--- a/revision.c
+++ b/revision.c
@@ -1093,9 +1093,7 @@ static void prepare_show_merge(struct rev_info *revs)
 	revs->limited = 1;
 }
 
-int handle_revision_arg(const char *arg_, struct rev_info *revs,
-			int flags,
-			int cant_be_filename)
+int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)
 {
 	struct object_context oc;
 	char *dotdot;
@@ -1103,6 +1101,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs,
 	unsigned char sha1[20];
 	int local_flags;
 	const char *arg = arg_;
+	int cant_be_filename = revarg_opt & REVARG_CANNOT_BE_FILENAME;
 
 	dotdot = strstr(arg, "..");
 	if (dotdot) {
@@ -1236,7 +1235,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,
 			}
 			die("options not supported in --stdin mode");
 		}
-		if (handle_revision_arg(sb.buf, revs, 0, 1))
+		if (handle_revision_arg(sb.buf, revs, 0, REVARG_CANNOT_BE_FILENAME))
 			die("bad revision '%s'", sb.buf);
 	}
 	if (seen_dashdash)
@@ -1684,7 +1683,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
  */
 int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *opt)
 {
-	int i, flags, left, seen_dashdash, read_from_stdin, got_rev_arg = 0;
+	int i, flags, left, seen_dashdash, read_from_stdin, got_rev_arg = 0, revarg_opt;
 	struct cmdline_pathspec prune_data;
 	const char *submodule = NULL;
 
@@ -1708,6 +1707,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 
 	/* Second, deal with arguments and options */
 	flags = 0;
+	revarg_opt = seen_dashdash ? REVARG_CANNOT_BE_FILENAME : 0;
 	read_from_stdin = 0;
 	for (left = i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -1743,7 +1743,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			continue;
 		}
 
-		if (handle_revision_arg(arg, revs, flags, seen_dashdash)) {
+
+		if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
 			int j;
 			if (seen_dashdash || *arg == '^')
 				die("bad revision '%s'", arg);
diff --git a/revision.h b/revision.h
index b8e9223..8eceaec 100644
--- a/revision.h
+++ b/revision.h
@@ -190,7 +190,8 @@ extern int setup_revisions(int argc, const char **argv, struct rev_info *revs, s
 extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 				 const struct option *options,
 				 const char * const usagestr[]);
-extern int handle_revision_arg(const char *arg, struct rev_info *revs,int flags,int cant_be_filename);
+#define REVARG_CANNOT_BE_FILENAME 01
+extern int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt);
 
 extern int prepare_revision_walk(struct rev_info *revs);
 extern struct commit *get_revision(struct rev_info *revs);
-- 
1.7.11.1.212.g52fe12e

[PATCH v4 18/18] revision.c: the "log" family, except for "show", takes committish

[Junio C Hamano]

Add a field to setup_revision_opt structure and allow these callers
to tell the setup_revisions command parsing machinery that short SHA1
it encounters are meant to name committish.

This step does not go all the way to connect the setup_revisions()
to sha1_name.c yet.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c                       |  3 +++
 revision.c                          | 11 +++++++++--
 revision.h                          |  2 ++
 t/t1512-rev-parse-disambiguation.sh |  2 +-
 4 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 7d1f6f8..9363f39 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -363,6 +363,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 	rev.simplify_history = 0;
 	memset(&opt, 0, sizeof(opt));
 	opt.def = "HEAD";
+	opt.revarg_opt = REVARG_COMMITTISH;
 	cmd_log_init(argc, argv, prefix, &rev, &opt);
 	if (!rev.diffopt.output_format)
 		rev.diffopt.output_format = DIFF_FORMAT_RAW;
@@ -543,6 +544,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 	rev.always_show_header = 1;
 	memset(&opt, 0, sizeof(opt));
 	opt.def = "HEAD";
+	opt.revarg_opt = REVARG_COMMITTISH;
 	cmd_log_init(argc, argv, prefix, &rev, &opt);
 	return cmd_log_walk(&rev);
 }
@@ -1144,6 +1146,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.subject_prefix = fmt_patch_subject_prefix;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
 	s_r_opt.def = "HEAD";
+	s_r_opt.revarg_opt = REVARG_COMMITTISH;
 
 	if (default_attach) {
 		rev.mime_boundary = default_attach;
diff --git a/revision.c b/revision.c
index 929497f..ec6f0c8 100644
--- a/revision.c
+++ b/revision.c
@@ -1102,6 +1102,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
 	int local_flags;
 	const char *arg = arg_;
 	int cant_be_filename = revarg_opt & REVARG_CANNOT_BE_FILENAME;
+	unsigned get_sha1_flags = 0;
 
 	dotdot = strstr(arg, "..");
 	if (dotdot) {
@@ -1179,7 +1180,11 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
 		local_flags = UNINTERESTING;
 		arg++;
 	}
-	if (get_sha1_with_context(arg, 0, sha1, &oc))
+
+	if (revarg_opt & REVARG_COMMITTISH)
+		get_sha1_flags = GET_SHA1_COMMITTISH;
+
+	if (get_sha1_with_context(arg, get_sha1_flags, sha1, &oc))
 		return revs->ignore_missing ? 0 : -1;
 	if (!cant_be_filename)
 		verify_non_filename(revs->prefix, arg);
@@ -1707,7 +1712,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 
 	/* Second, deal with arguments and options */
 	flags = 0;
-	revarg_opt = seen_dashdash ? REVARG_CANNOT_BE_FILENAME : 0;
+	revarg_opt = opt ? opt->revarg_opt : 0;
+	if (seen_dashdash)
+		revarg_opt |= REVARG_CANNOT_BE_FILENAME;
 	read_from_stdin = 0;
 	for (left = i = 1; i < argc; i++) {
 		const char *arg = argv[i];
diff --git a/revision.h b/revision.h
index 8eceaec..402f10d 100644
--- a/revision.h
+++ b/revision.h
@@ -183,6 +183,7 @@ struct setup_revision_opt {
 	const char *def;
 	void (*tweak)(struct rev_info *, struct setup_revision_opt *);
 	const char *submodule;
+	unsigned revarg_opt;
 };
 
 extern void init_revisions(struct rev_info *revs, const char *prefix);
@@ -191,6 +192,7 @@ extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ct
 				 const struct option *options,
 				 const char * const usagestr[]);
 #define REVARG_CANNOT_BE_FILENAME 01
+#define REVARG_COMMITTISH 02
 extern int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt);
 
 extern int prepare_revision_walk(struct rev_info *revs);
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 6ced848..aec3e1d 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -75,7 +75,7 @@ test_expect_success 'name1..name2 takes only commit-ishes on both ends' '
 	git log 11021982...
 '
 
-test_expect_failure 'git log takes only commit-ish' '
+test_expect_success 'git log takes only commit-ish' '
 	git log 11021982
 '
 
-- 
1.7.11.1.212.g52fe12e

Re: [PATCH v4 00/18] Extending the shelf-life of "git describe" output

[Jeff King]

On Mon, Jul 02, 2012 at 03:33:51PM -0700, Junio C Hamano wrote:

> This is take 4.  The earlier rounds were $gmane/200165, 200387, and
> 200506.
> 
> Compared to the previous round, it has more patches in the clean-up
> phase.  Most notably, patch 03/18 gets rid of get_sha1_with_mode_1()
> and replaces the only external caller of it with a call to a more
> straightforward die_on_misspelt_object_name().  The test suite added
> by patch 12/18 has more patterns that we can potentially improve on.
> 
> The disambiguation logic can now be asked to pick only committish,
> which can be used in places like "git commit -C deadbeef".  It also
> knows that A and B in "git log A..B" can only be committishes.
> 
> Adding support for treeish, if anybody is tempted to do so, should
> now be pretty straightforward.

I finally took a moment to read over this series. I really like the
cleanups in the early part (reducing the number of get_sha1_* functions
especially), and the design of the disambiguate_state code is nice and
clean. I didn't notice anything wrong while reading the patches
themselves.

I do see one slight regression. If you feed an unambiguous partial sha1
and it does not match the hint, we still select it, which is good. So
you still get:

  $ git show 000379^{commit}
  error: 000379^{commit}: expected commit type, but the object dereferences to blob type
  error: 000379^{commit}: expected commit type, but the object dereferences to blob type
  fatal: ambiguous argument '000379^{commit}': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions

So far so good (aside from the duplicated error message, but that is
unrelated). However, if you feed a partial sha1 for which there are
multiple matches, none of which satisfy the disambiguation hint, then we
used to say "short SHA1 is ambiguous", and now we don't.

  [before]
  $ git show 0003
  error: short SHA1 0003 is ambiguous.
  error: short SHA1 0003 is ambiguous.
  fatal: ambiguous argument '0003': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions

  $ git show 0003^{commit}
  error: short SHA1 0003 is ambiguous.
  error: short SHA1 0003 is ambiguous.
  fatal: ambiguous argument '0003^{commit}': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions

  [after]
  $ git show 0003
  error: short SHA1 0003 is ambiguous.
  error: short SHA1 0003 is ambiguous.
  fatal: ambiguous argument '0003': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions

  $ git show 0003^{commit}
  fatal: ambiguous argument '0003^{commit}': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions

I think the code is right not to return an object at all (since it is
ambiguous), but it might be helpful to say "your sha1 was ambiguous, but
none of the possibilities matched our criteria" or similar. Otherwise
the error message looks exactly like the "there is no such object at
all" case. I don't think this is a huge problem, but I can see it being
surprising if somebody is trying to debug an issue where they expect an
object to be of a different type.

Other than that, the series looks good to me.

-Peff

Re: [PATCH v4 00/18] Extending the shelf-life of "git describe" output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   $ git show 0003^{commit}
>   fatal: ambiguous argument '0003^{commit}': unknown revision or path not in the working tree.
>   Use '--' to separate paths from revisions
>
> I think the code is right not to return an object at all (since it is
> ambiguous), but it might be helpful to say "your sha1 was ambiguous, but
> none of the possibilities matched our criteria" or similar. Otherwise
> the error message looks exactly like the "there is no such object at
> all" case.

True.  Thanks for reading.

Re: [PATCH v4 00/18] Extending the shelf-life of "git describe" output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> However, if you feed a partial sha1 for which there are
> multiple matches, none of which satisfy the disambiguation hint, then we
> used to say "short SHA1 is ambiguous", and now we don't.

In finish_object_disambiguation(), if the candidate hasn't been
checked, there are two cases:

 - It is the first and only object that match the prefix; or
 - It replaced another object that matched the prefix but that
   object did not satisfy ds->fn() callback.

And the former case we set ds->candidate_ok to true without doing
anything else, while for the latter we check the candidate, which
may set ds->candidate_ok to false.

At this point in the code, ds->candidate_ok can be false only if
this last-round check found that the candidate does not pass the
check, because the state after update_candidates() returns cannot
satisfy

    !ds->ambiguous && ds->candidate_exists && ds->candidate_checked

and !ds->canidate_ok at the same time.

Hence, when we execute this "return", we know we have seen more than
one object that match the prefix (and none of them satisfied ds->fn),
meaning that we should say "the short name is ambiguous", not "there
is no object that matches the prefix".

Please sanity check; I think it is just this one-liner, but I am
having hard time convincing myself that it can be that simple.

 sha1_name.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sha1_name.c b/sha1_name.c
index 2e2dbb8..c824bdd 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -212,7 +212,7 @@ static int finish_object_disambiguation(struct disambiguate_state *ds,
 				    ds->fn(ds->candidate, ds->cb_data));
 
 	if (!ds->candidate_ok)
-		return SHORT_NAME_NOT_FOUND;
+		return SHORT_NAME_AMBIGUOUS;
 
 	hashcpy(sha1, ds->candidate);
 	return 0;

Re: [PATCH v4 00/18] Extending the shelf-life of "git describe" output

[Jeff King]

On Tue, Jul 03, 2012 at 11:20:10AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > However, if you feed a partial sha1 for which there are
> > multiple matches, none of which satisfy the disambiguation hint, then we
> > used to say "short SHA1 is ambiguous", and now we don't.
> 
> In finish_object_disambiguation(), if the candidate hasn't been
> checked, there are two cases:
> 
>  - It is the first and only object that match the prefix; or
>  - It replaced another object that matched the prefix but that
>    object did not satisfy ds->fn() callback.
> 
> And the former case we set ds->candidate_ok to true without doing
> anything else, while for the latter we check the candidate, which
> may set ds->candidate_ok to false.

Yeah, I think this is right.

> At this point in the code, ds->candidate_ok can be false only if
> this last-round check found that the candidate does not pass the
> check, because the state after update_candidates() returns cannot
> satisfy
> 
>     !ds->ambiguous && ds->candidate_exists && ds->candidate_checked
> 
> and !ds->canidate_ok at the same time.
> 
> Hence, when we execute this "return", we know we have seen more than
> one object that match the prefix (and none of them satisfied ds->fn),
> meaning that we should say "the short name is ambiguous", not "there
> is no object that matches the prefix".

Right. Per the comment just above your change, we are explicitly "ok"
whether or not we meet the criteria in the hint function if we have seen
only one. Which means the function is entirely about _disambiguating_.
It is never about filtering. Which is a good thing, because it leaves
the semantics of get_sha1 otherwise intact.

> Please sanity check; I think it is just this one-liner, but I am
> having hard time convincing myself that it can be that simple.

It looks right to me (and certainly fixes the behavior I mentioned).

-Peff

Re: [PATCH v4 00/18] Extending the shelf-life of "git describe" output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It looks right to me (and certainly fixes the behavior I mentioned).

OK, I have further updates on the topic; pushed out to 'pu'.

Re: [PATCH v4 00/18] Extending the shelf-life of "git describe" output

[Jeff King]

On Tue, Jul 03, 2012 at 11:41:42AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > It looks right to me (and certainly fixes the behavior I mentioned).
> 
> OK, I have further updates on the topic; pushed out to 'pu'.

Looks reasonable to me. One thing I wondered about is in:

> static int get_short_sha1(const char *name, int len, unsigned char *sha1,
>			  unsigned flags)
> {
> [...]
>	memset(&ds, 0, sizeof(ds));
>	if (flags & GET_SHA1_COMMIT)
>		ds.fn = disambiguate_commit_only;
>	else if (flags & GET_SHA1_COMMITTISH)
>		ds.fn = disambiguate_committish_only;
>	else if (flags & GET_SHA1_TREE)
>		ds.fn = disambiguate_tree_only;
>	else if (flags & GET_SHA1_TREEISH)
>		ds.fn = disambiguate_treeish_only;
>	else if (flags & GET_SHA1_BLOB)
>		ds.fn = disambiguate_blob_only;

What happens if I set multiple flags? One or more of them will be
ignored (you _could_ try to establish a hierarchy, for example that
TREEISH is a superset of COMMITISH, but that could not handle the *_only
cases, which really are mutually exclusive). IOW, I think these are not
really flags but rather enum elements.

It is probably an OK trade-off since we are also stuffing true flags
like GET_SHA1_QUIETLY in the same field, and we don't want to make the
parameter list too unwieldy by splitting out the enum. But it might be
worth throwing a comment into cache.h that a certain set of the flags
are mutually exclusive. Or I guess you could mask off that part and make
sure only one bit was set, which would catch the error at run-time. But
I think a comment is probably sufficient.

-Peff

Re: [PATCH v4 00/18] Extending the shelf-life of "git describe" output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> What happens if I set multiple flags? One or more of them will be
> ignored (you _could_ try to establish a hierarchy, for example that
> TREEISH is a superset of COMMITISH, but that could not handle the *_only
> cases, which really are mutually exclusive). IOW, I think these are not
> really flags but rather enum elements.

Yes, I was aware of that.  I counted five useful ones (see the new
ones near the tip of 'pu') but there may be others, so you cannot
fit in 2 bits and would need 3 bits.

> It is probably an OK trade-off since we are also stuffing true flags
> like GET_SHA1_QUIETLY in the same field, and we don't want to make the
> parameter list too unwieldy by splitting out the enum. But it might be
> worth throwing a comment into cache.h that a certain set of the flags
> are mutually exclusive. Or I guess you could mask off that part and make
> sure only one bit was set, which would catch the error at run-time. But
> I think a comment is probably sufficient.

I actually am thinking to move these bit assignments backto
sha1_name.c and make them private, as get_sha1_tree() and friends
are easier to understand public interface functions than
get_sha1_typish(... I_WANT_COMMIT|I_WANT_COMMITTISH|...).

Re: [PATCH v4 00/18] Extending the shelf-life of "git describe" output

[Jeff King]

On Tue, Jul 03, 2012 at 01:21:36PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > What happens if I set multiple flags? One or more of them will be
> > ignored (you _could_ try to establish a hierarchy, for example that
> > TREEISH is a superset of COMMITISH, but that could not handle the *_only
> > cases, which really are mutually exclusive). IOW, I think these are not
> > really flags but rather enum elements.
> 
> Yes, I was aware of that.  I counted five useful ones (see the new
> ones near the tip of 'pu') but there may be others, so you cannot
> fit in 2 bits and would need 3 bits.

I am not worried about the bits, only that adding a new 'enum
disambiguation_type' parameter would be a pain, as most places would
just be passing 0. But it would enforce the mutual exclusion at compile
time.

> I actually am thinking to move these bit assignments backto
> sha1_name.c and make them private, as get_sha1_tree() and friends
> are easier to understand public interface functions than
> get_sha1_typish(... I_WANT_COMMIT|I_WANT_COMMITTISH|...).

Yeah, I think that might be sane. If you really wanted to be flexible
anyway, you would let the caller pass a custom disambiguation function
(which I suspect describe would want for handling the "must come at this
point in the history" rule).

-Peff