Re: [PATCH 1/2] config: fix segfault when parsing "core.abbrev" without repo

From: Patrick Steinhardt <ps@pks.im>
Date: Jun 12, 2024, 05:55 UTC
Message-ID: <Zmk4PvvBMwEzePrk@tanuki>
In-Reply-To: <xmqqh6dzwek0.fsf@gitster.g>

On Tue, Jun 11, 2024 at 10:54:23AM -0700, Junio C Hamano wrote:

Show 33 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Fix both of these issues by not making it an error anymore when the
> > given length exceeds the hash length. Instead, if we have a repository,
> > then we truncate the length to the maximum length of `the_hash_algo`.
> > Otherwise, we simply leave the abbreviated length intact and store it
> > as-is. This is equivalent to the logic in `parse_opt_abbrev_cb()` and is
> > handled just fine by `repo_find_unique_abbrev_r()`. In practice, we
> > should never even end up using `default_abbrev` without a repository
> > anyway given that abbreviating object IDs to unique prefixes requires us
> > to have access to an object database.
> 
> Makes sense.
> 
> > diff --git a/config.c b/config.c
> > index abce05b774..ab2844d9e1 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -1460,11 +1460,14 @@ static int git_default_core_config(const char *var, const char *value,
> >  		if (!strcasecmp(value, "auto"))
> >  			default_abbrev = -1;
> >  		else if (!git_parse_maybe_bool_text(value))
> > -			default_abbrev = the_hash_algo->hexsz;
> > +			default_abbrev = startup_info->have_repository ?
> > +				the_hash_algo->hexsz : GIT_MAX_HEXSZ;
> 
> We will need to have some code that further adjusts overly long
> default_abbrev when we really have to abbreviate (at which time,
> hopefully we are already aware of the real hash algorithm used in
> the repository, and that may be SHA-1) anyway.
> 
> So do we even need the conditional here?  Can't we just set it to
> GIT_MAX_HEXSZ here unconditionally?

Not really. I was erring on the safe side here to retain the status quo for all relevant cases. As explained in the commit message, the length is only relevant when we have a repository because we otherwise wouldn't be able to abbreviate anything anyway. So essentially, this change is a functional no-op.

There's also the question of the second commit, where we only handle `abbrev == the_hash_algo->size`, but not `abbrev > the_hash_algo->size`. It works, but is slower when not truncating the length until the second patch fixes it.

So yes, we can set this unconditionally to `GIT_MAX_HEXSZ`, but out of abundance of caution I decided to make this conditional.

Patrick