| Message ID | 20190222062327.GC10248@sigill.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | prettier bisect output | expand |
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.
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
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)
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.\$\$
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(-)