Message ID | 20240813124550.GC968816@coredump.intra.peff.net (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | get_oid(): enforce minimum length for "-g<hex>" names | expand |
On Tue, Aug 13, 2024 at 08:45:50AM -0400, Jeff King wrote: > 3. Looking at the loop in get_describe_name(), we read from the back > end and check for "-g" when we see a non-hex digit. But if it's not > "-g", we keep looking! So for a name like "foo-g1234abc-bar", we'll > still pass "1234abc-bar" to get_short_oid()! This is OK in > practice, since it will barf when seeing the non-hex digits. But > let's confirm that it does so. This is particularly important > with our length checks, since "foo-gcc14-bar" would yield a length > of 8, which is plausibly long (so we are likewise depending on > get_short_oid() to reject it). So I think the current code is working as we'd want, and there's no correctness issue. I do think breaking out of the loop early would be more clear (and would provide a tiny speedup). Like: diff --git a/object-name.c b/object-name.c index 6507a30ace..89de9db8e9 100644 --- a/object-name.c +++ b/object-name.c @@ -1263,35 +1263,36 @@ static int peel_onion(struct repository *r, const char *name, int len, static int get_describe_name(struct repository *r, const char *name, int len, struct object_id *oid) { const char *cp; unsigned flags = GET_OID_QUIETLY | GET_OID_COMMIT; for (cp = name + len - 1; name + 2 <= cp; cp--) { char ch = *cp; if (!isxdigit(ch)) { /* We must be looking at g in "SOMETHING-g" * for it to be describe output. */ if (ch == 'g' && cp[-1] == '-') { /* * To reduce the chance of false positives, * assume that any "-g<hex>" must have some * minimum number of <hex> that matches what * we'd produce when abbreviating. */ int min_len = default_abbrev; if (min_len < 0) min_len = FALLBACK_DEFAULT_ABBREV; cp++; len -= cp - name; if (len < min_len) return -1; return get_short_oid(r, cp, len, oid, flags); } + break; } } return -1; } But I don't know that it matters much in practice. -Peff
diff --git a/object-name.c b/object-name.c index 527b853ac4..6507a30ace 100644 --- a/object-name.c +++ b/object-name.c @@ -1274,8 +1274,20 @@ static int get_describe_name(struct repository *r, * for it to be describe output. */ if (ch == 'g' && cp[-1] == '-') { + /* + * To reduce the chance of false positives, + * assume that any "-g<hex>" must have some + * minimum number of <hex> that matches what + * we'd produce when abbreviating. + */ + int min_len = default_abbrev; + if (min_len < 0) + min_len = FALLBACK_DEFAULT_ABBREV; + cp++; len -= cp - name; + if (len < min_len) + return -1; return get_short_oid(r, cp, len, oid, flags); } diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index 79e0f19deb..790afe40ac 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -707,4 +707,37 @@ test_expect_success 'describe --broken --dirty with a file with changed stat' ' ) ' +test_expect_success 'long describe name can be resolved' ' + name=$(git describe --long A) && + git rev-parse "A^{commit}" >expect && + git rev-parse "$name" >actual && + test_cmp expect actual +' + +test_expect_success 'resolving describe name does not depend on tag' ' + hash=$(git rev-parse A^{commit}) && + abbrev=$(echo $hash | cut -c1-30) && + echo "$hash" >expect && + git rev-parse "does-not-exist-g$abbrev" >actual && + test_cmp expect actual +' + +test_expect_success 'resolving describe name only valid at end' ' + hash=$(git rev-parse A^{commit}) && + abbrev=$(echo $hash | cut -c1-30) && + test_must_fail git rev-parse "foo-g$abbrev-bar" +' + +test_expect_success 'resolving describe name requires minimum abbrev (auto)' ' + hash=$(git rev-parse A^{commit}) && + abbrev=$(echo $hash | cut -c1-6) && + test_must_fail git -c core.abbrev=auto rev-parse "foo-g$abbrev" +' + +test_expect_success 'resolving describe name requires minimum abbrev (config)' ' + hash=$(git rev-parse A^{commit}) && + abbrev=$(echo $hash | cut -c1-20) && + test_must_fail git -c core.abbrev=25 rev-parse "foo-g$abbrev" +' + test_done