Remove some usages of the_repository
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
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
@@ -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
PATCH 2/2 apply: do not use the_repository
In reply to Philip Peterson via GitGitGadgetFrom: 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
@@ -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, structreturn 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, unsunsigned 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, unsreturn 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
Re: [PATCH 2/2] apply: do not use the_repository
Quoted 10 lines · Philip Peterson via GitGitGadget
apply.c +1
-1@@ -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
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@@ -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 accessingthe_repositoryviathe_hash_algo. We should likely convert those torepo_get_oid().
Quoted 2 lines
There are also other accesses to
the_hash_algoin this function, which should be converted to usestate->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.
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@@ -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 accessingthe_repositoryviathe_hash_algo. We should likely convert those torepo_get_oid().Quoted 2 lines
There are also other accesses to
the_hash_algoin this function, which should be converted to usestate->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
Re: [PATCH 2/2] apply: do not use the_repository
Quoted 11 lines · Patrick Steinhardt
If we had a
USE_THE_REPOSITORY_VARIABLEmacro 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.
;-)
Re: [PATCH 1/2] add-patch: do not use the_repository
In reply to Philip Peterson via GitGitGadgetadd-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.