From: Olliver Schinagl <oliver@schinagl.nl>
To: Junio C Hamano <gitster@pobox.com>
Cc: phillip.wood123@gmail.com, phillip.wood@dunelm.org.uk,
git@vger.kernel.org,
Christian Couder <christian.couder@gmail.com>,
Stefan Haller <lists@haller-berlin.de>
Subject: Re: [RFC] bisect: Introduce skip-when to automatically skip commits
Date: Wed, 10 Apr 2024 21:22:31 +0200 [thread overview]
Message-ID: <bb0dc4a7-e598-45f5-b707-e22de0890f26@schinagl.nl> (raw)
In-Reply-To: <xmqqzfu1upty.fsf@gitster.g>
On 10-04-2024 18:47, Junio C Hamano wrote:
> Olliver Schinagl <oliver@schinagl.nl> 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.
next prev parent reply other threads:[~2024-04-10 19:22 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-30 8:10 [RFC] bisect: Introduce skip-when to automatically skip commits Olliver Schinagl
2024-04-05 6:50 ` Olliver Schinagl
2024-04-06 1:08 ` Junio C Hamano
2024-04-06 10:06 ` Olliver Schinagl
2024-04-06 13:50 ` Phillip Wood
2024-04-06 19:17 ` Olliver Schinagl
2024-04-07 14:09 ` phillip.wood123
2024-04-07 14:52 ` Olliver Schinagl
2024-04-07 15:12 ` phillip.wood123
2024-04-07 21:11 ` Olliver Schinagl
2024-04-08 16:49 ` Junio C Hamano
2024-04-10 10:39 ` Olliver Schinagl
2024-04-10 16:47 ` Junio C Hamano
2024-04-10 19:22 ` Olliver Schinagl [this message]
2024-04-10 19:31 ` Junio C Hamano
2024-04-10 19:39 ` Olliver Schinagl
2024-04-12 13:35 ` Phillip Wood
2024-04-06 17:07 ` Junio C Hamano
2024-04-06 19:19 ` Olliver Schinagl
2024-04-06 10:22 ` Olliver Schinagl
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bb0dc4a7-e598-45f5-b707-e22de0890f26@schinagl.nl \
--to=oliver@schinagl.nl \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=lists@haller-berlin.de \
--cc=phillip.wood123@gmail.com \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).