# [PATCH] core.abbrev disables abbreviations 13 messages from 2020-09-01 to 2020-12-23. Participants: Eric Wong, Derrick Stolee, Junio C Hamano, brian m. carlson. Thread: https://gitlist.dev/t/54169 ## Eric Wong, 2020-09-01 07:43 Subject: [PATCH] core.abbrev disables abbreviations Message-ID: <20200901074355.GA4498@dcvr> URL: https://gitlist.dev/e/20200901074355.GA4498%40dcvr ``` These allows users to write hash-agnostic scripts and configs to disable abbreviations. Using "-c core.abbrev=40" will be insufficient with SHA-256, and "-c core.abbrev=64" won't work with SHA-1 repos today. Signed-off-by: Eric Wong --- I kinda wanted to allow a value of "max", but I figured the existing boolean falsiness words might make more sense with `--no-abbrev' in for some commands... Naming is hard :x config.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/config.c b/config.c index 2bdff4457b..f2e09c72ca 100644 --- a/config.c +++ b/config.c @@ -1217,6 +1217,10 @@ static int git_default_core_config(const char *var, const char *value, void *cb) return config_error_nonbool(var); if (!strcasecmp(value, "auto")) default_abbrev = -1; + else if (!strcasecmp(value, "false") || + !strcasecmp(value, "no") || + !strcasecmp(value, "off")) + default_abbrev = the_hash_algo->hexsz; else { int abbrev = git_config_int(var, value); if (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz) ``` ## Derrick Stolee, 2020-09-01 12:14 Subject: Re: [PATCH] core.abbrev disables abbreviations Message-ID: <9c00f29b-45e4-ccdf-6d81-5eabd58c875b@gmail.com> URL: https://gitlist.dev/e/9c00f29b-45e4-ccdf-6d81-5eabd58c875b%40gmail.com In-Reply-To: <20200901074355.GA4498@dcvr> ``` On 9/1/2020 3:43 AM, Eric Wong wrote: > These allows users to write hash-agnostic scripts and configs to > disable abbreviations. Using "-c core.abbrev=40" will be > insufficient with SHA-256, and "-c core.abbrev=64" won't work > with SHA-1 repos today. > > Signed-off-by: Eric Wong > --- > I kinda wanted to allow a value of "max", but I figured the existing > boolean falsiness words might make more sense with `--no-abbrev' in > for some commands... Naming is hard :x > > config.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/config.c b/config.c > index 2bdff4457b..f2e09c72ca 100644 > --- a/config.c > +++ b/config.c > @@ -1217,6 +1217,10 @@ static int git_default_core_config(const char *var, const char *value, void *cb) > return config_error_nonbool(var); > if (!strcasecmp(value, "auto")) > default_abbrev = -1; > + else if (!strcasecmp(value, "false") || > + !strcasecmp(value, "no") || > + !strcasecmp(value, "off")) > + default_abbrev = the_hash_algo->hexsz; I'm not sure we need three synonyms for "no-abbrev" here. "false" would be natural, except I think in a few places the config value "0" is also interpreted as "false", but as seen below a value of "0" snaps up to the minimum allowed abbreviation. > else { > int abbrev = git_config_int(var, value); > if (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz) Perhaps "core.abbrev = never" would be a good option? After we decide on the word, this patch needs: * Updates to Documentation/config/core.txt * A test that works with both hash versions. Thanks, -Stolee ``` ## Eric Wong, 2020-09-01 14:43 Subject: Re: [PATCH] core.abbrev disables abbreviations Message-ID: <20200901144323.GA14554@dcvr> URL: https://gitlist.dev/e/20200901144323.GA14554%40dcvr In-Reply-To: <9c00f29b-45e4-ccdf-6d81-5eabd58c875b@gmail.com> ``` Derrick Stolee wrote: > On 9/1/2020 3:43 AM, Eric Wong wrote: > > index 2bdff4457b..f2e09c72ca 100644 > > --- a/config.c > > +++ b/config.c > > @@ -1217,6 +1217,10 @@ static int git_default_core_config(const char *var, const char *value, void *cb) > > return config_error_nonbool(var); > > if (!strcasecmp(value, "auto")) > > default_abbrev = -1; > > + else if (!strcasecmp(value, "false") || > > + !strcasecmp(value, "no") || > > + !strcasecmp(value, "off")) > > + default_abbrev = the_hash_algo->hexsz; > > I'm not sure we need three synonyms for "no-abbrev" here. I just used the accepted synonyms since I figured users would be used to them, already. > "false" would be natural, except I think in a few places > the config value "0" is also interpreted as "false", but > as seen below a value of "0" snaps up to the minimum > allowed abbreviation. > > > else { > > int abbrev = git_config_int(var, value); > > if (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz) It actually errors out on the next line, here. Perhaps adopting parse_opt_abbrev_cb behavior of clamping to the minimum and maximum supported values is more consistent? > Perhaps "core.abbrev = never" would be a good option? *shrug* > After we decide on the word, this patch needs: > > * Updates to Documentation/config/core.txt Will do. > * A test that works with both hash versions. Will do, though not too sure where the tests for this should be. Thanks for the comments, will wait a few days for comments before sending out v2. ``` ## Derrick Stolee, 2020-09-01 14:59 Subject: Re: [PATCH] core.abbrev disables abbreviations Message-ID: URL: https://gitlist.dev/e/cbbc6d40-7126-cf28-aeb5-46b72b7e6d08%40gmail.com In-Reply-To: <20200901144323.GA14554@dcvr> ``` On 9/1/2020 10:43 AM, Eric Wong wrote: > Derrick Stolee wrote: >> After we decide on the word, this patch needs: >> >> * Updates to Documentation/config/core.txt > > Will do. Thanks. >> * A test that works with both hash versions. > > Will do, though not too sure where the tests for this should be. t3404-rebase-interactive.sh seems to already test the core.abbrev config value. Could be a good place to provide an extra check. t4205-log-pretty-formats.sh could also use some references to core.abbrev. Thanks, -Stolee ``` ## Junio C Hamano, 2020-09-01 15:49 Subject: Re: [PATCH] core.abbrev disables abbreviations Message-ID: URL: https://gitlist.dev/e/xmqqblipebto.fsf%40gitster.c.googlers.com In-Reply-To: <9c00f29b-45e4-ccdf-6d81-5eabd58c875b@gmail.com> ``` Derrick Stolee writes: >> + else if (!strcasecmp(value, "false") || >> + !strcasecmp(value, "no") || >> + !strcasecmp(value, "off")) >> + default_abbrev = the_hash_algo->hexsz; > > I'm not sure we need three synonyms for "no-abbrev" here. I do not particularly mind, but if we imitate the variety of various boolean false, I'd prefer to see the code to parse them shared to avoid them drifting apart over time. > "false" would be natural, except I think in a few places > the config value "0" is also interpreted as "false", but > as seen below a value of "0" snaps up to the minimum > allowed abbreviation. I was in the vicinity of this code recently for reviewing another topic, but IIRC, 0 came from the UI level does get rounded up to the minimum accepted and never reach "default_abbrev", but if you manage to place 0 or -1 in default_abbrev here (e.g. with additional code, like the above part with the right hand side of the assignment updated), I think the value will propagate throughout the codepath and causes the downstream code to do the right thing. 0 will give you no-abbreviation (i.e. full length depending on the length of the hash) and -1 will give you the "scale as appropriate for the size of the object store". I have mild preference for using 0 over hardcoded single "full length" here. Even though we currently do not plan to allow multiple hashes in use simultaneously in a single invocation of Git, if that ever happens, we will regret hardcoding the_hash_algo->hexsz on the right hand side of the assignment here, like this patch does. Telling the downstream code in the control flow that we want no truncation by using 0 would keep both 40-hexdigit and 64-hexdigit hashes to their original length (as opposed to telling it to truncate at 40 or 64 by using the_hash_algo->hexsz). Thanks. ``` ## Junio C Hamano, 2020-09-01 19:14 Subject: Re: [PATCH] core.abbrev disables abbreviations Message-ID: URL: https://gitlist.dev/e/xmqqtuwhcnrl.fsf%40gitster.c.googlers.com In-Reply-To: ``` Junio C Hamano writes: > Derrick Stolee writes: > >>> + else if (!strcasecmp(value, "false") || >>> + !strcasecmp(value, "no") || >>> + !strcasecmp(value, "off")) >>> + default_abbrev = the_hash_algo->hexsz; >> >> I'm not sure we need three synonyms for "no-abbrev" here. > > I do not particularly mind, but if we imitate the variety of various > boolean false, I'd prefer to see the code to parse them shared to > avoid them drifting apart over time. Just a clarification. - I do not particularly mind having multiple synonyms. - I do mind these one-off strcasecmp that will cause them to drift away from what we do for the boolean 'false'. Thanks. ``` ## brian m. carlson, 2020-09-01 23:37 Subject: Re: [PATCH] core.abbrev disables abbreviations Message-ID: <20200901233752.GC241078@camp.crustytoothpaste.net> URL: https://gitlist.dev/e/20200901233752.GC241078%40camp.crustytoothpaste.net In-Reply-To: ``` On 2020-09-01 at 15:49:55, Junio C Hamano wrote: > I was in the vicinity of this code recently for reviewing another > topic, but IIRC, 0 came from the UI level does get rounded up to the > minimum accepted and never reach "default_abbrev", but if you manage > to place 0 or -1 in default_abbrev here (e.g. with additional code, > like the above part with the right hand side of the assignment > updated), I think the value will propagate throughout the codepath > and causes the downstream code to do the right thing. 0 will give > you no-abbreviation (i.e. full length depending on the length of the > hash) and -1 will give you the "scale as appropriate for the size of > the object store". > > I have mild preference for using 0 over hardcoded single "full > length" here. Even though we currently do not plan to allow > multiple hashes in use simultaneously in a single invocation of Git, > if that ever happens, we will regret hardcoding the_hash_algo->hexsz > on the right hand side of the assignment here, like this patch does. I think we have some commands that accept --abbrev=0 as a value meaning "no abbreviation", because I've touched that code as part of the SHA-256 work. So as far as the option value is concerned, I think it may make sense to use 0 for that meaning and just document it. -- brian m. carlson: Houston, Texas, US ``` ## Junio C Hamano, 2020-12-22 19:42 Subject: Re: [PATCH] core.abbrev disables abbreviations Message-ID: URL: https://gitlist.dev/e/xmqqbleliq58.fsf%40gitster.c.googlers.com In-Reply-To: <20200901144323.GA14554@dcvr> ``` Eric Wong writes: > Thanks for the comments, will wait a few days for comments > before sending out v2. This has seen some review suggestions and as far as I remember, can be summarised as: - there was a rough consensus that this was a desirable feature. - a one-off hardcoded list of "false" would rather want to be consistent with config.c::git_parse_maybe_bool_text(). - documentation is missing for the configuration variable. It has been almost three months; has a v2 been posted that I missed? Thanks. ``` ## Junio C Hamano, 2020-12-22 23:24 Subject: Re: [PATCH] core.abbrev disables abbreviations Message-ID: URL: https://gitlist.dev/e/xmqqk0t9fmq6.fsf%40gitster.c.googlers.com In-Reply-To: <20201222231702.GA17661@dcvr> ``` Eric Wong writes: > Can somebody else take this over? Sorry my brain isn't working > well these months and I'm behind on other things :< Surely and thanks for letting us know. Take care and happy holidays and new year to you. ``` ## Eric Wong, 2020-12-22 23:17 Subject: Re: [PATCH] core.abbrev disables abbreviations Message-ID: <20201222231702.GA17661@dcvr> URL: https://gitlist.dev/e/20201222231702.GA17661%40dcvr In-Reply-To: ``` Junio C Hamano wrote: > Eric Wong writes: > > > Thanks for the comments, will wait a few days for comments > > before sending out v2. > > This has seen some review suggestions and as far as I remember, can > be summarised as: > > - there was a rough consensus that this was a desirable feature. > > - a one-off hardcoded list of "false" would rather want to be > consistent with config.c::git_parse_maybe_bool_text(). > > - documentation is missing for the configuration variable. > > It has been almost three months; has a v2 been posted that I missed? Can somebody else take this over? Sorry my brain isn't working well these months and I'm behind on other things :< ``` ## Junio C Hamano, 2020-12-23 00:10 Subject: Re: [PATCH] core.abbrev disables abbreviations Message-ID: URL: https://gitlist.dev/e/xmqq8s9pfkls.fsf%40gitster.c.googlers.com In-Reply-To: ``` From: Eric Wong Date: Tue, 1 Sep 2020 07:43:55 +0000 Subject: [PATCH] core.abbrev=no disables abbreviations These allows users to write hash-agnostic scripts and configs to disable abbreviations. Using "-c core.abbrev=40" will be insufficient with SHA-256, and "-c core.abbrev=64" won't work with SHA-1 repos today. Signed-off-by: Eric Wong [jc: tweaked implementation, added doc and a test] Signed-off-by: Junio C Hamano --- Documentation/config/core.txt | 2 ++ config.c | 2 ++ t/t3200-branch.sh | 2 ++ 3 files changed, 6 insertions(+) diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt index 160aacad84..c04f62a54a 100644 --- a/Documentation/config/core.txt +++ b/Documentation/config/core.txt @@ -625,4 +625,6 @@ core.abbrev:: computed based on the approximate number of packed objects in your repository, which hopefully is enough for abbreviated object names to stay unique for some time. + If set to "no", no abbreviation is made and the object names + are shown in their full length. The minimum length is 4. diff --git a/config.c b/config.c index 1137bd73af..4c0cf3a1c1 100644 --- a/config.c +++ b/config.c @@ -1217,6 +1217,8 @@ static int git_default_core_config(const char *var, const char *value, void *cb) return config_error_nonbool(var); if (!strcasecmp(value, "auto")) default_abbrev = -1; + else if (!git_parse_maybe_bool_text(value)) + default_abbrev = the_hash_algo->hexsz; else { int abbrev = git_config_int(var, value); if (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index a0b832d59e..67db316911 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -305,7 +305,9 @@ test_expect_success 'git branch --list -v with --abbrev' ' git branch -v --list --no-abbrev t >actual.noabbrev && git branch -v --list --abbrev=0 t >actual.0abbrev && + git -c core.abbrev=no branch -v --list t >actual.noabbrev-conf && test_cmp actual.noabbrev actual.0abbrev && + test_cmp actual.noabbrev actual.noabbrev-conf && git branch -v --list --abbrev=36 t >actual.36abbrev && # how many hexdigits are used? -- 2.30.0-rc1-197-gdf840da780 ``` ## Derrick Stolee, 2020-12-23 14:38 Subject: Re: [PATCH] core.abbrev disables abbreviations Message-ID: URL: https://gitlist.dev/e/ab6e0536-19ed-99b3-a13d-873008b0f70e%40gmail.com In-Reply-To: ``` On 12/22/2020 7:10 PM, Junio C Hamano wrote: > From: Eric Wong > Date: Tue, 1 Sep 2020 07:43:55 +0000 > Subject: [PATCH] core.abbrev=no disables abbreviations > > These allows users to write hash-agnostic scripts and configs to > disable abbreviations. Using "-c core.abbrev=40" will be > insufficient with SHA-256, and "-c core.abbrev=64" won't work > with SHA-1 repos today. > > Signed-off-by: Eric Wong > [jc: tweaked implementation, added doc and a test] > Signed-off-by: Junio C Hamano Thanks for picking this back up. Your version LGTM. -Stolee ``` ## brian m. carlson, 2020-12-23 20:21 Subject: Re: [PATCH] core.abbrev disables abbreviations Message-ID: URL: https://gitlist.dev/e/X%2BOmuBDBHhdzHjzn%40camp.crustytoothpaste.net In-Reply-To: ``` On 2020-12-23 at 00:10:23, Junio C Hamano wrote: > From: Eric Wong > Date: Tue, 1 Sep 2020 07:43:55 +0000 > Subject: [PATCH] core.abbrev=no disables abbreviations > > These allows users to write hash-agnostic scripts and configs to > disable abbreviations. Using "-c core.abbrev=40" will be > insufficient with SHA-256, and "-c core.abbrev=64" won't work > with SHA-1 repos today. > > Signed-off-by: Eric Wong > [jc: tweaked implementation, added doc and a test] > Signed-off-by: Junio C Hamano I agree this looks good, and it's a feature I think is valuable. Thanks to everyone involved. -- brian m. carlson (he/him or they/them) Houston, Texas, US ```