Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 29, 2016, 23:13 UTC
Message-ID: <xmqqbmz6cna5.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CA+55aFyVEQ+8TBBUm5KG9APtd9wy8cp_mRO=3nj12DXZNLAC9A@mail.gmail.com>

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 6 quoted lines

> Somebody should really double-check my heuristics, to see that I did
> the pack counting etc right.  It doesn't do alternate loose file
> counting at all, and maybe it could matter.  The advantage of the
> previous patch was that it got the object counting right almost
> automatically, this actually has its own new object counting code and
> maybe I screwed it up.

One thing that worries me is if we are ready to start accessing the object store in all codepaths when we ask for DEFAULT_ABBREV. The worries are twofold:

 (1) Do we do the right thing if object store is not available to
     us?  Some commands can be run outside repository, and if our
     call to prepare_packed_git() or loose object iteration barfed
     in some way, that would introduce a regression.

 (2) Is calling prepare_packed_git() too early interfere with how
     the commands expect its own prepare_packed_git() work?  That
     is, if a command has this sequence, "ask DEFAULT_ABBREV,
     arrange things, and then call prepare_packed_git()", and the
     existing "arrange things" step had something that causes a new
     pack to become eligible to be read by prepare_packed_git(),
     like adding to the list of alternate object stores, its own
     prepare_packed_git() will now become a no-op.

I browsed through "tig grep DEFAULT_ABBREV \*.c" and it seems that in majority of the hits, we not just are ready to start accessing, but already have an object or two, which must have come from an already open object store, so they are OK. Especially the ones that use it as the last argument to find_unique_abbrev() are OK as we are about to open the object store to do the computation.

There are very early ones in the program startup sequence in the following functions, but I do not think of a reason why our new and early call to prepare_packed_git() might be problematic, given that all of them require us to have an access to the repository (i.e. this change cannot introduce a regression where a command used to work outside a repository but barf when prepare_packed_git() is called early):

 - builtin/describe.c
 - builtin/rev-list.c
 - builtin/rev-parse.c

I thought that the one in diff.c might be problematic when the "git diff" command is run outside a repository with the "--no-index" option, but it appears that init_default_abbrev() seems to be OK when run outside a repository.

There is one in parse-options-cb.c that is used to parse the --abbrev command line option. This might cause a cosmetic problem but when the user is asking for an abbreviation, it is expected that we will have an access to the object store anyway, so it may be OK.

I am sorry that none of the above is about your math ;-) I suck at math so I won't comment.