threads / patch / 54169

patchcore.abbrev <off|false|no> disables abbreviations

Subject: [PATCH] core.abbrev <off|false|no> disables abbreviations

## tl;dr

13 messages between Sep 1, 2020 and Dec 23, 2020. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Eric Wong· Sep 1, 2020, 07:43 UTC · lore

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 <e@80x24.org>
---
  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(+)
Show changes to config.c +4 −0
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· Sep 1, 2020, 12:14 UTC · re: Eric Wong · lore

Re: [PATCH] core.abbrev <off|false|no> disables abbreviations

On 9/1/2020 3:43 AM, Eric Wong wrote:
Show 26 quoted lines
> 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 <e@80x24.org>
> ---
>   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· Sep 1, 2020, 14:43 UTC · re: Derrick Stolee · lore

Re: [PATCH] core.abbrev <off|false|no> disables abbreviations

Derrick Stolee <stolee@gmail.com> wrote:
Show 14 quoted lines
> 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.

Show 8 quoted lines
> "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· Sep 1, 2020, 14:59 UTC · re: Eric Wong · lore

Re: [PATCH] core.abbrev <off|false|no> disables abbreviations

On 9/1/2020 10:43 AM, Eric Wong wrote:
Show 6 quoted lines
> Derrick Stolee <stolee@gmail.com> 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· Dec 22, 2020, 19:42 UTC · re: Eric Wong · lore

Re: [PATCH] core.abbrev <off|false|no> disables abbreviations

Eric Wong <e@80x24.org> 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.
Eric Wong· Dec 22, 2020, 23:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] core.abbrev <off|false|no> disables abbreviations

Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
> Eric Wong <e@80x24.org> 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· Dec 22, 2020, 23:24 UTC · re: Eric Wong · lore

Re: [PATCH] core.abbrev <off|false|no> disables abbreviations

Eric Wong <e@80x24.org> 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.

Junio C Hamano· Dec 23, 2020, 00:10 UTC · re: Junio C Hamano · lore

Re: [PATCH] core.abbrev <off|false|no> disables abbreviations

From: Eric Wong <e@80x24.org>
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 <e@80x24.org>
[jc: tweaked implementation, added doc and a test]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config/core.txt | 2 ++
 config.c                      | 2 ++
 t/t3200-branch.sh             | 2 ++
 3 files changed, 6 insertions(+)
Show changes to 3 files +6 −0

Documentation/config/core.txt, config.c, t/t3200-branch.sh

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· Dec 23, 2020, 14:38 UTC · re: Junio C Hamano · lore

Re: [PATCH] core.abbrev <off|false|no> disables abbreviations

On 12/22/2020 7:10 PM, Junio C Hamano wrote:
Show 12 quoted lines
> From: Eric Wong <e@80x24.org>
> 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 <e@80x24.org>
> [jc: tweaked implementation, added doc and a test]
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Thanks for picking this back up. Your version LGTM.
-Stolee
brian m. carlson· Dec 23, 2020, 20:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] core.abbrev <off|false|no> disables abbreviations

On 2020-12-23 at 00:10:23, Junio C Hamano wrote:
Show 12 quoted lines
> From: Eric Wong <e@80x24.org>
> 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 <e@80x24.org>
> [jc: tweaked implementation, added doc and a test]
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

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
Junio C Hamano· Sep 1, 2020, 15:49 UTC · re: Derrick Stolee · lore

Re: [PATCH] core.abbrev <off|false|no> disables abbreviations

Derrick Stolee <stolee@gmail.com> writes:
Show 6 quoted lines
>> +		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· Sep 1, 2020, 19:14 UTC · re: Junio C Hamano · lore

Re: [PATCH] core.abbrev <off|false|no> disables abbreviations

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> Derrick Stolee <stolee@gmail.com> 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· Sep 1, 2020, 23:37 UTC · re: Junio C Hamano · lore

Re: [PATCH] core.abbrev <off|false|no> disables abbreviations

On 2020-09-01 at 15:49:55, Junio C Hamano wrote:
Show 16 quoted lines
> 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

← back to recent threads