| Message ID | abd796894c857fc9ad96b9942089474df01f0506.1713444783.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Use a "best effort" strategy in scheduled maintenance | expand |
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; } ``` Other than that this patch series looks good to me. Patrick
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.
On Fri, Apr 19, 2024 at 09:03:20AM -0700, Junio C Hamano wrote: > 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
Jeff King <peff@peff.net> writes: > 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". True, or when the process receives a signal, etc. With this change, we do lose information. > 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. True, but I think that is a separate issue. So, let's have a (hopefully final reroll to fix the error code when stopping at the first error and merge it to 'next'? Thanks.
diff --git a/Documentation/git-for-each-repo.txt b/Documentation/git-for-each-repo.txt index 94bd19da263..8c18001d825 100644 --- a/Documentation/git-for-each-repo.txt +++ b/Documentation/git-for-each-repo.txt @@ -42,6 +42,10 @@ These config values are loaded from system, global, and local Git config, as available. If `git for-each-repo` is run in a directory that is not a Git repository, then only the system and global config is used. +--keep-going:: + Continue with the remaining repositories if the command failed + on a repository. The exit code will still indicate that the + overall operation was not successful. SUBPROCESS BEHAVIOR ------------------- diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c index 28186b30f54..9bdf2b34f89 100644 --- a/builtin/for-each-repo.c +++ b/builtin/for-each-repo.c @@ -32,6 +32,7 @@ static int run_command_on_repo(const char *path, int argc, const char ** argv) int cmd_for_each_repo(int argc, const char **argv, const char *prefix) { static const char *config_key = NULL; + int keep_going = 0; int i, result = 0; const struct string_list *values; int err; @@ -39,6 +40,8 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix) const struct option options[] = { OPT_STRING(0, "config", &config_key, N_("config"), N_("config key storing a list of repository paths")), + OPT_BOOL(0, "keep-going", &keep_going, + N_("keep going even if command fails in a repository")), OPT_END() }; @@ -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; return result; } diff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh index 4b90b74d5d5..95019e01ed3 100755 --- a/t/t0068-for-each-repo.sh +++ b/t/t0068-for-each-repo.sh @@ -59,4 +59,20 @@ test_expect_success 'error on NULL value for config keys' ' test_cmp expect actual ' +test_expect_success '--keep-going' ' + git config keep.going non-existing && + git config --add keep.going . && + + test_must_fail git for-each-repo --config=keep.going \ + -- branch >out 2>err && + test_grep "cannot change to .*non-existing" err && + test_must_be_empty out && + + test_must_fail git for-each-repo --config=keep.going --keep-going \ + -- branch >out 2>err && + test_grep "cannot change to .*non-existing" err && + git branch >expect && + test_cmp expect out +' + test_done