Re: [PATCH] git: add --no-optional-locks option

From: Jeff King <peff@peff.net>
Date: Sep 21, 2017, 05:08 UTC
Message-ID: <20170921050835.mrbgx2zryy3jusdk@sigill.intra.peff.net>
In-Reply-To: <xmqqbmm4lkf5.fsf@gitster.mtv.corp.google.com>

On Thu, Sep 21, 2017 at 01:55:58PM +0900, Junio C Hamano wrote:

Show 18 quoted lines

> The phrase 'optional lock' does not answer this question clearly,
> though: does it make sense to extend the coverage of this option in
> the future to things more than the "opportunistic update to the
> index file"?
> 
> If the answer is no, then having 'index' instead of 'lock' in the
> name of the option would make more sense (and 'opportunistic' over
> 'optional', too), because what the change is about is to allow other
> processes that are directly interacting with the user to update the
> index, and 'lock' being hindrance is merely an implementation
> detail.  The comment on the "test" in the log message mentions as if
> it were a short-coming that it does not check the lock but checks
> if the index is written, but I think that is testing what matters
> and preferable than testing "did we lock and then unlock it?"
> 
> On the other hand, if the answer is yes, then I am curious what
> other things this may extend to, and if these other things are also
> opportunistic optimizations.

I left it intentionally vague exactly because I thought we might want to leave room for the answer to change to "yes" eventually. For instance, imagine that we had a ref storage format that required periodic compaction, and readers might sometimes choose to compact in order to save future readers from repeating some work they've done. If that compaction means holding a lock even for a brief period, I think it would fall under this option.

I admit that's just adding more hand-waving to the pile. But I don't think it really _hurts_ to leave that door open (aside from making the documentation a bit wishy-washy). And it helps because callers would pick up the new features automatically, without having to learn about a new option.

And I think that's really what this option is. It is less about the caller asking for some specific behavior, and more about them telling Git about the context in which it's executing so it can make intelligent decisions.

And in that sense, something descriptive like --background-process perhaps would be a better name. Except that I couldn't come up with a name that isn't confusing (certainly --background-process implies to me that Git would itself run in the background, which makes no sense here).

I also considered something like "--read-only" to tell Git that we should avoid writing to the repository. But that's really not what this does. It just avoids writes that may cause contention, not all writes.

I also considered using the word "opportunistic" in the option name, but decided it was too long and hard to spell.

So there. I am open to a better name, but I could not come up with one.

> Thanks (and sorry for not being Johannes ;-).

You lack his rugged good looks, but your review was still welcome.

-Peff