{"thread":{"id":"56145","subject":"[BUG?] submodule-config.c:config_from() has left me confused","startedAt":"2021-07-22T10:23:41Z","lastAt":"2021-07-22T11:24:37Z","messageCount":2,"participants":["Atharva Raykar","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"430903","messageId":"FB8C1154-6092-48C7-96B8-6B6D9A28D9AA@gmail.com","threadId":"56145","inReplyTo":null,"subject":"[BUG?] submodule-config.c:config_from() has left me confused","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-22T10:23:35Z","receivedAt":"2021-07-22T10:23:41Z","isPatch":false,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Hi Git Developers,\n\nWhile I was working on converting submodule code from shell to C, I had\na question about the submodule-config API.\n\nThe usual way of obtaining the submodule configuration is by using the\n'submodule_from_path()' or 'submodule_from_name()' functions. Taking the\nexample of the former, a call to it would look something like:\n\nconst struct submodule *sub = submodule_from_path(repo, oid, path-to-submod);\n\nSo my first question is what is the exact purpose of the second\nargument?\n\nThe API docs say it should be a tree-ish:\n\n   Given a tree-ish in the superproject and a path, return the submodule that\n   is bound at the path in the named tree.\n\nMy guess is that the tree-ish that we pass lets us obtain the submodule\nconfiguration for that specific commit or tree in the superproject.\n\nLooking at the underlying implementation[1], it seems to me that the 'oid'\nargument treats the 'null_oid()' specially to mean \"get the configuration from\nthe latest revision\".\n\nWhile I had these questions, Christian noticed a possible bug in the\nimplementation[2]:\n\n---8<------8<------8<------8<---\n\n> It seems to me that config_from() in submodule-config.c accepts a path or a\n> tree oid and submodule_from_path() just calls config_from().\n> \n> By the way it seems to me that config_from() is buggy when it's passed a\n> tree oid as it does:\n> \n> ```\n>         if (!gitmodule_oid_from_commit(treeish_name, &oid, &rev))\n>                 goto out;\n> ```\n> \n> but gitmodule_oid_from_commit() returns 0 on success so instead of doing a\n> `goto out` which will error out, it should continue to proceed using the oid\n> it got. So I think it should be something like:\n> \n> ```\n>         if (gitmodule_oid_from_commit(treeish_name, &oid, &rev)) {\n>                 switch (lookup_type) {\n>                 case lookup_name:\n>                         submodule = cache_lookup_name(cache, &oid, key);\n>                         break;\n>                 case lookup_path:\n>                         submodule = cache_lookup_path(cache, &oid, key);\n>                         break;\n>                 }\n>                 if (submodule)\n>                         goto out;\n>         }\n> ```\n> \n> Or maybe the gitmodule_oid_from_commit() call should be moved after the switch ().\n\n---8<------8<------8<------8<---\n\nI do agree that it looks wrong, but despite that, it still seems to pass 't7411' [3][4].\n\nThis should have failed according to my understanding:\n\ncat >super/expect <<EOF\nSubmodule name: 'a' for path 'a'\nSubmodule name: 'a' for path 'b'\nSubmodule name: 'submodule' for path 'submodule'\nSubmodule name: 'submodule' for path 'submodule'\nEOF\n\ntest_expect_success 'test parsing and lookup of submodule config by path' '\n\t(cd super &&\n\t\ttest-tool submodule-config \\\n\t\t\tHEAD^ a \\\n\t\t\tHEAD b \\\n\t\t\tHEAD^ submodule \\\n\t\t\tHEAD submodule \\\n\t\t\t\t>actual &&\n\t\ttest_cmp expect actual\n\t)\n'\n\nSince HEAD and HEAD^ are valid tree-ish objects, the gitmodule_oid_from_commit()\nline should have returned early with a NULL object, but that does not happen.\nThe output seems to match what I would expect from the API.\n\nApplying Christian's suggestions pass the same test as well.\n\nI also wonder what is the situation with the case where, the oid is non-null\nand invalid?\n\nIf we look at the code again:\n\nstatic int gitmodule_oid_from_commit(const struct object_id *treeish_name,\n\t\t\t\t     struct object_id *gitmodules_oid,\n\t\t\t\t     struct strbuf *rev)\n{\n\tint ret = 0;\n\n\tif (is_null_oid(treeish_name)) {\n\t\toidclr(gitmodules_oid);\n\t\treturn 1;\n\t}\n\n\tstrbuf_addf(rev, \"%s:.gitmodules\", oid_to_hex(treeish_name));\n\tif (get_oid(rev->buf, gitmodules_oid) >= 0)\n\t\tret = 1;\n\n\treturn ret;\n}\n\n...what happens to the value of gitmodules_oid when get_oid() fails?\n\n*If* it is set to NULL, it would probably lead to undefined behaviour when\nwe try to hash the oid for the submodule cache lookup in [1]. If it is set\nto 'null_oid()' or zero'd out (like with oidclr()), then it should return\nthe submodule from the latest revision, which does not seem desirable, as\nthe input is invalid.\n\nIs this a bug in my understanding or in the program? Or is it a bit of both?\n\nFootnotes:\n---------\n\n[1]: https://github.com/git/git/blob/daab8a564f8bbac55f70f8bf86c070e001a9b006/submodule-config.c#L545-L615\n[2]: https://github.com/tfidfwastaken/git/commit/7ad7a9d1d03653aadfdc87b60e3a152b1cb37f22#r53637226\n[3]: https://github.com/git/git/blob/master/t/t7411-submodule-config.sh\n[4]: https://github.com/git/git/blob/master/t/helper/test-submodule-config.c\n\n---\nAtharva Raykar\nಅಥರ್ವ ರಾಯ್ಕರ್\nअथर्व रायकर\n\n"},{"id":"430906","messageId":"87y29ytwu3.fsf@evledraar.gmail.com","threadId":"56145","inReplyTo":"FB8C1154-6092-48C7-96B8-6B6D9A28D9AA@gmail.com","subject":"Re: [BUG?] submodule-config.c:config_from() has left me confused","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-22T10:59:03Z","receivedAt":"2021-07-22T11:24:37Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jul 22 2021, Atharva Raykar wrote:\n\n> Hi Git Developers,\n\nHi, just skimming this, hopefully someone can get it the attention it\ndeserves...\n\n> Since HEAD and HEAD^ are valid tree-ish objects, the gitmodule_oid_from_commit()\n> line should have returned early with a NULL object, but that does not happen.\n> The output seems to match what I would expect from the API.\n>\n> Applying Christian's suggestions pass the same test as well.\n>\n> I also wonder what is the situation with the case where, the oid is non-null\n> and invalid?\n>\n> If we look at the code again:\n>\n> static int gitmodule_oid_from_commit(const struct object_id *treeish_name,\n> \t\t\t\t     struct object_id *gitmodules_oid,\n> \t\t\t\t     struct strbuf *rev)\n> {\n> \tint ret = 0;\n>\n> \tif (is_null_oid(treeish_name)) {\n> \t\toidclr(gitmodules_oid);\n> \t\treturn 1;\n> \t}\n>\n> \tstrbuf_addf(rev, \"%s:.gitmodules\", oid_to_hex(treeish_name));\n> \tif (get_oid(rev->buf, gitmodules_oid) >= 0)\n> \t\tret = 1;\n>\n> \treturn ret;\n> }\n>\n> ...what happens to the value of gitmodules_oid when get_oid() fails?\n>\n> *If* it is set to NULL, it would probably lead to undefined behaviour when\n> we try to hash the oid for the submodule cache lookup in [1]. If it is set\n> to 'null_oid()' or zero'd out (like with oidclr()), then it should return\n> the submodule from the latest revision, which does not seem desirable, as\n> the input is invalid.\n\nWe won't set it to NULL, we'll att worst oidclr() it, or leave it alone,\nno?\n\nIn general we should not rely on the values we expect to munge if we\nreturn an error. In practice it's probably OK to rely on an oid\npassed-in not being in an inconsistent state in that case.\n\nIt could be, e.g. this might eventually go to get_oid_basic() which in\ncalling get_hash_hex_algop() might partially have copied a hash when it\nfails. I have not traced this codepath but it could happen in general.\n\nIn practice I think it's OK to rely on passing an OID in, and it being\nbad or not, i.e.:\n\n    struct object_id *oid = null_oid();\n    if (get_oid(..., oid))\n        error(...);\n    else\n        /* oid is still == null_oid, not some bizarro value, probably... */\n\n"}]}