[RFC/PATCH 0/3] use '--bisect-refs' as bisect rev machinery option

[Christian Couder]

Using '--bisect' as a revision machinery argument is bogus because
it conflicts with the special '--bisect' argument for "rev-list".
And as shown by a test case added by a previous commit, this cannot
be fixed by adding a special flag to "struct rev_info".

So this commit just renames the '--bisect' flag to the revision
machinery into '--bisect-refs', and reverts the changes that added
the special flag to "struct rev_info".

This makes the test case added previously pass.

Signed-off-by: Christian Couder <[hidden email]>
---
 builtin-rev-list.c          |    2 --
 builtin-rev-parse.c         |    4 ++--
 revision.c                  |    5 ++---
 revision.h                  |    1 -
 t/t6030-bisect-porcelain.sh |    2 +-
 5 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 9e736b4..2ccbfbb 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -320,8 +320,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 
  memset(&info, 0, sizeof(info));
  info.revs = &revs;
- if (revs.bisect)
- bisect_list = 1;
 
  quiet = DIFF_OPT_TST(&revs.diffopt, QUICK);
  for (i = 1 ; i < argc; i++) {
diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index 9526aaf..201a458 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -39,7 +39,7 @@ static int is_rev_argument(const char *arg)
 {
  static const char *rev_args[] = {
  "--all",
- "--bisect",
+ "--bisect-refs",
  "--dense",
  "--branches",
  "--header",
@@ -554,7 +554,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
  for_each_ref(show_reference, NULL);
  continue;
  }
- if (!strcmp(arg, "--bisect")) {
+ if (!strcmp(arg, "--bisect-refs")) {
  for_each_ref_in("refs/bisect/bad", show_reference, NULL);
  for_each_ref_in("refs/bisect/good", anti_reference, NULL);
  continue;
diff --git a/revision.c b/revision.c
index 5cedd15..d1a1edc 100644
--- a/revision.c
+++ b/revision.c
@@ -995,7 +995,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
     !strcmp(arg, "--tags") || !strcmp(arg, "--remotes") ||
     !strcmp(arg, "--reflog") || !strcmp(arg, "--not") ||
     !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk") ||
-    !strcmp(arg, "--bisect"))
+    !strcmp(arg, "--bisect-refs"))
  {
  unkv[(*unkc)++] = arg;
  return 1;
@@ -1270,10 +1270,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
  handle_refs(revs, flags, for_each_branch_ref);
  continue;
  }
- if (!strcmp(arg, "--bisect")) {
+ if (!strcmp(arg, "--bisect-refs")) {
  handle_refs(revs, flags, for_each_bad_bisect_ref);
  handle_refs(revs, flags ^ UNINTERESTING, for_each_good_bisect_ref);
- revs->bisect = 1;
  continue;
  }
  if (!strcmp(arg, "--tags")) {
diff --git a/revision.h b/revision.h
index 921656a..b6421a6 100644
--- a/revision.h
+++ b/revision.h
@@ -63,7 +63,6 @@ struct rev_info {
  reverse:1,
  reverse_output_stage:1,
  cherry_pick:1,
- bisect:1,
  first_parent_only:1;
 
  /* Diff flags */
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 88a2877..4e4160e 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -392,7 +392,7 @@ test_expect_success 'bisect does not create a "bisect" branch' '
  git branch -D bisect
 '
 
-test_expect_failure 'bisect and "rev-list --bisect"' '
+test_expect_success 'bisect and "rev-list --bisect"' '
  rev_list2=$(git rev-list --bisect $HASH3 --not $HASH1) &&
  test "$rev_list2" = "$HASH2" &&
  rev_list4=$(git rev-list --bisect $HASH7 --not $HASH1) &&
--
1.6.5.1.gaf97d


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Christian Couder]

It is now shorter and safer to use the new '--bisect-refs' revision
machinery option, than to compute the refs that we must pass.

Signed-off-by: Christian Couder <[hidden email]>
---
 git-bisect.sh |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 8b3c585..41b9118 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -300,8 +300,7 @@ bisect_visualize() {
  esac
  fi
 
- not=$(git for-each-ref --format='%(refname)' "refs/bisect/good-*")
- eval '"$@"' refs/bisect/bad --not $not -- $(cat "$GIT_DIR/BISECT_NAMES")
+ eval '"$@"' --bisect-refs -- $(cat "$GIT_DIR/BISECT_NAMES")
 }
 
 bisect_reset() {
--
1.6.5.1.gaf97d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Christian Couder]

So I suggest to use '--bisect-refs' instead of '--bisect' as the new
bisect revision machinery option, because otherwise I think we get a
regression when we call "git rev-list --bisect BAD --not GOOD" and we
are already bisecting with bisect refs different than BAD and GOOD.
This also simplifies the code a little bit.

I had a look at using '--bisect-refs' in the git bisect helper instead
of collecting the good and bad refs in bisect.c::read_bisect_refs(),
but I gave up because I think we need the good and bad refs anyway for
other purposes like checking that all good refs are ancestor of the bad
ref. So I think we would not gain much if anything there.

If this is ok then the next steps I can do is add some documentation
and tests for the new '--bisect-refs' option.

Christian Couder (3):
  t6030: show "rev-list --bisect" breakage when bisecting
  revision: change '--bisect' rev machinery argument to 'bisect-refs'
  bisect: simplify calling visualizer using '--bisect-refs'

 builtin-rev-list.c          |    2 --
 builtin-rev-parse.c         |    4 ++--
 git-bisect.sh               |    3 +--
 revision.c                  |    5 ++---
 revision.h                  |    1 -
 t/t6030-bisect-porcelain.sh |   13 +++++++++++++
 6 files changed, 18 insertions(+), 10 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Christian Couder]

In the following commits:

ad3f9a7 Add '--bisect' revision machinery argument
81ee52b Merge branch 'lt/revision-bisect' into next

the '--bisect' argument was added to the revision machinery to easily
use the "bad" and "good" refs from the current bisection in any
command that expect some refs.

The problem was that '--bisect' already had a special meaning for
"git rev-list" outside the revision machinery and now it was eaten by
the revision machinery. So a flag named "bisect" was added to
"struct rev_info" in these commits, so that "git rev-list" outside the
revision machinery could see that "--bisect" had been used and operate
as if "--bisect" had been passed to it.

But the above does not fix everything, and this commit adds a test
case to show that.

Now "git rev-list --bisect BAD --not GOOD" behaves differently
depending on whether we are currently bisecting or not. If we are not
currently bisecting, it uses "BAD --not GOOD" as the bisect refs and
if we are bisecting it uses the bisect refs of the current bisection
as the bisect refs. This means that we don't behave like we used to
when we are bisecting and the reafs passed on the command line to
"git rev-list --bisect" are different from the bisect refs of the
current bisection.

The following commit will fix this regression.

Signed-off-by: Christian Couder <[hidden email]>
---
 t/t6030-bisect-porcelain.sh |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index def397c..88a2877 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -392,6 +392,19 @@ test_expect_success 'bisect does not create a "bisect" branch' '
  git branch -D bisect
 '
 
+test_expect_failure 'bisect and "rev-list --bisect"' '
+ rev_list2=$(git rev-list --bisect $HASH3 --not $HASH1) &&
+ test "$rev_list2" = "$HASH2" &&
+ rev_list4=$(git rev-list --bisect $HASH7 --not $HASH1) &&
+ test "$rev_list4" = "$HASH4" &&
+ git bisect start $HASH7 $HASH1 &&
+ rev_hash4=$(git rev-parse --verify HEAD) &&
+ test "$rev_hash4" = "$HASH4" &&
+ rev_list2=$(git rev-list --bisect $HASH3 --not $HASH1) &&
+ test "$rev_list2" = "$HASH2" &&
+ git bisect reset
+'
+
 # This creates a "side" branch to test "siblings" cases.
 #
 # H1-H2-H3-H4-H5-H6-H7  <--other
--
1.6.5.1.gaf97d


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Junio C Hamano]

Christian Couder <[hidden email]> writes:

> So I suggest to use '--bisect-refs' instead of '--bisect' as the new
> bisect revision machinery option, because otherwise I think we get a
> regression when we call "git rev-list --bisect BAD --not GOOD" and we
> are already bisecting with bisect refs different than BAD and GOOD.
> This also simplifies the code a little bit.

Just to make sure that I read you correctly, do you mean that Linus now
would say:

    $ git bisect ...
    ... inside bisect session
    $ gitk --bisect-refs arch/x86

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Linus Torvalds]

On Wed, 4 Nov 2009, Junio C Hamano wrote:

> Christian Couder <[hidden email]> writes:
>
> > So I suggest to use '--bisect-refs' instead of '--bisect' as the new
> > bisect revision machinery option, because otherwise I think we get a
> > regression when we call "git rev-list --bisect BAD --not GOOD" and we
> > are already bisecting with bisect refs different than BAD and GOOD.
> > This also simplifies the code a little bit.
>
> Just to make sure that I read you correctly, do you mean that Linus now
> would say:
>
>     $ git bisect ...
>     ... inside bisect session
>     $ gitk --bisect-refs arch/x86

I'd really rather prefer to keep '--bisect' for that, it doesn't make
sense to export '--bisect-refs' to be the user-visible switch, and then
have '--bisect' as some internal switch just for 'git rev-list'.

In fact, I'd argue that the ne wbehavior of 'git rev-list' is to some
degree _better_ (ie the existing bad/good ones are implicit while doing
bisection).

Yes, it is a behavioral change, but is it a bad one?

                        Linus
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Linus Torvalds]

On Wed, 4 Nov 2009, Linus Torvalds wrote:
>
> Yes, it is a behavioral change, but is it a bad one?

.. and perhaps we could introduce --bisect-refs as the "old behavior" of
'--bisect' to git rev-list?

I kind of suspect that it is unlikely that people are using 'git rev-list
--bisect' while _inside_ a bisection, but then wanting to bisect someting
that is outside the set of commits we're currently actively bisecting.

But maybe I'm wrong.

                Linus
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Junio C Hamano]

Linus Torvalds <[hidden email]> writes:

> On Wed, 4 Nov 2009, Linus Torvalds wrote:
>>
>> Yes, it is a behavioral change, but is it a bad one?
>
> .. and perhaps we could introduce --bisect-refs as the "old behavior" of
> '--bisect' to git rev-list?
>
> I kind of suspect that it is unlikely that people are using 'git rev-list
> --bisect' while _inside_ a bisection, but then wanting to bisect someting
> that is outside the set of commits we're currently actively bisecting.
>
> But maybe I'm wrong.

Maybe I'm wrong too, but I do not think that is plausible that people are
doing nested bisection that way.  It is probably a useful thing to do, but
if somebody has thought of doing so we would have at least seen a request
to add a way to tell "git-bisect" what names to use to record the good/bad
set of commits under to make their implementation easier.  I haven't, and
I take it an indication that it is very implausible that such scripts by
people exist to be broken by this change.

I was more worried about people who reinvented the wheel and are using
their own git-bisect.sh derivative.  It probably was forked from the
version that still used 'git rev-list --bisect", manually feeding good and
bad set of commits to it from the command line.  But then what they are
feeding would be the same as the new --bisect option implicitly gives them
anyway, so there won't be a regression either.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Christian Couder]

On Wednesday 04 November 2009, Junio C Hamano wrote:

> Linus Torvalds <[hidden email]> writes:
> > On Wed, 4 Nov 2009, Linus Torvalds wrote:
> >> Yes, it is a behavioral change, but is it a bad one?
> >
> > .. and perhaps we could introduce --bisect-refs as the "old behavior"
> > of '--bisect' to git rev-list?
> >
> > I kind of suspect that it is unlikely that people are using 'git
> > rev-list --bisect' while _inside_ a bisection, but then wanting to
> > bisect someting that is outside the set of commits we're currently
> > actively bisecting.
> >
> > But maybe I'm wrong.
>
> Maybe I'm wrong too, but I do not think that is plausible that people are
> doing nested bisection that way.  It is probably a useful thing to do,
> but if somebody has thought of doing so we would have at least seen a
> request to add a way to tell "git-bisect" what names to use to record the
> good/bad set of commits under to make their implementation easier.  I
> haven't, and I take it an indication that it is very implausible that
> such scripts by people exist to be broken by this change.
>
> I was more worried about people who reinvented the wheel and are using
> their own git-bisect.sh derivative.  It probably was forked from the
> version that still used 'git rev-list --bisect", manually feeding good
> and bad set of commits to it from the command line.  But then what they
> are feeding would be the same as the new --bisect option implicitly gives
> them anyway, so there won't be a regression either.

I don't remember exactly when, but at one time some people talked about
parallelizing bisection. The idea was that if it takes a long time to test
one commit but you can test many commits at the same time (for example on
different machines), then you can bisect faster by testing at the same time
the current commit that git bisect checked out for you and for example the
commit that git bisect would give you if the current commit is bad and the
commit it would give you if the current commit is good.

So to do that you would use "git bisect start ..." and then you could use:

$ git rev-list --bisect HEAD --not $GOOD_COMMITS

to get the commit that you would have to test if the current commit is bad
and:

$ git rev-list --bisect  $BAD --not $GOOD_COMMITS HEAD

to get the commit that you would have to test if the current commit is good.

Ok, perhaps nobody is doing that.

And yes I agree that it would be probably better to have --bisect be the
name of the revision machinery option and --bisect-refs or perhaps another
name like --bisect-compute be the name of the special option to "git
rev-list".

But perhaps we can introduce --bisect-compute to do the same thing
that --bisect currently does and deprecate --bisect with a warning and then
a few versions later remove it and after a few more versions
introduce --bisect to do the same as --bisect-refs.

Best regards,
Christian.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Junio C Hamano]

Christian Couder <[hidden email]> writes:

> So to do that you would use "git bisect start ..." and then you could use:
>
> $ git rev-list --bisect HEAD --not $GOOD_COMMITS
>
> to get the commit that you would have to test if the current commit is bad
> and:
>
> $ git rev-list --bisect  $BAD --not $GOOD_COMMITS HEAD
>
> to get the commit that you would have to test if the current commit is good.

Even in that case, the problem is still about narrowing the set of the
current bisection graph.  If --bisect option implicitly grabs good and bad
defined in the refspace like Linus's patch does, it will give you the same
behaviour of the above two commands, no?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Christian Couder]

On Wednesday 04 November 2009, Junio C Hamano wrote:

> Christian Couder <[hidden email]> writes:
> > So to do that you would use "git bisect start ..." and then you could
> > use:
> >
> > $ git rev-list --bisect HEAD --not $GOOD_COMMITS
> >
> > to get the commit that you would have to test if the current commit is
> > bad and:
> >
> > $ git rev-list --bisect  $BAD --not $GOOD_COMMITS HEAD
> >
> > to get the commit that you would have to test if the current commit is
> > good.
>
> Even in that case, the problem is still about narrowing the set of the
> current bisection graph.  If --bisect option implicitly grabs good and
> bad defined in the refspace like Linus's patch does, it will give you the
> same behaviour of the above two commands, no?

I think it will probably work when you add a good rev, but in the case where
you give a different bad (the first command above) it does not work the
same.

The test case in patch 1/3 shows that. It does:

git bisect start $HASH7 $HASH1 &&
...
rev_list2=$(git rev-list --bisect $HASH3 --not $HASH1) &&
test "$rev_list2" = "$HASH2"

and that last command fails.

Best regards,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html