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

["Ramkumar Ramachandra" <r@artagnon.com>]

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