On 10-04-2024 18:47, Junio C Hamano wrote:
Olliver Schinagl <oliver@xxxxxxxxxxx> writes:
While completly orthonogal, I agree; it would be nice to have and
'abuse' for the bisect-skip usecase. So if we ignore the fact that it
can be abused for this (which I don't think is a bad thing, it just
risks the recursive issue Phillip mentioned.
I do not see the "recursive" issue here, though. If we had such a
mechansim, those whose test cannot be driven by "bisect run" can
still use the "--post-checkout" and "--pre-resume" options, where
the post-checkout option names a file that has:
#!/bin/sh
if git merge-base --is-ancestor $the_other_bug HEAD
then
# we need the fix
git cherry-pick --no-commit $fix_for_the_other_bug ||
exit 125
fi
in it. There is no "recursive"-ness here. And then after manually
testing the checked out stuff (with tweak, thanks to the post-checkout
script), they can now say "git bisect good/bad/skip" and that is
when their --pre-resume script kicks in, which may do
#!/bin/sh
git reset --hard ;# undo the damage done by post-checkout
before the bisect machinery goes and picks the next commit to test.
Yep, that was all perfectly clear to me :) Though I do admit, I
initially overlooked the 'not' in your comment on 'those whose test
can**not** be driven by "bisect run"' bit.
Notice that I still kept the "exit 125" in the above post-checkout
example? That is where the "bisect next" that picked the commit to
test, checked out that commit and updated the working tree, and run
your post-checkout script, can be told that the version checked out
is untestable and to be skipped.
This is where things got stuck for me. I had the 'exit 125' bit for a
while, but couldn't figure out 'how to mark' stuff. Right now, it just
calls the script, and if you are a bad user, you can call `git bisect
skip` and things work as expected, albeit with the aforementioned recursion.
In the past, as I reported here as well, I tried to capture exit 125,
the above would still be true of course, but exit 125 would be a way to
'catch' this and respond accordingly. The 'accordingly' is where I get
stuck.
See, the hook is named 'post-checkout' and thus, it runs after checkout
has been performed. So we are now on the 'broken' commit we do not want
to test, git should have skipped this already, and not checked it out.
So ok fine, we can call it 'pre-checkout'. But then what. I experimented
with marking the skippyness just after `find_bisection()` here:
https://gitlab.com/olliver/git/-/blob/post_search/bisect.c?ref_type=heads#L1090
with a simple
strbuf_addf(&skip, "refs/bisect/skip-%s", oid_to_hex(oid));
oid_array_append(&skipped_revs, &oid);
While this kinda worked, it failed when two (or more) commits in order
where to be skipped and 'finished' with 'no possible commits to check'
So I kinda gave up here and went back to post-checkout.
So such a post-checkout script can
be treated as a strict superset of --skip-when script we have been
discussing.
Needless to say, if we were to do this, we probably should let
"bisect run" also pay attention to these two scripts. They are most
likely to become new parameters specified when "bisect start" is run
to be recorded as one of the many states "git bisect" creates.
The way I've done it now is that it's called from `bisect_next()` here
https://gitlab.com/olliver/git/-/commit/20dd6f5f0e2a55f940bab1e3aced0686d8dfd0c5#46324e17f99db64a67eb9a5983ffc3a680914ee3_672_717
but as I said, checkout has already commenced. Doing it here seems to
work with bisect run as well. Resume is done in `bisect_state()` here
https://gitlab.com/olliver/git/-/commit/f9b14a66ea5c4c98f48236db119d3eb60427c1bd#46324e17f99db64a67eb9a5983ffc3a680914ee3_1001_1028
which also happens in the run case.
The whole exit 125 and avoid recursion thing, is to me more like an
additional 'nice-to-have' feature. Recursion wouldn't be a huge thing
for modern systems generally anyway in the 'normal/common' case where
you recurse 10-ish times. It'll of course get worse if there's multiple
commits that would need to be skipped. So even if the recursion is
20-ish, it's ugly, but not horrible.
In any case, if the recursion thing is considered bad and must be solved
if a user does this, as I said before it's not clear to me how to
trigger git to say 'oh, I have to go back and do something else; or,
call git bisect skip internally, without causing the recursion; or 'put
the commit in the queue to be skipped, before it's checked out (which of
course is not a 'post-checkout' of course.
