Re: [PATCH 06/10] commit-graph: move remaining write parameters into options

[Patrick Steinhardt <ps@pks.im>]

On Wed, Nov 04, 2026 at 06:49:50PM +0100, Kristofer Karlsson wrote:
> On Wed, 4 Nov 2026 at 17:15, Patrick Steinhardt <ps@pks.im> wrote:
> >
> >                 if (graph_write_mode != GRAPH_WRITE_TIPS)
> >                         write_opts.flags |= COMMIT_GRAPH_WRITE_REACHABLE;
> > +               else
> > +                       write_opts.commits = &updated_tips;
> 
> Ah, this touches on my previous patch cef2d8c4cb
> (fetch: write commit-graph using updated refs only, 2026-10-07).
> 
> My only nit would be that the if/else also covers the skip case,
> so one of the branches always fires for it.  A switch would make
> each mode set exactly what it needs:
> 
>   switch (graph_write_mode) {
>   case GRAPH_WRITE_SKIP:
>       break;
>   case GRAPH_WRITE_REACHABLE:
>       write_opts.flags |= COMMIT_GRAPH_WRITE_REACHABLE;
>       break;
>   case GRAPH_WRITE_TIPS:
>       write_opts.commits = &updated_tips;
>       break;
>   }
> 
> It doesn't matter for correctness since the skip case never calls
> the writer, but it looks easier to reason about to me.
> 
> But that's just a preference, I don't feel strongly about it.

I was wondering whether we want to maybe introduce a tagged union where
it becomes implicitly clear that some of the options only apply to a
specific mode. Something like this:

diff --git a/commit-graph.h b/commit-graph.h
index 2156499a688..8d53f80f179 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -160,7 +160,6 @@ enum commit_graph_write_flags {
 	COMMIT_GRAPH_WRITE_SPLIT      = (1 << 2),
 	COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3),
 	COMMIT_GRAPH_NO_WRITE_BLOOM_FILTERS = (1 << 4),
-	COMMIT_GRAPH_WRITE_REACHABLE  = (1 << 5),
 };
 
 enum commit_graph_split_flags {
@@ -170,9 +169,18 @@ enum commit_graph_split_flags {
 };
 
 struct commit_graph_write_options {
+	enum commit_graph_write_mode {
+		COMMIT_GRAPH_WRITE_ALL,
+		COMMIT_GRAPH_WRITE_REACHABLE,
+		COMMIT_GRAPH_WRITE_PACKS_INDEXES,
+		COMMIT_GRAPH_WRITE_COMMITS,
+	} mode;
+	union {
+		const struct string_list *pack_indexes;
+		struct oidset *commits;
+	} mode_args;
+
 	enum commit_graph_write_flags flags;
-	const struct string_list *pack_indexes;
-	struct oidset *commits;
 	int size_multiple;
 	int max_commits;
 	timestamp_t expire_time;

I wasn't really sure whether that complexity is really worth it, so for
now I decided to not pursue it. I think I still tend to stick with that
decision for now, but it's maybe something to keep in mind for the
future?

Patrick