Allow git bisect to auto-skip

In some cases, we know a commit will always break bisect. This is bad and evil but sometimes needed.

An example, in my particular case, when OpenWRT does a kernel bump, patches and config files get copied to the new version. This is fine, but there is not git copy (one could also see that as an individual feature request which would solve the actual issue for now :p). Since the files are copied, git treats them as new files, without history. This makes tracking history almost impossible (config-4.14 -> config 5.15 -> config 6.6 etc). People have found out, there's some tricks to apply where we can make git not see the copy as such, and we'll have our history of both files [0]. In short, this hack is used to create two commits.

git checkout -b _tmp git mv old new git commit -m 'copy old to new' -m 'GIT_SKIP_BISECT' git checkout HEAD~ old git commit -m 'restore old' git switch _tmp git merge _tmp

and to remove the ugly merge commit

git rebase HEAD~1

which surely is a hack :p

But, it's a `git cp` if you will.

Now this does break git bisect, when the commit with the move comes in, things are broken until the next commit. It would be very nice if we can have a special marker keyword (GIT_SKIP_BISECT) in the commit message for example, to force git bisect to just skip this commit. It will fail, it is designed to fail.

There's probably also other very useful cases, but this is just one example.

And hey, if this leads to two patches, where a second one introduces `git cp` I'd be even more happy :)

Olliver

[0]: https://github.com/openwrt/openwrt/blob/main/scripts/kernel_bump.sh

Olliver Schinagl <oliver@schinagl.nl> writes:

If "I want a bisect to skip any commit that has 'Skip Me' in its subject" is the case, perhaps your "git bisect run" script can say

    #!/bin/sh

    case "$(git show -s --oneline)" in
    *"Skip Me"*) exit 125 ;;
    esac

    ... your test script body comes here ...
    if test successful
    then
        exit 0
    else
        exit 1
    fi

The _clue_ to mark a commit to be skipped does not have to be hardcoded commit title. It often is discovered that a commit breaks bisection after the fact and it is not feasible to rebase all the history after the commit. Maybe an approach more suitable in such a situation would attach a note to such untestable commits after the fact, and check if such a note is attached at the beginning of "git bisect run" script and exit with 125.

And a new "git bisect --skip-when <condition>" option can be added to manual bisection process. The <condition> part would contain something like

    case "$(git show -s --oneline)" in
    *"Skip Me"*) exit 125 ;;
    esac

taken from the above illustration.

But I am not sure what end result you are trying to achieve, so the above are random collection of ideas. Turning them into a patch is left as an exercise to readers.

Hey Junio,

On March 22, 2024 11:31:29 p.m. GMT+01:00, Junio C Hamano <gitster@pobox.com> wrote:

This is a nice way to wrap got bisect, but from a UX point of view, having native support from the got client would be much nicer. E.g. the user doesn't have to learn about special scripts. I'd argue, if we can do it the same way everywhere (I.e. shipping such a script as part of the got distro), why not handle it nativily.

The magic word could be a default with an override in the gitconfig.

I've read the notes solution from 13 years ago ;) and I do like the elegance. But I think there's two cases. One during the fact, when you know this will be an issue andere in deed ons after the fact.

I'll admit, I was oblivious by git notes, and will read up on it now. Always using a note for this feature does make sense, as its part of the repository.

In the end, is it (internally) even possible for got bisect to check this, and would a patch that handles this 'behind the scenes' be accepted, without the user having to install special tooling (scripts)?

Having said that, on a different note is 'git copy' feasable? E.g. git CP old new, where both files share the same history?

Olliver

On 22.03.24 23:31, Junio C Hamano wrote:

This reminds me of a similar problem with git blame, for which we have the blame.ignoreRevsFile config to work around it. Couldn't there be a similar mechanism for bisect, e.g. bisect.skipRevsFile?

-Stefan

Stefan Haller <lists@haller-berlin.de> writes:

A Very good point. If a breakage of a commit is "this does not even build" kind of breakage, such a mechanism would be an excellent fit.

But if a breakage is "only this particular test fails and we know the reason why it fails has nothing to do with the bug we are chasing", then compiling such a fixed list of commits, or pointing at such a list with a configuration variable, would not work very well, I am afraid.

Thanks.

On 23-03-2024 19:43, Junio C Hamano wrote:

This changes my view of the issue a little bit. Building vs testing, though in reality its the same of course.

I can totally see that a user would need a special bisect script to handle these cases 'this commit breaks test 54, 43 and 12; but the rest work'. This is too specific to handle generically. Probably in using a similar trick, store in the notes what works and does not work.

I think it still stands that having a 'generic' way of telling bisect to skip the entire commit is still reasonable. Kind of like how you can do `git push -o ci.skip=true` with GitLab.

Olliver

On 23-03-2024 21:51, Olliver Schinagl wrote:

P.S. One (imo big) issue to me is, that notes aren't automatically fetched or pushed according to what I found so far online (haven't personally tested it yet).

If that is the case, this makes the feature to be used in combination with bisect much more difficult for the un-informed. having a `refs/notes/<bisect-or-like-related-name>` notes tree is great, but only if it gets automatically pulled.

I think this also might be the reason it is a very undervalued thing maybe?

Olliver

On 23.03.24 19:43, Junio C Hamano wrote:

That's true, but the same can be said about blame.ignoreRevsFile. There may be commits that contain both uninteresting whitespace changes and real changes (not in a well-maintained project of course :-), so it wouldn't be a good idea to add those to blame.ignoreRevsFile. But that's not a reason not to offer the feature at all.

-Stefan

On Sun, Mar 24, 2024 at 11:16 AM Stefan Haller <lists@haller-berlin.de> wrote:

I am not against the feature, but I think it would be especially useful if the file(s) containing the revs that should be skipped is(are) tracked in Git. In this case though, any such file wouldn't be used automatically after cloning the repo as the bisect.skipRevsFile option would still need to be configured.

Also, how much better would this be compared to tracking "git bisect run" scripts in the repo, even if they have to be copied somewhere else before they are launched? I wonder about this because writing the conditions that decide whether the current commit is good or bad might not be so easy either. So if the goal is to simplify things for users, then simplifying all the way by providing example scripts with comments about how they could be customized might be even better.

On Sunday, March 24, 2024 10:29 AM, Christian Couder wrote:

In some situations, git bisect is used for compile issues, while others in debugging. In development branches, bisect is sufficient. However, if one has multiple production deploy branches, where feature merge squash happens multiple times into multiple branches, this makes finding the offending commit using bisect rather difficult as the defect may come in and out depending on the order in which feature branches are squashed (some may have the bug, while others don't). I can see the point of teaching bisect to knowingly skipping specific versions in this setting, although it is arguable that the investigation probably should not happen on the destination branches where functional stability cannot be guaranteed.

But this brought up a thought. What if bisect had the notion of parallel context: Meaning running two (or more) bisects in parallel on the same repo in search of two semi-independent defects, then analysing convergence between the two analyses - something like

 bisect 1: A(good) B(good) C(bad) D(bad);
 bisect 2: A(good) B(good) C(good) D(bad);
 bisect 3: A(good) B(bad) C(good) D(bad),   <- a production-like branch

with an interpretation that C and D may have interrelated defects but started showing in B in one situation. This is not a well-baked thought, but I can see this having some relevance when sub-modules are involved and bugs exist in more than one sub-module with bisects happening in the main repo. [note: I am not certain where I am going with this idea as of yet].

--Randall

Christian Couder <christian.couder@gmail.com> writes:

If we are driving our bisection session via "bisect run" script, computing the condition that we need to skip in the script is the most natural and obvious thing to do, but the way I guessed (because it was not explicitly written down) what the OP wanted was a way for bisect_next() called after even a manual "git bisect (good|bad)" to automatically skip certain set of commits. Because there are cases where you have to be testing manually and cannot afford to write "bisect run" script, giving a manual bisection a way to compute if a commit need to be skipped may be worth having, and that was where my "git bisect --skip-when <script>" came from. It would not be necessary if you are doing "bisect run", which can dynamically tell if the commit is untestable. And if the user is going to decide after manually testing the one that is suggested, it is not useful either, as the point would be to avoid even asking to test ones that need to be skipped. So it is likely that the set of commits that need skipping is known a-priori before the bisect session even begins. The end user experience may look like:

 * "git bisect start" takes "--skip-when <script>" and remembers it,
   together with other options "start" can take (like <good>, <bad>,
   <terms>, <pathspec>).

 - If <good> and <bad> are already given upon "start", the command
   may check out a revision and ask you to test.

 * Every time the command checks out a revision to be tested by the
   user, the command guarantees if it satisfies the --skip-when
   condition (and internally doing "git bisect skip").

 * Otherwise the interaction between the user and the session is
   exactly the same as usual.

On 24-03-2024 19:34, Junio C Hamano wrote:

I think you understood what I failed to properly explain :)

To come up with an RFC, I was trying to study the git code, and while some things are quite readable, others are a bit complex.

After some poking, I was thinking of using `find_bisection()`, or rather `do_find_bisection()` but got lost there.

What made sense initially was, that in `find_bisection()` there is a simple for loop that goes over the list of commits to count them. But writing this down, I realize the list of commits is already there in `struct commit_list *list`, so I should probably go find out where the list is being created!

Anyway, want I was thinking of, based a key somewhere in the message body (GIT_BISECT_SKIP=1 for example), mark the commit in the list to be skipped, as `git bisect skip` would. This so the skipped commit actualyl ends up on the list of skipped commits (`refs/bisect/skip-*`).

But being a bit lost, it would be nice to get some directional pointers.

Is this even possible? What is the easiest way to get the message from the `struct commit_list *list` item, are there helpers to parse the message at all?

Thanks, Olliver

Olliver Schinagl <oliver@schinagl.nl> writes:

I think it is very unlikely that we'd adopt any hardcoded scheme that allows only one supported way of marking commits as not to be tested. Certainly not the kind that the committer is forced to know the commit must be skipped during a bisection session before creating the commit. So I am not sure how much good it will do to know that commit_list->item is a commit object on the list, or calling repo_get_commit_buffer() on it would give the contents of the commit object.

Knowing that commit_list->item->object.oid is the object name of such a commit, and calling oid_to_hex() on such an object name stringifies it, should be sufficient to write a new code that calls out to a script specified via "git bisect --skip-when=<script>" using the run_command() API to decide if the commit object should be skipped, and if you do want GIT_BISECT_SKIP=1 in the log message, your script given via --skip-when may be a single-liner:

    #!/bin/sh
    # usage: this-script $commit_object_name
    # expected to exit with 0 when the commit should not be tested
    git cat-file commit "$1" | sed -e '1,/^$/d' | grep GIT_BISECT_SKIP=1

Thanks.

On 27-03-2024 20:24, Junio C Hamano wrote:

My goal is to make things work as automagically as possible, without special tooling or scripts. This to make the barrier for entry as low as possible. E.g. if someone says 'could you please bisect this for me', things should just work.

Having said that, I also realize that the use-cases where a commit message would be enough are limited. There's only a very rare cases where it is known before hand a commit will break bisect.

The `GIT_BISECT_SKIP=1` would obviously be configurable, so it could be set on a per-repo level, with a sane default, and probably wiser to use `git commit --skip-bisect` to make use of said flag.

Having said all that, right now, I'm just exploring how things work, how easy it would be, how logical it would be. So for thank you for bearing with me :)

I had learned about `oid_to_hex()` already, and I am charmed by that idea for sure. So much in that I'll look into this first. In the end, both solutions could even be used as they are in nearly the same spot I'd think.

Four things I lack right now. The first one I hope to have figured out before your reply (but don't shy the reply).

1) What is the best way to parse the skip-when argument. I see 
`bisect_start()` does arg parsing, but trying to see what 'the git way' 
is to get the argument of the new flag.

2) Where to store the argument. `bisect_terms` is passed around 
everywhere to hold the good and bad states, but with bisect doesn't pass 
much of anything else around. Though `bisect_run` isn't storing 
`command` either, and you do mention so I'll delve into that and see how 
that maps into the `run_command()` API.

3) Using the `run_command()` API implies to do the check 'on checkout'; 
which was my first idea as well, but a) where does this happen? I have 
not found this yet, though `bisect_run` might be more revealing here,

3) Using this approach, can I still do a 'regular' `git bisect skip` in 
that the skip gets recorded (in refs/bisect/skip-*)? I think it would be 
nice to see what was skipped.

Thanks,

Olliver