Skip to contentSkip to conversation

Remove some usages of the_repository

PATCH In review Philip Peterson via GitGitGadget 3 participants 8 messages May 28, 2024

lore
Share & tools
lore
#0 lore raw

cover Remove some usages of the_repository

Because usage of the global the_repository is deprecated, remove the usage of it in favor of a passed arg representing the repository.

Philip Peterson (2):
add-patch: do not use the_repository
apply: do not use the_repository

 add-patch.c |  6 +++---
 apply.c     | 30 +++++++++++++++---------------
 2 files changed, 18 insertions(+), 18 deletions(-)

base-commit: b9cfe4845cb2562584837bc0101c0ab76490a239
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1728%2Fphilip-peterson%2Fpeterson%2Fremove-the-repository-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1728/philip-peterson/peterson/remove-the-repository-v1
Pull-Request: https://github.com/git/git/pull/1728

Signature · 1 line
gitgitgadget
#1 lore raw

PATCH 1/2 add-patch: do not use the_repository

From: Philip Peterson <[email protected]>

Because usage of the global the_repository is deprecated, remove the usage of it in favor of a passed arg representing the repository.

---
 add-patch.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
add-patch.c +3 -3
index 2252895c280..b7f09122e1f 100644
@@ -399,7 +399,7 @@ static void complete_file(char marker, struct hunk *hunk)
 		hunk->splittable_into++;
 }
 
-static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
+static int parse_diff(struct repository *r, struct add_p_state *s, const struct pathspec *ps)
 {
 	struct strvec args = STRVEC_INIT;
 	const char *diff_algorithm = s->s.interactive_diff_algorithm;
@@ -419,7 +419,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
 		strvec_push(&args,
 			    /* could be on an unborn branch */
 			    !strcmp("HEAD", s->revision) &&
-			    repo_get_oid(the_repository, "HEAD", &oid) ?
+			    repo_get_oid(r, "HEAD", &oid) ?
 			    empty_tree_oid_hex() : s->revision);
 	}
 	color_arg_index = args.nr;
@@ -1768,7 +1768,7 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 	    (!s.mode->index_only &&
 	     repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 1,
 					  NULL, NULL, NULL) < 0) ||
-	    parse_diff(&s, ps) < 0) {
+	    parse_diff(r, &s, ps) < 0) {
 		add_p_state_clear(&s);
 		return -1;
 	}
Signature · 1 line
gitgitgadget
#3 lore raw

PATCH 2/2 apply: do not use the_repository

In reply to Philip Peterson via GitGitGadget

From: Philip Peterson <[email protected]>

Because usage of the global the_repository is deprecated, remove the usage of it in favor of a passed arg representing the repository.

---
 apply.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)
apply.c +15 -15
index 901b67e6255..364c05fbd06 100644
@@ -3218,13 +3218,13 @@ static int apply_binary(struct apply_state *state,
 		return 0; /* deletion patch */
 	}
 
-	if (has_object(the_repository, &oid, 0)) {
+	if (has_object(state->repo, &oid, 0)) {
 		/* We already have the postimage */
 		enum object_type type;
 		unsigned long size;
 		char *result;
 
-		result = repo_read_object_file(the_repository, &oid, &type,
+		result = repo_read_object_file(state->repo, &oid, &type,
 					       &size);
 		if (!result)
 			return error(_("the necessary postimage %s for "
@@ -3278,7 +3278,7 @@ static int apply_fragments(struct apply_state *state, struct image *img, struct
 	return 0;
 }
 
-static int read_blob_object(struct strbuf *buf, const struct object_id *oid, unsigned mode)
+static int read_blob_object(struct repository *r, struct strbuf *buf, const struct object_id *oid, unsigned mode)
 {
 	if (S_ISGITLINK(mode)) {
 		strbuf_grow(buf, 100);
@@ -3288,7 +3288,7 @@ static int read_blob_object(struct strbuf *buf, const struct object_id *oid, uns
 		unsigned long sz;
 		char *result;
 
-		result = repo_read_object_file(the_repository, oid, &type,
+		result = repo_read_object_file(r, oid, &type,
 					       &sz);
 		if (!result)
 			return -1;
@@ -3298,11 +3298,11 @@ static int read_blob_object(struct strbuf *buf, const struct object_id *oid, uns
 	return 0;
 }
 
-static int read_file_or_gitlink(const struct cache_entry *ce, struct strbuf *buf)
+static int read_file_or_gitlink(struct repository *r, const struct cache_entry *ce, struct strbuf *buf)
 {
 	if (!ce)
 		return 0;
-	return read_blob_object(buf, &ce->oid, ce->ce_mode);
+	return read_blob_object(r, buf, &ce->oid, ce->ce_mode);
 }
 
 static struct patch *in_fn_table(struct apply_state *state, const char *name)
@@ -3443,12 +3443,12 @@ static int load_patch_target(struct apply_state *state,
 			     unsigned expected_mode)
 {
 	if (state->cached || state->check_index) {
-		if (read_file_or_gitlink(ce, buf))
+		if (read_file_or_gitlink(state->repo, ce, buf))
 			return error(_("failed to read %s"), name);
 	} else if (name) {
 		if (S_ISGITLINK(expected_mode)) {
 			if (ce)
-				return read_file_or_gitlink(ce, buf);
+				return read_file_or_gitlink(state->repo, ce, buf);
 			else
 				return SUBMODULE_PATCH_WITHOUT_INDEX;
 		} else if (has_symlink_leading_path(name, strlen(name))) {
@@ -3510,14 +3510,14 @@ static int load_preimage(struct apply_state *state,
 	return 0;
 }
 
-static int resolve_to(struct image *image, const struct object_id *result_id)
+static int resolve_to(struct repository *r, struct image *image, const struct object_id *result_id)
 {
 	unsigned long size;
 	enum object_type type;
 
 	clear_image(image);
 
-	image->buf = repo_read_object_file(the_repository, result_id, &type,
+	image->buf = repo_read_object_file(r, result_id, &type,
 					   &size);
 	if (!image->buf || type != OBJ_BLOB)
 		die("unable to read blob object %s", oid_to_hex(result_id));
@@ -3539,9 +3539,9 @@ static int three_way_merge(struct apply_state *state,
 
 	/* resolve trivial cases first */
 	if (oideq(base, ours))
-		return resolve_to(image, theirs);
+		return resolve_to(state->repo, image, theirs);
 	else if (oideq(base, theirs) || oideq(ours, theirs))
-		return resolve_to(image, ours);
+		return resolve_to(state->repo, image, ours);
 
 	read_mmblob(&base_file, base);
 	read_mmblob(&our_file, ours);
@@ -3636,8 +3636,8 @@ static int try_threeway(struct apply_state *state,
 	/* Preimage the patch was prepared for */
 	if (patch->is_new)
 		write_object_file("", 0, OBJ_BLOB, &pre_oid);
-	else if (repo_get_oid(the_repository, patch->old_oid_prefix, &pre_oid) ||
-		 read_blob_object(&buf, &pre_oid, patch->old_mode))
+	else if (repo_get_oid(state->repo, patch->old_oid_prefix, &pre_oid) ||
+		 read_blob_object(state->repo, &buf, &pre_oid, patch->old_mode))
 		return error(_("repository lacks the necessary blob to perform 3-way merge."));
 
 	if (state->apply_verbosity > verbosity_silent && patch->direct_to_threeway)
@@ -4164,7 +4164,7 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list)
 			else
 				return error(_("sha1 information is lacking or "
 					       "useless for submodule %s"), name);
-		} else if (!repo_get_oid_blob(the_repository, patch->old_oid_prefix, &oid)) {
+		} else if (!repo_get_oid_blob(state->repo, patch->old_oid_prefix, &oid)) {
 			; /* ok */
 		} else if (!patch->lines_added && !patch->lines_deleted) {
 			/* mode-only change: update the current */
Signature · 1 line
gitgitgadget
#4 lore raw

Re: [PATCH 2/2] apply: do not use the_repository

Quoted 10 lines · Philip Peterson via GitGitGadget
apply.c +1 -1
index 901b67e6255..364c05fbd06 100644
@@ -3218,13 +3218,13 @@ static int apply_binary(struct apply_state *state,
 		return 0; /* deletion patch */
 	}
 
-	if (has_object(the_repository, &oid, 0)) {
+	if (has_object(state->repo, &oid, 0)) {

apply.c static int apply_binary(struct apply_state *state, · line 3230

Quoted 10 lines
apply.c +1 -1
 		/* We already have the postimage */
 		enum object_type type;
 		unsigned long size;
 		char *result;
 
-		result = repo_read_object_file(the_repository, &oid, &type,
+		result = repo_read_object_file(state->repo, &oid, &type,
 					       &size);
 		if (!result)
 			return error(_("the necessary postimage %s for "

We call get_oid_hex() in this function, which ultimately ends up accessing the_repository via the_hash_algo. We should likely convert those to repo_get_oid().

There are also other accesses to the_hash_algo in this function, which should be converted to use state->repo->hash_algo, as well.

apply.c static int three_way_merge(struct apply_state *state, · line 3547

Quoted 12 lines
apply.c +2 -2
@@ -3539,9 +3539,9 @@ static int three_way_merge(struct apply_state *state,
 
 	/* resolve trivial cases first */
 	if (oideq(base, ours))
-		return resolve_to(image, theirs);
+		return resolve_to(state->repo, image, theirs);
 	else if (oideq(base, theirs) || oideq(ours, theirs))
-		return resolve_to(image, ours);
+		return resolve_to(state->repo, image, ours);
 
 	read_mmblob(&base_file, base);
 	read_mmblob(&our_file, ours);

The calls to read_mmblob() also end up accessing the_repository.

Other than that the patches look good to me. Thanks for working on this!

Patrick

#5 lore raw

Re: [PATCH 2/2] apply: do not use the_repository

Quoted 26 lines · Patrick Steinhardt
Quoted 10 lines · Philip Peterson via GitGitGadget
apply.c +1 -1
index 901b67e6255..364c05fbd06 100644
@@ -3218,13 +3218,13 @@ static int apply_binary(struct apply_state *state,
 		return 0; /* deletion patch */
 	}
 
-	if (has_object(the_repository, &oid, 0)) {
+	if (has_object(state->repo, &oid, 0)) {

apply.c static int apply_binary(struct apply_state *state, · line 3230

Quoted 10 lines
apply.c +1 -1
 		/* We already have the postimage */
 		enum object_type type;
 		unsigned long size;
 		char *result;
 
-		result = repo_read_object_file(the_repository, &oid, &type,
+		result = repo_read_object_file(state->repo, &oid, &type,
 					       &size);
 		if (!result)
 			return error(_("the necessary postimage %s for "

We call get_oid_hex() in this function, which ultimately ends up accessing the_repository via the_hash_algo. We should likely convert those to repo_get_oid().

Quoted 2 lines

There are also other accesses to the_hash_algo in this function, which should be converted to use state->repo->hash_algo, as well.

We as a more experienced developers should come up with a way to help developers who are less familiar with the API set, so that they can chip in this effort to wean ourselves off of globals.

Would a bug like the ones you pointed out be easily caught by say running "GIT_TEST_DEFAULT_HASH=sha256 make test"?

By the way, for commands like "git apply" that can and does work outside a repository, a change to use state->repo instead of the_repository is only half a story. Dealing with cases where there is no repository is the other half. I _think_ we have drawn a reasonable line to punt and refuse binary patches and three-way fallback outside a repository, but there may be use cases that benefit from being able to do these things in a tarball extract.

#6 lore raw

Re: [PATCH 2/2] apply: do not use the_repository

Quoted 31 lines · Junio C Hamano
Quoted 26 lines · Patrick Steinhardt
Quoted 10 lines · Philip Peterson via GitGitGadget
apply.c +1 -1
index 901b67e6255..364c05fbd06 100644
@@ -3218,13 +3218,13 @@ static int apply_binary(struct apply_state *state,
 		return 0; /* deletion patch */
 	}
 
-	if (has_object(the_repository, &oid, 0)) {
+	if (has_object(state->repo, &oid, 0)) {

apply.c static int apply_binary(struct apply_state *state, · line 3230

Quoted 10 lines
apply.c +1 -1
 		/* We already have the postimage */
 		enum object_type type;
 		unsigned long size;
 		char *result;
 
-		result = repo_read_object_file(the_repository, &oid, &type,
+		result = repo_read_object_file(state->repo, &oid, &type,
 					       &size);
 		if (!result)
 			return error(_("the necessary postimage %s for "

We call get_oid_hex() in this function, which ultimately ends up accessing the_repository via the_hash_algo. We should likely convert those to repo_get_oid().

Quoted 2 lines

There are also other accesses to the_hash_algo in this function, which should be converted to use state->repo->hash_algo, as well.

Quoted 6 lines

We as a more experienced developers should come up with a way to help developers who are less familiar with the API set, so that they can chip in this effort to wean ourselves off of globals.

Would a bug like the ones you pointed out be easily caught by say running "GIT_TEST_DEFAULT_HASH=sha256 make test"?

No, I don't think so.

I was also thinking about different approaches to this problem overall. Ideally, I would want to catch this on the programmatic level so that we do not even have to detect this at runtime. And I think this should be feasible by introducing something similar to the USE_THE_INDEX_VARIABLE macro, which we have recently removed.

If we had a USE_THE_REPOSITORY_VARIABLE macro that guards declarations of:

- the_repository

- the_hash_algo

- Functions that implicitly rely on either of the above.

Then you could prove that a given code unit does not rely on any of the above anymore by not declaring that macro.

In fact, these patches prompted me to give this a go this morning. And while the changes are of course trivial by themselves, I quickly discovered that they are also pointless because we almost always rely on either of the above.

The most important reason of this is "hash.h", where struct object_id falls back to the_hash_algo in case its own hash algorithm wasn't set. Ideally, this would never be the case. But a quick test shows that there are many places where we do rely on the fallback value, mostly around null OIDs. Fixing this would be a necessary prerequisite.

Another important benefit of the macro would be that we stop working against a moving target. New files shouldn't declare it, and once a file has been refactored and the corresponding macro was removed we would know that we don't add new dependencies on either of the above by accident.

Patrick

#7 lore raw

Re: [PATCH 2/2] apply: do not use the_repository

Quoted 11 lines · Patrick Steinhardt

If we had a USE_THE_REPOSITORY_VARIABLE macro that guards declarations of:

- the_repository

- the_hash_algo

- Functions that implicitly rely on either of the above.

Then you could prove that a given code unit does not rely on any of the above anymore by not declaring that macro.

;-)

#2 lore raw

Re: [PATCH 1/2] add-patch: do not use the_repository

In reply to Philip Peterson via GitGitGadget

add-patch.c static void complete_file(char marker, struct hunk *hunk) · line 402

Quoted 2 lines · Philip Peterson via GitGitGadget
add-patch.c +1 -1
-static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
+static int parse_diff(struct repository *r, struct add_p_state *s, const struct pathspec *ps)

Given that add_p_state has add_i_state in it, which in turn as a pointer to the repository, which is initialized like so:

        int run_add_p(struct repository *r, enum add_p_mode mode,
                      const char *revision, const struct pathspec *ps)
        {
                struct add_p_state s = {
                        { r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
                };
                size_t i, binary_count = 0;

                init_add_i_state(&s.s, r);

this patch looks wrong. Adding a separate repository pointer to the function means you are saying that this function should be able to operate one repository in 'r' that may be DIFFERENT from the repository add_p_state was initialized for. I do not think you are achieving that with this patch (and I do not think such a feature makes much sense, either).

Instead just leave everything the same as before, and rewrite things that depend on the_repository (either by explicitly referring to it, or by implicitly using it, like empty_tree_oid_hex() which hardcodes the use of the_hash_algo which is the_repository->hash_algo) to refer to the repository the add_p_state was initialized for.