Re: [PATCH v3 2/2] merge: remember conflict labels

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

> @@ -347,10 +349,19 @@ static int checkout_merged(int pos, const struct checkout *state,
>  
>  	repo_config_get_bool(the_repository, "merge.renormalize", &renormalize);
>  	ll_opts.renormalize = renormalize;
> +	if (read_merge_labels(the_repository, &base_label, &ours_label,
> +			      &theirs_label)) {
> +		base_label = xstrdup("base");
> +		ours_label = xstrdup("ours");
> +		theirs_label = xstrdup("theirs");
> +	}
>  	ll_opts.conflict_style = conflict_style;
> -	merge_status = ll_merge(&result_buf, path, &ancestor, "base",
> -				&ours, "ours", &theirs, "theirs",
> +	merge_status = ll_merge(&result_buf, path, &ancestor, base_label,
> +				&ours, ours_label, &theirs, theirs_label,
>  				state->istate, &ll_opts);
> +	free(base_label);
> +	free(ours_label);
> +	free(theirs_label);
>  	free(ancestor.ptr);
>  	free(ours.ptr);
>  	free(theirs.ptr);


This function is called once for each conflicted path, which means
we would read the "merge.renormalize" configuration variable and the
MERGE_LABELS file, both of which will stay constant during a single
conflicted "checkout -m".  The issue is shared with the original,
but looking up the same configuration variable repeatedly would be
helped with in-core configset cache.  Compared to that, the overhead
added by this patch is to open the same unchanging file, read & parse,
allocate and deallocate.

Perhaps we want to have another preliminary [PATCH 1.5/2] before
this step to allow setting these "per invocation constants" once to
be reused?  Then step [PATCH 2/2] can read labels in the prepare
phace just once, use it from the data structure in
checkout_merged(), and free them in release phase when we are done.


 builtin/checkout.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git i/builtin/checkout.c w/builtin/checkout.c
index c0f0d2c700..a02cbacf1d 100644
--- i/builtin/checkout.c
+++ w/builtin/checkout.c
@@ -310,9 +310,27 @@ static int checkout_stage(int stage, const struct cache_entry *ce, int pos,
 		return error(_("path '%s' does not have their version"), ce->name);
 }
 
+struct checkout_merged_data {
+	int conflict_style;
+	int renormalize;
+	/* we will add more later */
+};
+
+static void checkout_merged_release(struct checkout_merged_data *data)
+{
+	; /* nothing to free (yet) */
+}
+
+static void checkout_merged_prepare(struct checkout_merged_data *data)
+{
+	int renormalize = 0;
+	repo_config_get_bool(the_repository, "merge.renormalize", &renormalize);
+	data->renormalize = renormalize;
+}
+
 static int checkout_merged(int pos, const struct checkout *state,
 			   int *nr_checkouts, struct mem_pool *ce_mem_pool,
-			   int conflict_style)
+			   struct checkout_merged_data *data)
 {
 	struct cache_entry *ce = the_repository->index->cache[pos];
 	const char *path = ce->name;
@@ -324,7 +342,6 @@ static int checkout_merged(int pos, const struct checkout *state,
 	struct object_id threeway[3];
 	unsigned mode = 0;
 	struct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;
-	int renormalize = 0;
 
 	memset(threeway, 0, sizeof(threeway));
 	while (pos < the_repository->index->cache_nr) {
@@ -345,9 +362,8 @@ static int checkout_merged(int pos, const struct checkout *state,
 	read_mmblob(&ours, the_repository->objects, &threeway[1]);
 	read_mmblob(&theirs, the_repository->objects, &threeway[2]);
 
-	repo_config_get_bool(the_repository, "merge.renormalize", &renormalize);
-	ll_opts.renormalize = renormalize;
-	ll_opts.conflict_style = conflict_style;
+	ll_opts.renormalize = data->renormalize;
+	ll_opts.conflict_style = data->conflict_style;
 	merge_status = ll_merge(&result_buf, path, &ancestor, "base",
 				&ours, "ours", &theirs, "theirs",
 				state->istate, &ll_opts);
@@ -446,6 +462,7 @@ static int checkout_worktree(const struct checkout_opts *opts,
 	int pos;
 	int pc_workers, pc_threshold;
 	struct mem_pool ce_mem_pool;
+	struct checkout_merged_data checkout_merged_data = {0};
 
 	state.force = 1;
 	state.refresh_cache = 1;
@@ -462,6 +479,10 @@ static int checkout_worktree(const struct checkout_opts *opts,
 	if (pc_workers > 1)
 		init_parallel_checkout();
 
+	if (opts->merge) {
+		checkout_merged_prepare(&checkout_merged_data);
+		checkout_merged_data.conflict_style = opts->conflict_style;
+	}
 	for (pos = 0; pos < the_repository->index->cache_nr; pos++) {
 		struct cache_entry *ce = the_repository->index->cache[pos];
 		if (ce->ce_flags & CE_MATCHED) {
@@ -479,10 +500,14 @@ static int checkout_worktree(const struct checkout_opts *opts,
 				errs |= checkout_merged(pos, &state,
 							&nr_unmerged,
 							&ce_mem_pool,
-							opts->conflict_style);
+							&checkout_merged_data);
 			pos = skip_same_name(ce, pos) - 1;
 		}
 	}
+
+	if (opts->merge)
+		checkout_merged_release(&checkout_merged_data);
+
 	if (pc_workers > 1)
 		errs |= run_parallel_checkout(&state, pc_workers, pc_threshold,
 					      NULL, NULL);