* Re: [RFC PATCH 1/1] maintenance: add config option for config-file
2025-12-18 18:48 ` [RFC PATCH 1/1] " Matthew Hughes
@ 2025-12-19 7:18 ` Patrick Steinhardt
2025-12-19 8:27 ` Junio C Hamano
1 sibling, 0 replies; 6+ messages in thread
From: Patrick Steinhardt @ 2025-12-19 7:18 UTC (permalink / raw)
To: Matthew Hughes; +Cc: git, Derrick Stolee
On Thu, Dec 18, 2025 at 06:48:19PM +0000, Matthew Hughes wrote:
> This is to allow splitting out this configuration from the global config
> file, e.g.:
>
> # in ~/.config/git/config
> [include]
> path = maintenance.config
> [maintenance]
> # use a separate files for reads/writes from
> # 'git maintenance {un,}register'
> configFile = ~/.config/git/maintenance.config
>
> # in ~/.config/git/maintenance.config
> [maintenance]
> repo = /path/to/some/repo
> repo = /path/to/another/repo
>
> My motivation for this is that I track my global config in git, so I'd
> like to avoid changes in there that depend on specific repos/workflows
> that I'm working with.
I think the idea is quite sensible, and I understand the need to split
up static configuration from dynamic one. One part I'm unsure about
though is the explicit need to use "include.path" for this. Ideally,
git-maintenance(1) should by itself know to include the path that the
user has configured in "maintenance.configFile".
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 92c6e7b954..257cceecf6 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -2124,6 +2124,10 @@ static int maintenance_register(int argc, const char **argv, const char *prefix,
> usage_with_options(builtin_maintenance_register_usage,
> options);
>
> + if (config_file == NULL) {
> + repo_config_get_pathname(the_repository, "maintenance.configFile", &config_file);
> + }
Note that in our codebase this would typically be written as:
if (!config_file)
repo_config_get_pathname(the_repository, "maintenance.configFile", &config_file);
Note the missing curly braces around a single-line statement as well as
the implicit check for NULL.
It's nice that we only need a two-line change here to make this work.
> @@ -2194,6 +2198,10 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
> usage_with_options(builtin_maintenance_unregister_usage,
> options);
>
> + if (config_file == NULL) {
> + repo_config_get_pathname(the_repository, "maintenance.configFile", &config_file);
> + }
Okay, this is the equivalent for unregistering maintenance. Makes senes.
So these both are straight-forward. The question is whether we can also
easily teach scheduled maintenance to respect `maintenance.configFile`
so that the user doesn't have to manually include the configured file.
I think the answer is "not quite".
The services that scheduled maintenance writes use git-for-each-repo(1)
to iterate through all repositories. We basically execute that command
via `git for-each-repo --config=maintenance.repo -- git maintenance
run --schedule=%%i`. But that command does not have a way to have it
read the configuration from a different file.
I think adding such a feature shouldn't be that hard though. You can use
the below (completely untested) patch as a starter for this. But once we
had such a feature we'd only have to adapt how we write the services
files to pass the new flag in case "maintenance.configFile" is set.
There's two questions in this context:
- Do we already know to rewrite the service files in case the commands
that Git would write change?
- Do we need to migrate any of the preexisting keys that exist in the
global configuration?
Also Cc'ing Stolee for input.
Thanks!
Patrick
-- >8 --
diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
index 325a7925f1..874f98ced6 100644
--- a/builtin/for-each-repo.c
+++ b/builtin/for-each-repo.c
@@ -1,6 +1,7 @@
#define USE_THE_REPOSITORY_VARIABLE
#include "builtin.h"
+#include "abspath.h"
#include "config.h"
#include "gettext.h"
#include "parse-options.h"
@@ -39,11 +40,16 @@ int cmd_for_each_repo(int argc,
int keep_going = 0;
int result = 0;
const struct string_list *values;
+ struct config_set configset = { 0 };
+ const char *config_file = NULL;
+ char *config_file_to_free = NULL;
int err;
const struct option options[] = {
OPT_STRING(0, "config", &config_key, N_("config"),
N_("config key storing a list of repository paths")),
+ OPT_STRING(0, "file", &config_file, N_("file"),
+ N_("use given config file")),
OPT_BOOL(0, "keep-going", &keep_going,
N_("keep going even if command fails in a repository")),
OPT_END()
@@ -55,21 +61,38 @@ int cmd_for_each_repo(int argc,
if (!config_key)
die(_("missing --config=<config>"));
- err = repo_config_get_string_multi(the_repository, config_key, &values);
+ if (config_file) {
+ if (!is_absolute_path(config_file) && prefix)
+ config_file = config_file_to_free = prefix_filename(prefix, config_file);
+
+ git_configset_init(&configset);
+ err = git_configset_add_file(&configset, config_file);
+ if (err < 0)
+ die_errno(_("config file could not be read: '%s'"), config_file);
+
+ err = git_configset_get_string_multi(&configset, config_key, &values);
+ } else {
+ err = repo_config_get_string_multi(the_repository, config_key, &values);
+ }
if (err < 0)
usage_msg_optf(_("got bad config --config=%s"),
for_each_repo_usage, options, config_key);
else if (err)
- return 0;
+ goto out;
for (size_t i = 0; i < values->nr; i++) {
int ret = run_command_on_repo(values->items[i].string, argc, argv);
if (ret) {
- if (!keep_going)
- return ret;
+ if (!keep_going) {
+ result = ret;
+ goto out;
+ }
result = 1;
}
}
+out:
+ git_configset_clear(&configset);
+ free(config_file_to_free);
return result;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [RFC PATCH 1/1] maintenance: add config option for config-file
2025-12-18 18:48 ` [RFC PATCH 1/1] " Matthew Hughes
2025-12-19 7:18 ` Patrick Steinhardt
@ 2025-12-19 8:27 ` Junio C Hamano
2025-12-22 8:26 ` Matthew Hughes
1 sibling, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2025-12-19 8:27 UTC (permalink / raw)
To: Matthew Hughes; +Cc: git
Matthew Hughes <matthewhughes934@gmail.com> writes:
> This is to allow splitting out this configuration from the global config
> file, e.g.:
I cannot guess what "this" refers to in this sentence.
> # in ~/.config/git/config
> [include]
> path = maintenance.config
> [maintenance]
> # use a separate files for reads/writes from
> # 'git maintenance {un,}register'
> configFile = ~/.config/git/maintenance.config
>
> # in ~/.config/git/maintenance.config
> [maintenance]
> repo = /path/to/some/repo
> repo = /path/to/another/repo
You are burdening your readers too heavily. After reading the above
three times and then trying to guess what you are trying to do for
several minutes, what I am guessing is:
* maintenance.configFile specifies an additional file to which
maintenance.repo configuration items are written out when "git
maintenance register/unregister" works.
* "git config" is not affected, so "git config set --global
--append maintenance.repo foo" would still write into the
per-user configuration file.
* Also, the general config API does not pay maintenance.configFile
at all, so setting it does not affect "git config list", for
example.
* You'd need an extra "[include] path = maintenance.config" in the
configuration file because of the previous point.
Am I following you well so far? Giving an explanation on your
_intent_, along with the sample configuration, would help your
readers, and I would expect something with a similar degree of
detail as above in the log message.
> My motivation for this is that I track my global config in git, so I'd
> like to avoid changes in there that depend on specific repos/workflows
> that I'm working with.
I am not sure if singling out "maintenance" is the right approach to
solve that issue. If we had a mechanism to have two per-user
configuration file, where one is read-only (as far as Git is
concerned) which is covered/overlayed with a separate read-write
file, not just "maintenance register/unregister" but all other
things that writes into "git config" would use that overlayed file
without touching the base configuration that is read-only. Wouldn't
that be closer to what you want?
> Signed-off-by: Matthew Hughes <matthewhughes934@gmail.com>
> ---
> builtin/gc.c | 8 ++++++++
> t/t7900-maintenance.sh | 13 +++++++++++++
> 2 files changed, 21 insertions(+)
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 92c6e7b954..257cceecf6 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -2124,6 +2124,10 @@ static int maintenance_register(int argc, const char **argv, const char *prefix,
> usage_with_options(builtin_maintenance_register_usage,
> options);
>
> + if (config_file == NULL) {
> + repo_config_get_pathname(the_repository, "maintenance.configFile", &config_file);
> + }
* Comparison with 0 or NULL should be spelled "if (!config_file)"
(meaning, 'is NULL') or "if (config_file)" (meaning, 'not NULL'),
in this project.
* This project omits {} around a single statement block.
* The function call is overly long. wrap to comfortably fit on
80-column terminal after getting quoted in an e-mail review twice
or so, which means ~72 columns is the practical width limit.
repo_config_get_pathname(the_repository,
"maintenance.configFile", &config_file);
> +test_expect_success 'register and unregister config from maintenance.configFile' '
> + test_when_finished git config --global --unset-all maintenance.configFile &&
> +
> + git config set --global maintenance.configFile ./maintenance.config &&
> + git maintenance register &&
> + pwd >>expect &&
Readers would wonder "To what existing contents is this being
appended? Do we have something that we care?" If not, do not use
">>" to mislead them.
Would the output of the pwd command match what "maintenance
register" writes into the file even on Windows? We often see
breakage between $(pwd) and $PWD and I can never get this right
without looking at past discussions.
> + git config get --file ./maintenance.config maintenance.repo >actual &&
> + test_cmp expect actual &&
For this particular case, would it be sufficient to ask
git config get --all --no-includes --file maintenance.config \
maintenance.repo
git config get --all --no-includes --file ./git/config \
maintenance.repo
and see if the former gives output and the latter does not, or
something?
Making sure the maintenance.repo file gets written is good, but for
your purpose, it is equally if not more important that the base
configuration file is not affected, no?
> + git maintenance unregister &&
> + test_must_be_empty ./maintenance.config
Ditto.
> +'
> +
> test_expect_success 'register with no value for maintenance.repo' '
> cp .git/config .git/config.orig &&
> test_when_finished mv .git/config.orig .git/config &&
^ permalink raw reply [flat|nested] 6+ messages in thread