| Message ID | 20200901074355.GA4498@dcvr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | core.abbrev <off|false|no> disables abbreviations | expand |
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
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.
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
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.
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.
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.
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.
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 :<
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.
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?
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
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.
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)
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(+)