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

From: Jeff King <peff@peff.net>
Date: Sep 23, 2017, 03:34 UTC
Message-ID: <20170923033425.6ilykxl5pi7unlj6@sigill.intra.peff.net>
In-Reply-To: <CAGZ79kYJH-4qb5hiT1C_aCFXLcdiJs5h+Z7Goz4trp52C5s3Lw@mail.gmail.com>

On Fri, Sep 22, 2017 at 02:41:06PM -0700, Stefan Beller wrote:

Show 10 quoted lines

> > Ah, thanks. I _thought_ we could already do that but when I went looking
> > for the standard --recursive option I couldn't find it.
> 
> Thanks for checking for submodules there.
> 
> I personally prefer --recurse-submodules despite the longer name,
> because a plain recursive option can mean anything in a sufficiently
> complex program such as Git (recurse into the tree (c.f. ls-tree), or
> for the algorithm used (c.f. merge, diff) or yet another dimension
> I did not think of).

Yeah, I agree that's a better name (mentioning --recursive is probably just showing my general ignorance of submodules).

Show 12 quoted lines

> > So yes, I would think we would want this option to apply recursively in
> > that case, even when we cross repository boundaries.
> 
> Regarding the actual patch which is heavily aimed at coping with IDEs
> despite the command line being used, I wonder how many IDEs pass
> --ignore-submodules and recurse themselves (if needed). Reason for
> my suspicion is [1] which does pay attention to submodules:
> 
> >    Our application calls status including the following flags:
> >    --porcelain=v2 --ignored --untracked-files=all --ignore-submodules=none
> 
> [1] https://public-inbox.org/git/2bbb1d0f-ae06-1878-d185-112bd51f75c9@gmail.com/

Yeah, I think that's probably Visual Studio (which is what Johannes presumably wrote the original patch for). GitHub Desktop does not currently pass it, though perhaps should consider doing so (though of course the user could tweak their config, too).

Show 5 quoted lines

> There might be another option to cope with the situation:
> 
>  4. Teach all commands to spinlock / busywait shortly for important
>      locks instead of giving up. In that case git-status rewriting
>      the index ought to be fine?

We do have all the infrastructure in place to do a reasonable busywait with backoff. I think the patch is roughly just:

diff --git a/read-cache.c b/read-cache.c
index 65f4fe8375..fc1ba122a3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1563,7 +1563,8 @@ static int read_index_extension(struct index_state *istate,
 
 int hold_locked_index(struct lock_file *lk, int lock_flags)
 {
-	return hold_lock_file_for_update(lk, get_index_file(), lock_flags);
+	return hold_lock_file_for_update_timeout(lk, get_index_file(),
+						 lock_flags, 500);
 }
 
 int read_index(struct index_state *istate)

though I think there are a few sites which manually call
hold_lock_file_for_update() on the index that would need similar
treatment.

I suspect it would work OK in practice, unless your index is so big that
500ms isn't enough. The user may also see minor stalls when there's lock
contention. I'm not sure how annoying that would be.

-Peff