[PATCH v3 0/9] Extending the shelf-life of "git describe" output

[PATCH v3 0/9] Extending the shelf-life of "git describe" output

[Junio C Hamano]

This is take 3.  The earlier rounds were $gmane/200165 and 200387.

Only the [9/9] is different.  It adds a test that creates a history
with many objects whose names share the same prefix, and tries to
check how the implementation employs various possible
disambiguations, e.g.

 (1) "$tagname-$generation-g$shortname" can only name a commit
     object, which is the topic of this series;

 (2) "$shortname" in "$tagname-$generation-g$shortname" alone may
     not be sufficiently unambiguous to name a commit, but there may
     be only one such commit if we take $tagname and $generation
     into account, which I hinted as a possible enhancement in the
     cover letter of the first round of this series;

 (3) "$shortname" in "$shortname:$path" can only name a tree-ish,
     which Thomas volunteered to build on top of this series;

 (4) "$shortname" in "$shortname^0" and "$shortname^{commit}" can
     only name a commit-ish, which is a natural extension of (3);

 (5) "$shortname" in "$shortname^0" and "$shortname^{commit}" might
     name more than one commit-ish, but when they are peeled to
     commits, they may name the same one (e.g. one is the commit,
     the other is a tag to the commit), which is a natural extension
     of (4).

Currently, the code implements only (1) and all other tests are
marked as test_expect_failure.

Junio C Hamano (9):
  sha1_name.c: indentation fix
  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                         | 282 +++++++++++++++++++++++-------------
 t/t1512-rev-parse-disambiguation.sh | 172 ++++++++++++++++++++++
 2 files changed, 354 insertions(+), 100 deletions(-)
 create mode 100755 t/t1512-rev-parse-disambiguation.sh

-- 
1.7.11.1.29.gf71be5c

[PATCH v3 1/9] 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.29.gf71be5c

[PATCH v3 2/9] 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 5b0c845..4cbca34 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.29.gf71be5c

[PATCH v3 3/9] 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 4cbca34..5224f39 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.29.gf71be5c

[PATCH v3 4/9] 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 5224f39..e03992c 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.29.gf71be5c

[PATCH v3 5/9] 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 e03992c..7062f72 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.29.gf71be5c

[PATCH v3 6/9] 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 7062f72..852d5c2 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.29.gf71be5c

[PATCH v3 7/9] 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.

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

diff --git a/sha1_name.c b/sha1_name.c
index 852d5c2..7cca7d9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -218,13 +218,16 @@ static int finish_object_disambiguation(struct disambiguate_state *ds,
 	return 0;
 }
 
+#define GET_SHORT_QUIETLY 01
+
 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_SHORT_QUIETLY);
 
 	if (len < MINIMUM_ABBREV || len > 40)
 		return -1;
@@ -272,7 +275,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_SHORT_QUIETLY);
 		if (exists
 		    ? !status
 		    : status == SHORT_NAME_NOT_FOUND) {
@@ -603,7 +606,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_SHORT_QUIETLY);
 			}
 		}
 	}
-- 
1.7.11.1.29.gf71be5c

[PATCH v3 8/9] sha1_name.c: teach get_short_sha1() a commit-only option

[Junio C Hamano]

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

diff --git a/sha1_name.c b/sha1_name.c
index 7cca7d9..58dbbe2 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -219,6 +219,13 @@ static int finish_object_disambiguation(struct disambiguate_state *ds,
 }
 
 #define GET_SHORT_QUIETLY 01
+#define GET_SHORT_COMMIT_ONLY 02
+
+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)
@@ -255,6 +262,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_SHORT_COMMIT_ONLY)
+		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.29.gf71be5c

[PATCH v3 9/9] sha1_name.c: get_describe_name() by definition groks only commits

[Junio C Hamano]

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

diff --git a/sha1_name.c b/sha1_name.c
index 58dbbe2..15e97eb 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -606,6 +606,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_SHORT_QUIETLY | GET_SHORT_COMMIT_ONLY;
 
 	for (cp = name + len - 1; name + 2 <= cp; cp--) {
 		char ch = *cp;
@@ -616,7 +617,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_SHORT_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..ad16f32
--- /dev/null
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -0,0 +1,172 @@
+#!/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_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}
+'
+
+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_done
-- 
1.7.11.1.29.gf71be5c

Re: [PATCH v3 0/9] Extending the shelf-life of "git describe" output

[Junio C Hamano]

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

> This is take 3.  The earlier rounds were $gmane/200165 and 200387.
>
> Only the [9/9] is different.  It adds a test that creates a history
> with many objects whose names share the same prefix, and tries to
> check how the implementation employs various possible
> disambiguations, e.g.

By the way, I noticed that "log --graph" is horribly broken when
used with "-p -m" to show a history with a merge.  For example, try
running the following:

	$ cd t
        $ sh t1512-rev-parse-disambiguation.sh -d
        $ cd tras*tory.t1512-*tion/
        $ git log --oneline -p -m -M

and then run the last one with "--graph":

	$ git log --oneline -p -m -M --graph

Two people who worked heavily on the --graph option in the past
CC'ed.