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
