Re: [PATCH 1/3] check_replace_refs: fix outdated comment

From: Jeff King <peff@peff.net>
Date: Jul 18, 2018, 22:52 UTC
Message-ID: <20180718225242.GB21451@sigill.intra.peff.net>
In-Reply-To: <xmqqlga8i2ux.fsf@gitster-ct.c.googlers.com>

On Wed, Jul 18, 2018 at 03:41:10PM -0700, Junio C Hamano wrote:

Show 21 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > Commit afc711b8e1 (rename read_replace_refs to
> > check_replace_refs, 2014-02-18) added a comment mentioning
> > that check_replace_refs is set in two ways:
> >
> >   - from user intent via --no-replace-objects, etc
> >
> >   - after seeing there are no replace refs to respect
> >
> > Since c3c36d7de2 (replace-object: check_replace_refs is safe
> > in multi repo environment, 2018-04-11) the second is no
> > longer true. Let's drop that part of the comment.
> >
> 
> I wonder if c3c36d7de2 should be corrected so that we would have
> this bit per in-core repository instance?  When the superproject and
> its three submodules all have an in-core repository instance each,
> and only one of the submodules uses replace ref, the original
> optimization disabled by that patch would be an obvious thing to
> have per repository.

I think c3c36d7de2 represents that bit by checking the non-emptiness of the refs/replace map.

What it doesn't do is represent a per-repository notion of "are we even even interested in the replace feature". Right now there are probably few cases where that would matter. It comes from the environment, so it's automatically passed down to children anyway. You could tweak the environment, but if you did so you'd probably be in a separate process anyway, so none of this multi-repo-in-one-process stuff would matter either way. But there may be cases with programs which manually tweak the value and then recurse into a submodule.

It would be made slightly worse with my config patch, if you expected:

  git config core.usereplacerefs true
  git -C submodule core.usereplacerefs false
  git log --submodule=log

to respect replace refs in the outer repo, but not the inner one.

-Peff