| Message ID | b48c50dd92769c7acc5c561f746a7d64dd4d2263.1718178996.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 524c0183c999c59940ce1a8712b78e4dbd87ae60 |
| Headers | show |
| Series | config: fix segfault when parsing "core.abbrev" without repo | expand |
Patrick Steinhardt <ps@pks.im> writes: > The "core.abbrev" config allows the user to specify the minimum length > when abbreviating object hashes. Next to the values "auto" and "no", > this config also accepts a concrete length that needs to be bigger or > equal to the minimum length and smaller or equal to the hash algorithm's > hex length. While the former condition is trivial, the latter depends on > the object format used by the current repository. It is thus a variable > upper boundary that may either be 40 (SHA-1) or 64 (SHA-256). > > This has two major downsides. First, the user that specifies this config > must be aware of the object hashes that its repository use. If they want > to configure the value globally, then they cannot pick any value in the > range `[41, 64]` if they have any repository that uses SHA-1. If they > did, Git would error out when parsing the config. > > Second, and more importantly, parsing "core.abbrev" crashes when outside > of a Git repository because we dereference `the_hash_algo` to figure out > its hex length. Starting with c8aed5e8da (repository: stop setting SHA1 > as the default object hash, 2024-05-07) though, we stopped initializing > `the_hash_algo` outside of Git repositories. > > Fix both of these issues by not making it an error anymore when the > given length exceeds the hash length. Instead, leave the abbreviated > length intact. `repo_find_unique_abbrev_r()` handles this just fine > except for a performance penalty which we will fix in a subsequent > commit. > > Reported-by: Kyle Lippincott <spectral@google.com> > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > config.c | 4 ++-- > t/t4202-log.sh | 12 ++++++++++++ > t/t5601-clone.sh | 7 +++++++ > 3 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/config.c b/config.c > index abce05b774..0416b0f2b6 100644 > --- a/config.c > +++ b/config.c > @@ -1460,10 +1460,10 @@ static int git_default_core_config(const char *var, const char *value, > if (!strcasecmp(value, "auto")) > default_abbrev = -1; > else if (!git_parse_maybe_bool_text(value)) > - default_abbrev = the_hash_algo->hexsz; > + default_abbrev = GIT_MAX_HEXSZ; So if the value is set to 'no'ish, we set it to the max value possible. > else { > int abbrev = git_config_int(var, value, ctx->kvi); > - if (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz) > + if (abbrev < minimum_abbrev) > return error(_("abbrev length out of range: %d"), abbrev); > default_abbrev = abbrev; > } > I was wondering if the documentation for 'core.abbrev' needs to be modified, but seems like we don't mention how the max value can error out in the first place so all good there. > diff --git a/t/t4202-log.sh b/t/t4202-log.sh > index 86c695eb0a..e97826458c 100755 > --- a/t/t4202-log.sh > +++ b/t/t4202-log.sh > @@ -1237,6 +1237,18 @@ test_expect_success 'log.abbrevCommit configuration' ' > test_cmp expect.whatchanged.full actual > ' > > +test_expect_success '--abbrev-commit with core.abbrev=false' ' > + git log --no-abbrev >expect && > + git -c core.abbrev=false log --abbrev-commit >actual && > + test_cmp expect actual > +' > + > +test_expect_success '--abbrev-commit with core.abbrev=9000' ' > + git log --no-abbrev >expect && > + git -c core.abbrev=9000 log --abbrev-commit >actual && > + test_cmp expect actual > +' > + > test_expect_success 'show added path under "--follow -M"' ' > # This tests for a regression introduced in v1.7.2-rc0~103^2~2 > test_create_repo regression && > diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh > index cc0b953f14..5d7ea147f1 100755 > --- a/t/t5601-clone.sh > +++ b/t/t5601-clone.sh > @@ -46,6 +46,13 @@ test_expect_success 'output from clone' ' > test $(grep Clon output | wc -l) = 1 > ' > > +test_expect_success 'output from clone with core.abbrev does not crash' ' > + rm -fr dst && > + echo "Cloning into ${SQ}dst${SQ}..." >expect && > + git -c core.abbrev=12 clone -n "file://$(pwd)/src" dst >actual 2>&1 && > + test_cmp expect actual > +' > + > test_expect_success 'clone does not keep pack' ' > > rm -fr dst && > -- > 2.45.2.457.g8d94cfb545.dirty
diff --git a/config.c b/config.c index abce05b774..0416b0f2b6 100644 --- a/config.c +++ b/config.c @@ -1460,10 +1460,10 @@ static int git_default_core_config(const char *var, const char *value, if (!strcasecmp(value, "auto")) default_abbrev = -1; else if (!git_parse_maybe_bool_text(value)) - default_abbrev = the_hash_algo->hexsz; + default_abbrev = GIT_MAX_HEXSZ; else { int abbrev = git_config_int(var, value, ctx->kvi); - if (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz) + if (abbrev < minimum_abbrev) return error(_("abbrev length out of range: %d"), abbrev); default_abbrev = abbrev; } diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 86c695eb0a..e97826458c 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -1237,6 +1237,18 @@ test_expect_success 'log.abbrevCommit configuration' ' test_cmp expect.whatchanged.full actual ' +test_expect_success '--abbrev-commit with core.abbrev=false' ' + git log --no-abbrev >expect && + git -c core.abbrev=false log --abbrev-commit >actual && + test_cmp expect actual +' + +test_expect_success '--abbrev-commit with core.abbrev=9000' ' + git log --no-abbrev >expect && + git -c core.abbrev=9000 log --abbrev-commit >actual && + test_cmp expect actual +' + test_expect_success 'show added path under "--follow -M"' ' # This tests for a regression introduced in v1.7.2-rc0~103^2~2 test_create_repo regression && diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index cc0b953f14..5d7ea147f1 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -46,6 +46,13 @@ test_expect_success 'output from clone' ' test $(grep Clon output | wc -l) = 1 ' +test_expect_success 'output from clone with core.abbrev does not crash' ' + rm -fr dst && + echo "Cloning into ${SQ}dst${SQ}..." >expect && + git -c core.abbrev=12 clone -n "file://$(pwd)/src" dst >actual 2>&1 && + test_cmp expect actual +' + test_expect_success 'clone does not keep pack' ' rm -fr dst &&
The "core.abbrev" config allows the user to specify the minimum length when abbreviating object hashes. Next to the values "auto" and "no", this config also accepts a concrete length that needs to be bigger or equal to the minimum length and smaller or equal to the hash algorithm's hex length. While the former condition is trivial, the latter depends on the object format used by the current repository. It is thus a variable upper boundary that may either be 40 (SHA-1) or 64 (SHA-256). This has two major downsides. First, the user that specifies this config must be aware of the object hashes that its repository use. If they want to configure the value globally, then they cannot pick any value in the range `[41, 64]` if they have any repository that uses SHA-1. If they did, Git would error out when parsing the config. Second, and more importantly, parsing "core.abbrev" crashes when outside of a Git repository because we dereference `the_hash_algo` to figure out its hex length. Starting with c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07) though, we stopped initializing `the_hash_algo` outside of Git repositories. Fix both of these issues by not making it an error anymore when the given length exceeds the hash length. Instead, leave the abbreviated length intact. `repo_find_unique_abbrev_r()` handles this just fine except for a performance penalty which we will fix in a subsequent commit. Reported-by: Kyle Lippincott <spectral@google.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> --- config.c | 4 ++-- t/t4202-log.sh | 12 ++++++++++++ t/t5601-clone.sh | 7 +++++++ 3 files changed, 21 insertions(+), 2 deletions(-)