{"thread":{"id":"65212","subject":"[PATCH v1] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","startedAt":"2026-03-11T18:17:26Z","lastAt":"2026-03-14T03:18:09Z","messageCount":10,"participants":["Tian Yuchen","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"538650","messageId":"20260311181704.958509-1-cat@malon.dev","threadId":"65212","inReplyTo":null,"subject":"[PATCH v1] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","fromName":"Tian Yuchen","fromEmail":"cat@malon.dev","sentAt":"2026-03-11T18:17:03Z","receivedAt":"2026-03-11T18:17:26Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"The 'cmd_mktree' already receives a 'struct repository *repo' but was\npreviously marked as UNUSED.\n\nPass the 'repo' down the 'mktree-line()' and 'write_tree()'.\nConsequently, remove the 'USE_THE_REPOSITORY_VARIABLE' macro, and\nreplace 'parse_oid_hex()' with its context-aware version\n'parse_oid_hex_algop()'.\n\nSigned-off-by: Tian Yuchen <cat@malon.dev>\n---\n\nI originally intended to attempt the #FIXME in t1006-cat-file.sh.\nI followed the clues all the way here, only to discover that the\nFIXME required a level of expertise far beyond my capabilities,\nso I gave up. However, I spot the global variable here, so I went\nahead and fixed it ;)\n\nTwo questions:\n\nThe 'oid_to_hex' function appears to use 'the_hash_algo' internally.\nSeems that it also implicitly relying on global state. Is there\nanything we should be aware of?\n\nI've always been unsure about who to CC on domain-specific patches,\nso I've only been sending them to Junio and the mailing list. Could\nthis be why my previous patch for a global variable refactor didn't\nreceive any review feedback? Here is the link:\n\nhttps://lore.kernel.org/git/20260302085738.2510514-1-a3205153416@gmail.com/\n\nI would be most grateful if you could provide any feedback. Thank you.\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":"538722","messageId":"abJjYNq_sxeH8yLQ@pks.im","threadId":"65212","inReplyTo":"20260311181704.958509-1-cat@malon.dev","subject":"Re: [PATCH v1] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-12T06:55:28Z","receivedAt":"2026-03-12T06:55:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 12, 2026 at 02:17:03AM +0800, Tian Yuchen wrote:\n> The 'cmd_mktree' already receives a 'struct repository *repo' but was\n> previously marked as UNUSED.\n> \n> Pass the 'repo' down the 'mktree-line()' and 'write_tree()'.\n\nI guess s/the/to/? Also, it's `mktree_line()`, not `mktree-line()`.\n\nOne thing that commit messages should also explain is why a certain\nrefactoring is safe to do. That is, can `repo` ever be `NULL`? For that\nyou have to look at \"git.c\" and figure out whether or not the command\nrequires a repository to exist.\n\n> The 'oid_to_hex' function appears to use 'the_hash_algo' internally.\n> Seems that it also implicitly relying on global state. Is there\n> anything we should be aware of?\n\n`oid_to_hex()` falls back to using `the_hash_algo` in case the object ID\nyou have doesn't have a proper hash specified. So this depends on how\nexactly you construct the object IDs: if you parse them with a proper\nhash algorithm, then you're fine.\n\n> I've always been unsure about who to CC on domain-specific patches,\n> so I've only been sending them to Junio and the mailing list. Could\n> this be why my previous patch for a global variable refactor didn't\n> receive any review feedback? Here is the link:\n\nIt's typically fine to just send to the mailing list, so you wouldn't\neven Cc Junio. Sometimes it's just a matter of capacity, and it's fine\nto eventually send a ping after a week or two have passed without any\nfeedback.\n\nThe patch itself looks good to me, thanks!\n\nPatrick\n"},{"id":"538764","messageId":"af2c4ae3-c273-40ba-bbca-cbbf687b1b91@malon.dev","threadId":"65212","inReplyTo":"abJjYNq_sxeH8yLQ@pks.im","subject":"Re: [PATCH v1] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","fromName":"Tian Yuchen","fromEmail":"cat@malon.dev","sentAt":"2026-03-12T16:21:41Z","receivedAt":"2026-03-12T16:21:48Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Hi Patrick,\n\nThanks for the review!\n\nOn 3/12/26 14:55, Patrick Steinhardt wrote:\n\n> I guess s/the/to/? Also, it's `mktree_line()`, not `mktree-line()`.\n\nThank you for pointing that out. I'll correct it right away.\n\n\n> One thing that commit messages should also explain is why a certain\n> refactoring is safe to do.\n\nI'll add it.\n\n> That is, can `repo` ever be `NULL`? For that\n> you have to look at \"git.c\" and figure out whether or not the command\n> requires a repository to exist.\nI checked git.c and found that there is:\n\n{ \"mktree\", cmd_mktree, RUN_SETUP }\n\nin commands[]. If my understanding is correct, before cmd_mktree is \ncalled, setup_git_directory() must have been fully executed. In that \ncase, if the current directory isn't a valid repository (NULL), it \nshould have already exited at an earlier stage, right?\n\n\n> `oid_to_hex()` falls back to using `the_hash_algo` in case the object ID\n> you have doesn't have a proper hash specified. So this depends on how\n> exactly you construct the object IDs: if you parse them with a proper\n> hash algorithm, then you're fine.\n\nI see. That's pretty much what I had in mind.\n\n> It's typically fine to just send to the mailing list, so you wouldn't\n> even Cc Junio. Sometimes it's just a matter of capacity, and it's fine\n> to eventually send a ping after a week or two have passed without any\n> feedback.\n\nOh, I see. I thought “repeatedly bringing up a patch no one cares about” \nwould be considered kinda *impolite*. Now I understand. Thank you.\n\nRegards,\n\nYuchen\n"},{"id":"538865","messageId":"abO2iS-7S0G-3Ftf@pks.im","threadId":"65212","inReplyTo":"af2c4ae3-c273-40ba-bbca-cbbf687b1b91@malon.dev","subject":"Re: [PATCH v1] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-13T07:02:33Z","receivedAt":"2026-03-13T07:02:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 13, 2026 at 12:21:41AM +0800, Tian Yuchen wrote:\n> > It's typically fine to just send to the mailing list, so you wouldn't\n> > even Cc Junio. Sometimes it's just a matter of capacity, and it's fine\n> > to eventually send a ping after a week or two have passed without any\n> > feedback.\n> \n> Oh, I see. I thought “repeatedly bringing up a patch no one cares about”\n> would be considered kinda *impolite*. Now I understand. Thank you.\n\nI mean if you nudge every second day that would certainly be considered\nimpolite. But nuding after a week or two, and then maybe nudging again\nafter a month is certainly fine. It just happens that patches fall\nthrough the cracks.\n\nThese aren't strict numbers by the way, it's mostly pulled out of thin\nair with some (hopefully) common sense applied to it by me.\n\nPatrick\n"},{"id":"538907","messageId":"xmqqpl577m3y.fsf@gitster.g","threadId":"65212","inReplyTo":"af2c4ae3-c273-40ba-bbca-cbbf687b1b91@malon.dev","subject":"Re: [PATCH v1] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-13T16:03:29Z","receivedAt":"2026-03-13T16:03:34Z","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>> That is, can `repo` ever be `NULL`? For that\n>> you have to look at \"git.c\" and figure out whether or not the command\n>> requires a repository to exist.\n> I checked git.c and found that there is:\n>\n> { \"mktree\", cmd_mktree, RUN_SETUP }\n>\n> in commands[]. If my understanding is correct, before cmd_mktree is \n> called, setup_git_directory() must have been fully executed. In that \n> case, if the current directory isn't a valid repository (NULL), it \n> should have already exited at an earlier stage, right?\n\nThere is one corner case; upon \"git foo -h\", your cmd_foo() will get\nrepo==NULL when the command is run outside a repository.  As long as\nyour cmd_foo() asks parse_options() to react to \"-h\" (which gives\nthe help message and then exits) before it uses repo assuming it\ncannot be NULL, you are safe.\n"},{"id":"538914","messageId":"4fb9c915-7246-4c55-b7c6-b4ef7ca91230@malon.dev","threadId":"65212","inReplyTo":"xmqqpl577m3y.fsf@gitster.g","subject":"Re: [PATCH v1] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","fromName":"Tian Yuchen","fromEmail":"cat@malon.dev","sentAt":"2026-03-13T17:15:23Z","receivedAt":"2026-03-13T17:15:33Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"On 3/14/26 00:03, Junio C Hamano wrote:\n\n> There is one corner case; upon \"git foo -h\", your cmd_foo() will get\n> repo==NULL when the command is run outside a repository.  As long as\n> your cmd_foo() asks parse_options() to react to \"-h\" (which gives\n> the help message and then exits) before it uses repo assuming it\n> cannot be NULL, you are safe.\n\nI was completely blown away Σ( ° △ °)\n\nThanks for pointing that out. Otherwise, I wouldn’t have been able to \nfigure it out no matter how hard I try.\n\nI just took a quick look at the code:\n\n> \tconst struct option option[] = {\n> \t\tOPT_BOOL('z', NULL, &nul_term_line, N_(\"input is NUL terminated\")),\n> \t\tOPT_SET_INT( 0 , \"missing\", &allow_missing, N_(\"allow missing objects\"), 1),\n> \t\tOPT_SET_INT( 0 , \"batch\", &is_batch_mode, N_(\"allow creation of more than one tree\"), 1),\n> \t\tOPT_END()\n> \t};\n> \n> \tac = parse_options(ac, av, prefix, option, mktree_usage, 0);\n> \tgetline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;\n> \n> \twhile (!got_eof) {\n> \t\twhile (1) { ...\n\nI think if there's a '-h' parameter, it gets intercepted in \nparse_options() and the process exits before repo is called. So there’s \nnothing to worry about, right?\n\n\nBy the way, I find it a bit confusing that the'`-h' parameter — which is \nsolely used for documentation query — is parsed and intercepted within a \nfunction that handles actual business logic. Wouldn’t it be more \nappropriate to intercept it at a higher level? Is this a technical debt? \nI don’t intend to write a patch to fix this, but this parameter has made \nme realize that this odd way of coding (at least in my understanding) is \nhighly unpredictable for someone with limited experience like me. I \ndon't know if there are any troubleshooting methods other than asking \nsomeone with experience.\n\nRegards,\n\nYuchen\n"},{"id":"538920","messageId":"xmqqzf4b4ntq.fsf@gitster.g","threadId":"65212","inReplyTo":"4fb9c915-7246-4c55-b7c6-b4ef7ca91230@malon.dev","subject":"Re: [PATCH v1] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-13T17:54:41Z","receivedAt":"2026-03-13T17:54:44Z","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> On 3/14/26 00:03, Junio C Hamano wrote:\n>\n>> There is one corner case; upon \"git foo -h\", your cmd_foo() will get\n>> repo==NULL when the command is run outside a repository.  As long as\n>> your cmd_foo() asks parse_options() to react to \"-h\" (which gives\n>> the help message and then exits) before it uses repo assuming it\n>> cannot be NULL, you are safe.\n>\n> I was completely blown away Σ( ° △ °)\n>\n> Thanks for pointing that out. Otherwise, I wouldn’t have been able to \n> figure it out no matter how hard I try.\n>\n> I just took a quick look at the code:\n>\n>> \tconst struct option option[] = {\n>> \t\tOPT_BOOL('z', NULL, &nul_term_line, N_(\"input is NUL terminated\")),\n>> \t\tOPT_SET_INT( 0 , \"missing\", &allow_missing, N_(\"allow missing objects\"), 1),\n>> \t\tOPT_SET_INT( 0 , \"batch\", &is_batch_mode, N_(\"allow creation of more than one tree\"), 1),\n>> \t\tOPT_END()\n>> \t};\n>> \n>> \tac = parse_options(ac, av, prefix, option, mktree_usage, 0);\n>> \tgetline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;\n>> \n>> \twhile (!got_eof) {\n>> \t\twhile (1) { ...\n>\n> I think if there's a '-h' parameter, it gets intercepted in \n> parse_options() and the process exits before repo is called. So there’s \n> nothing to worry about, right?\n\nCorrect.  The function calls parse_options() before it looks at \"repo\".\n\n> By the way, I find it a bit confusing that the'`-h' parameter — which is \n> solely used for documentation query — is parsed and intercepted within a \n> function that handles actual business logic.\n\nI strongly disagree your idea that 'z' is more business logic than\n'h' is.  Both are equally relevant.\n"},{"id":"538922","messageId":"e448f98d-58be-409d-9ff2-ae45442dbded@malon.dev","threadId":"65212","inReplyTo":"xmqqzf4b4ntq.fsf@gitster.g","subject":"Re: [PATCH v1] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","fromName":"Tian Yuchen","fromEmail":"cat@malon.dev","sentAt":"2026-03-13T18:12:07Z","receivedAt":"2026-03-13T18:12:18Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"On 3/14/26 01:54, Junio C Hamano wrote:\n\n> I strongly disagree your idea that 'z' is more business logic than\n> 'h' is.  Both are equally relevant.\n\nPerhaps I didn't explain myself clearly :(\n\nI do understand that *currently* both are part of the business logic. \nHowever, what puzzles me is: why is it written this way? Why isn't -h \nintercepted at the outer global level, but instead handed off to a \nfunction like parse_options() for interception?\n\nIs this due to historical reasons?\n\nPlease forgive my slowness. I would appreciate it if you could offer \nsome guidance!\n\nThanks,\n\nYuchen\n"},{"id":"538930","messageId":"xmqqh5qj4h1z.fsf@gitster.g","threadId":"65212","inReplyTo":"e448f98d-58be-409d-9ff2-ae45442dbded@malon.dev","subject":"Re: [PATCH v1] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-13T20:20:56Z","receivedAt":"2026-03-13T20:20:59Z","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> On 3/14/26 01:54, Junio C Hamano wrote:\n>\n>> I strongly disagree your idea that 'z' is more business logic than\n>> 'h' is.  Both are equally relevant.\n>\n> Perhaps I didn't explain myself clearly :(\n>\n> I do understand that *currently* both are part of the business logic. \n> However, what puzzles me is: why is it written this way? Why isn't -h \n> intercepted at the outer global level, but instead handed off to a \n> function like parse_options() for interception?\n>\n> Is this due to historical reasons?\n>\n> Please forgive my slowness. I would appreciate it if you could offer \n> some guidance!\n\nIt is perfectly OK to be slow.  Spend enough time to study the code\nso that you do not have to ask for forgiveness ;-)\n\nIn order to make a useful response to \"-h\", that business logic\nneeds to know what options are available and what argument they take\netc., which is already given to parse_options API.  What makes it\nmake any sense to split it to separate codepath?\n\n"},{"id":"538948","messageId":"77a9fc5cb543579ab925eca9fc9c2b1b@purelymail.com","threadId":"65212","inReplyTo":"xmqqh5qj4h1z.fsf@gitster.g","subject":"Re: [PATCH v1] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE","fromName":"Tian Yuchen","fromEmail":"cat@malon.dev","sentAt":"2026-03-14T03:17:58Z","receivedAt":"2026-03-14T03:18:09Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Hi Junio,\n\n> Tian Yuchen <cat@malon.dev> writes:\n> \n>> On 3/14/26 01:54, Junio C Hamano wrote:\n>> \n>>> I strongly disagree your idea that 'z' is more business logic than\n>>> 'h' is.  Both are equally relevant.\n>> \n>> Perhaps I didn't explain myself clearly :(\n>> \n>> I do understand that *currently* both are part of the business logic.\n>> However, what puzzles me is: why is it written this way? Why isn't -h\n>> intercepted at the outer global level, but instead handed off to a\n>> function like parse_options() for interception?\n>> \n>> Is this due to historical reasons?\n>> \n>> Please forgive my slowness. I would appreciate it if you could offer\n>> some guidance!\n> \n> It is perfectly OK to be slow.  Spend enough time to study the code\n> so that you do not have to ask for forgiveness ;-)\n> \n> In order to make a useful response to \"-h\", that business logic\n> needs to know what options are available and what argument they take\n> etc., which is already given to parse_options API.  What makes it\n> make any sense to split it to separate codepath?\n\nI see.\n\nI’ve been thinking about it, and moving it to an external file seems\nto break encapsulation also. If 'option[]' is no longer static, then the \ncode\nthat originally handled this logic would have to be moved to a file like\n'git.c', and the codebase would increase significantly. That’s probably\nnot what we want, right?\n\nThis is not only semantically confusing, but it also doesn't work any\nbetter in practice.\n\nI kept thinking about maintaining a separate framework to intercept\nall of this — I guess I’ve fallen into a certain mindset when it comes \nto\nmodern CLIs. These changes, which do not offer any significant \nadvantages,\nseem to actually undermine performance and local clarity. It really \nisn’t\nworth the effort.\n\nI hadn't actually planned to migrate anything. I was just a bit confused \nas\nto why it was written this way.\n\nThank you,\n\nYuchen\n"}]}