Re: [PATCH 3/3] bisect: make diff-tree output prettier

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2019, 17:49 UTC
Message-ID: <xmqqsgwfr9vr.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190222062327.GC10248@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 21 quoted lines

> After completing a bisection, we print out the commit we found using an
> internal version of diff-tree. The result is aesthetically lacking:
>
>   - it shows a raw diff, which is generally less informative for human
>     readers than "--stat --summary" (which we already decided was nice
>     for humans in format-patch's output).
>
>   - by not abbreviating hashes, the result is likely to wrap on most
>     people's terminals
>
>   - we don't use "-r", so if the commit touched files in a directory,
>     you only get to see the top-level directory mentioned
>
>   - we don't specify "--cc" or similar, so merges print nothing (not
>     even the commit message!)
>
> Even though bisect might be driven by scripts, there's no reason to
> consider this part of the output as machine-readable (if anything, the
> initial "$hash is the first bad commit" might be parsed, but we won't
> touch that here). Let's make it prettier and more informative for a
> human reading the output.

Sounds very sensible. One potential point that makes me worried is this move might tempt people to make the output even larger (e.g. a full diff with "--patch" is overkill if done unconditionally).

Show 10 quoted lines

> While we're tweaking the options, let's also switch to using the diff
> "ui" config. If we're accepting that this is human-readable output, then
> we should respect the user's options for how to display it.
> ...
> If we do care about the change in exit code from bisect, then it
> probably does make sense to go with an external process. Then it can
> happily die on the corruption, while bisect continues with the rest of
> the high-level operation. I'm not sure it really matters much, though.
> Once your repository is corrupted, all bets are off. It's nice that we
> can bisect in such a state at all.

This is about showing the very final message after finding which one is the culprit. Is there any other "clean-up" action we need to do after showing the message? I do not care too much about the exit code from the bisection, but if dying from diff-tree can interfere with such a clean-up, that would bother me a lot more, and at that point, given especially that this is not a performance sensitive thing at all (it is not even invoked log(n) times---just once at the end), moving to external process may make it a lot simpler and cleaner.