On 07-04-2024 17:12, phillip.wood123@xxxxxxxxx wrote:
On 07/04/2024 15:52, Olliver Schinagl wrote:
Hey Phillip,
On 07-04-2024 16:09, phillip.wood123@xxxxxxxxx wrote:
On 06/04/2024 20:17, Olliver Schinagl wrote:
Hey Phillip,
On 06-04-2024 15:50, Phillip Wood wrote:
Hi Olliver
On 06/04/2024 11:06, Olliver Schinagl wrote:
On 06-04-2024 03:08, Junio C Hamano wrote:
Olliver Schinagl <oliver@xxxxxxxxxxx> writes:
If you search builtin/bisect.c you'll see some existing callers of
strbuf_read_file() that read other files like BISECT_START. Those
callers should give you an idea of how to use it.
Yeah, I found after Junio's hint :) What threw me off, as I wrote
earlier, get_terms(). I wonder now, why is get_terms() implemented
as it is, and should it not use the same functions? Or is it because
terms is a multi-line file, whereas the others are all single line
(I didn't look, though I see addline functions for the strbuf
functions. Should this be refactored?
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
I suppose; I'd think that there would be a strbuf function call to do
exactly (more or less) of what was needed. But I'll let it go ;)
So with the name, I started to think some more about it, and after
playing with some names, I settled on 'bisect-post-checkout'. Things
then sort of fell more into place. It is still a hook/commandline
option, but it's a much smaller change (since we don't have any
special code to check the exit code anymore) as we can (obviously)
run `git bisect skip` instead of `exit 125` as well of course.
Does that mean you will be starting "git bisect skip" from the script
run by the current "git bisect" process. I don't think calling git
recursively like that is a good idea as you'll potentially end up
with a bunch of "git bisect" processes all waiting for their post
checkout script to finish running.
Well the process is inherently recursive, though that's up to the user
depending on what they put in their script of course. I don't think
git is 'waiting' is it? In that, git bisect runs the command, the
command runs git bisect, git bisect stores the commit hash in the skip
file and 'exists', which goes then back to the bisect job, which then
continues as it normally would.
So technically, we're not doing anything bad in git, but a user might
do something bad.
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.
Hmm, you might be right. I was thinking that `git bisect skip` would put
the hash in the file, and then exit, but of course it also goes to the
next checkout and thus triggers the script again (potentially), we don't
want that. We do want the hash to end up in the file, but then not
continue, as that would be the job of git bisect.
So then I go back to my previous solution, which expects exit code 125,
like the other case in bisect. That shouldn't cause that behavior, as
we'd otherwise have the same problem with the other exit code 125.
Thank you,
Olliver
Best Wishes
Phillip
