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

PATCH In review Eric Wong 4 participants 13 messages Sep 1, 2020 – Dec 23, 2020 Documentation

Share & tools

Eric Wong

author

2020-09-01 07:43 UTC #0 lore raw

PATCH 1/1 core.abbrev <off|false|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 <[email protected]>

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

config.c +4 -0

index 2bdff4457b..f2e09c72ca 100644
@@ -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:23 UTC #1 lore raw

Quoted 19 lines · 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 <[email protected]>
---
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(+)
config.c +0 -0
index 2bdff4457b..f2e09c72ca 100644

config.c static int git_default_core_config(const char *var, const char *value, void *cb) · line 1223

Quoted 7 lines

config.c +4 -0

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

config.c static int git_default_core_config(const char *var, const char *value, void *cb) · line 1226

Quoted 3 lines

config.c +0 -0

 		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

author

2020-09-01 14:43 UTC #2 lore raw

Quoted 14 lines · Derrick Stolee

config.c static int git_default_core_config(const char *var, const char *value, void *cb) · line 1223
Quoted 11 lines · Eric Wong
index 2bdff4457b..f2e09c72ca 100644
config.c +4 -0
@@ -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.

Quoted 8 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.
Quoted 3 lines
config.c +0 -0
 		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?

Quoted 1 line

Perhaps "core.abbrev = never" would be a good option?

*shrug*

Quoted 3 lines

After we decide on the word, this patch needs:
* Updates to Documentation/config/core.txt

Will do.

Quoted 1 line

* 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 15:00 UTC #3 lore raw

Quoted 6 lines · Eric Wong

Quoted 3 lines · Derrick Stolee
After we decide on the word, this patch needs:
* Updates to Documentation/config/core.txt
Will do.

Thanks.

Quoted 3 lines

Quoted 1 line
* 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-12-22 19:43 UTC #4 lore raw

In reply to Eric Wong

Quoted 2 lines · Eric Wong

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

author

2020-12-22 23:25 UTC #5 lore raw

Quoted 8 lines · Junio C Hamano

Quoted 2 lines · Eric Wong
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:

Quoted 8 lines

- 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-22 23:25 UTC #6 lore raw

Quoted 2 lines · Eric Wong

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

2020-12-23 00:11 UTC #7 lore raw

From: Eric Wong <[email protected]>
Date: Tue, 1 Sep 2020 07:43:55 +0000
Subject: [PATCH] core.abbrev=no disables abbreviations

Signed-off-by Eric Wong <[email protected]>

[jc: tweaked implementation, added doc and a test]

Signed-off-by Junio C Hamano <[email protected]>

---
 Documentation/config/core.txt | 2 ++
 config.c                      | 2 ++
 t/t3200-branch.sh             | 2 ++
 3 files changed, 6 insertions(+)

Documentation/config/core.txt +2 -0

index 160aacad84..c04f62a54a 100644
@@ -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.

config.c +2 -0

index 1137bd73af..4c0cf3a1c1 100644
@@ -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)

t/t3200-branch.sh +2 -0

index a0b832d59e..67db316911 100755
@@ -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?

Signature · 1 line

2.30.0-rc1-197-gdf840da780

Derrick Stolee

2020-12-23 14:39 UTC #8 lore raw

Quoted 4 lines · Junio C Hamano

From: Eric Wong <[email protected]>
Date: Tue, 1 Sep 2020 07:43:55 +0000
Subject: [PATCH] core.abbrev=no disables abbreviations

Quoted 8 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 <[email protected]>
[jc: tweaked implementation, added doc and a test]
Signed-off-by Junio C Hamano <[email protected]>

Thanks for picking this back up. Your version LGTM.

-Stolee

brian m. carlson

2020-12-23 20:22 UTC #9 lore raw

In reply to Junio C Hamano

Quoted 4 lines · Junio C Hamano

From: Eric Wong <[email protected]>
Date: Tue, 1 Sep 2020 07:43:55 +0000
Subject: [PATCH] core.abbrev=no disables abbreviations

Quoted 8 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 <[email protected]>
[jc: tweaked implementation, added doc and a test]
Signed-off-by Junio C Hamano <[email protected]>

I agree this looks good, and it's a feature I think is valuable. Thanks to everyone involved.

Signature · 2 lines

brian m. carlson (he/him or they/them)
Houston, Texas, US

Junio C Hamano

2020-09-01 15:51 UTC #10 lore raw

In reply to Derrick Stolee

Quoted 6 lines · Derrick Stolee

config.c static int git_default_core_config(const char *var, const char *value, void *cb) · line 1223
Quoted 4 lines
config.c +4 -0
+		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.

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

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:15 UTC #11 lore raw

Quoted 9 lines · Junio C Hamano

Quoted 6 lines · Derrick Stolee
config.c static int git_default_core_config(const char *var, const char *value, void *cb) · line 1223
Quoted 4 lines
config.c +4 -0
+		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.

Quoted 3 lines

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:38 UTC #12 lore raw

In reply to Junio C Hamano

Quoted 11 lines · Junio C Hamano

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

Quoted 5 lines

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.

Signature · 1 line

brian m. carlson: Houston, Texas, US