{"thread":{"id":"61619","subject":"SEGV (detected by Address Sanitizer) when using `core.abbrev` option","startedAt":"2024-06-10T22:27:27Z","lastAt":"2024-06-12T09:29:22Z","messageCount":14,"participants":["Kyle Lippincott","Junio C Hamano","Patrick Steinhardt","Karthik Nayak"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"496811","messageId":"CAO_smVimsHAPbMxy09mcYZY8apFgCbpnS9eSF7UOL6_BLqntNw@mail.gmail.com","threadId":"61619","inReplyTo":null,"subject":"SEGV (detected by Address Sanitizer) when using `core.abbrev` option","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-06-10T22:27:11Z","receivedAt":"2024-06-10T22:27:27Z","isPatch":false,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"c8aed5e8dadf (repository: stop setting SHA1 as the default object\nhash, 2024-05-07) stopped initializing the_hash_algo, but config.c\nreferences it when it observes a user setting core.abbrev, in two\nways:\n- if core.abbrev is detected as a 'no' boolean value, then\ndefault_abbrev is set to the_hash_algo->hexsz\n- if core.abbrev is set to an integer, it verifies that it's within\nrange for the hash algorithm (specifically: it errors out if the value\nis < minimum_abbrev or > the_hash_algo->hexsz).\n\nStack:\n==2421488==ERROR: AddressSanitizer: SEGV on unknown address\n0x000000000018 (pc 0x56344202585f bp 0x7fff9546fe10 sp 0x7fff9546fcb0\nT0)\n==2421488==The signal is caused by a READ memory access.\n==2421488==Hint: address points to the zero page.\n    #0 0x56344202585f in git_default_core_config git/config.c:1466\n    #1 0x56344202585f in git_default_config git/config.c:1815\n    #2 0x56344202064e in configset_iter git/config.c:2185\n    #3 0x563441d531cb in cmd_clone builtin/clone.c:981\n    #4 0x563441cebac2 in run_builtin git/git.c:474\n    #5 0x563441cebac2 in handle_builtin git/git.c:729\n    #6 0x563441ceed0a in run_argv git/git.c:793\n    #7 0x563441cf0aea in cmd_main git/git.c:928\n    #8 0x563441ce9323 in main git/common-main.c:62\n    #9 0x7fa3228456c9 in __libc_start_call_main\n../sysdeps/nptl/libc_start_call_main.h:58\n    #10 0x7fa322845784 in __libc_start_main_impl ../csu/libc-start.c:360\n    #11 0x563441ceb530 in _start (git/git+0x1e0530) (BuildId:\nc0e4b09d5b212a201769f1eb8e7592cddbe3af1d)\n\nAddressSanitizer can not provide additional info.\n\n---\n\nMy first thought for a fix was to just cap it at 40, with the\nassumption that the code would handle it correctly in the unlikely\nevent that the hash size ever decreased, but I don't think that does\nthe right thing if `core.abbrev=no`. That's documented as a way of\nobtaining the full hashes (with no abbreviation), and if we're using\nsha256, capping that at 40 hex (160bits) is incorrect.\n\nMy second thought was that we could store the requested value and\nvalidate it on every usage. This complicates usage locations, and can\nlead to poor behavior (crashes in the middle of operation when we\nfinally get around to checking the value).\n\nMy third thought was that we could store the requested value and\nvalidate when we have a repository that initializes the hash for us as\npart of that initialization. If we attempt to abbreviate some hashes\nwithout that setup, we act as if core.abbrev isn't set at all (so they\nget the default behavior). That seems like the best option overall.\nExploring that further ...\n\nI've looked at a semi-random collection of places where\n`default_abbrev` or `DEFAULT_ABBREV` are used, and they all seem to\neventually go through `repo_find_unique_abbrev_r`, and they pass in\nthe len. This also always has a repository available (otherwise it\nwouldn't be able to disambiguate). Furthermore, it has `if (len < 0)`.\nWe could thus carry the \"unvalidated\" request in default_abbrev by\nhaving special magic values (auto=-1 like today, no=-2 (replaced with\nhexsz when we know it), other requests are <= -4, for a requested\nlength of 4 or higher), or we could have another variable\n(requested_default_abbrev) that gets copied to default_abbrev when we\nhave a repo.\n\nThe one potential issue I can think of with this is that setting\n`core.abbrev = no`, having that resolve to 64 (sha256) when we set up\nthe repo, and then if we ever read from a repo that uses 40 hexsz\n(such as sha1), then we have to ensure that we tolerate a requested\nlength greater than the current hash algorithm's maximum length. This\nlikely wasn't a problem when sha1 was the default, because we're\nunlikely to go to a hash algorithm with <160 bits in the future. But\nif sha256 becomes the default, then this can be problematic.\n"},{"id":"496812","messageId":"CAO_smVjL+Ms1e_Rd5e1k-zMJb_NydBtX76Dh=ifK9Ym9ME9roQ@mail.gmail.com","threadId":"61619","inReplyTo":"CAO_smVimsHAPbMxy09mcYZY8apFgCbpnS9eSF7UOL6_BLqntNw@mail.gmail.com","subject":"Re: SEGV (detected by Address Sanitizer) when using `core.abbrev` option","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-06-10T23:01:42Z","receivedAt":"2024-06-10T23:02:04Z","isPatch":false,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Mon, Jun 10, 2024 at 3:27 PM Kyle Lippincott <spectral@google.com> wrote:\n>\n\nI just realized I didn't give a reproduction command, sorry about\nthat; here's the reproduction command provided by our user:\n\ngit config --global core.abbrev 12\ngit clone https://github.com/git/git.git\n\nI realized this because Josh Steadmon informed me that config\nloading/parsing is lazy when I asked why `git bisect` still worked\nwith this setting in my git config. So I think this might be a problem\nonly for things that go through `git_default_config` (if we set up the\nrepo object, we probably work just fine).\n\n> c8aed5e8dadf (repository: stop setting SHA1 as the default object\n> hash, 2024-05-07) stopped initializing the_hash_algo, but config.c\n> references it when it observes a user setting core.abbrev, in two\n> ways:\n> - if core.abbrev is detected as a 'no' boolean value, then\n> default_abbrev is set to the_hash_algo->hexsz\n> - if core.abbrev is set to an integer, it verifies that it's within\n> range for the hash algorithm (specifically: it errors out if the value\n> is < minimum_abbrev or > the_hash_algo->hexsz).\n>\n> Stack:\n> ==2421488==ERROR: AddressSanitizer: SEGV on unknown address\n> 0x000000000018 (pc 0x56344202585f bp 0x7fff9546fe10 sp 0x7fff9546fcb0\n> T0)\n> ==2421488==The signal is caused by a READ memory access.\n> ==2421488==Hint: address points to the zero page.\n>     #0 0x56344202585f in git_default_core_config git/config.c:1466\n>     #1 0x56344202585f in git_default_config git/config.c:1815\n>     #2 0x56344202064e in configset_iter git/config.c:2185\n>     #3 0x563441d531cb in cmd_clone builtin/clone.c:981\n>     #4 0x563441cebac2 in run_builtin git/git.c:474\n>     #5 0x563441cebac2 in handle_builtin git/git.c:729\n>     #6 0x563441ceed0a in run_argv git/git.c:793\n>     #7 0x563441cf0aea in cmd_main git/git.c:928\n>     #8 0x563441ce9323 in main git/common-main.c:62\n>     #9 0x7fa3228456c9 in __libc_start_call_main\n> ../sysdeps/nptl/libc_start_call_main.h:58\n>     #10 0x7fa322845784 in __libc_start_main_impl ../csu/libc-start.c:360\n>     #11 0x563441ceb530 in _start (git/git+0x1e0530) (BuildId:\n> c0e4b09d5b212a201769f1eb8e7592cddbe3af1d)\n>\n> AddressSanitizer can not provide additional info.\n>\n> ---\n>\n> My first thought for a fix was to just cap it at 40, with the\n> assumption that the code would handle it correctly in the unlikely\n> event that the hash size ever decreased, but I don't think that does\n> the right thing if `core.abbrev=no`. That's documented as a way of\n> obtaining the full hashes (with no abbreviation), and if we're using\n> sha256, capping that at 40 hex (160bits) is incorrect.\n>\n> My second thought was that we could store the requested value and\n> validate it on every usage. This complicates usage locations, and can\n> lead to poor behavior (crashes in the middle of operation when we\n> finally get around to checking the value).\n>\n> My third thought was that we could store the requested value and\n> validate when we have a repository that initializes the hash for us as\n> part of that initialization. If we attempt to abbreviate some hashes\n> without that setup, we act as if core.abbrev isn't set at all (so they\n> get the default behavior). That seems like the best option overall.\n> Exploring that further ...\n>\n> I've looked at a semi-random collection of places where\n> `default_abbrev` or `DEFAULT_ABBREV` are used, and they all seem to\n> eventually go through `repo_find_unique_abbrev_r`, and they pass in\n> the len. This also always has a repository available (otherwise it\n> wouldn't be able to disambiguate). Furthermore, it has `if (len < 0)`.\n> We could thus carry the \"unvalidated\" request in default_abbrev by\n> having special magic values (auto=-1 like today, no=-2 (replaced with\n> hexsz when we know it), other requests are <= -4, for a requested\n> length of 4 or higher), or we could have another variable\n> (requested_default_abbrev) that gets copied to default_abbrev when we\n> have a repo.\n>\n> The one potential issue I can think of with this is that setting\n> `core.abbrev = no`, having that resolve to 64 (sha256) when we set up\n> the repo, and then if we ever read from a repo that uses 40 hexsz\n> (such as sha1), then we have to ensure that we tolerate a requested\n> length greater than the current hash algorithm's maximum length. This\n> likely wasn't a problem when sha1 was the default, because we're\n> unlikely to go to a hash algorithm with <160 bits in the future. But\n> if sha256 becomes the default, then this can be problematic.\n"},{"id":"496814","messageId":"xmqq1q54z8nv.fsf@gitster.g","threadId":"61619","inReplyTo":"CAO_smVjL+Ms1e_Rd5e1k-zMJb_NydBtX76Dh=ifK9Ym9ME9roQ@mail.gmail.com","subject":"Re: SEGV (detected by Address Sanitizer) when using `core.abbrev` option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-10T23:21:08Z","receivedAt":"2024-06-10T23:21:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Lippincott <spectral@google.com> writes:\n\n> I just realized I didn't give a reproduction command, sorry about\n> that; here's the reproduction command provided by our user:\n>\n> git config --global core.abbrev 12\n> git clone https://github.com/git/git.git\n\nOr \"git -c core.abbrev=12 clone https://any/thing\".\n\nNicely spotted.  And this is a curious one, as you would generally\nexpect that having a fallback setting in the global space would help\nyou instead of hurting you.\n"},{"id":"496824","messageId":"ZmfpZm2vuwx0fl1w@tanuki","threadId":"61619","inReplyTo":"CAO_smVimsHAPbMxy09mcYZY8apFgCbpnS9eSF7UOL6_BLqntNw@mail.gmail.com","subject":"Re: SEGV (detected by Address Sanitizer) when using `core.abbrev` option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-11T06:06:30Z","receivedAt":"2024-06-11T06:06:37Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jun 10, 2024 at 03:27:11PM -0700, Kyle Lippincott wrote:\n> c8aed5e8dadf (repository: stop setting SHA1 as the default object\n> hash, 2024-05-07) stopped initializing the_hash_algo, but config.c\n> references it when it observes a user setting core.abbrev, in two\n> ways:\n> - if core.abbrev is detected as a 'no' boolean value, then\n> default_abbrev is set to the_hash_algo->hexsz\n> - if core.abbrev is set to an integer, it verifies that it's within\n> range for the hash algorithm (specifically: it errors out if the value\n> is < minimum_abbrev or > the_hash_algo->hexsz).\n> \n> Stack:\n> ==2421488==ERROR: AddressSanitizer: SEGV on unknown address\n> 0x000000000018 (pc 0x56344202585f bp 0x7fff9546fe10 sp 0x7fff9546fcb0\n> T0)\n> ==2421488==The signal is caused by a READ memory access.\n> ==2421488==Hint: address points to the zero page.\n>     #0 0x56344202585f in git_default_core_config git/config.c:1466\n>     #1 0x56344202585f in git_default_config git/config.c:1815\n>     #2 0x56344202064e in configset_iter git/config.c:2185\n>     #3 0x563441d531cb in cmd_clone builtin/clone.c:981\n>     #4 0x563441cebac2 in run_builtin git/git.c:474\n>     #5 0x563441cebac2 in handle_builtin git/git.c:729\n>     #6 0x563441ceed0a in run_argv git/git.c:793\n>     #7 0x563441cf0aea in cmd_main git/git.c:928\n>     #8 0x563441ce9323 in main git/common-main.c:62\n>     #9 0x7fa3228456c9 in __libc_start_call_main\n> ../sysdeps/nptl/libc_start_call_main.h:58\n>     #10 0x7fa322845784 in __libc_start_main_impl ../csu/libc-start.c:360\n>     #11 0x563441ceb530 in _start (git/git+0x1e0530) (BuildId:\n> c0e4b09d5b212a201769f1eb8e7592cddbe3af1d)\n> \n> AddressSanitizer can not provide additional info.\n\nGood find.\n\n> My first thought for a fix was to just cap it at 40, with the\n> assumption that the code would handle it correctly in the unlikely\n> event that the hash size ever decreased, but I don't think that does\n> the right thing if `core.abbrev=no`. That's documented as a way of\n> obtaining the full hashes (with no abbreviation), and if we're using\n> sha256, capping that at 40 hex (160bits) is incorrect.\n\nExactly, this doesn't feel like the right solution.\n\n> My second thought was that we could store the requested value and\n> validate it on every usage. This complicates usage locations, and can\n> lead to poor behavior (crashes in the middle of operation when we\n> finally get around to checking the value).\n\nThis feels more complex than needed indeed.\n\n> My third thought was that we could store the requested value and\n> validate when we have a repository that initializes the hash for us as\n> part of that initialization. If we attempt to abbreviate some hashes\n> without that setup, we act as if core.abbrev isn't set at all (so they\n> get the default behavior). That seems like the best option overall.\n> Exploring that further ...\n> \n> I've looked at a semi-random collection of places where\n> `default_abbrev` or `DEFAULT_ABBREV` are used, and they all seem to\n> eventually go through `repo_find_unique_abbrev_r`, and they pass in\n> the len. This also always has a repository available (otherwise it\n> wouldn't be able to disambiguate). Furthermore, it has `if (len < 0)`.\n> We could thus carry the \"unvalidated\" request in default_abbrev by\n> having special magic values (auto=-1 like today, no=-2 (replaced with\n> hexsz when we know it), other requests are <= -4, for a requested\n> length of 4 or higher), or we could have another variable\n> (requested_default_abbrev) that gets copied to default_abbrev when we\n> have a repo.\n\nThis also feels a bit too complex in my opinion.\n\n> The one potential issue I can think of with this is that setting\n> `core.abbrev = no`, having that resolve to 64 (sha256) when we set up\n> the repo, and then if we ever read from a repo that uses 40 hexsz\n> (such as sha1), then we have to ensure that we tolerate a requested\n> length greater than the current hash algorithm's maximum length. This\n> likely wasn't a problem when sha1 was the default, because we're\n> unlikely to go to a hash algorithm with <160 bits in the future. But\n> if sha256 becomes the default, then this can be problematic.\n\nI think this is the best solution. I don't quite see why we need to\nabort in the first place when the caller asks for something longer\nthan GIT_MAX_HEXSZ. If they do, it's rather clear that the intent is to\nshow the full object hash, so we can do that.\n\nIn other words, the following should be sufficient, shouldn't it?\n\ndiff --git a/config.c b/config.c\nindex abce05b774..0416b0f2b6 100644\n--- a/config.c\n+++ b/config.c\n@@ -1460,10 +1460,10 @@ static int git_default_core_config(const char *var, const char *value,\n \t\tif (!strcasecmp(value, \"auto\"))\n \t\t\tdefault_abbrev = -1;\n \t\telse if (!git_parse_maybe_bool_text(value))\n-\t\t\tdefault_abbrev = the_hash_algo->hexsz;\n+\t\t\tdefault_abbrev = GIT_MAX_HEXSZ;\n \t\telse {\n \t\t\tint abbrev = git_config_int(var, value, ctx->kvi);\n-\t\t\tif (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz)\n+\t\t\tif (abbrev < minimum_abbrev)\n \t\t\t\treturn error(_(\"abbrev length out of range: %d\"), abbrev);\n \t\t\tdefault_abbrev = abbrev;\n \t\t}\ndiff --git a/object-name.c b/object-name.c\nindex 523af6f64f..1be2ad1a16 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -837,7 +837,7 @@ int repo_find_unique_abbrev_r(struct repository *r, char *hex,\n \t}\n \n \toid_to_hex_r(hex, oid);\n-\tif (len == hexsz || !len)\n+\tif (len >= hexsz || !len)\n \t\treturn hexsz;\n \n \tmad.repo = r;\n\nThis is also in line with `parse_opt_abbrev_cb()`, which does the\nfollowing:\n\n    if (v && v < MINIMUM_ABBREV)\n        v = MINIMUM_ABBREV;\n    else if (startup_info->have_repository && v > the_hash_algo->hexsz)\n        v = the_hash_algo->hexsz;\n\nIn other words, it trims to `the_hash_algo->hexsz` or, if we don't have\na repository, it doesn't bother to trim the value at all.\n\nI'll send a patch that does this.\n\nPatrick\n"},{"id":"496852","messageId":"7ded51bbce1b23cf4110e3bf0abb7579efd4d344.1718095090.git.ps@pks.im","threadId":"61619","inReplyTo":"CAO_smVimsHAPbMxy09mcYZY8apFgCbpnS9eSF7UOL6_BLqntNw@mail.gmail.com","subject":"[PATCH 1/2] config: fix segfault when parsing \"core.abbrev\" without repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-11T08:42:02Z","receivedAt":"2024-06-11T08:42:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The \"core.abbrev\" config allows the user to specify the minimum length\nwhen abbreviating object hashes. Next to the values \"auto\" and \"no\",\nthis config also accepts a concrete length that needs to be bigger or\nequal to the minimum length and smaller or equal to the hash algorithm's\nhex length. While the former condition is trivial, the latter depends on\nthe object format used by the current repository. It is thus a variable\nupper boundary that may either be 40 (SHA-1) or 64 (SHA-256).\n\nThis has two major downsides. First, the user that specifies this config\nmust be aware of the object hashes that its repository use. If they want\nto configure the value globally, then they cannot pick any value in the\nrange `[41, 64]` if they have any repository that uses SHA-1. If they\ndid, Git would error out when parsing the config.\n\nSecond, and more importantly, parsing \"core.abbrev\" crashes when outside\nof a Git repository because we dereference `the_hash_algo` to figure out\nits hex length. Starting with c8aed5e8da (repository: stop setting SHA1\nas the default object hash, 2024-05-07) though, we stopped initializing\n`the_hash_algo` outside of Git repositories.\n\nFix both of these issues by not making it an error anymore when the\ngiven length exceeds the hash length. Instead, if we have a repository,\nthen we truncate the length to the maximum length of `the_hash_algo`.\nOtherwise, we simply leave the abbreviated length intact and store it\nas-is. This is equivalent to the logic in `parse_opt_abbrev_cb()` and is\nhandled just fine by `repo_find_unique_abbrev_r()`. In practice, we\nshould never even end up using `default_abbrev` without a repository\nanyway given that abbreviating object IDs to unique prefixes requires us\nto have access to an object database.\n\nReported-by: Kyle Lippincott <spectral@google.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n config.c         | 7 +++++--\n t/t4202-log.sh   | 6 ++++++\n t/t5601-clone.sh | 7 +++++++\n 3 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex abce05b774..ab2844d9e1 100644\n--- a/config.c\n+++ b/config.c\n@@ -1460,11 +1460,14 @@ static int git_default_core_config(const char *var, const char *value,\n \t\tif (!strcasecmp(value, \"auto\"))\n \t\t\tdefault_abbrev = -1;\n \t\telse if (!git_parse_maybe_bool_text(value))\n-\t\t\tdefault_abbrev = the_hash_algo->hexsz;\n+\t\t\tdefault_abbrev = startup_info->have_repository ?\n+\t\t\t\tthe_hash_algo->hexsz : GIT_MAX_HEXSZ;\n \t\telse {\n \t\t\tint abbrev = git_config_int(var, value, ctx->kvi);\n-\t\t\tif (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz)\n+\t\t\tif (abbrev < minimum_abbrev)\n \t\t\t\treturn error(_(\"abbrev length out of range: %d\"), abbrev);\n+\t\t\telse if (startup_info->have_repository && abbrev > the_hash_algo->hexsz)\n+\t\t\t\tabbrev = the_hash_algo->hexsz;\n \t\t\tdefault_abbrev = abbrev;\n \t\t}\n \t\treturn 0;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 86c695eb0a..99c063e4cd 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1237,6 +1237,12 @@ test_expect_success 'log.abbrevCommit configuration' '\n \ttest_cmp expect.whatchanged.full actual\n '\n \n+test_expect_success 'log.abbrevCommit with --abbrev=9000' '\n+\tgit log --no-abbrev >expect &&\n+\tgit log --abbrev-commit --abbrev=9000 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'show added path under \"--follow -M\"' '\n \t# This tests for a regression introduced in v1.7.2-rc0~103^2~2\n \ttest_create_repo regression &&\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex cc0b953f14..5d7ea147f1 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -46,6 +46,13 @@ test_expect_success 'output from clone' '\n \ttest $(grep Clon output | wc -l) = 1\n '\n \n+test_expect_success 'output from clone with core.abbrev does not crash' '\n+\trm -fr dst &&\n+\techo \"Cloning into ${SQ}dst${SQ}...\" >expect &&\n+\tgit -c core.abbrev=12 clone -n \"file://$(pwd)/src\" dst >actual 2>&1 &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'clone does not keep pack' '\n \n \trm -fr dst &&\n-- \n2.45.2.436.gcd77e87115.dirty\n\n"},{"id":"496853","messageId":"31c0405f85552b6f30468607b7a210d5961ea416.1718095090.git.ps@pks.im","threadId":"61619","inReplyTo":"CAO_smVimsHAPbMxy09mcYZY8apFgCbpnS9eSF7UOL6_BLqntNw@mail.gmail.com","subject":"[PATCH 2/2] object-name: don't try to abbreviate to lengths greater than hexsz","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-11T08:42:10Z","receivedAt":"2024-06-11T08:42:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When given a length that equals the current hash algorithm's hex size,\nthen `repo_find_unique_abbrev_r()` exits early without trying to find an\nabbreviation. This is only sensible because there is nothing to\nabbreviate in the first place, so searching through objects to find a\nunique prefix would be a waste of compute.\n\nWhat we don't handle though is the case where the user passes a length\ngreater than the hash length. This is fine in practice as we still\ncompute the correct result. But at the very least, this is a waste of\nresources as we try to abbreviate a value that cannot be abbreviated,\nwhich causes us to hit the object database.\n\nStart to explicitly handle values larger than hexsz to avoid this\nperformance penalty, which leads to a measureable speedup. The following\nbenchmark has been executed in linux.git:\n\n  Benchmark 1: git -c core.abbrev=9000 log --abbrev-commit (revision = HEAD~)\n    Time (mean ± σ):     12.812 s ±  0.040 s    [User: 12.225 s, System: 0.554 s]\n    Range (min … max):   12.723 s … 12.857 s    10 runs\n\n  Benchmark 2: git -c core.abbrev=9000 log --abbrev-commit (revision = HEAD)\n    Time (mean ± σ):     11.095 s ±  0.029 s    [User: 10.546 s, System: 0.521 s]\n    Range (min … max):   11.037 s … 11.122 s    10 runs\n\n  Summary\n    git -c core.abbrev=9000 log --abbrev-commit HEAD (revision = HEAD) ran\n      1.15 ± 0.00 times faster than git -c core.abbrev=9000 log --abbrev-commit HEAD (revision = HEAD~)\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n object-name.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex 523af6f64f..1be2ad1a16 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -837,7 +837,7 @@ int repo_find_unique_abbrev_r(struct repository *r, char *hex,\n \t}\n \n \toid_to_hex_r(hex, oid);\n-\tif (len == hexsz || !len)\n+\tif (len >= hexsz || !len)\n \t\treturn hexsz;\n \n \tmad.repo = r;\n-- \n2.45.2.436.gcd77e87115.dirty\n\n"},{"id":"496937","messageId":"xmqqh6dzwek0.fsf@gitster.g","threadId":"61619","inReplyTo":"7ded51bbce1b23cf4110e3bf0abb7579efd4d344.1718095090.git.ps@pks.im","subject":"Re: [PATCH 1/2] config: fix segfault when parsing \"core.abbrev\" without repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-11T17:54:23Z","receivedAt":"2024-06-11T17:54:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Fix both of these issues by not making it an error anymore when the\n> given length exceeds the hash length. Instead, if we have a repository,\n> then we truncate the length to the maximum length of `the_hash_algo`.\n> Otherwise, we simply leave the abbreviated length intact and store it\n> as-is. This is equivalent to the logic in `parse_opt_abbrev_cb()` and is\n> handled just fine by `repo_find_unique_abbrev_r()`. In practice, we\n> should never even end up using `default_abbrev` without a repository\n> anyway given that abbreviating object IDs to unique prefixes requires us\n> to have access to an object database.\n\nMakes sense.\n\n> diff --git a/config.c b/config.c\n> index abce05b774..ab2844d9e1 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1460,11 +1460,14 @@ static int git_default_core_config(const char *var, const char *value,\n>  \t\tif (!strcasecmp(value, \"auto\"))\n>  \t\t\tdefault_abbrev = -1;\n>  \t\telse if (!git_parse_maybe_bool_text(value))\n> -\t\t\tdefault_abbrev = the_hash_algo->hexsz;\n> +\t\t\tdefault_abbrev = startup_info->have_repository ?\n> +\t\t\t\tthe_hash_algo->hexsz : GIT_MAX_HEXSZ;\n\nWe will need to have some code that further adjusts overly long\ndefault_abbrev when we really have to abbreviate (at which time,\nhopefully we are already aware of the real hash algorithm used in\nthe repository, and that may be SHA-1) anyway.\n\nSo do we even need the conditional here?  Can't we just set it to\nGIT_MAX_HEXSZ here unconditionally?\n\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 86c695eb0a..99c063e4cd 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -1237,6 +1237,12 @@ test_expect_success 'log.abbrevCommit configuration' '\n>  \ttest_cmp expect.whatchanged.full actual\n>  '\n>  \n> +test_expect_success 'log.abbrevCommit with --abbrev=9000' '\n> +\tgit log --no-abbrev >expect &&\n> +\tgit log --abbrev-commit --abbrev=9000 >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nInteresting.  We didn't have coverage for \"we want to see full\nobject names\" case.\n\n> +test_expect_success 'output from clone with core.abbrev does not crash' '\n> +\trm -fr dst &&\n> +\techo \"Cloning into ${SQ}dst${SQ}...\" >expect &&\n> +\tgit -c core.abbrev=12 clone -n \"file://$(pwd)/src\" dst >actual 2>&1 &&\n> +\ttest_cmp expect actual\n> +'\n\nOK.\n"},{"id":"496984","messageId":"Zmk4PvvBMwEzePrk@tanuki","threadId":"61619","inReplyTo":"xmqqh6dzwek0.fsf@gitster.g","subject":"Re: [PATCH 1/2] config: fix segfault when parsing \"core.abbrev\" without repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-12T05:55:10Z","receivedAt":"2024-06-12T05:55:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jun 11, 2024 at 10:54:23AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > Fix both of these issues by not making it an error anymore when the\n> > given length exceeds the hash length. Instead, if we have a repository,\n> > then we truncate the length to the maximum length of `the_hash_algo`.\n> > Otherwise, we simply leave the abbreviated length intact and store it\n> > as-is. This is equivalent to the logic in `parse_opt_abbrev_cb()` and is\n> > handled just fine by `repo_find_unique_abbrev_r()`. In practice, we\n> > should never even end up using `default_abbrev` without a repository\n> > anyway given that abbreviating object IDs to unique prefixes requires us\n> > to have access to an object database.\n> \n> Makes sense.\n> \n> > diff --git a/config.c b/config.c\n> > index abce05b774..ab2844d9e1 100644\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -1460,11 +1460,14 @@ static int git_default_core_config(const char *var, const char *value,\n> >  \t\tif (!strcasecmp(value, \"auto\"))\n> >  \t\t\tdefault_abbrev = -1;\n> >  \t\telse if (!git_parse_maybe_bool_text(value))\n> > -\t\t\tdefault_abbrev = the_hash_algo->hexsz;\n> > +\t\t\tdefault_abbrev = startup_info->have_repository ?\n> > +\t\t\t\tthe_hash_algo->hexsz : GIT_MAX_HEXSZ;\n> \n> We will need to have some code that further adjusts overly long\n> default_abbrev when we really have to abbreviate (at which time,\n> hopefully we are already aware of the real hash algorithm used in\n> the repository, and that may be SHA-1) anyway.\n> \n> So do we even need the conditional here?  Can't we just set it to\n> GIT_MAX_HEXSZ here unconditionally?\n\nNot really. I was erring on the safe side here to retain the status quo\nfor all relevant cases. As explained in the commit message, the length\nis only relevant when we have a repository because we otherwise wouldn't\nbe able to abbreviate anything anyway. So essentially, this change is a\nfunctional no-op.\n\nThere's also the question of the second commit, where we only handle\n`abbrev == the_hash_algo->size`, but not `abbrev > the_hash_algo->size`.\nIt works, but is slower when not truncating the length until the second\npatch fixes it.\n\nSo yes, we can set this unconditionally to `GIT_MAX_HEXSZ`, but out of\nabundance of caution I decided to make this conditional.\n\nPatrick\n"},{"id":"496992","messageId":"cover.1718178996.git.ps@pks.im","threadId":"61619","inReplyTo":"CAO_smVimsHAPbMxy09mcYZY8apFgCbpnS9eSF7UOL6_BLqntNw@mail.gmail.com","subject":"[PATCH v2 0/3] config: fix segfault when parsing \"core.abbrev\" without repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-12T08:03:18Z","receivedAt":"2024-06-12T08:03:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that fixes a segfault when\nparsing \"core.abbrev\" without a repository.\n\nChanges compared to v1:\n\n  - Stop truncating the abbreviation length to the upper boundary\n    completely. It's unnecessary, complicates the code and makes us\n    dependent on `the_repository`.\n\n  - Adapt `parse_opt_abbrev_cb()` to also stop truncating such that the\n    behaviour of \"core.abbrev\" and `--abbrev` is the same.\n\n  - Extend test coverage a bit.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (3):\n  config: fix segfault when parsing \"core.abbrev\" without repo\n  parse-options-cb: stop clamping \"--abbrev=\" to hash length\n  object-name: don't try to abbreviate to lengths greater than hexsz\n\n config.c           |  4 ++--\n object-name.c      |  2 +-\n parse-options-cb.c |  2 --\n t/t4202-log.sh     | 24 ++++++++++++++++++++++++\n t/t5601-clone.sh   |  7 +++++++\n 5 files changed, 34 insertions(+), 5 deletions(-)\n\nRange-diff against v1:\n1:  7ded51bbce ! 1:  b48c50dd92 config: fix segfault when parsing \"core.abbrev\" without repo\n    @@ Commit message\n         `the_hash_algo` outside of Git repositories.\n     \n         Fix both of these issues by not making it an error anymore when the\n    -    given length exceeds the hash length. Instead, if we have a repository,\n    -    then we truncate the length to the maximum length of `the_hash_algo`.\n    -    Otherwise, we simply leave the abbreviated length intact and store it\n    -    as-is. This is equivalent to the logic in `parse_opt_abbrev_cb()` and is\n    -    handled just fine by `repo_find_unique_abbrev_r()`. In practice, we\n    -    should never even end up using `default_abbrev` without a repository\n    -    anyway given that abbreviating object IDs to unique prefixes requires us\n    -    to have access to an object database.\n    +    given length exceeds the hash length. Instead, leave the abbreviated\n    +    length intact. `repo_find_unique_abbrev_r()` handles this just fine\n    +    except for a performance penalty which we will fix in a subsequent\n    +    commit.\n     \n         Reported-by: Kyle Lippincott <spectral@google.com>\n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n    @@ config.c: static int git_default_core_config(const char *var, const char *value,\n      \t\t\tdefault_abbrev = -1;\n      \t\telse if (!git_parse_maybe_bool_text(value))\n     -\t\t\tdefault_abbrev = the_hash_algo->hexsz;\n    -+\t\t\tdefault_abbrev = startup_info->have_repository ?\n    -+\t\t\t\tthe_hash_algo->hexsz : GIT_MAX_HEXSZ;\n    ++\t\t\tdefault_abbrev = GIT_MAX_HEXSZ;\n      \t\telse {\n      \t\t\tint abbrev = git_config_int(var, value, ctx->kvi);\n     -\t\t\tif (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz)\n     +\t\t\tif (abbrev < minimum_abbrev)\n      \t\t\t\treturn error(_(\"abbrev length out of range: %d\"), abbrev);\n    -+\t\t\telse if (startup_info->have_repository && abbrev > the_hash_algo->hexsz)\n    -+\t\t\t\tabbrev = the_hash_algo->hexsz;\n      \t\t\tdefault_abbrev = abbrev;\n      \t\t}\n    - \t\treturn 0;\n     \n      ## t/t4202-log.sh ##\n     @@ t/t4202-log.sh: test_expect_success 'log.abbrevCommit configuration' '\n      \ttest_cmp expect.whatchanged.full actual\n      '\n      \n    -+test_expect_success 'log.abbrevCommit with --abbrev=9000' '\n    ++test_expect_success '--abbrev-commit with core.abbrev=false' '\n     +\tgit log --no-abbrev >expect &&\n    -+\tgit log --abbrev-commit --abbrev=9000 >actual &&\n    ++\tgit -c core.abbrev=false log --abbrev-commit >actual &&\n    ++\ttest_cmp expect actual\n    ++'\n    ++\n    ++test_expect_success '--abbrev-commit with core.abbrev=9000' '\n    ++\tgit log --no-abbrev >expect &&\n    ++\tgit -c core.abbrev=9000 log --abbrev-commit >actual &&\n     +\ttest_cmp expect actual\n     +'\n     +\n-:  ---------- > 2:  92860256a6 parse-options-cb: stop clamping \"--abbrev=\" to hash length\n2:  31c0405f85 = 3:  0ccb8d8efa object-name: don't try to abbreviate to lengths greater than hexsz\n-- \n2.45.2.457.g8d94cfb545.dirty\n\n"},{"id":"496993","messageId":"b48c50dd92769c7acc5c561f746a7d64dd4d2263.1718178996.git.ps@pks.im","threadId":"61619","inReplyTo":"cover.1718178996.git.ps@pks.im","subject":"[PATCH v2 1/3] config: fix segfault when parsing \"core.abbrev\" without repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-12T08:03:23Z","receivedAt":"2024-06-12T08:03:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The \"core.abbrev\" config allows the user to specify the minimum length\nwhen abbreviating object hashes. Next to the values \"auto\" and \"no\",\nthis config also accepts a concrete length that needs to be bigger or\nequal to the minimum length and smaller or equal to the hash algorithm's\nhex length. While the former condition is trivial, the latter depends on\nthe object format used by the current repository. It is thus a variable\nupper boundary that may either be 40 (SHA-1) or 64 (SHA-256).\n\nThis has two major downsides. First, the user that specifies this config\nmust be aware of the object hashes that its repository use. If they want\nto configure the value globally, then they cannot pick any value in the\nrange `[41, 64]` if they have any repository that uses SHA-1. If they\ndid, Git would error out when parsing the config.\n\nSecond, and more importantly, parsing \"core.abbrev\" crashes when outside\nof a Git repository because we dereference `the_hash_algo` to figure out\nits hex length. Starting with c8aed5e8da (repository: stop setting SHA1\nas the default object hash, 2024-05-07) though, we stopped initializing\n`the_hash_algo` outside of Git repositories.\n\nFix both of these issues by not making it an error anymore when the\ngiven length exceeds the hash length. Instead, leave the abbreviated\nlength intact. `repo_find_unique_abbrev_r()` handles this just fine\nexcept for a performance penalty which we will fix in a subsequent\ncommit.\n\nReported-by: Kyle Lippincott <spectral@google.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n config.c         |  4 ++--\n t/t4202-log.sh   | 12 ++++++++++++\n t/t5601-clone.sh |  7 +++++++\n 3 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex abce05b774..0416b0f2b6 100644\n--- a/config.c\n+++ b/config.c\n@@ -1460,10 +1460,10 @@ static int git_default_core_config(const char *var, const char *value,\n \t\tif (!strcasecmp(value, \"auto\"))\n \t\t\tdefault_abbrev = -1;\n \t\telse if (!git_parse_maybe_bool_text(value))\n-\t\t\tdefault_abbrev = the_hash_algo->hexsz;\n+\t\t\tdefault_abbrev = GIT_MAX_HEXSZ;\n \t\telse {\n \t\t\tint abbrev = git_config_int(var, value, ctx->kvi);\n-\t\t\tif (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz)\n+\t\t\tif (abbrev < minimum_abbrev)\n \t\t\t\treturn error(_(\"abbrev length out of range: %d\"), abbrev);\n \t\t\tdefault_abbrev = abbrev;\n \t\t}\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 86c695eb0a..e97826458c 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1237,6 +1237,18 @@ test_expect_success 'log.abbrevCommit configuration' '\n \ttest_cmp expect.whatchanged.full actual\n '\n \n+test_expect_success '--abbrev-commit with core.abbrev=false' '\n+\tgit log --no-abbrev >expect &&\n+\tgit -c core.abbrev=false log --abbrev-commit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--abbrev-commit with core.abbrev=9000' '\n+\tgit log --no-abbrev >expect &&\n+\tgit -c core.abbrev=9000 log --abbrev-commit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'show added path under \"--follow -M\"' '\n \t# This tests for a regression introduced in v1.7.2-rc0~103^2~2\n \ttest_create_repo regression &&\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex cc0b953f14..5d7ea147f1 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -46,6 +46,13 @@ test_expect_success 'output from clone' '\n \ttest $(grep Clon output | wc -l) = 1\n '\n \n+test_expect_success 'output from clone with core.abbrev does not crash' '\n+\trm -fr dst &&\n+\techo \"Cloning into ${SQ}dst${SQ}...\" >expect &&\n+\tgit -c core.abbrev=12 clone -n \"file://$(pwd)/src\" dst >actual 2>&1 &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'clone does not keep pack' '\n \n \trm -fr dst &&\n-- \n2.45.2.457.g8d94cfb545.dirty\n\n"},{"id":"496994","messageId":"92860256a61f14b60d6172d396f3ba5fd7be557c.1718178996.git.ps@pks.im","threadId":"61619","inReplyTo":"cover.1718178996.git.ps@pks.im","subject":"[PATCH v2 2/3] parse-options-cb: stop clamping \"--abbrev=\" to hash length","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-12T08:03:31Z","receivedAt":"2024-06-12T08:03:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `OPT__ABBREV()` option allows the user to specify the length that\nobject hashes shall be abbreviated to. This length needs to be in the\nrange of `(MIN_ABBREV, the_hash_algo->hexsz)`, which is why we clamp the\nvalue as required. While this makes sense in the case of `MIN_ABBREV`,\nit is unnecessary for the upper boundary as the value is eventually\npassed down to `repo_find_unnique_abbrev_r()`, which handles values\nlarger than the current hash length just fine.\n\nIn the preceding commit, we have changed parsing of the \"core.abbrev\"\nconfig to stop clamping to the upper boundary. Let's do the same here so\nthat the code becomes simpler, we are consistent with how we treat the\n\"core.abbrev\" config and so that we stop depending on `the_repository`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n parse-options-cb.c |  2 --\n t/t4202-log.sh     | 12 ++++++++++++\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex d99d688d3c..b2aa62a9dc 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -30,8 +30,6 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n \t\t\t\t     opt->long_name);\n \t\tif (v && v < MINIMUM_ABBREV)\n \t\t\tv = MINIMUM_ABBREV;\n-\t\telse if (startup_info->have_repository && v > the_hash_algo->hexsz)\n-\t\t\tv = the_hash_algo->hexsz;\n \t}\n \t*(int *)(opt->value) = v;\n \treturn 0;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex e97826458c..51f7beb59f 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1243,12 +1243,24 @@ test_expect_success '--abbrev-commit with core.abbrev=false' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--abbrev-commit with --no-abbrev' '\n+\tgit log --no-abbrev >expect &&\n+\tgit log --abbrev-commit --no-abbrev >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--abbrev-commit with core.abbrev=9000' '\n \tgit log --no-abbrev >expect &&\n \tgit -c core.abbrev=9000 log --abbrev-commit >actual &&\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--abbrev-commit with --abbrev=9000' '\n+\tgit log --no-abbrev >expect &&\n+\tgit log --abbrev-commit --abbrev=9000 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'show added path under \"--follow -M\"' '\n \t# This tests for a regression introduced in v1.7.2-rc0~103^2~2\n \ttest_create_repo regression &&\n-- \n2.45.2.457.g8d94cfb545.dirty\n\n"},{"id":"496995","messageId":"0ccb8d8efa4010c4a7ba00ca1abe7f3e923843ed.1718178996.git.ps@pks.im","threadId":"61619","inReplyTo":"cover.1718178996.git.ps@pks.im","subject":"[PATCH v2 3/3] object-name: don't try to abbreviate to lengths greater than hexsz","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-12T08:03:36Z","receivedAt":"2024-06-12T08:03:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When given a length that equals the current hash algorithm's hex size,\nthen `repo_find_unique_abbrev_r()` exits early without trying to find an\nabbreviation. This is only sensible because there is nothing to\nabbreviate in the first place, so searching through objects to find a\nunique prefix would be a waste of compute.\n\nWhat we don't handle though is the case where the user passes a length\ngreater than the hash length. This is fine in practice as we still\ncompute the correct result. But at the very least, this is a waste of\nresources as we try to abbreviate a value that cannot be abbreviated,\nwhich causes us to hit the object database.\n\nStart to explicitly handle values larger than hexsz to avoid this\nperformance penalty, which leads to a measureable speedup. The following\nbenchmark has been executed in linux.git:\n\n  Benchmark 1: git -c core.abbrev=9000 log --abbrev-commit (revision = HEAD~)\n    Time (mean ± σ):     12.812 s ±  0.040 s    [User: 12.225 s, System: 0.554 s]\n    Range (min … max):   12.723 s … 12.857 s    10 runs\n\n  Benchmark 2: git -c core.abbrev=9000 log --abbrev-commit (revision = HEAD)\n    Time (mean ± σ):     11.095 s ±  0.029 s    [User: 10.546 s, System: 0.521 s]\n    Range (min … max):   11.037 s … 11.122 s    10 runs\n\n  Summary\n    git -c core.abbrev=9000 log --abbrev-commit HEAD (revision = HEAD) ran\n      1.15 ± 0.00 times faster than git -c core.abbrev=9000 log --abbrev-commit HEAD (revision = HEAD~)\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n object-name.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex 523af6f64f..1be2ad1a16 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -837,7 +837,7 @@ int repo_find_unique_abbrev_r(struct repository *r, char *hex,\n \t}\n \n \toid_to_hex_r(hex, oid);\n-\tif (len == hexsz || !len)\n+\tif (len >= hexsz || !len)\n \t\treturn hexsz;\n \n \tmad.repo = r;\n-- \n2.45.2.457.g8d94cfb545.dirty\n\n"},{"id":"497009","messageId":"CAOLa=ZTLfN665svPMaCJvBT7rjVrg2_=CiZmb3u5XfmmOR6_aQ@mail.gmail.com","threadId":"61619","inReplyTo":"b48c50dd92769c7acc5c561f746a7d64dd4d2263.1718178996.git.ps@pks.im","subject":"Re: [PATCH v2 1/3] config: fix segfault when parsing \"core.abbrev\" without repo","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-06-12T09:22:57Z","receivedAt":"2024-06-12T09:22:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The \"core.abbrev\" config allows the user to specify the minimum length\n> when abbreviating object hashes. Next to the values \"auto\" and \"no\",\n> this config also accepts a concrete length that needs to be bigger or\n> equal to the minimum length and smaller or equal to the hash algorithm's\n> hex length. While the former condition is trivial, the latter depends on\n> the object format used by the current repository. It is thus a variable\n> upper boundary that may either be 40 (SHA-1) or 64 (SHA-256).\n>\n> This has two major downsides. First, the user that specifies this config\n> must be aware of the object hashes that its repository use. If they want\n> to configure the value globally, then they cannot pick any value in the\n> range `[41, 64]` if they have any repository that uses SHA-1. If they\n> did, Git would error out when parsing the config.\n>\n> Second, and more importantly, parsing \"core.abbrev\" crashes when outside\n> of a Git repository because we dereference `the_hash_algo` to figure out\n> its hex length. Starting with c8aed5e8da (repository: stop setting SHA1\n> as the default object hash, 2024-05-07) though, we stopped initializing\n> `the_hash_algo` outside of Git repositories.\n>\n> Fix both of these issues by not making it an error anymore when the\n> given length exceeds the hash length. Instead, leave the abbreviated\n> length intact. `repo_find_unique_abbrev_r()` handles this just fine\n> except for a performance penalty which we will fix in a subsequent\n> commit.\n>\n> Reported-by: Kyle Lippincott <spectral@google.com>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  config.c         |  4 ++--\n>  t/t4202-log.sh   | 12 ++++++++++++\n>  t/t5601-clone.sh |  7 +++++++\n>  3 files changed, 21 insertions(+), 2 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index abce05b774..0416b0f2b6 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1460,10 +1460,10 @@ static int git_default_core_config(const char *var, const char *value,\n>  \t\tif (!strcasecmp(value, \"auto\"))\n>  \t\t\tdefault_abbrev = -1;\n>  \t\telse if (!git_parse_maybe_bool_text(value))\n> -\t\t\tdefault_abbrev = the_hash_algo->hexsz;\n> +\t\t\tdefault_abbrev = GIT_MAX_HEXSZ;\n\nSo if the value is set to 'no'ish, we set it to the max value possible.\n\n>  \t\telse {\n>  \t\t\tint abbrev = git_config_int(var, value, ctx->kvi);\n> -\t\t\tif (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz)\n> +\t\t\tif (abbrev < minimum_abbrev)\n>  \t\t\t\treturn error(_(\"abbrev length out of range: %d\"), abbrev);\n>  \t\t\tdefault_abbrev = abbrev;\n>  \t\t}\n>\n\nI was wondering if the documentation for 'core.abbrev' needs to be\nmodified, but seems like we don't mention how the max value can error\nout in the first place so all good there.\n\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 86c695eb0a..e97826458c 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -1237,6 +1237,18 @@ test_expect_success 'log.abbrevCommit configuration' '\n>  \ttest_cmp expect.whatchanged.full actual\n>  '\n>\n> +test_expect_success '--abbrev-commit with core.abbrev=false' '\n> +\tgit log --no-abbrev >expect &&\n> +\tgit -c core.abbrev=false log --abbrev-commit >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success '--abbrev-commit with core.abbrev=9000' '\n> +\tgit log --no-abbrev >expect &&\n> +\tgit -c core.abbrev=9000 log --abbrev-commit >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'show added path under \"--follow -M\"' '\n>  \t# This tests for a regression introduced in v1.7.2-rc0~103^2~2\n>  \ttest_create_repo regression &&\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index cc0b953f14..5d7ea147f1 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -46,6 +46,13 @@ test_expect_success 'output from clone' '\n>  \ttest $(grep Clon output | wc -l) = 1\n>  '\n>\n> +test_expect_success 'output from clone with core.abbrev does not crash' '\n> +\trm -fr dst &&\n> +\techo \"Cloning into ${SQ}dst${SQ}...\" >expect &&\n> +\tgit -c core.abbrev=12 clone -n \"file://$(pwd)/src\" dst >actual 2>&1 &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'clone does not keep pack' '\n>\n>  \trm -fr dst &&\n> --\n> 2.45.2.457.g8d94cfb545.dirty\n"},{"id":"497010","messageId":"CAOLa=ZQBmp3Aj135HppKzkcVhpNC40JgtX6YN6Dam34cmeSg5Q@mail.gmail.com","threadId":"61619","inReplyTo":"cover.1718178996.git.ps@pks.im","subject":"Re: [PATCH v2 0/3] config: fix segfault when parsing \"core.abbrev\" without repo","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-06-12T09:29:21Z","receivedAt":"2024-06-12T09:29:22Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Hi,\n>\n> this is the second version of my patch series that fixes a segfault when\n> parsing \"core.abbrev\" without a repository.\n>\n> Changes compared to v1:\n>\n>   - Stop truncating the abbreviation length to the upper boundary\n>     completely. It's unnecessary, complicates the code and makes us\n>     dependent on `the_repository`.\n>\n>   - Adapt `parse_opt_abbrev_cb()` to also stop truncating such that the\n>     behaviour of \"core.abbrev\" and `--abbrev` is the same.\n>\n>   - Extend test coverage a bit.\n>\n> Thanks!\n>\n> Patrick\n\nThe changes look good, I wasn't expecting a benchmark in the last commit\nfor such a small change, but that was nice to see. Thanks.\n\n[snip]\n"}]}