[PATCH 0/7] builtin/commit-graph: some small cleanups

[PATCH 0/7] builtin/commit-graph: some small cleanups

[Patrick Steinhardt]

Hi,

I'm working into the direction of making commit-graphs pluggable so that
backends can fully control where exactly commit-graphs are written to
and read from. This patch series contains a couple of small preliminary
cleanups.

Most of the patches are rather uncontroversial, I'd assume. The only
one that might raise some eyebrows is where I "fix" git-commit-graph(1)
to only accept "--object-directory=" for subcommands instead of both as
a command-global and subcommand option. I feel like this is quite a weird
outlier, and it got in my way. In any case, I'd appreciate some extra
thoughts on this.

Thanks!

Patrick

---
Patrick Steinhardt (7):
      builtin/commit-graph: rename subcommand functions
      builtin/commit-graph: drop declaration for `read_replace_refs`
      builtin/commit-graph: get rid of `the_repository`
      builtin/commit-graph: drop global "--object-dir=" switch
      builtin/commit-graph: simplify how we wire up common options
      builtin/commit-graph: de-globalize write options handling
      builtin/commit-graph: de-globalize options handling

 builtin/commit-graph.c | 131 +++++++++++++++++++++++++------------------------
 1 file changed, 67 insertions(+), 64 deletions(-)


---
base-commit: 37b3475fb530c14109211faca33716a173db61ed
change-id: 20261028-pks-commit-graph-cleanups-d1dc30f3e05a

[PATCH 1/7] builtin/commit-graph: rename subcommand functions

[Patrick Steinhardt]

Subcommands that we wire up via `OPT_SUBCOMMAND()` are typically called
`cmd_<command>_<subcommand>()`, but we don't follow that pattern in
"builtin/commit-graph.c". Rename the functions.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/commit-graph.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index ff244c866fe..415eb6afee9 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -65,8 +65,10 @@ static struct option *add_common_options(struct option *to)
 	return parse_options_concat(common_opts, to);
 }
 
-static int graph_verify(int argc, const char **argv, const char *prefix,
-			struct repository *repo UNUSED)
+static int cmd_commit_graph_verify(int argc,
+				   const char **argv,
+				   const char *prefix,
+				   struct repository *repo UNUSED)
 {
 	struct commit_graph *graph = NULL;
 	struct odb_files_dir *dir = NULL;
@@ -224,8 +226,10 @@ static int git_commit_graph_write_config(const char *var, const char *value,
 	return 0;
 }
 
-static int graph_write(int argc, const char **argv, const char *prefix,
-		       struct repository *repo UNUSED)
+static int cmd_commit_graph_write(int argc,
+				  const char **argv,
+				  const char *prefix,
+				  struct repository *repo UNUSED)
 {
 	struct string_list pack_indexes = STRING_LIST_INIT_DUP;
 	struct strbuf buf = STRBUF_INIT;
@@ -362,8 +366,8 @@ int cmd_commit_graph(int argc,
 {
 	parse_opt_subcommand_fn *fn = NULL;
 	struct option builtin_commit_graph_options[] = {
-		OPT_SUBCOMMAND("verify", &fn, graph_verify),
-		OPT_SUBCOMMAND("write", &fn, graph_write),
+		OPT_SUBCOMMAND("verify", &fn, cmd_commit_graph_verify),
+		OPT_SUBCOMMAND("write", &fn, cmd_commit_graph_write),
 		OPT_END(),
 	};
 	struct option *options = parse_options_concat(builtin_commit_graph_options, common_opts);

-- 
2.56.0.341.g393f52fd96.dirty

[PATCH 2/7] builtin/commit-graph: drop declaration for `read_replace_refs`

[Patrick Steinhardt]

We have an `extern int read_replace_refs`, but that variable is not used
anymore since d24eda4e03c (repository: create disable_replace_refs(),
2023-06-06). In fact, that variable has been converted into a file-local
variable, and users are expected to call `disable_replace_refs()` now.

Remove the declaration.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/commit-graph.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 415eb6afee9..14f9db5456e 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -148,7 +148,6 @@ static int cmd_commit_graph_verify(int argc,
 	return ret;
 }
 
-extern int read_replace_refs;
 static struct commit_graph_opts write_opts;
 
 static int write_option_parse_split(const struct option *opt, const char *arg,

-- 
2.56.0.341.g393f52fd96.dirty

[PATCH 3/7] builtin/commit-graph: get rid of `the_repository`

[Patrick Steinhardt]

There are a couple of references to `the_repository` in
git-commit-graph(1). As that command cannot run outside of a repository
these references can be trivially replaced by the repository that's
passed in as parameter to `cmd_commit_graph()`.

We don't have to worry about the repository being `NULL` when called
with "-h", either: we do pass the repository to `repo_config()` before
calling `parse_options()`, but that function can handle a `NULL`
repository alright.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/commit-graph.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 14f9db5456e..f8825980f40 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -1,4 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
 #include "builtin.h"
 #include "commit.h"
 #include "config.h"
@@ -68,7 +67,7 @@ static struct option *add_common_options(struct option *to)
 static int cmd_commit_graph_verify(int argc,
 				   const char **argv,
 				   const char *prefix,
-				   struct repository *repo UNUSED)
+				   struct repository *repo)
 {
 	struct commit_graph *graph = NULL;
 	struct odb_files_dir *dir = NULL;
@@ -100,13 +99,13 @@ static int cmd_commit_graph_verify(int argc,
 		usage_with_options(builtin_commit_graph_verify_usage, options);
 
 	if (!opts.obj_dir)
-		opts.obj_dir = repo_get_object_directory(the_repository);
+		opts.obj_dir = repo_get_object_directory(repo);
 	if (opts.shallow)
 		flags |= COMMIT_GRAPH_VERIFY_SHALLOW;
 	if (opts.progress)
 		flags |= COMMIT_GRAPH_WRITE_PROGRESS;
 
-	dir = odb_source_files_find_dir(the_repository->objects, opts.obj_dir);
+	dir = odb_source_files_find_dir(repo->objects, opts.obj_dir);
 	if (!dir)
 		die(_("could not find object directory matching %s"), opts.obj_dir);
 
@@ -117,7 +116,7 @@ static int cmd_commit_graph_verify(int argc,
 	else if (errno != ENOENT)
 		die_errno(_("Could not open commit-graph '%s'"), graph_name);
 	else if (open_commit_graph_chain(chain_name, &fd, &st,
-					 the_repository->hash_algo))
+					 repo->hash_algo))
 		opened = OPENED_CHAIN;
 	else if (errno != ENOENT)
 		die_errno(_("could not open commit-graph chain '%s'"), chain_name);
@@ -129,9 +128,9 @@ static int cmd_commit_graph_verify(int argc,
 	if (opened == OPENED_NONE)
 		return 0;
 	else if (opened == OPENED_GRAPH)
-		graph = load_commit_graph_one_fd_st(the_repository, dir->abspath, fd, &st);
+		graph = load_commit_graph_one_fd_st(repo, dir->abspath, fd, &st);
 	else
-		graph = load_commit_graph_chain_fd_st(the_repository->objects, fd, &st,
+		graph = load_commit_graph_chain_fd_st(repo->objects, fd, &st,
 						      &incomplete_chain);
 
 	if (!graph)
@@ -171,17 +170,19 @@ static int write_option_parse_split(const struct option *opt, const char *arg,
 	return 0;
 }
 
-static int read_one_commit(struct oidset *commits, struct progress *progress,
+static int read_one_commit(struct repository *repo,
+			   struct oidset *commits,
+			   struct progress *progress,
 			   const char *hash)
 {
 	struct object *result;
 	struct object_id oid;
 	const char *end;
 
-	if (parse_oid_hex(hash, &oid, &end))
+	if (parse_oid_hex_algop(hash, &oid, &end, repo->hash_algo))
 		return error(_("unexpected non-hex object ID: %s"), hash);
 
-	result = deref_tag(the_repository, parse_object(the_repository, &oid),
+	result = deref_tag(repo, parse_object(repo, &oid),
 			   NULL, 0);
 	if (!result)
 		return error(_("invalid object: %s"), hash);
@@ -228,7 +229,7 @@ static int git_commit_graph_write_config(const char *var, const char *value,
 static int cmd_commit_graph_write(int argc,
 				  const char **argv,
 				  const char *prefix,
-				  struct repository *repo UNUSED)
+				  struct repository *repo)
 {
 	struct string_list pack_indexes = STRING_LIST_INIT_DUP;
 	struct strbuf buf = STRBUF_INIT;
@@ -277,7 +278,7 @@ static int cmd_commit_graph_write(int argc,
 
 	trace2_cmd_mode("write");
 
-	repo_config(the_repository, git_commit_graph_write_config, &opts);
+	repo_config(repo, git_commit_graph_write_config, &opts);
 
 	argc = parse_options(argc, argv, prefix,
 			     options,
@@ -288,7 +289,7 @@ static int cmd_commit_graph_write(int argc,
 	if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)
 		die(_("use at most one of --reachable, --stdin-commits, or --stdin-packs"));
 	if (!opts.obj_dir)
-		opts.obj_dir = repo_get_object_directory(the_repository);
+		opts.obj_dir = repo_get_object_directory(repo);
 	if (opts.append)
 		flags |= COMMIT_GRAPH_WRITE_APPEND;
 	if (opts.split)
@@ -301,12 +302,12 @@ static int cmd_commit_graph_write(int argc,
 	    git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0))
 		flags |= COMMIT_GRAPH_WRITE_BLOOM_FILTERS;
 
-	dir = odb_source_files_find_dir(the_repository->objects, opts.obj_dir);
+	dir = odb_source_files_find_dir(repo->objects, opts.obj_dir);
 	if (!dir)
 		die(_("could not find object directory matching %s"), opts.obj_dir);
 
 	if (opts.reachable) {
-		if (write_commit_graph_reachable(the_repository, dir->abspath, flags, &write_opts))
+		if (write_commit_graph_reachable(repo, dir->abspath, flags, &write_opts))
 			result = 1;
 		goto cleanup;
 	}
@@ -329,11 +330,11 @@ static int cmd_commit_graph_write(int argc,
 		oidset_init(&commits, 0);
 		if (opts.progress)
 			progress = start_delayed_progress(
-				the_repository,
+				repo,
 				_("Collecting commits from input"), 0);
 
 		while (strbuf_getline(&buf, stdin) != EOF) {
-			if (read_one_commit(&commits, progress, buf.buf)) {
+			if (read_one_commit(repo, &commits, progress, buf.buf)) {
 				result = 1;
 				stop_progress(&progress);
 				goto cleanup;
@@ -343,7 +344,7 @@ static int cmd_commit_graph_write(int argc,
 		stop_progress(&progress);
 	}
 
-	if (write_commit_graph(the_repository, dir->abspath,
+	if (write_commit_graph(repo, dir->abspath,
 			       opts.stdin_packs ? &pack_indexes : NULL,
 			       opts.stdin_commits ? &commits : NULL,
 			       flags,
@@ -371,7 +372,7 @@ int cmd_commit_graph(int argc,
 	};
 	struct option *options = parse_options_concat(builtin_commit_graph_options, common_opts);
 
-	repo_config(the_repository, git_default_config, NULL);
+	repo_config(repo, git_default_config, NULL);
 
 	disable_replace_refs();
 	save_commit_buffer = 0;

-- 
2.56.0.341.g393f52fd96.dirty

[PATCH 4/7] builtin/commit-graph: drop global "--object-dir=" switch

[Patrick Steinhardt]

When invoking git-commit-graph(1) it is possible for the user to pass
the "--object-dir=" switch either before or after the subcommand: both
"git commit-graph --object-dir= write" and "git commit-graph write
--object-dir=" do the same thing. This calling convention is very
atypical in Git, we don't document this and none of our tests even use
the former syntax.

Furthermore, this requires us to keep track of the object directory via
global state as we don't have an easy way to pass the parsed option down
to the individual subcommands. We _could_ of course adapt the logic and
prepend the object directory to the `argv` we end up passing down. But
that feels rather pointless.

Adapt the code to only allow the "--object-dir=" option for subcommands.
This brings the command in line with how Git typically handles options,
and it avoids adding complexity in subsequent commits where we're about
to remove all global state in "builtin/commit-graph.c".

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/commit-graph.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index f8825980f40..7140433a39a 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -365,12 +365,11 @@ int cmd_commit_graph(int argc,
 		     struct repository *repo)
 {
 	parse_opt_subcommand_fn *fn = NULL;
-	struct option builtin_commit_graph_options[] = {
+	struct option options[] = {
 		OPT_SUBCOMMAND("verify", &fn, cmd_commit_graph_verify),
 		OPT_SUBCOMMAND("write", &fn, cmd_commit_graph_write),
 		OPT_END(),
 	};
-	struct option *options = parse_options_concat(builtin_commit_graph_options, common_opts);
 
 	repo_config(repo, git_default_config, NULL);
 
@@ -379,7 +378,6 @@ int cmd_commit_graph(int argc,
 
 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_commit_graph_usage, 0);
-	FREE_AND_NULL(options);
 
 	return fn(argc, argv, prefix, repo);
 }

-- 
2.56.0.341.g393f52fd96.dirty

[PATCH 5/7] builtin/commit-graph: simplify how we wire up common options

[Patrick Steinhardt]

All of the git-commit-graph(1) subcommands accept the "--object-dir="
option. To avoid duplication we thus have a helper function that adds
this common option to our array of command-specific options. This works
alright, but it feels rather awkward as this forces us to allocate and
free a secondary array of options.

Refactor the code to instead have common options as a preprocessor
macro that we can add. This avoids the extra allocation and is overall a
bit less verbose.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/commit-graph.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 7140433a39a..165dfcb827f 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -52,17 +52,10 @@ static struct opts_commit_graph {
 	int enable_changed_paths;
 } opts;
 
-static struct option common_opts[] = {
-	OPT_STRING(0, "object-dir", &opts.obj_dir,
-		   N_("dir"),
-		   N_("the object directory to store the graph")),
-	OPT_END()
-};
-
-static struct option *add_common_options(struct option *to)
-{
-	return parse_options_concat(common_opts, to);
-}
+#define COMMIT_GRAPH_COMMON_OPTIONS(opts) \
+	OPT_STRING(0, "object-dir", &(opts)->obj_dir, \
+		   N_("dir"), \
+		   N_("the object directory to store the graph"))
 
 static int cmd_commit_graph_verify(int argc,
 				   const char **argv,
@@ -79,15 +72,14 @@ static int cmd_commit_graph_verify(int argc,
 	int flags = 0;
 	int incomplete_chain = 0;
 	int ret;
-
-	static struct option builtin_commit_graph_verify_options[] = {
+	static struct option options[] = {
 		OPT_BOOL(0, "shallow", &opts.shallow,
 			 N_("if the commit-graph is split, only verify the tip file")),
 		OPT_BOOL(0, "progress", &opts.progress,
 			 N_("force progress reporting")),
+		COMMIT_GRAPH_COMMON_OPTIONS(&opts),
 		OPT_END(),
 	};
-	struct option *options = add_common_options(builtin_commit_graph_verify_options);
 
 	trace2_cmd_mode("verify");
 
@@ -123,7 +115,6 @@ static int cmd_commit_graph_verify(int argc,
 
 	FREE_AND_NULL(graph_name);
 	FREE_AND_NULL(chain_name);
-	FREE_AND_NULL(options);
 
 	if (opened == OPENED_NONE)
 		return 0;
@@ -238,8 +229,7 @@ static int cmd_commit_graph_write(int argc,
 	int result = 0;
 	enum commit_graph_write_flags flags = 0;
 	struct progress *progress = NULL;
-
-	static struct option builtin_commit_graph_write_options[] = {
+	static struct option options[] = {
 		OPT_BOOL(0, "reachable", &opts.reachable,
 			N_("start walk at all refs")),
 		OPT_BOOL(0, "stdin-packs", &opts.stdin_packs,
@@ -265,9 +255,9 @@ static int cmd_commit_graph_write(int argc,
 			0, write_option_max_new_filters),
 		OPT_BOOL(0, "progress", &opts.progress,
 			 N_("force progress reporting")),
+		COMMIT_GRAPH_COMMON_OPTIONS(&opts),
 		OPT_END(),
 	};
-	struct option *options = add_common_options(builtin_commit_graph_write_options);
 
 	opts.progress = isatty(2);
 	opts.enable_changed_paths = -1;
@@ -352,7 +342,6 @@ static int cmd_commit_graph_write(int argc,
 		result = 1;
 
 cleanup:
-	FREE_AND_NULL(options);
 	string_list_clear(&pack_indexes, 0);
 	strbuf_release(&buf);
 	oidset_clear(&commits);

-- 
2.56.0.341.g393f52fd96.dirty

[PATCH 6/7] builtin/commit-graph: de-globalize write options handling

[Patrick Steinhardt]

The commit graph write options are stored as a global variable even
though they're really only required by `graph_write()` in the first
place. The only other function that needs access to it is our config
parser, but we can trivially pass it through to it via the callback
data.

Refactor the code accordingly. Note that as part of this refactoring we
introduce a struct dedicated to parsing the configuration, and that
struct only contains the write options. It's thus a bit heavy handed
right now, but we're about to add a second member in the next commit.

Note that this requires us to drop the `static` modifier for our options
array as it now references a local variable.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/commit-graph.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 165dfcb827f..2126619ad3e 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -138,7 +138,9 @@ static int cmd_commit_graph_verify(int argc,
 	return ret;
 }
 
-static struct commit_graph_opts write_opts;
+struct commit_graph_write_config_data {
+	struct commit_graph_opts *write_opts;
+};
 
 static int write_option_parse_split(const struct option *opt, const char *arg,
 				    int unset)
@@ -204,10 +206,12 @@ static int write_option_max_new_filters(const struct option *opt,
 
 static int git_commit_graph_write_config(const char *var, const char *value,
 					 const struct config_context *ctx,
-					 void *cb UNUSED)
+					 void *cb_data)
 {
+	struct commit_graph_write_config_data *data = cb_data;
+
 	if (!strcmp(var, "commitgraph.maxnewfilters"))
-		write_opts.max_new_filters = git_config_int(var, value, ctx->kvi);
+		data->write_opts->max_new_filters = git_config_int(var, value, ctx->kvi);
 	else if (!strcmp(var, "commitgraph.changedpaths"))
 		opts.enable_changed_paths = git_config_bool(var, value) ? 1 : -1;
 	/*
@@ -222,6 +226,13 @@ static int cmd_commit_graph_write(int argc,
 				  const char *prefix,
 				  struct repository *repo)
 {
+	struct commit_graph_opts write_opts = {
+		.size_multiple = 2,
+		.max_new_filters = -1,
+	};
+	struct commit_graph_write_config_data config_data = {
+		.write_opts = &write_opts,
+	};
 	struct string_list pack_indexes = STRING_LIST_INIT_DUP;
 	struct strbuf buf = STRBUF_INIT;
 	struct oidset commits = OIDSET_INIT;
@@ -229,7 +240,7 @@ static int cmd_commit_graph_write(int argc,
 	int result = 0;
 	enum commit_graph_write_flags flags = 0;
 	struct progress *progress = NULL;
-	static struct option options[] = {
+	struct option options[] = {
 		OPT_BOOL(0, "reachable", &opts.reachable,
 			N_("start walk at all refs")),
 		OPT_BOOL(0, "stdin-packs", &opts.stdin_packs,
@@ -261,14 +272,10 @@ static int cmd_commit_graph_write(int argc,
 
 	opts.progress = isatty(2);
 	opts.enable_changed_paths = -1;
-	write_opts.size_multiple = 2;
-	write_opts.max_commits = 0;
-	write_opts.expire_time = 0;
-	write_opts.max_new_filters = -1;
 
 	trace2_cmd_mode("write");
 
-	repo_config(repo, git_commit_graph_write_config, &opts);
+	repo_config(repo, git_commit_graph_write_config, &config_data);
 
 	argc = parse_options(argc, argv, prefix,
 			     options,

-- 
2.56.0.341.g393f52fd96.dirty

[PATCH 7/7] builtin/commit-graph: de-globalize options handling

[Patrick Steinhardt]

Same as in the preceding commit, de-globalize handling of options that
are common to both the "verify" and "write" subcommands. With this
change we don't have any global state in "builtin/commit-graph.c"
anymore.

Same as in the preceding commit, this requires us to drop the `static`
modifier for the options array as it references a local variable now.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/commit-graph.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 2126619ad3e..4d28a9d51ec 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -40,7 +40,7 @@ static char const * const builtin_commit_graph_usage[] = {
 	NULL,
 };
 
-static struct opts_commit_graph {
+struct opts_commit_graph {
 	const char *obj_dir;
 	int reachable;
 	int stdin_packs;
@@ -50,7 +50,7 @@ static struct opts_commit_graph {
 	int shallow;
 	int progress;
 	int enable_changed_paths;
-} opts;
+};
 
 #define COMMIT_GRAPH_COMMON_OPTIONS(opts) \
 	OPT_STRING(0, "object-dir", &(opts)->obj_dir, \
@@ -62,6 +62,9 @@ static int cmd_commit_graph_verify(int argc,
 				   const char *prefix,
 				   struct repository *repo)
 {
+	struct opts_commit_graph opts = {
+		.progress = isatty(2),
+	};
 	struct commit_graph *graph = NULL;
 	struct odb_files_dir *dir = NULL;
 	char *graph_name;
@@ -72,7 +75,7 @@ static int cmd_commit_graph_verify(int argc,
 	int flags = 0;
 	int incomplete_chain = 0;
 	int ret;
-	static struct option options[] = {
+	struct option options[] = {
 		OPT_BOOL(0, "shallow", &opts.shallow,
 			 N_("if the commit-graph is split, only verify the tip file")),
 		OPT_BOOL(0, "progress", &opts.progress,
@@ -83,7 +86,6 @@ static int cmd_commit_graph_verify(int argc,
 
 	trace2_cmd_mode("verify");
 
-	opts.progress = isatty(2);
 	argc = parse_options(argc, argv, prefix,
 			     options,
 			     builtin_commit_graph_verify_usage, 0);
@@ -140,23 +142,24 @@ static int cmd_commit_graph_verify(int argc,
 
 struct commit_graph_write_config_data {
 	struct commit_graph_opts *write_opts;
+	struct opts_commit_graph *opts;
 };
 
 static int write_option_parse_split(const struct option *opt, const char *arg,
 				    int unset)
 {
-	enum commit_graph_split_flags *flags = opt->value;
+	struct commit_graph_write_config_data *data = opt->value;
 
 	BUG_ON_OPT_NEG(unset);
 
-	opts.split = 1;
+	data->opts->split = 1;
 	if (!arg)
 		return 0;
 
 	if (!strcmp(arg, "no-merge"))
-		*flags = COMMIT_GRAPH_SPLIT_MERGE_PROHIBITED;
+		data->write_opts->split_flags = COMMIT_GRAPH_SPLIT_MERGE_PROHIBITED;
 	else if (!strcmp(arg, "replace"))
-		*flags = COMMIT_GRAPH_SPLIT_REPLACE;
+		data->write_opts->split_flags = COMMIT_GRAPH_SPLIT_REPLACE;
 	else
 		die(_("unrecognized --split argument, %s"), arg);
 
@@ -213,7 +216,7 @@ static int git_commit_graph_write_config(const char *var, const char *value,
 	if (!strcmp(var, "commitgraph.maxnewfilters"))
 		data->write_opts->max_new_filters = git_config_int(var, value, ctx->kvi);
 	else if (!strcmp(var, "commitgraph.changedpaths"))
-		opts.enable_changed_paths = git_config_bool(var, value) ? 1 : -1;
+		data->opts->enable_changed_paths = git_config_bool(var, value) ? 1 : -1;
 	/*
 	 * No need to fall-back to 'git_default_config', since this was already
 	 * called in 'cmd_commit_graph()'.
@@ -230,8 +233,13 @@ static int cmd_commit_graph_write(int argc,
 		.size_multiple = 2,
 		.max_new_filters = -1,
 	};
+	struct opts_commit_graph opts = {
+		.progress = isatty(2),
+		.enable_changed_paths = -1,
+	};
 	struct commit_graph_write_config_data config_data = {
 		.write_opts = &write_opts,
+		.opts = &opts,
 	};
 	struct string_list pack_indexes = STRING_LIST_INIT_DUP;
 	struct strbuf buf = STRBUF_INIT;
@@ -251,7 +259,7 @@ static int cmd_commit_graph_write(int argc,
 			N_("include all commits already in the commit-graph file")),
 		OPT_BOOL(0, "changed-paths", &opts.enable_changed_paths,
 			N_("enable computation for changed paths")),
-		OPT_CALLBACK_F(0, "split", &write_opts.split_flags, NULL,
+		OPT_CALLBACK_F(0, "split", &config_data, NULL,
 			N_("allow writing an incremental commit-graph file"),
 			PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
 			write_option_parse_split),
@@ -270,9 +278,6 @@ static int cmd_commit_graph_write(int argc,
 		OPT_END(),
 	};
 
-	opts.progress = isatty(2);
-	opts.enable_changed_paths = -1;
-
 	trace2_cmd_mode("write");
 
 	repo_config(repo, git_commit_graph_write_config, &config_data);

-- 
2.56.0.341.g393f52fd96.dirty

Re: [PATCH 7/7] builtin/commit-graph: de-globalize options handling

[Mark C. Chu-Carroll]

This all looks correct and unobjectionable to me.

    -Mark


-- 
Mark Craig Chu-Carroll (@MarkChuCarroll at gitlab)
*** Software Tools/Math Geek - Software Engineer at Gitlab
*** Work Email: mcarroll@gitlab.com / markchucarroll@fastmail.com
*** Personal Blog: http://goodmath.org/blog / Personal email: markcc@gmail.com

Re: [PATCH 0/7] builtin/commit-graph: some small cleanups

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Hi,
>
> I'm working into the direction of making commit-graphs pluggable so that
> backends can fully control where exactly commit-graphs are written to
> and read from. This patch series contains a couple of small preliminary
> cleanups.
>
> Most of the patches are rather uncontroversial, I'd assume. The only
> one that might raise some eyebrows is where I "fix" git-commit-graph(1)
> to only accept "--object-directory=" for subcommands instead of both as
> a command-global and subcommand option. I feel like this is quite a weird
> outlier, and it got in my way. In any case, I'd appreciate some extra
> thoughts on this.
>
> Thanks!
>
> Patrick

I read the first 4 steps and they all looked sensible.  May come
back to the remainder later this week if nobody beats me to it.

Thanks.

>
> ---
> Patrick Steinhardt (7):
>       builtin/commit-graph: rename subcommand functions
>       builtin/commit-graph: drop declaration for `read_replace_refs`
>       builtin/commit-graph: get rid of `the_repository`
>       builtin/commit-graph: drop global "--object-dir=" switch
>       builtin/commit-graph: simplify how we wire up common options
>       builtin/commit-graph: de-globalize write options handling
>       builtin/commit-graph: de-globalize options handling
>
>  builtin/commit-graph.c | 131 +++++++++++++++++++++++++------------------------
>  1 file changed, 67 insertions(+), 64 deletions(-)
>
>
> ---
> base-commit: 37b3475fb530c14109211faca33716a173db61ed
> change-id: 20261028-pks-commit-graph-cleanups-d1dc30f3e05a

Re: [PATCH 0/7] builtin/commit-graph: some small cleanups

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I read the first 4 steps and they all looked sensible.  May come
> back to the remainder later this week if nobody beats me to it.
>
> Thanks.

As to the rest, quite honestly, [6/7] and [7/7] were "Meh", even
though I do not think they hurt [*].

All the rest looked good to me.


[Footnote]

 * Unless these things are lifted out of bulitin/* hierarchy, there
   is no strong reason to avoid relying on global variables or even
   the_repository.  Once we move things out of there to make them
   truly reusable, these things start to matter, but until then...