{"thread":{"id":"61553","subject":"[PATCH 0/2] Remove some usages of the_repository","startedAt":"2024-05-28T06:32:04Z","lastAt":"2024-05-28T21:41:50Z","messageCount":8,"participants":["Philip Peterson via GitGitGadget","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"495731","messageId":"pull.1728.git.git.1716877921.gitgitgadget@gmail.com","threadId":"61553","inReplyTo":null,"subject":"[PATCH 0/2] Remove some usages of the_repository","fromName":"Philip Peterson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-28T06:31:59Z","receivedAt":"2024-05-28T06:32:04Z","isPatch":true,"sender":{"key":"name:Philip Peterson","avatar":null},"body":"Because usage of the global the_repository is deprecated, remove the usage\nof it in favor of a passed arg representing the repository.\n\nPhilip Peterson (2):\n  add-patch: do not use the_repository\n  apply: do not use the_repository\n\n add-patch.c |  6 +++---\n apply.c     | 30 +++++++++++++++---------------\n 2 files changed, 18 insertions(+), 18 deletions(-)\n\n\nbase-commit: b9cfe4845cb2562584837bc0101c0ab76490a239\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1728%2Fphilip-peterson%2Fpeterson%2Fremove-the-repository-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1728/philip-peterson/peterson/remove-the-repository-v1\nPull-Request: https://github.com/git/git/pull/1728\n-- \ngitgitgadget\n"},{"id":"495732","messageId":"c0bb37db12ea8724431fe4294d591cd8cef73e66.1716877921.git.gitgitgadget@gmail.com","threadId":"61553","inReplyTo":"pull.1728.git.git.1716877921.gitgitgadget@gmail.com","subject":"[PATCH 1/2] add-patch: do not use the_repository","fromName":"Philip Peterson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-28T06:32:00Z","receivedAt":"2024-05-28T06:32:06Z","isPatch":true,"sender":{"key":"name:Philip Peterson","avatar":null},"body":"From: Philip Peterson <philip.c.peterson@gmail.com>\n\nBecause usage of the global the_repository is deprecated, remove\nthe usage of it in favor of a passed arg representing the repository.\n\nSigned-off-by: Philip Peterson <philip.c.peterson@gmail.com>\n---\n add-patch.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 2252895c280..b7f09122e1f 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -399,7 +399,7 @@ static void complete_file(char marker, struct hunk *hunk)\n \t\thunk->splittable_into++;\n }\n \n-static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n+static int parse_diff(struct repository *r, struct add_p_state *s, const struct pathspec *ps)\n {\n \tstruct strvec args = STRVEC_INIT;\n \tconst char *diff_algorithm = s->s.interactive_diff_algorithm;\n@@ -419,7 +419,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t\tstrvec_push(&args,\n \t\t\t    /* could be on an unborn branch */\n \t\t\t    !strcmp(\"HEAD\", s->revision) &&\n-\t\t\t    repo_get_oid(the_repository, \"HEAD\", &oid) ?\n+\t\t\t    repo_get_oid(r, \"HEAD\", &oid) ?\n \t\t\t    empty_tree_oid_hex() : s->revision);\n \t}\n \tcolor_arg_index = args.nr;\n@@ -1768,7 +1768,7 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \t    (!s.mode->index_only &&\n \t     repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 1,\n \t\t\t\t\t  NULL, NULL, NULL) < 0) ||\n-\t    parse_diff(&s, ps) < 0) {\n+\t    parse_diff(r, &s, ps) < 0) {\n \t\tadd_p_state_clear(&s);\n \t\treturn -1;\n \t}\n-- \ngitgitgadget\n\n"},{"id":"495734","messageId":"36b44eb4c18cfd805ccecd8df695b0d5ee9c409f.1716877921.git.gitgitgadget@gmail.com","threadId":"61553","inReplyTo":"pull.1728.git.git.1716877921.gitgitgadget@gmail.com","subject":"[PATCH 2/2] apply: do not use the_repository","fromName":"Philip Peterson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-28T06:32:01Z","receivedAt":"2024-05-28T06:32:07Z","isPatch":true,"sender":{"key":"name:Philip Peterson","avatar":null},"body":"From: Philip Peterson <philip.c.peterson@gmail.com>\n\nBecause usage of the global the_repository is deprecated, remove\nthe usage of it in favor of a passed arg representing the repository.\n\nSigned-off-by: Philip Peterson <philip.c.peterson@gmail.com>\n---\n apply.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 901b67e6255..364c05fbd06 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3218,13 +3218,13 @@ static int apply_binary(struct apply_state *state,\n \t\treturn 0; /* deletion patch */\n \t}\n \n-\tif (has_object(the_repository, &oid, 0)) {\n+\tif (has_object(state->repo, &oid, 0)) {\n \t\t/* We already have the postimage */\n \t\tenum object_type type;\n \t\tunsigned long size;\n \t\tchar *result;\n \n-\t\tresult = repo_read_object_file(the_repository, &oid, &type,\n+\t\tresult = repo_read_object_file(state->repo, &oid, &type,\n \t\t\t\t\t       &size);\n \t\tif (!result)\n \t\t\treturn error(_(\"the necessary postimage %s for \"\n@@ -3278,7 +3278,7 @@ static int apply_fragments(struct apply_state *state, struct image *img, struct\n \treturn 0;\n }\n \n-static int read_blob_object(struct strbuf *buf, const struct object_id *oid, unsigned mode)\n+static int read_blob_object(struct repository *r, struct strbuf *buf, const struct object_id *oid, unsigned mode)\n {\n \tif (S_ISGITLINK(mode)) {\n \t\tstrbuf_grow(buf, 100);\n@@ -3288,7 +3288,7 @@ static int read_blob_object(struct strbuf *buf, const struct object_id *oid, uns\n \t\tunsigned long sz;\n \t\tchar *result;\n \n-\t\tresult = repo_read_object_file(the_repository, oid, &type,\n+\t\tresult = repo_read_object_file(r, oid, &type,\n \t\t\t\t\t       &sz);\n \t\tif (!result)\n \t\t\treturn -1;\n@@ -3298,11 +3298,11 @@ static int read_blob_object(struct strbuf *buf, const struct object_id *oid, uns\n \treturn 0;\n }\n \n-static int read_file_or_gitlink(const struct cache_entry *ce, struct strbuf *buf)\n+static int read_file_or_gitlink(struct repository *r, const struct cache_entry *ce, struct strbuf *buf)\n {\n \tif (!ce)\n \t\treturn 0;\n-\treturn read_blob_object(buf, &ce->oid, ce->ce_mode);\n+\treturn read_blob_object(r, buf, &ce->oid, ce->ce_mode);\n }\n \n static struct patch *in_fn_table(struct apply_state *state, const char *name)\n@@ -3443,12 +3443,12 @@ static int load_patch_target(struct apply_state *state,\n \t\t\t     unsigned expected_mode)\n {\n \tif (state->cached || state->check_index) {\n-\t\tif (read_file_or_gitlink(ce, buf))\n+\t\tif (read_file_or_gitlink(state->repo, ce, buf))\n \t\t\treturn error(_(\"failed to read %s\"), name);\n \t} else if (name) {\n \t\tif (S_ISGITLINK(expected_mode)) {\n \t\t\tif (ce)\n-\t\t\t\treturn read_file_or_gitlink(ce, buf);\n+\t\t\t\treturn read_file_or_gitlink(state->repo, ce, buf);\n \t\t\telse\n \t\t\t\treturn SUBMODULE_PATCH_WITHOUT_INDEX;\n \t\t} else if (has_symlink_leading_path(name, strlen(name))) {\n@@ -3510,14 +3510,14 @@ static int load_preimage(struct apply_state *state,\n \treturn 0;\n }\n \n-static int resolve_to(struct image *image, const struct object_id *result_id)\n+static int resolve_to(struct repository *r, struct image *image, const struct object_id *result_id)\n {\n \tunsigned long size;\n \tenum object_type type;\n \n \tclear_image(image);\n \n-\timage->buf = repo_read_object_file(the_repository, result_id, &type,\n+\timage->buf = repo_read_object_file(r, result_id, &type,\n \t\t\t\t\t   &size);\n \tif (!image->buf || type != OBJ_BLOB)\n \t\tdie(\"unable to read blob object %s\", oid_to_hex(result_id));\n@@ -3539,9 +3539,9 @@ static int three_way_merge(struct apply_state *state,\n \n \t/* resolve trivial cases first */\n \tif (oideq(base, ours))\n-\t\treturn resolve_to(image, theirs);\n+\t\treturn resolve_to(state->repo, image, theirs);\n \telse if (oideq(base, theirs) || oideq(ours, theirs))\n-\t\treturn resolve_to(image, ours);\n+\t\treturn resolve_to(state->repo, image, ours);\n \n \tread_mmblob(&base_file, base);\n \tread_mmblob(&our_file, ours);\n@@ -3636,8 +3636,8 @@ static int try_threeway(struct apply_state *state,\n \t/* Preimage the patch was prepared for */\n \tif (patch->is_new)\n \t\twrite_object_file(\"\", 0, OBJ_BLOB, &pre_oid);\n-\telse if (repo_get_oid(the_repository, patch->old_oid_prefix, &pre_oid) ||\n-\t\t read_blob_object(&buf, &pre_oid, patch->old_mode))\n+\telse if (repo_get_oid(state->repo, patch->old_oid_prefix, &pre_oid) ||\n+\t\t read_blob_object(state->repo, &buf, &pre_oid, patch->old_mode))\n \t\treturn error(_(\"repository lacks the necessary blob to perform 3-way merge.\"));\n \n \tif (state->apply_verbosity > verbosity_silent && patch->direct_to_threeway)\n@@ -4164,7 +4164,7 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list)\n \t\t\telse\n \t\t\t\treturn error(_(\"sha1 information is lacking or \"\n \t\t\t\t\t       \"useless for submodule %s\"), name);\n-\t\t} else if (!repo_get_oid_blob(the_repository, patch->old_oid_prefix, &oid)) {\n+\t\t} else if (!repo_get_oid_blob(state->repo, patch->old_oid_prefix, &oid)) {\n \t\t\t; /* ok */\n \t\t} else if (!patch->lines_added && !patch->lines_deleted) {\n \t\t\t/* mode-only change: update the current */\n-- \ngitgitgadget\n"},{"id":"495736","messageId":"ZlWHgBROsPBrmM0D@tanuki","threadId":"61553","inReplyTo":"36b44eb4c18cfd805ccecd8df695b0d5ee9c409f.1716877921.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] apply: do not use the_repository","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-28T07:28:00Z","receivedAt":"2024-05-28T07:28:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, May 28, 2024 at 06:32:01AM +0000, Philip Peterson via GitGitGadget wrote:\n> diff --git a/apply.c b/apply.c\n> index 901b67e6255..364c05fbd06 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -3218,13 +3218,13 @@ static int apply_binary(struct apply_state *state,\n>  \t\treturn 0; /* deletion patch */\n>  \t}\n>  \n> -\tif (has_object(the_repository, &oid, 0)) {\n> +\tif (has_object(state->repo, &oid, 0)) {\n>  \t\t/* We already have the postimage */\n>  \t\tenum object_type type;\n>  \t\tunsigned long size;\n>  \t\tchar *result;\n>  \n> -\t\tresult = repo_read_object_file(the_repository, &oid, &type,\n> +\t\tresult = repo_read_object_file(state->repo, &oid, &type,\n>  \t\t\t\t\t       &size);\n>  \t\tif (!result)\n>  \t\t\treturn error(_(\"the necessary postimage %s for \"\n\nWe call `get_oid_hex()` in this function, which ultimately ends up\naccessing `the_repository` via `the_hash_algo`. We should likely convert\nthose to `repo_get_oid()`.\n\nThere are also other accesses to `the_hash_algo` in this function, which\nshould be converted to use `state->repo->hash_algo`, as well.\n\n> @@ -3539,9 +3539,9 @@ static int three_way_merge(struct apply_state *state,\n>  \n>  \t/* resolve trivial cases first */\n>  \tif (oideq(base, ours))\n> -\t\treturn resolve_to(image, theirs);\n> +\t\treturn resolve_to(state->repo, image, theirs);\n>  \telse if (oideq(base, theirs) || oideq(ours, theirs))\n> -\t\treturn resolve_to(image, ours);\n> +\t\treturn resolve_to(state->repo, image, ours);\n>  \n>  \tread_mmblob(&base_file, base);\n>  \tread_mmblob(&our_file, ours);\n\nThe calls to `read_mmblob()` also end up accessing `the_repository`.\n\nOther than that the patches look good to me. Thanks for working on this!\n\nPatrick\n"},{"id":"495759","messageId":"xmqqh6eh7v7g.fsf@gitster.g","threadId":"61553","inReplyTo":"ZlWHgBROsPBrmM0D@tanuki","subject":"Re: [PATCH 2/2] apply: do not use the_repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-28T16:33:07Z","receivedAt":"2024-05-28T16:33:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Tue, May 28, 2024 at 06:32:01AM +0000, Philip Peterson via GitGitGadget wrote:\n>> diff --git a/apply.c b/apply.c\n>> index 901b67e6255..364c05fbd06 100644\n>> --- a/apply.c\n>> +++ b/apply.c\n>> @@ -3218,13 +3218,13 @@ static int apply_binary(struct apply_state *state,\n>>  \t\treturn 0; /* deletion patch */\n>>  \t}\n>>  \n>> -\tif (has_object(the_repository, &oid, 0)) {\n>> +\tif (has_object(state->repo, &oid, 0)) {\n>>  \t\t/* We already have the postimage */\n>>  \t\tenum object_type type;\n>>  \t\tunsigned long size;\n>>  \t\tchar *result;\n>>  \n>> -\t\tresult = repo_read_object_file(the_repository, &oid, &type,\n>> +\t\tresult = repo_read_object_file(state->repo, &oid, &type,\n>>  \t\t\t\t\t       &size);\n>>  \t\tif (!result)\n>>  \t\t\treturn error(_(\"the necessary postimage %s for \"\n>\n> We call `get_oid_hex()` in this function, which ultimately ends up\n> accessing `the_repository` via `the_hash_algo`. We should likely convert\n> those to `repo_get_oid()`.\n>\n> There are also other accesses to `the_hash_algo` in this function, which\n> should be converted to use `state->repo->hash_algo`, as well.\n\nWe as a more experienced developers should come up with a way to\nhelp developers who are less familiar with the API set, so that they\ncan chip in this effort to wean ourselves off of globals.\n\nWould a bug like the ones you pointed out be easily caught by say\nrunning \"GIT_TEST_DEFAULT_HASH=sha256 make test\"?\n\nBy the way, for commands like \"git apply\" that can and does work\noutside a repository, a change to use state->repo instead of\nthe_repository is only half a story.  Dealing with cases where there\nis no repository is the other half.  I _think_ we have drawn a\nreasonable line to punt and refuse binary patches and three-way\nfallback outside a repository, but there may be use cases that\nbenefit from being able to do these things in a tarball extract.\n\n\n"},{"id":"495764","messageId":"ZlYS4zHmjKrzFtF6@ncase","threadId":"61553","inReplyTo":"xmqqh6eh7v7g.fsf@gitster.g","subject":"Re: [PATCH 2/2] apply: do not use the_repository","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-28T17:22:43Z","receivedAt":"2024-05-28T17:22:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, May 28, 2024 at 09:33:07AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > On Tue, May 28, 2024 at 06:32:01AM +0000, Philip Peterson via GitGitGadget wrote:\n> >> diff --git a/apply.c b/apply.c\n> >> index 901b67e6255..364c05fbd06 100644\n> >> --- a/apply.c\n> >> +++ b/apply.c\n> >> @@ -3218,13 +3218,13 @@ static int apply_binary(struct apply_state *state,\n> >>  \t\treturn 0; /* deletion patch */\n> >>  \t}\n> >>  \n> >> -\tif (has_object(the_repository, &oid, 0)) {\n> >> +\tif (has_object(state->repo, &oid, 0)) {\n> >>  \t\t/* We already have the postimage */\n> >>  \t\tenum object_type type;\n> >>  \t\tunsigned long size;\n> >>  \t\tchar *result;\n> >>  \n> >> -\t\tresult = repo_read_object_file(the_repository, &oid, &type,\n> >> +\t\tresult = repo_read_object_file(state->repo, &oid, &type,\n> >>  \t\t\t\t\t       &size);\n> >>  \t\tif (!result)\n> >>  \t\t\treturn error(_(\"the necessary postimage %s for \"\n> >\n> > We call `get_oid_hex()` in this function, which ultimately ends up\n> > accessing `the_repository` via `the_hash_algo`. We should likely convert\n> > those to `repo_get_oid()`.\n> >\n> > There are also other accesses to `the_hash_algo` in this function, which\n> > should be converted to use `state->repo->hash_algo`, as well.\n> \n> We as a more experienced developers should come up with a way to\n> help developers who are less familiar with the API set, so that they\n> can chip in this effort to wean ourselves off of globals.\n> \n> Would a bug like the ones you pointed out be easily caught by say\n> running \"GIT_TEST_DEFAULT_HASH=sha256 make test\"?\n\nNo, I don't think so.\n\nI was also thinking about different approaches to this problem overall.\nIdeally, I would want to catch this on the programmatic level so that we\ndo not even have to detect this at runtime. And I think this should be\nfeasible by introducing something similar to the USE_THE_INDEX_VARIABLE\nmacro, which we have recently removed.\n\nIf we had a USE_THE_REPOSITORY_VARIABLE macro that guards declarations\nof:\n\n  - `the_repository`\n\n  - `the_hash_algo`\n\n  - Functions that implicitly rely on either of the above.\n\nThen you could prove that a given code unit does not rely on any of the\nabove anymore by not declaring that macro.\n\nIn fact, these patches prompted me to give this a go this morning. And\nwhile the changes are of course trivial by themselves, I quickly\ndiscovered that they are also pointless because we almost always rely on\neither of the above.\n\nThe most important reason of this is \"hash.h\", where `struct object_id`\nfalls back to `the_hash_algo` in case its own hash algorithm wasn't set.\nIdeally, this would never be the case. But a quick test shows that there\nare many places where we do rely on the fallback value, mostly around\nnull OIDs. Fixing this would be a necessary prerequisite.\n\nAnother important benefit of the macro would be that we stop working\nagainst a moving target. New files shouldn't declare it, and once a file\nhas been refactored and the corresponding macro was removed we would\nknow that we don't add new dependencies on either of the above by\naccident.\n\nPatrick\n"},{"id":"495766","messageId":"xmqq7cfd6dnb.fsf@gitster.g","threadId":"61553","inReplyTo":"ZlYS4zHmjKrzFtF6@ncase","subject":"Re: [PATCH 2/2] apply: do not use the_repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-28T17:37:44Z","receivedAt":"2024-05-28T17:37:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> If we had a USE_THE_REPOSITORY_VARIABLE macro that guards declarations\n> of:\n>\n>   - `the_repository`\n>\n>   - `the_hash_algo`\n>\n>   - Functions that implicitly rely on either of the above.\n>\n> Then you could prove that a given code unit does not rely on any of the\n> above anymore by not declaring that macro.\n\n;-)\n"},{"id":"495770","messageId":"xmqqwmnd397o.fsf@gitster.g","threadId":"61553","inReplyTo":"c0bb37db12ea8724431fe4294d591cd8cef73e66.1716877921.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] add-patch: do not use the_repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-28T21:41:47Z","receivedAt":"2024-05-28T21:41:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Peterson via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> -static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n> +static int parse_diff(struct repository *r, struct add_p_state *s, const struct pathspec *ps)\n\nGiven that add_p_state has add_i_state in it, which in turn as a\npointer to the repository, which is initialized like so:\n\n\n\n        int run_add_p(struct repository *r, enum add_p_mode mode,\n                      const char *revision, const struct pathspec *ps)\n        {\n                struct add_p_state s = {\n                        { r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT\n                };\n                size_t i, binary_count = 0;\n\n                init_add_i_state(&s.s, r);\n\nthis patch looks wrong.  Adding a separate repository pointer to the\nfunction means you are saying that this function should be able to\noperate one repository in 'r' that may be DIFFERENT from the\nrepository add_p_state was initialized for.  I do not think you are\nachieving that with this patch (and I do not think such a feature\nmakes much sense, either).\n\nInstead just leave everything the same as before, and rewrite things\nthat depend on the_repository (either by explicitly referring to it,\nor by implicitly using it, like empty_tree_oid_hex() which hardcodes\nthe use of the_hash_algo which is the_repository->hash_algo) to refer\nto the repository the add_p_state was initialized for.\n"}]}