Re: [PATCH v2 1/2] for-each-repo: optionally keep going on an error

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> On Thu, Apr 18, 2024 at 12:53:02PM +0000, Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> [snip]
>> @@ -55,8 +58,9 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)
>>  	else if (err)
>>  		return 0;
>>  
>> -	for (i = 0; !result && i < values->nr; i++)
>> -		result = run_command_on_repo(values->items[i].string, argc, argv);
>> +	for (i = 0; (keep_going || !result) && i < values->nr; i++)
>> +		if (run_command_on_repo(values->items[i].string, argc, argv))
>> +			result = 1;
>
> One thing that made me stop and think is whether the change in behaviour
> here may negatively impact some usecases. Before this change we would
> error out with the return code returned by the command that we have ran
> in repositories. It makes total sense that we don't do that anymore with
> `--keep-going`, because the result would likely be useless as all we
> could do was to OR the result codes with each other.
>
> But do we maybe want to make this conditional on whether or not the
> `--keep-going` flag is set? So something like this:
>
> ```
> for (i = 0; (keep_going || !result) && i < values->nr; i++) {
> 	int ret = run_command_on_repo(values->items[i].string, argc, argv);
> 	if (ret)
> 		result = keep_going ? 1 : ret;
> }
> ```

You mean that it could be a regression that we lose the raw return
value from run_command_on_repo() when !keep_going?

 - git.c:handle_builtin() does exit(run_builtin(builtin, argc, argv));
   In this case, builtin is set to cmd_for_each_repo.

 - cmd_for_each_repo does "return result" at its end.

 - result comes from run_command_on_repo(), which returns the value
   returned by run_command().

 - run_command() returns -1 for "not found".

So if run_command() failed due to missing command, we would have
exited with 255 (= (unsigned)(-1) & 0xFF), but with this change we
would now exit with 1.

Passing anything outside 0..255 to exit(3) is a bad manners, and but
this does change behaviour.  Hmmm.