diff mbox series

[v3,13/14] commit-graph: rename 'split_commit_graph_opts'

Message ID b2e33ecba880f4b49af7a94fb0decb08929dff36.1597178915.git.me@ttaylorr.com (mailing list archive)
State Superseded
Headers show
Series more miscellaneous Bloom filter improvements | expand

Commit Message

Taylor Blau Aug. 11, 2020, 8:52 p.m. UTC 
In the subsequent commit, additional options will be added to the
commit-graph API which have nothing to do with splitting.

Rename the 'split_commit_graph_opts' structure to the more-generic
'commit_graph_opts' to encompass both.

Suggsted-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/commit-graph.c | 20 ++++++++++----------
 commit-graph.c         | 40 ++++++++++++++++++++--------------------
 commit-graph.h         |  6 +++---
 3 files changed, 33 insertions(+), 33 deletions(-)

Comments

SZEDER Gábor Aug. 19, 2020, 9:56 a.m. UTC | #1 
On Tue, Aug 11, 2020 at 04:52:11PM -0400, Taylor Blau wrote:
> In the subsequent commit, additional options will be added to the
> commit-graph API which have nothing to do with splitting.
> 
> Rename the 'split_commit_graph_opts' structure to the more-generic
> 'commit_graph_opts' to encompass both.

Good.  Note, however, that write_commit_graph() has a 'flags'
parameter as well, and when a feature is enabled via this 'flags',
then first a corresponding 'ctx->foo' field is set, and that
'ctx->foo' is checked while computing and writing the commit-graph.
With the generic options struct some other feature will be enabled via
the 'opts->bar' field, so simply 'ctx->opts->bar' is checked while
writing the commit-graph.

With the generic options struct there really is no need for a separate
flags parameter, the values in the flags can be stored in the options
struct, and we can eliminate this inconsistency instead of adding even
more.


> diff --git a/commit-graph.h b/commit-graph.h
> index ddbca1b59d..af08c4505d 100644
> --- a/commit-graph.h
> +++ b/commit-graph.h
> @@ -109,7 +109,7 @@ enum commit_graph_split_flags {
>  	COMMIT_GRAPH_SPLIT_REPLACE          = 2
>  };
>  
> -struct split_commit_graph_opts {
> +struct commit_graph_opts {
>  	int size_multiple;
>  	int max_commits;
>  	timestamp_t expire_time;
        enum commit_graph_split_flags flags;

While this was 'struct split_commit_graph_opts *split_opts' it was
clear what kind of flags were in this 'flags' field.  Now that the
struct is generic it's not clear anymore, so perhaps it should be
renamed as well (e.g. 'split_flags'), or even turned into a couple of
bit fields.

> @@ -124,12 +124,12 @@ struct split_commit_graph_opts {
>   */
>  int write_commit_graph_reachable(struct object_directory *odb,
>  				 enum commit_graph_write_flags flags,
> -				 const struct split_commit_graph_opts *split_opts);
> +				 const struct commit_graph_opts *opts);
>  int write_commit_graph(struct object_directory *odb,
>  		       struct string_list *pack_indexes,
>  		       struct oidset *commits,
>  		       enum commit_graph_write_flags flags,
> -		       const struct split_commit_graph_opts *split_opts);
> +		       const struct commit_graph_opts *opts);
>  
>  #define COMMIT_GRAPH_VERIFY_SHALLOW	(1 << 0)
>  
> -- 
> 2.28.0.rc1.13.ge78abce653
>
Taylor Blau Sept. 2, 2020, 9:02 p.m. UTC | #2 
On Wed, Aug 19, 2020 at 11:56:56AM +0200, SZEDER Gábor wrote:
> On Tue, Aug 11, 2020 at 04:52:11PM -0400, Taylor Blau wrote:
> > In the subsequent commit, additional options will be added to the
> > commit-graph API which have nothing to do with splitting.
> >
> > Rename the 'split_commit_graph_opts' structure to the more-generic
> > 'commit_graph_opts' to encompass both.
>
> Good.  Note, however, that write_commit_graph() has a 'flags'
> parameter as well, and when a feature is enabled via this 'flags',
> then first a corresponding 'ctx->foo' field is set, and that
> 'ctx->foo' is checked while computing and writing the commit-graph.
> With the generic options struct some other feature will be enabled via
> the 'opts->bar' field, so simply 'ctx->opts->bar' is checked while
> writing the commit-graph.
>
> With the generic options struct there really is no need for a separate
> flags parameter, the values in the flags can be stored in the options
> struct, and we can eliminate this inconsistency instead of adding even
> more.

I like the direction that you're headed in, but I'm not entirely sure
what you're suggesting. Do you want to make the
'enum commit_graph_write_flags flags' part of the new options struct?
Break out the fields into individual bits on that struct?

I'm not opposed to either, but note that there is also already a 'flags'
field on the options structure related to splitting, so that would have
to be untangled, too.

What I'm trying to say is that I think there's more complexity here than
you're giving it credit for. I'd rather press on with what we have here,
and devote adequate time to unraveling the complexity appropriately than
try to shove in another patch that takes a half-step in the right
direction.

>
> > diff --git a/commit-graph.h b/commit-graph.h
> > index ddbca1b59d..af08c4505d 100644
> > --- a/commit-graph.h
> > +++ b/commit-graph.h
> > @@ -109,7 +109,7 @@ enum commit_graph_split_flags {
> >  	COMMIT_GRAPH_SPLIT_REPLACE          = 2
> >  };
> >
> > -struct split_commit_graph_opts {
> > +struct commit_graph_opts {
> >  	int size_multiple;
> >  	int max_commits;
> >  	timestamp_t expire_time;
>         enum commit_graph_split_flags flags;
>
> While this was 'struct split_commit_graph_opts *split_opts' it was
> clear what kind of flags were in this 'flags' field.  Now that the
> struct is generic it's not clear anymore, so perhaps it should be
> renamed as well (e.g. 'split_flags'), or even turned into a couple of
> bit fields.

This I can definitely vouch for, so I'll 's/flags/split_flags' in the
next revision.

Thanks,
Taylor
diff mbox series

Patch

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index ba5584463f..38f5f57d15 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -119,7 +119,7 @@  static int graph_verify(int argc, const char **argv)
 }
 
 extern int read_replace_refs;
-static struct split_commit_graph_opts split_opts;
+static struct commit_graph_opts write_opts;
 
 static int write_option_parse_split(const struct option *opt, const char *arg,
 				    int unset)
@@ -187,24 +187,24 @@  static int graph_write(int argc, const char **argv)
 		OPT_BOOL(0, "changed-paths", &opts.enable_changed_paths,
 			N_("enable computation for changed paths")),
 		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
-		OPT_CALLBACK_F(0, "split", &split_opts.flags, NULL,
+		OPT_CALLBACK_F(0, "split", &write_opts.flags, NULL,
 			N_("allow writing an incremental commit-graph file"),
 			PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
 			write_option_parse_split),
-		OPT_INTEGER(0, "max-commits", &split_opts.max_commits,
+		OPT_INTEGER(0, "max-commits", &write_opts.max_commits,
 			N_("maximum number of commits in a non-base split commit-graph")),
-		OPT_INTEGER(0, "size-multiple", &split_opts.size_multiple,
+		OPT_INTEGER(0, "size-multiple", &write_opts.size_multiple,
 			N_("maximum ratio between two levels of a split commit-graph")),
-		OPT_EXPIRY_DATE(0, "expire-time", &split_opts.expire_time,
+		OPT_EXPIRY_DATE(0, "expire-time", &write_opts.expire_time,
 			N_("only expire files older than a given date-time")),
 		OPT_END(),
 	};
 
 	opts.progress = isatty(2);
 	opts.enable_changed_paths = -1;
-	split_opts.size_multiple = 2;
-	split_opts.max_commits = 0;
-	split_opts.expire_time = 0;
+	write_opts.size_multiple = 2;
+	write_opts.max_commits = 0;
+	write_opts.expire_time = 0;
 
 	trace2_cmd_mode("write");
 
@@ -232,7 +232,7 @@  static int graph_write(int argc, const char **argv)
 	odb = find_odb(the_repository, opts.obj_dir);
 
 	if (opts.reachable) {
-		if (write_commit_graph_reachable(odb, flags, &split_opts))
+		if (write_commit_graph_reachable(odb, flags, &write_opts))
 			return 1;
 		return 0;
 	}
@@ -261,7 +261,7 @@  static int graph_write(int argc, const char **argv)
 			       opts.stdin_packs ? &pack_indexes : NULL,
 			       opts.stdin_commits ? &commits : NULL,
 			       flags,
-			       &split_opts))
+			       &write_opts))
 		result = 1;
 
 cleanup:
diff --git a/commit-graph.c b/commit-graph.c
index ea0583298c..6886f319a5 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1001,7 +1001,7 @@  struct write_commit_graph_context {
 		 changed_paths:1,
 		 order_by_pack:1;
 
-	const struct split_commit_graph_opts *split_opts;
+	const struct commit_graph_opts *opts;
 	size_t total_bloom_filter_data_size;
 	const struct bloom_filter_settings *bloom_settings;
 
@@ -1342,8 +1342,8 @@  static void close_reachable(struct write_commit_graph_context *ctx)
 {
 	int i;
 	struct commit *commit;
-	enum commit_graph_split_flags flags = ctx->split_opts ?
-		ctx->split_opts->flags : COMMIT_GRAPH_SPLIT_UNSPECIFIED;
+	enum commit_graph_split_flags flags = ctx->opts ?
+		ctx->opts->flags : COMMIT_GRAPH_SPLIT_UNSPECIFIED;
 
 	if (ctx->report_progress)
 		ctx->progress = start_delayed_progress(
@@ -1543,7 +1543,7 @@  static int add_ref_to_set(const char *refname,
 
 int write_commit_graph_reachable(struct object_directory *odb,
 				 enum commit_graph_write_flags flags,
-				 const struct split_commit_graph_opts *split_opts)
+				 const struct commit_graph_opts *opts)
 {
 	struct oidset commits = OIDSET_INIT;
 	struct refs_cb_data data;
@@ -1560,7 +1560,7 @@  int write_commit_graph_reachable(struct object_directory *odb,
 	stop_progress(&data.progress);
 
 	result = write_commit_graph(odb, NULL, &commits,
-				    flags, split_opts);
+				    flags, opts);
 
 	oidset_clear(&commits);
 	return result;
@@ -1675,8 +1675,8 @@  static uint32_t count_distinct_commits(struct write_commit_graph_context *ctx)
 static void copy_oids_to_commits(struct write_commit_graph_context *ctx)
 {
 	uint32_t i;
-	enum commit_graph_split_flags flags = ctx->split_opts ?
-		ctx->split_opts->flags : COMMIT_GRAPH_SPLIT_UNSPECIFIED;
+	enum commit_graph_split_flags flags = ctx->opts ?
+		ctx->opts->flags : COMMIT_GRAPH_SPLIT_UNSPECIFIED;
 
 	ctx->num_extra_edges = 0;
 	if (ctx->report_progress)
@@ -1962,13 +1962,13 @@  static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)
 	int max_commits = 0;
 	int size_mult = 2;
 
-	if (ctx->split_opts) {
-		max_commits = ctx->split_opts->max_commits;
+	if (ctx->opts) {
+		max_commits = ctx->opts->max_commits;
 
-		if (ctx->split_opts->size_multiple)
-			size_mult = ctx->split_opts->size_multiple;
+		if (ctx->opts->size_multiple)
+			size_mult = ctx->opts->size_multiple;
 
-		flags = ctx->split_opts->flags;
+		flags = ctx->opts->flags;
 	}
 
 	g = ctx->r->objects->commit_graph;
@@ -2146,8 +2146,8 @@  static void expire_commit_graphs(struct write_commit_graph_context *ctx)
 	size_t dirnamelen;
 	timestamp_t expire_time = time(NULL);
 
-	if (ctx->split_opts && ctx->split_opts->expire_time)
-		expire_time = ctx->split_opts->expire_time;
+	if (ctx->opts && ctx->opts->expire_time)
+		expire_time = ctx->opts->expire_time;
 	if (!ctx->split) {
 		char *chain_file_name = get_chain_filename(ctx->odb);
 		unlink(chain_file_name);
@@ -2198,7 +2198,7 @@  int write_commit_graph(struct object_directory *odb,
 		       struct string_list *pack_indexes,
 		       struct oidset *commits,
 		       enum commit_graph_write_flags flags,
-		       const struct split_commit_graph_opts *split_opts)
+		       const struct commit_graph_opts *opts)
 {
 	struct write_commit_graph_context *ctx;
 	uint32_t i, count_distinct = 0;
@@ -2215,7 +2215,7 @@  int write_commit_graph(struct object_directory *odb,
 	ctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;
 	ctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;
 	ctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;
-	ctx->split_opts = split_opts;
+	ctx->opts = opts;
 	ctx->total_bloom_filter_data_size = 0;
 
 	bloom_settings.bits_per_entry = git_env_ulong("GIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY",
@@ -2263,15 +2263,15 @@  int write_commit_graph(struct object_directory *odb,
 			}
 		}
 
-		if (ctx->split_opts)
-			replace = ctx->split_opts->flags & COMMIT_GRAPH_SPLIT_REPLACE;
+		if (ctx->opts)
+			replace = ctx->opts->flags & COMMIT_GRAPH_SPLIT_REPLACE;
 	}
 
 	ctx->approx_nr_objects = approximate_object_count();
 	ctx->oids.alloc = ctx->approx_nr_objects / 32;
 
-	if (ctx->split && split_opts && ctx->oids.alloc > split_opts->max_commits)
-		ctx->oids.alloc = split_opts->max_commits;
+	if (ctx->split && opts && ctx->oids.alloc > opts->max_commits)
+		ctx->oids.alloc = opts->max_commits;
 
 	if (ctx->append) {
 		prepare_commit_graph_one(ctx->r, ctx->odb);
diff --git a/commit-graph.h b/commit-graph.h
index ddbca1b59d..af08c4505d 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -109,7 +109,7 @@  enum commit_graph_split_flags {
 	COMMIT_GRAPH_SPLIT_REPLACE          = 2
 };
 
-struct split_commit_graph_opts {
+struct commit_graph_opts {
 	int size_multiple;
 	int max_commits;
 	timestamp_t expire_time;
@@ -124,12 +124,12 @@  struct split_commit_graph_opts {
  */
 int write_commit_graph_reachable(struct object_directory *odb,
 				 enum commit_graph_write_flags flags,
-				 const struct split_commit_graph_opts *split_opts);
+				 const struct commit_graph_opts *opts);
 int write_commit_graph(struct object_directory *odb,
 		       struct string_list *pack_indexes,
 		       struct oidset *commits,
 		       enum commit_graph_write_flags flags,
-		       const struct split_commit_graph_opts *split_opts);
+		       const struct commit_graph_opts *opts);
 
 #define COMMIT_GRAPH_VERIFY_SHALLOW	(1 << 0)