Re: [PATCH 10/20] diff-parseopt: convert --[no-]abbrev

[Duy Nguyen <pclouds@gmail.com>]

On Thu, Mar 21, 2019 at 6:00 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Wed, Mar 20 2019, Nguyễn Thái Ngọc Duy wrote:
>
> > [...]And the '40' change is self explanatory.
>
> Let me make an attempt at being dense anyway...
>
> > -             else if (v > 40)
> > -                     v = 40;
> > +             else if (v > the_hash_algo->hexsz)
> > +                     v = the_hash_algo->hexsz;
> >       }
>
> This is obviously not a regression, it's a hardcoded 40 *now*. So we
> should take this patch.
>
> But in general, I wonder how this is going to work once we get a few
> steps further into the SHA-256 migration. I.e. here we're still parsing
> the command-line, and the_hash_algo might be initialized early to SHA-1.

That would be wrong. the_hash_algo must be properly initialized by the
time any command parsing is done (except maybe "git <options> <cmd>").
While parse_options() most of the time is just a dumb "set this
variable, set that variable", it often can have callbacks to do more
complicated stuff and we can't just go with "pre-initialized to SHA-1"
assumption. That's as bad as "assume $CWD is worktree" until worktree
is discovered.

There is a corner case though. If some command takes hash algo as an
option (e.g. git hash-object should work without a repo) then yes we
might have a problem since the_hash_algo might not be initialized yet,
depending on option order.

> So if I set --abbrev=45 it'll be trimmed to --abbrev=40 by this code.
>
> But then shortly afterwards we pass my SHA-256 object down to some
> machinery, and will then want to abbreviate it.
>
> Isn't that part of the code something we're going to want to support
> looking up objects in either hash, even if we initially started out with
> SHA-1 in the_hash_algo? So we'll be over-abbreviating a SHA-256 object.
>
> Leaving aside the sillyness of wanting to abbreviate *anything* to 45
> characters, I wonder how those sorts of chicken & egg hash scenarios
> will go involving the_hash_algo.
-- 
Duy