git bisect - good vs bad output is different.

git bisect - good vs bad output is different.

[Bartosz Baranowski]

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

Re: git bisect - good vs bad output is different.

[Junio C Hamano]

Bartosz Baranowski <bbaranow@redhat.com> writes:

> 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


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"?

[PATCH 0/3] prettier bisect output

[Jeff King]

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

> > git bisect good
> > 3a9388eef42efc87c78ce22158d55e69a278b4eb is the first bad commit
> >
> > git --version
> > git version 2.14.1
> 
> 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"?

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

[PATCH 1/3] bisect: use string arguments to feed internal diff-tree

[Jeff King]

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

diff --git a/bisect.c b/bisect.c
index 3af955c4bc..8c81859835 100644
--- a/bisect.c
+++ b/bisect.c
@@ -896,24 +896,15 @@ static void show_diff_tree(struct repository *r,
 			   const char *prefix,
 			   struct commit *commit)
 {
+	const char *argv[] = {
+		"diff-tree", "--pretty", "--no-abbrev", "--raw", NULL
+	};
 	struct rev_info opt;
 
-	/* diff-tree init */
 	repo_init_revisions(r, &opt, prefix);
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
-	opt.abbrev = 0;
-	opt.diff = 1;
 
-	/* This is what "--pretty" does */
-	opt.verbose_header = 1;
-	opt.use_terminator = 0;
-	opt.commit_format = CMIT_FMT_DEFAULT;
-
-	/* diff-tree init */
-	if (!opt.diffopt.output_format)
-		opt.diffopt.output_format = DIFF_FORMAT_RAW;
-
-	setup_revisions(0, NULL, &opt, NULL);
+	setup_revisions(ARRAY_SIZE(argv) - 1, argv, &opt, NULL);
 	log_tree_commit(&opt, commit);
 }
 
-- 
2.21.0.rc2.577.g06bbe9cbd1

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

[Jeff King]

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

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);
-- 
2.21.0.rc2.577.g06bbe9cbd1

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

[Christian Couder]

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.

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

[Jeff King]

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

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

[Jeff King]

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

diff --git a/bisect.c b/bisect.c
index b04d7b2f63..e87ac29a51 100644
--- a/bisect.c
+++ b/bisect.c
@@ -897,11 +897,11 @@ static void show_diff_tree(struct repository *r,
 			   struct commit *commit)
 {
 	const char *argv[] = {
-		"diff-tree", "--pretty", "--no-abbrev", "--raw", NULL
+		"diff-tree", "--pretty", "--stat", "--summary", "--cc", NULL
 	};
 	struct rev_info opt;
 
-	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
+	git_config(git_diff_ui_config, NULL);
 	repo_init_revisions(r, &opt, prefix);
 
 	setup_revisions(ARRAY_SIZE(argv) - 1, argv, &opt, NULL);
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 55835ee4a4..49a394bd75 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -681,7 +681,7 @@ test_expect_success 'bisect: --no-checkout - target in breakage' '
 	check_same BROKEN_HASH6 BISECT_HEAD &&
 	git bisect bad BISECT_HEAD &&
 	check_same BROKEN_HASH5 BISECT_HEAD &&
-	git bisect good BISECT_HEAD &&
+	test_must_fail git bisect good BISECT_HEAD &&
 	check_same BROKEN_HASH6 bisect/bad &&
 	git bisect reset
 '
@@ -692,7 +692,7 @@ test_expect_success 'bisect: --no-checkout - target after breakage' '
 	check_same BROKEN_HASH6 BISECT_HEAD &&
 	git bisect good BISECT_HEAD &&
 	check_same BROKEN_HASH8 BISECT_HEAD &&
-	git bisect good BISECT_HEAD &&
+	test_must_fail git bisect good BISECT_HEAD &&
 	check_same BROKEN_HASH9 bisect/bad &&
 	git bisect reset
 '
@@ -701,7 +701,7 @@ test_expect_success 'bisect: demonstrate identification of damage boundary' "
 	git bisect reset &&
 	git checkout broken &&
 	git bisect start broken master --no-checkout &&
-	git bisect run \"\$SHELL_PATH\" -c '
+	test_must_fail git bisect run \"\$SHELL_PATH\" -c '
 		GOOD=\$(git for-each-ref \"--format=%(objectname)\" refs/bisect/good-*) &&
 		git rev-list --objects BISECT_HEAD --not \$GOOD >tmp.\$\$ &&
 		git pack-objects --stdout >/dev/null < tmp.\$\$
-- 
2.21.0.rc2.577.g06bbe9cbd1

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

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

[Jeff King]

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

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

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.

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

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

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

[Christian Couder]

On Sat, Feb 23, 2019 at 2:44 PM Jeff King <peff@peff.net> wrote:
>
> On Fri, Feb 22, 2019 at 09:49:44AM -0800, Junio C Hamano wrote:
>

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

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)

Re: [PATCH 0/3] prettier bisect output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

Re: [PATCH 0/3] prettier bisect output

[Christian Couder]

On Fri, Feb 22, 2019 at 7:19 AM Jeff King <peff@peff.net> wrote:
>
> On Thu, Feb 21, 2019 at 01:39:46PM -0800, Junio C Hamano wrote:

> > 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"?
>
> 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".

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

> 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

I am OK with the above patches.

Thanks,
Christian.


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

Re: [PATCH 0/3] prettier bisect output

[Jeff King]

On Sun, Mar 03, 2019 at 07:33:17PM +0100, Christian Couder 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".
> 
> Yeah, I am still OK with tweaking the output, though I'd rather not go
> back to using an external process.

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