{"thread":{"id":"59924","subject":"[PATCH] submodule: show inconsistent .gitmodules precedence","startedAt":"2023-06-27T23:57:42Z","lastAt":"2023-06-28T01:37:05Z","messageCount":3,"participants":["Glen Choo via GitGitGadget","Junio C Hamano","Glen Choo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"478941","messageId":"pull.1538.git.git.1687910254473.gitgitgadget@gmail.com","threadId":"59924","inReplyTo":null,"subject":"[PATCH] submodule: show inconsistent .gitmodules precedence","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-27T23:57:34Z","receivedAt":"2023-06-27T23:57:42Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo <chooglen@google.com>\n\nDemonstrate, using tests, an inconsistency in how Git treats repeated\nconfiguration keys in .gitmodules depending on whether we read it from\nthe working tree or from an object. Do not attempt to fix it yet,\nbecause we don't know how much test coverage we have here, and what the\n'right' fix is.\n\nWhen a .gitmodules file contains multiple configurations for a submodule\nlike so:\n\n  [submodule \"sub\"]\n    path = path1\n    path = path2\n\nIt's clearly misconfigured, but our docs don't state what we do in this\nsituation. If one checks this with \"test-tool submodule-config\", you'd\nsee that we ignore every value after the first (aka first-one-wins) and\nissue a warning. *But* if you actually tried this with \"git submodule\",\nyou'd find it practically impossible to trigger this behavior - what you\nactually see is last-one-wins!\n\nThe difference between the two is somewhat complicated because there are\ntwo factors at play. The first is a probable bug in how\nparse_config_parameter.overwrite affects the way submodule config is\ncached. In submodule-config.c:parse_config(), when \".overwrite = 1\", the\nsubmodule machinery gladly overwrites the existing value (last-one-wins)\ninstead of issuing the warning (first-one-wins). This is probably a bug\nbecause it seems that .overwrite is intended to overwrite cached values\nfrom a previous .gitmodules (e.g. if we read .gitmodules from the index\nand want to overwrite it with a newer version), not to overwrite values\nthat we read in the same file.\n\nThe second factor is that the two are reading differently cached values:\n\"git submodule\" is reading cached values with \".overwrite = 1\", but\ntest-tool is reading from cached values with \".overwrite = 0\". The\nsubmodule cache stores each submodule config based on the submodule name\nand the .gitmodules oid it was read from (null_oid() if it's from the\nworking tree). Both code paths eventually call repo_read_gitmodules() to\neagerly cache .gitmodules from the working tree, and which happens to\nuse \".overwrite = 1\". \"git submodule\" typically passes null_oid(), which\nreads back this value. However, test-tool reads back values from the\nactual .gitmodules oid. This causes a cache miss, but when we try to\npopulate the cache at that oid, we do it with \".overwrite = 0\", causing\nthe difference in behavior.\n\nTo make this behavior easy to demonstrate, I've opted to teach test-tool\nhow to use null_oid() rather than use a real Git command, but this is\nalmost certainly affecting us in real Git. It's probably flying under\nthe radar due to some combination of submodule_from_[path|name]() being\nrelatively uncommon in the codebase, and such misconfigurations being\nrare in practice.\n\nWe could fix this bug by targeting either of these factors:\n\n- Make \".overwrite = 1\" and \".overwrite = 0\" do the same thing with\n  repeated values in a .gitmodules.\n- Remove the eager caching (repo_read_gitmodules()). It seems like we\n  can lazily populate the cache on a miss, so we might not need this.\n\nBut I'm not sure how long this has been around, and whether users have\ncome to expect one over the other, so I've opted not to send a fix yet.\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n    submodule: show inconsistent .gitmodules precedence\n    \n    While I was writing a .gitmodules parser for jj\n    (https://github.com/martinvonz/jj, check it out, it's great!), a\n    reviewer asked what would happen if a submodule had repeated fields\n    (like .path). It turns out that the answer isn't just undefined (it's\n    nowhere in the docs), it's inconsistent!\n    \n    Here's a bug report patch that demonstrates the issue using test-tool.\n    I've stopped short of sending a fix because 1) I'm frankly not sure what\n    behavior users have come to rely on 2) this problem is multi-faceted, so\n    we could fix this in quite a few ways, but I'm not sure which way is\n    'right'.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1538%2Fchooglen%2Fpush-lzmyuzkpxxxq-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1538/chooglen/push-lzmyuzkpxxxq-v1\nPull-Request: https://github.com/git/git/pull/1538\n\n submodule-config.c               |  7 +++++++\n t/helper/test-submodule-config.c | 22 +++++++++++++++++++++-\n t/t7411-submodule-config.sh      | 22 ++++++++++++++++++++++\n 3 files changed, 50 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 7eb7a0d88d2..1c4b5afa8e4 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -441,6 +441,13 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\t\t\t     me->gitmodules_oid,\n \t\t\t\t\t     name.buf);\n \n+\t/*\n+\t * FIXME me->overwrite=1 is only meant to overwrite existing submodule\n+\t * configurations when we're reading from another .gitmodules (e.g. from\n+\t * another commit), but it also unintentionally changes behavior when\n+\t * there are multiple configurations in a single .gitmodules - instead\n+\t * of respecting the first value, we now respect the last value.\n+\t */\n \tif (!strcmp(item.buf, \"path\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\ndiff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c\nindex 9df2f03ac80..1bb1dc45878 100644\n--- a/t/helper/test-submodule-config.c\n+++ b/t/helper/test-submodule-config.c\n@@ -29,11 +29,31 @@ int cmd__submodule_config(int argc, const char **argv)\n \t\tmy_argc--;\n \t}\n \n-\tif (my_argc % 2 != 0)\n+\tif (my_argc > 1 && my_argc % 2 != 0)\n \t\tdie_usage(argc, argv, \"Wrong number of arguments.\");\n \n \tsetup_git_directory();\n \n+\tif (my_argc == 1) {\n+\t\tconst struct submodule *submodule;\n+\t\tconst char *path_or_name;\n+\n+\t\tpath_or_name = arg[0];\n+\t\tif (lookup_name) {\n+\t\t\tsubmodule = submodule_from_name(the_repository,\n+\t\t\t\t\t\t\tnull_oid(), path_or_name);\n+\t\t} else\n+\t\t\tsubmodule = submodule_from_path(the_repository,\n+\t\t\t\t\t\t\tnull_oid(), path_or_name);\n+\t\tif (!submodule)\n+\t\t\tdie_usage(argc, argv, \"Submodule not found.\");\n+\n+\t\tprintf(\"Submodule name: '%s' for path '%s'\\n\", submodule->name,\n+\t\t       submodule->path);\n+\n+\t\treturn 0;\n+\t}\n+\n \twhile (*arg) {\n \t\tstruct object_id commit_oid;\n \t\tconst struct submodule *submodule;\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex c0167944abd..b67eea7e085 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -258,4 +258,26 @@ test_expect_success 'reading nested submodules config when .gitmodules is not in\n \t)\n '\n \n+test_expect_success 'multiple config fields in .gitmodules' '\n+\ttest_when_finished \"rm -fr super-duplicate\" &&\n+\tmkdir super-duplicate &&\n+\t(cd super-duplicate &&\n+\t\tgit init &&\n+\t\tgit submodule add ../submodule &&\n+\t\tgit config --file .gitmodules --add submodule.submodule.path ignored &&\n+\t\tgit config --file .gitmodules --add submodule.submodule.url ignored &&\n+\t\tgit add .gitmodules &&\n+\t\tgit commit -m \"duplicate fields in .gitmodules\" &&\n+\t\ttest-tool submodule-config HEAD submodule >actual 2>warning &&\n+\t\tgrep \"Skipping second one\" warning &&\n+\t\techo \"Submodule name: ${SQ}submodule${SQ} for path ${SQ}submodule${SQ}\" >expect &&\n+\t\ttest_cmp expect actual &&\n+\t\t# FIXME this should give the same result as \"HEAD\", but there is\n+\t\t#   a bug where if we use null_oid() instead of the real commit\n+\t\t#   id, the second .path gets respected instead of the first.\n+\t\ttest_must_fail test-tool submodule-config submodule 2>null-oid-error &&\n+\t\tgrep \"Submodule not found\" null-oid-error\n+\t)\n+'\n+\n test_done\n\nbase-commit: 6ff334181cfb6485d3ba50843038209a2a253907\n-- \ngitgitgadget\n"},{"id":"478944","messageId":"xmqqo7l0e5x3.fsf@gitster.g","threadId":"59924","inReplyTo":"pull.1538.git.git.1687910254473.gitgitgadget@gmail.com","subject":"Re: [PATCH] submodule: show inconsistent .gitmodules precedence","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-28T01:23:20Z","receivedAt":"2023-06-28T01:23:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Glen Choo via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>   [submodule \"sub\"]\n>     path = path1\n>     path = path2\n>\n> It's clearly misconfigured, but our docs don't state what we do in this\n> situation. If one checks this with \"test-tool submodule-config\", you'd\n> see that we ignore every value after the first (aka first-one-wins) and\n> issue a warning. *But* if you actually tried this with \"git submodule\",\n> you'd find it practically impossible to trigger this behavior - what you\n> actually see is last-one-wins!\n\nThe last-one-wins sounds like a natural outcome for reusing the\nconfig reading machinery, and the first-one-wins sounds like a total\nconfusion, but we probably should fail any operation before the user\nfixes the .gitmodules by removing all but one path for each\nsubmodule.  Otherwise we risk operating on wrong submodules (e.g. we\nmay think we are running deinit on \"sub\" at path #1, but the code\nmay deinit something different).\n\nThanks.\n"},{"id":"478945","messageId":"kl6lsfacqsed.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59924","inReplyTo":"xmqqo7l0e5x3.fsf@gitster.g","subject":"Re: [PATCH] submodule: show inconsistent .gitmodules precedence","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-06-28T01:36:58Z","receivedAt":"2023-06-28T01:37:05Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> The last-one-wins sounds like a natural outcome for reusing the\n> config reading machinery, and the first-one-wins sounds like a total\n> confusion, but we probably should fail any operation before the user\n> fixes the .gitmodules by removing all but one path for each\n> submodule.\n\nAn informal poll amongst Googlers suggests that my team mostly agrees:\nlast-one-wins makes more sense than first-one-wins, but erroring out is\nthe most sensible thing to do.\n\nI'm not sure how reasonable it is to just fail. It makes sense if we\nwere only reading .gitmodules from the working tree (the user can fix\nthat), but we also read .gitmodules from commits, and I don't see (yet)\nhow a user could reasonably recover from that.\n"}]}