[v3,1/2] for-each-repo: optionally keep going on an error

Commit Message

Johannes Schindelin April 24, 2024, 4:14 p.m. UTC

From: Johannes Schindelin <[e-mail]>

In https://github.com/microsoft/git/issues/623, it was reported that
the regularly scheduled maintenance stops if one repo in the middle of
the list was found to be missing.

This is undesirable, and points out a gap in the design of `git
for-each-repo`: We need a mode where that command does not stop on an
error, but continues to try running the specified command with the other
repositories.

Imitating the `--keep-going` option of GNU make, this commit teaches
`for-each-repo` the same trick: to continue with the operation on all
the remaining repositories in case there was a problem with one
repository, still setting the exit code to indicate an error occurred.

Helped-by: Eric Sunshine <[e-mail]>
Helped-by: Patrick Steinhardt <[e-mail]>
Signed-off-by: Johannes Schindelin <[e-mail]>
---
 Documentation/git-for-each-repo.txt |  9 +++++++++
 builtin/for-each-repo.c             | 13 +++++++++++--
 t/t0068-for-each-repo.sh            | 16 ++++++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)

Comments

Junio C Hamano April 24, 2024, 5:02 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> @@ -55,8 +58,14 @@ 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; i < values->nr; i++) {
> +		int ret = run_command_on_repo(values->items[i].string, argc, argv);
> +		if (ret) {
> +			if (!keep_going)
> +					return ret;
> +			result = 1;
> +		}
> +	}
>  
>  	return result;
>  }

Hmph, as I wish that more experienced folks to give a good structure
to the codebase from get-go so that future developers who may be
less experienced would avoid mistakes, with my maintainer's hat on,
I would have expected something more like:

	for (i = 0; i < values->nr; i++) {
		int ret = run_command_on_repo(...);
		if (!ret)
			continue;
		if (keep_going) {
                	result = 1;			
		} else {
                	result = ret;
                        break;
		}
	}

That way, clean-up actions, when they need to be added, can go
before the single "return result" without structural changes,
future-proofing the shape of the control flow.

The loop is simple enough that it is acceptable to leave as the
responsibility of future developers who wants to do something that
require resource allocation and release before returning, of course
;-).

Patch

diff --git a/Documentation/git-for-each-repo.txt b/Documentation/git-for-each-repo.txt
index 94bd19da263..abe3527aaca 100644
--- a/Documentation/git-for-each-repo.txt
+++ b/Documentation/git-for-each-repo.txt
@@ -42,6 +42,15 @@  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.
++
+Note that the exact exit code of the failing command is not passed
+through as the exit code of the `for-each-repo` command: If the command
+failed in any of the specified repositories, the overall exit code will
+be 1.
 
 SUBPROCESS BEHAVIOR
 -------------------
diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
index 28186b30f54..c4fa41fda9f 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,14 @@  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; i < values->nr; i++) {
+		int ret = run_command_on_repo(values->items[i].string, argc, argv);
+		if (ret) {
+			if (!keep_going)
+					return ret;
+			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