git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] maintenance: make unregister idempotent
@ 2022-09-20  1:02 Derrick Stolee via GitGitGadget
  2022-09-21 17:19 ` Junio C Hamano
  2022-09-22 13:37 ` [PATCH v2 0/2] scalar: " Derrick Stolee via GitGitGadget
  0 siblings, 2 replies; 10+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2022-09-20  1:02 UTC (permalink / raw)
  To: git; +Cc: gitster, vdye, Derrick Stolee, Derrick Stolee

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] maintenance: make unregister idempotent
  2022-09-20  1:02 [PATCH] maintenance: make unregister idempotent Derrick Stolee via GitGitGadget
@ 2022-09-21 17:19 ` Junio C Hamano
  2022-09-22 12:37   ` Derrick Stolee
  2022-09-22 13:37 ` [PATCH v2 0/2] scalar: " Derrick Stolee via GitGitGadget
  1 sibling, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2022-09-21 17:19 UTC (permalink / raw)
  To: Derrick Stolee via GitGitGadget; +Cc: git, vdye, Derrick Stolee

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] maintenance: make unregister idempotent
  2022-09-21 17:19 ` Junio C Hamano
@ 2022-09-22 12:37   ` Derrick Stolee
  2022-09-22 19:31     ` Junio C Hamano
  0 siblings, 1 reply; 10+ messages in thread
From: Derrick Stolee @ 2022-09-22 12:37 UTC (permalink / raw)
  To: Junio C Hamano, Derrick Stolee via GitGitGadget; +Cc: git, vdye

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 0/2] scalar: make unregister idempotent
  2022-09-20  1:02 [PATCH] maintenance: make unregister idempotent Derrick Stolee via GitGitGadget
  2022-09-21 17:19 ` Junio C Hamano
@ 2022-09-22 13:37 ` Derrick Stolee via GitGitGadget
  2022-09-22 13:37   ` [PATCH v2 1/2] maintenance: add 'unregister --force' Derrick Stolee via GitGitGadget
  2022-09-22 13:37   ` [PATCH v2 2/2] scalar: make 'unregister' idempotent Derrick Stolee via GitGitGadget
  1 sibling, 2 replies; 10+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2022-09-22 13:37 UTC (permalink / raw)
  To: git; +Cc: gitster, vdye, Derrick Stolee

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/2] maintenance: add 'unregister --force'
  2022-09-22 13:37 ` [PATCH v2 0/2] scalar: " Derrick Stolee via GitGitGadget
@ 2022-09-22 13:37   ` Derrick Stolee via GitGitGadget
  2022-09-23 13:08     ` SZEDER Gábor
  2022-09-22 13:37   ` [PATCH v2 2/2] scalar: make 'unregister' idempotent Derrick Stolee via GitGitGadget
  1 sibling, 1 reply; 10+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2022-09-22 13:37 UTC (permalink / raw)
  To: git; +Cc: gitster, vdye, Derrick Stolee, Derrick Stolee

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' '
-- 
@@ -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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/2] scalar: make 'unregister' idempotent
  2022-09-22 13:37 ` [PATCH v2 0/2] scalar: " Derrick Stolee via GitGitGadget
  2022-09-22 13:37   ` [PATCH v2 1/2] maintenance: add 'unregister --force' Derrick Stolee via GitGitGadget
@ 2022-09-22 13:37   ` Derrick Stolee via GitGitGadget
  1 sibling, 0 replies; 10+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2022-09-22 13:37 UTC (permalink / raw)
  To: git; +Cc: gitster, vdye, Derrick Stolee, Derrick Stolee

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' '
-- 
@@ -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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] maintenance: make unregister idempotent
  2022-09-22 12:37   ` Derrick Stolee
@ 2022-09-22 19:31     ` Junio C Hamano
  2022-09-22 19:46       ` Derrick Stolee
  0 siblings, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2022-09-22 19:31 UTC (permalink / raw)
  To: Derrick Stolee; +Cc: Derrick Stolee via GitGitGadget, git, vdye

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?



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] maintenance: make unregister idempotent
  2022-09-22 19:31     ` Junio C Hamano
@ 2022-09-22 19:46       ` Derrick Stolee
  2022-09-22 20:44         ` Junio C Hamano
  0 siblings, 1 reply; 10+ messages in thread
From: Derrick Stolee @ 2022-09-22 19:46 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Derrick Stolee via GitGitGadget, git, vdye

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] maintenance: make unregister idempotent
  2022-09-22 19:46       ` Derrick Stolee
@ 2022-09-22 20:44         ` Junio C Hamano
  0 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2022-09-22 20:44 UTC (permalink / raw)
  To: Derrick Stolee; +Cc: Derrick Stolee via GitGitGadget, git, vdye

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] maintenance: add 'unregister --force'
  2022-09-22 13:37   ` [PATCH v2 1/2] maintenance: add 'unregister --force' Derrick Stolee via GitGitGadget
@ 2022-09-23 13:08     ` SZEDER Gábor
  0 siblings, 0 replies; 10+ messages in thread
From: SZEDER Gábor @ 2022-09-23 13:08 UTC (permalink / raw)
  To: Derrick Stolee via GitGitGadget; +Cc: git, gitster, vdye, Derrick Stolee

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-09-23 13:09 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-20  1:02 [PATCH] maintenance: make unregister idempotent Derrick Stolee via GitGitGadget
2022-09-21 17:19 ` Junio C Hamano
2022-09-22 12:37   ` Derrick Stolee
2022-09-22 19:31     ` Junio C Hamano
2022-09-22 19:46       ` Derrick Stolee
2022-09-22 20:44         ` Junio C Hamano
2022-09-22 13:37 ` [PATCH v2 0/2] scalar: " Derrick Stolee via GitGitGadget
2022-09-22 13:37   ` [PATCH v2 1/2] maintenance: add 'unregister --force' Derrick Stolee via GitGitGadget
2022-09-23 13:08     ` SZEDER Gábor
2022-09-22 13:37   ` [PATCH v2 2/2] scalar: make 'unregister' idempotent Derrick Stolee via GitGitGadget

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).