git bisect - good vs bad output is different.

Depending how you end bisecting, result/report is either one liner or dumps commit information:

$ git bisect bad
77c044d8d66f9f9bebdb805853409e920e537d59 is the first bad commit
commit 77c044d8d66f9f9bebdb805853409e920e537d59
Author: XXXX
Date:   Tue Jan 22 09:24:02 2019 -0500

   ISSUE-11626 Bad fish in the sea of comment

:040000 040000 ef2280aa5f7e0c23f8750c43a0bf05c0a9639fe3 f63bea979784cade7dffd653d939f665ff6a53b7 M clustering :040000 040000 6f4667c819106f6e9ddbb902253862212a2558f5 4d8c4dc85872c0665eb77957a6fd69c49c173188 M triton :040000 040000 0400075b3d5f7cb9e68683b25b8ede93fb1b293b 35ba4c831c4834f9ce612e394621dde382bc72f1 M web-common

vs

git bisect good 3a9388eef42efc87c78ce22158d55e69a278b4eb is the first bad commit

git --version git version 2.14.1

Bartosz Baranowski <bbaranow@redhat.com> writes:

Are you sure (and if so how did you reach that conclusion) that the above difference comes from the last 'bad' vs 'good' you finished, and not comes from the difference between 77c044d8 vs 3a9388ee?

At the end of the bisection session, bisect.c::show_diff_tree() is called on that "culprit" commit. Is it possible that 3a9388ee is a simple and trivial merge that does not have anything worth reporting for "git diff-tree"?

On Thu, Feb 21, 2019 at 01:39:46PM -0800, Junio C Hamano wrote:

I've run across this many times, too. Since it's been bugging me for a decade, I thought I'd finally try to address it. Here are some patches.

There was some discussion about a year ago about just using "git show" for this output:

  https://public-inbox.org/git/CAP8UFD3QhTUj+j3vBGrm0sTQ2dSOLS-m2_PwFj6DZS4VZHKRTQ@mail.gmail.com/

Christian seemed generally OK with tweaking the output, but preferred not to move all the way to running an external "git show". I'm not sure I completely agree, but it was easy enough to get the results I wanted just by fiddling the current code a bit. ;)

  [1/3]: bisect: use string arguments to feed internal diff-tree
  [2/3]: bisect: fix internal diff-tree config loading
  [3/3]: bisect: make diff-tree output prettier

 bisect.c                    | 19 +++++--------------
 t/t6030-bisect-porcelain.sh |  6 +++---
 2 files changed, 8 insertions(+), 17 deletions(-)

-Peff

Commit e22278c0a0 (bisect: display first bad commit without forking a new process, 2009-05-28) converted our external call to diff-tree to an internal use of the log_tree_commit(). But rather than individually setting options in the rev_info struct (and explaining in comments how they map to command-line options), we can just pass the command-line options to setup_revisions().

This is shorter, easier to change, and less likely to break if revision.c internals change.

Note that we unconditionally set the output format to "raw". The conditional in the original code didn't actually do anything useful, since nobody had an opportunity to set the format to anything.

Signed-off-by: Jeff King <peff@peff.net>
---
 bisect.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

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(-)

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

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?

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:

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.

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

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.

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.

Note that we have to touch a few tests in t6030. These check bisection in a corrupted repository (it's missing a subtree). They didn't fail with the previous code, because it didn't actually recurse far enough in the diff to find the broken tree. But now we'll see the corruption and complain.

Adjusting the tests to expect the die() is the best fix. We still confirm that we're able to bisect within the broken repo. And we'll still print "$hash is the first bad commit" as usual before dying; showing that is a reasonable outcome in a corrupt repository (and was what might happen already, if the root tree was corrupt).

Signed-off-by: Jeff King <peff@peff.net>
---
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.

 bisect.c                    | 4 ++--
 t/t6030-bisect-porcelain.sh | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

Jeff King <peff@peff.net> writes:

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).

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.

On Fri, Feb 22, 2019 at 09:49:44AM -0800, Junio C Hamano wrote:

Yeah, I agree that would be overkill. What I have here isn't actually much bigger. It mostly trades one line of --raw for one line of --stat. The big change is that we actually bother to recurse, but I think the old behavior was just buggy.

At any rate, I think we can discuss such a patch if and when it comes up.

Thanks, I had a vague feeling along these lines, but you nicely put it into words. As far as I can tell, no, we're not missing any important cleanup in that process; it looks like the only call to show_diff_tree() then calls exit(10) immediately after.

However, that does change our exit code, which git-bisect.sh then propagates instead of writing the entry into the BISECT_LOG.

I'm still not convinced this is really worth caring about, as it implies a corrupt repo. But if we did want to fix it, then I think the external diff-tree would still be the right thing (since it's hard for git-bisect.sh to tell the different between this death and another one).

-Peff

On Sat, Feb 23, 2019 at 2:44 PM Jeff King <peff@peff.net> wrote:

I don't care much about what happens in a corrupt repo, but I am adding Jon Seymour in CC who wrote those tests in:

d3dfeedf2e (bisect: add tests to document expected behaviour in presence of broken trees., 2011-08-04) b704a8b3fd (bisect: add tests for the --no-checkout option., 2011-08-04)

Jeff King <peff@peff.net> writes:

Looks good from a quick glance.

One unrelated thing that made me curious was that the output from

	git grep 'is the first '

had these two lines:

bisect.c: printf("%s is the first %s commit\n", oid_to_hex(bisect_rev), git-bisect.sh: if sane_grep "is the first $TERM_BAD commit" "$GIT_DIR/BISECT_RUN" >/dev/null

which means that we cannot localize this message without thought, unlike the usual "hey, this is end-user facing, so wrap it in _() out of spinal reflex."

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

Yeah, I am still OK with tweaking the output, though I'd rather not go back to using an external process.

I am OK with the above patches.

Thanks, Christian.

On Sun, Mar 03, 2019 at 07:33:17PM +0100, Christian Couder wrote:

By the way, I've done a few bisects since writing this, and I've been very happy with the result. So I endorse my own patch. ;)

-Peff