feature request - telling git bisect to skip, from inside a commit

feature request - telling git bisect to skip, from inside a commit

[Jim Cromie]

sometimes its feels clearer to devote a commit to changing (for example)
the definition of a struct;  and changing all users of that struct in
the next commit.
This isolates and highlights the definitional change, rather than burying it in
the middle of a huge diff.

The downside of doing this is that git bisect will trip over this 1/2 change.
It would be nice if a committer could mark the commit as not bisectable,
perhaps by just adding this, on a separate line, to the commit-message:

    "git bisect skip [optional range]"

the range presumably would be something like CURRENT^1
except that it would make more sense to flag successors than ancestors,
and of course, CURRENT would have to mean something.

Anyway, range isnt really needed, as any subsequent interim commits
could also flag themselves as such.

git bisect already has ability to skip a commit, this just helps an automated
bisection script.

Re: feature request - telling git bisect to skip, from inside a commit

[Christian Couder]

Hi,

On Saturday 26 March 2011 19:48:25 Jim Cromie wrote:
> sometimes its feels clearer to devote a commit to changing (for example)
> the definition of a struct;  and changing all users of that struct in
> the next commit.
> This isolates and highlights the definitional change, rather than burying
> it in the middle of a huge diff.
> 
> The downside of doing this is that git bisect will trip over this 1/2
> change. It would be nice if a committer could mark the commit as not
> bisectable, perhaps by just adding this, on a separate line, to the
> commit-message:
> 
>     "git bisect skip [optional range]"
> 
> the range presumably would be something like CURRENT^1
> except that it would make more sense to flag successors than ancestors,
> and of course, CURRENT would have to mean something.
> 
> Anyway, range isnt really needed, as any subsequent interim commits
> could also flag themselves as such.
> 
> git bisect already has ability to skip a commit, this just helps an
> automated bisection script.

Please look at this recent thread:

http://thread.gmane.org/gmane.comp.version-control.git/169026/

where there are other suggestions and discussions about how to deal with this 
kind of problems.

Thanks,
Christian.

Re: feature request - telling git bisect to skip, from inside a commit

[Jeff King]

On Sat, Mar 26, 2011 at 12:48:25PM -0600, Jim Cromie wrote:

> sometimes its feels clearer to devote a commit to changing (for example)
> the definition of a struct;  and changing all users of that struct in
> the next commit.
> This isolates and highlights the definitional change, rather than burying it in
> the middle of a huge diff.
> 
> The downside of doing this is that git bisect will trip over this 1/2 change.
> It would be nice if a committer could mark the commit as not bisectable,
> perhaps by just adding this, on a separate line, to the commit-message:
> 
>     "git bisect skip [optional range]"
> 
> the range presumably would be something like CURRENT^1
> except that it would make more sense to flag successors than ancestors,
> and of course, CURRENT would have to mean something.

That could work, though I would spell it as a pseudo-header:

  Bisect: Skip

at the end of each commit. But I'm not sure a range makes sense. Commits
can get rebased, or patches applied in a different order. So putting
something like that into the message at commit time can be fragile.

I wonder if doing this at commit time at all really makes sense, though.
It means you have to know up-front that you are breaking things. Which
yes, does happen. But it also happens frequently that you only realize
later while bisecting that your commit is bogus. Or maybe you realize
two commits down the line that your HEAD~2 is broken, but you can't use
"rebase -i" to fix it because you already pushed it.

So what if instead of marking at commit time, we kept a "skip cache".
Bisection would consult any entries marked in the skip cache and skip
them automatically. When you issued a "git bisect skip", it would not
only skip the commits now, but would also add them to the skip cache.

If we kept the skip cache in a git-notes tree, then you could share the
cache with others (though to be fair, this is slightly less convenient
than simply having it in the commit header, which survives over things
like format-patch).

And of course nothing would stop you from marking an item in the skip
cache at commit time, or any other time.

One downside to this scheme (or any skip-marking scheme) is that some
skips may depend on the bisection you are doing. Obviously if the
program doesn't build, that breaks everyone. But let's say you skip a
series of commits because they have a bug in the "foo" program, and your
bisection script must run "foo" then "bar" to get its result. But later,
you do another bisection that doesn't care about "foo" at all, but the
result of the bisection would be in the middle of the skipped series.
You won't find it exactly, because you are skipping too many commits.

> git bisect already has ability to skip a commit, this just helps an
> automated bisection script.

If you have an automated script, you could do something like this
outside of git-bisect entirely.

When you want to mark a commit as bad, do:

  git notes --ref=skip add -m "some reason it is bogus" <commit>

Then at the beginning of your test script, do something like:

  skip=`git notes --ref=skip show`
  if test -n "$skip"; then
    echo >&2 "Skipping commit: $skip"
    exit 125
  fi

Of course, for the example you gave, it may be even easier. The code in
your patch 1/2 would fail to compile. So just doing "make || exit 125"
in your bisect script would be enough, without any skip cache. But there
are certainly cases where it is nice to be able to mark something as
skippable (e.g., a kernel that builds, but is flaky on your test
hardware. You would much rather save the build and reboot just to find
out it is broken).

-Peff

Re: feature request - telling git bisect to skip, from inside a commit

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> That could work, though I would spell it as a pseudo-header:
>
>   Bisect: Skip
>
> at the end of each commit.

I think that is a saner approach, and further say it would be much saner
to make that token something like

    Broken: does not build

A commit may or may not build, a build product may work in some areas just
fine and may have known bugs in some other areas, depending on what kind
of breakage you are interested in.  You might even be looking for a change
that fixed a bug for cherry picking.  In short, "Bisect: Skip" is too
broad a brush, and does not convey enough information.

And then teach the script you give to "bisect run" to grep for that
"^Broken: " pattern to answer with exit 125 (cannot test), and you are
done.

Re: feature request - telling git bisect to skip, from inside a commit

[Jim Cromie]

On Mon, Mar 28, 2011 at 10:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> That could work, though I would spell it as a pseudo-header:
>>
>>   Bisect: Skip
>>
>> at the end of each commit.

I like this generically.  It is more constrained than a
/^bisect skip(.*)$/ matching, and perhaps easier to code.

Thank you for reading into my request and formulating a better version,
the discussion/explication of issues also helps.

If the pseudo-header can be added into the commit message,
it is trivial to use.

I see the merit of a skip-cache as working after-the-fact
on already published/shared patch-sets, but Im unsure how the
skip-cache can be shared currently.

>
> I think that is a saner approach, and further say it would be much saner
> to make that token something like
>
>    Broken: does not build

My only concern here is the negative connotation of Broken.
At least in my 1/2 change scenario, thats known to be incomplete,
its kinda pejorative.  But it certainly gets the job done.

Skip: make rc 125
Skip: 125
Skip: gcc error: no such field: foo
Skip: api change only, users must follow

Skip: has less pejorative, and closer name-association linkage
to the bisect skip command that it

>
> A commit may or may not build, a build product may work in some areas just
> fine and may have known bugs in some other areas, depending on what kind
> of breakage you are interested in.  You might even be looking for a change
> that fixed a bug for cherry picking.  In short, "Bisect: Skip" is too
> broad a brush, and does not convey enough information.
>

agreed -  It would be interesting, 6 months after the feature is added (I hope),
 to search commit messages and see how the header has been used.

> And then teach the script you give to "bisect run" to grep for that
> "^Broken: " pattern to answer with exit 125 (cannot test), and you are
> done.
>

With this, nothing needs to be added.
The only advantage to bisect looking for the pseudo-header is that
a convention is supported, such that it might get used.
I suppose a 1 line addition to git bisect run documentation would
do just as well.

thanks

Re: feature request - telling git bisect to skip, from inside a commit

[Jeff King]

On Tue, Mar 29, 2011 at 11:00:00AM -0600, Jim Cromie wrote:

> If the pseudo-header can be added into the commit message,
> it is trivial to use.
> 
> I see the merit of a skip-cache as working after-the-fact
> on already published/shared patch-sets, but Im unsure how the
> skip-cache can be shared currently.

If you use git-notes as I did in the example of my first response, then
you will end up with a ref "refs/notes/skip". You can push it to
wherever:

  git push origin refs/notes/skip

and then everybody else can configure their repos to fetch it:

  git config --add remote.origin.fetch \
    +refs/notes/skip:refs/notes/origin/skip

I think they would still have to manually merge it into their own skip
cache with "git notes merge". I'm not sure. The notes-merging code is
very new (v1.7.4, I believe), and I think we are still figuring out what
workflows make sense around it (which is why it is still manual).

> > I think that is a saner approach, and further say it would be much saner
> > to make that token something like
> >
> >    Broken: does not build
> 
> My only concern here is the negative connotation of Broken.
> At least in my 1/2 change scenario, thats known to be incomplete,
> its kinda pejorative.  But it certainly gets the job done.
> 
> Skip: make rc 125
> Skip: 125
> Skip: gcc error: no such field: foo
> Skip: api change only, users must follow
> 
> Skip: has less pejorative, and closer name-association linkage
> to the bisect skip command that it

Yeah, I think a more neutral name makes sense, since there are many
reasons to skip (and in fact, broken builds are among the least
interesting to mark, since those are easy to detect at bisection time).

> > And then teach the script you give to "bisect run" to grep for that
> > "^Broken: " pattern to answer with exit 125 (cannot test), and you are
> > done.
> >
> 
> With this, nothing needs to be added.
> The only advantage to bisect looking for the pseudo-header is that
> a convention is supported, such that it might get used.

If you do use this, I would love to hear a report on how it works 6
months in. If it's useful, then it might make more sense to clean up
rough edges with better tool support. Or maybe it works perfectly
without tool support, or maybe it turns out to be a flawed idea. :)

-Peff