Re: [PATCH 3/3] abbrev: auto size the default abbreviation
- From
Jeff King <peff@peff.net>
- Date
- Nov 2, 2016, 02:12 UTC
- Message-ID
- <20161102021205.xyxnlxxx4nufvhor@sigill.intra.peff.net>
- In-Reply-To
- <xmqqmvhimzrh.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 01, 2016 at 06:33:38PM -0700, Junio C Hamano wrote:
Show 22 quoted lines
> Jeff King <peff@peff.net> writes: > > > On Fri, Sep 30, 2016 at 05:19:37PM -0700, Junio C Hamano wrote: > > > >> Introduce a mechanism, where we estimate the number of objects in > >> the repository upon the first request to abbreviate an object name > >> with the default setting and come up with a sane default for the > >> repository. Based on the expectation that we would see collision in > >> a repository with 2^(2N) objects when using object names shortened > >> to first N bits, use sufficient number of hexdigits to cover the > >> number of objects in the repository. Each hexdigit (4-bits) we add > >> to the shortened name allows us to have four times (2-bits) as many > >> objects in the repository. > > I was idly browsing the draft release notes and then documentation > and noticed that, even though the new default is to auto-scale, > there is no mention of that in the documentation and there is no way > to explicitly ask for auto-scaling. > > I wonder if we want to have something like this. I actually am > inclined to drop the change to config.c and remove the new mention > of "auto" in the documentation.
I doubt anybody cares that much either way, but in theory core.abbrev=auto is a way to override core.abbrev=10 in /etc/gitconfig or something. Though I'm having trouble envisioning a case where anybody would set it in /etc/gitconfig, or why somebody would then want to override that back to auto.
So I think it is fine either way (but I do agree that the core.abbrev needs _some_ update to mention the auto-scaling behavior).
Show 22 quoted lines
> diff --git a/config.c b/config.c
> index 83fdecb1bc..c363cca4a9 100644
> --- a/config.c
> +++ b/config.c
> @@ -834,10 +834,16 @@ static int git_default_core_config(const char *var, const char *value)
> }
>
> if (!strcmp(var, "core.abbrev")) {
> - int abbrev = git_config_int(var, value);
> - if (abbrev < minimum_abbrev || abbrev > 40)
> - return -1;
> - default_abbrev = abbrev;
> + if (!value)
> + return config_error_nonbool(var);
> + if (!strcasecmp(value, "auto"))
> + default_abbrev = -1;
> + else {
> + int abbrev = git_config_int(var, value);
> + if (abbrev < minimum_abbrev || abbrev > 40)
> + return -1;
> + default_abbrev = abbrev;
> + }This isn't a new problem you added, but that "return -1" would probably leave people confused:
$ git -c core.abbrev=2 log fatal: unable to parse 'core.abbrev' from command-line config
Probably something like:
return error("abbrev length out of range: %d", abbrev);would be more descriptive. I doubt it's a big deal in practice, though (why would you set core.abbrev to something silly in the first place?).
-Peff