Skip to contentSkip to conversation

Avoid the_repository in merge-ort and replay

PATCH Superseded Elijah Newren via GitGitGadget 6 participants 34 messages Feb 18 – Feb 24

lore
Share & tools
lore
#0 lore raw

cover Avoid the_repository in merge-ort and replay

Remove explicit uses of the_repository and the_hash_algo from merge-ort, and since this has now been done multiple times for both merge-ort and replay, implement a small measure to prevent them from returning to either merge-ort or replay.

See
https://lore.kernel.org/git/CABPp-BH7E1Bh2g0vR3T4NEsv34DvFQPzMuJSsqtOAaWY-fFCxg@mail.gmail.com/
and
https://lore.kernel.org/git/CABPp-BFuwvqiCTCCpoyT6em9_1-qrgPWHWhrufQ3UuZ+Kfkb6A@mail.gmail.com/
for recent discussions on these.

Series overview: Patches 1-3: Mostly mechanical removal of existing uses Patches 4-5: Simple hammer to prevent the problem from returning

Elijah Newren (5):
merge-ort: pass repository to write_tree()
merge-ort: replace the_repository with opt->repo
merge-ort: replace the_hash_algo with opt->repo->hash_algo
merge-ort: prevent the_repository from coming back
replay: prevent the_repository from coming back

 merge-ort.c | 92 ++++++++++++++++++++++++++++-------------------------
 replay.c    |  2 ++
 2 files changed, 51 insertions(+), 43 deletions(-)

base-commit: 73fd77805fc6406f31c36212846d9e2541d19321
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2048%2Fnewren%2Favoid_the_repository-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2048/newren/avoid_the_repository-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2048

Signature · 1 line
gitgitgadget
#1 lore raw

PATCH 1/5 merge-ort: pass repository to write_tree()

From: Elijah Newren <[email protected]>

In order to get rid of a usage of the_repository, we need to know the value of opt->repo; pass it along to write_tree(). Once we have the repository, though, we no longer need to pass opt->repo->hash_algo->rawsz, we can have write_tree() look up that value itself.

---
 merge-ort.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
merge-ort.c +8 -8
index 0a59d1e596..42499f7b43 100644
@@ -3822,15 +3822,16 @@ static int tree_entry_order(const void *a_, const void *b_)
 				 b->string, strlen(b->string), bmi->result.mode);
 }
 
-static int write_tree(struct object_id *result_oid,
+static int write_tree(struct repository *repo,
+		      struct object_id *result_oid,
 		      struct string_list *versions,
-		      unsigned int offset,
-		      size_t hash_size)
+		      unsigned int offset)
 {
 	size_t maxlen = 0, extra;
 	unsigned int nr;
 	struct strbuf buf = STRBUF_INIT;
 	int i, ret = 0;
+	size_t hash_size = repo->hash_algo->rawsz;
 
 	assert(offset <= versions->nr);
 	nr = versions->nr - offset;
@@ -3856,7 +3857,7 @@ static int write_tree(struct object_id *result_oid,
 	}
 
 	/* Write this object file out, and record in result_oid */
-	if (odb_write_object(the_repository->objects, buf.buf,
+	if (odb_write_object(repo->objects, buf.buf,
 			     buf.len, OBJ_TREE, result_oid))
 		ret = -1;
 	strbuf_release(&buf);
@@ -4026,8 +4027,8 @@ static int write_completed_directory(struct merge_options *opt,
 		dir_info->is_null = 0;
 		dir_info->result.mode = S_IFDIR;
 		if (record_tree &&
-		    write_tree(&dir_info->result.oid, &info->versions, offset,
-			       opt->repo->hash_algo->rawsz) < 0)
+		    write_tree(opt->repo, &dir_info->result.oid, &info->versions,
+			       offset) < 0)
 			ret = -1;
 	}
 
@@ -4573,8 +4574,7 @@ static int process_entries(struct merge_options *opt,
 		BUG("dir_metadata accounting completely off; shouldn't happen");
 	}
 	if (record_tree &&
-	    write_tree(result_oid, &dir_metadata.versions, 0,
-		       opt->repo->hash_algo->rawsz) < 0)
+	    write_tree(opt->repo, result_oid, &dir_metadata.versions, 0) < 0)
 		ret = -1;
 cleanup:
 	string_list_clear(&plist, 0);
Signature · 1 line
gitgitgadget
#2 lore raw

PATCH 2/5 merge-ort: replace the_repository with opt->repo

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

We have a perfectly valid repository available and do not need to use the_repository, except for one location in prefetch_for_content_merges().

---
 merge-ort.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)
merge-ort.c +22 -22
index 42499f7b43..2106c5f632 100644
@@ -1732,9 +1732,9 @@ static int collect_merge_info(struct merge_options *opt,
 	info.data = opt;
 	info.show_all_errors = 1;
 
-	if (repo_parse_tree(the_repository, merge_base) < 0 ||
-	    repo_parse_tree(the_repository, side1) < 0 ||
-	    repo_parse_tree(the_repository, side2) < 0)
+	if (repo_parse_tree(opt->repo, merge_base) < 0 ||
+	    repo_parse_tree(opt->repo, side1) < 0 ||
+	    repo_parse_tree(opt->repo, side2) < 0)
 		return -1;
 	init_tree_desc(t + 0, &merge_base->object.oid,
 		       merge_base->buffer, merge_base->size);
@@ -2136,9 +2136,9 @@ static int merge_3way(struct merge_options *opt,
 		name2 = mkpathdup("%s:%s", opt->branch2,  pathnames[2]);
 	}
 
-	read_mmblob(&orig, the_repository->objects, o);
-	read_mmblob(&src1, the_repository->objects, a);
-	read_mmblob(&src2, the_repository->objects, b);
+	read_mmblob(&orig, opt->repo->objects, o);
+	read_mmblob(&src1, opt->repo->objects, a);
+	read_mmblob(&src2, opt->repo->objects, b);
 
 	merge_status = ll_merge(result_buf, path, &orig, base,
 				&src1, name1, &src2, name2,
@@ -2254,7 +2254,7 @@ static int handle_content_merge(struct merge_options *opt,
 		}
 
 		if (!ret && record_object &&
-		    odb_write_object(the_repository->objects, result_buf.ptr, result_buf.size,
+		    odb_write_object(opt->repo->objects, result_buf.ptr, result_buf.size,
 				     OBJ_BLOB, &result->oid)) {
 			path_msg(opt, ERROR_OBJECT_WRITE_FAILED, 0,
 				 pathnames[0], pathnames[1], pathnames[2], NULL,
@@ -3713,7 +3713,7 @@ static int read_oid_strbuf(struct merge_options *opt,
 	void *buf;
 	enum object_type type;
 	unsigned long size;
-	buf = odb_read_object(the_repository->objects, oid, &type, &size);
+	buf = odb_read_object(opt->repo->objects, oid, &type, &size);
 	if (!buf) {
 		path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,
 			 path, NULL, NULL, NULL,
@@ -4439,7 +4439,7 @@ static void prefetch_for_content_merges(struct merge_options *opt,
 	struct string_list_item *e;
 	struct oid_array to_fetch = OID_ARRAY_INIT;
 
-	if (opt->repo != the_repository || !repo_has_promisor_remote(the_repository))
+	if (opt->repo != the_repository || !repo_has_promisor_remote(opt->repo))
 		return;
 
 	for (e = &plist->items[plist->nr-1]; e >= plist->items; --e) {
@@ -4619,10 +4619,10 @@ static int checkout(struct merge_options *opt,
 	unpack_opts.verbose_update = (opt->verbosity > 2);
 	unpack_opts.fn = twoway_merge;
 	unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */
-	if (repo_parse_tree(the_repository, prev) < 0)
+	if (repo_parse_tree(opt->repo, prev) < 0)
 		return -1;
 	init_tree_desc(&trees[0], &prev->object.oid, prev->buffer, prev->size);
-	if (repo_parse_tree(the_repository, next) < 0)
+	if (repo_parse_tree(opt->repo, next) < 0)
 		return -1;
 	init_tree_desc(&trees[1], &next->object.oid, next->buffer, next->size);
 
@@ -5280,7 +5280,7 @@ redo:
 
 	if (result->clean >= 0) {
 		if (!opt->mergeability_only) {
-			result->tree = repo_parse_tree_indirect(the_repository,
+			result->tree = repo_parse_tree_indirect(opt->repo,
 								&working_tree_oid);
 			if (!result->tree)
 				die(_("unable to read tree (%s)"),
@@ -5309,7 +5309,7 @@ static void merge_ort_internal(struct merge_options *opt,
 	struct strbuf merge_base_abbrev = STRBUF_INIT;
 
 	if (!merge_bases) {
-		if (repo_get_merge_bases(the_repository, h1, h2,
+		if (repo_get_merge_bases(opt->repo, h1, h2,
 					 &merge_bases) < 0) {
 			result->clean = -1;
 			goto out;
@@ -5440,20 +5440,20 @@ static void merge_recursive_config(struct merge_options *opt, int ui)
 {
 	char *value = NULL;
 	int renormalize = 0;
-	repo_config_get_int(the_repository, "merge.verbosity", &opt->verbosity);
-	repo_config_get_int(the_repository, "diff.renamelimit", &opt->rename_limit);
-	repo_config_get_int(the_repository, "merge.renamelimit", &opt->rename_limit);
-	repo_config_get_bool(the_repository, "merge.renormalize", &renormalize);
+	repo_config_get_int(opt->repo, "merge.verbosity", &opt->verbosity);
+	repo_config_get_int(opt->repo, "diff.renamelimit", &opt->rename_limit);
+	repo_config_get_int(opt->repo, "merge.renamelimit", &opt->rename_limit);
+	repo_config_get_bool(opt->repo, "merge.renormalize", &renormalize);
 	opt->renormalize = renormalize;
-	if (!repo_config_get_string(the_repository, "diff.renames", &value)) {
+	if (!repo_config_get_string(opt->repo, "diff.renames", &value)) {
 		opt->detect_renames = git_config_rename("diff.renames", value);
 		free(value);
 	}
-	if (!repo_config_get_string(the_repository, "merge.renames", &value)) {
+	if (!repo_config_get_string(opt->repo, "merge.renames", &value)) {
 		opt->detect_renames = git_config_rename("merge.renames", value);
 		free(value);
 	}
-	if (!repo_config_get_string(the_repository, "merge.directoryrenames", &value)) {
+	if (!repo_config_get_string(opt->repo, "merge.directoryrenames", &value)) {
 		int boolval = git_parse_maybe_bool(value);
 		if (0 <= boolval) {
 			opt->detect_directory_renames = boolval ?
@@ -5466,7 +5466,7 @@ static void merge_recursive_config(struct merge_options *opt, int ui)
 		free(value);
 	}
 	if (ui) {
-		if (!repo_config_get_string(the_repository, "diff.algorithm", &value)) {
+		if (!repo_config_get_string(opt->repo, "diff.algorithm", &value)) {
 			long diff_algorithm = parse_algorithm_value(value);
 			if (diff_algorithm < 0)
 				die(_("unknown value for config '%s': %s"), "diff.algorithm", value);
@@ -5474,7 +5474,7 @@ static void merge_recursive_config(struct merge_options *opt, int ui)
 			free(value);
 		}
 	}
-	repo_config(the_repository, git_xmerge_config, NULL);
+	repo_config(opt->repo, git_xmerge_config, NULL);
 }
 
 static void init_merge_options(struct merge_options *opt,
Signature · 1 line
gitgitgadget
#3 lore raw

PATCH 3/5 merge-ort: replace the_hash_algo with opt->repo->hash_algo

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

---
 merge-ort.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)
merge-ort.c +13 -13
index 2106c5f632..40a11dca73 100644
@@ -1857,7 +1857,7 @@ static int merge_submodule(struct merge_options *opt,
 		BUG("submodule deleted on one side; this should be handled outside of merge_submodule()");
 
 	if ((sub_not_initialized = repo_submodule_init(&subrepo,
-		opt->repo, path, null_oid(the_hash_algo)))) {
+		opt->repo, path, null_oid(opt->repo->hash_algo)))) {
 		path_msg(opt, CONFLICT_SUBMODULE_NOT_INITIALIZED, 0,
 			 path, NULL, NULL, NULL,
 			 _("Failed to merge submodule %s (not checked out)"),
@@ -2240,7 +2240,7 @@ static int handle_content_merge(struct merge_options *opt,
 		two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode));
 
 		merge_status = merge_3way(opt, path,
-					  two_way ? null_oid(the_hash_algo) : &o->oid,
+					  two_way ? null_oid(opt->repo->hash_algo) : &o->oid,
 					  &a->oid, &b->oid,
 					  pathnames, extra_marker_size,
 					  &result_buf);
@@ -2272,7 +2272,7 @@ static int handle_content_merge(struct merge_options *opt,
 	} else if (S_ISGITLINK(a->mode)) {
 		int two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode));
 		clean = merge_submodule(opt, pathnames[0],
-					two_way ? null_oid(the_hash_algo) : &o->oid,
+					two_way ? null_oid(opt->repo->hash_algo) : &o->oid,
 					&a->oid, &b->oid, &result->oid);
 		if (clean < 0)
 			return -1;
@@ -2786,7 +2786,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
 		assert(!new_ci->match_mask);
 		new_ci->dirmask = 0;
 		new_ci->stages[1].mode = 0;
-		oidcpy(&new_ci->stages[1].oid, null_oid(the_hash_algo));
+		oidcpy(&new_ci->stages[1].oid, null_oid(opt->repo->hash_algo));
 
 		/*
 		 * Now that we have the file information in new_ci, make sure
@@ -2799,7 +2799,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
 				continue;
 			/* zero out any entries related to files */
 			ci->stages[i].mode = 0;
-			oidcpy(&ci->stages[i].oid, null_oid(the_hash_algo));
+			oidcpy(&ci->stages[i].oid, null_oid(opt->repo->hash_algo));
 		}
 
 		/* Now we want to focus on new_ci, so reassign ci to it. */
@@ -3214,7 +3214,7 @@ static int process_renames(struct merge_options *opt,
 			if (type_changed) {
 				/* rename vs. typechange */
 				/* Mark the original as resolved by removal */
-				memcpy(&oldinfo->stages[0].oid, null_oid(the_hash_algo),
+				memcpy(&oldinfo->stages[0].oid, null_oid(opt->repo->hash_algo),
 				       sizeof(oldinfo->stages[0].oid));
 				oldinfo->stages[0].mode = 0;
 				oldinfo->filemask &= 0x06;
@@ -4102,7 +4102,7 @@ static int process_entry(struct merge_options *opt,
 			if (ci->filemask & (1 << i))
 				continue;
 			ci->stages[i].mode = 0;
-			oidcpy(&ci->stages[i].oid, null_oid(the_hash_algo));
+			oidcpy(&ci->stages[i].oid, null_oid(opt->repo->hash_algo));
 		}
 	} else if (ci->df_conflict && ci->merged.result.mode != 0) {
 		/*
@@ -4149,7 +4149,7 @@ static int process_entry(struct merge_options *opt,
 				continue;
 			/* zero out any entries related to directories */
 			new_ci->stages[i].mode = 0;
-			oidcpy(&new_ci->stages[i].oid, null_oid(the_hash_algo));
+			oidcpy(&new_ci->stages[i].oid, null_oid(opt->repo->hash_algo));
 		}
 
 		/*
@@ -4271,11 +4271,11 @@ static int process_entry(struct merge_options *opt,
 			new_ci->merged.result.mode = ci->stages[2].mode;
 			oidcpy(&new_ci->merged.result.oid, &ci->stages[2].oid);
 			new_ci->stages[1].mode = 0;
-			oidcpy(&new_ci->stages[1].oid, null_oid(the_hash_algo));
+			oidcpy(&new_ci->stages[1].oid, null_oid(opt->repo->hash_algo));
 			new_ci->filemask = 5;
 			if ((S_IFMT & b_mode) != (S_IFMT & o_mode)) {
 				new_ci->stages[0].mode = 0;
-				oidcpy(&new_ci->stages[0].oid, null_oid(the_hash_algo));
+				oidcpy(&new_ci->stages[0].oid, null_oid(opt->repo->hash_algo));
 				new_ci->filemask = 4;
 			}
 
@@ -4283,11 +4283,11 @@ static int process_entry(struct merge_options *opt,
 			ci->merged.result.mode = ci->stages[1].mode;
 			oidcpy(&ci->merged.result.oid, &ci->stages[1].oid);
 			ci->stages[2].mode = 0;
-			oidcpy(&ci->stages[2].oid, null_oid(the_hash_algo));
+			oidcpy(&ci->stages[2].oid, null_oid(opt->repo->hash_algo));
 			ci->filemask = 3;
 			if ((S_IFMT & a_mode) != (S_IFMT & o_mode)) {
 				ci->stages[0].mode = 0;
-				oidcpy(&ci->stages[0].oid, null_oid(the_hash_algo));
+				oidcpy(&ci->stages[0].oid, null_oid(opt->repo->hash_algo));
 				ci->filemask = 2;
 			}
 
@@ -4415,7 +4415,7 @@ static int process_entry(struct merge_options *opt,
 		/* Deleted on both sides */
 		ci->merged.is_null = 1;
 		ci->merged.result.mode = 0;
-		oidcpy(&ci->merged.result.oid, null_oid(the_hash_algo));
+		oidcpy(&ci->merged.result.oid, null_oid(opt->repo->hash_algo));
 		assert(!ci->df_conflict);
 		ci->merged.clean = !ci->path_conflict;
 	}
Signature · 1 line
gitgitgadget
#6 lore raw

PATCH 4/5 merge-ort: prevent the_repository from coming back

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

There are two things preventing us from removing our usage of USE_THE_REPOSITORY_VARIABLE: one necessary use of the_repository in prefetch_for_content_merges(), and the use of DEFAULT_ABBREV. We have removed all other uses of the_repository in merge-ort before (multiple times), but without removing that definition, they keep coming back.

Define the_repository to make it a compilation error so that they don't come back any more, with a special carve-out for prefetch_for_content_merges().

---
 merge-ort.c | 6 ++++++
 1 file changed, 6 insertions(+)
merge-ort.c +6 -0
index 40a11dca73..ae19118550 100644
@@ -53,6 +53,8 @@
 #include "unpack-trees.h"
 #include "xdiff-interface.h"
 
+#define the_repository DO_NOT_USE_THE_REPOSITORY
+
 /*
  * We have many arrays of size 3.  Whenever we have such an array, the
  * indices refer to one of the sides of the three-way merge.  This is so
@@ -4433,6 +4435,8 @@ static int process_entry(struct merge_options *opt,
 	return 0;
 }
 
+#undef the_repository
+
 static void prefetch_for_content_merges(struct merge_options *opt,
 					struct string_list *plist)
 {
@@ -4481,6 +4485,8 @@ static void prefetch_for_content_merges(struct merge_options *opt,
 	oid_array_clear(&to_fetch);
 }
 
+#define the_repository DO_NOT_USE_the_repository
+
 static int process_entries(struct merge_options *opt,
 			   struct object_id *result_oid)
 {
Signature · 1 line
gitgitgadget
#13 lore raw

PATCH 5/5 replay: prevent the_repository from coming back

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Due to the use of DEFAULT_ABBREV, we cannot get rid of our usage of USE_THE_REPOSITORY_VARIABLE. We have removed all other uses of the_repository before, but without removing that definition, they keep coming back.

Define the_repository to make it a compilation error so that they don't come back any more.

---
 replay.c | 2 ++
 1 file changed, 2 insertions(+)
replay.c +2 -0
index f97d652f33..a962f53d03 100644
@@ -11,6 +11,8 @@
 #include "strmap.h"
 #include "tree.h"
 
+#define the_repository DO_NOT_USE_THE_REPOSITORY
+
 static const char *short_commit_name(struct repository *repo,
 				     struct commit *commit)
 {
Signature · 1 line
gitgitgadget
#7 lore raw

Re: [PATCH 4/5] merge-ort: prevent the_repository from coming back

In reply to Elijah Newren via GitGitGadget
Quoted 15 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

There are two things preventing us from removing our usage of USE_THE_REPOSITORY_VARIABLE: one necessary use of the_repository in prefetch_for_content_merges(), and the use of DEFAULT_ABBREV. We have removed all other uses of the_repository in merge-ort before (multiple times), but without removing that definition, they keep coming back.

Define the_repository to make it a compilation error so that they don't come back any more, with a special carve-out for prefetch_for_content_merges().

---
[snip]

merge-ort.c

Quoted 4 lines
merge-ort.c +3 -0
+#define the_repository DO_NOT_USE_THE_REPOSITORY
+
[snip]
+#define the_repository DO_NOT_USE_the_repository

Here the casing is different?

Quoted 4 lines
merge-ort.c +1 -0
+
 static int process_entries(struct merge_options *opt,
 			   struct object_id *result_oid)
 {
#9 lore raw

Re: [PATCH 4/5] merge-ort: prevent the_repository from coming back

In reply to Elijah Newren via GitGitGadget
Quoted 7 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

There are two things preventing us from removing our usage of USE_THE_REPOSITORY_VARIABLE: one necessary use of the_repository in prefetch_for_content_merges(), and the use of DEFAULT_ABBREV. We have removed all other uses of the_repository in merge-ort before (multiple times), but without removing that definition, they keep coming back.

Unfortunate, but I think it's okay to be pragmatic here.

merge-ort.c line 56

Quoted 9 lines
merge-ort.c +1 -0
index 40a11dca73..ae19118550 100644
@@ -53,6 +53,8 @@
 #include "unpack-trees.h"
 #include "xdiff-interface.h"
 
+#define the_repository DO_NOT_USE_THE_REPOSITORY

Do we want to have a comment here that explains why we do this dance?

Patrick

#14 lore raw

Re: [PATCH 5/5] replay: prevent the_repository from coming back

In reply to Elijah Newren via GitGitGadget

replay.c line 14

Quoted 9 lines · Elijah Newren via GitGitGadget
replay.c +1 -0
index f97d652f33..a962f53d03 100644
@@ -11,6 +11,8 @@
 #include "strmap.h"
 #include "tree.h"
 
+#define the_repository DO_NOT_USE_THE_REPOSITORY

Same remark here: might make sense to add a comment here to explain what's going on.

Other than that the series is a welcome cleanup, thanks!

Patrick

#8 lore raw

Re: [PATCH 4/5] merge-ort: prevent the_repository from coming back

In reply to Kristoffer Haugsbakk
Quoted 14 lines · Kristoffer Haugsbakk
Quoted 12 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

There are two things preventing us from removing our usage of USE_THE_REPOSITORY_VARIABLE: one necessary use of the_repository in prefetch_for_content_merges(), and the use of DEFAULT_ABBREV. We have removed all other uses of the_repository in merge-ort before (multiple times), but without removing that definition, they keep coming back.

Define the_repository to make it a compilation error so that they don't come back any more, with a special carve-out for prefetch_for_content_merges().

Quoted 9 lines

merge-ort.c

Quoted 7 lines

---
[snip]

merge-ort.c +3 -0
+#define the_repository DO_NOT_USE_THE_REPOSITORY
+
[snip]
+#define the_repository DO_NOT_USE_the_repository

Here the casing is different?

Oops. Will fix; thanks for taking a look.

#5 lore raw

Re: [PATCH 3/5] merge-ort: replace the_hash_algo with opt->repo->hash_algo

In reply to Patrick Steinhardt
Quoted 6 lines · Patrick Steinhardt
Quoted 1 line · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Nit: might make sense to have at least a oneliner here to explain what we're doing, even if the subject already says it all.

Fair enough; will do.

#10 lore raw

Re: [PATCH 4/5] merge-ort: prevent the_repository from coming back

In reply to Patrick Steinhardt
Quoted 11 lines · Patrick Steinhardt
Quoted 7 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

There are two things preventing us from removing our usage of USE_THE_REPOSITORY_VARIABLE: one necessary use of the_repository in prefetch_for_content_merges(), and the use of DEFAULT_ABBREV. We have removed all other uses of the_repository in merge-ort before (multiple times), but without removing that definition, they keep coming back.

Unfortunate, but I think it's okay to be pragmatic here.

Yeah, also full disclosure: I do not know why prefetch_for_content_merges() needs to use the_repository. When I introduced it back in 2bff554b23e8 (merge-ort: add prefetching for content merges, 2021-06-22), I was just looking at diffcore_std() and trying to mimic how it did the prefetch. I don't actually understand why the comparison against the_repository is there for either of these functions. Maybe someone else knows and could shed some light? (cc:
Jonathan Tan for the diffcore_std() case I was copying from...)

Quoted 11 lines

merge-ort.c line 56

Quoted 9 lines
merge-ort.c +1 -0
index 40a11dca73..ae19118550 100644
@@ -53,6 +53,8 @@
 #include "unpack-trees.h"
 #include "xdiff-interface.h"
 
+#define the_repository DO_NOT_USE_THE_REPOSITORY

Do we want to have a comment here that explains why we do this dance?

Sure, will do.

#11 lore raw

Re: [PATCH 4/5] merge-ort: prevent the_repository from coming back

Quoted 8 lines · Elijah Newren

Yeah, also full disclosure: I do not know why prefetch_for_content_merges() needs to use the_repository. When I introduced it back in 2bff554b23e8 (merge-ort: add prefetching for content merges, 2021-06-22), I was just looking at diffcore_std() and trying to mimic how it did the prefetch. I don't actually understand why the comparison against the_repository is there for either of these functions. Maybe someone else knows and could shed some light? (cc:
Jonathan Tan for the diffcore_std() case I was copying from...)

I did a bit of digging for you ;-)

The comparison with the_repository is from 7fbbcb21 (diff: batch fetching of missing blobs, 2019-04-05), whose original version did not have it, but was later amended with

https://lore.kernel.org/git/[email protected]/

And after that it survived across evolutions like b14ed5ad (Use promisor_remote_get_direct() and has_promisor_remote(), 2019-06-25), 95acf11a (diff: restrict when prefetching occurs, 2020-04-07), and finally a5183d76 (cocci: apply the "promisor-remote.h" part of "the_repository.pending", 2023-03-28).

#12 lore raw

Re: [PATCH 4/5] merge-ort: prevent the_repository from coming back

Quoted 12 lines · Junio C Hamano
Quoted 8 lines · Elijah Newren

Yeah, also full disclosure: I do not know why prefetch_for_content_merges() needs to use the_repository. When I introduced it back in 2bff554b23e8 (merge-ort: add prefetching for content merges, 2021-06-22), I was just looking at diffcore_std() and trying to mimic how it did the prefetch. I don't actually understand why the comparison against the_repository is there for either of these functions. Maybe someone else knows and could shed some light? (cc:
Jonathan Tan for the diffcore_std() case I was copying from...)

Quoted 1 line

I did a bit of digging for you ;-)

Thanks!

Quoted 5 lines

The comparison with the_repository is from 7fbbcb21 (diff: batch fetching of missing blobs, 2019-04-05), whose original version did not have it, but was later amended with

https://lore.kernel.org/git/[email protected]/

Ah, the explanation in that email doesn't exist inside the commit history, since the diff was squashed in and the relevant part of the explanation wasn't added to the commit message. I should have thought to check the mail archives too. Anyway, the important bit is:

Quoted 3 lines

Also, prefetch only if the repository being diffed
is the_repository (because we do not support lazy fetching for any other repository anyway).

This comment came from mid-2019. I then copied the logic from diffcore_std() in 2020 (though didn't get the relevant patch upstream until 2021), and as of 2021 we have ef830cc43412 (promisor-remote:
teach lazy-fetch in any repo, 2021-06-17), which means that this check has been obsoleted by events. And it looks like the check in diffcore_std() was left in place as an oversight as well.

So, we don't need that check anymore. I'll add a commit that simply removes these checks from both merge-ort.c and diff.c and reduces two more uses of the_repository.

#15 lore raw

v2 cover Avoid the_repository in merge-ort and replay

In reply to Elijah Newren via GitGitGadget

Changes since v1:

* Add a preparatory patch removing the_repository check from blob prefetching in both merge-ort and diff*; it's no longer necessary
* Fix casing mismatch
* Simplify the hammer a bit based on the new first patch, but add some simple comments explaining it

Remove explicit uses of the_repository and the_hash_algo from merge-ort, and since this has now been done multiple times for both merge-ort and replay, implement a small measure to prevent them from returning to either merge-ort or replay.

See
https://lore.kernel.org/git/CABPp-BH7E1Bh2g0vR3T4NEsv34DvFQPzMuJSsqtOAaWY-fFCxg@mail.gmail.com/
and
https://lore.kernel.org/git/CABPp-BFuwvqiCTCCpoyT6em9_1-qrgPWHWhrufQ3UuZ+Kfkb6A@mail.gmail.com/
for recent discussions on these.

As noted in the comments on v1, I actually do not know why prefetch_for_content_merges() needs to use the_repository. When I introduced it back in 2bff554b23e8 (merge-ort: add prefetching for content merges, 2021-06-22), I was just looking at diffcore_std() and trying to mimic how it did the prefetch, and it has such a comparison. If anyone knows why diffcore_std() needs to compare against the_repository, I'd love to hear...

Series overview: Patches 1-3: Mostly mechanical removal of existing uses Patches 4-5: Simple hammer to prevent the problem from returning

Elijah Newren (6):
merge,diff: remove the_repository check before prefetching blobs
merge-ort: pass repository to write_tree()
merge-ort: replace the_repository with opt->repo
merge-ort: replace the_hash_algo with opt->repo->hash_algo
merge-ort: prevent the_repository from coming back
replay: prevent the_repository from coming back

 diff.c            |  2 +-
 diffcore-break.c  |  2 +-
 diffcore-rename.c |  4 +-
 merge-ort.c       | 94 +++++++++++++++++++++++++----------------------
 replay.c          |  6 +++
 5 files changed, 61 insertions(+), 47 deletions(-)

base-commit: 73fd77805fc6406f31c36212846d9e2541d19321
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2048%2Fnewren%2Favoid_the_repository-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2048/newren/avoid_the_repository-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2048

Range-diff vs v1:

 -:  ---------- > 1:  7155a0da6f merge,diff: remove the_repository check before prefetching blobs
 1:  620c4ea38b = 2:  911cba991b merge-ort: pass repository to write_tree()
 2:  abba4bd762 ! 3:  68af47ed18 merge-ort: replace the_repository with opt->repo
     @@ merge-ort.c: static void prefetch_for_content_merges(struct merge_options *opt,
       	struct string_list_item *e;
       	struct oid_array to_fetch = OID_ARRAY_INIT;
       
     --	if (opt->repo != the_repository || !repo_has_promisor_remote(the_repository))
     -+	if (opt->repo != the_repository || !repo_has_promisor_remote(opt->repo))
     +-	if (!repo_has_promisor_remote(the_repository))
     ++	if (!repo_has_promisor_remote(opt->repo))
       		return;
       
       	for (e = &plist->items[plist->nr-1]; e >= plist->items; --e) {

3: 36c2713ceb ! 4: bfa68716af merge-ort: replace the_hash_algo with opt->repo->hash_algo
@@ Metadata
## Commit message ##
merge-ort: replace the_hash_algo with opt->repo->hash_algo

+ We have a perfectly valid repository available and do not need to use
+ the_hash_algo (a shorthand for the_repository->hash_algo), so use the
+ known repository instead.
+

## merge-ort.c ##
4: 46c24e0d05 ! 5: 932d945c9b merge-ort: prevent the_repository from coming back
@@ Metadata
## Commit message ##
merge-ort: prevent the_repository from coming back

- There are two things preventing us from removing our usage of
- USE_THE_REPOSITORY_VARIABLE: one necessary use of the_repository in
- prefetch_for_content_merges(), and the use of DEFAULT_ABBREV. We have
- removed all other uses of the_repository in merge-ort before (multiple
- times), but without removing that definition, they keep coming back.
+ Due to the use of DEFAULT_ABBREV, we cannot get rid of our usage of
+ USE_THE_REPOSITORY_VARIABLE. However, we have removed all other uses of
+ the_repository in merge-ort a few times. But they keep coming back.

Define the_repository to make it a compilation error so that they don't
- come back any more, with a special carve-out for
- prefetch_for_content_merges().
+ come back any more.

@@ merge-ort.c
#include "unpack-trees.h"
#include "xdiff-interface.h"

++/*
++ * We technically need USE_THE_REPOSITORY_VARIABLE above for DEFAULT_ABBREV, ++ * but do not want more uses of the_repository. Prevent them.
++ *
++ * opt->repo is available; use it instead.
++ */
+#define the_repository DO_NOT_USE_THE_REPOSITORY
+
/*
* We have many arrays of size 3. Whenever we have such an array, the
* indices refer to one of the sides of the three-way merge. This is so
-@@ merge-ort.c: static int process_entry(struct merge_options *opt,
- return 0;
- }
-
-+#undef the_repository
-+
- static void prefetch_for_content_merges(struct merge_options *opt,
- struct string_list *plist)
- {
-@@ merge-ort.c: static void prefetch_for_content_merges(struct merge_options *opt,
- oid_array_clear(&to_fetch);
- }
-
-+#define the_repository DO_NOT_USE_the_repository
-+
- static int process_entries(struct merge_options *opt,
- struct object_id *result_oid)
- {
5: d75a71aef9 ! 6: 67db46f34f replay: prevent the_repository from coming back

     @@ replay.c
       #include "strmap.h"
       #include "tree.h"
       
     ++/*
     ++ * We technically need USE_THE_REPOSITORY_VARIABLE for DEFAULT_ABBREV, but
     ++ * do not want to use the_repository.
     ++ */
      +#define the_repository DO_NOT_USE_THE_REPOSITORY
      +
       static const char *short_commit_name(struct repository *repo,
Signature · 1 line
gitgitgadget
#16 lore raw

PATCH v2 1/6 merge,diff: remove the_repository check before prefetching blobs

From: Elijah Newren <[email protected]>

Prefetching of blobs from promisor remotes was added to diff in 7fbbcb21b162 (diff: batch fetching of missing blobs, 2019-04-05). In that commit,

https://lore.kernel.org/git/[email protected]/

was squashed into

https://lore.kernel.org/git/44de02e584f449481e6fb00cf35d74adf0192e9d.1553895166.git.jonathantanmy@google.com/

without the extra explanation about the squashed changes being added to the commit message; in particular, this explanation from that first link is absent:

Quoted 3 lines

Also, prefetch only if the repository being diffed is the_repository (because we do not support lazy fetching for any other repository
anyway).

Then, later, this checking was spread from diff.c to diffcore-rename.c and diffcore-break.c by 95acf11a3dc3 (diff: restrict when prefetching occurs, 2020-04-07) and then further split in d331dd3b0c82 (diffcore-rename: allow different missing_object_cb functions, 2021-06-22). I also copied the logic from prefetching blobs from diff.c to merge-ort.c in 2bff554b23e8 (merge-ort: add prefetching for content merges, 2021-06-22).

The reason for all these checks was noted above -- we only supported lazy fetching for the_repository. However, that changed with ef830cc43412 (promisor-remote: teach lazy-fetch in any repo, 2021-06-17), so these checks are now unnecessary. Remove them.

---
 diff.c            | 2 +-
 diffcore-break.c  | 2 +-
 diffcore-rename.c | 4 ++--
 merge-ort.c       | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)
diff.c +1 -1
index 35b903a9a0..91d81f66ad 100644
@@ -7176,7 +7176,7 @@ void diffcore_std(struct diff_options *options)
 	 * If no prefetching occurs, diffcore_rename() will prefetch if it
 	 * decides that it needs inexact rename detection.
 	 */
-	if (options->repo == the_repository && repo_has_promisor_remote(the_repository) &&
+	if (repo_has_promisor_remote(the_repository) &&
 	    (options->output_format & output_formats_to_prefetch ||
 	     options->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK))
 		diff_queued_diff_prefetch(options->repo);
diffcore-break.c +1 -1
index c4c2173f30..5ce227ba22 100644
@@ -69,7 +69,7 @@ static int should_break(struct repository *r,
 	    oideq(&src->oid, &dst->oid))
 		return 0; /* they are the same */
 
-	if (r == the_repository && repo_has_promisor_remote(the_repository)) {
+	if (repo_has_promisor_remote(the_repository)) {
 		options.missing_object_cb = diff_queued_diff_prefetch;
 		options.missing_object_data = r;
 	}
diffcore-rename.c +2 -2
index d9476db35a..072752954e 100644
@@ -987,7 +987,7 @@ static int find_basename_matches(struct diff_options *options,
 			strintmap_set(&dests, base, i);
 	}
 
-	if (options->repo == the_repository && repo_has_promisor_remote(the_repository)) {
+	if (repo_has_promisor_remote(the_repository)) {
 		dpf_options.missing_object_cb = basename_prefetch;
 		dpf_options.missing_object_data = &prefetch_options;
 	}
@@ -1574,7 +1574,7 @@ void diffcore_rename_extended(struct diff_options *options,
 
 	/* Finish setting up dpf_options */
 	prefetch_options.skip_unmodified = skip_unmodified;
-	if (options->repo == the_repository && repo_has_promisor_remote(the_repository)) {
+	if (repo_has_promisor_remote(the_repository)) {
 		dpf_options.missing_object_cb = inexact_prefetch;
 		dpf_options.missing_object_data = &prefetch_options;
 	}
merge-ort.c +1 -1
index 0a59d1e596..593e3a2087 100644
@@ -4438,7 +4438,7 @@ static void prefetch_for_content_merges(struct merge_options *opt,
 	struct string_list_item *e;
 	struct oid_array to_fetch = OID_ARRAY_INIT;
 
-	if (opt->repo != the_repository || !repo_has_promisor_remote(the_repository))
+	if (!repo_has_promisor_remote(the_repository))
 		return;
 
 	for (e = &plist->items[plist->nr-1]; e >= plist->items; --e) {
Signature · 1 line
gitgitgadget
#19 lore raw

PATCH v2 2/6 merge-ort: pass repository to write_tree()

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

In order to get rid of a usage of the_repository, we need to know the value of opt->repo; pass it along to write_tree(). Once we have the repository, though, we no longer need to pass opt->repo->hash_algo->rawsz, we can have write_tree() look up that value itself.

---
 merge-ort.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
merge-ort.c +8 -8
index 593e3a2087..3535fc676f 100644
@@ -3822,15 +3822,16 @@ static int tree_entry_order(const void *a_, const void *b_)
 				 b->string, strlen(b->string), bmi->result.mode);
 }
 
-static int write_tree(struct object_id *result_oid,
+static int write_tree(struct repository *repo,
+		      struct object_id *result_oid,
 		      struct string_list *versions,
-		      unsigned int offset,
-		      size_t hash_size)
+		      unsigned int offset)
 {
 	size_t maxlen = 0, extra;
 	unsigned int nr;
 	struct strbuf buf = STRBUF_INIT;
 	int i, ret = 0;
+	size_t hash_size = repo->hash_algo->rawsz;
 
 	assert(offset <= versions->nr);
 	nr = versions->nr - offset;
@@ -3856,7 +3857,7 @@ static int write_tree(struct object_id *result_oid,
 	}
 
 	/* Write this object file out, and record in result_oid */
-	if (odb_write_object(the_repository->objects, buf.buf,
+	if (odb_write_object(repo->objects, buf.buf,
 			     buf.len, OBJ_TREE, result_oid))
 		ret = -1;
 	strbuf_release(&buf);
@@ -4026,8 +4027,8 @@ static int write_completed_directory(struct merge_options *opt,
 		dir_info->is_null = 0;
 		dir_info->result.mode = S_IFDIR;
 		if (record_tree &&
-		    write_tree(&dir_info->result.oid, &info->versions, offset,
-			       opt->repo->hash_algo->rawsz) < 0)
+		    write_tree(opt->repo, &dir_info->result.oid, &info->versions,
+			       offset) < 0)
 			ret = -1;
 	}
 
@@ -4573,8 +4574,7 @@ static int process_entries(struct merge_options *opt,
 		BUG("dir_metadata accounting completely off; shouldn't happen");
 	}
 	if (record_tree &&
-	    write_tree(result_oid, &dir_metadata.versions, 0,
-		       opt->repo->hash_algo->rawsz) < 0)
+	    write_tree(opt->repo, result_oid, &dir_metadata.versions, 0) < 0)
 		ret = -1;
 cleanup:
 	string_list_clear(&plist, 0);
Signature · 1 line
gitgitgadget
#20 lore raw

PATCH v2 3/6 merge-ort: replace the_repository with opt->repo

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

We have a perfectly valid repository available and do not need to use the_repository, except for one location in prefetch_for_content_merges().

---
 merge-ort.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)
merge-ort.c +22 -22
index 3535fc676f..9b6a4c312e 100644
@@ -1732,9 +1732,9 @@ static int collect_merge_info(struct merge_options *opt,
 	info.data = opt;
 	info.show_all_errors = 1;
 
-	if (repo_parse_tree(the_repository, merge_base) < 0 ||
-	    repo_parse_tree(the_repository, side1) < 0 ||
-	    repo_parse_tree(the_repository, side2) < 0)
+	if (repo_parse_tree(opt->repo, merge_base) < 0 ||
+	    repo_parse_tree(opt->repo, side1) < 0 ||
+	    repo_parse_tree(opt->repo, side2) < 0)
 		return -1;
 	init_tree_desc(t + 0, &merge_base->object.oid,
 		       merge_base->buffer, merge_base->size);
@@ -2136,9 +2136,9 @@ static int merge_3way(struct merge_options *opt,
 		name2 = mkpathdup("%s:%s", opt->branch2,  pathnames[2]);
 	}
 
-	read_mmblob(&orig, the_repository->objects, o);
-	read_mmblob(&src1, the_repository->objects, a);
-	read_mmblob(&src2, the_repository->objects, b);
+	read_mmblob(&orig, opt->repo->objects, o);
+	read_mmblob(&src1, opt->repo->objects, a);
+	read_mmblob(&src2, opt->repo->objects, b);
 
 	merge_status = ll_merge(result_buf, path, &orig, base,
 				&src1, name1, &src2, name2,
@@ -2254,7 +2254,7 @@ static int handle_content_merge(struct merge_options *opt,
 		}
 
 		if (!ret && record_object &&
-		    odb_write_object(the_repository->objects, result_buf.ptr, result_buf.size,
+		    odb_write_object(opt->repo->objects, result_buf.ptr, result_buf.size,
 				     OBJ_BLOB, &result->oid)) {
 			path_msg(opt, ERROR_OBJECT_WRITE_FAILED, 0,
 				 pathnames[0], pathnames[1], pathnames[2], NULL,
@@ -3713,7 +3713,7 @@ static int read_oid_strbuf(struct merge_options *opt,
 	void *buf;
 	enum object_type type;
 	unsigned long size;
-	buf = odb_read_object(the_repository->objects, oid, &type, &size);
+	buf = odb_read_object(opt->repo->objects, oid, &type, &size);
 	if (!buf) {
 		path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,
 			 path, NULL, NULL, NULL,
@@ -4439,7 +4439,7 @@ static void prefetch_for_content_merges(struct merge_options *opt,
 	struct string_list_item *e;
 	struct oid_array to_fetch = OID_ARRAY_INIT;
 
-	if (!repo_has_promisor_remote(the_repository))
+	if (!repo_has_promisor_remote(opt->repo))
 		return;
 
 	for (e = &plist->items[plist->nr-1]; e >= plist->items; --e) {
@@ -4619,10 +4619,10 @@ static int checkout(struct merge_options *opt,
 	unpack_opts.verbose_update = (opt->verbosity > 2);
 	unpack_opts.fn = twoway_merge;
 	unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */
-	if (repo_parse_tree(the_repository, prev) < 0)
+	if (repo_parse_tree(opt->repo, prev) < 0)
 		return -1;
 	init_tree_desc(&trees[0], &prev->object.oid, prev->buffer, prev->size);
-	if (repo_parse_tree(the_repository, next) < 0)
+	if (repo_parse_tree(opt->repo, next) < 0)
 		return -1;
 	init_tree_desc(&trees[1], &next->object.oid, next->buffer, next->size);
 
@@ -5280,7 +5280,7 @@ redo:
 
 	if (result->clean >= 0) {
 		if (!opt->mergeability_only) {
-			result->tree = repo_parse_tree_indirect(the_repository,
+			result->tree = repo_parse_tree_indirect(opt->repo,
 								&working_tree_oid);
 			if (!result->tree)
 				die(_("unable to read tree (%s)"),
@@ -5309,7 +5309,7 @@ static void merge_ort_internal(struct merge_options *opt,
 	struct strbuf merge_base_abbrev = STRBUF_INIT;
 
 	if (!merge_bases) {
-		if (repo_get_merge_bases(the_repository, h1, h2,
+		if (repo_get_merge_bases(opt->repo, h1, h2,
 					 &merge_bases) < 0) {
 			result->clean = -1;
 			goto out;
@@ -5440,20 +5440,20 @@ static void merge_recursive_config(struct merge_options *opt, int ui)
 {
 	char *value = NULL;
 	int renormalize = 0;
-	repo_config_get_int(the_repository, "merge.verbosity", &opt->verbosity);
-	repo_config_get_int(the_repository, "diff.renamelimit", &opt->rename_limit);
-	repo_config_get_int(the_repository, "merge.renamelimit", &opt->rename_limit);
-	repo_config_get_bool(the_repository, "merge.renormalize", &renormalize);
+	repo_config_get_int(opt->repo, "merge.verbosity", &opt->verbosity);
+	repo_config_get_int(opt->repo, "diff.renamelimit", &opt->rename_limit);
+	repo_config_get_int(opt->repo, "merge.renamelimit", &opt->rename_limit);
+	repo_config_get_bool(opt->repo, "merge.renormalize", &renormalize);
 	opt->renormalize = renormalize;
-	if (!repo_config_get_string(the_repository, "diff.renames", &value)) {
+	if (!repo_config_get_string(opt->repo, "diff.renames", &value)) {
 		opt->detect_renames = git_config_rename("diff.renames", value);
 		free(value);
 	}
-	if (!repo_config_get_string(the_repository, "merge.renames", &value)) {
+	if (!repo_config_get_string(opt->repo, "merge.renames", &value)) {
 		opt->detect_renames = git_config_rename("merge.renames", value);
 		free(value);
 	}
-	if (!repo_config_get_string(the_repository, "merge.directoryrenames", &value)) {
+	if (!repo_config_get_string(opt->repo, "merge.directoryrenames", &value)) {
 		int boolval = git_parse_maybe_bool(value);
 		if (0 <= boolval) {
 			opt->detect_directory_renames = boolval ?
@@ -5466,7 +5466,7 @@ static void merge_recursive_config(struct merge_options *opt, int ui)
 		free(value);
 	}
 	if (ui) {
-		if (!repo_config_get_string(the_repository, "diff.algorithm", &value)) {
+		if (!repo_config_get_string(opt->repo, "diff.algorithm", &value)) {
 			long diff_algorithm = parse_algorithm_value(value);
 			if (diff_algorithm < 0)
 				die(_("unknown value for config '%s': %s"), "diff.algorithm", value);
@@ -5474,7 +5474,7 @@ static void merge_recursive_config(struct merge_options *opt, int ui)
 			free(value);
 		}
 	}
-	repo_config(the_repository, git_xmerge_config, NULL);
+	repo_config(opt->repo, git_xmerge_config, NULL);
 }
 
 static void init_merge_options(struct merge_options *opt,
Signature · 1 line
gitgitgadget
#21 lore raw

PATCH v2 4/6 merge-ort: replace the_hash_algo with opt->repo->hash_algo

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

We have a perfectly valid repository available and do not need to use the_hash_algo (a shorthand for the_repository->hash_algo), so use the known repository instead.

---
 merge-ort.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)
merge-ort.c +13 -13
index 9b6a4c312e..60b4675f39 100644
@@ -1857,7 +1857,7 @@ static int merge_submodule(struct merge_options *opt,
 		BUG("submodule deleted on one side; this should be handled outside of merge_submodule()");
 
 	if ((sub_not_initialized = repo_submodule_init(&subrepo,
-		opt->repo, path, null_oid(the_hash_algo)))) {
+		opt->repo, path, null_oid(opt->repo->hash_algo)))) {
 		path_msg(opt, CONFLICT_SUBMODULE_NOT_INITIALIZED, 0,
 			 path, NULL, NULL, NULL,
 			 _("Failed to merge submodule %s (not checked out)"),
@@ -2240,7 +2240,7 @@ static int handle_content_merge(struct merge_options *opt,
 		two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode));
 
 		merge_status = merge_3way(opt, path,
-					  two_way ? null_oid(the_hash_algo) : &o->oid,
+					  two_way ? null_oid(opt->repo->hash_algo) : &o->oid,
 					  &a->oid, &b->oid,
 					  pathnames, extra_marker_size,
 					  &result_buf);
@@ -2272,7 +2272,7 @@ static int handle_content_merge(struct merge_options *opt,
 	} else if (S_ISGITLINK(a->mode)) {
 		int two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode));
 		clean = merge_submodule(opt, pathnames[0],
-					two_way ? null_oid(the_hash_algo) : &o->oid,
+					two_way ? null_oid(opt->repo->hash_algo) : &o->oid,
 					&a->oid, &b->oid, &result->oid);
 		if (clean < 0)
 			return -1;
@@ -2786,7 +2786,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
 		assert(!new_ci->match_mask);
 		new_ci->dirmask = 0;
 		new_ci->stages[1].mode = 0;
-		oidcpy(&new_ci->stages[1].oid, null_oid(the_hash_algo));
+		oidcpy(&new_ci->stages[1].oid, null_oid(opt->repo->hash_algo));
 
 		/*
 		 * Now that we have the file information in new_ci, make sure
@@ -2799,7 +2799,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
 				continue;
 			/* zero out any entries related to files */
 			ci->stages[i].mode = 0;
-			oidcpy(&ci->stages[i].oid, null_oid(the_hash_algo));
+			oidcpy(&ci->stages[i].oid, null_oid(opt->repo->hash_algo));
 		}
 
 		/* Now we want to focus on new_ci, so reassign ci to it. */
@@ -3214,7 +3214,7 @@ static int process_renames(struct merge_options *opt,
 			if (type_changed) {
 				/* rename vs. typechange */
 				/* Mark the original as resolved by removal */
-				memcpy(&oldinfo->stages[0].oid, null_oid(the_hash_algo),
+				memcpy(&oldinfo->stages[0].oid, null_oid(opt->repo->hash_algo),
 				       sizeof(oldinfo->stages[0].oid));
 				oldinfo->stages[0].mode = 0;
 				oldinfo->filemask &= 0x06;
@@ -4102,7 +4102,7 @@ static int process_entry(struct merge_options *opt,
 			if (ci->filemask & (1 << i))
 				continue;
 			ci->stages[i].mode = 0;
-			oidcpy(&ci->stages[i].oid, null_oid(the_hash_algo));
+			oidcpy(&ci->stages[i].oid, null_oid(opt->repo->hash_algo));
 		}
 	} else if (ci->df_conflict && ci->merged.result.mode != 0) {
 		/*
@@ -4149,7 +4149,7 @@ static int process_entry(struct merge_options *opt,
 				continue;
 			/* zero out any entries related to directories */
 			new_ci->stages[i].mode = 0;
-			oidcpy(&new_ci->stages[i].oid, null_oid(the_hash_algo));
+			oidcpy(&new_ci->stages[i].oid, null_oid(opt->repo->hash_algo));
 		}
 
 		/*
@@ -4271,11 +4271,11 @@ static int process_entry(struct merge_options *opt,
 			new_ci->merged.result.mode = ci->stages[2].mode;
 			oidcpy(&new_ci->merged.result.oid, &ci->stages[2].oid);
 			new_ci->stages[1].mode = 0;
-			oidcpy(&new_ci->stages[1].oid, null_oid(the_hash_algo));
+			oidcpy(&new_ci->stages[1].oid, null_oid(opt->repo->hash_algo));
 			new_ci->filemask = 5;
 			if ((S_IFMT & b_mode) != (S_IFMT & o_mode)) {
 				new_ci->stages[0].mode = 0;
-				oidcpy(&new_ci->stages[0].oid, null_oid(the_hash_algo));
+				oidcpy(&new_ci->stages[0].oid, null_oid(opt->repo->hash_algo));
 				new_ci->filemask = 4;
 			}
 
@@ -4283,11 +4283,11 @@ static int process_entry(struct merge_options *opt,
 			ci->merged.result.mode = ci->stages[1].mode;
 			oidcpy(&ci->merged.result.oid, &ci->stages[1].oid);
 			ci->stages[2].mode = 0;
-			oidcpy(&ci->stages[2].oid, null_oid(the_hash_algo));
+			oidcpy(&ci->stages[2].oid, null_oid(opt->repo->hash_algo));
 			ci->filemask = 3;
 			if ((S_IFMT & a_mode) != (S_IFMT & o_mode)) {
 				ci->stages[0].mode = 0;
-				oidcpy(&ci->stages[0].oid, null_oid(the_hash_algo));
+				oidcpy(&ci->stages[0].oid, null_oid(opt->repo->hash_algo));
 				ci->filemask = 2;
 			}
 
@@ -4415,7 +4415,7 @@ static int process_entry(struct merge_options *opt,
 		/* Deleted on both sides */
 		ci->merged.is_null = 1;
 		ci->merged.result.mode = 0;
-		oidcpy(&ci->merged.result.oid, null_oid(the_hash_algo));
+		oidcpy(&ci->merged.result.oid, null_oid(opt->repo->hash_algo));
 		assert(!ci->df_conflict);
 		ci->merged.clean = !ci->path_conflict;
 	}
Signature · 1 line
gitgitgadget
#22 lore raw

PATCH v2 5/6 merge-ort: prevent the_repository from coming back

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Due to the use of DEFAULT_ABBREV, we cannot get rid of our usage of USE_THE_REPOSITORY_VARIABLE. However, we have removed all other uses of the_repository in merge-ort a few times. But they keep coming back.

Define the_repository to make it a compilation error so that they don't come back any more.

---
 merge-ort.c | 8 ++++++++
 1 file changed, 8 insertions(+)
merge-ort.c +8 -0
index 60b4675f39..00923ce3cd 100644
@@ -53,6 +53,14 @@
 #include "unpack-trees.h"
 #include "xdiff-interface.h"
 
+/*
+ * We technically need USE_THE_REPOSITORY_VARIABLE above for DEFAULT_ABBREV,
+ * but do not want more uses of the_repository.  Prevent them.
+ *
+ * opt->repo is available; use it instead.
+ */
+#define the_repository DO_NOT_USE_THE_REPOSITORY
+
 /*
  * We have many arrays of size 3.  Whenever we have such an array, the
  * indices refer to one of the sides of the three-way merge.  This is so
Signature · 1 line
gitgitgadget
#23 lore raw

PATCH v2 6/6 replay: prevent the_repository from coming back

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Due to the use of DEFAULT_ABBREV, we cannot get rid of our usage of USE_THE_REPOSITORY_VARIABLE. We have removed all other uses of the_repository before, but without removing that definition, they keep coming back.

Define the_repository to make it a compilation error so that they don't come back any more.

---
 replay.c | 6 ++++++
 1 file changed, 6 insertions(+)
replay.c +6 -0
index f97d652f33..a63f6714c4 100644
@@ -11,6 +11,12 @@
 #include "strmap.h"
 #include "tree.h"
 
+/*
+ * We technically need USE_THE_REPOSITORY_VARIABLE for DEFAULT_ABBREV, but
+ * do not want to use the_repository.
+ */
+#define the_repository DO_NOT_USE_THE_REPOSITORY
+
 static const char *short_commit_name(struct repository *repo,
 				     struct commit *commit)
 {
Signature · 1 line
gitgitgadget
#17 lore raw

Re: [PATCH v2 1/6] merge,diff: remove the_repository check before prefetching blobs

In reply to Elijah Newren via GitGitGadget

diff.c void diffcore_std(struct diff_options *options) · line 7179

Quoted 10 lines · Elijah Newren via GitGitGadget
diff.c +1 -1
index 35b903a9a0..91d81f66ad 100644
@@ -7176,7 +7176,7 @@ void diffcore_std(struct diff_options *options)
 	 * If no prefetching occurs, diffcore_rename() will prefetch if it
 	 * decides that it needs inexact rename detection.
 	 */
-	if (options->repo == the_repository && repo_has_promisor_remote(the_repository) &&
+	if (repo_has_promisor_remote(the_repository) &&

I wonder though -- shouldn't we also pass options->repo to repo_has_promisor_remote() now? Otherwise we may support backfill fetches from arbitrary repositories, but we'll only do them in case the main repository has a promisor remote.

Patrick

#18 lore raw

Re: [PATCH v2 1/6] merge,diff: remove the_repository check before prefetching blobs

Quoted 17 lines · Patrick Steinhardt

diff.c void diffcore_std(struct diff_options *options) · line 7179

Quoted 10 lines · Elijah Newren via GitGitGadget
diff.c +1 -1
index 35b903a9a0..91d81f66ad 100644
@@ -7176,7 +7176,7 @@ void diffcore_std(struct diff_options *options)
       * If no prefetching occurs, diffcore_rename() will prefetch if it
       * decides that it needs inexact rename detection.
       */
-     if (options->repo == the_repository && repo_has_promisor_remote(the_repository) &&
+     if (repo_has_promisor_remote(the_repository) &&

I wonder though -- shouldn't we also pass options->repo to repo_has_promisor_remote() now? Otherwise we may support backfill fetches from arbitrary repositories, but we'll only do them in case the main repository has a promisor remote.

Doh. Definitely yes. Amid having this patch originally be last and rebasing and whatnot, I didn't look closely enough and missed that. Will fix all the callsites and resubmit.

#24 lore raw

v3 cover Avoid the_repository in merge-ort and replay

In reply to Elijah Newren via GitGitGadget

Changes since v2:

* In first patch, actually avoid the_repository when attempting to remove check against the_repository
* Fix commit message of patch 3 due to the new patch 1.
* Slight tweak to commit message of patch 6.

Changes since v1:

* Add a preparatory patch removing the_repository check from blob prefetching in both merge-ort and diff*; it's no longer necessary
* Fix casing mismatch
* Simplify the hammer a bit based on the new first patch, but add some simple comments explaining it

Remove explicit uses of the_repository and the_hash_algo from merge-ort, and since this has now been done multiple times for both merge-ort and replay, implement a small measure to prevent them from returning to either merge-ort or replay.

See
https://lore.kernel.org/git/CABPp-BH7E1Bh2g0vR3T4NEsv34DvFQPzMuJSsqtOAaWY-fFCxg@mail.gmail.com/
and
https://lore.kernel.org/git/CABPp-BFuwvqiCTCCpoyT6em9_1-qrgPWHWhrufQ3UuZ+Kfkb6A@mail.gmail.com/
for recent discussions on these.

As noted in the comments on v1, I actually do not know why prefetch_for_content_merges() needs to use the_repository. When I introduced it back in 2bff554b23e8 (merge-ort: add prefetching for content merges, 2021-06-22), I was just looking at diffcore_std() and trying to mimic how it did the prefetch, and it has such a comparison. If anyone knows why diffcore_std() needs to compare against the_repository, I'd love to hear...

Series overview: Patches 1-3: Mostly mechanical removal of existing uses Patches 4-5: Simple hammer to prevent the problem from returning

Elijah Newren (6):
merge,diff: remove the_repository check before prefetching blobs
merge-ort: pass repository to write_tree()
merge-ort: replace the_repository with opt->repo
merge-ort: replace the_hash_algo with opt->repo->hash_algo
merge-ort: prevent the_repository from coming back
replay: prevent the_repository from coming back

 diff.c            |  2 +-
 diffcore-break.c  |  2 +-
 diffcore-rename.c |  4 +-
 merge-ort.c       | 94 +++++++++++++++++++++++++----------------------
 replay.c          |  6 +++
 5 files changed, 61 insertions(+), 47 deletions(-)

base-commit: 73fd77805fc6406f31c36212846d9e2541d19321
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2048%2Fnewren%2Favoid_the_repository-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2048/newren/avoid_the_repository-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2048

Range-diff vs v2:

1: 7155a0da6f ! 1: e75334a5cf merge,diff: remove the_repository check before prefetching blobs

     @@ diff.c: void diffcore_std(struct diff_options *options)
       	 * decides that it needs inexact rename detection.
       	 */
      -	if (options->repo == the_repository && repo_has_promisor_remote(the_repository) &&
     -+	if (repo_has_promisor_remote(the_repository) &&
     ++	if (repo_has_promisor_remote(options->repo) &&
       	    (options->output_format & output_formats_to_prefetch ||
       	     options->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK))
       		diff_queued_diff_prefetch(options->repo);
     @@ diffcore-break.c: static int should_break(struct repository *r,
       		return 0; /* they are the same */
       
      -	if (r == the_repository && repo_has_promisor_remote(the_repository)) {
     -+	if (repo_has_promisor_remote(the_repository)) {
     ++	if (repo_has_promisor_remote(r)) {
       		options.missing_object_cb = diff_queued_diff_prefetch;
       		options.missing_object_data = r;
       	}
     @@ diffcore-rename.c: static int find_basename_matches(struct diff_options *options
       	}
       
      -	if (options->repo == the_repository && repo_has_promisor_remote(the_repository)) {
     -+	if (repo_has_promisor_remote(the_repository)) {
     ++	if (repo_has_promisor_remote(options->repo)) {
       		dpf_options.missing_object_cb = basename_prefetch;
       		dpf_options.missing_object_data = &prefetch_options;
       	}
     @@ diffcore-rename.c: void diffcore_rename_extended(struct diff_options *options,
       	/* Finish setting up dpf_options */
       	prefetch_options.skip_unmodified = skip_unmodified;
      -	if (options->repo == the_repository && repo_has_promisor_remote(the_repository)) {
     -+	if (repo_has_promisor_remote(the_repository)) {
     ++	if (repo_has_promisor_remote(options->repo)) {
       		dpf_options.missing_object_cb = inexact_prefetch;
       		dpf_options.missing_object_data = &prefetch_options;
       	}
     @@ merge-ort.c: static void prefetch_for_content_merges(struct merge_options *opt,
       	struct oid_array to_fetch = OID_ARRAY_INIT;
       
      -	if (opt->repo != the_repository || !repo_has_promisor_remote(the_repository))
     -+	if (!repo_has_promisor_remote(the_repository))
     ++	if (!repo_has_promisor_remote(opt->repo))
       		return;
       
       	for (e = &plist->items[plist->nr-1]; e >= plist->items; --e) {
 2:  911cba991b = 2:  a9a9d422a3 merge-ort: pass repository to write_tree()
 3:  68af47ed18 ! 3:  4ebfcb08a5 merge-ort: replace the_repository with opt->repo

@@ Commit message
merge-ort: replace the_repository with opt->repo

We have a perfectly valid repository available and do not need to use
- the_repository, except for one location in
- prefetch_for_content_merges().
+ the_repository.

@@ merge-ort.c: static int read_oid_strbuf(struct merge_options *opt,
if (!buf) {
path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,
path, NULL, NULL, NULL,
-@@ merge-ort.c: static void prefetch_for_content_merges(struct merge_options *opt,
- struct string_list_item *e;
- struct oid_array to_fetch = OID_ARRAY_INIT;
-
-- if (!repo_has_promisor_remote(the_repository))
-+ if (!repo_has_promisor_remote(opt->repo))
- return;
-
- for (e = &plist->items[plist->nr-1]; e >= plist->items; --e) {
@@ merge-ort.c: static int checkout(struct merge_options *opt,
unpack_opts.verbose_update = (opt->verbosity > 2);
unpack_opts.fn = twoway_merge;

 4:  bfa68716af = 4:  09076d81b6 merge-ort: replace the_hash_algo with opt->repo->hash_algo
 5:  932d945c9b = 5:  42a2576878 merge-ort: prevent the_repository from coming back
 6:  67db46f34f ! 6:  0654d04584 replay: prevent the_repository from coming back

@@ Commit message
coming back.

Define the_repository to make it a compilation error so that they don't
- come back any more.
+ come back any more; the repo parameter plumbed through the various
+ functions can be used instead.

Signature · 1 line
gitgitgadget
#25 lore raw

PATCH v3 1/6 merge,diff: remove the_repository check before prefetching blobs

From: Elijah Newren <[email protected]>

Prefetching of blobs from promisor remotes was added to diff in 7fbbcb21b162 (diff: batch fetching of missing blobs, 2019-04-05). In that commit,

https://lore.kernel.org/git/[email protected]/

was squashed into

https://lore.kernel.org/git/44de02e584f449481e6fb00cf35d74adf0192e9d.1553895166.git.jonathantanmy@google.com/

without the extra explanation about the squashed changes being added to the commit message; in particular, this explanation from that first link is absent:

Quoted 3 lines

Also, prefetch only if the repository being diffed is the_repository (because we do not support lazy fetching for any other repository
anyway).

Then, later, this checking was spread from diff.c to diffcore-rename.c and diffcore-break.c by 95acf11a3dc3 (diff: restrict when prefetching occurs, 2020-04-07) and then further split in d331dd3b0c82 (diffcore-rename: allow different missing_object_cb functions, 2021-06-22). I also copied the logic from prefetching blobs from diff.c to merge-ort.c in 2bff554b23e8 (merge-ort: add prefetching for content merges, 2021-06-22).

The reason for all these checks was noted above -- we only supported lazy fetching for the_repository. However, that changed with ef830cc43412 (promisor-remote: teach lazy-fetch in any repo, 2021-06-17), so these checks are now unnecessary. Remove them.

---
 diff.c            | 2 +-
 diffcore-break.c  | 2 +-
 diffcore-rename.c | 4 ++--
 merge-ort.c       | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)
diff.c +1 -1
index 35b903a9a0..9091e041b7 100644
@@ -7176,7 +7176,7 @@ void diffcore_std(struct diff_options *options)
 	 * If no prefetching occurs, diffcore_rename() will prefetch if it
 	 * decides that it needs inexact rename detection.
 	 */
-	if (options->repo == the_repository && repo_has_promisor_remote(the_repository) &&
+	if (repo_has_promisor_remote(options->repo) &&
 	    (options->output_format & output_formats_to_prefetch ||
 	     options->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK))
 		diff_queued_diff_prefetch(options->repo);
diffcore-break.c +1 -1
index c4c2173f30..91ae5e8dbb 100644
@@ -69,7 +69,7 @@ static int should_break(struct repository *r,
 	    oideq(&src->oid, &dst->oid))
 		return 0; /* they are the same */
 
-	if (r == the_repository && repo_has_promisor_remote(the_repository)) {
+	if (repo_has_promisor_remote(r)) {
 		options.missing_object_cb = diff_queued_diff_prefetch;
 		options.missing_object_data = r;
 	}
diffcore-rename.c +2 -2
index d9476db35a..c797d8ed2f 100644
@@ -987,7 +987,7 @@ static int find_basename_matches(struct diff_options *options,
 			strintmap_set(&dests, base, i);
 	}
 
-	if (options->repo == the_repository && repo_has_promisor_remote(the_repository)) {
+	if (repo_has_promisor_remote(options->repo)) {
 		dpf_options.missing_object_cb = basename_prefetch;
 		dpf_options.missing_object_data = &prefetch_options;
 	}
@@ -1574,7 +1574,7 @@ void diffcore_rename_extended(struct diff_options *options,
 
 	/* Finish setting up dpf_options */
 	prefetch_options.skip_unmodified = skip_unmodified;
-	if (options->repo == the_repository && repo_has_promisor_remote(the_repository)) {
+	if (repo_has_promisor_remote(options->repo)) {
 		dpf_options.missing_object_cb = inexact_prefetch;
 		dpf_options.missing_object_data = &prefetch_options;
 	}
merge-ort.c +1 -1
index 0a59d1e596..27a58a735d 100644
@@ -4438,7 +4438,7 @@ static void prefetch_for_content_merges(struct merge_options *opt,
 	struct string_list_item *e;
 	struct oid_array to_fetch = OID_ARRAY_INIT;
 
-	if (opt->repo != the_repository || !repo_has_promisor_remote(the_repository))
+	if (!repo_has_promisor_remote(opt->repo))
 		return;
 
 	for (e = &plist->items[plist->nr-1]; e >= plist->items; --e) {
Signature · 1 line
gitgitgadget
#26 lore raw

PATCH v3 2/6 merge-ort: pass repository to write_tree()

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

In order to get rid of a usage of the_repository, we need to know the value of opt->repo; pass it along to write_tree(). Once we have the repository, though, we no longer need to pass opt->repo->hash_algo->rawsz, we can have write_tree() look up that value itself.

---
 merge-ort.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
merge-ort.c +8 -8
index 27a58a735d..289a61822f 100644
@@ -3822,15 +3822,16 @@ static int tree_entry_order(const void *a_, const void *b_)
 				 b->string, strlen(b->string), bmi->result.mode);
 }
 
-static int write_tree(struct object_id *result_oid,
+static int write_tree(struct repository *repo,
+		      struct object_id *result_oid,
 		      struct string_list *versions,
-		      unsigned int offset,
-		      size_t hash_size)
+		      unsigned int offset)
 {
 	size_t maxlen = 0, extra;
 	unsigned int nr;
 	struct strbuf buf = STRBUF_INIT;
 	int i, ret = 0;
+	size_t hash_size = repo->hash_algo->rawsz;
 
 	assert(offset <= versions->nr);
 	nr = versions->nr - offset;
@@ -3856,7 +3857,7 @@ static int write_tree(struct object_id *result_oid,
 	}
 
 	/* Write this object file out, and record in result_oid */
-	if (odb_write_object(the_repository->objects, buf.buf,
+	if (odb_write_object(repo->objects, buf.buf,
 			     buf.len, OBJ_TREE, result_oid))
 		ret = -1;
 	strbuf_release(&buf);
@@ -4026,8 +4027,8 @@ static int write_completed_directory(struct merge_options *opt,
 		dir_info->is_null = 0;
 		dir_info->result.mode = S_IFDIR;
 		if (record_tree &&
-		    write_tree(&dir_info->result.oid, &info->versions, offset,
-			       opt->repo->hash_algo->rawsz) < 0)
+		    write_tree(opt->repo, &dir_info->result.oid, &info->versions,
+			       offset) < 0)
 			ret = -1;
 	}
 
@@ -4573,8 +4574,7 @@ static int process_entries(struct merge_options *opt,
 		BUG("dir_metadata accounting completely off; shouldn't happen");
 	}
 	if (record_tree &&
-	    write_tree(result_oid, &dir_metadata.versions, 0,
-		       opt->repo->hash_algo->rawsz) < 0)
+	    write_tree(opt->repo, result_oid, &dir_metadata.versions, 0) < 0)
 		ret = -1;
 cleanup:
 	string_list_clear(&plist, 0);
Signature · 1 line
gitgitgadget
#27 lore raw

PATCH v3 3/6 merge-ort: replace the_repository with opt->repo

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

We have a perfectly valid repository available and do not need to use the_repository.

---
 merge-ort.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)
merge-ort.c +21 -21
index 289a61822f..9b6a4c312e 100644
@@ -1732,9 +1732,9 @@ static int collect_merge_info(struct merge_options *opt,
 	info.data = opt;
 	info.show_all_errors = 1;
 
-	if (repo_parse_tree(the_repository, merge_base) < 0 ||
-	    repo_parse_tree(the_repository, side1) < 0 ||
-	    repo_parse_tree(the_repository, side2) < 0)
+	if (repo_parse_tree(opt->repo, merge_base) < 0 ||
+	    repo_parse_tree(opt->repo, side1) < 0 ||
+	    repo_parse_tree(opt->repo, side2) < 0)
 		return -1;
 	init_tree_desc(t + 0, &merge_base->object.oid,
 		       merge_base->buffer, merge_base->size);
@@ -2136,9 +2136,9 @@ static int merge_3way(struct merge_options *opt,
 		name2 = mkpathdup("%s:%s", opt->branch2,  pathnames[2]);
 	}
 
-	read_mmblob(&orig, the_repository->objects, o);
-	read_mmblob(&src1, the_repository->objects, a);
-	read_mmblob(&src2, the_repository->objects, b);
+	read_mmblob(&orig, opt->repo->objects, o);
+	read_mmblob(&src1, opt->repo->objects, a);
+	read_mmblob(&src2, opt->repo->objects, b);
 
 	merge_status = ll_merge(result_buf, path, &orig, base,
 				&src1, name1, &src2, name2,
@@ -2254,7 +2254,7 @@ static int handle_content_merge(struct merge_options *opt,
 		}
 
 		if (!ret && record_object &&
-		    odb_write_object(the_repository->objects, result_buf.ptr, result_buf.size,
+		    odb_write_object(opt->repo->objects, result_buf.ptr, result_buf.size,
 				     OBJ_BLOB, &result->oid)) {
 			path_msg(opt, ERROR_OBJECT_WRITE_FAILED, 0,
 				 pathnames[0], pathnames[1], pathnames[2], NULL,
@@ -3713,7 +3713,7 @@ static int read_oid_strbuf(struct merge_options *opt,
 	void *buf;
 	enum object_type type;
 	unsigned long size;
-	buf = odb_read_object(the_repository->objects, oid, &type, &size);
+	buf = odb_read_object(opt->repo->objects, oid, &type, &size);
 	if (!buf) {
 		path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,
 			 path, NULL, NULL, NULL,
@@ -4619,10 +4619,10 @@ static int checkout(struct merge_options *opt,
 	unpack_opts.verbose_update = (opt->verbosity > 2);
 	unpack_opts.fn = twoway_merge;
 	unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */
-	if (repo_parse_tree(the_repository, prev) < 0)
+	if (repo_parse_tree(opt->repo, prev) < 0)
 		return -1;
 	init_tree_desc(&trees[0], &prev->object.oid, prev->buffer, prev->size);
-	if (repo_parse_tree(the_repository, next) < 0)
+	if (repo_parse_tree(opt->repo, next) < 0)
 		return -1;
 	init_tree_desc(&trees[1], &next->object.oid, next->buffer, next->size);
 
@@ -5280,7 +5280,7 @@ redo:
 
 	if (result->clean >= 0) {
 		if (!opt->mergeability_only) {
-			result->tree = repo_parse_tree_indirect(the_repository,
+			result->tree = repo_parse_tree_indirect(opt->repo,
 								&working_tree_oid);
 			if (!result->tree)
 				die(_("unable to read tree (%s)"),
@@ -5309,7 +5309,7 @@ static void merge_ort_internal(struct merge_options *opt,
 	struct strbuf merge_base_abbrev = STRBUF_INIT;
 
 	if (!merge_bases) {
-		if (repo_get_merge_bases(the_repository, h1, h2,
+		if (repo_get_merge_bases(opt->repo, h1, h2,
 					 &merge_bases) < 0) {
 			result->clean = -1;
 			goto out;
@@ -5440,20 +5440,20 @@ static void merge_recursive_config(struct merge_options *opt, int ui)
 {
 	char *value = NULL;
 	int renormalize = 0;
-	repo_config_get_int(the_repository, "merge.verbosity", &opt->verbosity);
-	repo_config_get_int(the_repository, "diff.renamelimit", &opt->rename_limit);
-	repo_config_get_int(the_repository, "merge.renamelimit", &opt->rename_limit);
-	repo_config_get_bool(the_repository, "merge.renormalize", &renormalize);
+	repo_config_get_int(opt->repo, "merge.verbosity", &opt->verbosity);
+	repo_config_get_int(opt->repo, "diff.renamelimit", &opt->rename_limit);
+	repo_config_get_int(opt->repo, "merge.renamelimit", &opt->rename_limit);
+	repo_config_get_bool(opt->repo, "merge.renormalize", &renormalize);
 	opt->renormalize = renormalize;
-	if (!repo_config_get_string(the_repository, "diff.renames", &value)) {
+	if (!repo_config_get_string(opt->repo, "diff.renames", &value)) {
 		opt->detect_renames = git_config_rename("diff.renames", value);
 		free(value);
 	}
-	if (!repo_config_get_string(the_repository, "merge.renames", &value)) {
+	if (!repo_config_get_string(opt->repo, "merge.renames", &value)) {
 		opt->detect_renames = git_config_rename("merge.renames", value);
 		free(value);
 	}
-	if (!repo_config_get_string(the_repository, "merge.directoryrenames", &value)) {
+	if (!repo_config_get_string(opt->repo, "merge.directoryrenames", &value)) {
 		int boolval = git_parse_maybe_bool(value);
 		if (0 <= boolval) {
 			opt->detect_directory_renames = boolval ?
@@ -5466,7 +5466,7 @@ static void merge_recursive_config(struct merge_options *opt, int ui)
 		free(value);
 	}
 	if (ui) {
-		if (!repo_config_get_string(the_repository, "diff.algorithm", &value)) {
+		if (!repo_config_get_string(opt->repo, "diff.algorithm", &value)) {
 			long diff_algorithm = parse_algorithm_value(value);
 			if (diff_algorithm < 0)
 				die(_("unknown value for config '%s': %s"), "diff.algorithm", value);
@@ -5474,7 +5474,7 @@ static void merge_recursive_config(struct merge_options *opt, int ui)
 			free(value);
 		}
 	}
-	repo_config(the_repository, git_xmerge_config, NULL);
+	repo_config(opt->repo, git_xmerge_config, NULL);
 }
 
 static void init_merge_options(struct merge_options *opt,
Signature · 1 line
gitgitgadget
#28 lore raw

PATCH v3 4/6 merge-ort: replace the_hash_algo with opt->repo->hash_algo

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

We have a perfectly valid repository available and do not need to use the_hash_algo (a shorthand for the_repository->hash_algo), so use the known repository instead.

---
 merge-ort.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)
merge-ort.c +13 -13
index 9b6a4c312e..60b4675f39 100644
@@ -1857,7 +1857,7 @@ static int merge_submodule(struct merge_options *opt,
 		BUG("submodule deleted on one side; this should be handled outside of merge_submodule()");
 
 	if ((sub_not_initialized = repo_submodule_init(&subrepo,
-		opt->repo, path, null_oid(the_hash_algo)))) {
+		opt->repo, path, null_oid(opt->repo->hash_algo)))) {
 		path_msg(opt, CONFLICT_SUBMODULE_NOT_INITIALIZED, 0,
 			 path, NULL, NULL, NULL,
 			 _("Failed to merge submodule %s (not checked out)"),
@@ -2240,7 +2240,7 @@ static int handle_content_merge(struct merge_options *opt,
 		two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode));
 
 		merge_status = merge_3way(opt, path,
-					  two_way ? null_oid(the_hash_algo) : &o->oid,
+					  two_way ? null_oid(opt->repo->hash_algo) : &o->oid,
 					  &a->oid, &b->oid,
 					  pathnames, extra_marker_size,
 					  &result_buf);
@@ -2272,7 +2272,7 @@ static int handle_content_merge(struct merge_options *opt,
 	} else if (S_ISGITLINK(a->mode)) {
 		int two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode));
 		clean = merge_submodule(opt, pathnames[0],
-					two_way ? null_oid(the_hash_algo) : &o->oid,
+					two_way ? null_oid(opt->repo->hash_algo) : &o->oid,
 					&a->oid, &b->oid, &result->oid);
 		if (clean < 0)
 			return -1;
@@ -2786,7 +2786,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
 		assert(!new_ci->match_mask);
 		new_ci->dirmask = 0;
 		new_ci->stages[1].mode = 0;
-		oidcpy(&new_ci->stages[1].oid, null_oid(the_hash_algo));
+		oidcpy(&new_ci->stages[1].oid, null_oid(opt->repo->hash_algo));
 
 		/*
 		 * Now that we have the file information in new_ci, make sure
@@ -2799,7 +2799,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,
 				continue;
 			/* zero out any entries related to files */
 			ci->stages[i].mode = 0;
-			oidcpy(&ci->stages[i].oid, null_oid(the_hash_algo));
+			oidcpy(&ci->stages[i].oid, null_oid(opt->repo->hash_algo));
 		}
 
 		/* Now we want to focus on new_ci, so reassign ci to it. */
@@ -3214,7 +3214,7 @@ static int process_renames(struct merge_options *opt,
 			if (type_changed) {
 				/* rename vs. typechange */
 				/* Mark the original as resolved by removal */
-				memcpy(&oldinfo->stages[0].oid, null_oid(the_hash_algo),
+				memcpy(&oldinfo->stages[0].oid, null_oid(opt->repo->hash_algo),
 				       sizeof(oldinfo->stages[0].oid));
 				oldinfo->stages[0].mode = 0;
 				oldinfo->filemask &= 0x06;
@@ -4102,7 +4102,7 @@ static int process_entry(struct merge_options *opt,
 			if (ci->filemask & (1 << i))
 				continue;
 			ci->stages[i].mode = 0;
-			oidcpy(&ci->stages[i].oid, null_oid(the_hash_algo));
+			oidcpy(&ci->stages[i].oid, null_oid(opt->repo->hash_algo));
 		}
 	} else if (ci->df_conflict && ci->merged.result.mode != 0) {
 		/*
@@ -4149,7 +4149,7 @@ static int process_entry(struct merge_options *opt,
 				continue;
 			/* zero out any entries related to directories */
 			new_ci->stages[i].mode = 0;
-			oidcpy(&new_ci->stages[i].oid, null_oid(the_hash_algo));
+			oidcpy(&new_ci->stages[i].oid, null_oid(opt->repo->hash_algo));
 		}
 
 		/*
@@ -4271,11 +4271,11 @@ static int process_entry(struct merge_options *opt,
 			new_ci->merged.result.mode = ci->stages[2].mode;
 			oidcpy(&new_ci->merged.result.oid, &ci->stages[2].oid);
 			new_ci->stages[1].mode = 0;
-			oidcpy(&new_ci->stages[1].oid, null_oid(the_hash_algo));
+			oidcpy(&new_ci->stages[1].oid, null_oid(opt->repo->hash_algo));
 			new_ci->filemask = 5;
 			if ((S_IFMT & b_mode) != (S_IFMT & o_mode)) {
 				new_ci->stages[0].mode = 0;
-				oidcpy(&new_ci->stages[0].oid, null_oid(the_hash_algo));
+				oidcpy(&new_ci->stages[0].oid, null_oid(opt->repo->hash_algo));
 				new_ci->filemask = 4;
 			}
 
@@ -4283,11 +4283,11 @@ static int process_entry(struct merge_options *opt,
 			ci->merged.result.mode = ci->stages[1].mode;
 			oidcpy(&ci->merged.result.oid, &ci->stages[1].oid);
 			ci->stages[2].mode = 0;
-			oidcpy(&ci->stages[2].oid, null_oid(the_hash_algo));
+			oidcpy(&ci->stages[2].oid, null_oid(opt->repo->hash_algo));
 			ci->filemask = 3;
 			if ((S_IFMT & a_mode) != (S_IFMT & o_mode)) {
 				ci->stages[0].mode = 0;
-				oidcpy(&ci->stages[0].oid, null_oid(the_hash_algo));
+				oidcpy(&ci->stages[0].oid, null_oid(opt->repo->hash_algo));
 				ci->filemask = 2;
 			}
 
@@ -4415,7 +4415,7 @@ static int process_entry(struct merge_options *opt,
 		/* Deleted on both sides */
 		ci->merged.is_null = 1;
 		ci->merged.result.mode = 0;
-		oidcpy(&ci->merged.result.oid, null_oid(the_hash_algo));
+		oidcpy(&ci->merged.result.oid, null_oid(opt->repo->hash_algo));
 		assert(!ci->df_conflict);
 		ci->merged.clean = !ci->path_conflict;
 	}
Signature · 1 line
gitgitgadget
#29 lore raw

PATCH v3 5/6 merge-ort: prevent the_repository from coming back

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Due to the use of DEFAULT_ABBREV, we cannot get rid of our usage of USE_THE_REPOSITORY_VARIABLE. However, we have removed all other uses of the_repository in merge-ort a few times. But they keep coming back.

Define the_repository to make it a compilation error so that they don't come back any more.

---
 merge-ort.c | 8 ++++++++
 1 file changed, 8 insertions(+)
merge-ort.c +8 -0
index 60b4675f39..00923ce3cd 100644
@@ -53,6 +53,14 @@
 #include "unpack-trees.h"
 #include "xdiff-interface.h"
 
+/*
+ * We technically need USE_THE_REPOSITORY_VARIABLE above for DEFAULT_ABBREV,
+ * but do not want more uses of the_repository.  Prevent them.
+ *
+ * opt->repo is available; use it instead.
+ */
+#define the_repository DO_NOT_USE_THE_REPOSITORY
+
 /*
  * We have many arrays of size 3.  Whenever we have such an array, the
  * indices refer to one of the sides of the three-way merge.  This is so
Signature · 1 line
gitgitgadget
#30 lore raw
In reply to Elijah Newren via GitGitGadget
Quoted 13 lines · Elijah Newren via GitGitGadget

Changes since v2:

* In first patch, actually avoid the_repository when attempting to remove check against the_repository
* Fix commit message of patch 3 due to the new patch 1.
* Slight tweak to commit message of patch 6.
...
As noted in the comments on v1, I actually do not know why prefetch_for_content_merges() needs to use the_repository. When I introduced it back in 2bff554b23e8 (merge-ort: add prefetching for content merges, 2021-06-22), I was just looking at diffcore_std() and trying to mimic how it did the prefetch, and it has such a comparison. If anyone knows why diffcore_std() needs to compare against the_repository, I'd love to hear...

Is this comment still current?

Quoted 7 lines

Elijah Newren (6):
merge,diff: remove the_repository check before prefetching blobs
merge-ort: pass repository to write_tree()
merge-ort: replace the_repository with opt->repo
merge-ort: replace the_hash_algo with opt->repo->hash_algo
merge-ort: prevent the_repository from coming back
replay: prevent the_repository from coming back

I do not seem to see the last step on the list archive.

https://lore.kernel.org/git/[email protected]/

I'll resurrect it using the previous one and ...

Quoted 10 lines

6: 67db46f34f ! 6: 0654d04584 replay: prevent the_repository from coming back
@@ Commit message
coming back.

Define the_repository to make it a compilation error so that they don't
- come back any more.
+ come back any more; the repo parameter plumbed through the various
+ functions can be used instead.

... this piece of information.

#31 lore raw
Quoted 17 lines · Junio C Hamano
Quoted 13 lines · Elijah Newren via GitGitGadget

Changes since v2:

* In first patch, actually avoid the_repository when attempting to remove check against the_repository
* Fix commit message of patch 3 due to the new patch 1.
* Slight tweak to commit message of patch 6.
...
As noted in the comments on v1, I actually do not know why prefetch_for_content_merges() needs to use the_repository. When I introduced it back in 2bff554b23e8 (merge-ort: add prefetching for content merges, 2021-06-22), I was just looking at diffcore_std() and trying to mimic how it did the prefetch, and it has such a comparison. If anyone knows why diffcore_std() needs to compare against the_repository, I'd love to hear...

Quoted 1 line

Is this comment still current?

No, I should have pulled it out of the cover letter since the commit message of patch #1 answers this; sorry for the oversight.

Quoted 9 lines
Quoted 7 lines

Elijah Newren (6):
merge,diff: remove the_repository check before prefetching blobs
merge-ort: pass repository to write_tree()
merge-ort: replace the_repository with opt->repo
merge-ort: replace the_hash_algo with opt->repo->hash_algo
merge-ort: prevent the_repository from coming back
replay: prevent the_repository from coming back

I do not seem to see the last step on the list archive.

Weird.

Quoted 8 lines

https://lore.kernel.org/git/[email protected]/

I'll resurrect it using the previous one and ...

Quoted 4 lines

6: 67db46f34f ! 6: 0654d04584 replay: prevent the_repository from coming back
@@ Commit message
coming back.

Quoted 8 lines
Quoted 6 lines

Define the_repository to make it a compilation error so that they don't
- come back any more.
+ come back any more; the repo parameter plumbed through the various
+ functions can be used instead.

... this piece of information.

Thanks.

#32 lore raw
In reply to Elijah Newren via GitGitGadget
Quoted 15 lines · Elijah Newren via GitGitGadget

Changes since v2:

* In first patch, actually avoid the_repository when attempting to remove check against the_repository
* Fix commit message of patch 3 due to the new patch 1.
* Slight tweak to commit message of patch 6.

Changes since v1:

* Add a preparatory patch removing the_repository check from blob prefetching in both merge-ort and diff*; it's no longer necessary
* Fix casing mismatch
* Simplify the hammer a bit based on the new first patch, but add some simple comments explaining it

Quoted 5 lines

Remove explicit uses of the_repository and the_hash_algo from merge-ort, and since this has now been done multiple times for both merge-ort and replay, implement a small measure to prevent them from returning to either merge-ort or replay.

I reviewed this version (plus patch 6 from v2) and think it is good to go as-is. I went back to check the v1 feedback and thought it all valuable and resolved by this version.

Thanks,
-Stolee

#33 lore raw
Quoted 22 lines · Derrick Stolee
Quoted 15 lines · Elijah Newren via GitGitGadget

Changes since v2:

* In first patch, actually avoid the_repository when attempting to remove check against the_repository
* Fix commit message of patch 3 due to the new patch 1.
* Slight tweak to commit message of patch 6.

Changes since v1:

* Add a preparatory patch removing the_repository check from blob prefetching in both merge-ort and diff*; it's no longer necessary
* Fix casing mismatch
* Simplify the hammer a bit based on the new first patch, but add some simple comments explaining it

Quoted 5 lines

Remove explicit uses of the_repository and the_hash_algo from merge-ort, and since this has now been done multiple times for both merge-ort and replay, implement a small measure to prevent them from returning to either merge-ort or replay.

Quoted 3 lines

I reviewed this version (plus patch 6 from v2) and think it is good to go as-is. I went back to check the v1 feedback and thought it all valuable and resolved by this version.

Likewise. I only had a single comment on v2 that was addressed now. Thanks!

Patrick