git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip

From
MÅMartin Ågren <martin.agren@gmail.com>
Date
Jan 6, 2018, 08:21 UTC
Message-ID
<20180106082116.11057-1-martin.agren@gmail.com>
In-Reply-To
<CAELBRWLyZ5s2Wvk2U5Ks2adx23Z+51dbEs3QbAq+5nGLe21srQ@mail.gmail.com>
> On Fri, Jan 5, 2018 at 11:45 AM, Yasushi SHOJI <yasushi.shoji@gmail.com> wrote:
>> When does the list allowed to contain NULLs?
Short answer: there are no commits left to test.

The list is built in the for-loop in `find_bisection()`. So the technical answer is: if all commits in the initial list `commit_list` are UNINTERESTING (including if `commit_list` is empty to begin with).

It's also helpful to study where we should end up from there. We should take the `if (!revs.commits)` branch in `bisect_next_all()`. That is, we should print either "There are only 'skip'ped commits left to test. The first bad commit could be any of:" or "<commit> was both good and bad".

>> Since nobody noticed it since 7c117184d7, it must be a rare case, right?

Right, you marked a commit both good and bad. That's probably not very common. But it obviously happens. :-)

On 5 January 2018 at 06:28, Yasushi SHOJI <yasushi.shoji@gmail.com> wrote:
> OK, here is the step to reproduce on git.git

Thank you for providing a script for reproducing this. It helped me come up with the attached patch. The patch is based on ma/bisect-leakfix, which includes Ævar's patch.

I think this patch could be useful, either as a final "let's test something previously non-tested; this would have caught the segfault", or simply squashed into Ævar's patch as a "let's add a test that would have caught this, and which also tests a previously non-tested code path."

Thanks for digging and finding a reproduction recipe.
Martin
-- >8 --
Subject: [PATCH] bisect: add test for marking commit both good and bad

Since 670f5fe34f ([PATCH] Fix bisection terminating condition, 2005-08-30), we have noticed and complained when a commit was marked both good and bad. But we had no tests for this behavior.

Test the behavior when we mark a commit first bad, then good, but also when we are already in the bad state, where `git bisect skip` should notice it.

This test would have caught the segfault which was recently fixed in 2e9fdc795c (bisect: fix a regression causing a segfault, 2018-01-03).

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t6030-bisect-porcelain.sh | 9 +++++++++
 1 file changed, 9 insertions(+)
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 8c2c6eaef8..190f0ce0ab 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -894,4 +894,13 @@ test_expect_success 'bisect start takes options and revs in any order' '
 	test_cmp expected actual
 '
 
+test_expect_success 'marking commit both good and bad gets reported' '
+	git bisect reset &&
+	git bisect start HEAD &&
+	test_must_fail git bisect good HEAD >out &&
+	test_i18ngrep "both good and bad" out &&
+	test_must_fail git bisect skip >out &&
+	test_i18ngrep "both good and bad" out
+'
+
 test_done
-- 
2.16.0.rc1
Previous: Yasushi SHOJINext: Yasushi SHOJI
Message 7 of 14 in “[BUG] v2.16.0-rc0 seg faults when git bisect skip”
  1. Yasushi SHOJIJan 3, 2018
  2. Ævar Arnfjörð BjarmasonJan 3, 2018
  3. Martin ÅgrenJan 3, 2018
  4. bisect: fix a regression causing a segfaultÆvar Arnfjörð Bjarmason, Jan 3, 2018
  5. Yasushi SHOJIJan 5, 2018
  6. Yasushi SHOJIJan 5, 2018
  7. Martin ÅgrenJan 6, 2018
  8. Yasushi SHOJIJan 6, 2018
  9. Martin ÅgrenJan 6, 2018
  10. Christian CouderJan 6, 2018
  11. Yasushi SHOJIJan 8, 2018
  12. Christian CouderJan 8, 2018
  13. Yasushi SHOJIJan 9, 2018
  14. Junio C HamanoJan 5, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.