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

quoted

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.

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