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

From: Jeff King <peff@peff.net>
Date: Apr 19, 2024, 17:56 UTC
Message-ID: <20240419175621.GB14309@coredump.intra.peff.net>
In-Reply-To: <xmqqy199l4qf.fsf@gitster.g>

On Fri, Apr 19, 2024 at 09:03:20AM -0700, Junio C Hamano wrote:

Show 19 quoted lines

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

run_command() may also return the exit code of the program run. So imagine a setup like:

  git init
  git config alias.foo '!exit 123'
  git config repo.paths "$PWD"
  git for-each-repo --config=repo.paths foo
  echo $?

Before the patch we see "123" and after we see "1".

I do agree that passing -1 to exit is bad; we maybe should normalize to 127 for not found, though I think we could also see -1 for system errors like fork() failing.

-Peff