Re: [PATCH] diff-tree: obey the color.ui configuration

From: Christian Couder <christian.couder@gmail.com>
Date: Dec 30, 2017, 23:01 UTC
Message-ID: <CAP8UFD3QhTUj+j3vBGrm0sTQ2dSOLS-m2_PwFj6DZS4VZHKRTQ@mail.gmail.com>
In-Reply-To: <20171230181557.GA30351@sigill.intra.peff.net>

On Sat, Dec 30, 2017 at 7:15 PM, Jeff King <peff@peff.net> wrote:

Show 6 quoted lines

> On Sat, Dec 30, 2017 at 04:04:50PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > I do like the idea of using "show", though. We know the point is to show
>> > the output to the user, so we don't mind at all if the behavior or
>> > output of show changes in future versions (unless we consider the final
>> > output of bisect to be machine-readable, but I certainly don't).

I think that the first line that gives the sha1 of the first bad commit (XXX is the first bad commit) should be machine-readable, because there have been people writing scripts on top of git bisect. Below that I am ok if it is more fancy, especially because it looks like it already used some coloring.

Show 21 quoted lines

>> Not knowing the internal APIs for that well, is this basically a matter
>> of copy/pasting (or factoring out into a function), some of this:
>>
>>     git grep -W cmd_show -- builtin/log.c
>>
>> I.e. boilerplate + calling cmd_log_walk() to yield a result similar to
>> e22278c0a0 ("bisect: display first bad commit without forking a new
>> process", 2009-05-28).
>>
>> Or is it preferred to just fake up argc/argv and call cmd_show()
>> directly? I haven't seen many examples of that in the codebase:
>>
>>     git grep -W '(return|=)\s*cmd.*argc' -- '*.c'
>>
>> But I don't see why it wouldn't work, the cmd_show() doesn't call exit()
>> itself, and we're right about to call exit anyway when our current
>> diff-tree invocation is called.
>
> Hmm, I just assumed we were actually calling diff-tree. But looking at
> that code in bisect, it literally is calling log_tree_commit(), which is
> the same thing that git-show is doing.

Nice. This means that we should be able to make small changes to move from a diff-tree like output to a show like output.

Show 19 quoted lines

> So yet another option is to just set up our options similarly:
>
> diff --git a/bisect.c b/bisect.c
> index 0fca17c02b..1eadecd42a 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -893,9 +893,11 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
>
>         /* diff-tree init */
>         init_revisions(&opt, prefix);
> -       git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
> +       git_config(git_diff_ui_config, NULL);
>         opt.abbrev = 0;
>         opt.diff = 1;
> +       opt.combine_merges = 1;
> +       opt.dense_combined_merges = 1;
>
>         /* This is what "--pretty" does */
>         opt.verbose_header = 1;

I am ok with that kind of changes.

> Though I do kind of like the idea of just delegating to git-show.
> There's no real need for us to have our own logic.

I don't think there is a lot of logic in the above. We are mostly just setting parameters before calling setup_revisions() and log_tree_commit().

If we think that the above is too much "logic", then we should probably try to refactor the revision walking interface to make it simpler, so that not as much "logic" is needed to call it. If this succeeds, then this could help simplify a lot of things throughout the code base.

Show 9 quoted lines

> I think calling cmd_show() from bisect.c is supposed to be forbidden
> (library code shouldn't call up to builtin code). I was going to suggest
> just using run_command() to call git-show. After all, we do this only
> once at the very end of the bisection (which is pretty heavy-weight, as
> it surely has forked a lot of processes to do the actual testing).
>
> But that would be directly undoing Christian's e22278c0a0 (bisect:
> display first bad commit without forking a new process, 2009-05-28). I'm
> of the opinion that would be OK, but maybe Christian has input. :)

In general I am against adding forks that are not necessary and not specially useful, as they don't perform well on some platforms or some machines (like big servers where new processes tend to be allocated to a different CPU).

I know that in this case it happens only once after usually a lot of processing and forking, but I don't think it gives a good example and goes in the right direction.

Yes, it looks ugly to have 10 or 20 lines of code to just set parameters for setup_revisions() and log_tree_commit(), and yes I don't like the revision walking interface, but I think this is a different problem that we should take care of separately.

Thanks for working on this.