{"thread":{"id":"50549","subject":"git bisect - good vs bad output is different.","startedAt":"2019-02-21T10:47:42Z","lastAt":"2019-03-05T04:16:21Z","messageCount":14,"participants":["Bartosz Baranowski","Junio C Hamano","Jeff King","Christian Couder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"369765","messageId":"CAEpy5fTnyPjH0sVyjnGHi1qxo+_dpaerxwaD7MmNPbmLx6qyJA@mail.gmail.com","threadId":"50549","inReplyTo":null,"subject":"git bisect - good vs bad output is different.","fromName":"Bartosz Baranowski","fromEmail":"bbaranow@redhat.com","sentAt":"2019-02-21T10:47:29Z","receivedAt":"2019-02-21T10:47:42Z","isPatch":false,"sender":{"key":"bbaranow@redhat.com","avatar":null},"body":"Depending how you end bisecting, result/report is either one liner or\ndumps commit information:\n\n\n$ git bisect bad\n77c044d8d66f9f9bebdb805853409e920e537d59 is the first bad commit\ncommit 77c044d8d66f9f9bebdb805853409e920e537d59\nAuthor: XXXX\nDate:   Tue Jan 22 09:24:02 2019 -0500\n\n   ISSUE-11626 Bad fish in the sea of comment\n\n:040000 040000 ef2280aa5f7e0c23f8750c43a0bf05c0a9639fe3\nf63bea979784cade7dffd653d939f665ff6a53b7 M      clustering\n:040000 040000 6f4667c819106f6e9ddbb902253862212a2558f5\n4d8c4dc85872c0665eb77957a6fd69c49c173188 M      triton\n:040000 040000 0400075b3d5f7cb9e68683b25b8ede93fb1b293b\n35ba4c831c4834f9ce612e394621dde382bc72f1 M      web-common\n\nvs\n\n\ngit bisect good\n3a9388eef42efc87c78ce22158d55e69a278b4eb is the first bad commit\n\ngit --version\ngit version 2.14.1\n"},{"id":"369857","messageId":"xmqq1s40u8gt.fsf@gitster-ct.c.googlers.com","threadId":"50549","inReplyTo":"CAEpy5fTnyPjH0sVyjnGHi1qxo+_dpaerxwaD7MmNPbmLx6qyJA@mail.gmail.com","subject":"Re: git bisect - good vs bad output is different.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-21T21:39:46Z","receivedAt":"2019-02-21T21:39:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bartosz Baranowski <bbaranow@redhat.com> writes:\n\n> Depending how you end bisecting, result/report is either one liner or\n> dumps commit information:\n>\n>\n> $ git bisect bad\n> 77c044d8d66f9f9bebdb805853409e920e537d59 is the first bad commit\n> commit 77c044d8d66f9f9bebdb805853409e920e537d59\n> Author: XXXX\n> Date:   Tue Jan 22 09:24:02 2019 -0500\n>\n>    ISSUE-11626 Bad fish in the sea of comment\n>\n> :040000 040000 ef2280aa5f7e0c23f8750c43a0bf05c0a9639fe3\n> f63bea979784cade7dffd653d939f665ff6a53b7 M      clustering\n> :040000 040000 6f4667c819106f6e9ddbb902253862212a2558f5\n> 4d8c4dc85872c0665eb77957a6fd69c49c173188 M      triton\n> :040000 040000 0400075b3d5f7cb9e68683b25b8ede93fb1b293b\n> 35ba4c831c4834f9ce612e394621dde382bc72f1 M      web-common\n>\n> vs\n>\n>\n> git bisect good\n> 3a9388eef42efc87c78ce22158d55e69a278b4eb is the first bad commit\n>\n> git --version\n> git version 2.14.1\n\n\nAre you sure (and if so how did you reach that conclusion) that the\nabove difference comes from the last 'bad' vs 'good' you finished,\nand not comes from the difference between 77c044d8 vs 3a9388ee?\n\nAt the end of the bisection session, bisect.c::show_diff_tree() is\ncalled on that \"culprit\" commit.  Is it possible that 3a9388ee is a\nsimple and trivial merge that does not have anything worth reporting\nfor \"git diff-tree\"?\n\n\n"},{"id":"369884","messageId":"20190222061949.GA9875@sigill.intra.peff.net","threadId":"50549","inReplyTo":"xmqq1s40u8gt.fsf@gitster-ct.c.googlers.com","subject":"[PATCH 0/3] prettier bisect output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-22T06:19:49Z","receivedAt":"2019-02-22T06:19:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 21, 2019 at 01:39:46PM -0800, Junio C Hamano wrote:\n\n> > git bisect good\n> > 3a9388eef42efc87c78ce22158d55e69a278b4eb is the first bad commit\n> >\n> > git --version\n> > git version 2.14.1\n> \n> Are you sure (and if so how did you reach that conclusion) that the\n> above difference comes from the last 'bad' vs 'good' you finished,\n> and not comes from the difference between 77c044d8 vs 3a9388ee?\n> \n> At the end of the bisection session, bisect.c::show_diff_tree() is\n> called on that \"culprit\" commit.  Is it possible that 3a9388ee is a\n> simple and trivial merge that does not have anything worth reporting\n> for \"git diff-tree\"?\n\nI've run across this many times, too. Since it's been bugging me for a\ndecade, I thought I'd finally try to address it. Here are some patches.\n\nThere was some discussion about a year ago about just using \"git show\"\nfor this output:\n\n  https://public-inbox.org/git/CAP8UFD3QhTUj+j3vBGrm0sTQ2dSOLS-m2_PwFj6DZS4VZHKRTQ@mail.gmail.com/\n\nChristian seemed generally OK with tweaking the output, but preferred\nnot to move all the way to running an external \"git show\". I'm not sure\nI completely agree, but it was easy enough to get the results I wanted\njust by fiddling the current code a bit. ;)\n\n  [1/3]: bisect: use string arguments to feed internal diff-tree\n  [2/3]: bisect: fix internal diff-tree config loading\n  [3/3]: bisect: make diff-tree output prettier\n\n bisect.c                    | 19 +++++--------------\n t/t6030-bisect-porcelain.sh |  6 +++---\n 2 files changed, 8 insertions(+), 17 deletions(-)\n\n-Peff\n"},{"id":"369885","messageId":"20190222062037.GA10248@sigill.intra.peff.net","threadId":"50549","inReplyTo":"20190222061949.GA9875@sigill.intra.peff.net","subject":"[PATCH 1/3] bisect: use string arguments to feed internal diff-tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-22T06:20:37Z","receivedAt":"2019-02-22T06:20:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit e22278c0a0 (bisect: display first bad commit without forking a\nnew process, 2009-05-28) converted our external call to diff-tree to\nan internal use of the log_tree_commit(). But rather than individually\nsetting options in the rev_info struct (and explaining in comments how\nthey map to command-line options), we can just pass the command-line\noptions to setup_revisions().\n\nThis is shorter, easier to change, and less likely to break if\nrevision.c internals change.\n\nNote that we unconditionally set the output format to \"raw\". The\nconditional in the original code didn't actually do anything useful,\nsince nobody had an opportunity to set the format to anything.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n bisect.c | 17 ++++-------------\n 1 file changed, 4 insertions(+), 13 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 3af955c4bc..8c81859835 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -896,24 +896,15 @@ static void show_diff_tree(struct repository *r,\n \t\t\t   const char *prefix,\n \t\t\t   struct commit *commit)\n {\n+\tconst char *argv[] = {\n+\t\t\"diff-tree\", \"--pretty\", \"--no-abbrev\", \"--raw\", NULL\n+\t};\n \tstruct rev_info opt;\n \n-\t/* diff-tree init */\n \trepo_init_revisions(r, &opt, prefix);\n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n-\topt.abbrev = 0;\n-\topt.diff = 1;\n \n-\t/* This is what \"--pretty\" does */\n-\topt.verbose_header = 1;\n-\topt.use_terminator = 0;\n-\topt.commit_format = CMIT_FMT_DEFAULT;\n-\n-\t/* diff-tree init */\n-\tif (!opt.diffopt.output_format)\n-\t\topt.diffopt.output_format = DIFF_FORMAT_RAW;\n-\n-\tsetup_revisions(0, NULL, &opt, NULL);\n+\tsetup_revisions(ARRAY_SIZE(argv) - 1, argv, &opt, NULL);\n \tlog_tree_commit(&opt, commit);\n }\n \n-- \n2.21.0.rc2.577.g06bbe9cbd1\n\n"},{"id":"369886","messageId":"20190222062133.GB10248@sigill.intra.peff.net","threadId":"50549","inReplyTo":"20190222061949.GA9875@sigill.intra.peff.net","subject":"[PATCH 2/3] bisect: fix internal diff-tree config loading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-22T06:21:33Z","receivedAt":"2019-02-22T06:21:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we run our internal diff-tree to show the bisected commit, we call\ninit_revisions(), then load config, then setup_revisions(). But that\norder is wrong: we copy the configured defaults into the rev_info struct\nduring the init_revisions step, so our config load wasn't actually doing\nanything.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIt does feel a little weird loading config at all here, since it would\npotentially affect other in-process operations. This is where an\nexternal process might be cleaner.\n\n bisect.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 8c81859835..b04d7b2f63 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -901,8 +901,8 @@ static void show_diff_tree(struct repository *r,\n \t};\n \tstruct rev_info opt;\n \n-\trepo_init_revisions(r, &opt, prefix);\n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n+\trepo_init_revisions(r, &opt, prefix);\n \n \tsetup_revisions(ARRAY_SIZE(argv) - 1, argv, &opt, NULL);\n \tlog_tree_commit(&opt, commit);\n-- \n2.21.0.rc2.577.g06bbe9cbd1\n\n"},{"id":"369887","messageId":"20190222062327.GC10248@sigill.intra.peff.net","threadId":"50549","inReplyTo":"20190222061949.GA9875@sigill.intra.peff.net","subject":"[PATCH 3/3] bisect: make diff-tree output prettier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-22T06:23:28Z","receivedAt":"2019-02-22T06:23:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After completing a bisection, we print out the commit we found using an\ninternal version of diff-tree. The result is aesthetically lacking:\n\n  - it shows a raw diff, which is generally less informative for human\n    readers than \"--stat --summary\" (which we already decided was nice\n    for humans in format-patch's output).\n\n  - by not abbreviating hashes, the result is likely to wrap on most\n    people's terminals\n\n  - we don't use \"-r\", so if the commit touched files in a directory,\n    you only get to see the top-level directory mentioned\n\n  - we don't specify \"--cc\" or similar, so merges print nothing (not\n    even the commit message!)\n\nEven though bisect might be driven by scripts, there's no reason to\nconsider this part of the output as machine-readable (if anything, the\ninitial \"$hash is the first bad commit\" might be parsed, but we won't\ntouch that here). Let's make it prettier and more informative for a\nhuman reading the output.\n\nWhile we're tweaking the options, let's also switch to using the diff\n\"ui\" config. If we're accepting that this is human-readable output, then\nwe should respect the user's options for how to display it.\n\nNote that we have to touch a few tests in t6030. These check bisection\nin a corrupted repository (it's missing a subtree). They didn't fail\nwith the previous code, because it didn't actually recurse far enough in\nthe diff to find the broken tree. But now we'll see the corruption and\ncomplain.\n\nAdjusting the tests to expect the die() is the best fix. We still\nconfirm that we're able to bisect within the broken repo. And we'll\nstill print \"$hash is the first bad commit\" as usual before dying;\nshowing that is a reasonable outcome in a corrupt repository (and was\nwhat might happen already, if the root tree was corrupt).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIf we do care about the change in exit code from bisect, then it\nprobably does make sense to go with an external process. Then it can\nhappily die on the corruption, while bisect continues with the rest of\nthe high-level operation. I'm not sure it really matters much, though.\nOnce your repository is corrupted, all bets are off. It's nice that we\ncan bisect in such a state at all.\n\n bisect.c                    | 4 ++--\n t/t6030-bisect-porcelain.sh | 6 +++---\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex b04d7b2f63..e87ac29a51 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -897,11 +897,11 @@ static void show_diff_tree(struct repository *r,\n \t\t\t   struct commit *commit)\n {\n \tconst char *argv[] = {\n-\t\t\"diff-tree\", \"--pretty\", \"--no-abbrev\", \"--raw\", NULL\n+\t\t\"diff-tree\", \"--pretty\", \"--stat\", \"--summary\", \"--cc\", NULL\n \t};\n \tstruct rev_info opt;\n \n-\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n+\tgit_config(git_diff_ui_config, NULL);\n \trepo_init_revisions(r, &opt, prefix);\n \n \tsetup_revisions(ARRAY_SIZE(argv) - 1, argv, &opt, NULL);\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 55835ee4a4..49a394bd75 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -681,7 +681,7 @@ test_expect_success 'bisect: --no-checkout - target in breakage' '\n \tcheck_same BROKEN_HASH6 BISECT_HEAD &&\n \tgit bisect bad BISECT_HEAD &&\n \tcheck_same BROKEN_HASH5 BISECT_HEAD &&\n-\tgit bisect good BISECT_HEAD &&\n+\ttest_must_fail git bisect good BISECT_HEAD &&\n \tcheck_same BROKEN_HASH6 bisect/bad &&\n \tgit bisect reset\n '\n@@ -692,7 +692,7 @@ test_expect_success 'bisect: --no-checkout - target after breakage' '\n \tcheck_same BROKEN_HASH6 BISECT_HEAD &&\n \tgit bisect good BISECT_HEAD &&\n \tcheck_same BROKEN_HASH8 BISECT_HEAD &&\n-\tgit bisect good BISECT_HEAD &&\n+\ttest_must_fail git bisect good BISECT_HEAD &&\n \tcheck_same BROKEN_HASH9 bisect/bad &&\n \tgit bisect reset\n '\n@@ -701,7 +701,7 @@ test_expect_success 'bisect: demonstrate identification of damage boundary' \"\n \tgit bisect reset &&\n \tgit checkout broken &&\n \tgit bisect start broken master --no-checkout &&\n-\tgit bisect run \\\"\\$SHELL_PATH\\\" -c '\n+\ttest_must_fail git bisect run \\\"\\$SHELL_PATH\\\" -c '\n \t\tGOOD=\\$(git for-each-ref \\\"--format=%(objectname)\\\" refs/bisect/good-*) &&\n \t\tgit rev-list --objects BISECT_HEAD --not \\$GOOD >tmp.\\$\\$ &&\n \t\tgit pack-objects --stdout >/dev/null < tmp.\\$\\$\n-- \n2.21.0.rc2.577.g06bbe9cbd1\n"},{"id":"369959","messageId":"xmqqwolrradk.fsf@gitster-ct.c.googlers.com","threadId":"50549","inReplyTo":"20190222061949.GA9875@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] prettier bisect output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-22T17:39:03Z","receivedAt":"2019-02-22T17:39:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I've run across this many times, too. Since it's been bugging me for a\n> decade, I thought I'd finally try to address it. Here are some patches.\n>\n> There was some discussion about a year ago about just using \"git show\"\n> for this output:\n>\n>   https://public-inbox.org/git/CAP8UFD3QhTUj+j3vBGrm0sTQ2dSOLS-m2_PwFj6DZS4VZHKRTQ@mail.gmail.com/\n>\n> Christian seemed generally OK with tweaking the output, but preferred\n> not to move all the way to running an external \"git show\". I'm not sure\n> I completely agree, but it was easy enough to get the results I wanted\n> just by fiddling the current code a bit. ;)\n>\n>   [1/3]: bisect: use string arguments to feed internal diff-tree\n>   [2/3]: bisect: fix internal diff-tree config loading\n>   [3/3]: bisect: make diff-tree output prettier\n>\n>  bisect.c                    | 19 +++++--------------\n>  t/t6030-bisect-porcelain.sh |  6 +++---\n>  2 files changed, 8 insertions(+), 17 deletions(-)\n\nLooks good from a quick glance.\n\nOne unrelated thing that made me curious was that the output from\n\n\tgit grep 'is the first '\n\nhad these two lines:\n\nbisect.c:\t\tprintf(\"%s is the first %s commit\\n\", oid_to_hex(bisect_rev),\ngit-bisect.sh:\t\tif sane_grep \"is the first $TERM_BAD commit\" \"$GIT_DIR/BISECT_RUN\" >/dev/null\n\nwhich means that we cannot localize this message without thought,\nunlike the usual \"hey, this is end-user facing, so wrap it in _()\nout of spinal reflex.\"\n\n"},{"id":"369960","messageId":"xmqqsgwfr9vr.fsf@gitster-ct.c.googlers.com","threadId":"50549","inReplyTo":"20190222062327.GC10248@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] bisect: make diff-tree output prettier","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-22T17:49:44Z","receivedAt":"2019-02-22T17:49:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> After completing a bisection, we print out the commit we found using an\n> internal version of diff-tree. The result is aesthetically lacking:\n>\n>   - it shows a raw diff, which is generally less informative for human\n>     readers than \"--stat --summary\" (which we already decided was nice\n>     for humans in format-patch's output).\n>\n>   - by not abbreviating hashes, the result is likely to wrap on most\n>     people's terminals\n>\n>   - we don't use \"-r\", so if the commit touched files in a directory,\n>     you only get to see the top-level directory mentioned\n>\n>   - we don't specify \"--cc\" or similar, so merges print nothing (not\n>     even the commit message!)\n>\n> Even though bisect might be driven by scripts, there's no reason to\n> consider this part of the output as machine-readable (if anything, the\n> initial \"$hash is the first bad commit\" might be parsed, but we won't\n> touch that here). Let's make it prettier and more informative for a\n> human reading the output.\n\nSounds very sensible.  One potential point that makes me worried is\nthis move might tempt people to make the output even larger (e.g. a\nfull diff with \"--patch\" is overkill if done unconditionally).\n\n> While we're tweaking the options, let's also switch to using the diff\n> \"ui\" config. If we're accepting that this is human-readable output, then\n> we should respect the user's options for how to display it.\n> ...\n> If we do care about the change in exit code from bisect, then it\n> probably does make sense to go with an external process. Then it can\n> happily die on the corruption, while bisect continues with the rest of\n> the high-level operation. I'm not sure it really matters much, though.\n> Once your repository is corrupted, all bets are off. It's nice that we\n> can bisect in such a state at all.\n\nThis is about showing the very final message after finding which one\nis the culprit.  Is there any other \"clean-up\" action we need to do\nafter showing the message?  I do not care too much about the exit\ncode from the bisection, but if dying from diff-tree can interfere\nwith such a clean-up, that would bother me a lot more, and at that\npoint, given especially that this is not a performance sensitive\nthing at all (it is not even invoked log(n) times---just once at the\nend), moving to external process may make it a lot simpler and\ncleaner.\n\n"},{"id":"370020","messageId":"20190223134452.GE22403@sigill.intra.peff.net","threadId":"50549","inReplyTo":"xmqqsgwfr9vr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] bisect: make diff-tree output prettier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-23T13:44:53Z","receivedAt":"2019-02-23T13:44:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 22, 2019 at 09:49:44AM -0800, Junio C Hamano wrote:\n\n> > Even though bisect might be driven by scripts, there's no reason to\n> > consider this part of the output as machine-readable (if anything, the\n> > initial \"$hash is the first bad commit\" might be parsed, but we won't\n> > touch that here). Let's make it prettier and more informative for a\n> > human reading the output.\n> \n> Sounds very sensible.  One potential point that makes me worried is\n> this move might tempt people to make the output even larger (e.g. a\n> full diff with \"--patch\" is overkill if done unconditionally).\n\nYeah, I agree that would be overkill. What I have here isn't actually\nmuch bigger. It mostly trades one line of --raw for one line of --stat.\nThe big change is that we actually bother to recurse, but I think the\nold behavior was just buggy.\n\nAt any rate, I think we can discuss such a patch if and when it comes\nup.\n\n> > If we do care about the change in exit code from bisect, then it\n> > probably does make sense to go with an external process. Then it can\n> > happily die on the corruption, while bisect continues with the rest of\n> > the high-level operation. I'm not sure it really matters much, though.\n> > Once your repository is corrupted, all bets are off. It's nice that we\n> > can bisect in such a state at all.\n> \n> This is about showing the very final message after finding which one\n> is the culprit.  Is there any other \"clean-up\" action we need to do\n> after showing the message?  I do not care too much about the exit\n> code from the bisection, but if dying from diff-tree can interfere\n> with such a clean-up, that would bother me a lot more, and at that\n> point, given especially that this is not a performance sensitive\n> thing at all (it is not even invoked log(n) times---just once at the\n> end), moving to external process may make it a lot simpler and\n> cleaner.\n\nThanks, I had a vague feeling along these lines, but you nicely put it\ninto words. As far as I can tell, no, we're not missing any important\ncleanup in that process; it looks like the only call to show_diff_tree()\nthen calls exit(10) immediately after.\n\nHowever, that does change our exit code, which git-bisect.sh then\npropagates instead of writing the entry into the BISECT_LOG.\n\nI'm still not convinced this is really worth caring about, as it implies\na corrupt repo. But if we did want to fix it, then I think the external\ndiff-tree would still be the right thing (since it's hard for\ngit-bisect.sh to tell the different between this death and another one).\n\n-Peff\n"},{"id":"370551","messageId":"CAP8UFD0+MSaU4KmD_pfHnCDFoqr9H99Fp9tBP-Qw+vs+ambgFg@mail.gmail.com","threadId":"50549","inReplyTo":"20190222062133.GB10248@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] bisect: fix internal diff-tree config loading","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-03T17:59:19Z","receivedAt":"2019-03-03T17:59:34Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Feb 22, 2019 at 7:21 AM Jeff King <peff@peff.net> wrote:\n>\n> When we run our internal diff-tree to show the bisected commit, we call\n> init_revisions(), then load config, then setup_revisions(). But that\n> order is wrong: we copy the configured defaults into the rev_info struct\n> during the init_revisions step, so our config load wasn't actually doing\n> anything.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> It does feel a little weird loading config at all here, since it would\n> potentially affect other in-process operations.\n\nI like that this patch fixes a bug, but this still triggers some\nwondering/comments.\n\nWould it be better or at least less weird to load it at the beginning\nof `git bisect`?\n\nOr is the real problem a limitation of the config system, that prevent\nus from temporarily loading, and then maybe unloading, some config?\n\n> This is where an\n> external process might be cleaner.\n\nIt depends on which definition of clean we use. Yeah, having many\nglobal variables and not caring because we launch many external\nprocesses that will \"clean\" things up when they exit can seem \"clean\"\nfor some time. But I think we are past this point now, and I still\nwouldn't like us to go back to our previous way of doing things even\nhere. So thanks for not using an external process.\n\nThanks for working on this,\nChristian.\n"},{"id":"370552","messageId":"CAP8UFD3bBMNq6p+oSaO8up8pWXJpMCWMQZfDn_kH-ZGPhqUpFA@mail.gmail.com","threadId":"50549","inReplyTo":"20190223134452.GE22403@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] bisect: make diff-tree output prettier","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-03T18:25:02Z","receivedAt":"2019-03-03T18:25:17Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Feb 23, 2019 at 2:44 PM Jeff King <peff@peff.net> wrote:\n>\n> On Fri, Feb 22, 2019 at 09:49:44AM -0800, Junio C Hamano wrote:\n>\n\n> > > If we do care about the change in exit code from bisect, then it\n> > > probably does make sense to go with an external process. Then it can\n> > > happily die on the corruption, while bisect continues with the rest of\n> > > the high-level operation. I'm not sure it really matters much, though.\n> > > Once your repository is corrupted, all bets are off. It's nice that we\n> > > can bisect in such a state at all.\n> >\n> > This is about showing the very final message after finding which one\n> > is the culprit.  Is there any other \"clean-up\" action we need to do\n> > after showing the message?  I do not care too much about the exit\n> > code from the bisection, but if dying from diff-tree can interfere\n> > with such a clean-up, that would bother me a lot more, and at that\n> > point, given especially that this is not a performance sensitive\n> > thing at all (it is not even invoked log(n) times---just once at the\n> > end), moving to external process may make it a lot simpler and\n> > cleaner.\n>\n> Thanks, I had a vague feeling along these lines, but you nicely put it\n> into words. As far as I can tell, no, we're not missing any important\n> cleanup in that process; it looks like the only call to show_diff_tree()\n> then calls exit(10) immediately after.\n>\n> However, that does change our exit code, which git-bisect.sh then\n> propagates instead of writing the entry into the BISECT_LOG.\n>\n> I'm still not convinced this is really worth caring about, as it implies\n> a corrupt repo.\n\nI don't care much about what happens in a corrupt repo, but I am\nadding Jon Seymour in CC who wrote those tests in:\n\nd3dfeedf2e (bisect: add tests to document expected behaviour in\npresence of broken trees., 2011-08-04)\nb704a8b3fd (bisect: add tests for the --no-checkout option., 2011-08-04)\n"},{"id":"370553","messageId":"CAP8UFD1puPt2c+EJRaBZmd_Fi5uJn0e4fthw8cpKoj7k+5Oy4g@mail.gmail.com","threadId":"50549","inReplyTo":"20190222061949.GA9875@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] prettier bisect output","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-03T18:33:17Z","receivedAt":"2019-03-03T18:33:31Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Feb 22, 2019 at 7:19 AM Jeff King <peff@peff.net> wrote:\n>\n> On Thu, Feb 21, 2019 at 01:39:46PM -0800, Junio C Hamano wrote:\n\n> > At the end of the bisection session, bisect.c::show_diff_tree() is\n> > called on that \"culprit\" commit.  Is it possible that 3a9388ee is a\n> > simple and trivial merge that does not have anything worth reporting\n> > for \"git diff-tree\"?\n>\n> I've run across this many times, too. Since it's been bugging me for a\n> decade, I thought I'd finally try to address it. Here are some patches.\n>\n> There was some discussion about a year ago about just using \"git show\"\n> for this output:\n>\n>   https://public-inbox.org/git/CAP8UFD3QhTUj+j3vBGrm0sTQ2dSOLS-m2_PwFj6DZS4VZHKRTQ@mail.gmail.com/\n>\n> Christian seemed generally OK with tweaking the output, but preferred\n> not to move all the way to running an external \"git show\".\n\nYeah, I am still OK with tweaking the output, though I'd rather not go\nback to using an external process.\n\n> I'm not sure\n> I completely agree, but it was easy enough to get the results I wanted\n> just by fiddling the current code a bit. ;)\n>\n>   [1/3]: bisect: use string arguments to feed internal diff-tree\n>   [2/3]: bisect: fix internal diff-tree config loading\n>   [3/3]: bisect: make diff-tree output prettier\n\nI am OK with the above patches.\n\nThanks,\nChristian.\n\n\n>  bisect.c                    | 19 +++++--------------\n>  t/t6030-bisect-porcelain.sh |  6 +++---\n>  2 files changed, 8 insertions(+), 17 deletions(-)\n>\n> -Peff\n"},{"id":"370657","messageId":"20190305041515.GC19800@sigill.intra.peff.net","threadId":"50549","inReplyTo":"CAP8UFD0+MSaU4KmD_pfHnCDFoqr9H99Fp9tBP-Qw+vs+ambgFg@mail.gmail.com","subject":"Re: [PATCH 2/3] bisect: fix internal diff-tree config loading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-05T04:15:15Z","receivedAt":"2019-03-05T04:15:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 03, 2019 at 06:59:19PM +0100, Christian Couder wrote:\n\n> On Fri, Feb 22, 2019 at 7:21 AM Jeff King <peff@peff.net> wrote:\n> >\n> > When we run our internal diff-tree to show the bisected commit, we call\n> > init_revisions(), then load config, then setup_revisions(). But that\n> > order is wrong: we copy the configured defaults into the rev_info struct\n> > during the init_revisions step, so our config load wasn't actually doing\n> > anything.\n> >\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> > It does feel a little weird loading config at all here, since it would\n> > potentially affect other in-process operations.\n> \n> I like that this patch fixes a bug, but this still triggers some\n> wondering/comments.\n> \n> Would it be better or at least less weird to load it at the beginning\n> of `git bisect`?\n\nI guess you mean at the beginning of bisect--helper? That would be OK\nwith me, too, and maybe would be a little less weird. But if bisect is\nslowly becoming a single binary, maybe we should just wait for that.\n\n> Or is the real problem a limitation of the config system, that prevent\n> us from temporarily loading, and then maybe unloading, some config?\n\nIt's less the config system, and more the way Git is written. ;)\nTypically parsing the config means setting a bunch of globals, and\nforgetting what their original values are. That's not something the\nconfig system can fix.\n\n-Peff\n"},{"id":"370658","messageId":"20190305041617.GD19800@sigill.intra.peff.net","threadId":"50549","inReplyTo":"CAP8UFD1puPt2c+EJRaBZmd_Fi5uJn0e4fthw8cpKoj7k+5Oy4g@mail.gmail.com","subject":"Re: [PATCH 0/3] prettier bisect output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-05T04:16:17Z","receivedAt":"2019-03-05T04:16:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 03, 2019 at 07:33:17PM +0100, Christian Couder wrote:\n\n> > I've run across this many times, too. Since it's been bugging me for a\n> > decade, I thought I'd finally try to address it. Here are some patches.\n> >\n> > There was some discussion about a year ago about just using \"git show\"\n> > for this output:\n> >\n> >   https://public-inbox.org/git/CAP8UFD3QhTUj+j3vBGrm0sTQ2dSOLS-m2_PwFj6DZS4VZHKRTQ@mail.gmail.com/\n> >\n> > Christian seemed generally OK with tweaking the output, but preferred\n> > not to move all the way to running an external \"git show\".\n> \n> Yeah, I am still OK with tweaking the output, though I'd rather not go\n> back to using an external process.\n\nBy the way, I've done a few bisects since writing this, and I've been\nvery happy with the result. So I endorse my own patch. ;)\n\n-Peff\n"}]}