On Thu, Mar 21, 2019 at 6:00 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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
