{"thread":{"id":"63460","subject":"[PATCH] replay: replace the_repository with repo parameter passed to cmd_replay()","startedAt":"2025-05-14T20:33:30Z","lastAt":"2025-05-15T13:24:22Z","messageCount":3,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"518069","messageId":"pull.1921.git.1747254806067.gitgitgadget@gmail.com","threadId":"63460","inReplyTo":null,"subject":"[PATCH] replay: replace the_repository with repo parameter passed to cmd_replay()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-14T20:33:25Z","receivedAt":"2025-05-14T20:33:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nReplace the_repository everywhere with repo, feed repo from cmd_replay()\nto all the other functions in the file that need it, and remove the\nUNUSED annotation on repo.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    replay: replace the_repository with repo parameter passed to cmd_replay\n    \n    The point of this patch is not to remove USE_THE_REPOSITORY_VARIABLE; I\n    can't yet because DEFAULT_ABBREV and get_commit_output_encoding() both\n    require it and have no current alternatives. However, I still think it's\n    worthwhile to stop using the_repository everywhere while ignoring the\n    repo parameter explicitly passed in. That looks kinda ugly, and since\n    I'm poking around in replay right now, I don't want to push\n    the_repository in even more places when we have the appropriate value\n    available -- especially since that might make my local work conflict\n    should someone else come along and try to clean this up.\n    \n    --color-words is handy when viewing this patch; it may make it easier to\n    see the changes.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1921%2Fnewren%2Freplay-repo-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1921/newren/replay-repo-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1921\n\n builtin/replay.c | 65 ++++++++++++++++++++++++++----------------------\n 1 file changed, 35 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/replay.c b/builtin/replay.c\nindex 032c172b65e..225cef08807 100644\n--- a/builtin/replay.c\n+++ b/builtin/replay.c\n@@ -20,21 +20,22 @@\n #include <oidset.h>\n #include <tree.h>\n \n-static const char *short_commit_name(struct commit *commit)\n+static const char *short_commit_name(struct repository *repo,\n+\t\t\t\t     struct commit *commit)\n {\n-\treturn repo_find_unique_abbrev(the_repository, &commit->object.oid,\n+\treturn repo_find_unique_abbrev(repo, &commit->object.oid,\n \t\t\t\t       DEFAULT_ABBREV);\n }\n \n-static struct commit *peel_committish(const char *name)\n+static struct commit *peel_committish(struct repository *repo, const char *name)\n {\n \tstruct object *obj;\n \tstruct object_id oid;\n \n-\tif (repo_get_oid(the_repository, name, &oid))\n+\tif (repo_get_oid(repo, name, &oid))\n \t\treturn NULL;\n-\tobj = parse_object(the_repository, &oid);\n-\treturn (struct commit *)repo_peel_to_type(the_repository, name, 0, obj,\n+\tobj = parse_object(repo, &oid);\n+\treturn (struct commit *)repo_peel_to_type(repo, name, 0, obj,\n \t\t\t\t\t\t  OBJ_COMMIT);\n }\n \n@@ -50,7 +51,8 @@ static char *get_author(const char *message)\n \treturn NULL;\n }\n \n-static struct commit *create_commit(struct tree *tree,\n+static struct commit *create_commit(struct repository *repo,\n+\t\t\t\t    struct tree *tree,\n \t\t\t\t    struct commit *based_on,\n \t\t\t\t    struct commit *parent)\n {\n@@ -62,7 +64,7 @@ static struct commit *create_commit(struct tree *tree,\n \tstruct commit_extra_header *extra = NULL;\n \tstruct strbuf msg = STRBUF_INIT;\n \tconst char *out_enc = get_commit_output_encoding();\n-\tconst char *message = repo_logmsg_reencode(the_repository, based_on,\n+\tconst char *message = repo_logmsg_reencode(repo, based_on,\n \t\t\t\t\t\t   NULL, out_enc);\n \tconst char *orig_message = NULL;\n \tconst char *exclude_gpgsig[] = { \"gpgsig\", NULL };\n@@ -79,7 +81,7 @@ static struct commit *create_commit(struct tree *tree,\n \t\tgoto out;\n \t}\n \n-\tobj = parse_object(the_repository, &ret);\n+\tobj = parse_object(repo, &ret);\n \n out:\n \tfree_commit_extra_headers(extra);\n@@ -97,7 +99,8 @@ struct ref_info {\n \tint negative_refexprs;\n };\n \n-static void get_ref_information(struct rev_cmdline_info *cmd_info,\n+static void get_ref_information(struct repository *repo,\n+\t\t\t\tstruct rev_cmdline_info *cmd_info,\n \t\t\t\tstruct ref_info *ref_info)\n {\n \tint i;\n@@ -132,14 +135,14 @@ static void get_ref_information(struct rev_cmdline_info *cmd_info,\n \n \t\tif (*refexpr == '^')\n \t\t\trefexpr++;\n-\t\tif (repo_dwim_ref(the_repository, refexpr, strlen(refexpr), &oid, &fullname, 0) != 1)\n+\t\tif (repo_dwim_ref(repo, refexpr, strlen(refexpr), &oid, &fullname, 0) != 1)\n \t\t\tcan_uniquely_dwim = 0;\n \n \t\tif (e->flags & BOTTOM) {\n \t\t\tif (can_uniquely_dwim)\n \t\t\t\tstrset_add(&ref_info->negative_refs, fullname);\n \t\t\tif (!ref_info->negative_refexprs)\n-\t\t\t\tref_info->onto = lookup_commit_reference_gently(the_repository,\n+\t\t\t\tref_info->onto = lookup_commit_reference_gently(repo,\n \t\t\t\t\t\t\t\t\t\t&e->item->oid, 1);\n \t\t\tref_info->negative_refexprs++;\n \t\t} else {\n@@ -152,7 +155,8 @@ static void get_ref_information(struct rev_cmdline_info *cmd_info,\n \t}\n }\n \n-static void determine_replay_mode(struct rev_cmdline_info *cmd_info,\n+static void determine_replay_mode(struct repository *repo,\n+\t\t\t\t  struct rev_cmdline_info *cmd_info,\n \t\t\t\t  const char *onto_name,\n \t\t\t\t  char **advance_name,\n \t\t\t\t  struct commit **onto,\n@@ -160,14 +164,14 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info,\n {\n \tstruct ref_info rinfo;\n \n-\tget_ref_information(cmd_info, &rinfo);\n+\tget_ref_information(repo, cmd_info, &rinfo);\n \tif (!rinfo.positive_refexprs)\n \t\tdie(_(\"need some commits to replay\"));\n \n \tdie_for_incompatible_opt2(!!onto_name, \"--onto\",\n \t\t\t\t  !!*advance_name, \"--advance\");\n \tif (onto_name) {\n-\t\t*onto = peel_committish(onto_name);\n+\t\t*onto = peel_committish(repo, onto_name);\n \t\tif (rinfo.positive_refexprs <\n \t\t    strset_get_size(&rinfo.positive_refs))\n \t\t\tdie(_(\"all positive revisions given must be references\"));\n@@ -175,8 +179,8 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info,\n \t\tstruct object_id oid;\n \t\tchar *fullname = NULL;\n \n-\t\t*onto = peel_committish(*advance_name);\n-\t\tif (repo_dwim_ref(the_repository, *advance_name, strlen(*advance_name),\n+\t\t*onto = peel_committish(repo, *advance_name);\n+\t\tif (repo_dwim_ref(repo, *advance_name, strlen(*advance_name),\n \t\t\t     &oid, &fullname, 0) == 1) {\n \t\t\tfree(*advance_name);\n \t\t\t*advance_name = fullname;\n@@ -245,7 +249,8 @@ static struct commit *mapped_commit(kh_oid_map_t *replayed_commits,\n \treturn kh_value(replayed_commits, pos);\n }\n \n-static struct commit *pick_regular_commit(struct commit *pickme,\n+static struct commit *pick_regular_commit(struct repository *repo,\n+\t\t\t\t\t  struct commit *pickme,\n \t\t\t\t\t  kh_oid_map_t *replayed_commits,\n \t\t\t\t\t  struct commit *onto,\n \t\t\t\t\t  struct merge_options *merge_opt,\n@@ -257,12 +262,12 @@ static struct commit *pick_regular_commit(struct commit *pickme,\n \tbase = pickme->parents->item;\n \treplayed_base = mapped_commit(replayed_commits, base, onto);\n \n-\tresult->tree = repo_get_commit_tree(the_repository, replayed_base);\n-\tpickme_tree = repo_get_commit_tree(the_repository, pickme);\n-\tbase_tree = repo_get_commit_tree(the_repository, base);\n+\tresult->tree = repo_get_commit_tree(repo, replayed_base);\n+\tpickme_tree = repo_get_commit_tree(repo, pickme);\n+\tbase_tree = repo_get_commit_tree(repo, base);\n \n-\tmerge_opt->branch1 = short_commit_name(replayed_base);\n-\tmerge_opt->branch2 = short_commit_name(pickme);\n+\tmerge_opt->branch1 = short_commit_name(repo, replayed_base);\n+\tmerge_opt->branch2 = short_commit_name(repo, pickme);\n \tmerge_opt->ancestor = xstrfmt(\"parent of %s\", merge_opt->branch2);\n \n \tmerge_incore_nonrecursive(merge_opt,\n@@ -275,13 +280,13 @@ static struct commit *pick_regular_commit(struct commit *pickme,\n \tmerge_opt->ancestor = NULL;\n \tif (!result->clean)\n \t\treturn NULL;\n-\treturn create_commit(result->tree, pickme, replayed_base);\n+\treturn create_commit(repo, result->tree, pickme, replayed_base);\n }\n \n int cmd_replay(int argc,\n \t       const char **argv,\n \t       const char *prefix,\n-\t       struct repository *repo UNUSED)\n+\t       struct repository *repo)\n {\n \tconst char *advance_name_opt = NULL;\n \tchar *advance_name = NULL;\n@@ -329,7 +334,7 @@ int cmd_replay(int argc,\n \t\t    \"--advance\", \"--contained\");\n \tadvance_name = xstrdup_or_null(advance_name_opt);\n \n-\trepo_init_revisions(the_repository, &revs, prefix);\n+\trepo_init_revisions(repo, &revs, prefix);\n \n \t/*\n \t * Set desired values for rev walking options here. If they\n@@ -380,7 +385,7 @@ int cmd_replay(int argc,\n \t\trevs.simplify_history = 0;\n \t}\n \n-\tdetermine_replay_mode(&revs.cmdline, onto_name, &advance_name,\n+\tdetermine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name,\n \t\t\t      &onto, &update_refs);\n \n \tif (!onto) /* FIXME: Should handle replaying down to root commit */\n@@ -391,7 +396,7 @@ int cmd_replay(int argc,\n \t\tgoto cleanup;\n \t}\n \n-\tinit_basic_merge_options(&merge_opt, the_repository);\n+\tinit_basic_merge_options(&merge_opt, repo);\n \tmemset(&result, 0, sizeof(result));\n \tmerge_opt.show_rename_progress = 0;\n \tlast_commit = onto;\n@@ -406,8 +411,8 @@ int cmd_replay(int argc,\n \t\tif (commit->parents->next)\n \t\t\tdie(_(\"replaying merge commits is not supported yet!\"));\n \n-\t\tlast_commit = pick_regular_commit(commit, replayed_commits, onto,\n-\t\t\t\t\t\t  &merge_opt, &result);\n+\t\tlast_commit = pick_regular_commit(repo, commit, replayed_commits,\n+\t\t\t\t\t\t  onto, &merge_opt, &result);\n \t\tif (!last_commit)\n \t\t\tbreak;\n \n\nbase-commit: 1a8a4971cc6c179c4dd711f4a7f5d7178f4b3ab7\n-- \ngitgitgadget\n"},{"id":"518076","messageId":"xmqqplga7sij.fsf@gitster.g","threadId":"63460","inReplyTo":"pull.1921.git.1747254806067.gitgitgadget@gmail.com","subject":"Re: [PATCH] replay: replace the_repository with repo parameter passed to cmd_replay()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-14T22:10:44Z","receivedAt":"2025-05-14T22:10:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>     require it and have no current alternatives. However, I still think it's\n>     worthwhile to stop using the_repository everywhere while ignoring the\n>     repo parameter explicitly passed in.\n\nSensible.\n\n> diff --git a/builtin/replay.c b/builtin/replay.c\n> index 032c172b65e..225cef08807 100644\n> --- a/builtin/replay.c\n> +++ b/builtin/replay.c\n> @@ -20,21 +20,22 @@\n>  #include <oidset.h>\n>  #include <tree.h>\n>  \n> -static const char *short_commit_name(struct commit *commit)\n> +static const char *short_commit_name(struct repository *repo,\n> +\t\t\t\t     struct commit *commit)\n>  {\n> -\treturn repo_find_unique_abbrev(the_repository, &commit->object.oid,\n> +\treturn repo_find_unique_abbrev(repo, &commit->object.oid,\n>  \t\t\t\t       DEFAULT_ABBREV);\n>  }\n\nI do not mind this, but I do have to wonder if it is simpler to make\nthe two callers of this \"helper\" (which is not quite helping anything)\nto make these calls themselves.\n\n>  int cmd_replay(int argc,\n>  \t       const char **argv,\n>  \t       const char *prefix,\n> -\t       struct repository *repo UNUSED)\n> +\t       struct repository *repo)\n>  {\n>  \tconst char *advance_name_opt = NULL;\n>  \tchar *advance_name = NULL;\n> @@ -329,7 +334,7 @@ int cmd_replay(int argc,\n>  \t\t    \"--advance\", \"--contained\");\n>  \tadvance_name = xstrdup_or_null(advance_name_opt);\n>  \n> -\trepo_init_revisions(the_repository, &revs, prefix);\n> +\trepo_init_revisions(repo, &revs, prefix);\n\nOK, since this command is marked as RUN_SETUP, it is safe to\nunconditionally use repo here.  The only situation where it is\ncalled with repo==NULL is when somebody said \"git replay -h\" outside\na repository, which would have made parse_options() to do the right\nthing and exited already without reaching this code, so we should be\nable to trust \"repo\" to be usable.\n\nWill queue.  Thanks.\n\n"},{"id":"518136","messageId":"aCXq_ck96l0AjScp@pks.im","threadId":"63460","inReplyTo":"pull.1921.git.1747254806067.gitgitgadget@gmail.com","subject":"Re: [PATCH] replay: replace the_repository with repo parameter passed to cmd_replay()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-15T13:24:13Z","receivedAt":"2025-05-15T13:24:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 14, 2025 at 08:33:25PM +0000, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren <newren@gmail.com>\n> \n> Replace the_repository everywhere with repo, feed repo from cmd_replay()\n> to all the other functions in the file that need it, and remove the\n> UNUSED annotation on repo.\n> \n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>     replay: replace the_repository with repo parameter passed to cmd_replay\n>     \n>     The point of this patch is not to remove USE_THE_REPOSITORY_VARIABLE; I\n>     can't yet because DEFAULT_ABBREV and get_commit_output_encoding() both\n>     require it and have no current alternatives. However, I still think it's\n>     worthwhile to stop using the_repository everywhere while ignoring the\n>     repo parameter explicitly passed in. That looks kinda ugly, and since\n>     I'm poking around in replay right now, I don't want to push\n>     the_repository in even more places when we have the appropriate value\n>     available -- especially since that might make my local work conflict\n>     should someone else come along and try to clean this up.\n\nMakes sense. Especially the first one is something I have encountered as\na frequent blocker for removing `USE_THE_REPOSITORY_VARIABLE`. I guess\nit would be nice to tackle it sooner rather than later.\n\nCc'd Ayush, as he will be working on the global state reduction project\nas part of GSoC.\n\nPatrick\n"}]}