Re: [PATCH 1/3] repository: create disable_replace_refs()

From: Jeff King <peff@peff.net>
Date: Jun 1, 2023, 17:47 UTC
Message-ID: <20230601174746.GA4165405@coredump.intra.peff.net>
In-Reply-To: <ae89feda-0a76-29d7-14ce-662214414638@github.com>

On Wed, May 31, 2023 at 09:37:10AM -0400, Derrick Stolee wrote:

Show 10 quoted lines

> > In this place and several others in the file, you opt to not just
> > replace the assignment with a function call, but move the action line
> > to later in the file.  In some cases, much later.
> > 
> > I don't think it hurts things, but it certainly makes me wonder why it
> > was moved.  Did it break for some reason when called earlier?  (Is
> > there something trickier about this patch than I expected?)
> 
> Generally, I decided to move it after option-parsing, so it wouldn't
> be called if we are hitting an option-parse error.

Playing devil's advocate: would option parsing ever access an object? I think in most cases the answer is no, but I could imagine it happening for some special cases (e.g., update-index uses callbacks to act on options as we parse them, since order is important).

So I think as a general principle it makes sense for commands to set this flag as early as possible.

Show 6 quoted lines

> However, these moves were only important for a draft version where
> I had not separated the global and local scopes, so calling the method
> would also load config.
> 
> In this version of the patch, this is not needed at all, and I could
> do an in-line replacement. Thanks!

It sounds like you were going to switch the locations back anyway, but maybe the above gives an extra motivation. :)

-Peff