Patrick Steinhardt <ps@xxxxxx> writes:
> On Thu, Apr 18, 2024 at 12:53:02PM +0000, Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin <johannes.schindelin@xxxxxx>
> [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.
