Re: [BUG] Segfault with rev-list --bisect

Troy Moure [off-list ref] writes:

git rev-list --bisect --first-parent --parents HEAD --not HEAD~1

Hmm, as "rev-list --bisect" is not end-user facing command (it is
purely an implementation detail for "git bisect") and we never call
it with --first-parent, I am not sure if it is worth labelling it as
a BUG.  Surely, the command can refuse to operate when it sees both
options given, but that would be a fairly low priority.

Of course, if you are planning to do "git bisect --first-parent", it
is one of the things that needs to be addressed, together with
counting the rounds and bisecting the linear set of commits on the
first-parent chain correctly.

On Wed, Mar 4, 2015 at 6:44 PM, Junio C Hamano [off-list ref] wrote:

Troy Moure [off-list ref] writes:

quoted

git rev-list --bisect --first-parent --parents HEAD --not HEAD~1

Hmm, as "rev-list --bisect" is not end-user facing command (it is
purely an implementation detail for "git bisect") and we never call
it with --first-parent, I am not sure if it is worth labelling it as
a BUG.  Surely, the command can refuse to operate when it sees both
options given, but that would be a fairly low priority.

Hrm, ok. I didn't realize "--bisect" is only intended to be used by git-bisect
(although I suppose the fact that it treats ref/bisect/* specially should have
been a hint). If uses of "--bisect" other than by git-bisect are considered
unsupported, IMO it would be good to say that in the documentation - right now
it looks like just another rev-list parameter. (I realize rev-list itself is
"plumbing", but that's not the same as "not user facing", is it?)

If you're curious, I ran into this because I am working on a script that can be
run repeatedly to process commits, and uses git notes to mark commits that have
been processed.  Parents are always processed before their children, so if a
commit has a note, it means all its ancestors also have notes. I want to
quickly find the set of commits that have not yet been processed. I am thinking
of finding the "boundary" commits (commits that have a note and at least one
child that does not) by using a binary search to find the boundary commit on
the first-parent chain, and then recursively doing the same thing starting from
each non-first parent of each merge commit between the boundary commit and the
starting point.

Upon further thought, it's probably better to just read the whole first-parent
chain and do the binary search in the script, since "git rev-list --bisect"
would have generate the chain each time it's called. But I'd already run into
the segfault, so I thought I'd report it.

Of course, I'd appreciate any thoughts or comments on the problem I'm trying to
solve as well.

Thanks,
Troy

rev-list --bisect is used by git bisect, but never together with
--first-parent. Because rev-list --bisect together with --first-parent
is not handled currently, and even leads to segfaults, refuse to use
both options together.

Signed-off-by: Kevin Daudt <redacted>
---
This is my first code patch, and thought this was a nice exercise.

 Documentation/rev-list-options.txt | 3 ++-
 builtin/rev-list.c                 | 3 +++
 t/t6000-rev-list-misc.sh           | 4 ++++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4ed8587..05c3f6d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt

@@ -123,7 +123,8 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).
 	because merges into a topic branch tend to be only about
 	adjusting to updated upstream from time to time, and
 	this option allows you to ignore the individual commits
-	brought in to your history by such a merge.
+	brought in to your history by such a merge. Cannot be
+	combined with --bisect.
 
 --not::
 	Reverses the meaning of the '{caret}' prefix (or lack thereof)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ff84a82..c271e15 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c

@@ -291,6 +291,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	if (revs.bisect)
 		bisect_list = 1;
 
+	if(revs.first_parent_only && revs.bisect)
+		die(_("--first-parent is incompattible with --bisect"));
+
 	if (DIFF_OPT_TST(&revs.diffopt, QUICK))
 		info.flags |= REV_LIST_QUIET;
 	for (i = 1 ; i < argc; i++) {

diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 2602086..1f58b46 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh

@@ -96,4 +96,8 @@ test_expect_success 'rev-list can show index objects' '
 	test_cmp expect actual
 '
 
+test_expect_success '--bisect and --first-parent can not be combined' '
+	test_must_fail git rev-list --bisect --first-parent HEAD
+'
+
 test_done

-- 
2.3.1.184.g97c12a8.dirty

On Sat, Mar 07, 2015 at 10:31:16PM +0100, Kevin Daudt wrote:

I should have added a space after the if.

+		die(_("--first-parent is incompattible with --bisect"));
+
 	if (DIFF_OPT_TST(&revs.diffopt, QUICK))
 		info.flags |= REV_LIST_QUIET;
 	for (i = 1 ; i < argc; i++) {

rev-list --bisect is used by git bisect, but never together with
--first-parent. Because rev-list --bisect together with --first-parent
is not handled currently, and even leads to segfaults, refuse to use
both options together.

Signed-off-by: Kevin Daudt <redacted>
Suggested-by: Junio C. Hamano <redacted>
---
* Changes from v1: Added the missing SP between "if(",
  as per the code guidelines

Thanks for the feedback.

 Documentation/rev-list-options.txt | 3 ++-
 builtin/rev-list.c                 | 3 +++
 t/t6000-rev-list-misc.sh           | 4 ++++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4ed8587..05c3f6d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt

@@ -123,7 +123,8 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).
 	because merges into a topic branch tend to be only about
 	adjusting to updated upstream from time to time, and
 	this option allows you to ignore the individual commits
-	brought in to your history by such a merge.
+	brought in to your history by such a merge. Cannot be
+	combined with --bisect.
 
 --not::
 	Reverses the meaning of the '{caret}' prefix (or lack thereof)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ff84a82..c271e15 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c

@@ -291,6 +291,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	if (revs.bisect)
 		bisect_list = 1;
 
+	if(revs.first_parent_only && revs.bisect)
+		die(_("--first-parent is incompattible with --bisect"));
+
 	if (DIFF_OPT_TST(&revs.diffopt, QUICK))
 		info.flags |= REV_LIST_QUIET;
 	for (i = 1 ; i < argc; i++) {

diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 2602086..1f58b46 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh

@@ -96,4 +96,8 @@ test_expect_success 'rev-list can show index objects' '
 	test_cmp expect actual
 '
 
+test_expect_success '--bisect and --first-parent can not be combined' '
+	test_must_fail git rev-list --bisect --first-parent HEAD
+'
+
 test_done

-- 
2.3.1.184.g97c12a8.dirty

rev-list --bisect is used by git bisect, but never together with
--first-parent. Because rev-list --bisect together with --first-parent
is not handled currently, and even leads to segfaults, refuse to use
both options together.

Signed-off-by: Kevin Daudt <redacted>
Suggested-by: Junio C. Hamano <redacted>
---
Sorry for the false fix reroll in v2, forgot to actually commit the change.

* Changes from v2: Added the missing SP between "if(",
  as per the code guidelines")" (Now with the actual change)

 Documentation/rev-list-options.txt | 3 ++-
 builtin/rev-list.c                 | 3 +++
 t/t6000-rev-list-misc.sh           | 4 ++++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4ed8587..05c3f6d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt

@@ -123,7 +123,8 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).
 	because merges into a topic branch tend to be only about
 	adjusting to updated upstream from time to time, and
 	this option allows you to ignore the individual commits
-	brought in to your history by such a merge.
+	brought in to your history by such a merge. Cannot be
+	combined with --bisect.
 
 --not::
 	Reverses the meaning of the '{caret}' prefix (or lack thereof)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ff84a82..f5da2a4 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c

@@ -291,6 +291,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	if (revs.bisect)
 		bisect_list = 1;
 
+	if (revs.first_parent_only && revs.bisect)
+		die(_("--first-parent is incompattible with --bisect"));
+
 	if (DIFF_OPT_TST(&revs.diffopt, QUICK))
 		info.flags |= REV_LIST_QUIET;
 	for (i = 1 ; i < argc; i++) {

diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 2602086..1f58b46 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh

@@ -96,4 +96,8 @@ test_expect_success 'rev-list can show index objects' '
 	test_cmp expect actual
 '
 
+test_expect_success '--bisect and --first-parent can not be combined' '
+	test_must_fail git rev-list --bisect --first-parent HEAD
+'
+
 test_done

-- 
2.3.1.184.g97c12a8.dirty

rev-list --bisect is used by git bisect, but never together with
--first-parent. Because rev-list --bisect together with --first-parent
is not handled currently, and even leads to segfaults, refuse to use
both options together.

Signed-off-by: Kevin Daudt <redacted>
Suggested-by: Junio C. Hamano <redacted>
---
Sorry for the false fix reroll in v2, forgot to actually commit the change.

* Changes from v2: Added the missing SP between "if(",
  as per the code guidelines")" (Now with the actual change)

 Documentation/rev-list-options.txt | 3 ++-
 builtin/rev-list.c                 | 3 +++
 t/t6000-rev-list-misc.sh           | 4 ++++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4ed8587..05c3f6d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt

@@ -123,7 +123,8 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).
 	because merges into a topic branch tend to be only about
 	adjusting to updated upstream from time to time, and
 	this option allows you to ignore the individual commits
-	brought in to your history by such a merge.
+	brought in to your history by such a merge. Cannot be
+	combined with --bisect.
 
 --not::
 	Reverses the meaning of the '{caret}' prefix (or lack thereof)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ff84a82..f5da2a4 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c

@@ -291,6 +291,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	if (revs.bisect)
 		bisect_list = 1;
 
+	if (revs.first_parent_only && revs.bisect)
+		die(_("--first-parent is incompattible with --bisect"));
+
 	if (DIFF_OPT_TST(&revs.diffopt, QUICK))
 		info.flags |= REV_LIST_QUIET;
 	for (i = 1 ; i < argc; i++) {

diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 2602086..1f58b46 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh

@@ -96,4 +96,8 @@ test_expect_success 'rev-list can show index objects' '
 	test_cmp expect actual
 '
 
+test_expect_success '--bisect and --first-parent can not be combined' '
+	test_must_fail git rev-list --bisect --first-parent HEAD
+'
+
 test_done

-- 
2.3.1.184.g97c12a8.dirty

On Sun, Mar 8, 2015 at 11:03 AM, Kevin Daudt [off-list ref] wrote:

rev-list --bisect is used by git bisect, but never together with
--first-parent. Because rev-list --bisect together with --first-parent
is not handled currently, and even leads to segfaults, refuse to use
both options together.

Signed-off-by: Kevin Daudt <redacted>
Suggested-by: Junio C. Hamano <redacted>

It's customary for your sign-off to be last.

A couple questions:

Should the documentation for ---bisect be updated to mention this
restriction also?

Should this change be protected by a "ifndef::git-rev-list[]" as are
all other mentions of "bisect" in rev-list-options.txt?

 --not::
        Reverses the meaning of the '{caret}' prefix (or lack thereof)

On Sun, Mar 08, 2015 at 05:58:24PM -0400, Eric Sunshine wrote:

On Sun, Mar 8, 2015 at 11:03 AM, Kevin Daudt [off-list ref] wrote:

quoted

rev-list --bisect is used by git bisect, but never together with
--first-parent. Because rev-list --bisect together with --first-parent
is not handled currently, and even leads to segfaults, refuse to use
both options together.

Signed-off-by: Kevin Daudt <redacted>
Suggested-by: Junio C. Hamano <redacted>

It's customary for your sign-off to be last.

Ok, noted

quoted

---

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4ed8587..05c3f6d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt

@@ -123,7 +123,8 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).
        because merges into a topic branch tend to be only about
        adjusting to updated upstream from time to time, and
        this option allows you to ignore the individual commits
-       brought in to your history by such a merge.
+       brought in to your history by such a merge. Cannot be
+       combined with --bisect.

A couple questions:

Should the documentation for ---bisect be updated to mention this
restriction also?

Was doubting whether that was necessary as --bisect can be seen as a
mode, and --first-parent modifying that mode. But it can make sense to
also add it to that section.

Should this change be protected by a "ifndef::git-rev-list[]" as are
all other mentions of "bisect" in rev-list-options.txt?

Yes, I see why. git log also uses rev-list-options.txt and it has a
--bisect option that is unrelated to this one, so that comment doesn't
make sense for git log.

Will reroll this later.

rev-list --bisect is used by git bisect, but never together with
--first-parent. Because rev-list --bisect together with --first-parent
is not handled currently, and even leads to segfaults, refuse to use
both options together.

Suggested-by: Junio C. Hamano <redacted>
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Kevin Daudt <redacted>
---
Changes since v3:

* Added an ifdef::git-rev-list[] guard around the warning in the
  --first-parent section so that it only shows up in `man git-rev-list`
  and not in `man git log`

* Added the warning also to the --bisect section.

 Documentation/rev-list-options.txt | 4 ++++
 builtin/rev-list.c                 | 3 +++
 t/t6000-rev-list-misc.sh           | 4 ++++
 3 files changed, 11 insertions(+)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4ed8587..a148672 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt

@@ -124,6 +124,9 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).
 	adjusting to updated upstream from time to time, and
 	this option allows you to ignore the individual commits
 	brought in to your history by such a merge.
+ifdef::git-rev-list[]
+	Cannot be combined with --bisect.
+endif::git-rev-list[]
 
 --not::
 	Reverses the meaning of the '{caret}' prefix (or lack thereof)

@@ -567,6 +570,7 @@ would be of roughly the same length.  Finding the change which
 introduces a regression is thus reduced to a binary search: repeatedly
 generate and test new 'midpoint's until the commit chain is of length
 one.
+Cannot be combined with --first-parent.
 
 --bisect-vars::
 	This calculates the same as `--bisect`, except that refs in

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ff84a82..f5da2a4 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c

@@ -291,6 +291,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	if (revs.bisect)
 		bisect_list = 1;
 
+	if (revs.first_parent_only && revs.bisect)
+		die(_("--first-parent is incompattible with --bisect"));
+
 	if (DIFF_OPT_TST(&revs.diffopt, QUICK))
 		info.flags |= REV_LIST_QUIET;
 	for (i = 1 ; i < argc; i++) {

diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 2602086..1f58b46 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh

@@ -96,4 +96,8 @@ test_expect_success 'rev-list can show index objects' '
 	test_cmp expect actual
 '
 
+test_expect_success '--bisect and --first-parent can not be combined' '
+	test_must_fail git rev-list --bisect --first-parent HEAD
+'
+
 test_done

-- 
2.3.0

rev-list --bisect is used by git bisect, but never together with
--first-parent. Because rev-list --bisect together with --first-parent
is not handled currently, and even leads to segfaults, refuse to use
both options together.

Because this is not supported, it makes little sense to use git log
--bisect --first parent either, because refs/heads/bad is not limited to
the first parent chain.

Helped-by: Junio C. Hamano [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Kevin Daudt <redacted>
---
Updates since v4:

* Not only refusing rev-list --bisect --first-parent, but also log --bisect --first-parent

 Documentation/rev-list-options.txt | 7 ++++---
 revision.c                         | 3 +++
 t/t6000-rev-list-misc.sh           | 4 ++++
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4ed8587..e2de789 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt

@@ -123,7 +123,8 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).
 	because merges into a topic branch tend to be only about
 	adjusting to updated upstream from time to time, and
 	this option allows you to ignore the individual commits
-	brought in to your history by such a merge.
+	brought in to your history by such a merge. Cannot be
+	combined with --bisect.
 
 --not::
 	Reverses the meaning of the '{caret}' prefix (or lack thereof)

@@ -185,7 +186,7 @@ ifndef::git-rev-list[]
 	Pretend as if the bad bisection ref `refs/bisect/bad`
 	was listed and as if it was followed by `--not` and the good
 	bisection refs `refs/bisect/good-*` on the command
-	line.
+	line. Cannot be combined with --first-parent.
 endif::git-rev-list[]
 
 --stdin::

@@ -566,7 +567,7 @@ outputs 'midpoint', the output of the two commands
 would be of roughly the same length.  Finding the change which
 introduces a regression is thus reduced to a binary search: repeatedly
 generate and test new 'midpoint's until the commit chain is of length
-one.
+one. Cannot be combined with --first-parent.
 
 --bisect-vars::
 	This calculates the same as `--bisect`, except that refs in

diff --git a/revision.c b/revision.c
index 66520c6..ed3f6e9 100644
--- a/revision.c
+++ b/revision.c

@@ -2342,6 +2342,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
 		die("cannot use --grep-reflog without --walk-reflogs");
 
+	if (revs->first_parent_only && revs->bisect)
+		die(_("--first-parent is incompatible with --bisect"));
+
 	return left;
 }
 

diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 2602086..1f58b46 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh

@@ -96,4 +96,8 @@ test_expect_success 'rev-list can show index objects' '
 	test_cmp expect actual
 '
 
+test_expect_success '--bisect and --first-parent can not be combined' '
+	test_must_fail git rev-list --bisect --first-parent HEAD
+'
+
 test_done

-- 
2.3.2