git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH v2 0/2] git-maintenance quality-of-life improvements
@ 2022-11-08 19:49 Ronan Pigott
  2022-11-08 19:49 ` [PATCH v2 1/2] for-each-repo: interpolate repo path arguments Ronan Pigott
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Ronan Pigott @ 2022-11-08 19:49 UTC (permalink / raw)
  To: git; +Cc: me, derrickstolee

This is a pair of quality-of-life patches for git-maintenance that have
helped me in my workflow. PTAL.

Changes from v1:
 - Added a test case for path interpolation in for-each-repo
 - Changed [--config <file>] to [--config-file <path>] in
   register/unregister
 - Dropped the short-name option, -c
 - Avoid finding the global config when the user has supplied the config
   on the command line

Ronan Pigott (2):
  for-each-repo: interpolate repo path arguments
  maintenance: add option to register in a specific config

 Documentation/git-maintenance.txt | 14 ++++-----
 builtin/for-each-repo.c           |  5 ++-
 builtin/gc.c                      | 51 +++++++++++++++++++------------
 t/t0068-for-each-repo.sh          |  6 ++++
 t/t7900-maintenance.sh            | 15 +++++++++
 5 files changed, 64 insertions(+), 27 deletions(-)

-- 
2.38.1


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

* [PATCH v2 1/2] for-each-repo: interpolate repo path arguments
  2022-11-08 19:49 [PATCH v2 0/2] git-maintenance quality-of-life improvements Ronan Pigott
@ 2022-11-08 19:49 ` Ronan Pigott
  2022-11-08 19:49 ` [PATCH v2 2/2] maintenance: add option to register in a specific config Ronan Pigott
  2022-11-08 19:54 ` [PATCH v2 0/2] git-maintenance quality-of-life improvements Taylor Blau
  2 siblings, 0 replies; 10+ messages in thread
From: Ronan Pigott @ 2022-11-08 19:49 UTC (permalink / raw)
  To: git; +Cc: me, derrickstolee

This is a quality of life change for git-maintenance, so repos can be
recorded with the tilde syntax. The register subcommand will not record
repos in this format by default.

Signed-off-by: Ronan Pigott <ronan@rjp.ie>
---
 builtin/for-each-repo.c  | 5 ++++-
 t/t0068-for-each-repo.sh | 6 ++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
index d45d873f57..6aeac37148 100644
--- a/builtin/for-each-repo.c
+++ b/builtin/for-each-repo.c
@@ -14,13 +14,16 @@ static int run_command_on_repo(const char *path, int argc, const char ** argv)
 {
 	int i;
 	struct child_process child = CHILD_PROCESS_INIT;
+	char *abspath = interpolate_path(path, 0);
 
 	child.git_cmd = 1;
-	strvec_pushl(&child.args, "-C", path, NULL);
+	strvec_pushl(&child.args, "-C", abspath, NULL);
 
 	for (i = 0; i < argc; i++)
 		strvec_push(&child.args, argv[i]);
 
+	free(abspath);
+
 	return run_command(&child);
 }
 
diff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh
index 4675e85251..c6e0d65563 100755
--- a/t/t0068-for-each-repo.sh
+++ b/t/t0068-for-each-repo.sh
@@ -8,9 +8,11 @@ test_expect_success 'run based on configured value' '
 	git init one &&
 	git init two &&
 	git init three &&
+	git init ~/four &&
 	git -C two commit --allow-empty -m "DID NOT RUN" &&
 	git config run.key "$TRASH_DIRECTORY/one" &&
 	git config --add run.key "$TRASH_DIRECTORY/three" &&
+	git config --add run.key "~/four" &&
 	git for-each-repo --config=run.key commit --allow-empty -m "ran" &&
 	git -C one log -1 --pretty=format:%s >message &&
 	grep ran message &&
@@ -18,12 +20,16 @@ test_expect_success 'run based on configured value' '
 	! grep ran message &&
 	git -C three log -1 --pretty=format:%s >message &&
 	grep ran message &&
+	git -C ~/four log -1 --pretty=format:%s >message &&
+	grep ran message &&
 	git for-each-repo --config=run.key -- commit --allow-empty -m "ran again" &&
 	git -C one log -1 --pretty=format:%s >message &&
 	grep again message &&
 	git -C two log -1 --pretty=format:%s >message &&
 	! grep again message &&
 	git -C three log -1 --pretty=format:%s >message &&
+	grep again message &&
+	git -C ~/four log -1 --pretty=format:%s >message &&
 	grep again message
 '
 
-- 
2.38.1


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

* [PATCH v2 2/2] maintenance: add option to register in a specific config
  2022-11-08 19:49 [PATCH v2 0/2] git-maintenance quality-of-life improvements Ronan Pigott
  2022-11-08 19:49 ` [PATCH v2 1/2] for-each-repo: interpolate repo path arguments Ronan Pigott
@ 2022-11-08 19:49 ` Ronan Pigott
  2022-11-08 20:38   ` Phillip Wood
  2022-11-09 15:05   ` Derrick Stolee
  2022-11-08 19:54 ` [PATCH v2 0/2] git-maintenance quality-of-life improvements Taylor Blau
  2 siblings, 2 replies; 10+ messages in thread
From: Ronan Pigott @ 2022-11-08 19:49 UTC (permalink / raw)
  To: git; +Cc: me, derrickstolee

maintenance register currently records the maintenance repo exclusively
within the user's global configuration, but other configuration files
may be relevant when running maintenance if they are included from the
global config. This option allows the user to choose where maintenance
repos are recorded.

Signed-off-by: Ronan Pigott <ronan@rjp.ie>
---
 Documentation/git-maintenance.txt | 14 ++++-----
 builtin/gc.c                      | 51 +++++++++++++++++++------------
 t/t7900-maintenance.sh            | 15 +++++++++
 3 files changed, 54 insertions(+), 26 deletions(-)

diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
index bb888690e4..eb3ae9fbd5 100644
--- a/Documentation/git-maintenance.txt
+++ b/Documentation/git-maintenance.txt
@@ -50,13 +50,13 @@ stop::
 	the background maintenance is restarted later.
 
 register::
-	Initialize Git config values so any scheduled maintenance will
-	start running on this repository. This adds the repository to the
-	`maintenance.repo` config variable in the current user's global
-	config and enables some recommended configuration values for
-	`maintenance.<task>.schedule`. The tasks that are enabled are safe
-	for running in the background without disrupting foreground
-	processes.
+	Initialize Git config values so any scheduled maintenance will start
+	running on this repository. This adds the repository to the
+	`maintenance.repo` config variable in the current user's global config,
+	or the config specified by --config option, and enables some
+	recommended configuration values for `maintenance.<task>.schedule`. The
+	tasks that are enabled are safe for running in the background without
+	disrupting foreground processes.
 +
 The `register` subcommand will also set the `maintenance.strategy` config
 value to `incremental`, if this value is not previously set. The
diff --git a/builtin/gc.c b/builtin/gc.c
index 24ea85c7af..1709355bce 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1454,13 +1454,15 @@ static char *get_maintpath(void)
 }
 
 static char const * const builtin_maintenance_register_usage[] = {
-	"git maintenance register",
+	"git maintenance register [--config-file <path>]",
 	NULL
 };
 
 static int maintenance_register(int argc, const char **argv, const char *prefix)
 {
+	char *config_file = NULL;
 	struct option options[] = {
+		OPT_STRING(0, "config-file", &config_file, N_("file"), N_("use given config file")),
 		OPT_END(),
 	};
 	int found = 0;
@@ -1497,12 +1499,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
 
 	if (!found) {
 		int rc;
-		char *user_config, *xdg_config;
-		git_global_config(&user_config, &xdg_config);
-		if (!user_config)
-			die(_("$HOME not set"));
+		char *user_config = NULL, *xdg_config = NULL;
+
+		if (!config_file) {
+			git_global_config(&user_config, &xdg_config);
+			config_file = user_config;
+			if (!user_config)
+				die(_("$HOME not set"));
+		}
 		rc = git_config_set_multivar_in_file_gently(
-			user_config, "maintenance.repo", maintpath,
+			config_file, "maintenance.repo", maintpath,
 			CONFIG_REGEX_NONE, 0);
 		free(user_config);
 		free(xdg_config);
@@ -1517,14 +1523,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
 }
 
 static char const * const builtin_maintenance_unregister_usage[] = {
-	"git maintenance unregister [--force]",
+	"git maintenance unregister [--config-file <path>] [--force]",
 	NULL
 };
 
 static int maintenance_unregister(int argc, const char **argv, const char *prefix)
 {
 	int force = 0;
+	char *config_file = NULL;
 	struct option options[] = {
+		OPT_STRING(0, "config-file", &config_file, N_("file"), N_("use given config file")),
 		OPT__FORCE(&force,
 			   N_("return success even if repository was not registered"),
 			   PARSE_OPT_NOCOMPLETE),
@@ -1542,24 +1550,29 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 		usage_with_options(builtin_maintenance_unregister_usage,
 				   options);
 
-	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 (!config_file) {
+		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) {
+	if (found || config_file) {
 		int rc;
-		char *user_config, *xdg_config;
-		git_global_config(&user_config, &xdg_config);
-		if (!user_config)
-			die(_("$HOME not set"));
+		char *user_config = NULL, *xdg_config = NULL;
+		if (!config_file) {
+			git_global_config(&user_config, &xdg_config);
+			config_file = user_config;
+			if (!user_config)
+				die(_("$HOME not set"));
+		}
 		rc = git_config_set_multivar_in_file_gently(
-			user_config, key, NULL, maintpath,
+			config_file, key, NULL, maintpath,
 			CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);
 		free(user_config);
 		free(xdg_config);
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 96bdd42045..091da683a8 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -500,6 +500,21 @@ test_expect_success 'register and unregister' '
 	git config --global --get-all maintenance.repo >actual &&
 	test_cmp before actual &&
 
+	git config --file ./other --add maintenance.repo /existing1 &&
+	git config --file ./other --add maintenance.repo /existing2 &&
+	git config --file ./other --get-all maintenance.repo >before &&
+
+	git maintenance register --config-file ./other &&
+	test_cmp_config false maintenance.auto &&
+	git config --file ./other --get-all maintenance.repo >between &&
+	cp before expect &&
+	pwd >>expect &&
+	test_cmp expect between &&
+
+	git maintenance unregister --config-file ./other &&
+	git config --file ./other --get-all maintenance.repo >actual &&
+	test_cmp before actual &&
+
 	test_must_fail git maintenance unregister 2>err &&
 	grep "is not registered" err &&
 	git maintenance unregister --force
-- 
2.38.1


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

* Re: [PATCH v2 0/2] git-maintenance quality-of-life improvements
  2022-11-08 19:49 [PATCH v2 0/2] git-maintenance quality-of-life improvements Ronan Pigott
  2022-11-08 19:49 ` [PATCH v2 1/2] for-each-repo: interpolate repo path arguments Ronan Pigott
  2022-11-08 19:49 ` [PATCH v2 2/2] maintenance: add option to register in a specific config Ronan Pigott
@ 2022-11-08 19:54 ` Taylor Blau
  2 siblings, 0 replies; 10+ messages in thread
From: Taylor Blau @ 2022-11-08 19:54 UTC (permalink / raw)
  To: Ronan Pigott; +Cc: git, me, derrickstolee

Hi Ronan,

On Tue, Nov 08, 2022 at 12:49:28PM -0700, Ronan Pigott wrote:
> Ronan Pigott (2):
>   for-each-repo: interpolate repo path arguments
>   maintenance: add option to register in a specific config

Thanks, this round looks good to me. Will queue.

Thanks,
Taylor

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

* Re: [PATCH v2 2/2] maintenance: add option to register in a specific config
  2022-11-08 19:49 ` [PATCH v2 2/2] maintenance: add option to register in a specific config Ronan Pigott
@ 2022-11-08 20:38   ` Phillip Wood
  2022-11-08 20:57     ` Taylor Blau
  2022-11-09 15:05   ` Derrick Stolee
  1 sibling, 1 reply; 10+ messages in thread
From: Phillip Wood @ 2022-11-08 20:38 UTC (permalink / raw)
  To: Ronan Pigott, git; +Cc: me, derrickstolee

Hi Ronan

I think both these patches are useful improvements, thanks for working 
on them. I've left one comment below

On 08/11/2022 19:49, Ronan Pigott wrote:
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 24ea85c7af..1709355bce 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1497,12 +1499,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
>   
>   	if (!found) {
>   		int rc;
> -		char *user_config, *xdg_config;
> -		git_global_config(&user_config, &xdg_config);
> -		if (!user_config)
> -			die(_("$HOME not set"));
> +		char *user_config = NULL, *xdg_config = NULL;
> +
> +		if (!config_file) {
> +			git_global_config(&user_config, &xdg_config);
> +			config_file = user_config;

Here we need to decide whether to use user_config or xdg_config as the 
config file. In builtin/config.c we do this with

		if (access_or_warn(user_config, R_OK, 0) &&
		    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
			given_config_source.file = xdg_config;
			free(user_config);
		} else {
			given_config_source.file = user_config;
			free(xdg_config);
		}

We need something similar here (maybe we should create a helper function 
to find the appropriate file)

> +			if (!user_config)
> +				die(_("$HOME not set"));

This check needs to come before deciding which config file to use

Best Wishes

Phillip

> +		}
>   		rc = git_config_set_multivar_in_file_gently(
> -			user_config, "maintenance.repo", maintpath,
> +			config_file, "maintenance.repo", maintpath,
>   			CONFIG_REGEX_NONE, 0);
>   		free(user_config);
>   		free(xdg_config);
> @@ -1517,14 +1523,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
>   }
>   
>   static char const * const builtin_maintenance_unregister_usage[] = {
> -	"git maintenance unregister [--force]",
> +	"git maintenance unregister [--config-file <path>] [--force]",
>   	NULL
>   };
>   
>   static int maintenance_unregister(int argc, const char **argv, const char *prefix)
>   {
>   	int force = 0;
> +	char *config_file = NULL;
>   	struct option options[] = {
> +		OPT_STRING(0, "config-file", &config_file, N_("file"), N_("use given config file")),
>   		OPT__FORCE(&force,
>   			   N_("return success even if repository was not registered"),
>   			   PARSE_OPT_NOCOMPLETE),
> @@ -1542,24 +1550,29 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>   		usage_with_options(builtin_maintenance_unregister_usage,
>   				   options);
>   
> -	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 (!config_file) {
> +		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) {
> +	if (found || config_file) {
>   		int rc;
> -		char *user_config, *xdg_config;
> -		git_global_config(&user_config, &xdg_config);
> -		if (!user_config)
> -			die(_("$HOME not set"));
> +		char *user_config = NULL, *xdg_config = NULL;
> +		if (!config_file) {
> +			git_global_config(&user_config, &xdg_config);
> +			config_file = user_config;
> +			if (!user_config)
> +				die(_("$HOME not set"));
> +		}
>   		rc = git_config_set_multivar_in_file_gently(
> -			user_config, key, NULL, maintpath,
> +			config_file, key, NULL, maintpath,
>   			CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);
>   		free(user_config);
>   		free(xdg_config);
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 96bdd42045..091da683a8 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -500,6 +500,21 @@ test_expect_success 'register and unregister' '
>   	git config --global --get-all maintenance.repo >actual &&
>   	test_cmp before actual &&
>   
> +	git config --file ./other --add maintenance.repo /existing1 &&
> +	git config --file ./other --add maintenance.repo /existing2 &&
> +	git config --file ./other --get-all maintenance.repo >before &&
> +
> +	git maintenance register --config-file ./other &&
> +	test_cmp_config false maintenance.auto &&
> +	git config --file ./other --get-all maintenance.repo >between &&
> +	cp before expect &&
> +	pwd >>expect &&
> +	test_cmp expect between &&
> +
> +	git maintenance unregister --config-file ./other &&
> +	git config --file ./other --get-all maintenance.repo >actual &&
> +	test_cmp before actual &&
> +
>   	test_must_fail git maintenance unregister 2>err &&
>   	grep "is not registered" err &&
>   	git maintenance unregister --force

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

* Re: [PATCH v2 2/2] maintenance: add option to register in a specific config
  2022-11-08 20:38   ` Phillip Wood
@ 2022-11-08 20:57     ` Taylor Blau
  2022-11-08 21:28       ` Phillip Wood
  2022-11-08 22:45       ` ronan
  0 siblings, 2 replies; 10+ messages in thread
From: Taylor Blau @ 2022-11-08 20:57 UTC (permalink / raw)
  To: phillip.wood; +Cc: Ronan Pigott, git, me, derrickstolee

On Tue, Nov 08, 2022 at 08:38:51PM +0000, Phillip Wood wrote:
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index 24ea85c7af..1709355bce 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -1497,12 +1499,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
> >   	if (!found) {
> >   		int rc;
> > -		char *user_config, *xdg_config;
> > -		git_global_config(&user_config, &xdg_config);
> > -		if (!user_config)
> > -			die(_("$HOME not set"));
> > +		char *user_config = NULL, *xdg_config = NULL;
> > +
> > +		if (!config_file) {
> > +			git_global_config(&user_config, &xdg_config);
> > +			config_file = user_config;
>
> Here we need to decide whether to use user_config or xdg_config as the
> config file. In builtin/config.c we do this with
>
> 		if (access_or_warn(user_config, R_OK, 0) &&
> 		    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
> 			given_config_source.file = xdg_config;
> 			free(user_config);
> 		} else {
> 			given_config_source.file = user_config;
> 			free(xdg_config);
> 		}
>
> We need something similar here (maybe we should create a helper function to
> find the appropriate file)
>
> > +			if (!user_config)
> > +				die(_("$HOME not set"));
>
> This check needs to come before deciding which config file to use

True, but that problem existed before this series, too. So the new
behavior is no worse with respect to the XDG config stuff, and any
improvements to that behavior can be done independently on top.

Thanks,
Taylor

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

* Re: [PATCH v2 2/2] maintenance: add option to register in a specific config
  2022-11-08 20:57     ` Taylor Blau
@ 2022-11-08 21:28       ` Phillip Wood
  2022-11-08 22:45       ` ronan
  1 sibling, 0 replies; 10+ messages in thread
From: Phillip Wood @ 2022-11-08 21:28 UTC (permalink / raw)
  To: Taylor Blau; +Cc: Ronan Pigott, git, me, derrickstolee

Hi Taylor

On 08/11/2022 20:57, Taylor Blau wrote:
> On Tue, Nov 08, 2022 at 08:38:51PM +0000, Phillip Wood wrote:
>>> diff --git a/builtin/gc.c b/builtin/gc.c
>>> index 24ea85c7af..1709355bce 100644
>>> --- a/builtin/gc.c
>>> +++ b/builtin/gc.c
>>> @@ -1497,12 +1499,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
>>>    	if (!found) {
>>>    		int rc;
>>> -		char *user_config, *xdg_config;
>>> -		git_global_config(&user_config, &xdg_config);
>>> -		if (!user_config)
>>> -			die(_("$HOME not set"));
>>> +		char *user_config = NULL, *xdg_config = NULL;
>>> +
>>> +		if (!config_file) {
>>> +			git_global_config(&user_config, &xdg_config);
>>> +			config_file = user_config;
>>
>> Here we need to decide whether to use user_config or xdg_config as the
>> config file. In builtin/config.c we do this with
>>
>> 		if (access_or_warn(user_config, R_OK, 0) &&
>> 		    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
>> 			given_config_source.file = xdg_config;
>> 			free(user_config);
>> 		} else {
>> 			given_config_source.file = user_config;
>> 			free(xdg_config);
>> 		}
>>
>> We need something similar here (maybe we should create a helper function to
>> find the appropriate file)
>>
>>> +			if (!user_config)
>>> +				die(_("$HOME not set"));
>>
>> This check needs to come before deciding which config file to use
> 
> True, but that problem existed before this series, too.

Oh yes, sorry I'd missed that

Phillip

> So the new
> behavior is no worse with respect to the XDG config stuff, and any
> improvements to that behavior can be done independently on top. >> Thanks,
> Taylor

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

* Re: [PATCH v2 2/2] maintenance: add option to register in a specific config
  2022-11-08 20:57     ` Taylor Blau
  2022-11-08 21:28       ` Phillip Wood
@ 2022-11-08 22:45       ` ronan
  1 sibling, 0 replies; 10+ messages in thread
From: ronan @ 2022-11-08 22:45 UTC (permalink / raw)
  To: phillip.wood; +Cc: git

Yes, I saw that the xdg_config was ignored but did not try to change it.

If we add a helper function for this in the future it might be worth considering
some of the lesser supported parts of the xdg spec, specifically XDG_CONFIG_DIRS
which I use with neovim, for example, to achieve some more host-specific configurations.

The xdg spec isn't totally clear on how these additional configuration directories should
be used imo, so it would also be acceptable to just ignore them as is currently done.

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

* Re: [PATCH v2 2/2] maintenance: add option to register in a specific config
  2022-11-08 19:49 ` [PATCH v2 2/2] maintenance: add option to register in a specific config Ronan Pigott
  2022-11-08 20:38   ` Phillip Wood
@ 2022-11-09 15:05   ` Derrick Stolee
  2022-11-10  2:38     ` Taylor Blau
  1 sibling, 1 reply; 10+ messages in thread
From: Derrick Stolee @ 2022-11-09 15:05 UTC (permalink / raw)
  To: Ronan Pigott, git; +Cc: me

On 11/8/2022 2:49 PM, Ronan Pigott wrote:
> maintenance register currently records the maintenance repo exclusively
> within the user's global configuration, but other configuration files
> may be relevant when running maintenance if they are included from the
> global config. This option allows the user to choose where maintenance
> repos are recorded.

> +	Initialize Git config values so any scheduled maintenance will start
> +	running on this repository. This adds the repository to the
> +	`maintenance.repo` config variable in the current user's global config,
> +	or the config specified by --config option, and enables some

This still says --config...

> +	recommended configuration values for `maintenance.<task>.schedule`. The
> +	tasks that are enabled are safe for running in the background without
> +	disrupting foreground processes.

> +	"git maintenance register [--config-file <path>]",

> +	char *config_file = NULL;
>  	struct option options[] = {
> +		OPT_STRING(0, "config-file", &config_file, N_("file"), N_("use given config file")),

>  static char const * const builtin_maintenance_unregister_usage[] = {
> -	"git maintenance unregister [--force]",
> +	"git maintenance unregister [--config-file <path>] [--force]",

> +	char *config_file = NULL;
>  	struct option options[] = {
> +		OPT_STRING(0, "config-file", &config_file, N_("file"), N_("use given config file")),

...but these have been updated. Easy to miss that doc update.

> -	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 (!config_file) {
> +		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) {
> +	if (found || config_file) {

This seems like it will attempt to update the given config file
regardless of its contents. This might result in failures when the
file does not have that value, which is different than the typical
case.

I think the right thing to do would be to update the first
"if (!config_file)" block to have an else.


	if (!config_file)
		list = git_config_get_value_multi(key);
	else
		list = <get multi-value from the file>

	if (list) {
		for_each_string_list_item(item, list) {
			...

Then the "if (found)" case can be the same.

>  		int rc;
> -		char *user_config, *xdg_config;
> -		git_global_config(&user_config, &xdg_config);
> -		if (!user_config)
> -			die(_("$HOME not set"));
> +		char *user_config = NULL, *xdg_config = NULL;
> +		if (!config_file) {
> +			git_global_config(&user_config, &xdg_config);
> +			config_file = user_config;
> +			if (!user_config)
> +				die(_("$HOME not set"));
> +		}
>  		rc = git_config_set_multivar_in_file_gently(
> -			user_config, key, NULL, maintpath,
> +			config_file, key, NULL, maintpath,
>  			CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);

at least until this part that handles the config write.

>  		free(user_config);
>  		free(xdg_config);
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 96bdd42045..091da683a8 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -500,6 +500,21 @@ test_expect_success 'register and unregister' '
>  	git config --global --get-all maintenance.repo >actual &&
>  	test_cmp before actual &&
>  
> +	git config --file ./other --add maintenance.repo /existing1 &&
> +	git config --file ./other --add maintenance.repo /existing2 &&
> +	git config --file ./other --get-all maintenance.repo >before &&
> +
> +	git maintenance register --config-file ./other &&
> +	test_cmp_config false maintenance.auto &&
> +	git config --file ./other --get-all maintenance.repo >between &&
> +	cp before expect &&
> +	pwd >>expect &&
> +	test_cmp expect between &&
> +
> +	git maintenance unregister --config-file ./other &&
> +	git config --file ./other --get-all maintenance.repo >actual &&
> +	test_cmp before actual &&
> +

Please add a case of `git maintenance unregister --config-file` where
the given file does not already have that repo listed to make sure we
cover that case.

Thanks,
-Stolee

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

* Re: [PATCH v2 2/2] maintenance: add option to register in a specific config
  2022-11-09 15:05   ` Derrick Stolee
@ 2022-11-10  2:38     ` Taylor Blau
  0 siblings, 0 replies; 10+ messages in thread
From: Taylor Blau @ 2022-11-10  2:38 UTC (permalink / raw)
  To: Derrick Stolee; +Cc: Ronan Pigott, git, me

On Wed, Nov 09, 2022 at 10:05:32AM -0500, Derrick Stolee wrote:
> On 11/8/2022 2:49 PM, Ronan Pigott wrote:
> > maintenance register currently records the maintenance repo exclusively
> > within the user's global configuration, but other configuration files
> > may be relevant when running maintenance if they are included from the
> > global config. This option allows the user to choose where maintenance
> > repos are recorded.
>
> > +	Initialize Git config values so any scheduled maintenance will start
> > +	running on this repository. This adds the repository to the
> > +	`maintenance.repo` config variable in the current user's global config,
> > +	or the config specified by --config option, and enables some
>
> This still says --config...

Thanks for spotting. I graduated this topic too early, it seems. Let's
hold it in 'next' while we wait for a patch or two on top, or I can see
about reverting what's in 'next' and replacing it with a new round.

Thanks,
Taylor

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

end of thread, other threads:[~2022-11-10  2:40 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-08 19:49 [PATCH v2 0/2] git-maintenance quality-of-life improvements Ronan Pigott
2022-11-08 19:49 ` [PATCH v2 1/2] for-each-repo: interpolate repo path arguments Ronan Pigott
2022-11-08 19:49 ` [PATCH v2 2/2] maintenance: add option to register in a specific config Ronan Pigott
2022-11-08 20:38   ` Phillip Wood
2022-11-08 20:57     ` Taylor Blau
2022-11-08 21:28       ` Phillip Wood
2022-11-08 22:45       ` ronan
2022-11-09 15:05   ` Derrick Stolee
2022-11-10  2:38     ` Taylor Blau
2022-11-08 19:54 ` [PATCH v2 0/2] git-maintenance quality-of-life improvements Taylor Blau

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