diff mbox series

[v2,6/8] maintenance: add ability to pass config options

Message ID cfa6dca8ef4a64b6233e3b7b6021571447fc5ba9.1645719218.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series fetch: add repair: full refetch without negotiation (was: "refiltering") | expand

Commit Message

Robert Coup Feb. 24, 2022, 4:13 p.m. UTC 
From: Robert Coup <robert@coup.net.nz>

Make run_auto_maintenance() accept optional config options for a
specific invocation of the auto-maintenance process.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 builtin/am.c     | 2 +-
 builtin/commit.c | 2 +-
 builtin/fetch.c  | 2 +-
 builtin/merge.c  | 2 +-
 builtin/rebase.c | 2 +-
 run-command.c    | 8 +++++++-
 run-command.h    | 5 ++++-
 7 files changed, 16 insertions(+), 7 deletions(-)

Comments

Junio C Hamano Feb. 25, 2022, 6:57 a.m. UTC | #1 
"Robert Coup via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +	if (config_opts)
> +		for (i = 0; i<config_opts->nr; i++)

Style.  SP on both sides of '<'.

> +			strvec_pushl(&maint.args, "-c", config_opts->v[i], NULL);

>  	strvec_pushl(&maint.args, "maintenance", "run", "--auto", NULL);
>  	strvec_push(&maint.args, quiet ? "--quiet" : "--no-quiet");

It is unclear if it is generally a good idea to pass hardcoded set
of configuration variables to begin with, but provided if it makes
sense [*], the implementation seems OK.

	Side note.  And this is a big *IF*, as we can see in all
	other helper functions in run-commands.h, nobody has such a
	privision.  If supporting such a "feature" makes sense, we
	probably would need to do so with a common interface that
	can be used across run_command() API, not with an ad-hoc
	interface that is only usable with run_auto_maintenance(),
	which may look somewhat similar to how we have a common way
	to pass set of environment variables.

> diff --git a/run-command.h b/run-command.h
> index 07bed6c31b4..24021abd41f 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -222,8 +222,11 @@ int run_command(struct child_process *);
>  
>  /*
>   * Trigger an auto-gc
> + *
> + * config_opts is an optional list of additional config options to
> + * pass to the maintenance process in the form "some.option=value".
>   */
> -int run_auto_maintenance(int quiet);
> +int run_auto_maintenance(int quiet, const struct strvec *config_opts);
>  
>  #define RUN_COMMAND_NO_STDIN		(1<<0)
>  #define RUN_GIT_CMD			(1<<1)
n
Ævar Arnfjörð Bjarmason Feb. 25, 2022, 10:29 a.m. UTC | #2 
On Thu, Feb 24 2022, Robert Coup via GitGitGadget wrote:

> From: Robert Coup <robert@coup.net.nz>
>
> Make run_auto_maintenance() accept optional config options for a
> specific invocation of the auto-maintenance process.
> [...]
> -int run_auto_maintenance(int quiet)
> +int run_auto_maintenance(int quiet, const struct strvec *config_opts)
>  {
>  	int enabled;
> +	int i;
>  	struct child_process maint = CHILD_PROCESS_INIT;
>  
>  	if (!git_config_get_bool("maintenance.auto", &enabled) &&
> @@ -1809,6 +1810,11 @@ int run_auto_maintenance(int quiet)
>  
>  	maint.git_cmd = 1;
>  	maint.close_object_store = 1;
> +
> +	if (config_opts)
> +		for (i = 0; i<config_opts->nr; i++)
> +			strvec_pushl(&maint.args, "-c", config_opts->v[i], NULL);
> +
>  	strvec_pushl(&maint.args, "maintenance", "run", "--auto", NULL);
>  	strvec_push(&maint.args, quiet ? "--quiet" : "--no-quiet");
>  
> diff --git a/run-command.h b/run-command.h
> index 07bed6c31b4..24021abd41f 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -222,8 +222,11 @@ int run_command(struct child_process *);
>  
>  /*
>   * Trigger an auto-gc
> + *
> + * config_opts is an optional list of additional config options to
> + * pass to the maintenance process in the form "some.option=value".
>   */
> -int run_auto_maintenance(int quiet);
> +int run_auto_maintenance(int quiet, const struct strvec *config_opts);
>  
>  #define RUN_COMMAND_NO_STDIN		(1<<0)
>  #define RUN_GIT_CMD			(1<<1)


Shouldn't this bei using git grep the git_config_push.*parameter()
functions instead of adding a new custom method to do this.

Perhaps there's some subtle distinction between the two that's important
here, or perhaps you just didn't know about that API...
Robert Coup Feb. 28, 2022, 11:51 a.m. UTC | #3 
Hi Ævar,

On Fri, 25 Feb 2022 at 10:30, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Shouldn't this bei using git grep the git_config_push.*parameter()
> functions instead of adding a new custom method to do this.
>
> Perhaps there's some subtle distinction between the two that's important
> here, or perhaps you just didn't know about that API...

I didn't know of it — it could probably use a docstring too ;-) A few
other sub-commands seemed to use the `-c key=value` approach so it
didn't seem like GIT_CONFIG_PARAMETERS was universal.

Easy enough to swap to, thanks. Assuming we want it, but I'm about to
reply to Junio's email.

Rob :)
Robert Coup Feb. 28, 2022, 12:02 p.m. UTC | #4 
Hi Junio,

On Fri, 25 Feb 2022 at 06:57, Junio C Hamano <gitster@pobox.com> wrote:
>
> Style.  SP on both sides of '<'.

Thanks

> It is unclear if it is generally a good idea to pass hardcoded set
> of configuration variables to begin with, but provided if it makes
> sense [*], the implementation seems OK.

My RFC didn't do any repacking, and you (rightly) raised it at
https://lore.kernel.org/git/xmqqk0eecpl9.fsf@gitster.g/

>> To note:
>>
>>  1. This will produce duplicated objects between the existing and newly
>>     fetched packs, but gc will clean them up.
>
> ... it is not smart enough to stell them to exclude what we _ought_
> to have by telling them what the _old_ filter spec was.  That's OK
> for a starting point, I guess.  Hopefully, at the end of this
> operation, we should garbage collect the duplicated objects by
> default (with an option to turn it off)?

This was my best stab at (I think) safely doing it — it respects users
who have disabled auto-gc/repacking/maintenance, and doesn't add yet
another config variable. But we could also:

1. do nothing — the user's object DB will contain duplicate objects
until some repacking happens. This could be up to twice the size it
"should" be.
2. print a message to suggest running `git gc` / `git maintenance`

I'm keen on some input from Derrick or someone deeply familiar with
the maintenance/GC bits (particularly wrt incremental repacking)
whether what I'm doing would cause issues for people with complex
setups I haven't thought of.

>         Side note.  And this is a big *IF*, as we can see in all
>         other helper functions in run-commands.h, nobody has such a
>         privision.  If supporting such a "feature" makes sense, we
>         probably would need to do so with a common interface that
>         can be used across run_command() API, not with an ad-hoc
>         interface that is only usable with run_auto_maintenance(),
>         which may look somewhat similar to how we have a common way
>         to pass set of environment variables.

Ævar pointed out GIT_CONFIG_PARAMETERS has an API to wrap it's use, so
I can adapt the implementation to use that.

Thanks

Rob :)
Junio C Hamano Feb. 28, 2022, 5:07 p.m. UTC | #5 
Robert Coup <robert@coup.net.nz> writes:

> Ævar pointed out GIT_CONFIG_PARAMETERS has an API to wrap it's use, so
> I can adapt the implementation to use that.

Depending on how that "API to wrap" looks like, it may not be a wise
idea, though.  You'd need to consider how it interacts with those
who run "git -c var=val" to enter this codepath.
diff mbox series

Patch

diff --git a/builtin/am.c b/builtin/am.c
index 7de2c89ef22..298c6093bff 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1899,7 +1899,7 @@  next:
 	 */
 	if (!state->rebasing) {
 		am_destroy(state);
-		run_auto_maintenance(state->quiet);
+		run_auto_maintenance(state->quiet, NULL);
 	}
 }
 
diff --git a/builtin/commit.c b/builtin/commit.c
index b9ed0374e30..84e7ab0a4cc 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1844,7 +1844,7 @@  int cmd_commit(int argc, const char **argv, const char *prefix)
 	git_test_write_commit_graph_or_die();
 
 	repo_rerere(the_repository, 0);
-	run_auto_maintenance(quiet);
+	run_auto_maintenance(quiet, NULL);
 	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
 		commit_post_rewrite(the_repository, current_head, &oid);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8e5e590dd6e..f32b24d182b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -2227,7 +2227,7 @@  int cmd_fetch(int argc, const char **argv, const char *prefix)
 	}
 
 	if (enable_auto_gc)
-		run_auto_maintenance(verbosity < 0);
+		run_auto_maintenance(verbosity < 0, NULL);
 
  cleanup:
 	string_list_clear(&list, 0);
diff --git a/builtin/merge.c b/builtin/merge.c
index a94a03384ae..8d3e6d0de03 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -472,7 +472,7 @@  static void finish(struct commit *head_commit,
 			 * We ignore errors in 'gc --auto', since the
 			 * user should see them.
 			 */
-			run_auto_maintenance(verbosity < 0);
+			run_auto_maintenance(verbosity < 0, NULL);
 		}
 	}
 	if (new_head && show_diffstat) {
diff --git a/builtin/rebase.c b/builtin/rebase.c
index d858add3fe8..cbab6c05373 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -552,7 +552,7 @@  static int finish_rebase(struct rebase_options *opts)
 	 * We ignore errors in 'git maintenance run --auto', since the
 	 * user should see them.
 	 */
-	run_auto_maintenance(!(opts->flags & (REBASE_NO_QUIET|REBASE_VERBOSE)));
+	run_auto_maintenance(!(opts->flags & (REBASE_NO_QUIET|REBASE_VERBOSE)), NULL);
 	if (opts->type == REBASE_MERGE) {
 		struct replay_opts replay = REPLAY_OPTS_INIT;
 
diff --git a/run-command.c b/run-command.c
index a8501e38ceb..720fd7820c8 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1798,9 +1798,10 @@  int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task,
 	return result;
 }
 
-int run_auto_maintenance(int quiet)
+int run_auto_maintenance(int quiet, const struct strvec *config_opts)
 {
 	int enabled;
+	int i;
 	struct child_process maint = CHILD_PROCESS_INIT;
 
 	if (!git_config_get_bool("maintenance.auto", &enabled) &&
@@ -1809,6 +1810,11 @@  int run_auto_maintenance(int quiet)
 
 	maint.git_cmd = 1;
 	maint.close_object_store = 1;
+
+	if (config_opts)
+		for (i = 0; i<config_opts->nr; i++)
+			strvec_pushl(&maint.args, "-c", config_opts->v[i], NULL);
+
 	strvec_pushl(&maint.args, "maintenance", "run", "--auto", NULL);
 	strvec_push(&maint.args, quiet ? "--quiet" : "--no-quiet");
 
diff --git a/run-command.h b/run-command.h
index 07bed6c31b4..24021abd41f 100644
--- a/run-command.h
+++ b/run-command.h
@@ -222,8 +222,11 @@  int run_command(struct child_process *);
 
 /*
  * Trigger an auto-gc
+ *
+ * config_opts is an optional list of additional config options to
+ * pass to the maintenance process in the form "some.option=value".
  */
-int run_auto_maintenance(int quiet);
+int run_auto_maintenance(int quiet, const struct strvec *config_opts);
 
 #define RUN_COMMAND_NO_STDIN		(1<<0)
 #define RUN_GIT_CMD			(1<<1)