git bisect bad @

git bisect bad @

[Ramkumar Ramachandra]

Hi,

I bumped my head thrice (yes, thrice!) by using `git bisect bad @`. The error displayed to me at the end of one run was the following:

  Cela signifie que le bogue été corrigé entre ea3595845f5013359b2ba4402f948e454350a74c et 
  [2e100906d5d0c276335665ffefedb906d21165ca ea3595845f5013359b2ba4402f948e454350a74c].
  error: la bissection a échoué : 'git bisect--helper --bisect-state (null)' a retourné le code erreur -3

After the third attempt, I realized: ah yes, computers aren't magic; git-bisect.sh is basically a stupid shell script (no offense!).

Perhaps git-bisect.sh can ref-parse the arguments before starting its work? Agreed, none of the refs are expected to change during its operation, with the exception of the sneaky `@`.

Bonne année,
Ram

Re: git bisect bad @

[Junio C Hamano]

"Ramkumar Ramachandra" <r@artagnon.com> writes:

> I bumped my head thrice (yes, thrice!) by using `git bisect bad
> @`. The error displayed to me at the end of one run was the
> following:
>
>   Cela signifie que le bogue été corrigé entre ea3595845f5013359b2ba4402f948e454350a74c et 
>   [2e100906d5d0c276335665ffefedb906d21165ca ea3595845f5013359b2ba4402f948e454350a74c].
>   error: la bissection a échoué : 'git bisect--helper --bisect-state (null)' a retourné le code erreur -3
>
> After the third attempt, I realized: ah yes, computers aren't magic; git-bisect.sh is basically a stupid shell script (no offense!).
>
> Perhaps git-bisect.sh can ref-parse the arguments before starting its work? Agreed, none of the refs are expected to change during its operation, with the exception of the sneaky `@`.

As far as I know, the first thing it does to the command line is to
turn them into concrete object names.  I do not doubt that you had
some problem, and I do not doubt that it was with "git bisect bad"
with arguments, but I somehow doubt your diagnosis is correct.

In git-bisect.sh, we see:

        case "$#" in
        0)
                usage ;;
        *)
                cmd="$1"
                get_terms
                shift
                case "$cmd" in
                help)
                        git bisect -h ;;
                start)
                        git bisect--helper --bisect-start "$@" ;;
                bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
                        git bisect--helper --bisect-state "$cmd" "$@" ;;

So "git bisect--helper --bisect-state bad HEAD" is run in your case.
Now in builtin/bisect--helper.c::bisect_state(), here is what
happpens:

 * argc is checked to make sure at least one arg is there to give
   the 'state' (i.e. good or bad)

 * if there is no arg, the revision that is marked as the given
   'state' defaults to "HEAD"

 * each remaining arg is first passed to get_oid(), turned into a
   commit object.

All of the above should happen way before bisect_next_all() calls
check_good_are_ancestors_of_bad(), that eventually calls
handle_bad_merge_base() where your "The merge base X is bad" error
message comes from.

So, perhaps there is something you are not quite telling us,
e.g. your problem happens during a replay an old bisect session
after HEAD has moved---if we had a bug that records symbolic object
names in the replay log, it may produce a nonsense result in such a
case (but I doubt that is the case---I am reasonably sure that the
log also records concrete object names)?

Perhaps you can try again with a better minimum reproducible
example?

Thanks.

Re: git bisect bad @

[Ramkumar Ramachandra]

Hi,

Junio C Hamano wrote:
> So, perhaps there is something you are not quite telling us,
> e.g. your problem happens during a replay an old bisect session
> after HEAD has moved---if we had a bug that records symbolic object
> names in the replay log, it may produce a nonsense result in such a
> case (but I doubt that is the case---I am reasonably sure that the
> log also records concrete object names)?
> 
> Perhaps you can try again with a better minimum reproducible
> example?

Indeed, I was a bit naive to assume that bisect didn't rev-parse. I'm happy to report that I've found the minimum reproducible example.

  # on coq.git, for those curious
  $ git bisect start
  $ git bisect bad @
  $ git bisect good V8.14.1
  $ git bisect run bisect.sh # oops!
  Lancement de  'bisect.sh'
  'bisect.sh': bisect.sh: command not found
  La base de fusion ea3595845f5013359b2ba4402f948e454350a74c est mauvaise.
  Cela signifie que le bogue été corrigé entre
  ea3595845f5013359b2ba4402f948e454350a74c et [2e100906d5d0c276335665ffefedb906d21165ca].
  error: la bissection a échoué : 'git bisect--helper --bisect-state (null)' a retourné le code erreur -3
  $ git bisect run ./bisect.sh # let's try again!
  # churn ... build ... test
  Cela signifie que le bogue été corrigé entre ea3595845f5013359b2ba4402f948e454350a74c et 
  [2e100906d5d0c276335665ffefedb906d21165ca ea3595845f5013359b2ba4402f948e454350a74c].
  error: la bissection a échoué : 'git bisect--helper --bisect-state (null)' a retourné le code erreur -3

In all three of my runs, there was never a straightforward non-erroring sequence of git-bisect invocations (although my terminal history is lost). Perhaps git-bisect can be hardened a bit, instead of needlessly punishing the user with a long build + test that doesn't lead anywhere?

Warm regards,
Ram

[PATCH] bisect: report actual bisect_state() argument on error

[René Scharfe]

The strvec "args" in bisect_run() is initialized and cleared, but never
added to.  Nevertheless its first member is printed when reporting a
bisect_state() error.  That's not useful, since it's always NULL.

Before d1bbbe45df (bisect--helper: reimplement `bisect_run` shell
function in C, 2021-09-13) the new state was reported if it could not
been set.  Reinstate that behavior and remove the unused strvec.

Reported-by: Ramkumar Ramachandra <r@artagnon.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
This doesn't fix your problem, only replace the "(null)" in the error
messages with the actual state name -- which may be useful for
diagnosing its cause, though.

Patch generated with --function-context for easier review.

 builtin/bisect--helper.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 28a2e6a575..1dbc6294ef 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -1092,80 +1092,76 @@ static int bisect_visualize(struct bisect_terms *terms, const char **argv, int a
 static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 {
 	int res = BISECT_OK;
 	struct strbuf command = STRBUF_INIT;
-	struct strvec args = STRVEC_INIT;
 	struct strvec run_args = STRVEC_INIT;
 	const char *new_state;
 	int temporary_stdout_fd, saved_stdout;

 	if (bisect_next_check(terms, NULL))
 		return BISECT_FAILED;

 	if (argc)
 		sq_quote_argv(&command, argv);
 	else {
 		error(_("bisect run failed: no command provided."));
 		return BISECT_FAILED;
 	}

 	strvec_push(&run_args, command.buf);

 	while (1) {
-		strvec_clear(&args);
-
 		printf(_("running %s\n"), command.buf);
 		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);

 		if (res < 0 || 128 <= res) {
 			error(_("bisect run failed: exit code %d from"
 				" '%s' is < 0 or >= 128"), res, command.buf);
 			strbuf_release(&command);
 			return res;
 		}

 		if (res == 125)
 			new_state = "skip";
 		else if (!res)
 			new_state = terms->term_good;
 		else
 			new_state = terms->term_bad;

 		temporary_stdout_fd = open(git_path_bisect_run(), O_CREAT | O_WRONLY | O_TRUNC, 0666);

 		if (temporary_stdout_fd < 0)
 			return error_errno(_("cannot open file '%s' for writing"), git_path_bisect_run());

 		fflush(stdout);
 		saved_stdout = dup(1);
 		dup2(temporary_stdout_fd, 1);

 		res = bisect_state(terms, &new_state, 1);

 		fflush(stdout);
 		dup2(saved_stdout, 1);
 		close(saved_stdout);
 		close(temporary_stdout_fd);

 		print_file_to_stdout(git_path_bisect_run());

 		if (res == BISECT_ONLY_SKIPPED_LEFT)
 			error(_("bisect run cannot continue any more"));
 		else if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) {
 			printf(_("bisect run success"));
 			res = BISECT_OK;
 		} else if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
 			printf(_("bisect found first bad commit"));
 			res = BISECT_OK;
 		} else if (res) {
 			error(_("bisect run failed: 'git bisect--helper --bisect-state"
-			" %s' exited with error code %d"), args.v[0], res);
+			" %s' exited with error code %d"), new_state, res);
 		} else {
 			continue;
 		}

 		strbuf_release(&command);
-		strvec_clear(&args);
 		strvec_clear(&run_args);
 		return res;
 	}
 }
--
2.34.1

Re: [PATCH] bisect: report actual bisect_state() argument on error

[Ramkumar Ramachandra]

René Scharfe wrote:
> This doesn't fix your problem, only replace the "(null)" in the error
> messages with the actual state name -- which may be useful for
> diagnosing its cause, though.

Thanks René! As a next step, perhaps we can also abbreviate the SHAs in the error messages?

Warm regards,
Ram

Re: git bisect bad @

[Junio C Hamano]

"Ramkumar Ramachandra" <r@artagnon.com> writes:

>   # on coq.git, for those curious
>   $ git bisect start
>   $ git bisect bad @
>   $ git bisect good V8.14.1
>   $ git bisect run bisect.sh # oops!
>   Lancement de  'bisect.sh'
>   'bisect.sh': bisect.sh: command not found
>   La base de fusion ea3595845f5013359b2ba4402f948e454350a74c est mauvaise.
> ...

"command not found"?

Re: git bisect bad @

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano wrote:
> "Ramkumar Ramachandra" <r@artagnon.com> writes:
> 
> >   # on coq.git, for those curious
> >   $ git bisect start
> >   $ git bisect bad @
> >   $ git bisect good V8.14.1
> >   $ git bisect run bisect.sh # oops!
> >   Lancement de  'bisect.sh'
> >   'bisect.sh': bisect.sh: command not found
> >   La base de fusion ea3595845f5013359b2ba4402f948e454350a74c est mauvaise.
> > ...
> 
> "command not found"?

Yeah, I suppose bisect invokes exec(), which then probably expects the executable to either be in $PATH, or expects me to specify the path of the executable, failing that; in other words, './bisect.sh'. In any case, this minor typo shouldn't penalize the user by having to abort the bisect, and restart it, specifying good and bad commits all over again. Then again, there are other ways to bump your head: what if I forgot to chmod +x the bisect.sh? What if there is no bisect.sh? Should I have to restart the bisect process from the beginning?

This presents another possible opportunity for enhancement: in an overwhelmingly large majority of the use cases (or so I assume), './' is really redundant.

Warm regards,
Ram

Re: git bisect bad @

[René Scharfe]

Am 10.01.22 um 22:04 schrieb Ramkumar Ramachandra:
> Hi Junio,
>
> Junio C Hamano wrote:
>> "Ramkumar Ramachandra" <r@artagnon.com> writes:
>>
>>>   # on coq.git, for those curious
>>>   $ git bisect start
>>>   $ git bisect bad @
>>>   $ git bisect good V8.14.1
>>>   $ git bisect run bisect.sh # oops!
>>>   Lancement de  'bisect.sh'
>>>   'bisect.sh': bisect.sh: command not found
>>>   La base de fusion ea3595845f5013359b2ba4402f948e454350a74c est mauvaise.
>>> ...
>>
>> "command not found"?
>
> Yeah, I suppose bisect invokes exec(), which then probably expects
> the executable to either be in $PATH, or expects me to specify the
> path of the executable, failing that; in other words, './bisect.sh'.
> In any case, this minor typo shouldn't penalize the user by having to
> abort the bisect, and restart it, specifying good and bad commits all
> over again.

Yes, bisect run invokes the given command using the shell, which tries
to find it in $PATH.

It would be nice if we could determine if the command was not found by
the shell and halt the bisection.  This is actually indicated by the
shell using error code 127.  However, the script itself could also exit
with that code (e.g. if one of its commands was not found).  Currently
this is interpreted as a bad revision and bisection continues, as
documented in the man page of git bisect.

If we'd make error code 127 (and 126) special by stopping the bisection
(like we do for 128 and higher) then scripts that relied on that code
indicating a bad revision would require a manual "git bisect bad" at
each affected step.  Annoying, but not dangerous.  Such a script would
have to be modified to convert codes 126 and 127 to e.g. 1.

Seems like a reasonable trade-off to me.  Thoughts?

> Then again, there are other ways to bump your head: what
> if I forgot to chmod +x the bisect.sh?

That's indicated by error code 126.

> What if there is no bisect.sh?

You have to provide one, of course, but ...

> Should I have to restart the bisect process from the beginning?

... interpreting the non-existence of the script as all revisions being
bad seems odd indeed.  Halting the bisection at that point makes more
sense to me.

> This presents another possible opportunity for enhancement: in an
> overwhelmingly large majority of the use cases (or so I assume), './'
> is really redundant.
Adding the current directory to $PATH would be inconsistent and might
even be dangerous.

Prepending "./" to a given command that contains no directory separator
is speculative -- what if that command is actually found in $PATH?

Halting the bisection would take the sting out of such a typo, because
it's reported immediately and you can fix it and continue.
Additionally we could check for the command in the current directory
and suggest something like "'bisect.sh' not found; did you mean
'./bisect.sh'?".

René

Re: git bisect bad @

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> It would be nice if we could determine if the command was not found by
> the shell and halt the bisection.  This is actually indicated by the
> shell using error code 127.  However, the script itself could also exit
> with that code (e.g. if one of its commands was not found).  Currently
> this is interpreted as a bad revision and bisection continues, as
> documented in the man page of git bisect.
>
> If we'd make error code 127 (and 126) special by stopping the bisection
> (like we do for 128 and higher) then scripts that relied on that code
> indicating a bad revision would require a manual "git bisect bad" at
> each affected step.  Annoying, but not dangerous.  Such a script would
> have to be modified to convert codes 126 and 127 to e.g. 1.
>
> Seems like a reasonable trade-off to me.  Thoughts?

Probably.  It is safer than the current "all revisions including the
bottom one and the top one are bad" which leads to the "merge-base
says your good and bad are nonsense" error for the "command not
found" case, but what if the one that reports an error with 127 (or
126) is coming from something other than shell (i.e. the 'bisect
run' command was fed is not a script at all)?  Is it a no-no for a
random binary that is not an implementation of shell to exit with
these error status?

>> This presents another possible opportunity for enhancement: in an
>> overwhelmingly large majority of the use cases (or so I assume), './'
>> is really redundant.
> Adding the current directory to $PATH would be inconsistent and might
> even be dangerous.
>
> Prepending "./" to a given command that contains no directory separator
> is speculative -- what if that command is actually found in $PATH?

Bad idea.

> Additionally we could check for the command in the current directory
> and suggest something like "'bisect.sh' not found; did you mean
> './bisect.sh'?".

It may not hurt but I do not think it is necessary at all, as long
as the "halt the 'bisect run' session upon 126 and 127" 

Re: git bisect bad @

[René Scharfe]

Am 12.01.22 um 18:50 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> It would be nice if we could determine if the command was not found by
>> the shell and halt the bisection.  This is actually indicated by the
>> shell using error code 127.  However, the script itself could also exit
>> with that code (e.g. if one of its commands was not found).  Currently
>> this is interpreted as a bad revision and bisection continues, as
>> documented in the man page of git bisect.
>>
>> If we'd make error code 127 (and 126) special by stopping the bisection
>> (like we do for 128 and higher) then scripts that relied on that code
>> indicating a bad revision would require a manual "git bisect bad" at
>> each affected step.  Annoying, but not dangerous.  Such a script would
>> have to be modified to convert codes 126 and 127 to e.g. 1.
>>
>> Seems like a reasonable trade-off to me.  Thoughts?
>
> Probably.  It is safer than the current "all revisions including the
> bottom one and the top one are bad" which leads to the "merge-base
> says your good and bad are nonsense" error for the "command not
> found" case, but what if the one that reports an error with 127 (or
> 126) is coming from something other than shell (i.e. the 'bisect
> run' command was fed is not a script at all)?  Is it a no-no for a
> random binary that is not an implementation of shell to exit with
> these error status?

The man page of exit(3) mentions the implementation-defined constants
EXIT_SUCCESS and EXIT_FAILURE from C99.  It also says: "Cooperating
processes may use other values".

sysexits(3) on BSD mentions a few others, all below 100
(https://man.openbsd.org/sysexits.3).  Its BUGS section says:
"The choice of an appropriate exit value is often ambiguous.".

So exit code values are only very vaguely standardized.  It's very
possible that there are programs that use 126 or 127 to signal
something other than "can't execute" or "cannot find command".  Under
the new rules the bisect run script would have to translate them to
some lower value.

René

Re: git bisect bad @

[René Scharfe]

Am 12.01.22 um 19:34 schrieb René Scharfe:
>
> Am 12.01.22 um 18:50 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> It would be nice if we could determine if the command was not found by
>>> the shell and halt the bisection.  This is actually indicated by the
>>> shell using error code 127.  However, the script itself could also exit
>>> with that code (e.g. if one of its commands was not found).  Currently
>>> this is interpreted as a bad revision and bisection continues, as
>>> documented in the man page of git bisect.
>>>
>>> If we'd make error code 127 (and 126) special by stopping the bisection
>>> (like we do for 128 and higher) then scripts that relied on that code
>>> indicating a bad revision would require a manual "git bisect bad" at
>>> each affected step.  Annoying, but not dangerous.  Such a script would
>>> have to be modified to convert codes 126 and 127 to e.g. 1.
>>>
>>> Seems like a reasonable trade-off to me.  Thoughts?
>>
>> Probably.  It is safer than the current "all revisions including the
>> bottom one and the top one are bad" which leads to the "merge-base
>> says your good and bad are nonsense" error for the "command not
>> found" case, but what if the one that reports an error with 127 (or
>> 126) is coming from something other than shell (i.e. the 'bisect
>> run' command was fed is not a script at all)?  Is it a no-no for a
>> random binary that is not an implementation of shell to exit with
>> these error status?
>
> The man page of exit(3) mentions the implementation-defined constants
> EXIT_SUCCESS and EXIT_FAILURE from C99.  It also says: "Cooperating
> processes may use other values".
>
> sysexits(3) on BSD mentions a few others, all below 100
> (https://man.openbsd.org/sysexits.3).  Its BUGS section says:
> "The choice of an appropriate exit value is often ambiguous.".
>
> So exit code values are only very vaguely standardized.  It's very
> possible that there are programs that use 126 or 127 to signal
> something other than "can't execute" or "cannot find command".  Under
> the new rules the bisect run script would have to translate them to
> some lower value.

Reserving 126 and 127 shouldn't cause too much trouble, but there's
also a way to avoid it: bisect run could checkout a known-good
revision first and abort if the script returns non-zero for any
reason, including its non-existence.

René

Re: git bisect bad @

[Ramkumar Ramachandra]

René Scharfe wrote:
> > Am 12.01.22 um 18:50 schrieb Junio C Hamano:
> > So exit code values are only very vaguely standardized.  It's very
> > possible that there are programs that use 126 or 127 to signal
> > something other than "can't execute" or "cannot find command".  Under
> > the new rules the bisect run script would have to translate them to
> > some lower value.
> 
> Reserving 126 and 127 shouldn't cause too much trouble, but there's
> also a way to avoid it: bisect run could checkout a known-good
> revision first and abort if the script returns non-zero for any
> reason, including its non-existence.

I can't say I'm overly enthusiastic about this trade-off. I think most people would check their bisect scripts against the good revision by hand before starting bisect: why introduce one redundant step for users like me who tend to bump their heads, because they're a bit rusty with machines?

Again, I don't know if this is a good idea, but if exit codes from the shell aren't standardized, surely fork() and exec() would have a better spec? So, perhaps remove the little git-bisect.sh and rewrite it in C? I'd be up for this task, if we decide that this is a better way to go.

Warm regards,
Ram

Re: git bisect bad @

[Christian Couder]

On Thu, Jan 13, 2022 at 10:32 AM Ramkumar Ramachandra <r@artagnon.com> wrote:
>
> René Scharfe wrote:

> > Reserving 126 and 127 shouldn't cause too much trouble,

I don't think it's a good idea at this point to reserve the 126 and
127 error codes as there might be existing scripts relying on them to
mean "bad".

Perhaps we could introduce a new command line option, for example
--bad-is-only-1, to specify that the only error code considered bad
will be 1. Or perhaps a more general --bad-is=<list of ranges>, to be
able to specify all the values and ranges that should be considered
bad.

> > but there's
> > also a way to avoid it: bisect run could checkout a known-good
> > revision first and abort if the script returns non-zero for any
> > reason, including its non-existence.
>
> I can't say I'm overly enthusiastic about this trade-off. I think
> most people would check their bisect scripts against the good
> revision by hand before starting bisect: why introduce one
> redundant step for users like me who tend to bump their heads,
> because they're a bit rusty with machines?

I also don't like introducing a redundant step, unless a special
command line option is introduced for it.

> Again, I don't know if this is a good idea, but if exit codes from
> the shell aren't standardized, surely fork() and exec() would have
> a better spec? So, perhaps remove the little git-bisect.sh and
> rewrite it in C? I'd be up for this task, if we decide that this is a
> better way to go.

There has been a lot of effort, especially by Miriam (added in Cc), to
port git-bisect.sh to C over the years.

Re: git bisect bad @

[René Scharfe]

Am 13.01.22 um 13:28 schrieb Christian Couder:
> On Thu, Jan 13, 2022 at 10:32 AM Ramkumar Ramachandra
> <r@artagnon.com> wrote:
>>
>> René Scharfe wrote:
>
>>> Reserving 126 and 127 shouldn't cause too much trouble,
>
> I don't think it's a good idea at this point to reserve the 126 and
> 127 error codes as there might be existing scripts relying on them
> to mean "bad".

Certainly possible -- people get the weirdest ideas.

> Perhaps we could introduce a new command line option, for example
> --bad-is-only-1, to specify that the only error code considered bad
> will be 1. Or perhaps a more general --bad-is=<list of ranges>, to
> be able to specify all the values and ranges that should be
> considered bad.

This would only help someone who mistyped the script name or forgot to
make it executable if they also used that option.  I can't imagine
someone planning their mistakes ahead like that.  And always using this
option would be annoying.

>>> but there's also a way to avoid it: bisect run could checkout a
>>> known-good revision first and abort if the script returns
>>> non-zero for any reason, including its non-existence.
>>
>> I can't say I'm overly enthusiastic about this trade-off. I think
>> most people would check their bisect scripts against the good
>> revision by hand before starting bisect: why introduce one
>> redundant step for users like me who tend to bump their heads,
>> because they're a bit rusty with machines?

It would slow the normal operation a bit, but reduce the time to error
and its impact significantly.

> I also don't like introducing a redundant step, unless a special
> command line option is introduced for it.

My comment regarding --is-bad applies here as well.

OK, here's another idea: We verify using a known-good commit only if
the return code of the first run of the bisect script is 126 or 127.
If we get the same value again then we report the script as broken and
leave the bisect state unchanged.  Otherwise we know it's an old school
script, register the first revision as bad and continue without further
verification steps.

>> Again, I don't know if this is a good idea, but if exit codes from
>> the shell aren't standardized, surely fork() and exec() would have
>> a better spec? So, perhaps remove the little git-bisect.sh and
>> rewrite it in C? I'd be up for this task, if we decide that this is
>> a better way to go.
>
> There has been a lot of effort, especially by Miriam (added in Cc),
> to port git-bisect.sh to C over the years.

The implementation language of git bisect is not immediately relevant
here, but that the shell is used to call the user-supplied bisect run
script is.  If we'd run it directly (without RUN_USING_SHELL) we could
distinguish error code 126/127 from execution errors.  I assume the
option is used to stay compatible with the old shell version of bisect.

René

Re: git bisect bad @

[Ramkumar Ramachandra]

René Scharfe wrote:
> Am 13.01.22 um 13:28 schrieb Christian Couder:
> > I don't think it's a good idea at this point to reserve the 126 and
> > 127 error codes as there might be existing scripts relying on them
> > to mean "bad".
> 
> Certainly possible -- people get the weirdest ideas.

My gut reaction is that this is an overly conservative point of view. There are two factors to consider: first, bisect scripts are usually short one-time throwaway scripts tailored to one project for one problem. Second, how likely is it that these people that have a complex script with 126/127 as the exit code, which they have been using for years to run bisect on the same project for the same problem, update their version of git frequently?

Again, I might be wrong, because I don't know how people use bisect. Worst case, we can display a warning about this backward incompatibility in the next few versions.

> The implementation language of git bisect is not immediately relevant
> here, but that the shell is used to call the user-supplied bisect run
> script is.  If we'd run it directly (without RUN_USING_SHELL) we could
> distinguish error code 126/127 from execution errors.  I assume the
> option is used to stay compatible with the old shell version of bisect.

Sorry, my misunderstanding. I thought the external command was being called from git-bisect.sh. I don't think I understand the purpose of RUN_USING_SHELL (it just seems to put an "sh -c" in the beginning):

	static const char **prepare_shell_cmd(struct strvec *out, const char **argv)
	{
        ...
			strvec_push(out, "sh");
			strvec_push(out, "-c");

			if (!argv[1])
				strvec_push(out, argv[0]);
			else
				strvec_pushf(out, "%s \"$@\"", argv[0]);
       ...
	}

Warm regards,
Ram

Re: git bisect bad @

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> The implementation language of git bisect is not immediately relevant
> here, but that the shell is used to call the user-supplied bisect run
> script is.  If we'd run it directly (without RUN_USING_SHELL) we could
> distinguish error code 126/127 from execution errors.

Yes, but it means that we'd need to reimplement command line
splitting, environment and variable substitutions, etc. in a
way that people expect from executing their run "script" with
a shell.

I'd rather not to see us go there.

Thanks.

Re: git bisect bad @

[René Scharfe]

Am 13.01.22 um 16:16 schrieb Ramkumar Ramachandra:
> René Scharfe wrote:
>> The implementation language of git bisect is not immediately relevant
>> here, but that the shell is used to call the user-supplied bisect run
>> script is.  If we'd run it directly (without RUN_USING_SHELL) we could
>> distinguish error code 126/127 from execution errors.  I assume the
>> option is used to stay compatible with the old shell version of bisect.
>
> Sorry, my misunderstanding. I thought the external command was being
> called from git-bisect.sh. I don't think I understand the purpose of
> RUN_USING_SHELL (it just seems to put an "sh -c" in the beginning):
>
> 	static const char **prepare_shell_cmd(struct strvec *out, const char **argv)
> 	{
>         ...
> 			strvec_push(out, "sh");
> 			strvec_push(out, "-c");
>
> 			if (!argv[1])
> 				strvec_push(out, argv[0]);
> 			else
> 				strvec_pushf(out, "%s \"$@\"", argv[0]);
>        ...
> 	}

Using the shell allows the bisect run command to be any shell command,
not just some script.  E.g. you could bisect a build failure with just
"git bisect run make".  Quite useful.

René

Re: git bisect bad @

[Ramkumar Ramachandra]

René Scharfe wrote:
> Using the shell allows the bisect run command to be any shell command,
> not just some script.  E.g. you could bisect a build failure with just
> "git bisect run make".  Quite useful.

Ah, that's quite useful, yes. The problem of improving user experience with bisect is getting more and more hairy. May I suggest something tractable, albeit not too elegant, and certainly not perfect, in view of improving user experience in common use cases?

1. If argv[0] of the supplied command is found in $PATH, check it for executable permissions. Otherwise, error out. It's highly unlikely that the user meant a shell builtin, which would supersede the executable in $PATH.
2. If argv[0] is found in the current directory, prompt for "Did you mean ... [Y/n]?"
3. If checking on merge-base fails, improve the error message with "Perhaps your bisect script is broken?" and reset bisect automatically.

Warm regards,
Ram

Re: git bisect bad @

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 13.01.22 um 16:16 schrieb Ramkumar Ramachandra:
>> René Scharfe wrote:
>>> The implementation language of git bisect is not immediately relevant
>>> here, but that the shell is used to call the user-supplied bisect run
>>> script is.  If we'd run it directly (without RUN_USING_SHELL) we could
>>> distinguish error code 126/127 from execution errors.  I assume the
>>> option is used to stay compatible with the old shell version of bisect.
>>
>> Sorry, my misunderstanding. I thought the external command was being
>> called from git-bisect.sh. I don't think I understand the purpose of
>> RUN_USING_SHELL (it just seems to put an "sh -c" in the beginning):
>>
>> 	static const char **prepare_shell_cmd(struct strvec *out, const char **argv)
>> 	{
>>         ...
>> 			strvec_push(out, "sh");
>> 			strvec_push(out, "-c");
>>
>> 			if (!argv[1])
>> 				strvec_push(out, argv[0]);
>> 			else
>> 				strvec_pushf(out, "%s \"$@\"", argv[0]);
>>        ...
>> 	}
>
> Using the shell allows the bisect run command to be any shell command,
> not just some script.  E.g. you could bisect a build failure with just
> "git bisect run make".  Quite useful.

True, but for example

	$ git bisect run make test

internally gets argv[] = { "make", "test", NULL } in bisect_run()
and then we are the one who make them into a single string, i.e.

	if (argc)
		sq_quote_argv(&command, argv);
	else {
		error(_("bisect run failed: no command provided."));
		return BISECT_FAILED;
	}

and that is what we run via the shell in the loop, i.e.

	while (1) {
		strvec_clear(&args);

		printf(_("running %s\n"), command.buf);
		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);

I do not offhand recall the reason why we need to do that, instead
of using the original argv[] to invoke run_command_v_opt().

And my earlier "let's not go there" may need to be rethought.  I
somehow thought we are getting a single string from the end-user
and will become responsible for splitting it out or substituting
an environment variable with its value, but I was mistaken.

Re: git bisect bad @

[René Scharfe]

Am 14.01.22 um 09:04 schrieb Ramkumar Ramachandra:
> René Scharfe wrote:
>> Using the shell allows the bisect run command to be any shell command,
>> not just some script.  E.g. you could bisect a build failure with just
>> "git bisect run make".  Quite useful.
>
> Ah, that's quite useful, yes. The problem of improving user
> experience with bisect is getting more and more hairy. May I suggest
> something tractable, albeit not too elegant, and certainly not
> perfect, in view of improving user experience in common use cases?

I like the pragmatism.

> 1. If argv[0] of the supplied command is found in $PATH, check it for
> executable permissions. Otherwise, error out. It's highly unlikely
> that the user meant a shell builtin, which would supersede the
> executable in $PATH.

Installing something in $PATH and forgetting to make it executable
sounds like a very rare case.  And if we find such a thing, would it
warrant erroring out?  What if there is an executable version somewhere
else in the $PATH or a shell builtin or an alias?

> 2. If argv[0] is found in the current directory, prompt for "Did you
> mean ... [Y/n]?"

That is more likely, I imagine, but is the existence of such a file
strong enough a signal to interrupt the program?  I think we are
better off making sure there actually is a problem first.

> 3. If checking on merge-base fails, improve the error message with
> "Perhaps your bisect script is broken?" and reset bisect
> automatically.

That would have no effect if the bad commit is a straight descendant of
the good one.  And a merge base being bad can have other causes than a
missing script.

René

[PATCH v2 1/4] bisect--helper: report actual bisect_state() argument on error

[René Scharfe]

The strvec "args" in bisect_run() is initialized and cleared, but never
added to.  Nevertheless its first member is printed when reporting a
bisect_state() error.  That's not useful, since it's always NULL.

Before d1bbbe45df (bisect--helper: reimplement `bisect_run` shell
function in C, 2021-09-13) the intended new state was reported if it
could not be set.  Reinstate that behavior and remove the unused strvec.

Reported-by: Ramkumar Ramachandra <r@artagnon.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Title and commit message slightly improved, same code change as in
https://lore.kernel.org/git/5d8c8a72-6c4f-35e4-a6a4-4ed7d6f23c4e@web.de/

 builtin/bisect--helper.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 28a2e6a575..1dbc6294ef 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -1093,7 +1093,6 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 {
 	int res = BISECT_OK;
 	struct strbuf command = STRBUF_INIT;
-	struct strvec args = STRVEC_INIT;
 	struct strvec run_args = STRVEC_INIT;
 	const char *new_state;
 	int temporary_stdout_fd, saved_stdout;
@@ -1111,8 +1110,6 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 	strvec_push(&run_args, command.buf);

 	while (1) {
-		strvec_clear(&args);
-
 		printf(_("running %s\n"), command.buf);
 		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);

@@ -1158,13 +1155,12 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 			res = BISECT_OK;
 		} else if (res) {
 			error(_("bisect run failed: 'git bisect--helper --bisect-state"
-			" %s' exited with error code %d"), args.v[0], res);
+			" %s' exited with error code %d"), new_state, res);
 		} else {
 			continue;
 		}

 		strbuf_release(&command);
-		strvec_clear(&args);
 		strvec_clear(&run_args);
 		return res;
 	}
--
2.34.1

[PATCH v2 2/4] bisect--helper: release strbuf and strvec on run error

[René Scharfe]

Move the cleanup code out of the loop and make sure all execution paths
pass through it to avoid leaking memory.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/bisect--helper.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1dbc6294ef..e529665d9f 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -1116,8 +1116,7 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 		if (res < 0 || 128 <= res) {
 			error(_("bisect run failed: exit code %d from"
 				" '%s' is < 0 or >= 128"), res, command.buf);
-			strbuf_release(&command);
-			return res;
+			break;
 		}

 		if (res == 125)
@@ -1129,8 +1128,10 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)

 		temporary_stdout_fd = open(git_path_bisect_run(), O_CREAT | O_WRONLY | O_TRUNC, 0666);

-		if (temporary_stdout_fd < 0)
-			return error_errno(_("cannot open file '%s' for writing"), git_path_bisect_run());
+		if (temporary_stdout_fd < 0) {
+			res = error_errno(_("cannot open file '%s' for writing"), git_path_bisect_run());
+			break;
+		}

 		fflush(stdout);
 		saved_stdout = dup(1);
@@ -1159,11 +1160,12 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 		} else {
 			continue;
 		}
-
-		strbuf_release(&command);
-		strvec_clear(&run_args);
-		return res;
+		break;
 	}
+
+	strbuf_release(&command);
+	strvec_clear(&run_args);
+	return res;
 }

 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
--
2.34.1

[PATCH v2 3/4] bisect: document run behavior with exit codes 126 and 127

[René Scharfe]

Shells report non-executable and missing commands with exit codes 126
and 127, respectively.  For historical reasons "git bisect run"
interprets them as indicating a bad commit, though.  Document the
current behavior by adding basic tests that cover these cases.

Reported-by: Ramkumar Ramachandra <r@artagnon.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/t6030-bisect-porcelain.sh | 45 +++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 1be85d064e..fc18796517 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -278,6 +278,51 @@ test_expect_success '"git bisect run" with more complex "git bisect start"' '
 	git bisect reset
 '

+test_expect_success 'bisect run accepts exit code 126 as bad' '
+	test_when_finished "git bisect reset" &&
+	write_script test_script.sh <<-\EOF &&
+	! grep Another hello || exit 126 >/dev/null
+	EOF
+	git bisect start &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH4 &&
+	git bisect run ./test_script.sh >my_bisect_log.txt &&
+	grep "$HASH3 is the first bad commit" my_bisect_log.txt
+'
+
+test_expect_failure POSIXPERM 'bisect run fails with non-executable test script' '
+	test_when_finished "git bisect reset" &&
+	>not-executable.sh &&
+	chmod -x not-executable.sh &&
+	git bisect start &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH4 &&
+	test_must_fail git bisect run ./not-executable.sh >my_bisect_log.txt &&
+	! grep "is the first bad commit" my_bisect_log.txt
+'
+
+test_expect_success 'bisect run accepts exit code 127 as bad' '
+	test_when_finished "git bisect reset" &&
+	write_script test_script.sh <<-\EOF &&
+	! grep Another hello || exit 127 >/dev/null
+	EOF
+	git bisect start &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH4 &&
+	git bisect run ./test_script.sh >my_bisect_log.txt &&
+	grep "$HASH3 is the first bad commit" my_bisect_log.txt
+'
+
+test_expect_failure 'bisect run fails with missing test script' '
+	test_when_finished "git bisect reset" &&
+	rm -f does-not-exist.sh &&
+	git bisect start &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH4 &&
+	test_must_fail git bisect run ./does-not-exist.sh >my_bisect_log.txt &&
+	! grep "is the first bad commit" my_bisect_log.txt
+'
+
 # $HASH1 is good, $HASH5 is bad, we skip $HASH3
 # but $HASH4 is good,
 # so we should find $HASH5 as the first bad commit
--
2.34.1

[PATCH v2 4/4] bisect--helper: double-check run command on exit code 126 and 127

[René Scharfe]

When a run command cannot be executed or found, shells return exit code
126 or 127, respectively.  Valid run commands are allowed to return
these codes as well to indicate bad revisions, though, for historical
reasons.  This means typos can cause bogus bisect runs that go over the
full distance and end up reporting invalid results.

The best solution would be to reserve exit codes 126 and 127, like
71b0251cdd (Bisect run: "skip" current commit if script exit code is
125., 2007-10-26) did for 125, and abort bisect run when we get them.
That might be inconvenient for those who relied on the documentation
stating that 126 and 127 can be used for bad revisions, though.

The workaround used by this patch is to run the command on a known-good
revision and abort if we still get the same error code.  This adds one
step to runs with scripts that use exit codes 126 and 127, but keeps
them supported, with one exception: It won't work with commands that
cannot recognize the (manually marked) known-good revision as such.

Run commands that use low exit codes are unaffected.  Typos are reported
after executing the missing command twice and three checkouts (the first
step, the known good revision and back to the revision of the first
step).

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 bisect.c                    |  3 +-
 bisect.h                    |  3 ++
 builtin/bisect--helper.c    | 63 +++++++++++++++++++++++++++++++++++++
 t/t6030-bisect-porcelain.sh |  4 +--
 4 files changed, 70 insertions(+), 3 deletions(-)

diff --git a/bisect.c b/bisect.c
index 888949fba6..9e6a2b7f20 100644
--- a/bisect.c
+++ b/bisect.c
@@ -724,7 +724,8 @@ static int is_expected_rev(const struct object_id *oid)
 	return res;
 }

-static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int no_checkout)
+enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
+				  int no_checkout)
 {
 	char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
 	struct commit *commit;
diff --git a/bisect.h b/bisect.h
index ec24ac2d7e..748adf0cff 100644
--- a/bisect.h
+++ b/bisect.h
@@ -69,4 +69,7 @@ void read_bisect_terms(const char **bad, const char **good);

 int bisect_clean_state(void);

+enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
+				  int no_checkout);
+
 #endif
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index e529665d9f..50783a586c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -1089,6 +1089,44 @@ static int bisect_visualize(struct bisect_terms *terms, const char **argv, int a
 	return res;
 }

+static int get_first_good(const char *refname, const struct object_id *oid,
+			  int flag, void *cb_data)
+{
+	oidcpy(cb_data, oid);
+	return 1;
+}
+
+static int verify_good(const struct bisect_terms *terms,
+		       const char **quoted_argv)
+{
+	int rc;
+	enum bisect_error res;
+	struct object_id good_rev;
+	struct object_id current_rev;
+	char *good_glob = xstrfmt("%s-*", terms->term_good);
+	int no_checkout = ref_exists("BISECT_HEAD");
+
+	for_each_glob_ref_in(get_first_good, good_glob, "refs/bisect/",
+			     &good_rev);
+	free(good_glob);
+
+	if (read_ref(no_checkout ? "BISECT_HEAD" : "HEAD", &current_rev))
+		return -1;
+
+	res = bisect_checkout(&good_rev, no_checkout);
+	if (res != BISECT_OK)
+		return -1;
+
+	printf(_("running %s\n"), quoted_argv[0]);
+	rc = run_command_v_opt(quoted_argv, RUN_USING_SHELL);
+
+	res = bisect_checkout(&current_rev, no_checkout);
+	if (res != BISECT_OK)
+		return -1;
+
+	return rc;
+}
+
 static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 {
 	int res = BISECT_OK;
@@ -1096,6 +1134,7 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 	struct strvec run_args = STRVEC_INIT;
 	const char *new_state;
 	int temporary_stdout_fd, saved_stdout;
+	int is_first_run = 1;

 	if (bisect_next_check(terms, NULL))
 		return BISECT_FAILED;
@@ -1113,6 +1152,30 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 		printf(_("running %s\n"), command.buf);
 		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);

+		/*
+		 * Exit code 126 and 127 can either come from the shell
+		 * if it was unable to execute or even find the script,
+		 * or from the script itself.  Check with a known-good
+		 * revision to avoid trashing the bisect run due to a
+		 * missing or non-executable script.
+		 */
+		if (is_first_run && (res == 126 || res == 127)) {
+			int rc = verify_good(terms, run_args.v);
+			is_first_run = 0;
+			if (rc < 0) {
+				error(_("unable to verify '%s' on good"
+					" revision"), command.buf);
+				res = BISECT_FAILED;
+				break;
+			}
+			if (rc == res) {
+				error(_("bogus exit code %d for good revision"),
+				      rc);
+				res = BISECT_FAILED;
+				break;
+			}
+		}
+
 		if (res < 0 || 128 <= res) {
 			error(_("bisect run failed: exit code %d from"
 				" '%s' is < 0 or >= 128"), res, command.buf);
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index fc18796517..5382e5d216 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -290,7 +290,7 @@ test_expect_success 'bisect run accepts exit code 126 as bad' '
 	grep "$HASH3 is the first bad commit" my_bisect_log.txt
 '

-test_expect_failure POSIXPERM 'bisect run fails with non-executable test script' '
+test_expect_success POSIXPERM 'bisect run fails with non-executable test script' '
 	test_when_finished "git bisect reset" &&
 	>not-executable.sh &&
 	chmod -x not-executable.sh &&
@@ -313,7 +313,7 @@ test_expect_success 'bisect run accepts exit code 127 as bad' '
 	grep "$HASH3 is the first bad commit" my_bisect_log.txt
 '

-test_expect_failure 'bisect run fails with missing test script' '
+test_expect_success 'bisect run fails with missing test script' '
 	test_when_finished "git bisect reset" &&
 	rm -f does-not-exist.sh &&
 	git bisect start &&
--
2.34.1

Re: [PATCH v2 4/4] bisect--helper: double-check run command on exit code 126 and 127

[Junio C Hamano]

> diff --git a/bisect.h b/bisect.h
> index ec24ac2d7e..748adf0cff 100644
> --- a/bisect.h
> +++ b/bisect.h
> @@ -69,4 +69,7 @@ void read_bisect_terms(const char **bad, const char **good);
>
>  int bisect_clean_state(void);
>
> +enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
> +                                 int no_checkout);
> +
>  #endif

https://github.com/git/git/runs/4861805265?check_suite_focus=true#step:4:65

In file included from bisect.hcc:2:0:
bisect.h:72:48: error: ‘struct object_id’ declared inside parameter
list will not be visible outside of this definition or declaration
[-Werror]
 enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
                                                ^~~~~~~~~
cc1: all warnings being treated as errors

Re: [PATCH v2 4/4] bisect--helper: double-check run command on exit code 126 and 127

[René Scharfe]

Am 19.01.22 um 03:36 schrieb Junio C Hamano:
>> diff --git a/bisect.h b/bisect.h
>> index ec24ac2d7e..748adf0cff 100644
>> --- a/bisect.h
>> +++ b/bisect.h
>> @@ -69,4 +69,7 @@ void read_bisect_terms(const char **bad, const char **good);
>>
>>  int bisect_clean_state(void);
>>
>> +enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
>> +                                 int no_checkout);
>> +
>>  #endif
>
> https://github.com/git/git/runs/4861805265?check_suite_focus=true#step:4:65
>
> In file included from bisect.hcc:2:0:
> bisect.h:72:48: error: ‘struct object_id’ declared inside parameter
> list will not be visible outside of this definition or declaration
> [-Werror]
>  enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
>                                                 ^~~~~~~~~
> cc1: all warnings being treated as errors

Oops, and I didn't even know the make target hdr-check exists. :-/

--- >8 ---
Subject: [PATCH] fixup! bisect--helper: double-check run command on exit code 126 and 127

---
 bisect.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/bisect.h b/bisect.h
index 748adf0cff..1015aeb8ea 100644
--- a/bisect.h
+++ b/bisect.h
@@ -3,6 +3,7 @@

 struct commit_list;
 struct repository;
+struct object_id;

 /*
  * Find bisection. If something is found, `reaches` will be the number of
--
2.34.1

Re: [PATCH v2 4/4] bisect--helper: double-check run command on exit code 126 and 127

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> When a run command cannot be executed or found, shells return exit code
> 126 or 127, respectively.  Valid run commands are allowed to return
> these codes as well to indicate bad revisions, though, for historical
> reasons.  This means typos can cause bogus bisect runs that go over the
> full distance and end up reporting invalid results.
>
> The best solution would be to reserve exit codes 126 and 127, like
> 71b0251cdd (Bisect run: "skip" current commit if script exit code is
> 125., 2007-10-26) did for 125, and abort bisect run when we get them.
> That might be inconvenient for those who relied on the documentation
> stating that 126 and 127 can be used for bad revisions, though.

I think the basic idea is sound and useful.  How happy are we who
was involved in the discussion with this result?

> +static int get_first_good(const char *refname, const struct object_id *oid,
> +			  int flag, void *cb_data)
> +{
> +	oidcpy(cb_data, oid);
> +	return 1;
> +}

OK, this iterates and stops at the first one.

> +static int verify_good(const struct bisect_terms *terms,
> +		       const char **quoted_argv)
> +{
> +	int rc;
> +	enum bisect_error res;
> +	struct object_id good_rev;
> +	struct object_id current_rev;
> +	char *good_glob = xstrfmt("%s-*", terms->term_good);
> +	int no_checkout = ref_exists("BISECT_HEAD");
> +
> +	for_each_glob_ref_in(get_first_good, good_glob, "refs/bisect/",
> +			     &good_rev);
> +	free(good_glob);
> +
> +	if (read_ref(no_checkout ? "BISECT_HEAD" : "HEAD", &current_rev))
> +		return -1;

 * Could the current_rev already be marked as "good", in which case
   we can avoid cost of rewriting working tree files to a
   potentially distant revision?  I often do manual tests to mark
   "bisect good" or "bisect bad" before using "bisect run".

 * Can we have *no* rev that is marked as "good"?  I think we made
   it possible to say "my time is more valuable than machine cycles,
   so I'll only tell you that this revision is broken and give you
   no limit on the bottom side of the history.  still assume that
   there was only one good-to-bad transition in the history and find
   it" by supplying only one "bad" and no "good" when starting to
   bisect.  And in such a case, ...

> +	res = bisect_checkout(&good_rev, no_checkout);

... this would feed an uninitialized object_id to bisect_checkout.

Thanks.

Re: [PATCH v2 4/4] bisect--helper: double-check run command on exit code 126 and 127

[René Scharfe]

Am 04.02.22 um 01:42 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> When a run command cannot be executed or found, shells return exit code
>> 126 or 127, respectively.  Valid run commands are allowed to return
>> these codes as well to indicate bad revisions, though, for historical
>> reasons.  This means typos can cause bogus bisect runs that go over the
>> full distance and end up reporting invalid results.
>>
>> The best solution would be to reserve exit codes 126 and 127, like
>> 71b0251cdd (Bisect run: "skip" current commit if script exit code is
>> 125., 2007-10-26) did for 125, and abort bisect run when we get them.
>> That might be inconvenient for those who relied on the documentation
>> stating that 126 and 127 can be used for bad revisions, though.
>
> I think the basic idea is sound and useful.  How happy are we who
> was involved in the discussion with this result?
>
>> +static int get_first_good(const char *refname, const struct object_id *oid,
>> +			  int flag, void *cb_data)
>> +{
>> +	oidcpy(cb_data, oid);
>> +	return 1;
>> +}
>
> OK, this iterates and stops at the first one.
>
>> +static int verify_good(const struct bisect_terms *terms,
>> +		       const char **quoted_argv)
>> +{
>> +	int rc;
>> +	enum bisect_error res;
>> +	struct object_id good_rev;
>> +	struct object_id current_rev;
>> +	char *good_glob = xstrfmt("%s-*", terms->term_good);
>> +	int no_checkout = ref_exists("BISECT_HEAD");
>> +
>> +	for_each_glob_ref_in(get_first_good, good_glob, "refs/bisect/",
>> +			     &good_rev);
>> +	free(good_glob);
>> +
>> +	if (read_ref(no_checkout ? "BISECT_HEAD" : "HEAD", &current_rev))
>> +		return -1;
>
>  * Could the current_rev already be marked as "good", in which case
>    we can avoid cost of rewriting working tree files to a
>    potentially distant revision?  I often do manual tests to mark
>    "bisect good" or "bisect bad" before using "bisect run".
>
>  * Can we have *no* rev that is marked as "good"?  I think we made
>    it possible to say "my time is more valuable than machine cycles,
>    so I'll only tell you that this revision is broken and give you
>    no limit on the bottom side of the history.  still assume that
>    there was only one good-to-bad transition in the history and find
>    it" by supplying only one "bad" and no "good" when starting to
>    bisect.  And in such a case, ...
>
>> +	res = bisect_checkout(&good_rev, no_checkout);
>
> ... this would feed an uninitialized object_id to bisect_checkout.

bisect_run() starts by calling bisect_next_check() with a current_term
parameter value of NULL.  It checks if the good rev is missing and calls
decide_next(), which returns -1 if current_term is NULL unless both good
and bad revs are present.  bisect_next_check() passes this value along.
bisect_run() exits if it's non-zero.

So AFAICS the uninitialized access would only happen if the good rev ref
was deleted between the bisect_next_check() call and the verify_good()
call.  I considered this scenario to be practically impossible with the
current code.  We can handle it more gracefully by doing something like
in the patch below.

Supporting a bad-only git bisect run would take more work -- perhaps by
making verify_good() pick a root commit to check as an assumed good rev
(plus fix whatever else caused the current code to pass NULL as
current_term).

René


---
 builtin/bisect--helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 50783a586c..e1e58de3b2 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -1106,9 +1106,12 @@ static int verify_good(const struct bisect_terms *terms,
 	char *good_glob = xstrfmt("%s-*", terms->term_good);
 	int no_checkout = ref_exists("BISECT_HEAD");

+	oidcpy(&good_rev, null_oid());
 	for_each_glob_ref_in(get_first_good, good_glob, "refs/bisect/",
 			     &good_rev);
 	free(good_glob);
+	if (is_null_oid(&good_rev))
+		return -1;

 	if (read_ref(no_checkout ? "BISECT_HEAD" : "HEAD", &current_rev))
 		return -1;
--
2.35.0

Re: [PATCH v2 4/4] bisect--helper: double-check run command on exit code 126 and 127

[Ramkumar Ramachandra]

René Scharfe wrote:
> The workaround used by this patch is to run the command on a known-good
> revision and abort if we still get the same error code.  This adds one
> step to runs with scripts that use exit codes 126 and 127, but keeps
> them supported, with one exception: It won't work with commands that
> cannot recognize the (manually marked) known-good revision as such.
>
> Run commands that use low exit codes are unaffected.  Typos are reported
> after executing the missing command twice and three checkouts (the first
> step, the known good revision and back to the revision of the first
> step).

I'm happy with the description of this patch. It doesn't add an extra step for an overwhelming majority of users, while not breaking backward compatibility.
 
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -1089,6 +1089,44 @@ static int bisect_visualize(struct bisect_terms *terms, const char **argv, int a
> +static int get_first_good(const char *refname, const struct object_id *oid,
> +   int flag, void *cb_data)
> +{
> + oidcpy(cb_data, oid);
> + return 1;

I assume you return 1 here to stop the for_each_glob_ref() iteration, after copying the oid.

> @@ -1113,6 +1152,30 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
> + int rc = verify_good(terms, run_args.v);
> + is_first_run = 0;
> + if (rc < 0) {
> + error(_("unable to verify '%s' on good"
> + " revision"), command.buf);

Perhaps in a subsequent patch, we can consider sha1_abbrev() to print nicer messages everywhere in bisect.

> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index fc18796517..5382e5d216 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -290,7 +290,7 @@ test_expect_success 'bisect run accepts exit code 126 as bad' '
> -test_expect_failure POSIXPERM 'bisect run fails with non-executable test script' '
> +test_expect_success POSIXPERM 'bisect run fails with non-executable test script' '

> -test_expect_failure 'bisect run fails with missing test script' '
> +test_expect_success 'bisect run fails with missing test script' '

Yes, these are precisely the two problems I had.

Thanks.

Warm regards,
Ram

Re: [PATCH v2 4/4] bisect--helper: double-check run command on exit code 126 and 127

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> * Can we have *no* rev that is marked as "good"?  I think we made
>    it possible to say "my time is more valuable than machine cycles,
>    so I'll only tell you that this revision is broken and give you
>    no limit on the bottom side of the history.  still assume that
>    there was only one good-to-bad transition in the history and find
>    it" by supplying only one "bad" and no "good" when starting to
>    bisect.  And in such a case, ...

Hm, this addition might be an unpleasant special-case syntax, breaking both `git bisect start [bad [good]]` and `git bisect bad ...; git bisect start`.

R.

Re: [PATCH v2 4/4] bisect--helper: double-check run command on exit code 126 and 127

[Junio C Hamano]

"Ramkumar Ramachandra" <r@artagnon.com> writes:

> Junio C Hamano wrote:
>> * Can we have *no* rev that is marked as "good"?  I think we made
>>    it possible to say "my time is more valuable than machine cycles,
>>    so I'll only tell you that this revision is broken and give you
>>    no limit on the bottom side of the history.  still assume that
>>    there was only one good-to-bad transition in the history and find
>>    it" by supplying only one "bad" and no "good" when starting to
>>    bisect.  And in such a case, ...
>
> Hm, this addition might be an unpleasant special-case syntax, breaking both `git bisect start [bad [good]]` and `git bisect bad ...; git bisect start`.

Interesting.  Our "start" does allow you to give one "bad" and then
zero "good" commits.  And it will sit and wait until you give at
least one "good".  So we'd need an "--assume-roots-are-good" option
or something to force bisect the whole history below the "bad" one.