{"thread":{"id":"54700","subject":"[PATCH] submodule: fix fetch_in_submodule logic","startedAt":"2020-11-24T09:06:29Z","lastAt":"2020-11-24T21:14:22Z","messageCount":2,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"410646","messageId":"X7zM/d1CVuyCstZo@coredump.intra.peff.net","threadId":"54700","inReplyTo":null,"subject":"[PATCH] submodule: fix fetch_in_submodule logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-24T09:06:05Z","receivedAt":"2020-11-24T09:06:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a fix on top of ab/retire-parse-remote, which is now in next. I\nthink submodule fetching is pretty broken, so we should do this or\nsomething like it soon.\n\n-- >8 --\nCommit 1c1518071c (submodule: use \"fetch\" logic instead of custom remote\ndiscovery, 2020-11-14) rewrote the logic in fetch_in_submodule to do:\n\n  elif test \"$2\" -ne \"\"\n\nBut this is nonsense in shell: -ne is for numeric comparisons. This\nshould be \"=\" or more idiomatically:\n\n  elif test -n \"$2\"\n\nBut once we fix that, many tests start failing. Because that commit\nintroduced another problem. The caller that passes 3 arguments looks\nlike this:\n\n    fetch_in_submodule \"$sm_path\" $depth \"$sha1\"\n\nNote the unquoted $depth parameter. When it isn't set, the function will\nsee only 2 arguments, and the function has no idea if what it sees in $2\nis an option to go on the command line, or a refspec to pass on stdin.\nIn the old code before that commit:\n\n   fetch_in_submodule () (\n        sanitize_submodule_env &&\n        cd \"$1\" &&\n  -     case \"$2\" in\n  -     '')\n  -             git fetch ;;\n  -     *)\n  -             shift\n  -             git fetch $(get_default_remote) \"$@\" ;;\n  -     esac\n\nwe treated those the same, so it didn't matter. But in the new logic\n(with my fix above):\n\n  +     if test $# -eq 3\n  +     then\n  +             echo \"$3\" | git fetch --stdin \"$2\"\n  +     elif test -n \"$n\"\n  +     then\n  +             git fetch \"$2\"\n  +     else\n  +             git fetch\n  +     fi\n\nwe use the number of parameters to distinguish the two. Let's insist\nthat the caller pass an empty string for positional parameter two if\nthey want to have a third parameter after it.\n\nBut that still leaves one problem. In the --stdin block, we\nunconditionally pass \"$2\" to git-fetch, even if it's the empty string.\nRather than add another conditional, we can use :+ parameter expansion\nto include it only if it's non-empty. In fact, we can do the same for\nthe elif, too, simplifying it further. Technically this is overkill,\nsince we know the --depth parameter will not have whitespace (and\nindeed, most callers do not bother quoting it), but it doesn't hurt for\nthe function to be careful.\n\nIt's somewhat amazing that no tests were failing. I think what happened\nis that:\n\n  - the 3-arg form rarely triggered; any call with a non-empty $depth\n    and a $sha1 would work, but one with an empty $depth would only have\n    2 arguments\n\n  - because of the wrong arguments to \"test\", the shell would complain\n    and exit non-zero. So we never ran the middle conditional at all\n\n  - that left every call running \"git fetch\" with no arguments. A\n    well-written test could have detected the distinction here, but in\n    practice omitting --depth just means fetching more commits, and\n    fetching everything (rather than a single sha1) works as long as the\n    commit in question is reachable\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nA few side notes:\n\n  - I imagine the mixup between \"ne\" and \"=\" is because they have the\n    opposite meaning in perl. :)\n\n  - it probably wouldn't hurt to beef up the tests, especially around\n    fetching an unreachable sha1, but after getting lost for an hour in\n    the spaghetti of the submodule code and its tests, I gave up. I do\n    at least feel this code is being exercised (because once the initial\n    problem is fixed, tons of things fail).\n\n git-submodule.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 86ad60c05c..eb90f18229 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -412,17 +412,17 @@ is_tip_reachable () (\n \ttest -z \"$rev\"\n )\n \n+# usage: fetch_in_submodule <module_path> [<depth>] [<sha1>]\n+# Because arguments are positional, use an empty string to omit <depth>\n+# but include <sha1>.\n fetch_in_submodule () (\n \tsanitize_submodule_env &&\n \tcd \"$1\" &&\n \tif test $# -eq 3\n \tthen\n-\t\techo \"$3\" | git fetch --stdin \"$2\"\n-\telif test \"$2\" -ne \"\"\n-\tthen\n-\t\tgit fetch \"$2\"\n+\t\techo \"$3\" | git fetch --stdin ${2:+\"$2\"}\n \telse\n-\t\tgit fetch\n+\t\tgit fetch ${2:+\"$2\"}\n \tfi\n )\n \n@@ -603,7 +603,7 @@ cmd_update()\n \t\t\t\t# Now we tried the usual fetch, but $sha1 may\n \t\t\t\t# not be reachable from any of the refs\n \t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n-\t\t\t\tfetch_in_submodule \"$sm_path\" $depth \"$sha1\" ||\n+\t\t\t\tfetch_in_submodule \"$sm_path\" \"$depth\" \"$sha1\" ||\n \t\t\t\tdie \"$(eval_gettext \"Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n \t\t\tfi\n \n-- \n2.29.2.784.g3af989cc94\n"},{"id":"410712","messageId":"xmqqzh36ihkc.fsf@gitster.c.googlers.com","threadId":"54700","inReplyTo":"X7zM/d1CVuyCstZo@coredump.intra.peff.net","subject":"Re: [PATCH] submodule: fix fetch_in_submodule logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-24T21:13:55Z","receivedAt":"2020-11-24T21:14:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This is a fix on top of ab/retire-parse-remote, which is now in next. I\n> think submodule fetching is pretty broken, so we should do this or\n> something like it soon.\n>\n> -- >8 --\n> Commit 1c1518071c (submodule: use \"fetch\" logic instead of custom remote\n> discovery, 2020-11-14) rewrote the logic in fetch_in_submodule to do:\n>\n>   elif test \"$2\" -ne \"\"\n>\n> But this is nonsense in shell: -ne is for numeric comparisons. This\n> should be \"=\" or more idiomatically:\n>\n>   elif test -n \"$2\"\n>\n> But once we fix that, many tests start failing. Because that commit\n> introduced another problem. The caller that passes 3 arguments looks\n> like this:\n>\n>     fetch_in_submodule \"$sm_path\" $depth \"$sha1\"\n>\n> Note the unquoted $depth parameter. When it isn't set, the function will\n> see only 2 arguments, and the function has no idea if what it sees in $2\n> is an option to go on the command line, or a refspec to pass on stdin.\n> In the old code before that commit:\n>\n>    fetch_in_submodule () (\n>         sanitize_submodule_env &&\n>         cd \"$1\" &&\n>   -     case \"$2\" in\n>   -     '')\n>   -             git fetch ;;\n>   -     *)\n>   -             shift\n>   -             git fetch $(get_default_remote) \"$@\" ;;\n>   -     esac\n>\n> we treated those the same, so it didn't matter. But in the new logic\n> (with my fix above):\n>\n>   +     if test $# -eq 3\n>   +     then\n>   +             echo \"$3\" | git fetch --stdin \"$2\"\n>   +     elif test -n \"$n\"\n>   +     then\n>   +             git fetch \"$2\"\n>   +     else\n>   +             git fetch\n>   +     fi\n>\n> we use the number of parameters to distinguish the two. Let's insist\n> that the caller pass an empty string for positional parameter two if\n> they want to have a third parameter after it.\n\nThanks for catching.  I thought we stared at this part long enough\nto be updated between the rounds; it is embarrassing that we've\nmissed it.\n\n>   - it probably wouldn't hurt to beef up the tests, especially around\n>     fetching an unreachable sha1, but after getting lost for an hour in\n>     the spaghetti of the submodule code and its tests, I gave up. I do\n>     at least feel this code is being exercised (because once the initial\n>     problem is fixed, tons of things fail).\n\nTrue.\n\n> +# usage: fetch_in_submodule <module_path> [<depth>] [<sha1>]\n> +# Because arguments are positional, use an empty string to omit <depth>\n> +# but include <sha1>.\n>  fetch_in_submodule () (\n>  \tsanitize_submodule_env &&\n>  \tcd \"$1\" &&\n>  \tif test $# -eq 3\n>  \tthen\n> -\t\techo \"$3\" | git fetch --stdin \"$2\"\n> -\telif test \"$2\" -ne \"\"\n> -\tthen\n> -\t\tgit fetch \"$2\"\n> +\t\techo \"$3\" | git fetch --stdin ${2:+\"$2\"}\n>  \telse\n> -\t\tgit fetch\n> +\t\tgit fetch ${2:+\"$2\"}\n>  \tfi\n>  )\n\nMakes sense.  Thanks.\n\n> @@ -603,7 +603,7 @@ cmd_update()\n>  \t\t\t\t# Now we tried the usual fetch, but $sha1 may\n>  \t\t\t\t# not be reachable from any of the refs\n>  \t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n> -\t\t\t\tfetch_in_submodule \"$sm_path\" $depth \"$sha1\" ||\n> +\t\t\t\tfetch_in_submodule \"$sm_path\" \"$depth\" \"$sha1\" ||\n>  \t\t\t\tdie \"$(eval_gettext \"Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n>  \t\t\tfi\n"}]}