| Message ID | 20190222062133.GB10248@sigill.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | prettier bisect output | expand |
On Fri, Feb 22, 2019 at 7:21 AM Jeff King <peff@peff.net> wrote: > > 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.
On Sun, Mar 03, 2019 at 06:59:19PM +0100, Christian Couder wrote: > On Fri, Feb 22, 2019 at 7:21 AM Jeff King <peff@peff.net> wrote: > > > > 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`? I guess you mean at the beginning of bisect--helper? That would be OK with me, too, and maybe would be a little less weird. But if bisect is slowly becoming a single binary, maybe we should just wait for that. > Or is the real problem a limitation of the config system, that prevent > us from temporarily loading, and then maybe unloading, some config? It's less the config system, and more the way Git is written. ;) Typically parsing the config means setting a bunch of globals, and forgetting what their original values are. That's not something the config system can fix. -Peff
diff --git a/bisect.c b/bisect.c index 8c81859835..b04d7b2f63 100644 --- a/bisect.c +++ b/bisect.c @@ -901,8 +901,8 @@ static void show_diff_tree(struct repository *r, }; struct rev_info opt; - repo_init_revisions(r, &opt, prefix); git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ + repo_init_revisions(r, &opt, prefix); setup_revisions(ARRAY_SIZE(argv) - 1, argv, &opt, NULL); log_tree_commit(&opt, commit);
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. This is where an external process might be cleaner. bisect.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)