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

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

[Eric Wong]

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;
 		else {
 			int abbrev = git_config_int(var, value);
 			if (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz)

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

[Derrick Stolee]

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 <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

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

[Eric Wong]

Derrick Stolee <stolee@gmail.com> 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.

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

[Derrick Stolee]

On 9/1/2020 10:43 AM, Eric Wong wrote:
> 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

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

[Junio C Hamano]

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.

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

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> 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 :<

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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(+)

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

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

[Derrick Stolee]

On 12/22/2020 7:10 PM, Junio C Hamano wrote:
> 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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

On 2020-12-23 at 00:10:23, Junio C Hamano wrote:
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[Junio C Hamano]

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.

> "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.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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.

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]