git-bisect working only from toplevel dir

git-bisect working only from toplevel dir

[Adam Borowski]

[-- Attachment #1.1: Type: text/plain, Size: 487 bytes --]

Hi!

The requirement to be in the toplevel directory when calling git-bisect is
pretty infuriating.  I tried to find an explanation for this, and the only
reference I found was:

http://thread.gmane.org/gmane.comp.version-control.git/27524/focus=27596

However, since then, git-reset has been changed (in a81c311f).  What about
changing git-bisect as well?

A trivial patch seems to work for me, but I might have missed some corner
case.

-- 
1KB		// Yo momma uses IPv4!

[-- Attachment #1.2: 0001-git-bisect-allow-using-it-from-a-subdirectory.patch --]
[-- Type: text/x-diff, Size: 740 bytes --]

From 1dd5dda6a9db3d987e15784c4de24e593cc596e0 Mon Sep 17 00:00:00 2001
From: Adam Borowski <kilobyte@angband.pl>
Date: Wed, 23 Nov 2011 15:08:42 +0100
Subject: [PATCH] git-bisect: allow using it from a subdirectory.

Just like git-reset, restricting it to toplevel is an annoyance, and the
latter has been changed in a81c311f.
---
 git-bisect.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 99efbe8..fd6ccdd 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -27,6 +27,7 @@ git bisect run <cmd>...
 Please use "git help bisect" to get the full man page.'
 
 OPTIONS_SPEC=
+SUBDIRECTORY_OK=Yes
 . git-sh-setup
 . git-sh-i18n
 
-- 
1.7.8.rc3.31.g017d1


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: git-bisect working only from toplevel dir

[Junio C Hamano]

Adam Borowski <kilobyte@angband.pl> writes:

> The requirement to be in the toplevel directory when calling git-bisect is
> pretty infuriating.  I tried to find an explanation for this, and the only
> reference I found was:
>
> http://thread.gmane.org/gmane.comp.version-control.git/27524/focus=27596

Interesting. It used to be that people were thankful when a command
happened to work from a subdirectory, and it was a minor irritation when
some command didn't; in the early days, everything in Git was to be used
from the top-legvel.

> However, since then, git-reset has been changed (in a81c311f).  What about
> changing git-bisect as well?
>
> A trivial patch seems to work for me, but I might have missed some corner
> case.

Thanks; read and follow Documentation/SubmittingPatches the next time
perhaps?

As to the approach, I suspect that it would be far better if it made
workable with cd_to_toplevel at the beginning, instead of saying
SUBDIRECTORY_OK.

After all, the current directory may disappear during the course of
bisection, upon checking out a revision that did not have the directory
you started your bisection from.

>
> -- 
> 1KB		// Yo momma uses IPv4!
>
> From 1dd5dda6a9db3d987e15784c4de24e593cc596e0 Mon Sep 17 00:00:00 2001
> From: Adam Borowski <kilobyte@angband.pl>
> Date: Wed, 23 Nov 2011 15:08:42 +0100
> Subject: [PATCH] git-bisect: allow using it from a subdirectory.
>
> Just like git-reset, restricting it to toplevel is an annoyance, and the
> latter has been changed in a81c311f.
> ---
>  git-bisect.sh |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 99efbe8..fd6ccdd 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -27,6 +27,7 @@ git bisect run <cmd>...
>  Please use "git help bisect" to get the full man page.'
>  
>  OPTIONS_SPEC=
> +SUBDIRECTORY_OK=Yes
>  . git-sh-setup
>  . git-sh-i18n

Re: git-bisect working only from toplevel dir

[Jeff King]

On Wed, Nov 23, 2011 at 11:09:29AM -0800, Junio C Hamano wrote:

> As to the approach, I suspect that it would be far better if it made
> workable with cd_to_toplevel at the beginning, instead of saying
> SUBDIRECTORY_OK.
> 
> After all, the current directory may disappear during the course of
> bisection, upon checking out a revision that did not have the directory
> you started your bisection from.

But from what directory would you expect:

  git bisect run make

to run from? If you use a GNU-ish layout with all of your code in
"src/", then I can see it useful to do something like:

  cd src
  git bisect run make

If we cd_to_toplevel, we can remember the prefix that we started from
and cd to it before running the user's command, but there is no
guarantee that it actually exists. Maybe that commit should be
considered indeterminate then?

I dunno. I haven't thought that hard about it. But I don't think it's
quite as simple as just telling bisect it's OK to run from a subdir.

-Peff

Re: git-bisect working only from toplevel dir

[Adam Borowski]

On Wed, Nov 23, 2011 at 02:23:29PM -0500, Jeff King wrote:
> On Wed, Nov 23, 2011 at 11:09:29AM -0800, Junio C Hamano wrote:
> 
> > As to the approach, I suspect that it would be far better if it made
> > workable with cd_to_toplevel at the beginning, instead of saying
> > SUBDIRECTORY_OK.
> > 
> > After all, the current directory may disappear during the course of
> > bisection, upon checking out a revision that did not have the directory
> > you started your bisection from.

No different from git-reset or git-checkout.
> 
> But from what directory would you expect:
> 
>   git bisect run make
> 
> to run from? If you use a GNU-ish layout with all of your code in
> "src/",

In a vast majority of cases the layout remains constant during the whole
bisection.

> then I can see it useful to do something like:
> 
>   cd src
>   git bisect run make
> 
> If we cd_to_toplevel, we can remember the prefix that we started from
> and cd to it before running the user's command, but there is no
> guarantee that it actually exists.

I guess, the best that can be done is going into as many path components as
possible.

> Maybe that commit should be considered indeterminate then?

Why?  If you're running an automated command, then it will probably fail,
yeah.  I guess most people bisect manually though, so even in repositories
that do have this problem, there's someone who can test the given commit
anyway.

> I dunno. I haven't thought that hard about it. But I don't think it's
> quite as simple as just telling bisect it's OK to run from a subdir.

At the very least, generally working with a caveat in corner cases seems to
be better than outright failing.

If you're paranoid, there's an option of having a config setting "yes, I've
read the manual why automated bisection can fail".

-- 
1KB		// Yo momma uses IPv4!

Re: git-bisect working only from toplevel dir

[Jeff King]

On Wed, Nov 23, 2011 at 09:09:20PM +0100, Adam Borowski wrote:

> > But from what directory would you expect:
> > 
> >   git bisect run make
> > 
> > to run from? If you use a GNU-ish layout with all of your code in
> > "src/",
> 
> In a vast majority of cases the layout remains constant during the whole
> bisection.

Agreed. But you need to think about what happens when it does not. I
think marking the commit as untestable is probably best, with bisect
barfing a reasonable second. Accidentally marking the commit as "bad" is
probably the worst thing we could do. That would produce a subtly wrong
bisection result.

> > Maybe that commit should be considered indeterminate then?
> 
> Why?  If you're running an automated command, then it will probably fail,
> yeah.  I guess most people bisect manually though, so even in repositories
> that do have this problem, there's someone who can test the given commit
> anyway.

If you're not doing "bisect run", then it is a non-issue, no?  If you
are bisecting by hand, then "git bisect good|bad" will delete your
working directory, and probably your shell will start complaining, and
an intelligent tester will see what happened. This is only a problem for
automated bisection, which does not have such a tester.

> > I dunno. I haven't thought that hard about it. But I don't think it's
> > quite as simple as just telling bisect it's OK to run from a subdir.
> 
> At the very least, generally working with a caveat in corner cases seems to
> be better than outright failing.

To be clear: I think this is a good feature that will help a lot of
people, and I don't think an uncommon corner case should prevent it from
going into git.  But I _do_ think we should consider what happens in the
corner cases and at least fail gracefully, rather than produce subtly
wrong results.

-Peff

Re: git-bisect working only from toplevel dir

[Peter Baumann]

On Wed, Nov 23, 2011 at 02:23:29PM -0500, Jeff King wrote:
> On Wed, Nov 23, 2011 at 11:09:29AM -0800, Junio C Hamano wrote:
> 
> > As to the approach, I suspect that it would be far better if it made
> > workable with cd_to_toplevel at the beginning, instead of saying
> > SUBDIRECTORY_OK.
> > 
> > After all, the current directory may disappear during the course of
> > bisection, upon checking out a revision that did not have the directory
> > you started your bisection from.
> 
> But from what directory would you expect:
> 
>   git bisect run make
> 
> to run from? If you use a GNU-ish layout with all of your code in
> "src/", then I can see it useful to do something like:
> 
>   cd src
>   git bisect run make
> 
> If we cd_to_toplevel, we can remember the prefix that we started from
> and cd to it before running the user's command, but there is no
> guarantee that it actually exists. Maybe that commit should be
> considered indeterminate then?
> 

Why not simply fail the run with exit(-1)? If the directory doesn't exist
in an older commit (which I think is not that common) git bisect should
simply stop and let the user proceed. 

And yes, I find the current behaviour to forbid running git bisect from
a subdirectory slighly annoying and I'm glad somebody took a stab at it.

-Peter

Re: git-bisect working only from toplevel dir

[Jeff King]

On Wed, Nov 23, 2011 at 09:26:43PM +0100, Peter Baumann wrote:

> > If we cd_to_toplevel, we can remember the prefix that we started from
> > and cd to it before running the user's command, but there is no
> > guarantee that it actually exists. Maybe that commit should be
> > considered indeterminate then?
> > 
> 
> Why not simply fail the run with exit(-1)? If the directory doesn't exist
> in an older commit (which I think is not that common) git bisect should
> simply stop and let the user proceed.

The point of "git bisect run" is to run unattended until we reach an
answer. I don't think most people would be happy with it not running to
come to _some_ answer (e.g., imagine checking the results of an
overnight "bisect run" in the morning only to find that it stopped 20
minutes in).

That's why I think just marking the commit as indeterminate would be
better; it jumps over parts of history that omit the directory, and will
generally still come to a good conclusion. If it's possible to get an
answer, that is. It might say "we can't come up with an answer because
all of these commits are not testable". But that tells you something,
too: your bisection test is not a good one.

> And yes, I find the current behaviour to forbid running git bisect from
> a subdirectory slighly annoying and I'm glad somebody took a stab at it.

Agreed. I often bisect by hand with two terminals, doing something like:

  [terminal 1]
  git bisect start ...
  make

  [terminal 2]
  cd t
  ./t1234-whatever -v

  [terminal 1]
  git bisect good|bad
  make

  [terminal 2]
  ./t1234-whatever

And then want to type "git bisect good|bad" into terminal 2. Which
doesn't work, of course (yes, in this simple case I could automate the
running of the test script from terminal 1; but often times it is
simpler to just eyeball the output during the bisection).

-Peff

Re: git-bisect working only from toplevel dir

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Nov 23, 2011 at 11:09:29AM -0800, Junio C Hamano wrote:
>
>> As to the approach, I suspect that it would be far better if it made
>> workable with cd_to_toplevel at the beginning, instead of saying
>> SUBDIRECTORY_OK.
>> 
>> After all, the current directory may disappear during the course of
>> bisection, upon checking out a revision that did not have the directory
>> you started your bisection from.
>
> But from what directory would you expect:
>
>   git bisect run make

My usual way to enlighten somebody is by forcing him/her to think the
consequences, but because you did the thinking for the OP in this thread
instead, it didn't work. Makes me somewhat sad ;-<.

> If we cd_to_toplevel, we can remember the prefix that we started from
> and cd to it before running the user's command, but there is no
> guarantee that it actually exists. Maybe that commit should be
> considered indeterminate then?

Yeah that sounds like a reasonable thing to do.

> I dunno. I haven't thought that hard about it. But I don't think it's
> quite as simple as just telling bisect it's OK to run from a subdir.

Absolutely. Saying SUBDIRECTORY_OK without thinking about the consequence
through is a good discussion starter but is not a good patch.

Also didn't we make bisect workable in a bare repository recently? So the
start-up sequence has to be something more elaborate like...

        . git-sh-setup
        if we are in a bare repository
        then
         	: we are happy...nothing funky needs to be done
	elif we are not in a working tree
		barf
	elif we are not at the top
        	prefix=$(git rev-parse --show-prefix)
		cd_to_toplevel
	fi

and then inside bisect_next() you would check if $prefix exists, and go
there to run bisect--helper (or fail to go there and say "cannot test").

Re: git-bisect working only from toplevel dir

[Peter Baumann]

On Wed, Nov 23, 2011 at 12:45:18PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Nov 23, 2011 at 11:09:29AM -0800, Junio C Hamano wrote:
> >
> >> As to the approach, I suspect that it would be far better if it made
> >> workable with cd_to_toplevel at the beginning, instead of saying
> >> SUBDIRECTORY_OK.
> >> 
> >> After all, the current directory may disappear during the course of
> >> bisection, upon checking out a revision that did not have the directory
> >> you started your bisection from.
> >
> > But from what directory would you expect:
> >
> >   git bisect run make
> 
> My usual way to enlighten somebody is by forcing him/her to think the
> consequences, but because you did the thinking for the OP in this thread
> instead, it didn't work. Makes me somewhat sad ;-<.
> 
> > If we cd_to_toplevel, we can remember the prefix that we started from
> > and cd to it before running the user's command, but there is no
> > guarantee that it actually exists. Maybe that commit should be
> > considered indeterminate then?
> 
> Yeah that sounds like a reasonable thing to do.
> 
> > I dunno. I haven't thought that hard about it. But I don't think it's
> > quite as simple as just telling bisect it's OK to run from a subdir.
> 
> Absolutely. Saying SUBDIRECTORY_OK without thinking about the consequence
> through is a good discussion starter but is not a good patch.
> 
> Also didn't we make bisect workable in a bare repository recently? So the
> start-up sequence has to be something more elaborate like...
> 
>         . git-sh-setup
>         if we are in a bare repository
>         then
>          	: we are happy...nothing funky needs to be done
> 	elif we are not in a working tree
> 		barf
> 	elif we are not at the top
>         	prefix=$(git rev-parse --show-prefix)
> 		cd_to_toplevel
> 	fi
> 
> and then inside bisect_next() you would check if $prefix exists, and go
> there to run bisect--helper (or fail to go there and say "cannot test").
> 

But is the "cannot test" aka exit(127) the best we can do in this case?
I think having a failing make, because someone has checked in code which doesn't
even compile, is something totally different than having no Makefile at all,
because the directory doesn't even exist. To me, this seems more like an error
in the run script to not handle all the cases of the (dis)appearing directory.

On the other hand we don't waste much time trying to test such an "untestable"
commit, because this check will be fast because no compiling is involved.
The only time wasted will bethe build time for the "usable" commits and the
time the user needs to figure out why the heck some(/most?) of his commits
are "untestable".

Re: git-bisect working only from toplevel dir

[Junio C Hamano]

Peter Baumann <waste.manager@gmx.de> writes:

> On Wed, Nov 23, 2011 at 12:45:18PM -0800, Junio C Hamano wrote:
> ...
>> Also didn't we make bisect workable in a bare repository recently? So the
>> start-up sequence has to be something more elaborate like...
>> ...
>> and then inside bisect_next() you would check if $prefix exists, and go
>> there to run bisect--helper (or fail to go there and say "cannot test").
>
> But is the "cannot test" aka exit(127) the best we can do in this case?

Yeah, thinking about it a bit more, it may probably be better to make it a
failure. The user explicitly asked "be in _this_ directory and run make;
it should succeed for the bisection test to pass". If the bisection test
criterion the user was interested in was a successful build of the whole
project (not the subpart of the current directory), the user would have
gone up to the top-level and "bisect run make" there.

Re: git-bisect working only from toplevel dir

[Jeff King]

On Thu, Nov 24, 2011 at 03:50:22AM -0800, Junio C Hamano wrote:

> > On Wed, Nov 23, 2011 at 12:45:18PM -0800, Junio C Hamano wrote:
> > ...
> >> Also didn't we make bisect workable in a bare repository recently? So the
> >> start-up sequence has to be something more elaborate like...
> >> ...
> >> and then inside bisect_next() you would check if $prefix exists, and go
> >> there to run bisect--helper (or fail to go there and say "cannot test").
> >
> > But is the "cannot test" aka exit(127) the best we can do in this case?
> 
> Yeah, thinking about it a bit more, it may probably be better to make it a
> failure. The user explicitly asked "be in _this_ directory and run make;
> it should succeed for the bisection test to pass". If the bisection test
> criterion the user was interested in was a successful build of the whole
> project (not the subpart of the current directory), the user would have
> gone up to the top-level and "bisect run make" there.

There are more possibilities than that. For example, imagine a project
with two sibling directories, one a library and one a command that is
built on the library. The library has a bug that we want to bisect, but
the command is the only mechanism we have to test the bug. The command's
Makefile points to the library directory (e.g., using recursive
make[1]).  It would be natural for the user to do something like:

  cd cmd
  make && ./test-cmd
  : hmph, it's broken
  git bisect start
  git bisect bad
  : I think v1.1 was OK
  git checkout v1.1
  make && ./test-cmd
  : Yep, let's run.
  git bisect good
  git bisect run 'make && ./test-cmd'

If, somewhere in the middle, the current directory doesn't exist, then
our test harness does not exist. And we can't say good or bad, but only
"don't know".  Not knowing all of the details of what the user's command
does, that seems to me to be the only safe option.

The worst case is that the bisection takes longer to run and says "I
don't know where the bug is, but it's in this range", and the user has
to go back and run it again with a smarter test. But if we return "no,
the test failed" then we are likely going to just produce nonsensical
results, as our search is hitting on two different errors, and the "bug"
will appear to come and go.

It might be tempting to say that this case can't come up. After all, at
the branch tip the bug is there, and in v1.1 it isn't. What is the
chance that the test harness goes away in the middle? In a linear
history, not hight. But if you have history like this:


       D--*--*--*
      /          \
  *--*--A--B--C---*--E

where:

  - A introduces the "cmd" directory
  - B is v1.1 (known good)
  - C is the location of the actual bug
  - D is on a side branch, but does _not_ have "cmd"
  - E is our current tip (known bad)

then we will have to search down the side branch towards D to look for
the bug.

If this seems contrived, well, it is. In 99% of cases, the directory
_won't_ go away, and none of this will matter.  And of course you can
have this exact same problem even without the directory issue. If your
test command is "make && ./test-harness", and the side branch doesn't
have the harness, then it's going to erroneously report the presence of
the bug. But that's your fault for writing a crappy test command that
wasn't careful about verifying the pre-conditions.

So maybe it doesn't matter; there are a lot of ways to shoot yourself in
the foot with a bisection. I just think git should set a good example
and default to being conservative with its claims.

-Peff