get_short_oid UI improvements

I started out just wanting to do 04/09 so I'd get prettier output, but then noticed that ^{tag}, ^{commit}< ^{blob} and ^{tree} didn't behave as expected with the disambiguation output, and that core.disambiguate had never been documented.

Ævar Arnfjörð Bjarmason (9):
sha1-name.c: remove stray newline
sha1-array.h: align function arguments
sha1-name.c: move around the collect_ambiguous() function
get_short_oid: sort ambiguous objects by type, then SHA-1
get_short_oid: learn to disambiguate by ^{tag}
get_short_oid: learn to disambiguate by ^{blob} get_short_oid / peel_onion: ^{tree} should mean tree, not treeish get_short_oid / peel_onion: ^{tree} should mean commit, not commitish config doc: document core.disambiguate

 Documentation/config.txt            | 14 ++++++
 cache.h                             |  5 ++-
 sha1-array.c                        | 15 +++++++
 sha1-array.h                        |  7 ++-
 sha1-name.c                         | 69 ++++++++++++++++++++++++-----
 t/t1512-rev-parse-disambiguation.sh | 32 ++++++++++---
 6 files changed, 120 insertions(+), 22 deletions(-)

2.17.0.290.gded63e768a

This stray newline was accidentally introduced in d2b7d9c7ed ("sha1_name: convert disambiguate_hint_fn to take object_id", 2017-03-26).

---
 sha1-name.c | 1 -
 1 file changed, 1 deletion(-)

index 5b93bf8da3..cd3b133aae 100644
@@ -346,7 +346,6 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
 	struct strbuf desc = STRBUF_INIT;
 	int type;
 
-
 	if (ds->fn && !ds->fn(oid, ds->cb_data))
 		return 0;
 

2.17.0.290.gded63e768a

The arguments weren't lined up with the opening parenthesis. Fixes up code added in cff38a5e11 ("receive-pack: eliminate duplicate .have refs", 2011-05-19).

---
 sha1-array.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 04b0756334..1e1d24b009 100644
@@ -17,7 +17,7 @@ void oid_array_clear(struct oid_array *array);
 typedef int (*for_each_oid_fn)(const struct object_id *oid,
 			       void *data);
 int oid_array_for_each_unique(struct oid_array *array,
-			       for_each_oid_fn fn,
-			       void *data);
+			      for_each_oid_fn fn,
+			      void *data);
 
 #endif /* SHA1_ARRAY_H */

2.17.0.290.gded63e768a

A subsequent change will make use of this static function in the get_short_oid() function, which is defined above where the collect_ambiguous() function is now, which would result in a compilation error due to a forward declaration.

---
 sha1-name.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

index cd3b133aae..9d7bbd3e96 100644
@@ -372,6 +372,12 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
 	return 0;
 }
 
+static int collect_ambiguous(const struct object_id *oid, void *data)
+{
+	oid_array_append(data, oid);
+	return 0;
+}
+
 static int get_short_oid(const char *name, int len, struct object_id *oid,
 			  unsigned flags)
 {

@@ -421,12 +427,6 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
 	return status;
 }
 
-static int collect_ambiguous(const struct object_id *oid, void *data)
-{
-	oid_array_append(data, oid);
-	return 0;
-}
-
 int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
 {
 	struct oid_array collect = OID_ARRAY_INIT;

2.17.0.290.gded63e768a

Change the output emitted when an ambiguous object is encountered so that we show tags first, then commits, followed by trees, and finally blobs. Within each type we show objects in hashcmp(). Before this change the objects were only ordered by hashcmp().

The reason for doing this is that the output looks better as a result, e.g. the v2.17.0 tag before this change on "git show e8f2" would display:

    hint: The candidates are:
    hint:   e8f2093055 tree
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f25a3a50 tree
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2650052 tag v2.17.0
    hint:   e8f2867228 blob
    hint:   e8f28d537c tree
    hint:   e8f2a35526 blob
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2cf6ec0 tree

Now we'll instead show:

    hint:   e8f2650052 tag v2.17.0
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f2867228 blob
    hint:   e8f2a35526 blob

Since we show the commit data in the output that's nicely aligned once we sort by object type. The decision to show tags before commits is pretty arbitrary, but it's much less likely that we'll display a tag, so if there is one it makes sense to show it first.

---
 sha1-array.c | 15 +++++++++++++++
 sha1-array.h |  3 +++
 sha1-name.c  | 37 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 54 insertions(+), 1 deletion(-)

index 838b3bf847..48bd9e9230 100644
@@ -41,6 +41,21 @@ void oid_array_clear(struct oid_array *array)
 	array->sorted = 0;
 }
 
+
+int oid_array_for_each(struct oid_array *array,
+		       for_each_oid_fn fn,
+		       void *data)
+{
+	int i;
+
+	for (i = 0; i < array->nr; i++) {
+		int ret = fn(array->oid + i, data);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
 int oid_array_for_each_unique(struct oid_array *array,
 				for_each_oid_fn fn,
 				void *data)

index 1e1d24b009..232bf95017 100644
@@ -16,6 +16,9 @@ void oid_array_clear(struct oid_array *array);
 
 typedef int (*for_each_oid_fn)(const struct object_id *oid,
 			       void *data);
+int oid_array_for_each(struct oid_array *array,
+		       for_each_oid_fn fn,
+		       void *data);
 int oid_array_for_each_unique(struct oid_array *array,
 			      for_each_oid_fn fn,
 			      void *data);

index 9d7bbd3e96..46d8b1afa6 100644
@@ -378,6 +378,34 @@ static int collect_ambiguous(const struct object_id *oid, void *data)
 	return 0;
 }
 
+static int sort_ambiguous(const void *a, const void *b)
+{
+	int a_type = oid_object_info(a, NULL);
+	int b_type = oid_object_info(b, NULL);
+	int a_type_sort;
+	int b_type_sort;
+
+	/*
+	 * Sorts by hash within the same object type, just as
+	 * oid_array_for_each_unique() would do.
+	 */
+	if (a_type == b_type)
+		return oidcmp(a, b);
+
+	/*
+	 * Between object types show tags, then commits, and finally
+	 * trees and blobs.
+	 *
+	 * The object_type enum is commit, tree, blob, tag, but we
+	 * want tag, commit, tree blob. Cleverly (perhaps too
+	 * cleverly) do that with modulus, since the enum assigns 1 to
+	 * commit, so tag becomes 0.
+	 */
+	a_type_sort = a_type % 4;
+	b_type_sort = b_type % 4;
+	return a_type_sort > b_type_sort ? 1 : -1;
+}
+
 static int get_short_oid(const char *name, int len, struct object_id *oid,
 			  unsigned flags)
 {

@@ -409,6 +437,8 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
 	status = finish_object_disambiguation(&ds, oid);
 
 	if (!quietly && (status == SHORT_NAME_AMBIGUOUS)) {
+		struct oid_array collect = OID_ARRAY_INIT;
+
 		error(_("short SHA1 %s is ambiguous"), ds.hex_pfx);
 
 		/*

@@ -421,7 +451,12 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
 			ds.fn = NULL;
 
 		advise(_("The candidates are:"));
-		for_each_abbrev(ds.hex_pfx, show_ambiguous_object, &ds);
+		for_each_abbrev(ds.hex_pfx, collect_ambiguous, &collect);
+		QSORT(collect.oid, collect.nr, sort_ambiguous);
+
+		if (oid_array_for_each(&collect, show_ambiguous_object, &ds))
+			BUG("show_ambiguous_object shouldn't return non-zero");
+		oid_array_clear(&collect);
 	}
 
 	return status;

2.17.0.290.gded63e768a

Here's a non-arbitrary reason: the object types are ordered topologically (ignoring self-references):

tag -> commit, tree, blob
commit -> tree
tree -> blob

Thanks. I'll add a patch with that comment to v2.

When preparing v2 I realized how confusing this was, so I'd added this to the commit message of my WIP re-roll which should explain this:

A note on the implementation: I started out with something much simpler which just replaced oid_array_sort() in sha1-array.c with a custom sort function before calling oid_array_for_each_unique(). But then dumbly noticed that it doesn't work because the output function was tangled up with the code added in fad6b9e590 ("for_each_abbrev:
drop duplicate objects", 2016-09-26) to ensure we don't display duplicate objects.

That's why we're doing two passes here, first we need to sort the list and de-duplicate the objects, then sort them in our custom order, and finally output them without re-sorting them. I suppose we could also make oid_array_for_each_unique() maintain a hashmap of emitted objects, but that would increase its memory profile and wouldn't be worth the complexity for this one-off use-case, oid_array_for_each_unique() is used in many other places.

How would sorting in our custom order before de-duplicating fail the de-duplication? We will still pair identical OIDs as consecutive elements and oid_array_for_each_unique only cares about consecutive elements having distinct OIDs, not lex-ordered OIDs.

Perhaps the noise is because we rely on oid_array_sort() to mark the array as sorted inside oid_array_for_each_unique(), but that could be remedied by calling our QSORT() inside for_each_abbrev() and marking the array as sorted before calling oid_array_for_each_unique().

(Again, my comments are not meant to block this series.)

Thanks,
-Stolee

Because there's no de-duplication without the array first being sorted in oidcmp() order, which oid_array_for_each_unique() checks for and re-sorts if !array->sorted. I.e. its de-duplication is just a state machine where it won't call the callback if the currently processed element has the same SHA1 as the last one.

As noted above this won't work, because the function inherently relies on the array being sorted to be able to de-duplicate. Doing this will yield duplicate entries.

I'm confused as to why my suggestion doesn't work, so I made it concrete. I sent an alternate commit 6/12 to your v2 series [1].

Thanks,
-Stolee

[1] https://public-inbox.org/git/[email protected]/T/#u

Add support for ^{tag} to the disambiguation logic. Before this ^{tag} would simply be ignored:

    $ git rev-parse e8f2^{tag}
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f2650052 tag v2.17.0
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f2867228 blob
    hint:   e8f2a35526 blob
    e8f2^{tag}

Now the logic added in ed1ca6025f ("peel_onion: disambiguate to favor tree-ish when we know we want a tree-ish", 2013-03-31) has been extended to support it.

    $ git rev-parse e8f2^{tag}
    e8f2650052f3ff646023725e388ea1112b020e79

---
 cache.h                             |  5 +++--
 sha1-name.c                         | 13 ++++++++++++-
 t/t1512-rev-parse-disambiguation.sh |  5 ++++-
 3 files changed, 19 insertions(+), 4 deletions(-)

index 77b7acebb6..3f6a292ba6 100644
@@ -1322,8 +1322,9 @@ struct object_context {
 #define GET_OID_TREE             010
 #define GET_OID_TREEISH          020
 #define GET_OID_BLOB             040
-#define GET_OID_FOLLOW_SYMLINKS 0100
-#define GET_OID_RECORD_PATH     0200
+#define GET_OID_TAG             0100
+#define GET_OID_FOLLOW_SYMLINKS 0200
+#define GET_OID_RECORD_PATH     0400
 #define GET_OID_ONLY_TO_DIE    04000
 
 #define GET_OID_DISAMBIGUATORS \

index 46d8b1afa6..68d5f65362 100644
@@ -221,6 +221,12 @@ static int finish_object_disambiguation(struct disambiguate_state *ds,
 	return 0;
 }
 
+static int disambiguate_tag_only(const struct object_id *oid, void *cb_data_unused)
+{
+	int kind = oid_object_info(oid, NULL);
+	return kind == OBJ_TAG;
+}
+
 static int disambiguate_commit_only(const struct object_id *oid, void *cb_data_unused)
 {
 	int kind = oid_object_info(oid, NULL);

@@ -288,7 +294,8 @@ int set_disambiguate_hint_config(const char *var, const char *value)
 		{ "committish", disambiguate_committish_only },
 		{ "tree", disambiguate_tree_only },
 		{ "treeish", disambiguate_treeish_only },
-		{ "blob", disambiguate_blob_only }
+		{ "blob", disambiguate_blob_only },
+		{ "tag", disambiguate_tag_only }
 	};
 	int i;
 

@@ -429,6 +436,8 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
 		ds.fn = disambiguate_treeish_only;
 	else if (flags & GET_OID_BLOB)
 		ds.fn = disambiguate_blob_only;
+	else if (flags & GET_OID_TAG)
+		ds.fn = disambiguate_tag_only;
 	else
 		ds.fn = default_disambiguate_hint;
 

@@ -958,6 +967,8 @@ static int peel_onion(const char *name, int len, struct object_id *oid,
 	lookup_flags &= ~GET_OID_DISAMBIGUATORS;
 	if (expected_type == OBJ_COMMIT)
 		lookup_flags |= GET_OID_COMMITTISH;
+	else if (expected_type == OBJ_TAG)
+		lookup_flags |= GET_OID_TAG;
 	else if (expected_type == OBJ_TREE)
 		lookup_flags |= GET_OID_TREEISH;
 

index 711704ba5a..c7ceda2f21 100755
@@ -334,7 +334,10 @@ test_expect_success C_LOCALE_OUTPUT 'ambiguity hints respect type' '
 	test_must_fail git rev-parse 000000000^{commit} 2>stderr &&
 	grep ^hint: stderr >hints &&
 	# 5 commits, 1 tag (which is a commitish), plus intro line
-	test_line_count = 7 hints
+	test_line_count = 7 hints &&
+	git rev-parse 000000000^{tag} >stdout &&
+	test_line_count = 1 stdout &&
+	grep -q ^0000000000f8f stdout
 '
 
 test_expect_success C_LOCALE_OUTPUT 'failed type-selector still shows hint' '

2.17.0.290.gded63e768a

The disambiguation logic had all the pieces necessary to only print out those blobs that were ambiguous, but they hadn't been connected.

The initial logic was added in daba53aeaf ("sha1_name.c: add support for disambiguating other types", 2012-07-02), and when the flags were propagated in 8a10fea49b ("get_sha1: propagate flags to child functions", 2016-09-26) GET_OID_BLOB wasn't added to lookup_flags.

Before this change requests for blobs were simply ignored:

    $ git rev-parse e8f2^{blob}
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f2650052 tag v2.17.0
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f2867228 blob
    hint:   e8f2a35526 blob
    e8f2^{blob}
    [...]

But now we'll do the right thing and only print the blobs:

    $ git rev-parse e8f2^{blob}
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f2867228 blob
    hint:   e8f2a35526 blob
    e8f2^{blob}
    [...]

---
 sha1-name.c                         | 2 ++
 t/t1512-rev-parse-disambiguation.sh | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

index 68d5f65362..023f9471a8 100644
@@ -971,6 +971,8 @@ static int peel_onion(const char *name, int len, struct object_id *oid,
 		lookup_flags |= GET_OID_TAG;
 	else if (expected_type == OBJ_TREE)
 		lookup_flags |= GET_OID_TREEISH;
+	else if (expected_type == OBJ_BLOB)
+		lookup_flags |= GET_OID_BLOB;
 
 	if (get_oid_1(name, sp - name - 2, &outer, lookup_flags))
 		return -1;

index c7ceda2f21..08ae73e2a5 100755
@@ -337,7 +337,11 @@ test_expect_success C_LOCALE_OUTPUT 'ambiguity hints respect type' '
 	test_line_count = 7 hints &&
 	git rev-parse 000000000^{tag} >stdout &&
 	test_line_count = 1 stdout &&
-	grep -q ^0000000000f8f stdout
+	grep -q ^0000000000f8f stdout &&
+	test_must_fail git rev-parse 000000000^{blob} 2>stderr &&
+	grep ^hint: stderr >hints &&
+	# 5 blobs plus intro line &&
+	test_line_count = 6 hints
 '
 
 test_expect_success C_LOCALE_OUTPUT 'failed type-selector still shows hint' '

2.17.0.290.gded63e768a

After the recent series of patches ^{tag} and ^{blob} now work to get just the tags and blobs, but ^{tree} will still list any tree-ish (commits, tags and trees).

The previous behavior was added in ed1ca6025f ("peel_onion:
disambiguate to favor tree-ish when we know we want a tree-ish", 2013-03-31). I may have missed some special-case but this makes more sense to me.

Now "$sha1:" can be used as before to mean treeish

    $ git rev-parse e8f2:
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f2650052 tag v2.17.0
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    [...]

But ^{tree} shows just the trees, but would previously be equivalent to the above:

    $ git rev-parse e8f2^{tree}
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    [...]

---
 sha1-name.c                         |  2 +-
 t/t1512-rev-parse-disambiguation.sh | 18 ++++++++++++++----
 2 files changed, 15 insertions(+), 5 deletions(-)

index 023f9471a8..b61c0558d9 100644
@@ -970,7 +970,7 @@ static int peel_onion(const char *name, int len, struct object_id *oid,
 	else if (expected_type == OBJ_TAG)
 		lookup_flags |= GET_OID_TAG;
 	else if (expected_type == OBJ_TREE)
-		lookup_flags |= GET_OID_TREEISH;
+		lookup_flags |= GET_OID_TREE;
 	else if (expected_type == OBJ_BLOB)
 		lookup_flags |= GET_OID_BLOB;
 

index 08ae73e2a5..2acf564714 100755
@@ -159,9 +159,13 @@ test_expect_failure 'two semi-ambiguous commit-ish' '
 	git log 0000000000...
 '
 
-test_expect_failure 'three semi-ambiguous tree-ish' '
+test_expect_success '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
+	test_must_fail git rev-parse --verify 0000000000: &&
+
+	# For ^{tree} we can disambiguate because HEAD and v1.0.0 will
+	# be excluded.
 	git rev-parse --verify 0000000000^{tree}
 '
 

@@ -267,8 +271,12 @@ test_expect_success 'ambiguous commit-ish' '
 # There are three objects with this prefix: a blob, a tree, and a tag. We know
 # the blob will not pass as a treeish, but the tree and tag should (and thus
 # cause an error).
-test_expect_success 'ambiguous tags peel to treeish' '
-	test_must_fail git rev-parse 0000000000f^{tree}
+test_expect_success 'ambiguous tags peel to treeish or tree' '
+	test_must_fail git rev-parse 0000000000f: &&
+	git rev-parse 0000000000f^{tree} >stdout &&
+	test_line_count = 1 stdout &&
+	grep -q ^0000000000fd8bcc56 stdout
+
 '
 
 test_expect_success 'rev-parse --disambiguate' '

@@ -365,7 +373,9 @@ test_expect_success 'core.disambiguate config can prefer types' '
 test_expect_success 'core.disambiguate does not override context' '
 	# treeish ambiguous between tag and tree
 	test_must_fail \
-		git -c core.disambiguate=committish rev-parse $sha1^{tree}
+		git -c core.disambiguate=committish rev-parse $sha1: &&
+	# tree not ambiguous between tag and tree
+	git -c core.disambiguate=committish rev-parse $sha1^{tree}
 '
 
 test_done

2.17.0.290.gded63e768a

Continue the untangling of peel disambiguation syntax. Before this e8f2^{commit} would show the v2.17.0 tag, but now it'll just show ambiguous commits:

    $ git rev-parse e8f2^{commit}
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    [...]

---
 sha1-name.c                         | 2 +-
 t/t1512-rev-parse-disambiguation.sh | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

index b61c0558d9..1d2a74a29c 100644
@@ -966,7 +966,7 @@ static int peel_onion(const char *name, int len, struct object_id *oid,
 
 	lookup_flags &= ~GET_OID_DISAMBIGUATORS;
 	if (expected_type == OBJ_COMMIT)
-		lookup_flags |= GET_OID_COMMITTISH;
+		lookup_flags |= GET_OID_COMMIT;
 	else if (expected_type == OBJ_TAG)
 		lookup_flags |= GET_OID_TAG;
 	else if (expected_type == OBJ_TREE)

index 2acf564714..b3ef236db8 100755
@@ -341,8 +341,8 @@ test_expect_success C_LOCALE_OUTPUT 'ambiguity hints' '
 test_expect_success C_LOCALE_OUTPUT 'ambiguity hints respect type' '
 	test_must_fail git rev-parse 000000000^{commit} 2>stderr &&
 	grep ^hint: stderr >hints &&
-	# 5 commits, 1 tag (which is a commitish), plus intro line
-	test_line_count = 7 hints &&
+	# 5 commits plus intro line
+	test_line_count = 6 hints &&
 	git rev-parse 000000000^{tag} >stdout &&
 	test_line_count = 1 stdout &&
 	grep -q ^0000000000f8f stdout &&

@@ -366,7 +366,8 @@ test_expect_success 'core.disambiguate config can prefer types' '
 	# ambiguous between tree and tag
 	sha1=0000000000f &&
 	test_must_fail git rev-parse $sha1 &&
-	git rev-parse $sha1^{commit} &&
+	# there is no commit so ^{commit} comes up empty
+	test_must_fail git rev-parse $sha1^{commit} &&
 	git -c core.disambiguate=committish rev-parse $sha1
 '
 

2.17.0.290.gded63e768a

s/tree/commit/

The core.disambiguate variable was added in 5b33cb1fd7 ("get_short_sha1: make default disambiguation configurable", 2016-09-27) but never documented.

---
 Documentation/config.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

index 2659153cb3..6fee67c12d 100644
@@ -910,6 +910,20 @@ core.abbrev::
 	abbreviated object names to stay unique for some time.
 	The minimum length is 4.
 
+core.disambiguate::
+	If Git is given a SHA-1 that's ambigous it'll suggest what
+	objects you might mean. By default it'll print out all
+	potential objects with that prefix regardless of their
+	type. This setting, along with the `^{<type>}` peel syntax
+	(see linkgit:gitrevisions[7]), allows for narrowing that down.
+
++
+Is set to `none` by default, can also be `commit` (peel syntax:
+`$sha1^{commit}`), `commitish` (commits and tags), `tree` (peel:
+`$sha1^{tree}`), `treeish` (everything except blobs), `blob` (peel:
+`$sha1^{blob}`) or `tag` (peel: `$sha1^{tag}`). The peel syntax will
+override any config value.
+
 add.ignoreErrors::
 add.ignore-errors (deprecated)::
 	Tells 'git add' to continue adding files when some files cannot be

2.17.0.290.gded63e768a

This whole series, including the comment in which you wonder if the code is overly smart, is

As mentioned, I'm a bit unsure that patches 7 and 8 are entirely correct. I've mostly convinced myself that they are after looking at peel_onion, but I'm still harboring lingering doubts for some reason.

The rest of the series looked fine to me. Thanks for cleaning up my stray newline.

brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

This is a good series. Please take a look at my suggestion in Patch 4/9, but feel free to keep this series as written.

A v2 addressing feedback so far. Comments inline per-patch.

Ævar Arnfjörð Bjarmason (12):
sha1-name.c: remove stray newline
sha1-array.h: align function arguments

No changes.

git-p4: change "commitish" typo to "committish"

New, I fixed my own "commitish" elsewhere, fixing it here in this unrelated area while I'm at it.

cache.h: add comment explaining the order in object_type

New: Derrick Stolee pointed out why the object type enum used later is ordered that way, explain that with a comment.

sha1-name.c: move around the collect_ambiguous() function

Trivial grammar correction in commit message:
- collect_ambiguous() function is now, which would result in a
+ collect_ambiguous() function is now. Without this we'd then have a

get_short_oid: sort ambiguous objects by type, then SHA-1

* Grammar fixes in commit message
* Add docs to api-oid-array.txt documenting the new oid_array_for_each()
* Document in the commit message why we sort twice
* Note inline in sha1-array.c why oid_array_for_each() doesn't sort with a pointer to the API docs.
* Add test to assert that we sort objects in the order we expect, and that they're hash sorted within the object types.

get_short_oid: learn to disambiguate by ^{tag}
get_short_oid: learn to disambiguate by ^{blob}

No changes.

get_short_oid / peel_onion: ^{tree} should be tree, not treeish

s/mean/be/ in subject line (to avoid wrapping in E-Mail).

get_short_oid / peel_onion: ^{commit} should be commit, not committish

Rewrite commit message, now assumes less context from the rest of the series & is easier to read stand-alone.

config doc: document core.disambiguate

Change commitish to committish, and note the $sha1: peel syntax.

get_short_oid: document & warn if we ignore the type selector

New: Explain why we ignore e.g. $sha1^{blob} if there's no blobs with the $sha1 prefix.

 Documentation/config.txt                  | 17 +++++
 Documentation/technical/api-oid-array.txt | 17 +++--
 cache.h                                   | 13 +++-
 git-p4.py                                 |  6 +-
 sha1-array.c                              | 17 +++++
 sha1-array.h                              |  7 +-
 sha1-name.c                               | 80 +++++++++++++++++++----
 t/t1512-rev-parse-disambiguation.sh       | 58 +++++++++++++---
 8 files changed, 182 insertions(+), 33 deletions(-)

2.17.0.290.gded63e768a

This stray newline was accidentally introduced in d2b7d9c7ed ("sha1_name: convert disambiguate_hint_fn to take object_id", 2017-03-26).

---
 sha1-name.c | 1 -
 1 file changed, 1 deletion(-)

index 5b93bf8da3..cd3b133aae 100644
@@ -346,7 +346,6 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
 	struct strbuf desc = STRBUF_INIT;
 	int type;
 
-
 	if (ds->fn && !ds->fn(oid, ds->cb_data))
 		return 0;
 

2.17.0.290.gded63e768a

The arguments weren't lined up with the opening parenthesis. Fixes up code added in cff38a5e11 ("receive-pack: eliminate duplicate .have refs", 2011-05-19).

---
 sha1-array.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 04b0756334..1e1d24b009 100644
@@ -17,7 +17,7 @@ void oid_array_clear(struct oid_array *array);
 typedef int (*for_each_oid_fn)(const struct object_id *oid,
 			       void *data);
 int oid_array_for_each_unique(struct oid_array *array,
-			       for_each_oid_fn fn,
-			       void *data);
+			      for_each_oid_fn fn,
+			      void *data);
 
 #endif /* SHA1_ARRAY_H */

2.17.0.290.gded63e768a

This was the only occurrence of "commitish" in the tree, but as the log will reveal we've had others in the past. Fixes up code added in 00ad6e3182 ("git-p4: work with a detached head", 2015-11-21).

---
 git-p4.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 7bb9cadc69..1afa87cd9d 100755
@@ -2099,11 +2099,11 @@ class P4Submit(Command, P4UserMap):
 
         commits = []
         if self.master:
-            commitish = self.master
+            committish = self.master
         else:
-            commitish = 'HEAD'
+            committish = 'HEAD'
 
-        for line in read_pipe_lines(["git", "rev-list", "--no-merges", "%s..%s" % (self.origin, commitish)]):
+        for line in read_pipe_lines(["git", "rev-list", "--no-merges", "%s..%s" % (self.origin, committish)]):
             commits.append(line.strip())
         commits.reverse()
 

2.17.0.290.gded63e768a

The order in the enum might seem arbitrary, and isn't explained by 72518e9c26 ("more lightweight revalidation while reusing deflated stream in packing", 2006-09-03) which added it, but Derrick Stolee suggested that it's ordered topologically in [email protected]. Makes sense to me, add that as a comment.

---
 cache.h | 8 ++++++++
 1 file changed, 8 insertions(+)

index 77b7acebb6..354903c3ea 100644
@@ -376,6 +376,14 @@ extern void free_name_hash(struct index_state *istate);
 enum object_type {
 	OBJ_BAD = -1,
 	OBJ_NONE = 0,
+	/*
+	 * Why have our our "real" object types in this order? They're
+	 * ordered topologically:
+	 *
+	 * tag(4)    -> commit(1), tree(2), blob(3)
+	 * commit(1) -> tree(2)
+	 * tree(2)   -> blob(3)
+	 */
 	OBJ_COMMIT = 1,
 	OBJ_TREE = 2,
 	OBJ_BLOB = 3,

2.17.0.290.gded63e768a

A subsequent change will make use of this static function in the get_short_oid() function, which is defined above where the collect_ambiguous() function is now. Without this we'd then have a compilation error due to a forward declaration.

---
 sha1-name.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

index cd3b133aae..9d7bbd3e96 100644
@@ -372,6 +372,12 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
 	return 0;
 }
 
+static int collect_ambiguous(const struct object_id *oid, void *data)
+{
+	oid_array_append(data, oid);
+	return 0;
+}
+
 static int get_short_oid(const char *name, int len, struct object_id *oid,
 			  unsigned flags)
 {

@@ -421,12 +427,6 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
 	return status;
 }
 
-static int collect_ambiguous(const struct object_id *oid, void *data)
-{
-	oid_array_append(data, oid);
-	return 0;
-}
-
 int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
 {
 	struct oid_array collect = OID_ARRAY_INIT;

2.17.0.290.gded63e768a

Change the output emitted when an ambiguous object is encountered so that we show tags first, then commits, followed by trees, and finally blobs. Within each type we show objects in hashcmp() order. Before this change the objects were only ordered by hashcmp().

The reason for doing this is that the output looks better as a result, e.g. the v2.17.0 tag before this change on "git show e8f2" would display:

    hint: The candidates are:
    hint:   e8f2093055 tree
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f25a3a50 tree
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2650052 tag v2.17.0
    hint:   e8f2867228 blob
    hint:   e8f28d537c tree
    hint:   e8f2a35526 blob
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2cf6ec0 tree

Now we'll instead show:

    hint:   e8f2650052 tag v2.17.0
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f2867228 blob
    hint:   e8f2a35526 blob

Since we show the commit data in the output that's nicely aligned once we sort by object type. The decision to show tags before commits is pretty arbitrary. I don't want to order by object_type since there tags come last after blobs, which doesn't make sense if we want to show the most important things first.

I could display them after commits, but it's much less likely that we'll display a tag, so if there is one it makes sense to show it prominently at the top.

A note on the implementation: I started out with something much simpler which just replaced oid_array_sort() in sha1-array.c with a custom sort function before calling oid_array_for_each_unique(). But then dumbly noticed that it doesn't work because the output function was tangled up with the code added in fad6b9e590 ("for_each_abbrev:
drop duplicate objects", 2016-09-26) to ensure we don't display duplicate objects.

That's why we're doing two passes here, first we need to sort the list and de-duplicate the objects, then sort them in our custom order, and finally output them without re-sorting them. I suppose we could also make oid_array_for_each_unique() maintain a hashmap of emitted objects, but that would increase its memory profile and wouldn't be worth the complexity for this one-off use-case, oid_array_for_each_unique() is used in many other places.

---
 Documentation/technical/api-oid-array.txt | 17 +++++++----
 sha1-array.c                              | 17 +++++++++++
 sha1-array.h                              |  3 ++
 sha1-name.c                               | 37 ++++++++++++++++++++++-
 t/t1512-rev-parse-disambiguation.sh       | 21 +++++++++++++
 5 files changed, 88 insertions(+), 7 deletions(-)

index b0c11f868d..94b529722c 100644
@@ -35,13 +35,18 @@ Functions
 	Free all memory associated with the array and return it to the
 	initial, empty state.
 
+`oid_array_for_each`::
+	Iterate over each element of the list, executing the callback
+	function for each one. Does not sort the list, so any custom
+	hash order is retained. If the callback returns a non-zero
+	value, the iteration ends immediately and the callback's
+	return is propagated; otherwise, 0 is returned.
+
 `oid_array_for_each_unique`::
-	Efficiently iterate over each unique element of the list,
-	executing the callback function for each one. If the array is
-	not sorted, this function has the side effect of sorting it. If
-	the callback returns a non-zero value, the iteration ends
-	immediately and the callback's return is propagated; otherwise,
-	0 is returned.
+	Iterate over each unique element of the list in sort order ,
+	but otherwise behaves like `oid_array_for_each`. If the array
+	is not sorted, this function has the side effect of sorting
+	it.
 
 Examples
 --------

index 838b3bf847..5b2793615b 100644
@@ -41,6 +41,23 @@ void oid_array_clear(struct oid_array *array)
 	array->sorted = 0;
 }
 
+
+int oid_array_for_each(struct oid_array *array,
+		       for_each_oid_fn fn,
+		       void *data)
+{
+	int i;
+
+	/* No oid_array_sort() here! See the api-oid-array.txt docs! */
+
+	for (i = 0; i < array->nr; i++) {
+		int ret = fn(array->oid + i, data);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
 int oid_array_for_each_unique(struct oid_array *array,
 				for_each_oid_fn fn,
 				void *data)

index 1e1d24b009..232bf95017 100644
@@ -16,6 +16,9 @@ void oid_array_clear(struct oid_array *array);
 
 typedef int (*for_each_oid_fn)(const struct object_id *oid,
 			       void *data);
+int oid_array_for_each(struct oid_array *array,
+		       for_each_oid_fn fn,
+		       void *data);
 int oid_array_for_each_unique(struct oid_array *array,
 			      for_each_oid_fn fn,
 			      void *data);

index 9d7bbd3e96..46d8b1afa6 100644
@@ -378,6 +378,34 @@ static int collect_ambiguous(const struct object_id *oid, void *data)
 	return 0;
 }
 
+static int sort_ambiguous(const void *a, const void *b)
+{
+	int a_type = oid_object_info(a, NULL);
+	int b_type = oid_object_info(b, NULL);
+	int a_type_sort;
+	int b_type_sort;
+
+	/*
+	 * Sorts by hash within the same object type, just as
+	 * oid_array_for_each_unique() would do.
+	 */
+	if (a_type == b_type)
+		return oidcmp(a, b);
+
+	/*
+	 * Between object types show tags, then commits, and finally
+	 * trees and blobs.
+	 *
+	 * The object_type enum is commit, tree, blob, tag, but we
+	 * want tag, commit, tree blob. Cleverly (perhaps too
+	 * cleverly) do that with modulus, since the enum assigns 1 to
+	 * commit, so tag becomes 0.
+	 */
+	a_type_sort = a_type % 4;
+	b_type_sort = b_type % 4;
+	return a_type_sort > b_type_sort ? 1 : -1;
+}
+
 static int get_short_oid(const char *name, int len, struct object_id *oid,
 			  unsigned flags)
 {

@@ -409,6 +437,8 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
 	status = finish_object_disambiguation(&ds, oid);
 
 	if (!quietly && (status == SHORT_NAME_AMBIGUOUS)) {
+		struct oid_array collect = OID_ARRAY_INIT;
+
 		error(_("short SHA1 %s is ambiguous"), ds.hex_pfx);
 
 		/*

@@ -421,7 +451,12 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
 			ds.fn = NULL;
 
 		advise(_("The candidates are:"));
-		for_each_abbrev(ds.hex_pfx, show_ambiguous_object, &ds);
+		for_each_abbrev(ds.hex_pfx, collect_ambiguous, &collect);
+		QSORT(collect.oid, collect.nr, sort_ambiguous);
+
+		if (oid_array_for_each(&collect, show_ambiguous_object, &ds))
+			BUG("show_ambiguous_object shouldn't return non-zero");
+		oid_array_clear(&collect);
 	}
 
 	return status;

index 711704ba5a..2701462041 100755
@@ -361,4 +361,25 @@ test_expect_success 'core.disambiguate does not override context' '
 		git -c core.disambiguate=committish rev-parse $sha1^{tree}
 '
 
+test_expect_success C_LOCALE_OUTPUT 'ambiguous commits are printed by type first, then hash order' '
+	test_must_fail git rev-parse 0000 2>stderr &&
+	grep ^hint: stderr >hints &&
+	grep 0000 hints >objects &&
+	cat >expected <<-\EOF &&
+	tag
+	commit
+	tree
+	blob
+	EOF
+	awk "{print \$3}" <objects >objects.types &&
+	uniq <objects.types >objects.types.uniq &&
+	test_cmp expected objects.types.uniq &&
+	for type in tag commit tree blob
+	do
+		grep $type objects >$type.objects &&
+		sort $type.objects >$type.objects.sorted &&
+		test_cmp $type.objects.sorted $type.objects
+	done
+'
+
 test_done

2.17.0.290.gded63e768a

Add support for ^{tag} to the disambiguation logic. Before this ^{tag} would simply be ignored:

    $ git rev-parse e8f2^{tag}
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f2650052 tag v2.17.0
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f2867228 blob
    hint:   e8f2a35526 blob
    e8f2^{tag}

Now the logic added in ed1ca6025f ("peel_onion: disambiguate to favor tree-ish when we know we want a tree-ish", 2013-03-31) has been extended to support it.

    $ git rev-parse e8f2^{tag}
    e8f2650052f3ff646023725e388ea1112b020e79

---
 cache.h                             |  5 +++--
 sha1-name.c                         | 13 ++++++++++++-
 t/t1512-rev-parse-disambiguation.sh |  5 ++++-
 3 files changed, 19 insertions(+), 4 deletions(-)

index 354903c3ea..a141995cc7 100644
@@ -1330,8 +1330,9 @@ struct object_context {
 #define GET_OID_TREE             010
 #define GET_OID_TREEISH          020
 #define GET_OID_BLOB             040
-#define GET_OID_FOLLOW_SYMLINKS 0100
-#define GET_OID_RECORD_PATH     0200
+#define GET_OID_TAG             0100
+#define GET_OID_FOLLOW_SYMLINKS 0200
+#define GET_OID_RECORD_PATH     0400
 #define GET_OID_ONLY_TO_DIE    04000
 
 #define GET_OID_DISAMBIGUATORS \

index 46d8b1afa6..68d5f65362 100644
@@ -221,6 +221,12 @@ static int finish_object_disambiguation(struct disambiguate_state *ds,
 	return 0;
 }
 
+static int disambiguate_tag_only(const struct object_id *oid, void *cb_data_unused)
+{
+	int kind = oid_object_info(oid, NULL);
+	return kind == OBJ_TAG;
+}
+
 static int disambiguate_commit_only(const struct object_id *oid, void *cb_data_unused)
 {
 	int kind = oid_object_info(oid, NULL);

@@ -288,7 +294,8 @@ int set_disambiguate_hint_config(const char *var, const char *value)
 		{ "committish", disambiguate_committish_only },
 		{ "tree", disambiguate_tree_only },
 		{ "treeish", disambiguate_treeish_only },
-		{ "blob", disambiguate_blob_only }
+		{ "blob", disambiguate_blob_only },
+		{ "tag", disambiguate_tag_only }
 	};
 	int i;
 

@@ -429,6 +436,8 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
 		ds.fn = disambiguate_treeish_only;
 	else if (flags & GET_OID_BLOB)
 		ds.fn = disambiguate_blob_only;
+	else if (flags & GET_OID_TAG)
+		ds.fn = disambiguate_tag_only;
 	else
 		ds.fn = default_disambiguate_hint;
 

@@ -958,6 +967,8 @@ static int peel_onion(const char *name, int len, struct object_id *oid,
 	lookup_flags &= ~GET_OID_DISAMBIGUATORS;
 	if (expected_type == OBJ_COMMIT)
 		lookup_flags |= GET_OID_COMMITTISH;
+	else if (expected_type == OBJ_TAG)
+		lookup_flags |= GET_OID_TAG;
 	else if (expected_type == OBJ_TREE)
 		lookup_flags |= GET_OID_TREEISH;
 

index 2701462041..74e7d9c178 100755
@@ -334,7 +334,10 @@ test_expect_success C_LOCALE_OUTPUT 'ambiguity hints respect type' '
 	test_must_fail git rev-parse 000000000^{commit} 2>stderr &&
 	grep ^hint: stderr >hints &&
 	# 5 commits, 1 tag (which is a commitish), plus intro line
-	test_line_count = 7 hints
+	test_line_count = 7 hints &&
+	git rev-parse 000000000^{tag} >stdout &&
+	test_line_count = 1 stdout &&
+	grep -q ^0000000000f8f stdout
 '
 
 test_expect_success C_LOCALE_OUTPUT 'failed type-selector still shows hint' '

2.17.0.290.gded63e768a

The disambiguation logic had all the pieces necessary to only print out those blobs that were ambiguous, but they hadn't been connected.

The initial logic was added in daba53aeaf ("sha1_name.c: add support for disambiguating other types", 2012-07-02), and when the flags were propagated in 8a10fea49b ("get_sha1: propagate flags to child functions", 2016-09-26) GET_OID_BLOB wasn't added to lookup_flags.

Before this change requests for blobs were simply ignored:

    $ git rev-parse e8f2^{blob}
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f2650052 tag v2.17.0
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f2867228 blob
    hint:   e8f2a35526 blob
    e8f2^{blob}
    [...]

But now we'll do the right thing and only print the blobs:

    $ git rev-parse e8f2^{blob}
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f2867228 blob
    hint:   e8f2a35526 blob
    e8f2^{blob}
    [...]

---
 sha1-name.c                         | 2 ++
 t/t1512-rev-parse-disambiguation.sh | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

index 68d5f65362..023f9471a8 100644
@@ -971,6 +971,8 @@ static int peel_onion(const char *name, int len, struct object_id *oid,
 		lookup_flags |= GET_OID_TAG;
 	else if (expected_type == OBJ_TREE)
 		lookup_flags |= GET_OID_TREEISH;
+	else if (expected_type == OBJ_BLOB)
+		lookup_flags |= GET_OID_BLOB;
 
 	if (get_oid_1(name, sp - name - 2, &outer, lookup_flags))
 		return -1;

index 74e7d9c178..9ce9cc3bc3 100755
@@ -337,7 +337,11 @@ test_expect_success C_LOCALE_OUTPUT 'ambiguity hints respect type' '
 	test_line_count = 7 hints &&
 	git rev-parse 000000000^{tag} >stdout &&
 	test_line_count = 1 stdout &&
-	grep -q ^0000000000f8f stdout
+	grep -q ^0000000000f8f stdout &&
+	test_must_fail git rev-parse 000000000^{blob} 2>stderr &&
+	grep ^hint: stderr >hints &&
+	# 5 blobs plus intro line &&
+	test_line_count = 6 hints
 '
 
 test_expect_success C_LOCALE_OUTPUT 'failed type-selector still shows hint' '

2.17.0.290.gded63e768a

After the recent series of patches ^{tag} and ^{blob} now work to get just the tags and blobs, but ^{tree} will still list any tree-ish (commits, tags and trees).

The previous behavior was added in ed1ca6025f ("peel_onion:
disambiguate to favor tree-ish when we know we want a tree-ish", 2013-03-31). I may have missed some special-case but this makes more sense to me.

Now "$sha1:" can be used as before to mean treeish

    $ git rev-parse e8f2:
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f2650052 tag v2.17.0
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    [...]

But ^{tree} shows just the trees, but would previously be equivalent to the above:

    $ git rev-parse e8f2^{tree}
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    [...]

---
 sha1-name.c                         |  2 +-
 t/t1512-rev-parse-disambiguation.sh | 18 ++++++++++++++----
 2 files changed, 15 insertions(+), 5 deletions(-)

index 023f9471a8..b61c0558d9 100644
@@ -970,7 +970,7 @@ static int peel_onion(const char *name, int len, struct object_id *oid,
 	else if (expected_type == OBJ_TAG)
 		lookup_flags |= GET_OID_TAG;
 	else if (expected_type == OBJ_TREE)
-		lookup_flags |= GET_OID_TREEISH;
+		lookup_flags |= GET_OID_TREE;
 	else if (expected_type == OBJ_BLOB)
 		lookup_flags |= GET_OID_BLOB;
 

index 9ce9cc3bc3..81076449a2 100755
@@ -159,9 +159,13 @@ test_expect_failure 'two semi-ambiguous commit-ish' '
 	git log 0000000000...
 '
 
-test_expect_failure 'three semi-ambiguous tree-ish' '
+test_expect_success '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
+	test_must_fail git rev-parse --verify 0000000000: &&
+
+	# For ^{tree} we can disambiguate because HEAD and v1.0.0 will
+	# be excluded.
 	git rev-parse --verify 0000000000^{tree}
 '
 

@@ -267,8 +271,12 @@ test_expect_success 'ambiguous commit-ish' '
 # There are three objects with this prefix: a blob, a tree, and a tag. We know
 # the blob will not pass as a treeish, but the tree and tag should (and thus
 # cause an error).
-test_expect_success 'ambiguous tags peel to treeish' '
-	test_must_fail git rev-parse 0000000000f^{tree}
+test_expect_success 'ambiguous tags peel to treeish or tree' '
+	test_must_fail git rev-parse 0000000000f: &&
+	git rev-parse 0000000000f^{tree} >stdout &&
+	test_line_count = 1 stdout &&
+	grep -q ^0000000000fd8bcc56 stdout
+
 '
 
 test_expect_success 'rev-parse --disambiguate' '

@@ -365,7 +373,9 @@ test_expect_success 'core.disambiguate config can prefer types' '
 test_expect_success 'core.disambiguate does not override context' '
 	# treeish ambiguous between tag and tree
 	test_must_fail \
-		git -c core.disambiguate=committish rev-parse $sha1^{tree}
+		git -c core.disambiguate=committish rev-parse $sha1: &&
+	# tree not ambiguous between tag and tree
+	git -c core.disambiguate=committish rev-parse $sha1^{tree}
 '
 
 test_expect_success C_LOCALE_OUTPUT 'ambiguous commits are printed by type first, then hash order' '

2.17.0.290.gded63e768a

Change the ^{commit} syntax to mean just commits instead of committish for the purpose of disambiguation. Before this e8f2^{commit} would show the v2.17.0 tag as a disambiguation candidate, but now it'll just show ambiguous commits:

    $ git rev-parse e8f2^{commit}
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    [...]

---
 sha1-name.c                         | 2 +-
 t/t1512-rev-parse-disambiguation.sh | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

index b61c0558d9..1d2a74a29c 100644
@@ -966,7 +966,7 @@ static int peel_onion(const char *name, int len, struct object_id *oid,
 
 	lookup_flags &= ~GET_OID_DISAMBIGUATORS;
 	if (expected_type == OBJ_COMMIT)
-		lookup_flags |= GET_OID_COMMITTISH;
+		lookup_flags |= GET_OID_COMMIT;
 	else if (expected_type == OBJ_TAG)
 		lookup_flags |= GET_OID_TAG;
 	else if (expected_type == OBJ_TREE)

index 81076449a2..b17973a266 100755
@@ -341,8 +341,8 @@ test_expect_success C_LOCALE_OUTPUT 'ambiguity hints' '
 test_expect_success C_LOCALE_OUTPUT 'ambiguity hints respect type' '
 	test_must_fail git rev-parse 000000000^{commit} 2>stderr &&
 	grep ^hint: stderr >hints &&
-	# 5 commits, 1 tag (which is a commitish), plus intro line
-	test_line_count = 7 hints &&
+	# 5 commits plus intro line
+	test_line_count = 6 hints &&
 	git rev-parse 000000000^{tag} >stdout &&
 	test_line_count = 1 stdout &&
 	grep -q ^0000000000f8f stdout &&

@@ -366,7 +366,8 @@ test_expect_success 'core.disambiguate config can prefer types' '
 	# ambiguous between tree and tag
 	sha1=0000000000f &&
 	test_must_fail git rev-parse $sha1 &&
-	git rev-parse $sha1^{commit} &&
+	# there is no commit so ^{commit} comes up empty
+	test_must_fail git rev-parse $sha1^{commit} &&
 	git -c core.disambiguate=committish rev-parse $sha1
 '
 

2.17.0.290.gded63e768a

The core.disambiguate variable was added in 5b33cb1fd7 ("get_short_sha1: make default disambiguation configurable", 2016-09-27) but never documented.

---
 Documentation/config.txt | 13 +++++++++++++
 1 file changed, 13 insertions(+)

index 2659153cb3..14a3d57e77 100644
@@ -910,6 +910,19 @@ core.abbrev::
 	abbreviated object names to stay unique for some time.
 	The minimum length is 4.
 
+core.disambiguate::
+	If Git is given a SHA-1 that's ambigous it'll suggest what
+	objects you might mean. By default it'll print out all
+	potential objects with that prefix regardless of their
+	type. This setting, along with the `^{<type>}` peel syntax
+	(see linkgit:gitrevisions[7]), allows for narrowing that down.
++
+Is set to `none` by default to show all object types. Can also be
+`commit` (peel syntax: `$sha1^{commit}`), `committish` (commits and
+tags), `tree` (peel: `$sha1^{tree}`), `treeish` (everything except
+blobs, peel syntax: `$sha1:`), `blob` (peel: `$sha1^{blob}`) or `tag`
+(peel: `$sha1^{tag}`). The peel syntax will override any config value.
+
 add.ignoreErrors::
 add.ignore-errors (deprecated)::
 	Tells 'git add' to continue adding files when some files cannot be

2.17.0.290.gded63e768a

The SHA1 prefix 06fa currently matches no blobs in git.git. When disambiguating short SHA1s we've been quietly ignoring the user's type selector as a fallback mechanism, this was intentionally added in 1ffa26c461 ("get_short_sha1: list ambiguous objects on error", 2016-09-26).

I think that behavior makes sense, it's not very useful to just show nothing because a preference has been expressed via core.disambiguate, but it's bad that we're quietly doing this. The user might thing that we just didn't understand what e.g 06fa^{blob} meant.

Now we'll instead print a warning if no objects of the requested type were found:

    $ git rev-parse 06fa^{blob}
    error: short SHA1 06fa is ambiguous
    hint: The candidates are:
    [... no blobs listed ...]
    warning: Your hint (via core.disambiguate or peel syntax) was ignored, we fell
    back to showing all object types since no object of the requested type
    matched the provide short SHA1 06fa

---
 Documentation/config.txt            |  4 ++++
 sha1-name.c                         | 11 ++++++++++-
 t/t1512-rev-parse-disambiguation.sh |  5 ++++-
 3 files changed, 18 insertions(+), 2 deletions(-)

index 14a3d57e77..e14f2c0492 100644
@@ -922,6 +922,10 @@ Is set to `none` by default to show all object types. Can also be
 tags), `tree` (peel: `$sha1^{tree}`), `treeish` (everything except
 blobs, peel syntax: `$sha1:`), `blob` (peel: `$sha1^{blob}`) or `tag`
 (peel: `$sha1^{tag}`). The peel syntax will override any config value.
++
+If no objects of the selected type exist the disambiguation will fall
+back to `none` and print a warning indicating no objects of the
+selected type could be found for that prefix.
 
 add.ignoreErrors::
 add.ignore-errors (deprecated)::

index 1d2a74a29c..9789764a38 100644
@@ -447,6 +447,7 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
 
 	if (!quietly && (status == SHORT_NAME_AMBIGUOUS)) {
 		struct oid_array collect = OID_ARRAY_INIT;
+		int ignored_hint = 0;
 
 		error(_("short SHA1 %s is ambiguous"), ds.hex_pfx);
 

@@ -456,8 +457,10 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
 		 * that case, we still want to show them, so disable the hint
 		 * function entirely.
 		 */
-		if (!ds.ambiguous)
+		if (!ds.ambiguous) {
 			ds.fn = NULL;
+			ignored_hint = 1;
+		}
 
 		advise(_("The candidates are:"));
 		for_each_abbrev(ds.hex_pfx, collect_ambiguous, &collect);

@@ -466,6 +469,12 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
 		if (oid_array_for_each(&collect, show_ambiguous_object, &ds))
 			BUG("show_ambiguous_object shouldn't return non-zero");
 		oid_array_clear(&collect);
+
+		if (ignored_hint) {
+			warning(_("Your hint (via core.disambiguate or peel syntax) was ignored, we fell\n"
+				  "back to showing all object types since no object of the requested type\n"
+				  "matched the provide short SHA1 %s"), ds.hex_pfx);
+		}
 	}
 
 	return status;

index b17973a266..940f323ee9 100755
@@ -359,7 +359,10 @@ test_expect_success C_LOCALE_OUTPUT 'failed type-selector still shows hint' '
 	echo 872 | git hash-object --stdin -w &&
 	test_must_fail git rev-parse ee3d^{commit} 2>stderr &&
 	grep ^hint: stderr >hints &&
-	test_line_count = 3 hints
+	test_line_count = 3 hints &&
+	grep ^warning stderr >warnings &&
+	grep -q "Your hint.*was ignored" warnings &&
+	grep -q "the provide short SHA1 ee3d" stderr
 '
 
 test_expect_success 'core.disambiguate config can prefer types' '

2.17.0.290.gded63e768a

From: Ævar Arnfjörð Bjarmason <[email protected]>

Here is what I mean by sorting during for_each_abbrev(). This seems to work for me, so I don't know what the issue is with this one-pass approach.

Thanks,
-Stolee

-- >8 --

Change the output emitted when an ambiguous object is encountered so that we show tags first, then commits, followed by trees, and finally blobs. Within each type we show objects in hashcmp() order. Before this change the objects were only ordered by hashcmp().

The reason for doing this is that the output looks better as a result, e.g. the v2.17.0 tag before this change on "git show e8f2" would display:

    hint: The candidates are:
    hint:   e8f2093055 tree
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f25a3a50 tree
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2650052 tag v2.17.0
    hint:   e8f2867228 blob
    hint:   e8f28d537c tree
    hint:   e8f2a35526 blob
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2cf6ec0 tree

Now we'll instead show:

    hint:   e8f2650052 tag v2.17.0
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f2867228 blob
    hint:   e8f2a35526 blob

Since we show the commit data in the output that's nicely aligned once we sort by object type. The decision to show tags before commits is pretty arbitrary. I don't want to order by object_type since there tags come last after blobs, which doesn't make sense if we want to show the most important things first.

I could display them after commits, but it's much less likely that we'll display a tag, so if there is one it makes sense to show it prominently at the top.

---
 sha1-name.c                         | 31 +++++++++++++++++++++++++++++
 t/t1512-rev-parse-disambiguation.sh | 21 +++++++++++++++++++
 2 files changed, 52 insertions(+)

index 5deebab56d..0336630c64 100644
@@ -385,6 +385,34 @@ static int collect_ambiguous(const struct object_id *oid, void *data)
 	return 0;
 }
 
+static int sort_ambiguous(const void *a, const void *b)
+{
+	int a_type = oid_object_info(a, NULL);
+	int b_type = oid_object_info(b, NULL);
+	int a_type_sort;
+	int b_type_sort;
+
+	/*
+	 * Sorts by hash within the same object type, just as
+	 * oid_array_for_each_unique() would do.
+	 */
+	if (a_type == b_type)
+		return oidcmp(a, b);
+
+	/*
+	 * Between object types show tags, then commits, and finally
+	 * trees and blobs.
+	 *
+	 * The object_type enum is commit, tree, blob, tag, but we
+	 * want tag, commit, tree blob. Cleverly (perhaps too
+	 * cleverly) do that with modulus, since the enum assigns 1 to
+	 * commit, so tag becomes 0.
+	 */
+	a_type_sort = a_type % 4;
+	b_type_sort = b_type % 4;
+	return a_type_sort > b_type_sort ? 1 : -1;
+}
+
 static int get_short_oid(const char *name, int len, struct object_id *oid,
 			  unsigned flags)
 {

@@ -451,6 +479,9 @@ int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
 	find_short_object_filename(&ds);
 	find_short_packed_object(&ds);
 
+	QSORT(collect.oid, collect.nr, sort_ambiguous);
+	collect.sorted = 1;
+
 	ret = oid_array_for_each_unique(&collect, fn, cb_data);
 	oid_array_clear(&collect);
 	return ret;

index c7ceda2f21..74e7d9c178 100755
@@ -364,4 +364,25 @@ test_expect_success 'core.disambiguate does not override context' '
 		git -c core.disambiguate=committish rev-parse $sha1^{tree}
 '
 
+test_expect_success C_LOCALE_OUTPUT 'ambiguous commits are printed by type first, then hash order' '
+	test_must_fail git rev-parse 0000 2>stderr &&
+	grep ^hint: stderr >hints &&
+	grep 0000 hints >objects &&
+	cat >expected <<-\EOF &&
+	tag
+	commit
+	tree
+	blob
+	EOF
+	awk "{print \$3}" <objects >objects.types &&
+	uniq <objects.types >objects.types.uniq &&
+	test_cmp expected objects.types.uniq &&
+	for type in tag commit tree blob
+	do
+		grep $type objects >$type.objects &&
+		sort $type.objects >$type.objects.sorted &&
+		test_cmp $type.objects.sorted $type.objects
+	done
+'
+
 test_done

2.17.0.39.g685157f7fb

To work properly, duplicate entries must be consecutive. Since duplicate entries have the same type, our sort satisfies this condition.

That makes sense.

Sounds good.

Thanks,
-Stolee

Actually I'm having second thoughts about that and thinking I might keep my original approach (with a better explanation).

A few more lines of code seems worthwhile in order to not break the assumptions a documented API is making, no matter how briefly, so I set about documenting this case and supporting it, since
e.g. oid_array_lookup() will completely fail with the hack of setting the .sorted member, and came up with this:

index b0c11f868d..ff87260220 100644
@@ -16,6 +16,20 @@ Data Structures
 	the actual data. The `nr` member contains the number of items in
 	the set.  The `alloc` and `sorted` members are used internally,
 	and should not be needed by API callers.
++
+Both the `oid_array_lookup` and `oid_array_for_each_unique` functions
+rely on the array being sorted. For the former it's an absolute
+requirenment that the internal `oid_array_sort` function has been
+called on it, bu for the latter it's enough that the elements are
+ordered in such a way as to guarantee that identical object IDs are
+adjacent in the array.
++
+This is useful e.g. to print output where commits, tags etc. are
+grouped together (barring a hash collision they won't have the same
+object ID), in such cases the `custom_sorted` member can be set to `1`
+before calling `oid_array_for_each_unique`, and it'll skip its own
+sorting. Once it's been set calling e.g. `oid_array_lookup` without it
+being cleared again will cause an internal panic, so use it carefully.
 
 Functions
 ---------

index 466a926aa3..cbae07ff78 100644
@@ -18,6 +18,7 @@ static void oid_array_sort(struct oid_array *array)
 {
 	QSORT(array->oid, array->nr, void_hashcmp);
 	array->sorted = 1;
+	array->custom_sorted = 0;
 }
 
 static const unsigned char *sha1_access(size_t index, void *table)

@@ -28,6 +29,13 @@ static const unsigned char *sha1_access(size_t index, void *table)
 
 int oid_array_lookup(struct oid_array *array, const struct object_id *oid)
 {
+	if (array->custom_sorted)
+		/*
+		 * We could also just clear custom_sorted here, but if
+		 * the caller is custom sorting and then calling this
+		 * that's likely something they'd like to know about.
+		 */
+		BUG("PANIC: Cannot lookup OIDs in arrays with a custom sort!");
 	if (!array->sorted)
 		oid_array_sort(array);
 	return sha1_pos(oid->hash, array->oid, array->nr, sha1_access);

@@ -39,6 +47,7 @@ void oid_array_clear(struct oid_array *array)
 	array->nr = 0;
 	array->alloc = 0;
 	array->sorted = 0;
+	array->custom_sorted = 0;
 }
 
 int oid_array_for_each_unique(struct oid_array *array,

@@ -47,7 +56,7 @@ int oid_array_for_each_unique(struct oid_array *array,
 {
 	int i;
 
-	if (!array->sorted)
+	if (!array->sorted && !array->custom_sorted)
 		oid_array_sort(array);
 
 	for (i = 0; i < array->nr; i++) {

index 1e1d24b009..bfa77ba1e4 100644
@@ -6,6 +6,7 @@ struct oid_array {
 	int nr;
 	int alloc;
 	int sorted;
+	int custom_sorted;
 };
 
 #define OID_ARRAY_INIT { NULL, 0, 0, 0 }

index b81e07adbb..d190800db0 100644
@@ -490,9 +490,11 @@ int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
 	find_short_packed_object(&ds);
 
 	QSORT(collect.oid, collect.nr, sort_ambiguous);
-	collect.sorted = 1;
 
+	collect.custom_sorted = 1;
 	ret = oid_array_for_each_unique(&collect, fn, cb_data);
+	collect.custom_sorted = 0;
+
 	oid_array_clear(&collect);
 	return ret;
 }

So maybe I should just stop worrying and YOLO it, it just seems wrong to leave such a fragile setup in place where we set .sorted=1 and some future refactoring reasonably tries to call oid_array_lookup() on it and silently fails.

What do you think?

I think this extra custom_sort check is worth keeping the API stable to future changes.

Thanks,
-Stolee

Comments inline:

Ævar Arnfjörð Bjarmason (12):
sha1-name.c: remove stray newline

No changes.

sha1-array.h: align function arguments

Mention the correct commit to blame for disalignment in the commit message, and also fix it in the *.c file.

git-p4: change "commitish" typo to "committish"

No changes.

cache.h: add comment explaining the order in object_type

Trivial commit message rewording.

sha1-name.c: move around the collect_ambiguous() function

No changes.

get_short_oid: sort ambiguous objects by type, then SHA-1

The biggest change in v3 is the no change at all to the code, but a lengthy explanation of why I didn't go for Derrick's simpler implementation. Maybe I'm wrong about that, but I felt uneasy offloading undocumented (or if I documented it, it would only be for this one edge-case) magic on the oid_array API. Instead I'm just making this patch a bit more complex.

get_short_oid: learn to disambiguate by ^{tag}
get_short_oid: learn to disambiguate by ^{blob} get_short_oid / peel_onion: ^{tree} should be tree, not treeish get_short_oid / peel_onion: ^{commit} should be commit, not committish config doc: document core.disambiguate
get_short_oid: document & warn if we ignore the type selector

No changes except one trivial commit message formatting fix.

 Documentation/config.txt                  | 17 +++++
 Documentation/technical/api-oid-array.txt | 17 +++--
 cache.h                                   | 13 +++-
 git-p4.py                                 |  6 +-
 sha1-array.c                              | 21 +++++-
 sha1-array.h                              |  7 +-
 sha1-name.c                               | 80 +++++++++++++++++++----
 t/t1512-rev-parse-disambiguation.sh       | 58 +++++++++++++---
 8 files changed, 184 insertions(+), 35 deletions(-)

2.17.0.290.gded63e768a

This stray newline was accidentally introduced in d2b7d9c7ed ("sha1_name: convert disambiguate_hint_fn to take object_id", 2017-03-26).

---
 sha1-name.c | 1 -
 1 file changed, 1 deletion(-)

index 5b93bf8da3..cd3b133aae 100644
@@ -346,7 +346,6 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
 	struct strbuf desc = STRBUF_INIT;
 	int type;
 
-
 	if (ds->fn && !ds->fn(oid, ds->cb_data))
 		return 0;
 

2.17.0.290.gded63e768a

The arguments weren't lined up with the opening parenthesis. Fixes up code added in aae0caf19e ("sha1-array.h: align function arguments", 2018-04-30).

---
 sha1-array.c | 4 ++--
 sha1-array.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

index 838b3bf847..466a926aa3 100644
@@ -42,8 +42,8 @@ void oid_array_clear(struct oid_array *array)
 }
 
 int oid_array_for_each_unique(struct oid_array *array,
-				for_each_oid_fn fn,
-				void *data)
+			      for_each_oid_fn fn,
+			      void *data)
 {
 	int i;
 

index 04b0756334..1e1d24b009 100644
@@ -17,7 +17,7 @@ void oid_array_clear(struct oid_array *array);
 typedef int (*for_each_oid_fn)(const struct object_id *oid,
 			       void *data);
 int oid_array_for_each_unique(struct oid_array *array,
-			       for_each_oid_fn fn,
-			       void *data);
+			      for_each_oid_fn fn,
+			      void *data);
 
 #endif /* SHA1_ARRAY_H */

2.17.0.290.gded63e768a

This was the only occurrence of "commitish" in the tree, but as the log will reveal we've had others in the past. Fixes up code added in 00ad6e3182 ("git-p4: work with a detached head", 2015-11-21).

---
 git-p4.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 7bb9cadc69..1afa87cd9d 100755
@@ -2099,11 +2099,11 @@ class P4Submit(Command, P4UserMap):
 
         commits = []
         if self.master:
-            commitish = self.master
+            committish = self.master
         else:
-            commitish = 'HEAD'
+            committish = 'HEAD'
 
-        for line in read_pipe_lines(["git", "rev-list", "--no-merges", "%s..%s" % (self.origin, commitish)]):
+        for line in read_pipe_lines(["git", "rev-list", "--no-merges", "%s..%s" % (self.origin, committish)]):
             commits.append(line.strip())
         commits.reverse()
 

2.17.0.290.gded63e768a

The order in the enum might seem arbitrary, and isn't explained by 72518e9c26 ("more lightweight revalidation while reusing deflated stream in packing", 2006-09-03) which added it.

Derrick Stolee suggested that it's ordered topologically in [email protected]. Makes sense to me, add that as a comment.

---
 cache.h | 8 ++++++++
 1 file changed, 8 insertions(+)

index 77b7acebb6..354903c3ea 100644
@@ -376,6 +376,14 @@ extern void free_name_hash(struct index_state *istate);
 enum object_type {
 	OBJ_BAD = -1,
 	OBJ_NONE = 0,
+	/*
+	 * Why have our our "real" object types in this order? They're
+	 * ordered topologically:
+	 *
+	 * tag(4)    -> commit(1), tree(2), blob(3)
+	 * commit(1) -> tree(2)
+	 * tree(2)   -> blob(3)
+	 */
 	OBJ_COMMIT = 1,
 	OBJ_TREE = 2,
 	OBJ_BLOB = 3,

2.17.0.290.gded63e768a

When referring to a message-id, please do not omit surrounding <>, which is part of the message-id string. That's like writing your e-mail address as avarab gmail.com without at sign.

The current document mentions OBJ_* constants without their actual values. A git developer would know these are from cache.h but that's not very friendly to a person who wants to read this file to implement a pack file parser.

Similarly, the deltified representation is not documented at all (the "document" is basically patch-delta.c). Translate that C code in English.

---
I noticed that these object type values are not documented in pack-format.txt so here's my attempt to improve it.

While at there, I also add some text about this obscure delta format. We occasionally have questions about this on the mailing list if I remember correctly.

 Documentation/technical/pack-format.txt | 41 +++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

index 8e5bf60be3..2c7d5c0e74 100644
@@ -36,6 +36,47 @@ Git pack format
 
   - The trailer records 20-byte SHA-1 checksum of all of the above.
 
+Valid object types are:
+
+- OBJ_COMMIT (1)
+- OBJ_TREE (2)
+- OBJ_BLOB (3)
+- OBJ_TAG (4)
+- OBJ_OFS_DELTA (6)
+- OBJ_REF_DELTA (7)
+
+Type 5 is reserved for future expansion.
+
+Deltified representation is a sequence of one byte command optionally
+followed by more data for the command. The following commands are
+recognized:
+
+- If bit 7 is set, the remaining bits in the command byte specifies
+  how to extract copy offset and size to copy. The following must be
+  evaluated in this exact order:
+  - If bit 0 is set, the following byte contains bits 0-7 of the copy
+    offset (this also resets all other bits in the copy offset to
+    zero).
+  - If bit 1 is set, the following byte contains bits 8-15 of the copy
+    offset.
+  - If bit 2 is set, the following byte contains bits 16-23 of the
+    copy offset.
+  - If bit 3 is set, the following byte contains bits 24-31 of the
+    copy offset.
+  - If bit 4 is set, the following byte contains bits 0-7 of the copy
+    size (this also resets all other bits in the copy size to zero_.
+  - If bit 5 is set, the following byte contains bits 8-15 of the copy
+    size.
+  - If bit 6 is set, the following byte contains bits 16-23 of the
+    copy size.
+
+  Copy size zero means 0x10000 bytes. The data from source object at
+  the given copy offset is copied back to the destination buffer.
+
+- If bit 7 is not set, it is the copy size in bytes. The following
+  bytes are copied to destination buffer
+- Command byte zero is reserved for future expansion.
+
 == Original (version 1) pack-*.idx files have the following format:
 
   - The header consists of 256 4-byte network byte order

2.17.0.705.g3525833791

Thanks for sending this patch!

Let me see if I can understand it, as I am not well versed in the delta format, so ideally I would understand it from the patch here?

Well yes. I don't expect my first version to be that easy to understand. This is where you come in to help ;-)

type 0 is invalid. I think in some encoding it's not even possible to encode zero. Anyway yes it should be spelled out.

Yes. Both OFS and REF deltas have the same "body" which is what this part is about. The differences between OFS and REF deltas are not described (in fact I don't think we describe what OFS and REF deltas are at all).

I started to update this more because even this text is hard to get even to me. So let's get the background first.

We have a source object somewhere (the object name comes from ofs/ref delta's header), basically we have the whole content. This delta thingy tells us how to use that source object to create a new (target) object.

The delta is actually a sequence of instructions (of variable length). One is for copying from the source object. The other copies from the delta itself (e.g. this is new data in the target which is not available anywhere in the source object to copy from). The last bit of the first byte determines what instruction type it is.

My first attempt at "translating to English" is like a constructing C from assembly: it's horrible.

The instruction looks like this

bit 0 1 2 3 4 5 6

  +----------+--------+--------+--------+--------+------+------+------+
  | 1xxxxxxx | offset | offset | offset | offset | size | size | size |
  +----------+--------+--------+--------+--------+------+------+------+

Here you can see it in its full form, each box represents a byte. The first byte has bit 7 set as mentioned. We can see here that offsets (where to copy from in the source object) takes 4 bytes and size (how many bytes to copy) takes 3. Offset size size is in LSB order.

The "xxxxxxx" part lets us shrink this down. If the offset can fit in 16 bits, there's no reason to waste the last two bytes describing zero. Each 'x' marks whether the corresponding byte is present. The bit number is in the first row. So if you have offset 255 and size 1, the instruction is three bytes 10010001b, 255, 1. The octets on "bit column" 1, 2, 3, 5 and 6 are missing because the corresponding bit in the first bit is not set.

I think this is a corner case in this format. I think Nico meant to specify consecutive bytes: if size is 2 bytes then you have to specify _both_ of them even if the first byte could be zero and omitted.

The implementation detail is, if bit 6 is set but bit 4 is not, then the size value is pretty much random. It's only when bit 4 is set that we first clear out "size" and start adding bits to it.

Yes. There's no point to copy nothing. And it saves space to not specify "size" at all. I think this is meant to copy a very large part from the source, so you just continue to copy a series of 0x10000 chunks.

Duy

Maybe we should?

The previous paragraph and this sentence are great for my understanding. thanks! (Maybe keep it in a similar form around?)

ok that makes sense. I can think of it as a "HTTP range request", just optimized for packfiles and the source is inside the same pack. So it would say "Goto object <sha1> and copy bytes 13-168 here"

itself means the same object here, that we are describing here? or does it mean other deltas?

.. by indicating how much prefix we can skip and assume it be all zero(?)

So for a full instruction (as above), we'd have to

1 1111 111 <4 bytes offset> <3 bytes size>

for smaller instructions we have

1 1100 100 <2 bytes offset> <1 byte size>
and here the offset is in range 0..64k and
the size is 1-255 or 0x10000 ?

Modes to skip bytes in between are not allowed, e.g. 1 1101 101 < 3 bytes of offsets> <2 bytes of size> and the missing bytes would be assumed to be 0?

Oh it is the other way round, the size will be just one byte, indicating we can have a range of 1-255 or 0x10000 and an offset of 0..255.

So it is not a mutually exclusive group, but a sequence (similar as in git-bisect), where we start with 0 and end with exactly one edge in between (sort of, we can also start with 1, then we have to have all 1s)

That sounds similar to what I spelled out above.

Thanks for taking on the documentation here.
The box with numbers really helped me!

Stefan

Thanks, will drop my version from v4, although a comment for the enum in cache.h pointing the reader to these docs would be very useful.

True. I will add some together with the pack-format.txt update.

Duy

The current document mentions OBJ_* constants without their actual values. A git developer would know these are from cache.h but that's not very friendly to a person who wants to read this file to implement a pack file parser.

Similarly, the deltified representation is not documented at all (the "document" is basically patch-delta.c). Translate that C code to English with a bit more about what ofs-delta and ref-delta mean.

---
This is a much better description than v1. I hope.

 Documentation/technical/pack-format.txt | 78 +++++++++++++++++++++++++
 cache.h                                 |  5 ++
 2 files changed, 83 insertions(+)

index 8e5bf60be3..d20bf592aa 100644
@@ -36,6 +36,84 @@ Git pack format
 
   - The trailer records 20-byte SHA-1 checksum of all of the above.
 
+=== Object types
+
+Valid object types are:
+
+- OBJ_COMMIT (1)
+- OBJ_TREE (2)
+- OBJ_BLOB (3)
+- OBJ_TAG (4)
+- OBJ_OFS_DELTA (6)
+- OBJ_REF_DELTA (7)
+
+Type 5 is reserved for future expansion. Type 0 is invalid.
+
+=== Deltified representation
+
+Conceptually there are only four object types: commit, tree, tag and
+blob. However to save space, an object could be stored as a "delta" of
+another "base" object. These representations are assigned new types
+ofs-delta and ref-delta, which is only valid in a pack file.
+
+Both ofs-delta and ref-delta store the "delta" against another
+object. The difference between them is, ref-delta directly encodes
+20-byte base object name. If the base object is in the same pack,
+ofs-delta encodes the offset of the base object in the pack instead.
+
+The delta data is a sequence of instructions to reconstruct an object
+from the base object. Each instruction appends more and more data to
+the target object until it's complete. There are two supported
+instructions so far: one for copy a byte range from the source object
+and one for inserting new data embedded in the instruction itself.
+
+Each instruction has variable length. Instruction type is determined
+by the seventh bit of the first octet. The following diagrams follow
+the convention in RFC 1951 (Deflate compressed data format).
+
+  +----------+---------+---------+---------+---------+-------+-------+-------+
+  | 1xxxxxxx | offset1 | offset2 | offset3 | offset4 | size1 | size2 | size3 |
+  +----------+---------+---------+---------+---------+-------+-------+-------+
+
+This is the instruction format to copy a byte range from the source
+object. It encodes the offset to copy from any the number of bytes to
+copy. Offset and size are in little-endian order.
+
+All offset and size bytes are optional. This is to reduce the
+instruction size when encoding small offsets or sizes. The first seven
+bits in the first octet determines which of the next seven octets is
+present. If bit zero is set, offset1 is present. If bit one is set
+offset2 is present and so on.
+
+Note that a more compact instruction does not change offset and size
+encoding. For example, if only offset2 is omitted like below, offset3
+still contains bits 16-23. It does not become offset2 and contains
+bits 8-15 even if it's right next to offset1.
+
+  +----------+---------+---------+
+  | 10000101 | offset1 | offset3 |
+  +----------+---------+---------+
+
+In its most compact form, this instruction only takes up one byte
+(0x80) with both offset and size omitted, which will have default
+values zero. There is another exception: size zero is automatically
+converted to 0x10000.
+
+  +----------+============+
+  | 0xxxxxxx |    data    |
+  +----------+============+
+
+This is the instruction to construct target object without the base
+object. The following data is appended to the target object. The first
+seven bits of the first octet determines the size of data in
+bytes. The size must be non-zero.
+
+  +----------+============
+  | 00000000 |
+  +----------+============
+
+This is the instruction reserved for future expansion.
+
 == Original (version 1) pack-*.idx files have the following format:
 
   - The header consists of 256 4-byte network byte order

index 77b7acebb6..ad549e258e 100644
@@ -373,6 +373,11 @@ extern void free_name_hash(struct index_state *istate);
 #define read_blob_data_from_cache(path, sz) read_blob_data_from_index(&the_index, (path), (sz))
 #endif
 
+/*
+ * Values in this enum (except those outside the 3 bit range) are part
+ * of pack file format. See Documentation/technical/pack-format.txt
+ * for more information.
+ */
 enum object_type {
 	OBJ_BAD = -1,
 	OBJ_NONE = 0,

2.17.0.705.g3525833791

Yeah. This is pretty much an implementation detail of a pack. The "real" type is always blob/commit/tree/tag. But you only see this when you dig deep down in pack-related code.

I guess so. I never bothered to track down the source for that.

Yeah that's the delta chain concept. I'll just make a note here about base object potentially being a delta object as well.

Duy

The current document mentions OBJ_* constants without their actual values. A git developer would know these are from cache.h but that's not very friendly to a person who wants to read this file to implement a pack file parser.

Similarly, the deltified representation is not documented at all (the "document" is basically patch-delta.c). Translate that C code to English with a bit more about what ofs-delta and ref-delta mean.

---
Diff from v2

diff --git a/Documentation/technical/pack-format.txt b/Documentation/technical/pack-format.txt index 00351cb822..70a99fd142 100644
--- a/Documentation/technical/pack-format.txt
+++ b/Documentation/technical/pack-format.txt
@@ -56,27 +56,37 @@ blob. However to save space, an object could be stored as a "delta" of
another "base" object. These representations are assigned new types
ofs-delta and ref-delta, which is only valid in a pack file.

-Both ofs-delta and ref-delta store the "delta" against another -object. The difference between them is, ref-delta directly encodes -20-byte base object name. If the base object is in the same pack, -ofs-delta encodes the offset of the base object in the pack instead. +Both ofs-delta and ref-delta store the "delta" to be applied to +another object (called 'base object') to reconstruct the object. The +difference between them is, ref-delta directly encodes 20-byte base +object name. If the base object is in the same pack, ofs-delta encodes +the offset of the base object in the pack instead.
+
+The base object could also be deltified if it's in the same pack. +Ref-delta can also refer to an object outside the pack (i.e. the +so-called "thin pack"). When stored on disk however, the pack should +be self contained to avoid cyclic dependency.

The delta data is a sequence of instructions to reconstruct an object
-from the base object. Each instruction appends more and more data to -the target object until it's complete. There are two supported -instructions so far: one for copy a byte range from the source object -and one for inserting new data embedded in the instruction itself. +from the base object. If the base object is deltified, it must be +converted to canonical form first. Each instruction appends more and +more data to the target object until it's complete. There are two +supported instructions so far: one for copy a byte range from the +source object and one for inserting new data embedded in the +instruction itself.

Each instruction has variable length. Instruction type is determined by the seventh bit of the first octet. The following diagrams follow the convention in RFC 1951 (Deflate compressed data format).

+==== Instruction to copy from base object
+
+----------+---------+---------+---------+---------+-------+-------+-------+
| 1xxxxxxx | offset1 | offset2 | offset3 | offset4 | size1 | size2 | size3 |
+----------+---------+---------+---------+---------+-------+-------+-------+

This is the instruction format to copy a byte range from the source
-object. It encodes the offset to copy from any the number of bytes to +object. It encodes the offset to copy from and the number of bytes to
copy. Offset and size are in little-endian order.

All offset and size bytes are optional. This is to reduce the
@@ -99,6 +109,8 @@ In its most compact form, this instruction only takes up one byte
values zero. There is another exception: size zero is automatically converted to 0x10000.

+==== Instruction to add new data
+
+----------+============+
| 0xxxxxxx | data |
+----------+============+
@@ -108,6 +120,8 @@ object. The following data is appended to the target object. The first
seven bits of the first octet determines the size of data in
bytes. The size must be non-zero.

+==== Reserved instruction
+
+----------+============
| 00000000 |
+----------+============

 Documentation/technical/pack-format.txt | 92 +++++++++++++++++++++++++
 cache.h                                 |  5 ++
 2 files changed, 97 insertions(+)

index 8e5bf60be3..70a99fd142 100644
@@ -36,6 +36,98 @@ Git pack format
 
   - The trailer records 20-byte SHA-1 checksum of all of the above.
 
+=== Object types
+
+Valid object types are:
+
+- OBJ_COMMIT (1)
+- OBJ_TREE (2)
+- OBJ_BLOB (3)
+- OBJ_TAG (4)
+- OBJ_OFS_DELTA (6)
+- OBJ_REF_DELTA (7)
+
+Type 5 is reserved for future expansion. Type 0 is invalid.
+
+=== Deltified representation
+
+Conceptually there are only four object types: commit, tree, tag and
+blob. However to save space, an object could be stored as a "delta" of
+another "base" object. These representations are assigned new types
+ofs-delta and ref-delta, which is only valid in a pack file.
+
+Both ofs-delta and ref-delta store the "delta" to be applied to
+another object (called 'base object') to reconstruct the object. The
+difference between them is, ref-delta directly encodes 20-byte base
+object name. If the base object is in the same pack, ofs-delta encodes
+the offset of the base object in the pack instead.
+
+The base object could also be deltified if it's in the same pack.
+Ref-delta can also refer to an object outside the pack (i.e. the
+so-called "thin pack"). When stored on disk however, the pack should
+be self contained to avoid cyclic dependency.
+
+The delta data is a sequence of instructions to reconstruct an object
+from the base object. If the base object is deltified, it must be
+converted to canonical form first. Each instruction appends more and
+more data to the target object until it's complete. There are two
+supported instructions so far: one for copy a byte range from the
+source object and one for inserting new data embedded in the
+instruction itself.
+
+Each instruction has variable length. Instruction type is determined
+by the seventh bit of the first octet. The following diagrams follow
+the convention in RFC 1951 (Deflate compressed data format).
+
+==== Instruction to copy from base object
+
+  +----------+---------+---------+---------+---------+-------+-------+-------+
+  | 1xxxxxxx | offset1 | offset2 | offset3 | offset4 | size1 | size2 | size3 |
+  +----------+---------+---------+---------+---------+-------+-------+-------+
+
+This is the instruction format to copy a byte range from the source
+object. It encodes the offset to copy from and the number of bytes to
+copy. Offset and size are in little-endian order.
+
+All offset and size bytes are optional. This is to reduce the
+instruction size when encoding small offsets or sizes. The first seven
+bits in the first octet determines which of the next seven octets is
+present. If bit zero is set, offset1 is present. If bit one is set
+offset2 is present and so on.
+
+Note that a more compact instruction does not change offset and size
+encoding. For example, if only offset2 is omitted like below, offset3
+still contains bits 16-23. It does not become offset2 and contains
+bits 8-15 even if it's right next to offset1.
+
+  +----------+---------+---------+
+  | 10000101 | offset1 | offset3 |
+  +----------+---------+---------+
+
+In its most compact form, this instruction only takes up one byte
+(0x80) with both offset and size omitted, which will have default
+values zero. There is another exception: size zero is automatically
+converted to 0x10000.
+
+==== Instruction to add new data
+
+  +----------+============+
+  | 0xxxxxxx |    data    |
+  +----------+============+
+
+This is the instruction to construct target object without the base
+object. The following data is appended to the target object. The first
+seven bits of the first octet determines the size of data in
+bytes. The size must be non-zero.
+
+==== Reserved instruction
+
+  +----------+============
+  | 00000000 |
+  +----------+============
+
+This is the instruction reserved for future expansion.
+
 == Original (version 1) pack-*.idx files have the following format:
 
   - The header consists of 256 4-byte network byte order

index 77b7acebb6..ad549e258e 100644
@@ -373,6 +373,11 @@ extern void free_name_hash(struct index_state *istate);
 #define read_blob_data_from_cache(path, sz) read_blob_data_from_index(&the_index, (path), (sz))
 #endif
 
+/*
+ * Values in this enum (except those outside the 3 bit range) are part
+ * of pack file format. See Documentation/technical/pack-format.txt
+ * for more information.
+ */
 enum object_type {
 	OBJ_BAD = -1,
 	OBJ_NONE = 0,

2.17.0.705.g3525833791

A subsequent change will make use of this static function in the get_short_oid() function, which is defined above where the collect_ambiguous() function is now. Without this we'd then have a compilation error due to a forward declaration.

---
 sha1-name.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

index cd3b133aae..9d7bbd3e96 100644
@@ -372,6 +372,12 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
 	return 0;
 }
 
+static int collect_ambiguous(const struct object_id *oid, void *data)
+{
+	oid_array_append(data, oid);
+	return 0;
+}
+
 static int get_short_oid(const char *name, int len, struct object_id *oid,
 			  unsigned flags)
 {

@@ -421,12 +427,6 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
 	return status;
 }
 
-static int collect_ambiguous(const struct object_id *oid, void *data)
-{
-	oid_array_append(data, oid);
-	return 0;
-}
-
 int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
 {
 	struct oid_array collect = OID_ARRAY_INIT;

2.17.0.290.gded63e768a

Change the output emitted when an ambiguous object is encountered so that we show tags first, then commits, followed by trees, and finally blobs. Within each type we show objects in hashcmp() order. Before this change the objects were only ordered by hashcmp().

The reason for doing this is that the output looks better as a result, e.g. the v2.17.0 tag before this change on "git show e8f2" would display:

    hint: The candidates are:
    hint:   e8f2093055 tree
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f25a3a50 tree
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2650052 tag v2.17.0
    hint:   e8f2867228 blob
    hint:   e8f28d537c tree
    hint:   e8f2a35526 blob
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2cf6ec0 tree

Now we'll instead show:

    hint:   e8f2650052 tag v2.17.0
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f2867228 blob
    hint:   e8f2a35526 blob

Since we show the commit data in the output that's nicely aligned once we sort by object type. The decision to show tags before commits is pretty arbitrary. I don't want to order by object_type since there tags come last after blobs, which doesn't make sense if we want to show the most important things first.

I could display them after commits, but it's much less likely that we'll display a tag, so if there is one it makes sense to show it prominently at the top.

A note on the implementation: Derrick rightly pointed out[1] that we're bending over backwards here in get_short_oid() to first de-duplicate the list, and then emit it, but could simply do it in one step.

The reason for that is that oid_array_for_each_unique() doesn't actually require that the array be sorted by oid_array_sort(), it just needs to be sorted in some order that guarantees that all objects with the same ID are adjacent to one another, which (barring a hash collision, which'll be someone else's problem) the sort_ambiguous() function does.

I agree that would be simpler for this code, and had forgotten why I initially wrote it like this[2]. But on further reflection I think it's better to do more work here just so we're not underhandedly using the oid-array API where we lie about the list being sorted. That would break any subsequent use of oid_array_lookup() in subtle ways.

I could get around that by hacking the API itself to support this use-case and documenting it, which I did as a WIP patch in [3], but I think it's too much code smell just for this one call site. It's simpler for the API to just introduce a oid_array_for_each() function to eagerly spew out the list without sorting or de-duplication, and then do the de-duplication and sorting in two passes.

1. https://public-inbox.org/git/[email protected]/
2. https://public-inbox.org/git/[email protected]/
3. https://public-inbox.org/git/[email protected]/

---
 Documentation/technical/api-oid-array.txt | 17 +++++++----
 sha1-array.c                              | 17 +++++++++++
 sha1-array.h                              |  3 ++
 sha1-name.c                               | 37 ++++++++++++++++++++++-
 t/t1512-rev-parse-disambiguation.sh       | 21 +++++++++++++
 5 files changed, 88 insertions(+), 7 deletions(-)

index b0c11f868d..94b529722c 100644
@@ -35,13 +35,18 @@ Functions
 	Free all memory associated with the array and return it to the
 	initial, empty state.
 
+`oid_array_for_each`::
+	Iterate over each element of the list, executing the callback
+	function for each one. Does not sort the list, so any custom
+	hash order is retained. If the callback returns a non-zero
+	value, the iteration ends immediately and the callback's
+	return is propagated; otherwise, 0 is returned.
+
 `oid_array_for_each_unique`::
-	Efficiently iterate over each unique element of the list,
-	executing the callback function for each one. If the array is
-	not sorted, this function has the side effect of sorting it. If
-	the callback returns a non-zero value, the iteration ends
-	immediately and the callback's return is propagated; otherwise,
-	0 is returned.
+	Iterate over each unique element of the list in sort order ,
+	but otherwise behaves like `oid_array_for_each`. If the array
+	is not sorted, this function has the side effect of sorting
+	it.
 
 Examples
 --------

index 466a926aa3..265941fbf4 100644
@@ -41,6 +41,23 @@ void oid_array_clear(struct oid_array *array)
 	array->sorted = 0;
 }
 
+
+int oid_array_for_each(struct oid_array *array,
+		       for_each_oid_fn fn,
+		       void *data)
+{
+	int i;
+
+	/* No oid_array_sort() here! See the api-oid-array.txt docs! */
+
+	for (i = 0; i < array->nr; i++) {
+		int ret = fn(array->oid + i, data);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
 int oid_array_for_each_unique(struct oid_array *array,
 			      for_each_oid_fn fn,
 			      void *data)

index 1e1d24b009..232bf95017 100644
@@ -16,6 +16,9 @@ void oid_array_clear(struct oid_array *array);
 
 typedef int (*for_each_oid_fn)(const struct object_id *oid,
 			       void *data);
+int oid_array_for_each(struct oid_array *array,
+		       for_each_oid_fn fn,
+		       void *data);
 int oid_array_for_each_unique(struct oid_array *array,
 			      for_each_oid_fn fn,
 			      void *data);

index 9d7bbd3e96..46d8b1afa6 100644
@@ -378,6 +378,34 @@ static int collect_ambiguous(const struct object_id *oid, void *data)
 	return 0;
 }
 
+static int sort_ambiguous(const void *a, const void *b)
+{
+	int a_type = oid_object_info(a, NULL);
+	int b_type = oid_object_info(b, NULL);
+	int a_type_sort;
+	int b_type_sort;
+
+	/*
+	 * Sorts by hash within the same object type, just as
+	 * oid_array_for_each_unique() would do.
+	 */
+	if (a_type == b_type)
+		return oidcmp(a, b);
+
+	/*
+	 * Between object types show tags, then commits, and finally
+	 * trees and blobs.
+	 *
+	 * The object_type enum is commit, tree, blob, tag, but we
+	 * want tag, commit, tree blob. Cleverly (perhaps too
+	 * cleverly) do that with modulus, since the enum assigns 1 to
+	 * commit, so tag becomes 0.
+	 */
+	a_type_sort = a_type % 4;
+	b_type_sort = b_type % 4;
+	return a_type_sort > b_type_sort ? 1 : -1;
+}
+
 static int get_short_oid(const char *name, int len, struct object_id *oid,
 			  unsigned flags)
 {

@@ -409,6 +437,8 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
 	status = finish_object_disambiguation(&ds, oid);
 
 	if (!quietly && (status == SHORT_NAME_AMBIGUOUS)) {
+		struct oid_array collect = OID_ARRAY_INIT;
+
 		error(_("short SHA1 %s is ambiguous"), ds.hex_pfx);
 
 		/*

@@ -421,7 +451,12 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
 			ds.fn = NULL;
 
 		advise(_("The candidates are:"));
-		for_each_abbrev(ds.hex_pfx, show_ambiguous_object, &ds);
+		for_each_abbrev(ds.hex_pfx, collect_ambiguous, &collect);
+		QSORT(collect.oid, collect.nr, sort_ambiguous);
+
+		if (oid_array_for_each(&collect, show_ambiguous_object, &ds))
+			BUG("show_ambiguous_object shouldn't return non-zero");
+		oid_array_clear(&collect);
 	}
 
 	return status;

index 711704ba5a..2701462041 100755
@@ -361,4 +361,25 @@ test_expect_success 'core.disambiguate does not override context' '
 		git -c core.disambiguate=committish rev-parse $sha1^{tree}
 '
 
+test_expect_success C_LOCALE_OUTPUT 'ambiguous commits are printed by type first, then hash order' '
+	test_must_fail git rev-parse 0000 2>stderr &&
+	grep ^hint: stderr >hints &&
+	grep 0000 hints >objects &&
+	cat >expected <<-\EOF &&
+	tag
+	commit
+	tree
+	blob
+	EOF
+	awk "{print \$3}" <objects >objects.types &&
+	uniq <objects.types >objects.types.uniq &&
+	test_cmp expected objects.types.uniq &&
+	for type in tag commit tree blob
+	do
+		grep $type objects >$type.objects &&
+		sort $type.objects >$type.objects.sorted &&
+		test_cmp $type.objects.sorted $type.objects
+	done
+'
+
 test_done

2.17.0.290.gded63e768a

FWIW, I agree that this gives a big improvement to readability.

-Peff

Add support for ^{tag} to the disambiguation logic. Before this ^{tag} would simply be ignored:

    $ git rev-parse e8f2^{tag}
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f2650052 tag v2.17.0
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f2867228 blob
    hint:   e8f2a35526 blob
    e8f2^{tag}

Now the logic added in ed1ca6025f ("peel_onion: disambiguate to favor tree-ish when we know we want a tree-ish", 2013-03-31) has been extended to support it.

    $ git rev-parse e8f2^{tag}
    e8f2650052f3ff646023725e388ea1112b020e79

---
 cache.h                             |  5 +++--
 sha1-name.c                         | 13 ++++++++++++-
 t/t1512-rev-parse-disambiguation.sh |  5 ++++-
 3 files changed, 19 insertions(+), 4 deletions(-)

index 354903c3ea..a141995cc7 100644
@@ -1330,8 +1330,9 @@ struct object_context {
 #define GET_OID_TREE             010
 #define GET_OID_TREEISH          020
 #define GET_OID_BLOB             040
-#define GET_OID_FOLLOW_SYMLINKS 0100
-#define GET_OID_RECORD_PATH     0200
+#define GET_OID_TAG             0100
+#define GET_OID_FOLLOW_SYMLINKS 0200
+#define GET_OID_RECORD_PATH     0400
 #define GET_OID_ONLY_TO_DIE    04000
 
 #define GET_OID_DISAMBIGUATORS \

index 46d8b1afa6..68d5f65362 100644
@@ -221,6 +221,12 @@ static int finish_object_disambiguation(struct disambiguate_state *ds,
 	return 0;
 }
 
+static int disambiguate_tag_only(const struct object_id *oid, void *cb_data_unused)
+{
+	int kind = oid_object_info(oid, NULL);
+	return kind == OBJ_TAG;
+}
+
 static int disambiguate_commit_only(const struct object_id *oid, void *cb_data_unused)
 {
 	int kind = oid_object_info(oid, NULL);

@@ -288,7 +294,8 @@ int set_disambiguate_hint_config(const char *var, const char *value)
 		{ "committish", disambiguate_committish_only },
 		{ "tree", disambiguate_tree_only },
 		{ "treeish", disambiguate_treeish_only },
-		{ "blob", disambiguate_blob_only }
+		{ "blob", disambiguate_blob_only },
+		{ "tag", disambiguate_tag_only }
 	};
 	int i;
 

@@ -429,6 +436,8 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
 		ds.fn = disambiguate_treeish_only;
 	else if (flags & GET_OID_BLOB)
 		ds.fn = disambiguate_blob_only;
+	else if (flags & GET_OID_TAG)
+		ds.fn = disambiguate_tag_only;
 	else
 		ds.fn = default_disambiguate_hint;
 

@@ -958,6 +967,8 @@ static int peel_onion(const char *name, int len, struct object_id *oid,
 	lookup_flags &= ~GET_OID_DISAMBIGUATORS;
 	if (expected_type == OBJ_COMMIT)
 		lookup_flags |= GET_OID_COMMITTISH;
+	else if (expected_type == OBJ_TAG)
+		lookup_flags |= GET_OID_TAG;
 	else if (expected_type == OBJ_TREE)
 		lookup_flags |= GET_OID_TREEISH;
 

index 2701462041..74e7d9c178 100755
@@ -334,7 +334,10 @@ test_expect_success C_LOCALE_OUTPUT 'ambiguity hints respect type' '
 	test_must_fail git rev-parse 000000000^{commit} 2>stderr &&
 	grep ^hint: stderr >hints &&
 	# 5 commits, 1 tag (which is a commitish), plus intro line
-	test_line_count = 7 hints
+	test_line_count = 7 hints &&
+	git rev-parse 000000000^{tag} >stdout &&
+	test_line_count = 1 stdout &&
+	grep -q ^0000000000f8f stdout
 '
 
 test_expect_success C_LOCALE_OUTPUT 'failed type-selector still shows hint' '

2.17.0.290.gded63e768a

The disambiguation logic had all the pieces necessary to only print out those blobs that were ambiguous, but they hadn't been connected.

The initial logic was added in daba53aeaf ("sha1_name.c: add support for disambiguating other types", 2012-07-02), and when the flags were propagated in 8a10fea49b ("get_sha1: propagate flags to child functions", 2016-09-26) GET_OID_BLOB wasn't added to lookup_flags.

Before this change requests for blobs were simply ignored:

    $ git rev-parse e8f2^{blob}
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f2650052 tag v2.17.0
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f2867228 blob
    hint:   e8f2a35526 blob
    e8f2^{blob}
    [...]

But now we'll do the right thing and only print the blobs:

    $ git rev-parse e8f2^{blob}
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f21d02f7 blob
    hint:   e8f21d577c blob
    hint:   e8f2867228 blob
    hint:   e8f2a35526 blob
    e8f2^{blob}
    [...]

---
 sha1-name.c                         | 2 ++
 t/t1512-rev-parse-disambiguation.sh | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

index 68d5f65362..023f9471a8 100644
@@ -971,6 +971,8 @@ static int peel_onion(const char *name, int len, struct object_id *oid,
 		lookup_flags |= GET_OID_TAG;
 	else if (expected_type == OBJ_TREE)
 		lookup_flags |= GET_OID_TREEISH;
+	else if (expected_type == OBJ_BLOB)
+		lookup_flags |= GET_OID_BLOB;
 
 	if (get_oid_1(name, sp - name - 2, &outer, lookup_flags))
 		return -1;

index 74e7d9c178..9ce9cc3bc3 100755
@@ -337,7 +337,11 @@ test_expect_success C_LOCALE_OUTPUT 'ambiguity hints respect type' '
 	test_line_count = 7 hints &&
 	git rev-parse 000000000^{tag} >stdout &&
 	test_line_count = 1 stdout &&
-	grep -q ^0000000000f8f stdout
+	grep -q ^0000000000f8f stdout &&
+	test_must_fail git rev-parse 000000000^{blob} 2>stderr &&
+	grep ^hint: stderr >hints &&
+	# 5 blobs plus intro line &&
+	test_line_count = 6 hints
 '
 
 test_expect_success C_LOCALE_OUTPUT 'failed type-selector still shows hint' '

2.17.0.290.gded63e768a

After the recent series of patches ^{tag} and ^{blob} now work to get just the tags and blobs, but ^{tree} will still list any tree-ish (commits, tags and trees).

The previous behavior was added in ed1ca6025f ("peel_onion:
disambiguate to favor tree-ish when we know we want a tree-ish", 2013-03-31). I may have missed some special-case but this makes more sense to me.

Now "$sha1:" can be used as before to mean treeish:

    $ git rev-parse e8f2:
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f2650052 tag v2.17.0
    hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached HEAD abbreviated object name
    hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from jeffhostetler/jeffhostetler/register_rename_src
    hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for multiple remote.url entries
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    [...]

But ^{tree} shows just the trees, but would previously be equivalent to the above:

    $ git rev-parse e8f2^{tree}
    error: short SHA1 e8f2 is ambiguous
    hint: The candidates are:
    hint:   e8f2093055 tree
    hint:   e8f25a3a50 tree
    hint:   e8f28d537c tree
    hint:   e8f2cf6ec0 tree
    [...]

---
 sha1-name.c                         |  2 +-
 t/t1512-rev-parse-disambiguation.sh | 18 ++++++++++++++----
 2 files changed, 15 insertions(+), 5 deletions(-)

index 023f9471a8..b61c0558d9 100644
@@ -970,7 +970,7 @@ static int peel_onion(const char *name, int len, struct object_id *oid,
 	else if (expected_type == OBJ_TAG)
 		lookup_flags |= GET_OID_TAG;
 	else if (expected_type == OBJ_TREE)
-		lookup_flags |= GET_OID_TREEISH;
+		lookup_flags |= GET_OID_TREE;
 	else if (expected_type == OBJ_BLOB)
 		lookup_flags |= GET_OID_BLOB;
 

index 9ce9cc3bc3..81076449a2 100755
@@ -159,9 +159,13 @@ test_expect_failure 'two semi-ambiguous commit-ish' '
 	git log 0000000000...
 '
 
-test_expect_failure 'three semi-ambiguous tree-ish' '
+test_expect_success '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
+	test_must_fail git rev-parse --verify 0000000000: &&
+
+	# For ^{tree} we can disambiguate because HEAD and v1.0.0 will
+	# be excluded.
 	git rev-parse --verify 0000000000^{tree}
 '
 

@@ -267,8 +271,12 @@ test_expect_success 'ambiguous commit-ish' '
 # There are three objects with this prefix: a blob, a tree, and a tag. We know
 # the blob will not pass as a treeish, but the tree and tag should (and thus
 # cause an error).
-test_expect_success 'ambiguous tags peel to treeish' '
-	test_must_fail git rev-parse 0000000000f^{tree}
+test_expect_success 'ambiguous tags peel to treeish or tree' '
+	test_must_fail git rev-parse 0000000000f: &&
+	git rev-parse 0000000000f^{tree} >stdout &&
+	test_line_count = 1 stdout &&
+	grep -q ^0000000000fd8bcc56 stdout
+
 '
 
 test_expect_success 'rev-parse --disambiguate' '

@@ -365,7 +373,9 @@ test_expect_success 'core.disambiguate config can prefer types' '
 test_expect_success 'core.disambiguate does not override context' '
 	# treeish ambiguous between tag and tree
 	test_must_fail \
-		git -c core.disambiguate=committish rev-parse $sha1^{tree}
+		git -c core.disambiguate=committish rev-parse $sha1: &&
+	# tree not ambiguous between tag and tree
+	git -c core.disambiguate=committish rev-parse $sha1^{tree}
 '
 
 test_expect_success C_LOCALE_OUTPUT 'ambiguous commits are printed by type first, then hash order' '

2.17.0.290.gded63e768a

When a user says "git $cmd e8f2^{tree}", the user is telling Git that s/he knows e8f2 *is* a tree-ish, but for whatever reason $cmd wants a tree and does not accept an arbitrary tree-ish---that is the whole piont of appending ^{tree} as a suffix. A useful hint in such a case would be "oh, you said e8f2 is a tree-ish, but there are more than one tree-ish, so let me show them to you to help you decide which one among them is the one you meant". When $cmd is rev-parse, I would even say that the user is saying "I know e8f2 is a tree-ish, and I know it not a tree--it merely is a tree-ish. I want the tree that e8f2 thing points at".

Limiting that hint to show only real trees does not make any sense to me. I do not think we care _too_ deeply, because most of the time, command line location that expects a tree-ish can be given a tree-ish, so there is not much reason to use ^{tree} suffix these days. But in a case where it _does_ matter, I think this change makes the "hint" almost useless.

Or am I misleading what you wanted to achieve with this patch?

The reason I'm doing this is because I found it confusing that I can't do:

for t in tag commit tree blob; do ./git --exec-path=$PWD rev-parse 7452^{$t}; done

And get, respectively, only the SHAs that match the respective type, but currently (with released git) you can do:

for t in tag commit committish treeish tree blob; do git -c core.disambiguate=$t rev-parse 7452; done

And while =tag doesn't work the others do (inluding =blob), so core.disambiguate=tree gives you just trees, but ^{tree} gives you treeish.

Why should ^{tree} be giving me ^{treeish} but =tree be giving me trees, and =treeish be synonymous with ^{tree}?

There's no other cases I know of where the ^{<type>} peel syntax won't give you *only* the <type> you asked for. See peel_onion() -> peel_to_type() and how get_oid_1() will short-circuit if it has an answer, and then finally fall back to this get_short_oid() codepath.

Looking at the code & git log maybe it'll do that internally, but when you peel a tag or commit ^{tree} will only ever find one thing, unlike this disambiguation case where we can match multiple things.

So:

1) Am I missing some subtlety or am I correct that there was no way to get git to return more than one SHA-1 for ^{commit} or ^{tree} before this disambiguation feature was added?

2) I think the behavior I've implemented is consistent with how the peel syntax has been documented in revisions.txt:

'<rev>{caret}{<type>}', e.g. 'v0.99.8{caret}\{commit\}'::
A suffix '{caret}' followed by an object type name enclosed in brace pair means dereference the object at '<rev>' recursively until an object of type '<type>' is found or the object cannot be dereferenced anymore (in which case, barf).
For example, if '<rev>' is a commit-ish, '<rev>{caret}\{commit\}' describes the corresponding commit object.
Similarly, if '<rev>' is a tree-ish, '<rev>{caret}\{tree\}' describes the corresponding tree object.
'<rev>{caret}0'
is a short-hand for '<rev>{caret}\{commit\}'.

Note "until an object of type '<type>' is found". I.e. my mental model of this has been that yes you can *start* the search at a different object (e.g. tag -> tree), but it'll only ever return the tree. The disambiguation implementation has been inconsistent with this as documented, because it hasn't been drilling don to an object of '<type>' given a request like $short^{<type>}, but rather returning something matching $short because it could be a container for <type>.

Anyway, I'm not saying I'm right. This is the first time I really look at sha1-name.c in any detail. But the above describes the thought process behind this patch.

Exactly. The former asks "I (think I) know 7452 can be used to name an object of type $t, with peeling if necessary--give me the underlying object of type $t". In short, the fact that you can write "$X^{$t}" says that $X is a $t-ish (otherwise $X cannot be used as a stand-in for an object of type $t) and that you fully expect that $X can merely be of type $t-ish and not exactly $t (otherwise you wouldn't be making sure to coerce $X into $t with ^{$t} notation).

In *THAT* context, disambiguation help that lists objects whose name begins with "7452" you gave, hoping that it is a unique enough prefix when it wasn't in reality, *MUST* give $t-ish; restricting it to $t makes the help mostly useless.

There is no such feature either before or after the disambiguation help. I am not saying there shouldn't exist such a feature. I am saying that breaking the existing feature and making it useless is not the way to add such a feature.

Right, and I'm with you so far, this makes sense to me for all existing uses of the peel syntax, otherwise v2.17.0^{tree} wouldn't be the same as rev-parse v2.17.0^{tree}^{tree}...

I still don't get how what you're proposing is going to be consistent, but let's fully enumerate the output of 7452 with my patch to take that case-by-case[1]:

    ^{tag}:
    7452b4b5786778d5d87f5c90a94fab8936502e20
    ^{commit}:
    hint:   74521eee4c commit 2007-12-01 - git-gui: install-sh from automake does not like -m755
    hint:   745224e04a commit 2014-06-18 - refs.c: SSE2 optimizations for check_refname_component
    ^{tree}:
    hint:   7452336aa3 tree
    hint:   74524f384d tree
    hint:   7452813bcd tree
    hint:   7452b1a701 tree
    hint:   7452b73c42 tree
    hint:   7452ca1557 tree
    ^{blob}:
    hint:   7452001351 blob
    hint:   745254665d blob
    hint:   7452a572c1 blob
    hint:   7452b9fd21 blob
    hint:   7452db13c8 blob
    hint:   7452fce0da blob

And[2]:

    core.disambiguate=tag:
    [same as ^{tag]
    core.disambiguate=commit:
    [same as ^{commit}]
    core.disambiguate=committish:
    hint:   7452b4b578 tag v2.1.0
    hint:   74521eee4c commit 2007-12-01 - git-gui: install-sh from automake does not like -m755
    hint:   745224e04a commit 2014-06-18 - refs.c: SSE2 optimizations for check_refname_component
    core.disambiguate=tree:
    [same as ^{tree}]
    core.disambiguate=treeish (same as $sha1:)
    hint:   7452b4b578 tag v2.1.0
    hint:   74521eee4c commit 2007-12-01 - git-gui: install-sh from automake does not like -m755
    hint:   745224e04a commit 2014-06-18 - refs.c: SSE2 optimizations for check_refname_component
    hint:   7452336aa3 tree
    hint:   74524f384d tree
    hint:   7452813bcd tree
    hint:   7452b1a701 tree
    hint:   7452b73c42 tree
    hint:   7452ca1557 tree
    core.disambiguate=blob:
    [same as ^{blob}]

So from my understanding of what you're saying you'd like to list tag, commits and trees given $sha1^{tree}, because they're all types that can be used to reach a tree.

I don't think that's very useful, yes it would "break" existing disambiguations, but this is such an obscure (and purely manual) use-case than I think that's fine.

Because I think to the extent anyone's going to use this it's because they know they have e.g. a short blob, commit etc. SHA-1 they're not going to use it because they have some short $SHA they know is a tree, and then want all SHA-1s of that *and* random tag & commit objects that happen to have the same object prefix just because tags and commits can also point to trees.

How does that make any sense? The entire reason for using the normal peel syntax is because you e.g. have v2.17.0 and want to get to the ^{tree} or the ^{commit} tht v2.17.0 directly points to. That's entirely orthogonal to what the disambiguation is doing. There with your proposed semantics you're peeling 7452 as 7452^{tree} because (IMO) you're looking for trees, just to get some entirely unrelated commits and tags.

But *leaving that aside*, i.e. I don't see why the use-case would make sense. What I *don't* get is why, if you think that, you only want to apply that rule to ^{tree}. I.e. wouldn't it then be consistent to say:

# a)
^{tag} = tag
^{commit} = tag, commit
^{tree} = tag, commit, tree
^{blob} = tag, blob (blobish)

Whereas my patch now does:

# b)
^{tag} = tag
^{commit} = commit
^{tree} = tree
^{blob} = blob

But from what you seem to be proposing (or maybe you just didn't have a chance to critique the ^{blob} and ^{commit} patches):

# c)
^{tag} = tag
^{commit} = commit
^{tree} = tag, tree, commit
^{blob} = blob

1. for type in tag commit tree blob; do echo "^{$type}:" && ./git --exec-path=$PWD rev-parse 7452^{$type} 2>&1|grep -E -e ^hint -e '^[0-9a-f]{40}$' |grep -v are:; done
2. for cfg in tag commit committish tree treeish blob; do echo "core.disambiguate=$cfg:" && ./git --exec-path=$PWD -c core.disambiguate=$cfg rev-parse 7452 2>&1|grep -E -e ^hint -e '^[0-9a-f]{40}$' |grep -v are:; done

More importantly, you could spell v2.17.0 part of the above with a short hexadecimal string. And that string should be naming some tree-ish, the most important thing being that it is *NOT* required to be a tree (and practically, it is likely that the user has a tree-ish that is *NOT* a tree).

I guess I have a reaction to the title

"get_short_oid/peel_onion: ^{tree} should be tree"

"X^{tree}" should *RESULT* in a tree, but it should *REQUIRE* X to be a tree-ish. It is unclear "should be tree" is about the former and I read (perhaps mis-read) it as saying "it should require X to be a tree"---that statement is utterly incorrect as we agreed above.

I take it as "git rev-parse 7452^{tag}" output (similarly ^{$type} for the rest)? That probably is desirable, as blobs, trees and commits cannot be peeled down to a tag.

If 7452 points at a commit, that tag itself should also be given as a possible object the user may have meant in the "hint" thing. I agree it is a good idea to exclude trees and blobs from the hint, for the same reason why I think it makes sense to exclude blobs, trees and commits from hints for a X in "X^{tag}" above.

Again, if there is a commit or a tag (that points at a commit or a tree) whose name begins with 7452, it should be included in the hint above. Not having blobs in the hint of course makes sense, as a blob cannot be X in "X^{tree}".

When core.disambiguate tells us to "interprete hexadecimal literals to name commit objects only", giving only commits in hints: section makes sense, because we are explicitly saying that "when I say 7452, I do not mean any tag whose name begins with 7452", so "sorry, your request is not explicit enough---there are two commits and a tag that begin with that prefix" is not helpful---it should stop at "you may have meant one of these two commits" and not mention any tag.

FWIW, I had the same feeling as you when reading this, that this commit (and the one after) are doing the wrong thing. And these paragraphs sum it up. The "^{tree}" is about asking us to peel to a tree, not about resolving X in the first place. We can use it as a _hint_ when resolving X, but the correct hint is "something that can be peeled to a tree", not "is definitely a tree".

-Peff

Maybe I'm just being dense, but I still don't get from this & Junio's E-Mails what the general rule should be.

I think a response to the part after "leaving that aside" of my upthread E-Mail
(https://public-inbox.org/git/[email protected]/) would help me out.

Not to belabor the point, but here's a patch I came up with to revisions.txt that's a WIP version of something that would describe the worldview after this v3:

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index dfcc49c72c..0bf68f4ad2 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -143,29 +143,52 @@ thing no matter the case.
'<rev>{caret}1{caret}1{caret}1'. See below for an illustration of the usage of this form.

'<rev>{caret}{<type>}', e.g. 'v0.99.8{caret}\{commit\}'::
A suffix '{caret}' followed by an object type name enclosed in brace pair means dereference the object at '<rev>' recursively until an object of type '<type>' is found or the object cannot be
- dereferenced anymore (in which case, barf).
+ dereferenced anymore (in which case either return that object type, or barf). For example, if '<rev>' is a commit-ish, '<rev>{caret}\{commit\}' describes the corresponding commit object.
Similarly, if '<rev>' is a tree-ish, '<rev>{caret}\{tree\}' describes the corresponding tree object.
'<rev>{caret}0'
is a short-hand for '<rev>{caret}\{commit\}'.
+
'rev{caret}\{object\}' can be used to make sure 'rev' names an object that exists, without requiring 'rev' to be a tag, and without dereferencing 'rev'; because a tag is already an object, it does not have to be dereferenced even once to get to an object. +
'rev{caret}\{tag\}' can be used to ensure that 'rev' identifies an existing tag object.
++
+Object types that don't have a 1=1 mapping to other object types +cannot be dereferenced with the peel syntax, and will return an +error. E.g. '<treeid>{caret}{commit}' or '<treeid>{caret}{tree}' is +allowed because a tag can only point to one commit, and a commit can +only point to one tree. But '<treeid>{caret}{blob}' will always +produce an error since trees in general don't 1=1 map to blobs, even +though the specific '<treeid>' in question might only contain one +blob. Note that '<tagid>{caret}{blob}' is not an error if '<tagid>' is +a tag that points directly to a blob, since that again becomes +unambiguous.
++
+'<rev>{caret}{<type>}' takes on a different meaning when '<rev>' is a +SHA-1 that's ambiguous within the object store. In that case we don't +have a 1=1 mapping anymore. E.g. e8f2 in git.git can refer to multiple +objects of all the different object types. In that case +{caret}{<type>} should always be an error to be consistent with the +logic above, but that wouldn't be useful to anybody. Instead it'll +fall back to being selector syntax for the given object types, +e.g. e8f2{caret}{tag} will (as of writing this) return the v2.17.0 +tag, and {caret}{commit}, {caret}{tree} and {caret}{blob} will return +commit, tree and blob objects, respectively.
+
[...]

My understanding of what you two are saying is that somehow the peel semantics should be preserved when we take this beyond the 1=1 mapping case, but I don't see how if we run with that how we wouldn't need to introduce the concept of blobish for consistency as I noted upthread.

So it would be very useful to me if you or someone who understands the behavior you & Junio seem to want could write a version of the patch I have above where the last paragraph is different, and describes the desired semantics, because I still don't get it. Why would we 1=many peel commits to trees as a special case, but not 1=many do the same for trees & blobs?

Let me try to lay out my thinking a bit more clearly, and then I'll try to respond to the points you laid out below.

Before we had any disambiguation code, resolving X^{tree} really was two independent steps: resolve X, and then peel it to a tree. When we added the disambiguation code, the goal was to provide a hint to the first step in such a way that we could never eliminate any resolutions that the user _might_ have meant. But it's OK to take a situation where every case but one would result in an error, and assume the user meant that case. Sort of a "do no harm" rule.

By disambiguating with just a tree and not a tree-ish, that hint is now eliminating possibilities that would have worked in the second step, which violates the rule.

Does thinking about it that way make more sense?

I'll quote that bit here:

Yes, that makes sense to me conceptually, and would follow the rule I gave above. And I think that's what we do now, with the exception that there is no blobish disambiguation. Presumably nobody ever bothered because probably because tagged blobs are pretty rare (and obviously though trees point to blobs, you cannot disambiguate that way since there's no one-to-one correspondence).

So I doubt anybody really cares in practice, but I agree that it would improve consistency to write a patch to introduce GET_OID_BLOBISH and have "^{blob}" parsing use it. And possibly add "blobish" to core.disambiguate (or is it "blobbish"?), though that's almost certainly something nobody would ever use.

Yeah, I think the lack of blobish is a bug, just one that nobody has ever really cared about.

I'm not sure I understand the mention of trees in the final sentence. AFAICT tree disambiguation is consistent with the peeling rules.

-Peff

Okey, so to rephrase that to make sure I understand it. It would be documented as something like this:

When the short SHA-1 X is ambiguous X^{<type>} doesn't mean do the peel itself in X any way, rather it means list all those objects matching X where a subsequent X^{<type>} wouldn't be an error.

I.e. X^{commit} will list tags and commits, since both can be peeled to reveal a commit, X^{tree} will similarly list tags, commits and trees, and X^{blob} will list tags and blobs[1], and X^{tag} will only list tags.

But core.disambiguate=[tag|commit|tree|blob] is not at all like ^{[tag|commit|tree|blob]} and is unlike the peel syntax only going to list the objects of the respective type. The config synonym for the peel syntax is committish, treeish, and the nonexistent blobish.

Yeah, I'll introduce it for consistency. To clarify I wasn't trying to make some argument on the basis that we didn't have it, but I was confused because I couldn't see how the general rule would apply to ^{tree} and not ^{blob}.

Yeah nevermind that, I was imagining some semantics where because we dropped the 1=1 mapping ^{tree} would list blobs, but in the worldview you describe above (if I got it right) that doesn't make sense.

1. Not currently, but I should amend my ^{blob} patch to work like that.