[PATCH 0/2] Use a "best effort" strategy in scheduled maintenance

[PATCH 0/2] Use a "best effort" strategy in scheduled maintenance

[Johannes Schindelin via GitGitGadget]

Over in https://github.com/microsoft/git/issues/623, it was pointed out that
scheduled maintenance will error out when it encounters a missing
repository. The scheduled maintenance should exit with an error, all right,
but what about the remaining repositories for which maintenance was
scheduled, and that may not be missing?

This patch series addresses this by introducing a new for-each-repo option
and then using it in the command that is run via scheduled maintenance.

Johannes Schindelin (2):
  for-each-repo: optionally keep going on an error
  maintenance: running maintenance should not stop on errors

 Documentation/git-for-each-repo.txt |  4 ++++
 builtin/for-each-repo.c             |  8 ++++++--
 builtin/gc.c                        |  7 ++++---
 t/t0068-for-each-repo.sh            | 16 ++++++++++++++++
 t/t7900-maintenance.sh              |  6 +++---
 5 files changed, 33 insertions(+), 8 deletions(-)


base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1719%2Fdscho%2Ffor-each-repo-stop-on-error-2.44-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1719/dscho/for-each-repo-stop-on-error-2.44-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1719
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-for-each-repo.txt |  4 ++++
 builtin/for-each-repo.c             |  8 ++++++--
 t/t0068-for-each-repo.sh            | 16 ++++++++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

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..74498539e9c 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_("stop at the first repository where the operation failed")),
 		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
-- 
gitgitgadget

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

[Eric Sunshine]

On Wed, Apr 17, 2024 at 4:29 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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 the running the specified command with the
> other repositories.

s/try the running/try running/

> 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.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
> @@ -39,6 +40,8 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)
> +               OPT_BOOL(0, "keep-going", &keep_going,
> +                        N_("stop at the first repository where the operation failed")),

Isn't this help string opposite the intended meaning? Taking a hint
from GNU "make --help", should it instead by something like:

    N_("keep going even if command fails in a repository")),

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> 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 the running the specified command with the
>> other repositories.
>
> s/try the running/try running/
> ...
>> +               OPT_BOOL(0, "keep-going", &keep_going,
>> +                        N_("stop at the first repository where the operation failed")),
>
> Isn't this help string opposite the intended meaning? Taking a hint
> from GNU "make --help", should it instead by something like:
>
>     N_("keep going even if command fails in a repository")),

Good eyes.  Thanks for carefully reading.

[PATCH 2/2] maintenance: running maintenance should not stop on errors

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In https://github.com/microsoft/git/issues/623, it was reported that
maintenance stops on a missing repository, omitting the remaining
repositories that were scheduled for maintenance.

This is undesirable, as it should be a best effort type of operation.

It should still fail due to the missing repository, of course, but not
leave the non-missing repositories in unmaintained shapes.

Let's use `for-each-repo`'s shiny new `--keep-going` option that we just
introduced for that very purpose.

This change will be picked up when running `git maintenance start`,
which is run implicitly by `scalar reconfigure`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/gc.c           | 7 ++++---
 t/t7900-maintenance.sh | 6 +++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index cb80ced6cb5..b069aa49c50 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1858,6 +1858,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
 		   "<string>%s/git</string>\n"
 		   "<string>--exec-path=%s</string>\n"
 		   "<string>for-each-repo</string>\n"
+		   "<string>--keep-going</string>\n"
 		   "<string>--config=maintenance.repo</string>\n"
 		   "<string>maintenance</string>\n"
 		   "<string>run</string>\n"
@@ -2100,7 +2101,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 	      "<Actions Context=\"Author\">\n"
 	      "<Exec>\n"
 	      "<Command>\"%s\\headless-git.exe\"</Command>\n"
-	      "<Arguments>--exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%s</Arguments>\n"
+	      "<Arguments>--exec-path=\"%s\" for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=%s</Arguments>\n"
 	      "</Exec>\n"
 	      "</Actions>\n"
 	      "</Task>\n";
@@ -2245,7 +2246,7 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 			"# replaced in the future by a Git command.\n\n");
 
 		strbuf_addf(&line_format,
-			    "%%d %%s * * %%s \"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%s\n",
+			    "%%d %%s * * %%s \"%s/git\" --exec-path=\"%s\" for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=%%s\n",
 			    exec_path, exec_path);
 		fprintf(cron_in, line_format.buf, minute, "1-23", "*", "hourly");
 		fprintf(cron_in, line_format.buf, minute, "0", "1-6", "daily");
@@ -2446,7 +2447,7 @@ static int systemd_timer_write_service_template(const char *exec_path)
 	       "\n"
 	       "[Service]\n"
 	       "Type=oneshot\n"
-	       "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n"
+	       "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=%%i\n"
 	       "LockPersonality=yes\n"
 	       "MemoryDenyWriteExecute=yes\n"
 	       "NoNewPrivileges=yes\n"
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 0943dfa18a3..8595489cebe 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -639,9 +639,9 @@ test_expect_success 'start from empty cron table' '
 	# start registers the repo
 	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
 
-	grep "for-each-repo --config=maintenance.repo maintenance run --schedule=daily" cron.txt &&
-	grep "for-each-repo --config=maintenance.repo maintenance run --schedule=hourly" cron.txt &&
-	grep "for-each-repo --config=maintenance.repo maintenance run --schedule=weekly" cron.txt
+	grep "for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=daily" cron.txt &&
+	grep "for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=hourly" cron.txt &&
+	grep "for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=weekly" cron.txt
 '
 
 test_expect_success 'stop from existing schedule' '
-- 
gitgitgadget

Re: [PATCH 2/2] maintenance: running maintenance should not stop on errors

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In https://github.com/microsoft/git/issues/623, it was reported that
> maintenance stops on a missing repository, omitting the remaining
> repositories that were scheduled for maintenance.
>
> This is undesirable, as it should be a best effort type of operation.
>
> It should still fail due to the missing repository, of course, but not
> leave the non-missing repositories in unmaintained shapes.
>
> Let's use `for-each-repo`'s shiny new `--keep-going` option that we just
> introduced for that very purpose.
>
> This change will be picked up when running `git maintenance start`,
> which is run implicitly by `scalar reconfigure`.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/gc.c           | 7 ++++---
>  t/t7900-maintenance.sh | 6 +++---
>  2 files changed, 7 insertions(+), 6 deletions(-)

Other than the N_("") thing Eric noticed, I didn't find anything
glaringly wrong in these two patches.  Nicely done.

We may want to fold overly long lines we see in the patch, but I'd
prefer to see it done as a post-cleanup task (#leftoverbits), as the
lines in the preimage of the patch are already overly long.

Thanks.

Re: [PATCH 0/2] Use a "best effort" strategy in scheduled maintenance

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> Over in https://github.com/microsoft/git/issues/623, it was pointed out that
> scheduled maintenance will error out when it encounters a missing
> repository. The scheduled maintenance should exit with an error, all right,
> but what about the remaining repositories for which maintenance was
> scheduled, and that may not be missing?

Interesting.

As the order of the maintenance run across repositories is not
controlled by the end-user, I think it is reasonable to keep going
and finish the others, at least by default (if you could control the
ordering, you could make an arrangement where having ran maintenance
on repository A successfully would be necessary to do maintenance on
repository B, but we do not give that power to the end users).

> This patch series addresses this by introducing a new for-each-repo option
> and then using it in the command that is run via scheduled maintenance.

OK.

[PATCH v2 0/2] Use a "best effort" strategy in scheduled maintenance

[Johannes Schindelin via GitGitGadget]

Over in https://github.com/microsoft/git/issues/623, it was pointed out that
scheduled maintenance will error out when it encounters a missing
repository. The scheduled maintenance should exit with an error, all right,
but what about the remaining repositories for which maintenance was
scheduled, and that may not be missing?

This patch series addresses this by introducing a new for-each-repo option
and then using it in the command that is run via scheduled maintenance.

Changes since v1 (thanks Eric!):

 * Changed the option's documentation to reflect the current state (instead
   of the original design)
 * Fixed grammar issues

Johannes Schindelin (2):
  for-each-repo: optionally keep going on an error
  maintenance: running maintenance should not stop on errors

 Documentation/git-for-each-repo.txt |  4 ++++
 builtin/for-each-repo.c             |  8 ++++++--
 builtin/gc.c                        |  7 ++++---
 t/t0068-for-each-repo.sh            | 16 ++++++++++++++++
 t/t7900-maintenance.sh              |  6 +++---
 5 files changed, 33 insertions(+), 8 deletions(-)


base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1719%2Fdscho%2Ffor-each-repo-stop-on-error-2.44-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1719/dscho/for-each-repo-stop-on-error-2.44-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1719

Range-diff vs v1:

 1:  6721e3ada5d ! 1:  abd796894c8 for-each-repo: optionally keep going on an error
     @@ Commit message
      
          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 the running the specified command with the
     -    other repositories.
     +    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 <sunshine@sunshineco.com>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## Documentation/git-for-each-repo.txt ##
     @@ builtin/for-each-repo.c: int cmd_for_each_repo(int argc, const char **argv, cons
       		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_("stop at the first repository where the operation failed")),
     ++			 N_("keep going even if command fails in a repository")),
       		OPT_END()
       	};
       
 2:  a86bcf2e1a0 = 2:  1ae11553052 maintenance: running maintenance should not stop on errors

-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <sunshine@sunshineco.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-for-each-repo.txt |  4 ++++
 builtin/for-each-repo.c             |  8 ++++++--
 t/t0068-for-each-repo.sh            | 16 ++++++++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

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

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1360 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

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.

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

[Jeff King]

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

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

[Junio C Hamano]

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.

[PATCH v2 2/2] maintenance: running maintenance should not stop on errors

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In https://github.com/microsoft/git/issues/623, it was reported that
maintenance stops on a missing repository, omitting the remaining
repositories that were scheduled for maintenance.

This is undesirable, as it should be a best effort type of operation.

It should still fail due to the missing repository, of course, but not
leave the non-missing repositories in unmaintained shapes.

Let's use `for-each-repo`'s shiny new `--keep-going` option that we just
introduced for that very purpose.

This change will be picked up when running `git maintenance start`,
which is run implicitly by `scalar reconfigure`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/gc.c           | 7 ++++---
 t/t7900-maintenance.sh | 6 +++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index cb80ced6cb5..b069aa49c50 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1858,6 +1858,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
 		   "<string>%s/git</string>\n"
 		   "<string>--exec-path=%s</string>\n"
 		   "<string>for-each-repo</string>\n"
+		   "<string>--keep-going</string>\n"
 		   "<string>--config=maintenance.repo</string>\n"
 		   "<string>maintenance</string>\n"
 		   "<string>run</string>\n"
@@ -2100,7 +2101,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 	      "<Actions Context=\"Author\">\n"
 	      "<Exec>\n"
 	      "<Command>\"%s\\headless-git.exe\"</Command>\n"
-	      "<Arguments>--exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%s</Arguments>\n"
+	      "<Arguments>--exec-path=\"%s\" for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=%s</Arguments>\n"
 	      "</Exec>\n"
 	      "</Actions>\n"
 	      "</Task>\n";
@@ -2245,7 +2246,7 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 			"# replaced in the future by a Git command.\n\n");
 
 		strbuf_addf(&line_format,
-			    "%%d %%s * * %%s \"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%s\n",
+			    "%%d %%s * * %%s \"%s/git\" --exec-path=\"%s\" for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=%%s\n",
 			    exec_path, exec_path);
 		fprintf(cron_in, line_format.buf, minute, "1-23", "*", "hourly");
 		fprintf(cron_in, line_format.buf, minute, "0", "1-6", "daily");
@@ -2446,7 +2447,7 @@ static int systemd_timer_write_service_template(const char *exec_path)
 	       "\n"
 	       "[Service]\n"
 	       "Type=oneshot\n"
-	       "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n"
+	       "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=%%i\n"
 	       "LockPersonality=yes\n"
 	       "MemoryDenyWriteExecute=yes\n"
 	       "NoNewPrivileges=yes\n"
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 0943dfa18a3..8595489cebe 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -639,9 +639,9 @@ test_expect_success 'start from empty cron table' '
 	# start registers the repo
 	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
 
-	grep "for-each-repo --config=maintenance.repo maintenance run --schedule=daily" cron.txt &&
-	grep "for-each-repo --config=maintenance.repo maintenance run --schedule=hourly" cron.txt &&
-	grep "for-each-repo --config=maintenance.repo maintenance run --schedule=weekly" cron.txt
+	grep "for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=daily" cron.txt &&
+	grep "for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=hourly" cron.txt &&
+	grep "for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=weekly" cron.txt
 '
 
 test_expect_success 'stop from existing schedule' '
-- 
gitgitgadget

[PATCH v3 0/2] Use a "best effort" strategy in scheduled maintenance

[Johannes Schindelin via GitGitGadget]

Over in https://github.com/microsoft/git/issues/623, it was pointed out that
scheduled maintenance will error out when it encounters a missing
repository. The scheduled maintenance should exit with an error, all right,
but what about the remaining repositories for which maintenance was
scheduled, and that may not be missing?

This patch series addresses this by introducing a new for-each-repo option
and then using it in the command that is run via scheduled maintenance.

Changes since v2 (thanks Patrick, Jeff and Junio):

 * When not passing the new --keep-going option, the exit code is the same
   as before.
 * Clarified in the documentation of the --keep-going option that it is 0
   for success, 1 for failure, no matter the exact exit code of the failing
   command invocation(s).

Changes since v1 (thanks Eric!):

 * Changed the option's documentation to reflect the current state (instead
   of the original design)
 * Fixed grammar issues

Johannes Schindelin (2):
  for-each-repo: optionally keep going on an error
  maintenance: running maintenance should not stop on errors

 Documentation/git-for-each-repo.txt |  9 +++++++++
 builtin/for-each-repo.c             | 13 +++++++++++--
 builtin/gc.c                        |  7 ++++---
 t/t0068-for-each-repo.sh            | 16 ++++++++++++++++
 t/t7900-maintenance.sh              |  6 +++---
 5 files changed, 43 insertions(+), 8 deletions(-)


base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1719%2Fdscho%2Ffor-each-repo-stop-on-error-2.44-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1719/dscho/for-each-repo-stop-on-error-2.44-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1719

Range-diff vs v2:

 1:  abd796894c8 ! 1:  39ee6386aab for-each-repo: optionally keep going on an error
     @@ Commit message
          repository, still setting the exit code to indicate an error occurred.
      
          Helped-by: Eric Sunshine <sunshine@sunshineco.com>
     +    Helped-by: Patrick Steinhardt <ps@pks.im>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## Documentation/git-for-each-repo.txt ##
     @@ Documentation/git-for-each-repo.txt: These config values are loaded from system,
      +	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
       -------------------
     @@ builtin/for-each-repo.c: int cmd_for_each_repo(int argc, const char **argv, cons
       
      -	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))
     ++	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;
       }
 2:  1ae11553052 = 2:  540962859c5 maintenance: running maintenance should not stop on errors

-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <sunshine@sunshineco.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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(-)

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

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
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 v3 2/2] maintenance: running maintenance should not stop on errors

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In https://github.com/microsoft/git/issues/623, it was reported that
maintenance stops on a missing repository, omitting the remaining
repositories that were scheduled for maintenance.

This is undesirable, as it should be a best effort type of operation.

It should still fail due to the missing repository, of course, but not
leave the non-missing repositories in unmaintained shapes.

Let's use `for-each-repo`'s shiny new `--keep-going` option that we just
introduced for that very purpose.

This change will be picked up when running `git maintenance start`,
which is run implicitly by `scalar reconfigure`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/gc.c           | 7 ++++---
 t/t7900-maintenance.sh | 6 +++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index cb80ced6cb5..b069aa49c50 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1858,6 +1858,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
 		   "<string>%s/git</string>\n"
 		   "<string>--exec-path=%s</string>\n"
 		   "<string>for-each-repo</string>\n"
+		   "<string>--keep-going</string>\n"
 		   "<string>--config=maintenance.repo</string>\n"
 		   "<string>maintenance</string>\n"
 		   "<string>run</string>\n"
@@ -2100,7 +2101,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 	      "<Actions Context=\"Author\">\n"
 	      "<Exec>\n"
 	      "<Command>\"%s\\headless-git.exe\"</Command>\n"
-	      "<Arguments>--exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%s</Arguments>\n"
+	      "<Arguments>--exec-path=\"%s\" for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=%s</Arguments>\n"
 	      "</Exec>\n"
 	      "</Actions>\n"
 	      "</Task>\n";
@@ -2245,7 +2246,7 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 			"# replaced in the future by a Git command.\n\n");
 
 		strbuf_addf(&line_format,
-			    "%%d %%s * * %%s \"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%s\n",
+			    "%%d %%s * * %%s \"%s/git\" --exec-path=\"%s\" for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=%%s\n",
 			    exec_path, exec_path);
 		fprintf(cron_in, line_format.buf, minute, "1-23", "*", "hourly");
 		fprintf(cron_in, line_format.buf, minute, "0", "1-6", "daily");
@@ -2446,7 +2447,7 @@ static int systemd_timer_write_service_template(const char *exec_path)
 	       "\n"
 	       "[Service]\n"
 	       "Type=oneshot\n"
-	       "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n"
+	       "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=%%i\n"
 	       "LockPersonality=yes\n"
 	       "MemoryDenyWriteExecute=yes\n"
 	       "NoNewPrivileges=yes\n"
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 0943dfa18a3..8595489cebe 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -639,9 +639,9 @@ test_expect_success 'start from empty cron table' '
 	# start registers the repo
 	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
 
-	grep "for-each-repo --config=maintenance.repo maintenance run --schedule=daily" cron.txt &&
-	grep "for-each-repo --config=maintenance.repo maintenance run --schedule=hourly" cron.txt &&
-	grep "for-each-repo --config=maintenance.repo maintenance run --schedule=weekly" cron.txt
+	grep "for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=daily" cron.txt &&
+	grep "for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=hourly" cron.txt &&
+	grep "for-each-repo --keep-going --config=maintenance.repo maintenance run --schedule=weekly" cron.txt
 '
 
 test_expect_success 'stop from existing schedule' '
-- 
gitgitgadget

Re: [PATCH v3 0/2] Use a "best effort" strategy in scheduled maintenance

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1161 bytes --]

On Wed, Apr 24, 2024 at 04:14:57PM +0000, Johannes Schindelin via GitGitGadget wrote:
> Over in https://github.com/microsoft/git/issues/623, it was pointed out that
> scheduled maintenance will error out when it encounters a missing
> repository. The scheduled maintenance should exit with an error, all right,
> but what about the remaining repositories for which maintenance was
> scheduled, and that may not be missing?
> 
> This patch series addresses this by introducing a new for-each-repo option
> and then using it in the command that is run via scheduled maintenance.
> 
> Changes since v2 (thanks Patrick, Jeff and Junio):
> 
>  * When not passing the new --keep-going option, the exit code is the same
>    as before.
>  * Clarified in the documentation of the --keep-going option that it is 0
>    for success, 1 for failure, no matter the exact exit code of the failing
>    command invocation(s).
> 
> Changes since v1 (thanks Eric!):
> 
>  * Changed the option's documentation to reflect the current state (instead
>    of the original design)
>  * Fixed grammar issues

Thanks, this version looks good to me!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]