Re: [PATCH 2/3] bisect: fix internal diff-tree config loading

From: Christian Couder <christian.couder@gmail.com>
Date: Mar 3, 2019, 17:59 UTC
Message-ID: <CAP8UFD0+MSaU4KmD_pfHnCDFoqr9H99Fp9tBP-Qw+vs+ambgFg@mail.gmail.com>
In-Reply-To: <20190222062133.GB10248@sigill.intra.peff.net>

On Fri, Feb 22, 2019 at 7:21 AM Jeff King <peff@peff.net> wrote:

Show 11 quoted lines

>
> When we run our internal diff-tree to show the bisected commit, we call
> init_revisions(), then load config, then setup_revisions(). But that
> order is wrong: we copy the configured defaults into the rev_info struct
> during the init_revisions step, so our config load wasn't actually doing
> anything.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> It does feel a little weird loading config at all here, since it would
> potentially affect other in-process operations.

I like that this patch fixes a bug, but this still triggers some wondering/comments.

Would it be better or at least less weird to load it at the beginning of `git bisect`?

Or is the real problem a limitation of the config system, that prevent us from temporarily loading, and then maybe unloading, some config?

> This is where an
> external process might be cleaner.

It depends on which definition of clean we use. Yeah, having many global variables and not caring because we launch many external processes that will "clean" things up when they exit can seem "clean" for some time. But I think we are past this point now, and I still wouldn't like us to go back to our previous way of doing things even here. So thanks for not using an external process.

Thanks for working on this, Christian.