{"thread":{"id":"61902","subject":"Bug: Git sees branch as valid commit ref and works; should fail","startedAt":"2024-08-05T15:08:12Z","lastAt":"2024-08-13T15:41:28Z","messageCount":7,"participants":["Matt Thompson","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"500084","messageId":"CAFb48S8LDz4kiWsKSCBn8J=AHyQ5SVPFH4GY=z+8=DntT=PyAw@mail.gmail.com","threadId":"61902","inReplyTo":null,"subject":"Bug: Git sees branch as valid commit ref and works; should fail","fromName":"Matt Thompson","fromEmail":"fortran@gmail.com","sentAt":"2024-08-05T15:07:41Z","receivedAt":"2024-08-05T15:08:12Z","isPatch":false,"sender":{"key":"fortran@gmail.com","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\n$ git clone https://github.com/GEOS-ESM/GFDL_atmos_cubed_sphere.git fvdycore\n$ cd fvdycore\n$ git checkout bugfix/mathomp4/trivial-ci-commit-gcc14\n\nWhat did you expect to happen? (Expected behavior)\n\nI expected a failure as there is no branch named\n'bugfix/mathomp4/trivial-ci-commit-gcc14' in the repository.\n\nWhat happened instead? (Actual behavior)\n\n$ git checkout bugfix/mathomp4/trivial-ci-commit-gcc14\nNote: switching to 'bugfix/mathomp4/trivial-ci-commit-gcc14'.\n\nYou are in 'detached HEAD' state. You can look around, make experimental\nchanges and commit them, and you can discard any commits you make in this\nstate without impacting any branches by switching back to a branch.\n\nIf you want to create a new branch to retain commits you create, you may\ndo so (now or later) by using -c with the switch command. Example:\n\n  git switch -c <new-branch-name>\n\nOr undo this operation with:\n\n  git switch -\n\nTurn off this advice by setting config variable advice.detachedHead to false\n\nHEAD is now at cc14d30 removed an unused module use in fv_sg\n\nWhat's different between what you expected and what actually happened?\n\nWell, git somehow saw 'bugfix/mathomp4/trivial-ci-commit-gcc14' as a\nvalid commit reference and checked it out. I was expecting it\nto...not.\n\nAnything else you want to add:\n\nYou can see similar behavior with git-switch, though slightly different:\n\n$ git switch bugfix/mathomp4/trivial-ci-commit-gcc14\nfatal: a branch is expected, got commit\n'bugfix/mathomp4/trivial-ci-commit-gcc14'\nhint: If you want to detach HEAD at the commit, try again with the\n--detach option.\n$ git switch --detach bugfix/mathomp4/trivial-ci-commit-gcc14\nHEAD is now at cc14d30 removed an unused module use in fv_sg\n\nSo even git-switch sees 'bugfix/mathomp4/trivial-ci-commit-gcc14' as a\ncommit reference.\n\nAlso, I have duplicated this behavior on a SLES 15 Linux machine with\ngit verison 2.45.0 and on another RHEL8 box with 2.45.2\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.46.0\ncpu: arm64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nfeature: fsmonitor--daemon\nlibcurl: 8.4.0\nzlib: 1.2.12\nuname: Darwin 23.6.0 Darwin Kernel Version 23.6.0: Fri Jul  5 17:53:24\nPDT 2024; root:xnu-10063.141.1~2/RELEASE_ARM64_T6020 arm64\ncompiler info: clang: 15.0.0 (clang-1500.3.9.4)\nlibc info: no libc information available\n$SHELL (typically, interactive shell): /bin/zsh\n\n\n[Enabled Hooks]\nnot run from a git repository - no hooks to show\n\n-- \nMatt Thompson\n   “The fact is, this is about us identifying what we do best and\n   finding more ways of doing less of it better” -- Director of Better\nAnna Rampton\n"},{"id":"500098","messageId":"xmqqy15b2aiz.fsf@gitster.g","threadId":"61902","inReplyTo":"CAFb48S8LDz4kiWsKSCBn8J=AHyQ5SVPFH4GY=z+8=DntT=PyAw@mail.gmail.com","subject":"Re: Bug: Git sees branch as valid commit ref and works; should fail","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-05T16:35:00Z","receivedAt":"2024-08-05T16:35:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matt Thompson <fortran@gmail.com> writes:\n\n> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n>\n> What did you do before the bug happened? (Steps to reproduce your issue)\n>\n> $ git clone https://github.com/GEOS-ESM/GFDL_atmos_cubed_sphere.git fvdycore\n> $ cd fvdycore\n> $ git checkout bugfix/mathomp4/trivial-ci-commit-gcc14\n>\n> What did you expect to happen? (Expected behavior)\n>\n> I expected a failure as there is no branch named\n> 'bugfix/mathomp4/trivial-ci-commit-gcc14' in the repository.\n\nIf you run\n\n  $ git for-each-ref | grep /bugfix/mathomp4/trivial-ci-commit-gcc14\n\nI suspect you have a remote-tracking branch that matches the\npattern.  In such a case, \"bugfix/mathomp4/trivial-ci-commit-gcc14\"\nis a very valid way to refer to a commit to any Git command.  It is\nhandy to say \"git show bugfix/mathomp4/trivial-ci-commit-gcc14\", for\nexample.\n\nAnd when \"git checkout\" is given a commit that is not a local\nbranch, what it did below (by the way, thanks for giving a very\naccurate \"Actual behavior\" description in your report) is very much\nexpected behaviour, including the part that it gave a message to\nadvise how to work on a detached HEAD.\n\n> What happened instead? (Actual behavior)\n>\n> $ git checkout bugfix/mathomp4/trivial-ci-commit-gcc14\n> Note: switching to 'bugfix/mathomp4/trivial-ci-commit-gcc14'.\n> ...\n\n"},{"id":"500109","messageId":"CAFb48S8+X0=Zqi8oisB0fAgx7HoyQrahF-RGQdagXTX3RdfSNQ@mail.gmail.com","threadId":"61902","inReplyTo":"xmqqy15b2aiz.fsf@gitster.g","subject":"Re: Bug: Git sees branch as valid commit ref and works; should fail","fromName":"Matt Thompson","fromEmail":"fortran@gmail.com","sentAt":"2024-08-05T16:58:57Z","receivedAt":"2024-08-05T16:59:27Z","isPatch":false,"sender":{"key":"fortran@gmail.com","avatar":null},"body":"Well, I tried the \"for-each-ref\" command and:\n\n$ git for-each-ref | grep /bugfix/mathomp4/trivial-ci-commit-gcc14\n$ echo $?\n1\n\nIndeed, if I try a few other greps, there are no \"trivial\" or \"gcc14\":\n\n$ git for-each-ref | grep -i gcc14\n$ echo $?\n1\n$ git for-each-ref | grep -i trivial\n$ echo $?\n1\n\nBut, I did some experimenting and I found that \"ci-gcc14\" does\ntrigger...something:\n\n$ git for-each-ref | grep -i ci-gcc14\nmathomp4@gslwl2023080107 ~/GitBugReport/fvdycore geos/main\n$ echo $?\n0\n\nBut I'm confused as to how the grep returned a status of 0 but didn't\nprint anything? So it said \"Yes I see this string\" but it actually\ndidn't? And it can switch to it:\n\n$ git switch ci-gcc14\nfatal: a branch is expected, got commit 'ci-gcc14'\nhint: If you want to detach HEAD at the commit, try again with the\n--detach option.\nmathomp4@gslwl2023080107 ~/GitBugReport/fvdycore geos/main\n$ git switch --detach ci-gcc14\nHEAD is now at cc14d30 removed an unused module use in fv_sg\n\n\nOn Mon, Aug 5, 2024 at 12:35 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Matt Thompson <fortran@gmail.com> writes:\n>\n> > Thank you for filling out a Git bug report!\n> > Please answer the following questions to help us understand your issue.\n> >\n> > What did you do before the bug happened? (Steps to reproduce your issue)\n> >\n> > $ git clone https://github.com/GEOS-ESM/GFDL_atmos_cubed_sphere.git fvdycore\n> > $ cd fvdycore\n> > $ git checkout bugfix/mathomp4/trivial-ci-commit-gcc14\n> >\n> > What did you expect to happen? (Expected behavior)\n> >\n> > I expected a failure as there is no branch named\n> > 'bugfix/mathomp4/trivial-ci-commit-gcc14' in the repository.\n>\n> If you run\n>\n>   $ git for-each-ref | grep /bugfix/mathomp4/trivial-ci-commit-gcc14\n>\n> I suspect you have a remote-tracking branch that matches the\n> pattern.  In such a case, \"bugfix/mathomp4/trivial-ci-commit-gcc14\"\n> is a very valid way to refer to a commit to any Git command.  It is\n> handy to say \"git show bugfix/mathomp4/trivial-ci-commit-gcc14\", for\n> example.\n>\n> And when \"git checkout\" is given a commit that is not a local\n> branch, what it did below (by the way, thanks for giving a very\n> accurate \"Actual behavior\" description in your report) is very much\n> expected behaviour, including the part that it gave a message to\n> advise how to work on a detached HEAD.\n>\n> > What happened instead? (Actual behavior)\n> >\n> > $ git checkout bugfix/mathomp4/trivial-ci-commit-gcc14\n> > Note: switching to 'bugfix/mathomp4/trivial-ci-commit-gcc14'.\n> > ...\n>\n\n\n-- \nMatt Thompson\n   “The fact is, this is about us identifying what we do best and\n   finding more ways of doing less of it better” -- Director of Better\nAnna Rampton\n"},{"id":"500769","messageId":"20240813115358.GB968816@coredump.intra.peff.net","threadId":"61902","inReplyTo":"CAFb48S8+X0=Zqi8oisB0fAgx7HoyQrahF-RGQdagXTX3RdfSNQ@mail.gmail.com","subject":"Re: Bug: Git sees branch as valid commit ref and works; should fail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-13T11:53:58Z","receivedAt":"2024-08-13T11:54:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 05, 2024 at 12:58:57PM -0400, Matt Thompson wrote:\n\n> $ git for-each-ref | grep /bugfix/mathomp4/trivial-ci-commit-gcc14\n> $ echo $?\n> 1\n\nHmm, this is quite an interesting case. Let's simplify the test case by\njust resolving the name instead of using checkout:\n\n  $ git clone https://github.com/GEOS-ESM/GFDL_atmos_cubed_sphere.git fvdycore\n  [...]\n  $ cd fvdycore\n  $ git rev-parse bugfix/mathomp4/trivial-ci-commit-gcc14\n  cc14d30e332cd06327fe5a81ed26c24140882f42\n\nThat narrows it down to the name resolution code. If I step through it\nin a debugger, the culprit seems to be get_describe_name(). I think it's\ntaking the \"-gcc14\" on the end as a git-describe. E.g., in a clean repo:\n\n  $ git init\n  $ git commit --allow-empty -m foo\n  $ git tag -m mytag mytag\n  $ git commit --allow-empty -m bar\n  $ git describe\n  mytag-1-g1a4fb75\n\nSo git describe will append \"-g<hex_hash>\", and we likewise accept that\nstyle during name resolution. But in this case, \"cc14\" happens to be a\nvalid hex (and you'll note that it matches the start of the one we\nfound!).\n\nIn other words, it's a false positive in the name resolver looking for\n\"describe\" names. We'd prefer a real ref of that full name, I think, but\nsince there isn't one, we prefer the describe resolution rather than\ntreating it as a path.\n\nI can think of a few ways to make this better:\n\n  - we ignore everything before the \"-g<hex>\" part entirely. Generally\n    this should be the name of a tag or at least some ref, so we could\n    perhaps verify that. But part of the point of sticking the hash in\n    the name is that you might have gotten the name from another source,\n    and your local one might not have the same tag. So that might be a\n    bad direction.\n\n  - the hash is abbreviated in the usual way, making it as short as\n    possible while remaining unambiguous. But unless the user goes out\n    of their way to set core.abbrev to something smaller, the minimum is\n    always 7. So perhaps get_describe_name() should be a bit more picky\n    about about that?\n\n    That doesn't fix the problem, but it makes it a lot less likely to\n    trigger in the real world. And anybody who really does somehow end\n    up with a describe name with 4 characters can always pick the hash\n    out of the string themselves (or just set core.abbrev in their local\n    repo to be more permissive).\n\nI think the second one is something like this:\n\ndiff --git a/object-name.c b/object-name.c\nindex 527b853ac4..a90338aa62 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1276,6 +1276,10 @@ static int get_describe_name(struct repository *r,\n \t\t\tif (ch == 'g' && cp[-1] == '-') {\n \t\t\t\tcp++;\n \t\t\t\tlen -= cp - name;\n+\t\t\t\tif (len < (default_abbrev < 0 ?\n+\t\t\t\t\t   FALLBACK_DEFAULT_ABBREV :\n+\t\t\t\t\t   default_abbrev))\n+\t\t\t\t\treturn -1;\n \t\t\t\treturn get_short_oid(r,\n \t\t\t\t\t\t     cp, len, oid, flags);\n \t\t\t}\n\n-Peff\n"},{"id":"500772","messageId":"20240813124550.GC968816@coredump.intra.peff.net","threadId":"61902","inReplyTo":"20240813115358.GB968816@coredump.intra.peff.net","subject":"[PATCH] get_oid(): enforce minimum length for \"-g<hex>\" names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-13T12:45:50Z","receivedAt":"2024-08-13T12:45:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 13, 2024 at 07:53:58AM -0400, Jeff King wrote:\n\n>   - the hash is abbreviated in the usual way, making it as short as\n>     possible while remaining unambiguous. But unless the user goes out\n>     of their way to set core.abbrev to something smaller, the minimum is\n>     always 7. So perhaps get_describe_name() should be a bit more picky\n>     about about that?\n> \n>     That doesn't fix the problem, but it makes it a lot less likely to\n>     trigger in the real world. And anybody who really does somehow end\n>     up with a describe name with 4 characters can always pick the hash\n>     out of the string themselves (or just set core.abbrev in their local\n>     repo to be more permissive).\n> \n> I think the second one is something like this:\n> \n> diff --git a/object-name.c b/object-name.c\n> index 527b853ac4..a90338aa62 100644\n> --- a/object-name.c\n> +++ b/object-name.c\n> @@ -1276,6 +1276,10 @@ static int get_describe_name(struct repository *r,\n>  \t\t\tif (ch == 'g' && cp[-1] == '-') {\n>  \t\t\t\tcp++;\n>  \t\t\t\tlen -= cp - name;\n> +\t\t\t\tif (len < (default_abbrev < 0 ?\n> +\t\t\t\t\t   FALLBACK_DEFAULT_ABBREV :\n> +\t\t\t\t\t   default_abbrev))\n> +\t\t\t\t\treturn -1;\n>  \t\t\t\treturn get_short_oid(r,\n>  \t\t\t\t\t\t     cp, len, oid, flags);\n>  \t\t\t}\n\nAfter thinking on this for a bit, it seems like the correct direction.\nSo here it is written in a slightly more readable way, and with a commit\nmessage and tests.\n\n-- >8 --\nSubject: [PATCH] get_oid(): enforce minimum length for \"-g<hex>\" names\n\nSince 7dd45e15c2 (sha1_name.c: understand \"describe\" output as a valid\nobject name, 2006-09-20), we'll resolve a name like \"foo-g1234abc\" into\nthe object name \"1234abc\". However, this has a small chance of creating\na surprising ambiguity.\n\nFor example, in a real world case we saw a name like \"foo-gcc14\" where\n\"gcc14\" refers to the compiler, but which caused us to look for an\nobject with prefix \"cc14\". And if the repo is large enough to have such\nan object, and small enough that there is only one such commit (since we\nfeed the disambiguation lookup code with the \"commit\" hint), then we'll\nreturn that object.\n\nNote that we would still resolve \"foo-gcc14\" as a tag name in preference\nto the describe name. But in this case it did not exist, and resolving\nanything was a surprise.\n\nWe can't solve the ambiguity completely, but we can reduce the chances\nof it happening significantly by enforcing a minimum length we'll accept\nfor the hex component. Since the name may have been generated by another\nrepository, we can't know for sure what minimum they would have used,\nbut a good guess is the value of core.abbrev (or if it's set to \"auto\",\nwhich is the default these days, the hard-coded minimum of \"7\").\n\nThere are five new tests here:\n\n  1. We check that describe names can be resolved at all. As far as I\n     can tell we had no existing test that covered this. It passes\n     before and after this patch.\n\n  2. Another option for solving this would be to insist that \"foo\" in\n     \"foo-gcc14\" matches an existing ref (which would have been the\n     source of the description). I don't think this is a good idea,\n     though, as part of the point of having the \"-g<hex>\" suffix is that\n     you might not have the same tags as whoever generated it. So this\n     test codifies the existing behavior that we do not care about the\n     parts before the \"-g\" at all.\n\n  3. Looking at the loop in get_describe_name(), we read from the back\n     end and check for \"-g\" when we see a non-hex digit. But if it's not\n     \"-g\", we keep looking! So for a name like \"foo-g1234abc-bar\", we'll\n     still pass \"1234abc-bar\" to get_short_oid()! This is OK in\n     practice, since it will barf when seeing the non-hex digits. But\n     let's confirm that it does so. This is particularly important\n     with our length checks, since \"foo-gcc14-bar\" would yield a length\n     of 8, which is plausibly long (so we are likewise depending on\n     get_short_oid() to reject it).\n\n  4. We check that names shorter than core.abbrev are rejected (i.e.,\n     the fix in this patch).\n\n  5. Likewise, when core.abbrev is \"auto\", we enforce the 7-character\n     minimum.\n\nReported-by: Matt Thompson <fortran@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n object-name.c       | 12 ++++++++++++\n t/t6120-describe.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 45 insertions(+)\n\ndiff --git a/object-name.c b/object-name.c\nindex 527b853ac4..6507a30ace 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1274,8 +1274,20 @@ static int get_describe_name(struct repository *r,\n \t\t\t * for it to be describe output.\n \t\t\t */\n \t\t\tif (ch == 'g' && cp[-1] == '-') {\n+\t\t\t\t/*\n+\t\t\t\t * To reduce the chance of false positives,\n+\t\t\t\t * assume that any \"-g<hex>\" must have some\n+\t\t\t\t * minimum number of <hex> that matches what\n+\t\t\t\t * we'd produce when abbreviating.\n+\t\t\t\t */\n+\t\t\t\tint min_len = default_abbrev;\n+\t\t\t\tif (min_len < 0)\n+\t\t\t\t\tmin_len = FALLBACK_DEFAULT_ABBREV;\n+\n \t\t\t\tcp++;\n \t\t\t\tlen -= cp - name;\n+\t\t\t\tif (len < min_len)\n+\t\t\t\t\treturn -1;\n \t\t\t\treturn get_short_oid(r,\n \t\t\t\t\t\t     cp, len, oid, flags);\n \t\t\t}\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 79e0f19deb..790afe40ac 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -707,4 +707,37 @@ test_expect_success 'describe --broken --dirty with a file with changed stat' '\n \t)\n '\n \n+test_expect_success 'long describe name can be resolved' '\n+\tname=$(git describe --long A) &&\n+\tgit rev-parse \"A^{commit}\" >expect &&\n+\tgit rev-parse \"$name\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'resolving describe name does not depend on tag' '\n+\thash=$(git rev-parse A^{commit}) &&\n+\tabbrev=$(echo $hash | cut -c1-30) &&\n+\techo \"$hash\" >expect &&\n+\tgit rev-parse \"does-not-exist-g$abbrev\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'resolving describe name only valid at end' '\n+\thash=$(git rev-parse A^{commit}) &&\n+\tabbrev=$(echo $hash | cut -c1-30) &&\n+\ttest_must_fail git rev-parse \"foo-g$abbrev-bar\"\n+'\n+\n+test_expect_success 'resolving describe name requires minimum abbrev (auto)' '\n+\thash=$(git rev-parse A^{commit}) &&\n+\tabbrev=$(echo $hash | cut -c1-6) &&\n+\ttest_must_fail git -c core.abbrev=auto rev-parse \"foo-g$abbrev\"\n+'\n+\n+test_expect_success 'resolving describe name requires minimum abbrev (config)' '\n+\thash=$(git rev-parse A^{commit}) &&\n+\tabbrev=$(echo $hash | cut -c1-20) &&\n+\ttest_must_fail git -c core.abbrev=25 rev-parse \"foo-g$abbrev\"\n+'\n+\n test_done\n-- \n2.46.0.452.ga6607598b6\n\n"},{"id":"500776","messageId":"20240813130203.GE968816@coredump.intra.peff.net","threadId":"61902","inReplyTo":"20240813124550.GC968816@coredump.intra.peff.net","subject":"Re: [PATCH] get_oid(): enforce minimum length for \"-g<hex>\" names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-13T13:02:03Z","receivedAt":"2024-08-13T13:02:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 13, 2024 at 08:45:50AM -0400, Jeff King wrote:\n\n>   3. Looking at the loop in get_describe_name(), we read from the back\n>      end and check for \"-g\" when we see a non-hex digit. But if it's not\n>      \"-g\", we keep looking! So for a name like \"foo-g1234abc-bar\", we'll\n>      still pass \"1234abc-bar\" to get_short_oid()! This is OK in\n>      practice, since it will barf when seeing the non-hex digits. But\n>      let's confirm that it does so. This is particularly important\n>      with our length checks, since \"foo-gcc14-bar\" would yield a length\n>      of 8, which is plausibly long (so we are likewise depending on\n>      get_short_oid() to reject it).\n\nSo I think the current code is working as we'd want, and there's no\ncorrectness issue. I do think breaking out of the loop early would be\nmore clear (and would provide a tiny speedup). Like:\n\ndiff --git a/object-name.c b/object-name.c\nindex 6507a30ace..89de9db8e9 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1263,35 +1263,36 @@ static int peel_onion(struct repository *r, const char *name, int len,\n static int get_describe_name(struct repository *r,\n \t\t\t     const char *name, int len,\n \t\t\t     struct object_id *oid)\n {\n \tconst char *cp;\n \tunsigned flags = GET_OID_QUIETLY | GET_OID_COMMIT;\n \n \tfor (cp = name + len - 1; name + 2 <= cp; cp--) {\n \t\tchar ch = *cp;\n \t\tif (!isxdigit(ch)) {\n \t\t\t/* We must be looking at g in \"SOMETHING-g\"\n \t\t\t * for it to be describe output.\n \t\t\t */\n \t\t\tif (ch == 'g' && cp[-1] == '-') {\n \t\t\t\t/*\n \t\t\t\t * To reduce the chance of false positives,\n \t\t\t\t * assume that any \"-g<hex>\" must have some\n \t\t\t\t * minimum number of <hex> that matches what\n \t\t\t\t * we'd produce when abbreviating.\n \t\t\t\t */\n \t\t\t\tint min_len = default_abbrev;\n \t\t\t\tif (min_len < 0)\n \t\t\t\t\tmin_len = FALLBACK_DEFAULT_ABBREV;\n \n \t\t\t\tcp++;\n \t\t\t\tlen -= cp - name;\n \t\t\t\tif (len < min_len)\n \t\t\t\t\treturn -1;\n \t\t\t\treturn get_short_oid(r,\n \t\t\t\t\t\t     cp, len, oid, flags);\n \t\t\t}\n+\t\t\tbreak;\n \t\t}\n \t}\n \treturn -1;\n }\n\n\nBut I don't know that it matters much in practice.\n\n-Peff\n"},{"id":"500797","messageId":"xmqqle108m73.fsf@gitster.g","threadId":"61902","inReplyTo":"20240813115358.GB968816@coredump.intra.peff.net","subject":"Re: Bug: Git sees branch as valid commit ref and works; should fail","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-13T15:41:20Z","receivedAt":"2024-08-13T15:41:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>   $ git rev-parse bugfix/mathomp4/trivial-ci-commit-gcc14\n>   cc14d30e332cd06327fe5a81ed26c24140882f42\n>\n> That narrows it down to the name resolution code. If I step through it\n> in a debugger, the culprit seems to be get_describe_name().\n\nROTFL.  Couldn't resist laughing with wonder, as I totally missed\nthe fact that -gcc14 exactly looks like a valid describe suffix.\n\n> In other words, it's a false positive in the name resolver looking for\n> \"describe\" names. We'd prefer a real ref of that full name, I think, but\n> since there isn't one, we prefer the describe resolution rather than\n> treating it as a path.\n>\n> I can think of a few ways to make this better:\n>\n>   - we ignore everything before the \"-g<hex>\" part entirely. Generally\n>     this should be the name of a tag or at least some ref, so we could\n>     perhaps verify that. But part of the point of sticking the hash in\n>     the name is that you might have gotten the name from another source,\n>     and your local one might not have the same tag. So that might be a\n>     bad direction.\n\nSad but you are absolutely right.\n\n>   - the hash is abbreviated in the usual way, making it as short as\n>     possible while remaining unambiguous. But unless the user goes out\n>     of their way to set core.abbrev to something smaller, the minimum is\n>     always 7. So perhaps get_describe_name() should be a bit more picky\n>     about about that?\n>\n>     That doesn't fix the problem, but it makes it a lot less likely to\n>     trigger in the real world. And anybody who really does somehow end\n>     up with a describe name with 4 characters can always pick the hash\n>     out of the string themselves (or just set core.abbrev in their local\n>     repo to be more permissive).\n\nThat sounds like not-so-bad a direction to go.  But notice that\nautogenerated preformatted documentation repositories record the\ncorresponding source material using the \"describe\" name that is\ninstructed to use absolutely minimum 4 chars when possible.  I would\nimagine that it is not only me who does that, assuming that those\nwho care enough about the correspondence between the commits in\nartifact repository and the commits in source repository would have\ntags used by the \"describe\" from the source repository.\n\n> I think the second one is something like this:\n>\n> diff --git a/object-name.c b/object-name.c\n> index 527b853ac4..a90338aa62 100644\n> --- a/object-name.c\n> +++ b/object-name.c\n> @@ -1276,6 +1276,10 @@ static int get_describe_name(struct repository *r,\n>  \t\t\tif (ch == 'g' && cp[-1] == '-') {\n>  \t\t\t\tcp++;\n>  \t\t\t\tlen -= cp - name;\n> +\t\t\t\tif (len < (default_abbrev < 0 ?\n> +\t\t\t\t\t   FALLBACK_DEFAULT_ABBREV :\n> +\t\t\t\t\t   default_abbrev))\n> +\t\t\t\t\treturn -1;\n>  \t\t\t\treturn get_short_oid(r,\n>  \t\t\t\t\t\t     cp, len, oid, flags);\n>  \t\t\t}\n\nSo, perhaps we probably would want to allow shorter than the\nfallback default hexadecimal after \"-g\" _iff_ the leading \"tag\" part\nactually names an existing tag, or something like that.\n\nThanks.\n"}]}