{"thread":{"id":"65222","subject":"[PATCH v2] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","startedAt":"2026-03-12T16:42:14Z","lastAt":"2026-03-12T19:58:46Z","messageCount":4,"participants":["Tian Yuchen","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"538768","messageId":"20260312164203.964033-1-cat@malon.dev","threadId":"65222","inReplyTo":null,"subject":"[PATCH v2] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","fromName":"Tian Yuchen","fromEmail":"cat@malon.dev","sentAt":"2026-03-12T16:42:03Z","receivedAt":"2026-03-12T16:42:14Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"The 'cmd_mktree()' function already receives a 'struct repository *repo'\npointer, but it was previously marked as UNUSED.\n\nPass the 'repo' pointer down to 'mktree_line()' and 'write_tree()'.\nConsequently, remove the 'USE_THE_REPOSITORY_VARIABLE' macro, replace\nusages of 'the_repository', and swap 'parse_oid_hex()' with its context-aware\nversion 'parse_oid_hex_algop()'.\n\nThis refactoring is safe because 'cmd_mktree()' is registered with the\n'RUN_SETUP' flag in 'git.c', which guarantees that the command is\nexecuted within a initialized repository, ensuring that the passed 'repo'\npointer is never 'NULL'.\n\nSigned-off-by: Tian Yuchen <cat@malon.dev>\n---\n builtin/mktree.c | 19 +++++++++----------\n 1 file changed, 9 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/mktree.c b/builtin/mktree.c\nindex 12772303f5..4084e32476 100644\n--- a/builtin/mktree.c\n+++ b/builtin/mktree.c\n@@ -3,7 +3,6 @@\n  *\n  * Copyright (c) Junio C Hamano, 2006, 2009\n  */\n-#define USE_THE_REPOSITORY_VARIABLE\n #include \"builtin.h\"\n #include \"gettext.h\"\n #include \"hex.h\"\n@@ -46,7 +45,7 @@ static int ent_compare(const void *a_, const void *b_)\n \t\t\t\t b->name, b->len, b->mode);\n }\n \n-static void write_tree(struct object_id *oid)\n+static void write_tree(struct repository *repo, struct object_id *oid)\n {\n \tstruct strbuf buf;\n \tsize_t size;\n@@ -60,10 +59,10 @@ static void write_tree(struct object_id *oid)\n \tfor (i = 0; i < used; i++) {\n \t\tstruct treeent *ent = entries[i];\n \t\tstrbuf_addf(&buf, \"%o %s%c\", ent->mode, ent->name, '\\0');\n-\t\tstrbuf_add(&buf, ent->oid.hash, the_hash_algo->rawsz);\n+\t\tstrbuf_add(&buf, ent->oid.hash, repo->hash_algo->rawsz);\n \t}\n \n-\todb_write_object(the_repository->objects, buf.buf, buf.len, OBJ_TREE, oid);\n+\todb_write_object(repo->objects, buf.buf, buf.len, OBJ_TREE, oid);\n \tstrbuf_release(&buf);\n }\n \n@@ -72,7 +71,7 @@ static const char *const mktree_usage[] = {\n \tNULL\n };\n \n-static void mktree_line(char *buf, int nul_term_line, int allow_missing)\n+static void mktree_line(struct repository *repo, char *buf, int nul_term_line, int allow_missing)\n {\n \tchar *ptr, *ntr;\n \tconst char *p;\n@@ -93,7 +92,7 @@ static void mktree_line(char *buf, int nul_term_line, int allow_missing)\n \t\tdie(\"input format error: %s\", buf);\n \tptr = ntr + 1; /* type */\n \tntr = strchr(ptr, ' ');\n-\tif (!ntr || parse_oid_hex(ntr + 1, &oid, &p) ||\n+\tif (!ntr || parse_oid_hex_algop(ntr + 1, &oid, &p, repo->hash_algo) ||\n \t    *p != '\\t')\n \t\tdie(\"input format error: %s\", buf);\n \n@@ -124,7 +123,7 @@ static void mktree_line(char *buf, int nul_term_line, int allow_missing)\n \n \t/* Check the type of object identified by oid without fetching objects */\n \toi.typep = &obj_type;\n-\tif (odb_read_object_info_extended(the_repository->objects, &oid, &oi,\n+\tif (odb_read_object_info_extended(repo->objects, &oid, &oi,\n \t\t\t\t\t  OBJECT_INFO_LOOKUP_REPLACE |\n \t\t\t\t\t  OBJECT_INFO_QUICK |\n \t\t\t\t\t  OBJECT_INFO_SKIP_FETCH_OBJECT) < 0)\n@@ -155,7 +154,7 @@ static void mktree_line(char *buf, int nul_term_line, int allow_missing)\n int cmd_mktree(int ac,\n \t       const char **av,\n \t       const char *prefix,\n-\t       struct repository *repo UNUSED)\n+\t       struct repository *repo)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct object_id oid;\n@@ -187,7 +186,7 @@ int cmd_mktree(int ac,\n \t\t\t\t\tbreak;\n \t\t\t\tdie(\"input format error: (blank line only valid in batch mode)\");\n \t\t\t}\n-\t\t\tmktree_line(sb.buf, nul_term_line, allow_missing);\n+\t\t\tmktree_line(repo, sb.buf, nul_term_line, allow_missing);\n \t\t}\n \t\tif (is_batch_mode && got_eof && used < 1) {\n \t\t\t/*\n@@ -197,7 +196,7 @@ int cmd_mktree(int ac,\n \t\t\t */\n \t\t\t; /* skip creating an empty tree */\n \t\t} else {\n-\t\t\twrite_tree(&oid);\n+\t\t\twrite_tree(repo, &oid);\n \t\t\tputs(oid_to_hex(&oid));\n \t\t\tfflush(stdout);\n \t\t}\n-- \n2.43.0\n\n"},{"id":"538773","messageId":"xmqqsea5ezwl.fsf@gitster.g","threadId":"65222","inReplyTo":"20260312164203.964033-1-cat@malon.dev","subject":"Re: [PATCH v2] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-12T17:11:22Z","receivedAt":"2026-03-12T17:11:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tian Yuchen <cat@malon.dev> writes:\n\n> The 'cmd_mktree()' function already receives a 'struct repository *repo'\n> pointer, but it was previously marked as UNUSED.\n>\n> Pass the 'repo' pointer down to 'mktree_line()' and 'write_tree()'.\n> Consequently, remove the 'USE_THE_REPOSITORY_VARIABLE' macro, replace\n> usages of 'the_repository', and swap 'parse_oid_hex()' with its context-aware\n> version 'parse_oid_hex_algop()'.\n>\n> This refactoring is safe because 'cmd_mktree()' is registered with the\n> 'RUN_SETUP' flag in 'git.c', which guarantees that the command is\n> executed within a initialized repository, ensuring that the passed 'repo'\n> pointer is never 'NULL'.\n\nRUN_SETUP also guarantees that the repo points at the_repository.\n\nThe patch is not wrong per-se, but at the same time, it is not a\nvery interesting change exactly for this reason.\n\nWhere did you read that dropping USE_THE_REPOSITORY_VARIABLE is a\ngood idea?\n\nAs somebody (Phillip?) said earlier, we probably should update\ndocument and clearly say that removing USE_THE_REPOSITORY_VARIABLE\nis not a high-value target when done in the builtin/ directory, even\nthough it is very desirable thing to do for more library-ish part of\nthe codebase.\n\n> Signed-off-by: Tian Yuchen <cat@malon.dev>\n> ---\n>  builtin/mktree.c | 19 +++++++++----------\n>  1 file changed, 9 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/mktree.c b/builtin/mktree.c\n> index 12772303f5..4084e32476 100644\n> --- a/builtin/mktree.c\n> +++ b/builtin/mktree.c\n> @@ -3,7 +3,6 @@\n>   *\n>   * Copyright (c) Junio C Hamano, 2006, 2009\n>   */\n> -#define USE_THE_REPOSITORY_VARIABLE\n>  #include \"builtin.h\"\n>  #include \"gettext.h\"\n>  #include \"hex.h\"\n> @@ -46,7 +45,7 @@ static int ent_compare(const void *a_, const void *b_)\n>  \t\t\t\t b->name, b->len, b->mode);\n>  }\n>  \n> -static void write_tree(struct object_id *oid)\n> +static void write_tree(struct repository *repo, struct object_id *oid)\n>  {\n>  \tstruct strbuf buf;\n>  \tsize_t size;\n> @@ -60,10 +59,10 @@ static void write_tree(struct object_id *oid)\n>  \tfor (i = 0; i < used; i++) {\n>  \t\tstruct treeent *ent = entries[i];\n>  \t\tstrbuf_addf(&buf, \"%o %s%c\", ent->mode, ent->name, '\\0');\n> -\t\tstrbuf_add(&buf, ent->oid.hash, the_hash_algo->rawsz);\n> +\t\tstrbuf_add(&buf, ent->oid.hash, repo->hash_algo->rawsz);\n>  \t}\n>  \n> -\todb_write_object(the_repository->objects, buf.buf, buf.len, OBJ_TREE, oid);\n> +\todb_write_object(repo->objects, buf.buf, buf.len, OBJ_TREE, oid);\n>  \tstrbuf_release(&buf);\n>  }\n>  \n> @@ -72,7 +71,7 @@ static const char *const mktree_usage[] = {\n>  \tNULL\n>  };\n>  \n> -static void mktree_line(char *buf, int nul_term_line, int allow_missing)\n> +static void mktree_line(struct repository *repo, char *buf, int nul_term_line, int allow_missing)\n>  {\n>  \tchar *ptr, *ntr;\n>  \tconst char *p;\n> @@ -93,7 +92,7 @@ static void mktree_line(char *buf, int nul_term_line, int allow_missing)\n>  \t\tdie(\"input format error: %s\", buf);\n>  \tptr = ntr + 1; /* type */\n>  \tntr = strchr(ptr, ' ');\n> -\tif (!ntr || parse_oid_hex(ntr + 1, &oid, &p) ||\n> +\tif (!ntr || parse_oid_hex_algop(ntr + 1, &oid, &p, repo->hash_algo) ||\n>  \t    *p != '\\t')\n>  \t\tdie(\"input format error: %s\", buf);\n>  \n> @@ -124,7 +123,7 @@ static void mktree_line(char *buf, int nul_term_line, int allow_missing)\n>  \n>  \t/* Check the type of object identified by oid without fetching objects */\n>  \toi.typep = &obj_type;\n> -\tif (odb_read_object_info_extended(the_repository->objects, &oid, &oi,\n> +\tif (odb_read_object_info_extended(repo->objects, &oid, &oi,\n>  \t\t\t\t\t  OBJECT_INFO_LOOKUP_REPLACE |\n>  \t\t\t\t\t  OBJECT_INFO_QUICK |\n>  \t\t\t\t\t  OBJECT_INFO_SKIP_FETCH_OBJECT) < 0)\n> @@ -155,7 +154,7 @@ static void mktree_line(char *buf, int nul_term_line, int allow_missing)\n>  int cmd_mktree(int ac,\n>  \t       const char **av,\n>  \t       const char *prefix,\n> -\t       struct repository *repo UNUSED)\n> +\t       struct repository *repo)\n>  {\n>  \tstruct strbuf sb = STRBUF_INIT;\n>  \tstruct object_id oid;\n> @@ -187,7 +186,7 @@ int cmd_mktree(int ac,\n>  \t\t\t\t\tbreak;\n>  \t\t\t\tdie(\"input format error: (blank line only valid in batch mode)\");\n>  \t\t\t}\n> -\t\t\tmktree_line(sb.buf, nul_term_line, allow_missing);\n> +\t\t\tmktree_line(repo, sb.buf, nul_term_line, allow_missing);\n>  \t\t}\n>  \t\tif (is_batch_mode && got_eof && used < 1) {\n>  \t\t\t/*\n> @@ -197,7 +196,7 @@ int cmd_mktree(int ac,\n>  \t\t\t */\n>  \t\t\t; /* skip creating an empty tree */\n>  \t\t} else {\n> -\t\t\twrite_tree(&oid);\n> +\t\t\twrite_tree(repo, &oid);\n>  \t\t\tputs(oid_to_hex(&oid));\n>  \t\t\tfflush(stdout);\n>  \t\t}\n"},{"id":"538787","messageId":"2c9861c0-fdac-4123-8cd9-4a841755abf3@malon.dev","threadId":"65222","inReplyTo":"xmqqsea5ezwl.fsf@gitster.g","subject":"Re: [PATCH v2] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","fromName":"Tian Yuchen","fromEmail":"cat@malon.dev","sentAt":"2026-03-12T18:49:05Z","receivedAt":"2026-03-12T18:49:21Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Hi Junio,\n\n> RUN_SETUP also guarantees that the repo points at the_repository.\n> \n> The patch is not wrong per-se, but at the same time, it is not a\n> very interesting change exactly for this reason.\n> \n> Where did you read that dropping USE_THE_REPOSITORY_VARIABLE is a\n> good idea?\n\nI mentioned this at the bottom of v1:\n\n> I originally intended to attempt the #FIXME in t1006-cat-file.sh.\n> I followed the clues all the way here, only to discover that the\n> FIXME required a level of expertise far beyond my capabilities,\n> so I gave up. However, I spot the global variable here, so I went\n> ahead and fixed it 😉\n\nIn other words, I just happened to see this thing. I didn't go looking \nfor it ;)\n\n> As somebody (Phillip?) said earlier, we probably should update\n> document and clearly say that removing USE_THE_REPOSITORY_VARIABLE\n> is not a high-value target when done in the builtin/ directory, even\n> though it is very desirable thing to do for more library-ish part of\n> the codebase.\n\nI am fully aware of this, and I did not specifically modify \nthe_repository in builtin/ during previous patches. It's just that this \nmacro makes me particularly uncomfortable, and I believe it would be \nbetter to remove it.\n\nOn the other hand, this patch is indeed boring and useless. Feel free to \nignore it.\n\nRegards,\n\nYuchen\n"},{"id":"538804","messageId":"xmqqfr64es5o.fsf@gitster.g","threadId":"65222","inReplyTo":"2c9861c0-fdac-4123-8cd9-4a841755abf3@malon.dev","subject":"Re: [PATCH v2] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-12T19:58:43Z","receivedAt":"2026-03-12T19:58:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tian Yuchen <cat@malon.dev> writes:\n\n>> As somebody (Phillip?) said earlier, we probably should update\n>> document and clearly say that removing USE_THE_REPOSITORY_VARIABLE\n>> is not a high-value target when done in the builtin/ directory, even\n>> though it is very desirable thing to do for more library-ish part of\n>> the codebase.\n>\n> I am fully aware of this, and I did not specifically modify \n> the_repository in builtin/ during previous patches. It's just that this \n> macro makes me particularly uncomfortable, and I believe it would be \n> better to remove it.\n>\n> On the other hand, this patch is indeed boring and useless. Feel free to \n> ignore it.\n\nNah, I think we do want to keep it; once it is written, it is a\nwaste to discard it, especially given that the change is not wrong\nper-se.  If anything else, having it will save somebody else time\nand effort to do the same thing again ;-).\n\nThanks.\n"}]}