[PATCH] maintenance: make unregister idempotent

[PATCH] maintenance: make unregister idempotent

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The 'git maintenance unregister' subcommand has a step that removes the
current repository from the multi-valued maitenance.repo config key.
This fails if the repository is not listed in that key. This makes
running 'git maintenance unregister' twice result in a failure in the
second instance.

Make this task idempotent, since the end result is the same in both
cases: maintenance will no longer run on this repository.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
    maintenance: make unregister idempotent
    
    I noticed this while we were updating the microsoft/git fork to include
    v2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent
    before, but instead some change in 'scalar unregister' led to it relying
    on the return code of 'git maintenance unregister'. Our functional tests
    expect 'scalar unregister' to be idempotent, and I think that's a good
    pattern for 'git maintenance unregister', so I'm fixing it at that
    layer.
    
    Despite finding this during the 2.38.0-rc0 integration, this isn't
    critical to the release.
    
    Perhaps an argument could be made that "failure means it wasn't
    registered before", but I think that isn't terribly helpful.
    
    Our functional tests are running the unregister subcommand to disable
    maintenance in order to run tests on the object store (such as running
    maintenance commands in the foreground and checking the object store
    afterwards). This is a form of automation using 'unregister' as a check
    that maintenance will not run at the same time, and it doesn't care if
    maintenance was already disabled. I can imagine other scripting
    scenarios wanting that kind of guarantee.
    
    Thanks, -Stolee

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1358%2Fderrickstolee%2Fmaintenance-unregister-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1358/derrickstolee/maintenance-unregister-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1358

 builtin/gc.c           | 24 +++++++++++++++++++-----
 t/t7900-maintenance.sh |  5 ++++-
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 0accc024067..787e9c702b2 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1528,9 +1528,13 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 	struct option options[] = {
 		OPT_END(),
 	};
-	int rc;
+	const char *key = "maintenance.repo";
+	int rc = 0;
 	struct child_process config_unset = CHILD_PROCESS_INIT;
 	char *maintpath = get_maintpath();
+	int found = 0;
+	struct string_list_item *item;
+	const struct string_list *list = git_config_get_value_multi(key);
 
 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_maintenance_unregister_usage, 0);
@@ -1538,11 +1542,21 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 		usage_with_options(builtin_maintenance_unregister_usage,
 				   options);
 
-	config_unset.git_cmd = 1;
-	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
-		     "--fixed-value", "maintenance.repo", maintpath, NULL);
+	for_each_string_list_item(item, list) {
+		if (!strcmp(maintpath, item->string)) {
+			found = 1;
+			break;
+		}
+	}
+
+	if (found) {
+		config_unset.git_cmd = 1;
+		strvec_pushl(&config_unset.args, "config", "--global", "--unset",
+			     "--fixed-value", key, maintpath, NULL);
+
+		rc = run_command(&config_unset);
+	}
 
-	rc = run_command(&config_unset);
 	free(maintpath);
 	return rc;
 }
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 2724a44fe3e..3747e4a14f8 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -493,7 +493,10 @@ test_expect_success 'register and unregister' '
 
 	git maintenance unregister &&
 	git config --global --get-all maintenance.repo >actual &&
-	test_cmp before actual
+	test_cmp before actual &&
+
+	# Expect unregister to be idempotent.
+	git maintenance unregister
 '
 
 test_expect_success !MINGW 'register and unregister with regex metacharacters' '

base-commit: d3fa443f97e3a8d75b51341e2d5bac380b7422df
-- 
gitgitgadget

Re: [PATCH] maintenance: make unregister idempotent

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <derrickstolee@github.com>
>
> The 'git maintenance unregister' subcommand has a step that removes the
> current repository from the multi-valued maitenance.repo config key.
> This fails if the repository is not listed in that key. This makes
> running 'git maintenance unregister' twice result in a failure in the
> second instance.
>
> Make this task idempotent, since the end result is the same in both
> cases: maintenance will no longer run on this repository.

I am not sure if this is a good idea.  What is the ultimate reason
why we want to allow running it blindly without knowing if it is
necessary?  Is it because there is no easy way to tell if unregister
is needed in the first place?

If this were "unregister X" that takes a parameter that tells what
to unregister, I would be vehemently opposed to the idea, because
I'd expect people make typos and want to be reminded of them when
they happen, but ...

>  	git maintenance unregister &&
>  	git config --global --get-all maintenance.repo >actual &&
> -	test_cmp before actual
> +	test_cmp before actual &&
> +
> +	# Expect unregister to be idempotent.
> +	git maintenance unregister
>  '

... given that the user does not have any control over what to
unregister from the command line (it is decided implicitly by where
the user is), I am halfway OK with it.

A user with two repositories may still be upset after running
"unregister" in the one they did not mean to, and not getting told
about the mistake, though, e.g.

    $ ls *.git
    a.git b.git
    $ cd a.git
    $ git maintenance unregister
    $ cd b.git
    ... this would give you an error message ...
    $ git maintenance unregister
    ... this is silent with the change ...

Surely, the second "cd" should be to ../b.git instead of b.git and
surely the user should have noticed that "cd" gave them an error,
but the unregister that complains would be an extra chance for them
to notice the mistake when they wanted to unregister the maintenance
tasks from all their repositories.

I wonder if something like the "--force" option, i.e.

    $ >cruft
    $ rm curft; echo $?
    rm: cannot remove 'curft': No such file or directory
    1
    $ rm cruft; echo $?
    0
    $ rm cruft; echo $?
    rm: cannot remove 'cruft': No such file or directory
    1
    $ rm -f cruft; echo $?
    0

which gives us both typo detection while supporting blind removal,
is a usable workaround?  I dunno.

But as I said, I am halfway OK with the change.  I just think it is
a bit unfriendly to the users.

Thanks.

Re: [PATCH] maintenance: make unregister idempotent

[Derrick Stolee]

On 9/21/2022 1:19 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> The 'git maintenance unregister' subcommand has a step that removes the
>> current repository from the multi-valued maitenance.repo config key.
>> This fails if the repository is not listed in that key. This makes
>> running 'git maintenance unregister' twice result in a failure in the
>> second instance.
>>
>> Make this task idempotent, since the end result is the same in both
>> cases: maintenance will no longer run on this repository.
> 
> I am not sure if this is a good idea.  What is the ultimate reason
> why we want to allow running it blindly without knowing if it is
> necessary?  Is it because there is no easy way to tell if unregister
> is needed in the first place?

We want to leave the internal details of what it means to be
registered as hidden to the user. They could look for the repo in
the global config, but that seems like a hassle when they just
want to make sure they are not currently registered. 

>> +	# Expect unregister to be idempotent.
>> +	git maintenance unregister
>>  '
> 
> ... given that the user does not have any control over what to
> unregister from the command line (it is decided implicitly by where
> the user is), I am halfway OK with it.
> 
> A user with two repositories may still be upset after running
> "unregister" in the one they did not mean to, and not getting told
> about the mistake, though, e.g.
...
> I wonder if something like the "--force" option, 

I like the --force option. I'll add it in v2 and then update Scalar
to use that option.

Thanks,
-Stolee

[PATCH v2 0/2] scalar: make unregister idempotent

[Derrick Stolee via GitGitGadget]

I noticed this while we were updating the microsoft/git fork to include
v2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent
before, but instead some change in 'scalar unregister' led to it relying on
the return code of 'git maintenance unregister'. Our functional tests expect
'scalar unregister' to be idempotent, and I think that's a good pattern for
'git maintenance unregister', so I'm fixing it at that layer.

Despite finding this during the 2.38.0-rc0 integration, this isn't critical
to the release.

Perhaps an argument could be made that "failure means it wasn't registered
before", but I think that isn't terribly helpful.

Our functional tests are running the unregister subcommand to disable
maintenance in order to run tests on the object store (such as running
maintenance commands in the foreground and checking the object store
afterwards). This is a form of automation using 'unregister' as a check that
maintenance will not run at the same time, and it doesn't care if
maintenance was already disabled. I can imagine other scripting scenarios
wanting that kind of guarantee.


Updates in v2
=============

 * This is now a two-patch series.
 * I rebased onto v2.38.0-rc1 for two reasons: Scalar is now merged, and the
   usage for 'git maintenance unregister' removed its translation markers.
 * Instead of making git maintenance unregister idempotent, add a --force
   option for those who do not want to require that the repository is
   already registered.
 * Make scalar unregister idempotent, with reasons argued in patch 2.

Thanks, -Stolee

Derrick Stolee (2):
  maintenance: add 'unregister --force'
  scalar: make 'unregister' idempotent

 Documentation/git-maintenance.txt |  6 +++++-
 builtin/gc.c                      | 31 +++++++++++++++++++++++++------
 scalar.c                          |  5 ++++-
 t/t7900-maintenance.sh            |  6 +++++-
 t/t9210-scalar.sh                 |  5 ++++-
 5 files changed, 43 insertions(+), 10 deletions(-)


base-commit: 1b3d6e17fe83eb6f79ffbac2f2c61bbf1eaef5f8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1358%2Fderrickstolee%2Fmaintenance-unregister-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1358/derrickstolee/maintenance-unregister-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1358

Range-diff vs v1:

 1:  f028208a3ab ! 1:  69c74f52eef maintenance: make unregister idempotent
     @@ Metadata
      Author: Derrick Stolee <derrickstolee@github.com>
      
       ## Commit message ##
     -    maintenance: make unregister idempotent
     +    maintenance: add 'unregister --force'
      
          The 'git maintenance unregister' subcommand has a step that removes the
          current repository from the multi-valued maitenance.repo config key.
     @@ Commit message
          running 'git maintenance unregister' twice result in a failure in the
          second instance.
      
     -    Make this task idempotent, since the end result is the same in both
     -    cases: maintenance will no longer run on this repository.
     +    This failure exit code is helpful, but its message is not. Add a new
     +    die() message that explicitly calls out the failure due to the
     +    repository not being registered.
     +
     +    In some cases, users may want to run 'git maintenance unregister' just
     +    to make sure that background jobs will not start on this repository, but
     +    they do not want to check to see if it is registered first. Add a new
     +    '--force' option that will siltently succeed if the repository is not
     +    already registered.
      
          Signed-off-by: Derrick Stolee <derrickstolee@github.com>
      
     + ## Documentation/git-maintenance.txt ##
     +@@ Documentation/git-maintenance.txt: SYNOPSIS
     + [verse]
     + 'git maintenance' run [<options>]
     + 'git maintenance' start [--scheduler=<scheduler>]
     +-'git maintenance' (stop|register|unregister)
     ++'git maintenance' (stop|register|unregister) [<options>]
     + 
     + 
     + DESCRIPTION
     +@@ Documentation/git-maintenance.txt: unregister::
     + 	Remove the current repository from background maintenance. This
     + 	only removes the repository from the configured list. It does not
     + 	stop the background maintenance processes from running.
     +++
     ++The `unregister` subcommand will report an error if the current repository
     ++is not already registered. Use the `--force` option to return success even
     ++when the current repository is not registered.
     + 
     + TASKS
     + -----
     +
       ## builtin/gc.c ##
     -@@ builtin/gc.c: static int maintenance_unregister(int argc, const char **argv, const char *prefi
     +@@ builtin/gc.c: done:
     + }
     + 
     + static char const * const builtin_maintenance_unregister_usage[] = {
     +-	"git maintenance unregister",
     ++	"git maintenance unregister [--force]",
     + 	NULL
     + };
     + 
     + static int maintenance_unregister(int argc, const char **argv, const char *prefix)
     + {
     ++	int force = 0;
       	struct option options[] = {
     ++		OPT_BOOL(0, "force", &force,
     ++			 N_("return success even if repository was not registered")),
       		OPT_END(),
       	};
      -	int rc;
     @@ builtin/gc.c: static int maintenance_unregister(int argc, const char **argv, con
      +			     "--fixed-value", key, maintpath, NULL);
      +
      +		rc = run_command(&config_unset);
     ++	} else if (!force) {
     ++		die(_("repository '%s' is not registered"), maintpath);
      +	}
       
      -	rc = run_command(&config_unset);
     @@ t/t7900-maintenance.sh: test_expect_success 'register and unregister' '
      -	test_cmp before actual
      +	test_cmp before actual &&
      +
     -+	# Expect unregister to be idempotent.
     -+	git maintenance unregister
     ++	test_must_fail git maintenance unregister 2>err &&
     ++	grep "is not registered" err &&
     ++	git maintenance unregister --force
       '
       
       test_expect_success !MINGW 'register and unregister with regex metacharacters' '
 -:  ----------- > 2:  f5d8d6e4901 scalar: make 'unregister' idempotent

-- 
gitgitgadget

[PATCH v2 1/2] maintenance: add 'unregister --force'

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The 'git maintenance unregister' subcommand has a step that removes the
current repository from the multi-valued maitenance.repo config key.
This fails if the repository is not listed in that key. This makes
running 'git maintenance unregister' twice result in a failure in the
second instance.

This failure exit code is helpful, but its message is not. Add a new
die() message that explicitly calls out the failure due to the
repository not being registered.

In some cases, users may want to run 'git maintenance unregister' just
to make sure that background jobs will not start on this repository, but
they do not want to check to see if it is registered first. Add a new
'--force' option that will siltently succeed if the repository is not
already registered.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 Documentation/git-maintenance.txt |  6 +++++-
 builtin/gc.c                      | 31 +++++++++++++++++++++++++------
 t/t7900-maintenance.sh            |  6 +++++-
 3 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
index 9c630efe19c..bb888690e4d 100644
--- a/Documentation/git-maintenance.txt
+++ b/Documentation/git-maintenance.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git maintenance' run [<options>]
 'git maintenance' start [--scheduler=<scheduler>]
-'git maintenance' (stop|register|unregister)
+'git maintenance' (stop|register|unregister) [<options>]
 
 
 DESCRIPTION
@@ -79,6 +79,10 @@ unregister::
 	Remove the current repository from background maintenance. This
 	only removes the repository from the configured list. It does not
 	stop the background maintenance processes from running.
++
+The `unregister` subcommand will report an error if the current repository
+is not already registered. Use the `--force` option to return success even
+when the current repository is not registered.
 
 TASKS
 -----
diff --git a/builtin/gc.c b/builtin/gc.c
index 2753bd15a5e..3189b4ba2b1 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1519,18 +1519,25 @@ done:
 }
 
 static char const * const builtin_maintenance_unregister_usage[] = {
-	"git maintenance unregister",
+	"git maintenance unregister [--force]",
 	NULL
 };
 
 static int maintenance_unregister(int argc, const char **argv, const char *prefix)
 {
+	int force = 0;
 	struct option options[] = {
+		OPT_BOOL(0, "force", &force,
+			 N_("return success even if repository was not registered")),
 		OPT_END(),
 	};
-	int rc;
+	const char *key = "maintenance.repo";
+	int rc = 0;
 	struct child_process config_unset = CHILD_PROCESS_INIT;
 	char *maintpath = get_maintpath();
+	int found = 0;
+	struct string_list_item *item;
+	const struct string_list *list = git_config_get_value_multi(key);
 
 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_maintenance_unregister_usage, 0);
@@ -1538,11 +1545,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 		usage_with_options(builtin_maintenance_unregister_usage,
 				   options);
 
-	config_unset.git_cmd = 1;
-	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
-		     "--fixed-value", "maintenance.repo", maintpath, NULL);
+	for_each_string_list_item(item, list) {
+		if (!strcmp(maintpath, item->string)) {
+			found = 1;
+			break;
+		}
+	}
+
+	if (found) {
+		config_unset.git_cmd = 1;
+		strvec_pushl(&config_unset.args, "config", "--global", "--unset",
+			     "--fixed-value", key, maintpath, NULL);
+
+		rc = run_command(&config_unset);
+	} else if (!force) {
+		die(_("repository '%s' is not registered"), maintpath);
+	}
 
-	rc = run_command(&config_unset);
 	free(maintpath);
 	return rc;
 }
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 2724a44fe3e..cfe3236567a 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -493,7 +493,11 @@ test_expect_success 'register and unregister' '
 
 	git maintenance unregister &&
 	git config --global --get-all maintenance.repo >actual &&
-	test_cmp before actual
+	test_cmp before actual &&
+
+	test_must_fail git maintenance unregister 2>err &&
+	grep "is not registered" err &&
+	git maintenance unregister --force
 '
 
 test_expect_success !MINGW 'register and unregister with regex metacharacters' '
-- 
gitgitgadget

[PATCH v2 2/2] scalar: make 'unregister' idempotent

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The 'scalar unregister' command removes a repository from the list of
registered Scalar repositories and removes it from the list of
repositories registered for background maintenance. If the repository
was not already registered for background maintenance, then the command
fails, even if the repository was still registered as a Scalar
repository.

After using 'scalar clone' or 'scalar register', the repository would be
enrolled in background maintenance since those commands run 'git
maintenance start'. If the user runs 'git maintenance unregister' on
that repository, then it is still in the list of repositories which get
new config updates from 'scalar reconfigure'. The 'scalar unregister'
command would fail since 'git maintenance unregister' would fail.

Further, the add_or_remove_enlistment() method in scalar.c already has
this idempotent nature built in as an expectation since it returns zero
when the scalar.repo list already has the proper containment of the
repository.

The previous change added the 'git maintenance unregister --force'
option, so use it within 'scalar unregister' to make it idempotent.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 scalar.c          | 5 ++++-
 t/t9210-scalar.sh | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/scalar.c b/scalar.c
index c5c1ce68919..6de9c0ee523 100644
--- a/scalar.c
+++ b/scalar.c
@@ -207,7 +207,10 @@ static int set_recommended_config(int reconfigure)
 
 static int toggle_maintenance(int enable)
 {
-	return run_git("maintenance", enable ? "start" : "unregister", NULL);
+	return run_git("maintenance",
+		       enable ? "start" : "unregister",
+		       enable ? NULL : "--force",
+		       NULL);
 }
 
 static int add_or_remove_enlistment(int add)
diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
index 14ca575a214..be51a8bb7a4 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -116,7 +116,10 @@ test_expect_success 'scalar unregister' '
 	test_must_fail git config --get --global --fixed-value \
 		maintenance.repo "$(pwd)/vanish/src" &&
 	scalar list >scalar.repos &&
-	! grep -F "$(pwd)/vanish/src" scalar.repos
+	! grep -F "$(pwd)/vanish/src" scalar.repos &&
+
+	# scalar unregister should be idempotent
+	scalar unregister vanish
 '
 
 test_expect_success 'set up repository to clone' '
-- 
gitgitgadget

Re: [PATCH] maintenance: make unregister idempotent

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

>> I am not sure if this is a good idea.  What is the ultimate reason
>> why we want to allow running it blindly without knowing if it is
>> necessary?  Is it because there is no easy way to tell if unregister
>> is needed in the first place?
>
> We want to leave the internal details of what it means to be
> registered as hidden to the user. They could look for the repo in
> the global config, but that seems like a hassle when they just
> want to make sure they are not currently registered. 

OK, so there is no published officially sanctioned way to ask "is
this repository under maintenance's control and cron jobs run in
it?" or "give me the list of such repositories".  

Then I can see why you want to allow users to blindly run
"unregister", with or without "--force".

But doesn't it point at a more fundamental problem?  

Is there a reason why we want to hide the list of repositories
(enlistments?) from the users?

Re: [PATCH] maintenance: make unregister idempotent

[Derrick Stolee]

On 9/22/2022 3:31 PM, Junio C Hamano wrote:
> Derrick Stolee <derrickstolee@github.com> writes:
> 
>>> I am not sure if this is a good idea.  What is the ultimate reason
>>> why we want to allow running it blindly without knowing if it is
>>> necessary?  Is it because there is no easy way to tell if unregister
>>> is needed in the first place?
>>
>> We want to leave the internal details of what it means to be
>> registered as hidden to the user. They could look for the repo in
>> the global config, but that seems like a hassle when they just
>> want to make sure they are not currently registered. 
> 
> OK, so there is no published officially sanctioned way to ask "is
> this repository under maintenance's control and cron jobs run in
> it?" or "give me the list of such repositories".  
> 
> Then I can see why you want to allow users to blindly run
> "unregister", with or without "--force".
> 
> But doesn't it point at a more fundamental problem?  
> 
> Is there a reason why we want to hide the list of repositories
> (enlistments?) from the users?

I don't think we want to hide it, but we've never needed to present
the list in a canonical way before. It's been sufficient to let
users run 'git config --global --get-all maintenance.repo', assuming
they know that config key is the important one.

Adding a 'git maintenance list-registered' or something would solve
that problem, but I'm not sure it is a super high priority.

Thanks,
-Stolee

Re: [PATCH] maintenance: make unregister idempotent

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> Adding a 'git maintenance list-registered' or something would solve
> that problem, but I'm not sure it is a super high priority.

Is that "git maintenance" or "scalar" that list-registered would
hang below?

In any case, I tend to think the word "idempotent" is used as a
rough synonym to "sloppy", but unregistering from the automatic
maintenance (but still known as part of enlistment?) would probably
be a rare event that would not be a huge deal if the user failed to
do so without getting reminded, so I would not oppose to the step
[2/2] of the updated series.

Thanks, queued.

Re: [PATCH v2 1/2] maintenance: add 'unregister --force'

[SZEDER Gábor]

On Thu, Sep 22, 2022 at 01:37:16PM +0000, Derrick Stolee via GitGitGadget wrote:
>  static int maintenance_unregister(int argc, const char **argv, const char *prefix)
>  {
> +	int force = 0;
>  	struct option options[] = {
> +		OPT_BOOL(0, "force", &force,
> +			 N_("return success even if repository was not registered")),

This could be shortened a bit by using OPT__FORCE() instead of
OPT_BOOL().  OTOH, please make it a bit longer, and declare the option
with the PARSE_OPT_NOCOMPLETE flag to hide it from completion:
'--force' is usually used to enable something potentially dangerous,
and therefore should be used with care, so our completion script in
general doesn't offer it, giving the users more opportunity to think
things through while typing it out.  Though, arguably in this
particular case it seems there is not much danger to be afraid of.


> @@ -1538,11 +1545,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>  		usage_with_options(builtin_maintenance_unregister_usage,
>  				   options);
>  
> -	config_unset.git_cmd = 1;
> -	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
> -		     "--fixed-value", "maintenance.repo", maintpath, NULL);
> +	for_each_string_list_item(item, list) {
> +		if (!strcmp(maintpath, item->string)) {
> +			found = 1;
> +			break;
> +		}
> +	}
> +
> +	if (found) {
> +		config_unset.git_cmd = 1;
> +		strvec_pushl(&config_unset.args, "config", "--global", "--unset",
> +			     "--fixed-value", key, maintpath, NULL);
> +
> +		rc = run_command(&config_unset);

It seems a bit heavy-handed to fork() an extra git process just to
unset a config variable.  Wouldn't a properly parametrized
git_config_set_multivar_in_file_gently() call be sufficient?  Though
TBH I don't offhand know what those parameters should be, in
particular whether there is a convenient way to find out the path of
the global config file in order to pass it to this function.

(Not an issue with this patch, as the context shows that this has been
done with the extra process before; it just caught my eye.)

Re: [PATCH v2 1/2] maintenance: add 'unregister --force'

[Derrick Stolee]

On 9/23/2022 9:08 AM, SZEDER Gábor wrote:
> On Thu, Sep 22, 2022 at 01:37:16PM +0000, Derrick Stolee via GitGitGadget wrote:
>>  static int maintenance_unregister(int argc, const char **argv, const char *prefix)
>>  {
>> +	int force = 0;
>>  	struct option options[] = {
>> +		OPT_BOOL(0, "force", &force,
>> +			 N_("return success even if repository was not registered")),
> 
> This could be shortened a bit by using OPT__FORCE() instead of
> OPT_BOOL().  OTOH, please make it a bit longer, and declare the option
> with the PARSE_OPT_NOCOMPLETE flag to hide it from completion:

Looks like I can do both like this:

		OPT__FORCE(&force,
			   N_("return success even if repository was not registered"),
			   PARSE_OPT_NOCOMPLETE),

>> @@ -1538,11 +1545,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>>  		usage_with_options(builtin_maintenance_unregister_usage,
>>  				   options);
>>  
>> -	config_unset.git_cmd = 1;
>> -	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
>> -		     "--fixed-value", "maintenance.repo", maintpath, NULL);
>> +	for_each_string_list_item(item, list) {
>> +		if (!strcmp(maintpath, item->string)) {
>> +			found = 1;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (found) {
>> +		config_unset.git_cmd = 1;
>> +		strvec_pushl(&config_unset.args, "config", "--global", "--unset",
>> +			     "--fixed-value", key, maintpath, NULL);
>> +
>> +		rc = run_command(&config_unset);
> 
> It seems a bit heavy-handed to fork() an extra git process just to
> unset a config variable.  Wouldn't a properly parametrized
> git_config_set_multivar_in_file_gently() call be sufficient?  Though
> TBH I don't offhand know what those parameters should be, in
> particular whether there is a convenient way to find out the path of
> the global config file in order to pass it to this function.
> 
> (Not an issue with this patch, as the context shows that this has been
> done with the extra process before; it just caught my eye.)

Thanks. I'll look into it.

-Stolee

Re: [PATCH v2 1/2] maintenance: add 'unregister --force'

[Ævar Arnfjörð Bjarmason]

On Mon, Sep 26 2022, Derrick Stolee wrote:

> On 9/23/2022 9:08 AM, SZEDER Gábor wrote:
>> On Thu, Sep 22, 2022 at 01:37:16PM +0000, Derrick Stolee via GitGitGadget wrote:
>>>  static int maintenance_unregister(int argc, const char **argv, const char *prefix)
>>>  {
>>> +	int force = 0;
>>>  	struct option options[] = {
>>> +		OPT_BOOL(0, "force", &force,
>>> +			 N_("return success even if repository was not registered")),
>> 
>> This could be shortened a bit by using OPT__FORCE() instead of
>> OPT_BOOL().  OTOH, please make it a bit longer, and declare the option
>> with the PARSE_OPT_NOCOMPLETE flag to hide it from completion:
>
> Looks like I can do both like this:
>
> 		OPT__FORCE(&force,
> 			   N_("return success even if repository was not registered"),
> 			   PARSE_OPT_NOCOMPLETE),

I don't think PARSE_OPT_NOCOMPLETE is appropriate here. Yes we use it
for most of --force, but in some non-destructive cases (e.g. "add") we
don't.

This seems to be such a case, we'll destroy no data or do anything
irrecoverable. It's really just a
--do-not-be-so-anal-about-your-exit-code option.

I'm guessing that you wanted to be able to error check this more
strictly in some cases. For "git remote" I post-hoc filled in this
use-case by exiting with a code of 2 on missing remotes on e.g. "git
remote remove", see: 9144ba4cf52 (remote: add meaningful exit code on
missing/existing, 2020-10-27).

In this case we would return exit code 5 for this in most case before,
as we wouldn't be able to find the key, wouldn't we? I.e. the return
value from "git config". Seems so:
	
	$ GIT_TRACE=1 git maintenance unregister; echo $?
	17:48:59.984529 exec-cmd.c:90           trace: resolved executable path from procfs: /home/avar/local/bin/git
	17:48:59.984566 exec-cmd.c:237          trace: resolved executable dir: /home/avar/local/bin
	17:48:59.986795 git.c:509               trace: built-in: git maintenance unregister
	17:48:59.986817 run-command.c:654       trace: run_command: git config --global --unset --fixed-value maintenance.repo /home/avar/g/git
	17:48:59.987532 exec-cmd.c:90           trace: resolved executable path from procfs: /home/avar/local/bin/git
	17:48:59.987561 exec-cmd.c:237          trace: resolved executable dir: /home/avar/local/bin
	17:48:59.988733 git.c:509               trace: built-in: git config --global --unset --fixed-value maintenance.repo /home/avar/g/git
	5

Maybe we want to just define an exit code here too? I think doing so is
a better interface, the user can then pipe the STDERR to /dev/null
themselves (or equivalent).

Aside from anything else, I think this would be much clearer if it were
split up so that:

 * We first test what we do now without --force, which is clearly
   untested and undocumented (you are adding tests for it here
   while-at-it)

 * This commit, which adds a --force.

Also:

> @@ -1538,11 +1545,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>  		usage_with_options(builtin_maintenance_unregister_usage,
>  				   options);
>  
> -	config_unset.git_cmd = 1;
> -	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
> -		     "--fixed-value", "maintenance.repo", maintpath, NULL);
> +	for_each_string_list_item(item, list) {
> +		if (!strcmp(maintpath, item->string)) {
> +			found = 1;
> +			break;
> +		}
> +	}

This code now has a race condition it didn't before. Before we just did
a "git config --unset" which would have locked the config file, so if we
didn't have a key we'd return 5.

> +	if (found) {

But here we looked for the key *earlier*, so in that window we could
have raced and had the key again, so ....

> +		config_unset.git_cmd = 1;
> +		strvec_pushl(&config_unset.args, "config", "--global", "--unset",
> +			     "--fixed-value", key, maintpath, NULL);
> +
> +		rc = run_command(&config_unset);
> +	} else if (!force) {

...found would not be true, and if you you didn't have --force...

> +		die(_("repository '%s' is not registered"), maintpath);
> +	}
>  
> -	rc = run_command(&config_unset);

...this removal would cause us to still have the key in the end, no? I.e.:

 1. We check if the key is there
 2. Another process LOCKS config
 3. Another process SETS the key
 4. Another process UNLOCKS config
 5. We act with the assumption that the key isn't set

Maybe it's not racy, or it doesn't matter.

Re: [PATCH v2 1/2] maintenance: add 'unregister --force'

[Derrick Stolee]

On 9/26/2022 11:39 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Sep 26 2022, Derrick Stolee wrote:
> 
>> On 9/23/2022 9:08 AM, SZEDER Gábor wrote:
>>> On Thu, Sep 22, 2022 at 01:37:16PM +0000, Derrick Stolee via GitGitGadget wrote:
>>>>  static int maintenance_unregister(int argc, const char **argv, const char *prefix)
>>>>  {
>>>> +	int force = 0;
>>>>  	struct option options[] = {
>>>> +		OPT_BOOL(0, "force", &force,
>>>> +			 N_("return success even if repository was not registered")),
>>>
>>> This could be shortened a bit by using OPT__FORCE() instead of
>>> OPT_BOOL().  OTOH, please make it a bit longer, and declare the option
>>> with the PARSE_OPT_NOCOMPLETE flag to hide it from completion:
>>
>> Looks like I can do both like this:
>>
>> 		OPT__FORCE(&force,
>> 			   N_("return success even if repository was not registered"),
>> 			   PARSE_OPT_NOCOMPLETE),
> 
> I don't think PARSE_OPT_NOCOMPLETE is appropriate here. Yes we use it
> for most of --force, but in some non-destructive cases (e.g. "add") we
> don't.
> 
> This seems to be such a case, we'll destroy no data or do anything
> irrecoverable. It's really just a
> --do-not-be-so-anal-about-your-exit-code option.

I agree, so I wasn't completely sold on PARSE_OPT_NOCOMPLETE. I'll use
your vote to not add that option.

> I'm guessing that you wanted to be able to error check this more
> strictly in some cases. For "git remote" I post-hoc filled in this
> use-case by exiting with a code of 2 on missing remotes on e.g. "git
> remote remove", see: 9144ba4cf52 (remote: add meaningful exit code on
> missing/existing, 2020-10-27).

Generally, I'm not terribly interested in the exit code value other
than 0, 128, and <otherwise non-zero> being three categories. I definitely
don't want to create a strict list of exit code modes here.

> In this case we would return exit code 5 for this in most case before,
> as we wouldn't be able to find the key, wouldn't we? I.e. the return
> value from "git config".

We definitely inherited an exit code from 'git config' here, but
I should probably convert it into a die() message to make it clearer.

> This code now has a race condition it didn't before. Before we just did
> a "git config --unset" which would have locked the config file, so if we
> didn't have a key we'd return 5.
> 
>> +	if (found) {
> 
> But here we looked for the key *earlier*, so in that window we could
> have raced and had the key again, so ....
> 
>> +		config_unset.git_cmd = 1;
>> +		strvec_pushl(&config_unset.args, "config", "--global", "--unset",
>> +			     "--fixed-value", key, maintpath, NULL);
>> +
>> +		rc = run_command(&config_unset);
>> +	} else if (!force) {
> 
> ...found would not be true, and if you you didn't have --force...
> 
>> +		die(_("repository '%s' is not registered"), maintpath);
>> +	}
>>  
>> -	rc = run_command(&config_unset);
> 
> ...this removal would cause us to still have the key in the end, no? I.e.:
> 
>  1. We check if the key is there
>  2. Another process LOCKS config
>  3. Another process SETS the key
>  4. Another process UNLOCKS config>  5. We act with the assumption that the key isn't set

I think this list of events is fine, since we check the value and then
do not try to modify state if it isn't there.

Instead if we had this scenario:

 1. We check and see that it _is_there.
 2. Another process modifies config to remove the value.
 3. We try to remove the value and fail.

In this case, the --force option would still fail even though the end
result is the same. We could ignore a "not found" case during a --force
option to avoid failing due to such concurrency.
 
> Maybe it's not racy, or it doesn't matter.
Generally, I think in this case it doesn't matter, but we can be a bit
more careful about handling races.

Thanks,
-Stolee

[PATCH v3 0/3] scalar: make unregister idempotent

[Derrick Stolee via GitGitGadget]

I noticed this while we were updating the microsoft/git fork to include
v2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent
before, but instead some change in 'scalar unregister' led to it relying on
the return code of 'git maintenance unregister'. Our functional tests expect
'scalar unregister' to be idempotent, and I think that's a good pattern for
'git maintenance unregister', so I'm fixing it at that layer.

Despite finding this during the 2.38.0-rc0 integration, this isn't critical
to the release.

Perhaps an argument could be made that "failure means it wasn't registered
before", but I think that isn't terribly helpful.

Our functional tests are running the unregister subcommand to disable
maintenance in order to run tests on the object store (such as running
maintenance commands in the foreground and checking the object store
afterwards). This is a form of automation using 'unregister' as a check that
maintenance will not run at the same time, and it doesn't care if
maintenance was already disabled. I can imagine other scripting scenarios
wanting that kind of guarantee.


Updates in v3
=============

 * The --force option now uses OPT_FORCE and is hidden from autocomplete.
 * A new commit is added that removes the use of Git subprocesses in favor
   of git_config_set_multivar_in_file_gently().


Updates in v2
=============

 * This is now a two-patch series.
 * I rebased onto v2.38.0-rc1 for two reasons: Scalar is now merged, and the
   usage for 'git maintenance unregister' removed its translation markers.
 * Instead of making git maintenance unregister idempotent, add a --force
   option for those who do not want to require that the repository is
   already registered.
 * Make scalar unregister idempotent, with reasons argued in patch 2.

Thanks, -Stolee

Derrick Stolee (3):
  maintenance: add 'unregister --force'
  scalar: make 'unregister' idempotent
  gc: replace config subprocesses with API calls

 Documentation/git-maintenance.txt |  6 +-
 builtin/gc.c                      | 95 +++++++++++++++++++++----------
 scalar.c                          |  5 +-
 t/t7900-maintenance.sh            |  6 +-
 t/t9210-scalar.sh                 |  5 +-
 5 files changed, 82 insertions(+), 35 deletions(-)


base-commit: 1b3d6e17fe83eb6f79ffbac2f2c61bbf1eaef5f8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1358%2Fderrickstolee%2Fmaintenance-unregister-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1358/derrickstolee/maintenance-unregister-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1358

Range-diff vs v2:

 1:  69c74f52eef ! 1:  8a8bffaec89 maintenance: add 'unregister --force'
     @@ builtin/gc.c: done:
       {
      +	int force = 0;
       	struct option options[] = {
     -+		OPT_BOOL(0, "force", &force,
     -+			 N_("return success even if repository was not registered")),
     ++		OPT__FORCE(&force,
     ++			   N_("return success even if repository was not registered"),
     ++			   PARSE_OPT_NOCOMPLETE),
       		OPT_END(),
       	};
      -	int rc;
 2:  f5d8d6e4901 = 2:  06d5ef3fc57 scalar: make 'unregister' idempotent
 -:  ----------- > 3:  260d7bee36e gc: replace config subprocesses with API calls

-- 
gitgitgadget

[PATCH v3 1/3] maintenance: add 'unregister --force'

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The 'git maintenance unregister' subcommand has a step that removes the
current repository from the multi-valued maitenance.repo config key.
This fails if the repository is not listed in that key. This makes
running 'git maintenance unregister' twice result in a failure in the
second instance.

This failure exit code is helpful, but its message is not. Add a new
die() message that explicitly calls out the failure due to the
repository not being registered.

In some cases, users may want to run 'git maintenance unregister' just
to make sure that background jobs will not start on this repository, but
they do not want to check to see if it is registered first. Add a new
'--force' option that will siltently succeed if the repository is not
already registered.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 Documentation/git-maintenance.txt |  6 +++++-
 builtin/gc.c                      | 32 +++++++++++++++++++++++++------
 t/t7900-maintenance.sh            |  6 +++++-
 3 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
index 9c630efe19c..bb888690e4d 100644
--- a/Documentation/git-maintenance.txt
+++ b/Documentation/git-maintenance.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git maintenance' run [<options>]
 'git maintenance' start [--scheduler=<scheduler>]
-'git maintenance' (stop|register|unregister)
+'git maintenance' (stop|register|unregister) [<options>]
 
 
 DESCRIPTION
@@ -79,6 +79,10 @@ unregister::
 	Remove the current repository from background maintenance. This
 	only removes the repository from the configured list. It does not
 	stop the background maintenance processes from running.
++
+The `unregister` subcommand will report an error if the current repository
+is not already registered. Use the `--force` option to return success even
+when the current repository is not registered.
 
 TASKS
 -----
diff --git a/builtin/gc.c b/builtin/gc.c
index 2753bd15a5e..8d154586272 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1519,18 +1519,26 @@ done:
 }
 
 static char const * const builtin_maintenance_unregister_usage[] = {
-	"git maintenance unregister",
+	"git maintenance unregister [--force]",
 	NULL
 };
 
 static int maintenance_unregister(int argc, const char **argv, const char *prefix)
 {
+	int force = 0;
 	struct option options[] = {
+		OPT__FORCE(&force,
+			   N_("return success even if repository was not registered"),
+			   PARSE_OPT_NOCOMPLETE),
 		OPT_END(),
 	};
-	int rc;
+	const char *key = "maintenance.repo";
+	int rc = 0;
 	struct child_process config_unset = CHILD_PROCESS_INIT;
 	char *maintpath = get_maintpath();
+	int found = 0;
+	struct string_list_item *item;
+	const struct string_list *list = git_config_get_value_multi(key);
 
 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_maintenance_unregister_usage, 0);
@@ -1538,11 +1546,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 		usage_with_options(builtin_maintenance_unregister_usage,
 				   options);
 
-	config_unset.git_cmd = 1;
-	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
-		     "--fixed-value", "maintenance.repo", maintpath, NULL);
+	for_each_string_list_item(item, list) {
+		if (!strcmp(maintpath, item->string)) {
+			found = 1;
+			break;
+		}
+	}
+
+	if (found) {
+		config_unset.git_cmd = 1;
+		strvec_pushl(&config_unset.args, "config", "--global", "--unset",
+			     "--fixed-value", key, maintpath, NULL);
+
+		rc = run_command(&config_unset);
+	} else if (!force) {
+		die(_("repository '%s' is not registered"), maintpath);
+	}
 
-	rc = run_command(&config_unset);
 	free(maintpath);
 	return rc;
 }
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 2724a44fe3e..cfe3236567a 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -493,7 +493,11 @@ test_expect_success 'register and unregister' '
 
 	git maintenance unregister &&
 	git config --global --get-all maintenance.repo >actual &&
-	test_cmp before actual
+	test_cmp before actual &&
+
+	test_must_fail git maintenance unregister 2>err &&
+	grep "is not registered" err &&
+	git maintenance unregister --force
 '
 
 test_expect_success !MINGW 'register and unregister with regex metacharacters' '
-- 
gitgitgadget

[PATCH v3 2/3] scalar: make 'unregister' idempotent

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The 'scalar unregister' command removes a repository from the list of
registered Scalar repositories and removes it from the list of
repositories registered for background maintenance. If the repository
was not already registered for background maintenance, then the command
fails, even if the repository was still registered as a Scalar
repository.

After using 'scalar clone' or 'scalar register', the repository would be
enrolled in background maintenance since those commands run 'git
maintenance start'. If the user runs 'git maintenance unregister' on
that repository, then it is still in the list of repositories which get
new config updates from 'scalar reconfigure'. The 'scalar unregister'
command would fail since 'git maintenance unregister' would fail.

Further, the add_or_remove_enlistment() method in scalar.c already has
this idempotent nature built in as an expectation since it returns zero
when the scalar.repo list already has the proper containment of the
repository.

The previous change added the 'git maintenance unregister --force'
option, so use it within 'scalar unregister' to make it idempotent.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 scalar.c          | 5 ++++-
 t/t9210-scalar.sh | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/scalar.c b/scalar.c
index c5c1ce68919..6de9c0ee523 100644
--- a/scalar.c
+++ b/scalar.c
@@ -207,7 +207,10 @@ static int set_recommended_config(int reconfigure)
 
 static int toggle_maintenance(int enable)
 {
-	return run_git("maintenance", enable ? "start" : "unregister", NULL);
+	return run_git("maintenance",
+		       enable ? "start" : "unregister",
+		       enable ? NULL : "--force",
+		       NULL);
 }
 
 static int add_or_remove_enlistment(int add)
diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
index 14ca575a214..be51a8bb7a4 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -116,7 +116,10 @@ test_expect_success 'scalar unregister' '
 	test_must_fail git config --get --global --fixed-value \
 		maintenance.repo "$(pwd)/vanish/src" &&
 	scalar list >scalar.repos &&
-	! grep -F "$(pwd)/vanish/src" scalar.repos
+	! grep -F "$(pwd)/vanish/src" scalar.repos &&
+
+	# scalar unregister should be idempotent
+	scalar unregister vanish
 '
 
 test_expect_success 'set up repository to clone' '
-- 
gitgitgadget

[PATCH v3 3/3] gc: replace config subprocesses with API calls

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The 'git maintenance [un]register' commands set or unset the multi-
valued maintenance.repo config key with the absolute path of the current
repository. These are set in the global config file.

Instead of calling a subcommand and creating a new process, create the
proper API calls to git_config_set_multivar_in_file_gently(). It
requires loading the filename for the global config file (and erroring
out if now $HOME value is set). We also need to be careful about using
CONFIG_REGEX_NONE when adding the value and using
CONFIG_FLAGS_FIXED_VALUE when removing the value. In both cases, we
check that the value already exists (this check already existed for
'unregister').

Also, remove the transparent translation of the error code from the
config API to the exit code of 'git maintenance'. Instead, use die() to
recover from failures at that level. In the case of 'unregister
--force', allow the CONFIG_NOTHING_SET error code to be a success. This
allows a possible race where another process removes the config value.
The end result is that the config value is not set anymore, so we can
treat this as a success.

Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 builtin/gc.c | 75 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 31 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 8d154586272..4c59235950d 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1470,11 +1470,12 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_END(),
 	};
-	int rc;
+	int found = 0;
+	const char *key = "maintenance.repo";
 	char *config_value;
-	struct child_process config_set = CHILD_PROCESS_INIT;
-	struct child_process config_get = CHILD_PROCESS_INIT;
 	char *maintpath = get_maintpath();
+	struct string_list_item *item;
+	const struct string_list *list;
 
 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_maintenance_register_usage, 0);
@@ -1491,31 +1492,35 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
 	else
 		git_config_set("maintenance.strategy", "incremental");
 
-	config_get.git_cmd = 1;
-	strvec_pushl(&config_get.args, "config", "--global", "--get",
-		     "--fixed-value", "maintenance.repo", maintpath, NULL);
-	config_get.out = -1;
-
-	if (start_command(&config_get)) {
-		rc = error(_("failed to run 'git config'"));
-		goto done;
+	list = git_config_get_value_multi(key);
+	if (list) {
+		for_each_string_list_item(item, list) {
+			if (!strcmp(maintpath, item->string)) {
+				found = 1;
+				break;
+			}
+		}
 	}
 
-	/* We already have this value in our config! */
-	if (!finish_command(&config_get)) {
-		rc = 0;
-		goto done;
+	if (!found) {
+		int rc;
+		char *user_config, *xdg_config;
+		git_global_config(&user_config, &xdg_config);
+		if (!user_config)
+			die(_("$HOME not set"));
+		rc = git_config_set_multivar_in_file_gently(
+			user_config, "maintenance.repo", maintpath,
+			CONFIG_REGEX_NONE, 0);
+		free(user_config);
+		free(xdg_config);
+
+		if (rc)
+			die(_("unable to add '%s' value of '%s'"),
+			    key, maintpath);
 	}
 
-	config_set.git_cmd = 1;
-	strvec_pushl(&config_set.args, "config", "--add", "--global", "maintenance.repo",
-		     maintpath, NULL);
-
-	rc = run_command(&config_set);
-
-done:
 	free(maintpath);
-	return rc;
+	return 0;
 }
 
 static char const * const builtin_maintenance_unregister_usage[] = {
@@ -1533,8 +1538,6 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 		OPT_END(),
 	};
 	const char *key = "maintenance.repo";
-	int rc = 0;
-	struct child_process config_unset = CHILD_PROCESS_INIT;
 	char *maintpath = get_maintpath();
 	int found = 0;
 	struct string_list_item *item;
@@ -1554,17 +1557,27 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 	}
 
 	if (found) {
-		config_unset.git_cmd = 1;
-		strvec_pushl(&config_unset.args, "config", "--global", "--unset",
-			     "--fixed-value", key, maintpath, NULL);
-
-		rc = run_command(&config_unset);
+		int rc;
+		char *user_config, *xdg_config;
+		git_global_config(&user_config, &xdg_config);
+		if (!user_config)
+			die(_("$HOME not set"));
+		rc = git_config_set_multivar_in_file_gently(
+			user_config, key, NULL, maintpath,
+			CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);
+		free(user_config);
+		free(xdg_config);
+
+		if (rc &&
+		    (!force || rc == CONFIG_NOTHING_SET))
+			die(_("unable to unset '%s' value of '%s'"),
+			    key, maintpath);
 	} else if (!force) {
 		die(_("repository '%s' is not registered"), maintpath);
 	}
 
 	free(maintpath);
-	return rc;
+	return 0;
 }
 
 static const char *get_frequency(enum schedule_priority schedule)
-- 
gitgitgadget

Re: [PATCH v2 1/2] maintenance: add 'unregister --force'

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> On 9/26/2022 11:39 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Mon, Sep 26 2022, Derrick Stolee wrote:
>> 
>>> On 9/23/2022 9:08 AM, SZEDER Gábor wrote:
>>>> On Thu, Sep 22, 2022 at 01:37:16PM +0000, Derrick Stolee via GitGitGadget wrote:
>>>>>  static int maintenance_unregister(int argc, const char **argv, const char *prefix)
>>>>>  {
>>>>> +	int force = 0;
>>>>>  	struct option options[] = {
>>>>> +		OPT_BOOL(0, "force", &force,
>>>>> +			 N_("return success even if repository was not registered")),
>>>>
>>>> This could be shortened a bit by using OPT__FORCE() instead of
>>>> OPT_BOOL().  OTOH, please make it a bit longer, and declare the option
>>>> with the PARSE_OPT_NOCOMPLETE flag to hide it from completion:
>>>
>>> Looks like I can do both like this:
>>>
>>> 		OPT__FORCE(&force,
>>> 			   N_("return success even if repository was not registered"),
>>> 			   PARSE_OPT_NOCOMPLETE),
>> 
>> I don't think PARSE_OPT_NOCOMPLETE is appropriate here. Yes we use it
>> for most of --force, but in some non-destructive cases (e.g. "add") we
>> don't.
>> 
>> This seems to be such a case, we'll destroy no data or do anything
>> irrecoverable. It's really just a
>> --do-not-be-so-anal-about-your-exit-code option.
>
> I agree, so I wasn't completely sold on PARSE_OPT_NOCOMPLETE. I'll use
> your vote to not add that option.

I am perfectly OK with that.  Given that "git reset --hard" is not
given nocomplete option, I do not see much point in "destructive
ones are not completed" guideline in practice anyway.  After all,
"add --force" would be destructively removing the object name
recorded for the path previously.

Thanks for carefully thinking the UI remifications through.

Re: [PATCH v3 1/3] maintenance: add 'unregister --force'

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> @@ -11,7 +11,7 @@ SYNOPSIS
>  [verse]
>  'git maintenance' run [<options>]
>  'git maintenance' start [--scheduler=<scheduler>]
> -'git maintenance' (stop|register|unregister)
> +'git maintenance' (stop|register|unregister) [<options>]

An unrelated tangent, but should register complain when given in a
repository that is already registered as well?  Just being curious.

Re: [PATCH v3 3/3] gc: replace config subprocesses with API calls

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <derrickstolee@github.com>
>
> The 'git maintenance [un]register' commands set or unset the multi-
> valued maintenance.repo config key with the absolute path of the current
> repository. These are set in the global config file.

OK.  This step is new but it looks reasonable.  

Embarrassingly sadly, we open, rewrite, and close the configuration
file for each of these "proper API calls", so the IO load is not
reduced, even though we do not have to spawn extra processes ;-)

All three patches queued.  Thanks.

Re: [PATCH v3 1/3] maintenance: add 'unregister --force'

[Derrick Stolee]

On 9/26/2022 3:23 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> @@ -11,7 +11,7 @@ SYNOPSIS
>>  [verse]
>>  'git maintenance' run [<options>]
>>  'git maintenance' start [--scheduler=<scheduler>]
>> -'git maintenance' (stop|register|unregister)
>> +'git maintenance' (stop|register|unregister) [<options>]
> 
> An unrelated tangent, but should register complain when given in a
> repository that is already registered as well?  Just being curious.

Let's leave that as a #leftoverbits and perhaps as something to
consider next to something like 'git maintenance list' to list the
currently-registered repositories.

Thanks,
-Stolee

Re: [PATCH v3 1/3] maintenance: add 'unregister --force'

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> @@ -1538,11 +1546,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>  		usage_with_options(builtin_maintenance_unregister_usage,
>  				   options);
>  
> -	config_unset.git_cmd = 1;
> -	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
> -		     "--fixed-value", "maintenance.repo", maintpath, NULL);
> +	for_each_string_list_item(item, list) {
> +		if (!strcmp(maintpath, item->string)) {
> +			found = 1;
> +			break;
> +		}
> +	}

Just out of curiosity, I ran this in a fresh repository and got a
segfault.  An attached patch obviously fixes it, but I am wondering
if a better "fix" is to teach for_each_string_list_item() that it is
perfectly reasonable to see a NULL passed to it as the list, which
is a mere special case that the caller has a string list with 0
items on it.

Thoughts?

Thanks.



diff --git a/builtin/gc.c b/builtin/gc.c
index 4c59235950..67f41fad4b 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1549,6 +1549,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 		usage_with_options(builtin_maintenance_unregister_usage,
 				   options);
 
+	if (list)
 	for_each_string_list_item(item, list) {
 		if (!strcmp(maintpath, item->string)) {
 			found = 1;
-- 
2.38.0-rc1-123-g02867222b8

Re: [PATCH v3 1/3] maintenance: add 'unregister --force'

[Derrick Stolee]

On 9/26/2022 5:55 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> @@ -1538,11 +1546,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>>  		usage_with_options(builtin_maintenance_unregister_usage,
>>  				   options);
>>  
>> -	config_unset.git_cmd = 1;
>> -	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
>> -		     "--fixed-value", "maintenance.repo", maintpath, NULL);
>> +	for_each_string_list_item(item, list) {
>> +		if (!strcmp(maintpath, item->string)) {
>> +			found = 1;
>> +			break;
>> +		}
>> +	}
> 
> Just out of curiosity, I ran this in a fresh repository and got a
> segfault.

Yikes! Thanks for catching. I think there was another instance in
the 'register' code that I caught by tests, but I appreciate you
catching this one.

>  An attached patch obviously fixes it, but I am wondering
> if a better "fix" is to teach for_each_string_list_item() that it is
> perfectly reasonable to see a NULL passed to it as the list, which
> is a mere special case that the caller has a string list with 0
> items on it.
> 
> Thoughts?

I agree that for_each_string_list_item() could handle NULL lists
better, especially because it looks like a method and hides some
details. Plus, wrapping the for-ish loop with an if statement is
particularly ugly.

I think this might be more confusing that
git_configset_get_value_multi() can return a NULL list instead of
an empty list. It boils down to this diff:

diff --git a/config.c b/config.c
index cbb5a3bab74..39cc0534170 100644
--- a/config.c
+++ b/config.c
@@ -2416,8 +2416,9 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char *
 
 const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)
 {
+	static struct string_list empty_list = STRING_LIST_INIT_NODUP;
 	struct config_set_element *e = configset_find_element(cs, key);
-	return e ? &e->value_list : NULL;
+	return e ? &e->value_list : &empty_list;
 }
 
 int git_configset_get_string(struct config_set *cs, const char *key, char **dest)

However, this change causes a lot of cascading failures that
are probably not worth tracking down.

I'll get a patch put together that changes the behavior of
for_each_string_list_item() and adds the missing 'unregister' test
so we can avoid this problem.

Thanks,
-Stolee

Re: [PATCH v3 1/3] maintenance: add 'unregister --force'

[Derrick Stolee]

On 9/27/2022 7:38 AM, Derrick Stolee wrote:
> On 9/26/2022 5:55 PM, Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> @@ -1538,11 +1546,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>>>  		usage_with_options(builtin_maintenance_unregister_usage,
>>>  				   options);
>>>  
>>> -	config_unset.git_cmd = 1;
>>> -	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
>>> -		     "--fixed-value", "maintenance.repo", maintpath, NULL);
>>> +	for_each_string_list_item(item, list) {
>>> +		if (!strcmp(maintpath, item->string)) {
>>> +			found = 1;
>>> +			break;
>>> +		}
>>> +	}
>>
>> Just out of curiosity, I ran this in a fresh repository and got a
>> segfault.
> 
> Yikes! Thanks for catching. I think there was another instance in
> the 'register' code that I caught by tests, but I appreciate you
> catching this one.
> 
>>  An attached patch obviously fixes it, but I am wondering
>> if a better "fix" is to teach for_each_string_list_item() that it is
>> perfectly reasonable to see a NULL passed to it as the list, which
>> is a mere special case that the caller has a string list with 0
>> items on it.
>>
>> Thoughts?
> 
> I agree that for_each_string_list_item() could handle NULL lists
> better, especially because it looks like a method and hides some
> details. Plus, wrapping the for-ish loop with an if statement is
> particularly ugly.
...
> I'll get a patch put together that changes the behavior of
> for_each_string_list_item() and adds the missing 'unregister' test
> so we can avoid this problem.

Of course, there is a reason why we don't check for NULL here,
and it's because -Werror=address complains when we use a non-pointer
value in the macro:

string-list.h:146:28: error: the address of ‘friendly_ref_names’ will always evaluate as ‘true’ [-Werror=address]
  146 |         for (item = (list) ? (list)->items : NULL;      \
      |

I tried searching for a way to suppress this error in a particular
case like this (perhaps using something like an attribute?), but I
couldn't find anything.

Thanks,
-Stolee

Re: [PATCH v3 1/3] maintenance: add 'unregister --force'

[Ævar Arnfjörð Bjarmason]

On Tue, Sep 27 2022, Derrick Stolee wrote:

> On 9/27/2022 7:38 AM, Derrick Stolee wrote:
>> On 9/26/2022 5:55 PM, Junio C Hamano wrote:
>>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>
>>>> @@ -1538,11 +1546,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>>>>  		usage_with_options(builtin_maintenance_unregister_usage,
>>>>  				   options);
>>>>  
>>>> -	config_unset.git_cmd = 1;
>>>> -	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
>>>> -		     "--fixed-value", "maintenance.repo", maintpath, NULL);
>>>> +	for_each_string_list_item(item, list) {
>>>> +		if (!strcmp(maintpath, item->string)) {
>>>> +			found = 1;
>>>> +			break;
>>>> +		}
>>>> +	}
>>>
>>> Just out of curiosity, I ran this in a fresh repository and got a
>>> segfault.
>> 
>> Yikes! Thanks for catching. I think there was another instance in
>> the 'register' code that I caught by tests, but I appreciate you
>> catching this one.
>> 
>>>  An attached patch obviously fixes it, but I am wondering
>>> if a better "fix" is to teach for_each_string_list_item() that it is
>>> perfectly reasonable to see a NULL passed to it as the list, which
>>> is a mere special case that the caller has a string list with 0
>>> items on it.
>>>
>>> Thoughts?
>> 
>> I agree that for_each_string_list_item() could handle NULL lists
>> better, especially because it looks like a method and hides some
>> details. Plus, wrapping the for-ish loop with an if statement is
>> particularly ugly.
> ...
>> I'll get a patch put together that changes the behavior of
>> for_each_string_list_item() and adds the missing 'unregister' test
>> so we can avoid this problem.
>
> Of course, there is a reason why we don't check for NULL here,
> and it's because -Werror=address complains when we use a non-pointer
> value in the macro:
>
> string-list.h:146:28: error: the address of ‘friendly_ref_names’ will always evaluate as ‘true’ [-Werror=address]
>   146 |         for (item = (list) ? (list)->items : NULL;      \
>       |
>
> I tried searching for a way to suppress this error in a particular
> case like this (perhaps using something like an attribute?), but I
> couldn't find anything.

We discussed this exact issue just a few months ago, see:
https://lore.kernel.org/git/220614.86czfcytlz.gmgdl@evledraar.gmail.com/

In general I don't think we should be teaching
for_each_string_list_item() to handle NULL.

Instead most callers that need to deal with a "NULL" list should
probably just use a list that's never NULL. See:
https://lore.kernel.org/git/220616.86bkuswuh5.gmgdl@evledraar.gmail.com/

In this case however it seems perfectly reasonable to return a valid
pointer or NULL, and the function documents as much:
	
	/**
	 * Finds and returns the value list, sorted in order of increasing priority
	 * for the configuration variable `key`. When the configuration variable
	 * `key` is not found, returns NULL. The caller should not free or modify
	 * the returned pointer, as it is owned by the cache.
	 */
	const struct string_list *git_config_get_value_multi(const char *key);

You also have code in 3/3 that uses that API in the correct way, I think
just adjusting this callsite in 1/3 would be the right move here.

This also gives the reader & compiler more information to e.g. eliminate
dead code. You're calling maintpath() unconditionally, but if you have
no config & the user provided --force we'll never end up using it, so we
can avoid allocating it in the first place.

Re: [PATCH v3 1/3] maintenance: add 'unregister --force'

[Derrick Stolee]

On 9/27/2022 9:36 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Sep 27 2022, Derrick Stolee wrote:
>> Of course, there is a reason why we don't check for NULL here,
>> and it's because -Werror=address complains when we use a non-pointer
>> value in the macro:
>>
>> string-list.h:146:28: error: the address of ‘friendly_ref_names’ will always evaluate as ‘true’ [-Werror=address]
>>   146 |         for (item = (list) ? (list)->items : NULL;      \
>>       |
>>
>> I tried searching for a way to suppress this error in a particular
>> case like this (perhaps using something like an attribute?), but I
>> couldn't find anything.
> 
> We discussed this exact issue just a few months ago, see:
> https://lore.kernel.org/git/220614.86czfcytlz.gmgdl@evledraar.gmail.com/

Thanks for finding this thread. I knew it was vaguely familiar.
 
> In general I don't think we should be teaching
> for_each_string_list_item() to handle NULL.
> 
> Instead most callers that need to deal with a "NULL" list should
> probably just use a list that's never NULL. See:
> https://lore.kernel.org/git/220616.86bkuswuh5.gmgdl@evledraar.gmail.com/
> 
> In this case however it seems perfectly reasonable to return a valid
> pointer or NULL, and the function documents as much:
> 	
> 	/**
> 	 * Finds and returns the value list, sorted in order of increasing priority
> 	 * for the configuration variable `key`. When the configuration variable
> 	 * `key` is not found, returns NULL. The caller should not free or modify
> 	 * the returned pointer, as it is owned by the cache.
> 	 */
> 	const struct string_list *git_config_get_value_multi(const char *key);

It documents that it will never return an empty list, and instead will
return NULL. There are several places that check that condition explicitly.
Converting them is not terribly hard, though, and I'll send an RFC soon
that performs that conversion.

> This also gives the reader & compiler more information to e.g. eliminate
> dead code. You're calling maintpath() unconditionally, but if you have
> no config & the user provided --force we'll never end up using it, so we
> can avoid allocating it in the first place.

While you're correct that we could avoid that allocation, it makes the
code look terrible and hard to reason about, so I won't make that change.

Thanks,
-Stolee
  

[PATCH v4 0/4] scalar: make unregister idempotent

[Derrick Stolee via GitGitGadget]

I noticed this while we were updating the microsoft/git fork to include
v2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent
before, but instead some change in 'scalar unregister' led to it relying on
the return code of 'git maintenance unregister'. Our functional tests expect
'scalar unregister' to be idempotent, and I think that's a good pattern for
'git maintenance unregister', so I'm fixing it at that layer.

Despite finding this during the 2.38.0-rc0 integration, this isn't critical
to the release.

Perhaps an argument could be made that "failure means it wasn't registered
before", but I think that isn't terribly helpful.

Our functional tests are running the unregister subcommand to disable
maintenance in order to run tests on the object store (such as running
maintenance commands in the foreground and checking the object store
afterwards). This is a form of automation using 'unregister' as a check that
maintenance will not run at the same time, and it doesn't care if
maintenance was already disabled. I can imagine other scripting scenarios
wanting that kind of guarantee.


Updates in v4
=============

 * The previous version would segfault if 'git maintenance unregister' was
   called with an empty 'maintenance.repo' config list. This scenario is
   fixed and tested.
 * Part of the issue is that for_each_string_list_item() cannot handle a
   NULL value. The macro can't be made smarter without failing
   -Werror=address issues, so for now I added a patch that adds a warning to
   its doc comment.


Updates in v3
=============

 * The --force option now uses OPT_FORCE and is hidden from autocomplete.
 * A new commit is added that removes the use of Git subprocesses in favor
   of git_config_set_multivar_in_file_gently().


Updates in v2
=============

 * This is now a two-patch series.
 * I rebased onto v2.38.0-rc1 for two reasons: Scalar is now merged, and the
   usage for 'git maintenance unregister' removed its translation markers.
 * Instead of making git maintenance unregister idempotent, add a --force
   option for those who do not want to require that the repository is
   already registered.
 * Make scalar unregister idempotent, with reasons argued in patch 2.

Thanks, -Stolee

Derrick Stolee (4):
  maintenance: add 'unregister --force'
  scalar: make 'unregister' idempotent
  gc: replace config subprocesses with API calls
  string-list: document iterator behavior on NULL input

 Documentation/git-maintenance.txt |  6 +-
 builtin/gc.c                      | 98 +++++++++++++++++++++----------
 scalar.c                          |  5 +-
 string-list.h                     |  7 ++-
 t/t7900-maintenance.sh            | 11 +++-
 t/t9210-scalar.sh                 |  5 +-
 6 files changed, 96 insertions(+), 36 deletions(-)


base-commit: 1b3d6e17fe83eb6f79ffbac2f2c61bbf1eaef5f8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1358%2Fderrickstolee%2Fmaintenance-unregister-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1358/derrickstolee/maintenance-unregister-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1358

Range-diff vs v3:

 1:  8a8bffaec89 ! 1:  c3301e21109 maintenance: add 'unregister --force'
     @@ Commit message
          '--force' option that will siltently succeed if the repository is not
          already registered.
      
     +    Also add an extra test of 'git maintenance unregister' at a point where
     +    there are no registered repositories. This should fail without --force.
     +
          Signed-off-by: Derrick Stolee <derrickstolee@github.com>
      
       ## Documentation/git-maintenance.txt ##
     @@ builtin/gc.c: done:
       	char *maintpath = get_maintpath();
      +	int found = 0;
      +	struct string_list_item *item;
     -+	const struct string_list *list = git_config_get_value_multi(key);
     ++	const struct string_list *list;
       
       	argc = parse_options(argc, argv, prefix, options,
       			     builtin_maintenance_unregister_usage, 0);
     @@ builtin/gc.c: static int maintenance_unregister(int argc, const char **argv, con
      -	config_unset.git_cmd = 1;
      -	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
      -		     "--fixed-value", "maintenance.repo", maintpath, NULL);
     -+	for_each_string_list_item(item, list) {
     -+		if (!strcmp(maintpath, item->string)) {
     -+			found = 1;
     -+			break;
     ++	list = git_config_get_value_multi(key);
     ++	if (list) {
     ++		for_each_string_list_item(item, list) {
     ++			if (!strcmp(maintpath, item->string)) {
     ++				found = 1;
     ++				break;
     ++			}
      +		}
      +	}
      +
     @@ builtin/gc.c: static int maintenance_unregister(int argc, const char **argv, con
       }
      
       ## t/t7900-maintenance.sh ##
     +@@ t/t7900-maintenance.sh: test_expect_success 'maintenance.strategy inheritance' '
     + 
     + test_expect_success 'register and unregister' '
     + 	test_when_finished git config --global --unset-all maintenance.repo &&
     ++
     ++	test_must_fail git maintenance unregister 2>err &&
     ++	grep "is not registered" err &&
     ++	git maintenance unregister --force &&
     ++
     + 	git config --global --add maintenance.repo /existing1 &&
     + 	git config --global --add maintenance.repo /existing2 &&
     + 	git config --global --get-all maintenance.repo >before &&
      @@ t/t7900-maintenance.sh: test_expect_success 'register and unregister' '
       
       	git maintenance unregister &&
 2:  06d5ef3fc57 = 2:  a768c326c0f scalar: make 'unregister' idempotent
 3:  260d7bee36e = 3:  5aa9cc1d6b9 gc: replace config subprocesses with API calls
 -:  ----------- > 4:  73a262cdca4 string-list: document iterator behavior on NULL input

-- 
gitgitgadget

[PATCH v4 1/4] maintenance: add 'unregister --force'

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The 'git maintenance unregister' subcommand has a step that removes the
current repository from the multi-valued maitenance.repo config key.
This fails if the repository is not listed in that key. This makes
running 'git maintenance unregister' twice result in a failure in the
second instance.

This failure exit code is helpful, but its message is not. Add a new
die() message that explicitly calls out the failure due to the
repository not being registered.

In some cases, users may want to run 'git maintenance unregister' just
to make sure that background jobs will not start on this repository, but
they do not want to check to see if it is registered first. Add a new
'--force' option that will siltently succeed if the repository is not
already registered.

Also add an extra test of 'git maintenance unregister' at a point where
there are no registered repositories. This should fail without --force.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 Documentation/git-maintenance.txt |  6 +++++-
 builtin/gc.c                      | 35 +++++++++++++++++++++++++------
 t/t7900-maintenance.sh            | 11 +++++++++-
 3 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
index 9c630efe19c..bb888690e4d 100644
--- a/Documentation/git-maintenance.txt
+++ b/Documentation/git-maintenance.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git maintenance' run [<options>]
 'git maintenance' start [--scheduler=<scheduler>]
-'git maintenance' (stop|register|unregister)
+'git maintenance' (stop|register|unregister) [<options>]
 
 
 DESCRIPTION
@@ -79,6 +79,10 @@ unregister::
 	Remove the current repository from background maintenance. This
 	only removes the repository from the configured list. It does not
 	stop the background maintenance processes from running.
++
+The `unregister` subcommand will report an error if the current repository
+is not already registered. Use the `--force` option to return success even
+when the current repository is not registered.
 
 TASKS
 -----
diff --git a/builtin/gc.c b/builtin/gc.c
index 2753bd15a5e..dc0ba9e3648 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1519,18 +1519,26 @@ done:
 }
 
 static char const * const builtin_maintenance_unregister_usage[] = {
-	"git maintenance unregister",
+	"git maintenance unregister [--force]",
 	NULL
 };
 
 static int maintenance_unregister(int argc, const char **argv, const char *prefix)
 {
+	int force = 0;
 	struct option options[] = {
+		OPT__FORCE(&force,
+			   N_("return success even if repository was not registered"),
+			   PARSE_OPT_NOCOMPLETE),
 		OPT_END(),
 	};
-	int rc;
+	const char *key = "maintenance.repo";
+	int rc = 0;
 	struct child_process config_unset = CHILD_PROCESS_INIT;
 	char *maintpath = get_maintpath();
+	int found = 0;
+	struct string_list_item *item;
+	const struct string_list *list;
 
 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_maintenance_unregister_usage, 0);
@@ -1538,11 +1546,26 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 		usage_with_options(builtin_maintenance_unregister_usage,
 				   options);
 
-	config_unset.git_cmd = 1;
-	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
-		     "--fixed-value", "maintenance.repo", maintpath, NULL);
+	list = git_config_get_value_multi(key);
+	if (list) {
+		for_each_string_list_item(item, list) {
+			if (!strcmp(maintpath, item->string)) {
+				found = 1;
+				break;
+			}
+		}
+	}
+
+	if (found) {
+		config_unset.git_cmd = 1;
+		strvec_pushl(&config_unset.args, "config", "--global", "--unset",
+			     "--fixed-value", key, maintpath, NULL);
+
+		rc = run_command(&config_unset);
+	} else if (!force) {
+		die(_("repository '%s' is not registered"), maintpath);
+	}
 
-	rc = run_command(&config_unset);
 	free(maintpath);
 	return rc;
 }
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 2724a44fe3e..96bdd420456 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -480,6 +480,11 @@ test_expect_success 'maintenance.strategy inheritance' '
 
 test_expect_success 'register and unregister' '
 	test_when_finished git config --global --unset-all maintenance.repo &&
+
+	test_must_fail git maintenance unregister 2>err &&
+	grep "is not registered" err &&
+	git maintenance unregister --force &&
+
 	git config --global --add maintenance.repo /existing1 &&
 	git config --global --add maintenance.repo /existing2 &&
 	git config --global --get-all maintenance.repo >before &&
@@ -493,7 +498,11 @@ test_expect_success 'register and unregister' '
 
 	git maintenance unregister &&
 	git config --global --get-all maintenance.repo >actual &&
-	test_cmp before actual
+	test_cmp before actual &&
+
+	test_must_fail git maintenance unregister 2>err &&
+	grep "is not registered" err &&
+	git maintenance unregister --force
 '
 
 test_expect_success !MINGW 'register and unregister with regex metacharacters' '
-- 
gitgitgadget

[PATCH v4 2/4] scalar: make 'unregister' idempotent

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The 'scalar unregister' command removes a repository from the list of
registered Scalar repositories and removes it from the list of
repositories registered for background maintenance. If the repository
was not already registered for background maintenance, then the command
fails, even if the repository was still registered as a Scalar
repository.

After using 'scalar clone' or 'scalar register', the repository would be
enrolled in background maintenance since those commands run 'git
maintenance start'. If the user runs 'git maintenance unregister' on
that repository, then it is still in the list of repositories which get
new config updates from 'scalar reconfigure'. The 'scalar unregister'
command would fail since 'git maintenance unregister' would fail.

Further, the add_or_remove_enlistment() method in scalar.c already has
this idempotent nature built in as an expectation since it returns zero
when the scalar.repo list already has the proper containment of the
repository.

The previous change added the 'git maintenance unregister --force'
option, so use it within 'scalar unregister' to make it idempotent.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 scalar.c          | 5 ++++-
 t/t9210-scalar.sh | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/scalar.c b/scalar.c
index c5c1ce68919..6de9c0ee523 100644
--- a/scalar.c
+++ b/scalar.c
@@ -207,7 +207,10 @@ static int set_recommended_config(int reconfigure)
 
 static int toggle_maintenance(int enable)
 {
-	return run_git("maintenance", enable ? "start" : "unregister", NULL);
+	return run_git("maintenance",
+		       enable ? "start" : "unregister",
+		       enable ? NULL : "--force",
+		       NULL);
 }
 
 static int add_or_remove_enlistment(int add)
diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
index 14ca575a214..be51a8bb7a4 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -116,7 +116,10 @@ test_expect_success 'scalar unregister' '
 	test_must_fail git config --get --global --fixed-value \
 		maintenance.repo "$(pwd)/vanish/src" &&
 	scalar list >scalar.repos &&
-	! grep -F "$(pwd)/vanish/src" scalar.repos
+	! grep -F "$(pwd)/vanish/src" scalar.repos &&
+
+	# scalar unregister should be idempotent
+	scalar unregister vanish
 '
 
 test_expect_success 'set up repository to clone' '
-- 
gitgitgadget

[PATCH v4 3/4] gc: replace config subprocesses with API calls

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The 'git maintenance [un]register' commands set or unset the multi-
valued maintenance.repo config key with the absolute path of the current
repository. These are set in the global config file.

Instead of calling a subcommand and creating a new process, create the
proper API calls to git_config_set_multivar_in_file_gently(). It
requires loading the filename for the global config file (and erroring
out if now $HOME value is set). We also need to be careful about using
CONFIG_REGEX_NONE when adding the value and using
CONFIG_FLAGS_FIXED_VALUE when removing the value. In both cases, we
check that the value already exists (this check already existed for
'unregister').

Also, remove the transparent translation of the error code from the
config API to the exit code of 'git maintenance'. Instead, use die() to
recover from failures at that level. In the case of 'unregister
--force', allow the CONFIG_NOTHING_SET error code to be a success. This
allows a possible race where another process removes the config value.
The end result is that the config value is not set anymore, so we can
treat this as a success.

Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 builtin/gc.c | 75 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 31 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index dc0ba9e3648..7a585f0b71d 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1470,11 +1470,12 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_END(),
 	};
-	int rc;
+	int found = 0;
+	const char *key = "maintenance.repo";
 	char *config_value;
-	struct child_process config_set = CHILD_PROCESS_INIT;
-	struct child_process config_get = CHILD_PROCESS_INIT;
 	char *maintpath = get_maintpath();
+	struct string_list_item *item;
+	const struct string_list *list;
 
 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_maintenance_register_usage, 0);
@@ -1491,31 +1492,35 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
 	else
 		git_config_set("maintenance.strategy", "incremental");
 
-	config_get.git_cmd = 1;
-	strvec_pushl(&config_get.args, "config", "--global", "--get",
-		     "--fixed-value", "maintenance.repo", maintpath, NULL);
-	config_get.out = -1;
-
-	if (start_command(&config_get)) {
-		rc = error(_("failed to run 'git config'"));
-		goto done;
+	list = git_config_get_value_multi(key);
+	if (list) {
+		for_each_string_list_item(item, list) {
+			if (!strcmp(maintpath, item->string)) {
+				found = 1;
+				break;
+			}
+		}
 	}
 
-	/* We already have this value in our config! */
-	if (!finish_command(&config_get)) {
-		rc = 0;
-		goto done;
+	if (!found) {
+		int rc;
+		char *user_config, *xdg_config;
+		git_global_config(&user_config, &xdg_config);
+		if (!user_config)
+			die(_("$HOME not set"));
+		rc = git_config_set_multivar_in_file_gently(
+			user_config, "maintenance.repo", maintpath,
+			CONFIG_REGEX_NONE, 0);
+		free(user_config);
+		free(xdg_config);
+
+		if (rc)
+			die(_("unable to add '%s' value of '%s'"),
+			    key, maintpath);
 	}
 
-	config_set.git_cmd = 1;
-	strvec_pushl(&config_set.args, "config", "--add", "--global", "maintenance.repo",
-		     maintpath, NULL);
-
-	rc = run_command(&config_set);
-
-done:
 	free(maintpath);
-	return rc;
+	return 0;
 }
 
 static char const * const builtin_maintenance_unregister_usage[] = {
@@ -1533,8 +1538,6 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 		OPT_END(),
 	};
 	const char *key = "maintenance.repo";
-	int rc = 0;
-	struct child_process config_unset = CHILD_PROCESS_INIT;
 	char *maintpath = get_maintpath();
 	int found = 0;
 	struct string_list_item *item;
@@ -1557,17 +1560,27 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 	}
 
 	if (found) {
-		config_unset.git_cmd = 1;
-		strvec_pushl(&config_unset.args, "config", "--global", "--unset",
-			     "--fixed-value", key, maintpath, NULL);
-
-		rc = run_command(&config_unset);
+		int rc;
+		char *user_config, *xdg_config;
+		git_global_config(&user_config, &xdg_config);
+		if (!user_config)
+			die(_("$HOME not set"));
+		rc = git_config_set_multivar_in_file_gently(
+			user_config, key, NULL, maintpath,
+			CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);
+		free(user_config);
+		free(xdg_config);
+
+		if (rc &&
+		    (!force || rc == CONFIG_NOTHING_SET))
+			die(_("unable to unset '%s' value of '%s'"),
+			    key, maintpath);
 	} else if (!force) {
 		die(_("repository '%s' is not registered"), maintpath);
 	}
 
 	free(maintpath);
-	return rc;
+	return 0;
 }
 
 static const char *get_frequency(enum schedule_priority schedule)
-- 
gitgitgadget

[PATCH v4 4/4] string-list: document iterator behavior on NULL input

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The for_each_string_list_item() macro takes a string_list and
automatically constructs a for loop to iterate over its contents. This
macro will segfault if the list is non-NULL.

We cannot change the macro to be careful around NULL values because
there are many callers that use the address of a local variable, which
will never be NULL and will cause compile errors with -Werror=address.

For now, leave a documentation comment to try to avoid mistakes in the
future where a caller does not check for a NULL list.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 string-list.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/string-list.h b/string-list.h
index d5a744e1438..c7b0d5d0008 100644
--- a/string-list.h
+++ b/string-list.h
@@ -141,7 +141,12 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c
 int for_each_string_list(struct string_list *list,
 			 string_list_each_func_t func, void *cb_data);
 
-/** Iterate over each item, as a macro. */
+/**
+ * Iterate over each item, as a macro.
+ *
+ * Be sure that 'list' is non-NULL. The macro cannot perform NULL
+ * checks due to -Werror=address errors.
+ */
 #define for_each_string_list_item(item,list)            \
 	for (item = (list)->items;                      \
 	     item && item < (list)->items + (list)->nr; \
-- 
gitgitgadget

Re: [PATCH v4 0/4] scalar: make unregister idempotent

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> I noticed this while we were updating the microsoft/git fork to include
> v2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent
> before, but instead some change in 'scalar unregister' led to it relying on
> the return code of 'git maintenance unregister'. Our functional tests expect
> 'scalar unregister' to be idempotent, and I think that's a good pattern for
> 'git maintenance unregister', so I'm fixing it at that layer.
>
> Despite finding this during the 2.38.0-rc0 integration, this isn't critical
> to the release.
>
> Perhaps an argument could be made that "failure means it wasn't registered
> before", but I think that isn't terribly helpful.

I happen _not_ to share the idea that "unregister is expected to be
idempotent" is a good pattern at all, and it is a better pattern to
make failure mean that the object specified to be acted upon did not
even exist (cf. "rm no-such-file").  But that aside, does what the
above paragraphs mention still true for this round, namely, you are
"fixing" it at that (i.e. "maintenance unregister") layer?

The cover letter does not become part of the permanent history, so
it is not a huge deal as long as the reviewers know what they are
looking at ;-).  Just leaving a note in case somebody who missed the
iterations up to v3 is now interested in the topic.

Thanks for a quick iteration.

Re: [PATCH v4 4/4] string-list: document iterator behavior on NULL input

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <derrickstolee@github.com>
>
> The for_each_string_list_item() macro takes a string_list and
> automatically constructs a for loop to iterate over its contents. This
> macro will segfault if the list is non-NULL.
>
> We cannot change the macro to be careful around NULL values because
> there are many callers that use the address of a local variable, which
> will never be NULL and will cause compile errors with -Werror=address.
>
> For now, leave a documentation comment to try to avoid mistakes in the
> future where a caller does not check for a NULL list.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  string-list.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

For exactly the -Werror=address reason, any other way that checks
list for NULL-ness cannot be done here (other than with Peff's
inline helper that returns the first item or NULL), which is a bit
of shame but this is totally outside the topic, and an additional
comment is a good thing to have here.

Thanks.  All four patches look reasonable.  Will queue (but after I
make sure I can tag and push out -rc2 today).


> diff --git a/string-list.h b/string-list.h
> index d5a744e1438..c7b0d5d0008 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -141,7 +141,12 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c
>  int for_each_string_list(struct string_list *list,
>  			 string_list_each_func_t func, void *cb_data);
>  
> -/** Iterate over each item, as a macro. */
> +/**
> + * Iterate over each item, as a macro.
> + *
> + * Be sure that 'list' is non-NULL. The macro cannot perform NULL
> + * checks due to -Werror=address errors.
> + */
>  #define for_each_string_list_item(item,list)            \
>  	for (item = (list)->items;                      \
>  	     item && item < (list)->items + (list)->nr; \

Re: [PATCH v4 0/4] scalar: make unregister idempotent

[Derrick Stolee]

On 9/27/2022 12:31 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> I noticed this while we were updating the microsoft/git fork to include
>> v2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent
>> before, but instead some change in 'scalar unregister' led to it relying on
>> the return code of 'git maintenance unregister'. Our functional tests expect
>> 'scalar unregister' to be idempotent, and I think that's a good pattern for
>> 'git maintenance unregister', so I'm fixing it at that layer.
>>
>> Despite finding this during the 2.38.0-rc0 integration, this isn't critical
>> to the release.
>>
>> Perhaps an argument could be made that "failure means it wasn't registered
>> before", but I think that isn't terribly helpful.
> 
> I happen _not_ to share the idea that "unregister is expected to be
> idempotent" is a good pattern at all, and it is a better pattern to
> make failure mean that the object specified to be acted upon did not
> even exist (cf. "rm no-such-file").  But that aside, does what the
> above paragraphs mention still true for this round, namely, you are
> "fixing" it at that (i.e. "maintenance unregister") layer?

Sorry, I forgot to update the cover letter when updating the title.
"git maintenance unregister" will fail if the repository is not already
registered (unless --force is given). Now, "scalar unregister" _is_
idempotent (it uses "git maintenance unregister --force").
 
> The cover letter does not become part of the permanent history, so
> it is not a huge deal as long as the reviewers know what they are
> looking at ;-).  Just leaving a note in case somebody who missed the
> iterations up to v3 is now interested in the topic.
> 
> Thanks for a quick iteration.

Thank you for your very careful review!

-Stolee