From 7243ffdd78d56738e568abb544eba00e8079f329 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 26 Sep 2016 07:59:15 -0400 Subject: get_sha1: avoid repeating ourselves via ONLY_TO_DIE When the revision code cannot parse an argument like "HEAD:foo", it will call maybe_die_on_misspelt_object_name(), which re-runs get_sha1() with an extra ONLY_TO_DIE flag. We then spend more effort to generate a better error message. Unfortunately, a side effect is that our second call may repeat the same error messages from the original get_sha1() call. You can see this with: $ git show 0017 error: short SHA1 0017 is ambiguous. error: short SHA1 0017 is ambiguous. fatal: ambiguous argument '0017': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git [...] -- [...]' where the second "error:" line comes from the ONLY_TO_DIE call. To fix this, we can make ONLY_TO_DIE imply QUIETLY. This is a little odd, because the whole point of ONLY_TO_DIE is to output error messages. But what we want to do is tell the rest of the get_sha1() code (particularly get_sha1_1()) that the _regular_ messages should be quiet, but the only-to-die ones should not. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1512-rev-parse-disambiguation.sh | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 't') diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh index e221167cfb..16f970982b 100755 --- a/t/t1512-rev-parse-disambiguation.sh +++ b/t/t1512-rev-parse-disambiguation.sh @@ -291,4 +291,10 @@ test_expect_success 'ambiguous short sha1 ref' ' grep "refname.*${REF}.*ambiguous" err ' +test_expect_success C_LOCALE_OUTPUT 'ambiguity errors are not repeated' ' + test_must_fail git rev-parse 00000 2>stderr && + grep "is ambiguous" stderr >errors && + test_line_count = 1 errors +' + test_done -- cgit v1.2.3 From 8a10fea49b1f95daeea445072a60139590b216cf Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 26 Sep 2016 07:59:41 -0400 Subject: get_sha1: propagate flags to child functions The get_sha1() function is actually implementation by many sub-functions, but we do not always pass our flags around to all of those functions. As a result, we may forget that our caller asked us to resolve with GET_SHA1_QUIETLY and output messages. The two triggerable cases are: 1. Resolving treeish:path will resolve the "treeish" portion using GET_SHA1_TREEISH, dropping all other flags. 2. The peel_onion() function did not take flags at all but recurses to get_sha1_1(), which does. The solution for both is to bitwise-OR their new flags with the existing ones (after dropping any mutually exclusive disambiguation flags). This bug can trigger with "git rev-parse --quiet", which asks for quiet resolution. But it can also happen in a more vanilla code path when we do a follow-up ONLY_TO_DIE invocation of get_sha1(), and that's what the tests check. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1512-rev-parse-disambiguation.sh | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) (limited to 't') diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh index 16f970982b..30e0b80f05 100755 --- a/t/t1512-rev-parse-disambiguation.sh +++ b/t/t1512-rev-parse-disambiguation.sh @@ -291,10 +291,22 @@ test_expect_success 'ambiguous short sha1 ref' ' grep "refname.*${REF}.*ambiguous" err ' -test_expect_success C_LOCALE_OUTPUT 'ambiguity errors are not repeated' ' +test_expect_success C_LOCALE_OUTPUT 'ambiguity errors are not repeated (raw)' ' test_must_fail git rev-parse 00000 2>stderr && grep "is ambiguous" stderr >errors && test_line_count = 1 errors ' +test_expect_success C_LOCALE_OUTPUT 'ambiguity errors are not repeated (treeish)' ' + test_must_fail git rev-parse 00000:foo 2>stderr && + grep "is ambiguous" stderr >errors && + test_line_count = 1 errors +' + +test_expect_success C_LOCALE_OUTPUT 'ambiguity errors are not repeated (peel)' ' + test_must_fail git rev-parse 00000^{commit} 2>stderr && + grep "is ambiguous" stderr >errors && + test_line_count = 1 errors +' + test_done -- cgit v1.2.3 From 5d5def2aa56dc9b6d4eccfb333fd08aa713733e7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 26 Sep 2016 07:59:48 -0400 Subject: get_short_sha1: parse tags when looking for treeish The treeish disambiguation function tries to peel tags, but it does so by calling: deref_tag(lookup_object(sha1), ...); This will only work if we have previously looked at the tag and created a "struct tag" for it. Since parsing revision arguments typically happens before anything else, this is usually not the case, and we would fail to peel the tag (we are lucky that deref_tag() gracefully handles the NULL and does not segfault). Instead, we can use parse_object(). Note that this is the same fix done by 94d75d1 (get_short_sha1(): correctly disambiguate type-limited abbreviation, 2013-07-01), but that commit fixed only the committish disambiguator, and left the bug in the treeish one. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1512-rev-parse-disambiguation.sh | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 't') diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh index 30e0b80f05..dfd356721e 100755 --- a/t/t1512-rev-parse-disambiguation.sh +++ b/t/t1512-rev-parse-disambiguation.sh @@ -264,6 +264,13 @@ test_expect_success 'ambiguous commit-ish' ' test_must_fail git log 000000000... ' +# 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 'rev-parse --disambiguate' ' # The test creates 16 objects that share the prefix and two # commits created by commit-tree in earlier tests share a -- cgit v1.2.3 From 16ddcd403bdd74f47f3ae1a7e58a01e36e54a7d7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 26 Sep 2016 08:00:29 -0400 Subject: sha1_array: let callbacks interrupt iteration The callbacks for iterating a sha1_array must have a void return. This is unlike our usual for_each semantics, where a callback may interrupt iteration and have its value propagated. Let's switch it to the usual form, which will enable its use in more places (e.g., where we are replacing an existing iteration with a different data structure). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/helper/test-sha1-array.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 't') diff --git a/t/helper/test-sha1-array.c b/t/helper/test-sha1-array.c index 09f7790971..f7a53c4ad6 100644 --- a/t/helper/test-sha1-array.c +++ b/t/helper/test-sha1-array.c @@ -1,9 +1,10 @@ #include "cache.h" #include "sha1-array.h" -static void print_sha1(const unsigned char sha1[20], void *data) +static int print_sha1(const unsigned char sha1[20], void *data) { puts(sha1_to_hex(sha1)); + return 0; } int cmd_main(int argc, const char **argv) -- cgit v1.2.3 From fad6b9e5905f00654f394cac4093a052b7a3cfb6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 26 Sep 2016 08:00:33 -0400 Subject: for_each_abbrev: drop duplicate objects If an object appears multiple times in the object database (e.g., in both loose and packed form, or in two separate packs), the disambiguation machinery may see it more than once. The get_short_sha1() function handles this already, but for_each_abbrev() blindly fires the callback for each instance it finds. We can fix this by collecting the output in a sha1 array and de-duplicating it. As a bonus, the sort done for the de-duplication means that our output will be stable, regardless of the order in which the objects are found. Note that the old code normalized the callback's output to 0/1 to store in the 1-bit ds->ambiguous flag (which both halted the iteration and was returned from the for_each_abbrev function). Now that we are using sha1_array, we can return the real value. In practice, it doesn't matter as the sole caller only ever returns 0. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1512-rev-parse-disambiguation.sh | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 't') diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh index dfd356721e..1d8f550996 100755 --- a/t/t1512-rev-parse-disambiguation.sh +++ b/t/t1512-rev-parse-disambiguation.sh @@ -280,6 +280,13 @@ test_expect_success 'rev-parse --disambiguate' ' test "$(sed -e "s/^\(.........\).*/\1/" actual | sort -u)" = 000000000 ' +test_expect_success 'rev-parse --disambiguate drops duplicates' ' + git rev-parse --disambiguate=000000000 >expect && + git pack-objects .git/objects/pack/pack actual && + test_cmp expect actual +' + test_expect_success 'ambiguous 40-hex ref' ' TREE=$(git mktree Date: Mon, 26 Sep 2016 08:00:36 -0400 Subject: get_short_sha1: list ambiguous objects on error When the user gives us an ambiguous short sha1, we print an error and refuse to resolve it. In some cases, the next step is for them to feed us more characters (e.g., if they were retyping or cut-and-pasting from a full sha1). But in other cases, that might be all they have. For example, an old commit message may have used a 7-character hex that was unique at the time, but is now ambiguous. Git doesn't provide any information about the ambiguous objects it found, so it's hard for the user to find out which one they probably meant. This patch teaches get_short_sha1() to list the sha1s of the objects it found, along with a few bits of information that may help the user decide which one they meant. Here's what it looks like on git.git: $ git rev-parse b2e1 error: short SHA1 b2e1 is ambiguous hint: The candidates are: hint: b2e1196 tag v2.8.0-rc1 hint: b2e11d1 tree hint: b2e1632 commit 2007-11-14 - Merge branch 'bs/maint-commit-options' hint: b2e1759 blob hint: b2e18954 blob hint: b2e1895c blob fatal: ambiguous argument 'b2e1': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git [...] -- [...]' We show the tagname for tags, and the date and subject for commits. For trees and blobs, in theory we could dig in the history to find the paths at which they were present. But that's very expensive (on the order of 30s for the kernel), and it's not likely to be all that helpful. Most short references are to commits, so the useful information is typically going to be that the object in question _isn't_ a commit. So it's silly to spend a lot of CPU preemptively digging up the path; the user can do it themselves if they really need to. And of course it's somewhat ironic that we abbreviate the sha1s in the disambiguation hint. But full sha1s would cause annoying line wrapping for the commit lines, and presumably the user is going to just re-issue their command immediately with the corrected sha1. We also restrict the list to those that match any disambiguation hint. E.g.: $ git rev-parse b2e1:foo error: short SHA1 b2e1 is ambiguous hint: The candidates are: hint: b2e1196 tag v2.8.0-rc1 hint: b2e11d1 tree hint: b2e1632 commit 2007-11-14 - Merge branch 'bs/maint-commit-options' fatal: Invalid object name 'b2e1'. does not bother reporting the blobs, because they cannot work as a treeish. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1512-rev-parse-disambiguation.sh | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) (limited to 't') diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh index 1d8f550996..c5447ef877 100755 --- a/t/t1512-rev-parse-disambiguation.sh +++ b/t/t1512-rev-parse-disambiguation.sh @@ -323,4 +323,28 @@ test_expect_success C_LOCALE_OUTPUT 'ambiguity errors are not repeated (peel)' ' test_line_count = 1 errors ' +test_expect_success C_LOCALE_OUTPUT 'ambiguity hints' ' + test_must_fail git rev-parse 000000000 2>stderr && + grep ^hint: stderr >hints && + # 16 candidates, plus one intro line + test_line_count = 17 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 +' + +test_expect_success C_LOCALE_OUTPUT 'failed type-selector still shows hint' ' + # these two blobs share the same prefix "ee3d", but neither + # will pass for a commit + echo 851 | git hash-object --stdin -w && + 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_done -- cgit v1.2.3 From 5b33cb1fd733f581da07ae8afa7e9547eafd248e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 27 Sep 2016 08:38:01 -0400 Subject: get_short_sha1: make default disambiguation configurable When we find ambiguous short sha1s, we may get a disambiguation rule from our caller's context. But if we don't, we fall back to treating all sha1s the same, even though most projects will tend to refer only to commits by their short sha1s. This patch introduces a configuration option that lets the user pick a different fallback (e.g., only commits). It's possible that we may want to make this the default, but it's a good idea to start as a config option for two reasons: 1. It lets people experiment with this and see if it's a good idea (i.e., the "tend to" above is an assumption; we don't really know if this will break some obscure cases). 2. Even if we do flip the default, it gives people an escape hatch if it causes problems (you can sometimes override it by asking for "1234^{tree}", but not all combinations are possible). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1512-rev-parse-disambiguation.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 't') diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh index c5447ef877..7c659eb585 100755 --- a/t/t1512-rev-parse-disambiguation.sh +++ b/t/t1512-rev-parse-disambiguation.sh @@ -347,4 +347,18 @@ test_expect_success C_LOCALE_OUTPUT 'failed type-selector still shows hint' ' test_line_count = 3 hints ' +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} && + git -c core.disambiguate=committish rev-parse $sha1 +' + +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} +' + test_done -- cgit v1.2.3