Subject: Re: [RFC] bisect: Introduce skip-when to automatically skip commits

From: Junio C Hamano <[email-masked]>
Date: Mon, 08 Apr 2024 09:49:14 -0700

[email-masked] writes:

>>> get_terms() wants to read the first line into `term_bad` and the
>>> second line into `term_good` so it makes sense that it uses two
>>> calls to `strbuf_getline()` to do that. It does not want to read
>>> the whole file into a single buffer as we do here.
>> Right, but I why not use strbuf_getline()?
>
> Because you want the whole file, not just one line as the script name
> could potentially contain a newline

It is technically true, but it somehow sounds like an implausible
scenario to me.  The real reason why read_file() is preferrable is
because you do not have to write, and we do not want to see you write,
the whole "open (and handle error), read, chomp, and return" sequence.

I would even suspect that get_terms() is a poorly written
anti-pattern.  If I were adding that function to the system today, I
wouldn't be surprised if I did read_file() the whole thing and
worked in-core to split two items out.

> If I understand correctly we're encouraging the user to run "git
> bisect skip" from the post checkout script. Doesn't that mean we'll
> end up with a set of processes that look like
>
> 	- git bisect start
> 	  - post checkout script
>             - git bisect skip
>               - post checkout script
>                 - git bisect skip
>                   ...
>
> as the "git bisect start" is waiting for the post checkout script to
> finish running, but that script is waiting for "git bisect skip" to
> finish running and so on. Each of those processes takes up system
> resources, similar to how a recursive function can exhaust the
> available stack space by calling itself over and over again.

True.  What such a post-checkout script can do is to only mark the
HEAD as "untestable", just like a run script given to "bisect run"
signals that fact by returnint 125.  And at that point, I doubt it
makes sense to add such a post-checkout script for the purpose of
allowing "bisect skip".

Having said that, a post-checkout script and pre-resume script may
have a huge value in helping those whose tests cannot be automated
(in other words, they cannot do "git bisect run") when they need to
tweak the working tree during bisection.  We all have seen, during a
bisection session that spans a segment of history that has another
bug that affects our test *but* is orthogonal to the bug we are
chasing, that we "cherry-pick --no-commit" the fix for that other
problem inside "git bisect run" script.  It might look something
like

    #!/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

    make test
    status=$?
    git reset --hard ;# undo the cherry-pick
    exit $status

But to those whose test is not a good match to "git bisect run", if
we had a mechanism to tweak the checked out working tree after the
"bisect next" (which is an internal mechanism that "bisect good",
"bisect bad", and "bisect skip" share to give you the next HEAD and
the working tree to test) checks out the working tree before it
gives the control back to you, we could split the above script into
two parts and throw the "conditionally cherry-pick the fix" part
into that mechanism.  We'd need to have a companion script to "redo
the damage" (the "reset --hard" in the above illustration) if this
were to work seamlessly.  That obviously is totally orthogonal to
what we are discussing in this thread, but may make a good #leftoverbits
material (but not for novices).