Re: git bisect bad @

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 14, 2022, 18:42 UTC
Message-ID: <xmqqmtjycg6g.fsf@gitster.g>
In-Reply-To: <88899d16-5e3e-2bb2-07e9-59f7607c91a8@web.de>

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

Show 28 quoted lines

> 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.