{"thread":{"id":"55693","subject":"Bug in git submodule update --remote","startedAt":"2021-05-14T18:26:46Z","lastAt":"2021-05-22T19:15:57Z","messageCount":8,"participants":["Ben Avison","Atharva Raykar","Christian Couder","Philippe Blain"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"424596","messageId":"c4b27662-1228-a1ff-26fc-637897ffc8e7@riscosopen.org","threadId":"55693","inReplyTo":null,"subject":"Bug in git submodule update --remote","fromName":"Ben Avison","fromEmail":"bavison@riscosopen.org","sentAt":"2021-05-14T18:11:04Z","receivedAt":"2021-05-14T18:26:46Z","isPatch":false,"sender":{"key":"bavison@riscosopen.org","avatar":"https://avatars.githubusercontent.com/u/3324657?v=4"},"body":"A recent-ish change in git 2.27.1, introduced in commit f0a96e8d, has \nalso got me wondering about whether some related functionality is \ncorrect. I'm not sure what the best way to fix things is, so can I \ninvite opinions?\n\nThe scenario: I have a repository with a submodule. The submodule's \nupstream repository uses a fork workflow, so the submodule has two \nremotes, one for pulling in other people's changes, and one for \nuploading my own pull requests.\n\n$ mkdir myproject\n$ cd myproject\n$ git init\n$ git submodule add https://github.com/acme/library\n$ cd library\n$ git remote add -f myfork https://github.com/user/library\n$ cd ..\n$ git add .\n$ git commit -m \"Initial commit\"\n$ git remote add origin https://github.com/user/myproject\n$ git push -u origin master\n\nFurthermore, assume I forked https://github.com/acme/example some time \nago, so that the master branches between it and my fork have diverged.\n\nTime passes. People hack away on both projects. I want to fix a bug or \nimplement a new feature in the library, so I start by ensuring both are \nup-to-date:\n\n$ git pull\n$ git submodule update --remote\n$ cd library\n$ git checkout -b new-feature\n$ # hack away\n$ git add .\n$ git commit -m \"New feature\"\n$ git push -u myfork new-feature\n$ cd ..\n\nSome more time passes, and I want to work on it again. Again, I start by \nensuring I'm up-to-date. Before git 2.27.1, I could do:\n\n$ git submodule update --remote\n\nNow, I get:\n\nfatal: Needed a single revision\nUnable to find current myfork/HEAD revision in submodule path 'library'\n\nWhat's going on is that within \"git submodule update --remote\", the sha1 \nused is formed by looking up the submodule's ref \"<remote>/<branch>\". \nThe change in git 2.27.1 is that if no remote tracking branch is stated \nin the superproject's .gitmodules file, <branch> defaults to \"HEAD\" \nrather than \"master\" as previously. That's fine if <remote> is \"origin\", \nbut in practice, it depends on how the submodule is currently checked out:\n\n* if it's in detached HEAD state, <remote> is set to \"origin\"\n* if its branch is not a tracking branch, \"origin\" is also used\n* but if it's a tracking branch (as I used in my workflow above - not \nuncommon in pull request workflows because you might grant other people \naccess to the branch during the review process) then it looks up the \nname of the remote which is being tracked\n\nFirst observation: a ref called \"myfork/HEAD\" presumably *could* have \nbeen created by the \"git remote add\" command, reflecting that remote's \ndefault branch. Should it?\n\nIn practice, that's not actually what I'd want, though. If I do \"git \nsubmodule update --remote\", I personally normally do so as a shortcut \ncompared to cloning everything again. I don't expect the behaviour to \nchange depending on whether I happen to have left one of the submodules \nchecked out on a tracking branch or not: myfork/master is potentially \nquite out of date compared to origin/master.\n\nIt also makes very little sense to me that issuing \"git submodule update \n--remote\" twice in quick succession would leave us in a different state, \nbecause the first call puts all the submodules into detached HEAD state, \nmeaning that the second call always looks up the remote tracking \nbranches from the submodule's \"origin\" remote.\n\nOne way this could be fixed is that if <branch> turns out to be \"HEAD\", \nwe could force <remote> to be \"origin\". However, this doesn't address \nthe equivalent problem that arises if the remote tracking branch *is* \nnamed in .gitmodules.\n\nI'd therefore like to propose that \"git submodule update --remote\" \n*always* uses the remote named \"origin\" to form the ref used to check \nout the submodule. However, I'm not sure whether everyone would agree \nthat this constitutes a bugfix, or whether I'd need to introduce a new \nswitch to enforce this behaviour.\n\nSo, what do you think?\n\nBen\n"},{"id":"424937","messageId":"EB55447C-538F-44DE-BFC0-A3BD1A970E99@gmail.com","threadId":"55693","inReplyTo":"c4b27662-1228-a1ff-26fc-637897ffc8e7@riscosopen.org","subject":"Re: Bug in git submodule update --remote","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-05-19T10:49:21Z","receivedAt":"2021-05-19T10:49:29Z","isPatch":false,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"On 14-May-2021, at 23:41, Ben Avison <bavison@riscosopen.org> wrote:\n> \n> What's going on is that within \"git submodule update --remote\", the sha1 used is formed by looking up the submodule's ref \"<remote>/<branch>\". The change in git 2.27.1 is that if no remote tracking branch is stated in the superproject's .gitmodules file, <branch> defaults to \"HEAD\" rather than \"master\" as previously. That's fine if <remote> is \"origin\", but in practice, it depends on how the submodule is currently checked out:\n> \n> * if it's in detached HEAD state, <remote> is set to \"origin\"\n> * if its branch is not a tracking branch, \"origin\" is also used\n> * but if it's a tracking branch (as I used in my workflow above - not uncommon in pull request workflows because you might grant other people access to the branch during the review process) then it looks up the name of the remote which is being tracked\n> \n> First observation: a ref called \"myfork/HEAD\" presumably *could* have been created by the \"git remote add\" command, reflecting that remote's default branch. Should it?\n> \n> In practice, that's not actually what I'd want, though. If I do \"git submodule update --remote\", I personally normally do so as a shortcut compared to cloning everything again. I don't expect the behaviour to change depending on whether I happen to have left one of the submodules checked out on a tracking branch or not: myfork/master is potentially quite out of date compared to origin/master.\n> \n> It also makes very little sense to me that issuing \"git submodule update --remote\" twice in quick succession would leave us in a different state, because the first call puts all the submodules into detached HEAD state, meaning that the second call always looks up the remote tracking branches from the submodule's \"origin\" remote.\n\nI agree with you, this behaviour is rather inconsistent, and\ncan be quite surprising to an unsuspecting user.\n\n> One way this could be fixed is that if <branch> turns out to be \"HEAD\", we could force <remote> to be \"origin\". However, this doesn't address the equivalent problem that arises if the remote tracking branch *is* named in .gitmodules.\n> \n> I'd therefore like to propose that \"git submodule update --remote\" *always* uses the remote named \"origin\" to form the ref used to check out the submodule. However, I'm not sure whether everyone would agree that this constitutes a bugfix, or whether I'd need to introduce a new switch to enforce this behaviour.\n> \n> So, what do you think?\n\nIf I understood you correctly, you'd prefer that the updating\nof the submodule should be independent of the ref that is checked\nout in the submodule's directory.\n\nWhile I am not sure of the reason why the design of 'update --remote'\nuses the remote-tracking branch of the submodule, I can imagine\nadding a switch like 'submodule.<name>.remote' that defaults to\n'origin'. Then the behaviour could be changed such that it always\npulls from the remote specified in that option.\n\nThis would help make the behaviour consistent in all the cases you\nmentioned, while also giving the option for a user to update the\nsubmodule from the remote of their choice (which may not be origin).\n\n--\nAtharva Raykar\n\n"},{"id":"424939","messageId":"876768D2-4F22-47F7-A6D4-3C0CB7FD2E6E@gmail.com","threadId":"55693","inReplyTo":"c4b27662-1228-a1ff-26fc-637897ffc8e7@riscosopen.org","subject":"Re: Bug in git submodule update --remote","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-05-19T11:19:05Z","receivedAt":"2021-05-19T11:19:14Z","isPatch":false,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"I am also reposting your message, adding hard line breaks so that\nit displays more readably in the list archive, and also so that\nanyone else can choose to quote you from this.\n\nOn 14-May-2021, at 23:41, Ben Avison <bavison@riscosopen.org> wrote:\n> \n> A recent-ish change in git 2.27.1, introduced in commit f0a96e8d, has\n> also got me wondering about whether some related functionality is\n> correct. I'm not sure what the best way to fix things is, so can I\n> invite opinions?\n> \n> The scenario: I have a repository with a submodule. The submodule's\n> upstream repository uses a fork workflow, so the submodule has two\n> remotes, one for pulling in other people's changes, and one for\n> uploading my own pull requests.\n> \n> $ mkdir myproject\n> $ cd myproject\n> $ git init\n> $ git submodule add https://github.com/acme/library\n> $ cd library\n> $ git remote add -f myfork https://github.com/user/library\n> $ cd ..\n> $ git add .\n> $ git commit -m \"Initial commit\"\n> $ git remote add origin https://github.com/user/myproject\n> $ git push -u origin master\n> \n> Furthermore, assume I forked https://github.com/acme/example some time\n> ago, so that the master branches between it and my fork have diverged.\n> \n> Time passes. People hack away on both projects. I want to fix a bug or\n> implement a new feature in the library, so I start by ensuring both are\n> up-to-date:\n> \n> $ git pull $ git submodule update --remote\n> $ cd library\n> $ git checkout -b new-feature\n> $ # hack away\n> $ git add .\n> $ git commit -m \"New feature\"\n> $ git push -u myfork new-feature\n> $ cd ..\n> \n> Some more time passes, and I want to work on it again. Again, I start by\n> ensuring I'm up-to-date. Before git 2.27.1, I could do:\n> \n> $ git submodule update --remote\n> \n> Now, I get:\n> \n> fatal: Needed a single revision Unable to find current myfork/HEAD\n> revision in submodule path 'library'\n> \n> What's going on is that within \"git submodule update --remote\", the sha1\n> used is formed by looking up the submodule's ref \"<remote>/<branch>\".\n> The change in git 2.27.1 is that if no remote tracking branch is stated\n> in the superproject's .gitmodules file, <branch> defaults to \"HEAD\"\n> rather than \"master\" as previously. That's fine if <remote> is \"origin\",\n> but in practice, it depends on how the submodule is currently checked\n> out:\n> \n> * if it's in detached HEAD state, <remote> is set to \"origin\"\n> * if its branch is not a tracking branch, \"origin\" is also used\n> * but if it's a tracking branch (as I used in my workflow above - not\n>   uncommon in pull request workflows because you might grant other\n>   people access to the branch during the review process) then it looks\n>   up the name of the remote which is being tracked\n> \n> First observation: a ref called \"myfork/HEAD\" presumably *could* have\n> been created by the \"git remote add\" command, reflecting that remote's\n> default branch. Should it?\n> \n> In practice, that's not actually what I'd want, though. If I do \"git\n> submodule update --remote\", I personally normally do so as a shortcut\n> compared to cloning everything again. I don't expect the behaviour to\n> change depending on whether I happen to have left one of the submodules\n> checked out on a tracking branch or not: myfork/master is potentially\n> quite out of date compared to origin/master.\n> \n> It also makes very little sense to me that issuing \"git submodule update\n> --remote\" twice in quick succession would leave us in a different state,\n> because the first call puts all the submodules into detached HEAD state,\n> meaning that the second call always looks up the remote tracking\n> branches from the submodule's \"origin\" remote.\n> \n> One way this could be fixed is that if <branch> turns out to be \"HEAD\",\n> we could force <remote> to be \"origin\". However, this doesn't address\n> the equivalent problem that arises if the remote tracking branch *is*\n> named in .gitmodules.\n> \n> I'd therefore like to propose that \"git submodule update --remote\"\n> *always* uses the remote named \"origin\" to form the ref used to check\n> out the submodule. However, I'm not sure whether everyone would agree\n> that this constitutes a bugfix, or whether I'd need to introduce a new\n> switch to enforce this behaviour.\n> \n> So, what do you think?\n> \n> Ben\n\n"},{"id":"424966","messageId":"a727c1af-4993-684d-d299-00bf1a47cfc7@riscosopen.org","threadId":"55693","inReplyTo":"EB55447C-538F-44DE-BFC0-A3BD1A970E99@gmail.com","subject":"Re: Bug in git submodule update --remote","fromName":"Ben Avison","fromEmail":"bavison@riscosopen.org","sentAt":"2021-05-19T14:41:56Z","receivedAt":"2021-05-19T14:57:43Z","isPatch":false,"sender":{"key":"bavison@riscosopen.org","avatar":"https://avatars.githubusercontent.com/u/3324657?v=4"},"body":"On 19/05/2021 11:49, Atharva Raykar wrote:\n> If I understood you correctly, you'd prefer that the updating of the\n> submodule should be independent of the ref that is checked out in the\n> submodule's directory.\n> \n> While I am not sure of the reason why the design of 'update\n> --remote' uses the remote-tracking branch of the submodule, I can\n> imagine adding a switch like 'submodule.<name>.remote' that defaults\n> to 'origin'. Then the behaviour could be changed such that it always \n> pulls from the remote specified in that option.\n> \n> This would help make the behaviour consistent in all the cases you \n> mentioned, while also giving the option for a user to update the \n> submodule from the remote of their choice (which may not be origin).\n\nI like that solution. Although, I should note that if the user has set\nsubmodule.<name>.remote to something other than 'origin', they will need\nto ensure that submodule.<name>.branch is also set, or they will still\nhit the \"Unable to find current <remote>/HEAD revision in submodule\"\nerror that I initially stumbled on.\n\nHow about an implementation like the following? I introduced a new \"git\nsubmodule--helper\" command rather than modify \"print-default-remote\" for\na couple of reasons:\n\n1) \"print-default-remote\" is also used for \"git submodule sync\" (I'm not\nsure if we should change its behaviour too)\n\n2) \"print-default-remote\" needs to be executed from within the\nsubmodule, and takes no arguments, whereas I need to parse the\nsuperproject's .git/config so need to be executed from the superproject\nand take the submodule path as an argument\n\nThe two functions I added are heavily based on \"git submodule--helper\nremote-branch\". However:\n\n* Unlike with the branch name, I don't fall back to using the name for\nthe remote cached from when we read the .gitmodules file, if it isn't\nfound in .git/config. It doesn't make sense to me for the .gitmodules\nfile to include this information, as any new clones will only contain\n\"origin\" remotes anyway.\n\n* I removed \"struct strbuf sb\" since I don't think it's used.\n\nBen\n\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 9d505a6329..25ce3c8a1d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2444,6 +2444,41 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,\n \treturn 0;\n }\n \n+static const char *remote_submodule_remote(const char *path)\n+{\n+\tconst struct submodule *sub;\n+\tconst char *remote = NULL;\n+\tchar *key;\n+\n+\tsub = submodule_from_path(the_repository, &null_oid, path);\n+\tif (!sub)\n+\t\treturn NULL;\n+\n+\tkey = xstrfmt(\"submodule.%s.remote\", sub->name);\n+\trepo_config_get_string_tmp(the_repository, key, &remote);\n+\tfree(key);\n+\n+\tif (!remote)\n+\t\treturn \"origin\";\n+\n+\treturn remote;\n+}\n+\n+static int resolve_remote_submodule_remote(int argc, const char **argv,\n+\t\tconst char *prefix)\n+{\n+\tconst char *ret;\n+\tif (argc != 2)\n+\t\tdie(\"submodule--helper remote-remote takes exactly one arguments, got %d\", argc);\n+\n+\tret = remote_submodule_remote(argv[1]);\n+\tif (!ret)\n+\t\tdie(\"submodule %s doesn't exist\", argv[1]);\n+\n+\tprintf(\"%s\", ret);\n+\treturn 0;\n+}\n+\n static int push_check(int argc, const char **argv, const char *prefix)\n {\n \tstruct remote *remote;\n@@ -2770,6 +2805,7 @@ static struct cmd_struct commands[] = {\n \t{\"deinit\", module_deinit, 0},\n \t{\"summary\", module_summary, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n+\t{\"remote-remote\", resolve_remote_submodule_remote, 0},\n \t{\"push-check\", push_check, 0},\n \t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n \t{\"is-active\", is_active, 0},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex eb90f18229..4d0df1cf5a 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -577,7 +577,7 @@ cmd_update()\n \t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n \t\t\tfi\n-\t\t\tremote_name=$(sanitize_submodule_env; cd \"$sm_path\" && git submodule--helper print-default-remote)\n+\t\t\tremote_name=$(git submodule--helper remote-remote \"$sm_path\")\n \t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify \"${remote_name}/${branch}\") ||\n \t\t\tdie \"$(eval_gettext \"Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n"},{"id":"425202","messageId":"FCF5A210-4042-4C1B-87A5-83D916F02FCE@gmail.com","threadId":"55693","inReplyTo":"a727c1af-4993-684d-d299-00bf1a47cfc7@riscosopen.org","subject":"Re: Bug in git submodule update --remote","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-05-21T11:47:30Z","receivedAt":"2021-05-21T11:47:38Z","isPatch":false,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"On 19-May-2021, at 20:11, Ben Avison <bavison@riscosopen.org> wrote:\n> \n> On 19/05/2021 11:49, Atharva Raykar wrote:\n>> If I understood you correctly, you'd prefer that the updating of the\n>> submodule should be independent of the ref that is checked out in the\n>> submodule's directory.\n>> \n>> While I am not sure of the reason why the design of 'update\n>> --remote' uses the remote-tracking branch of the submodule, I can\n>> imagine adding a switch like 'submodule.<name>.remote' that defaults\n>> to 'origin'. Then the behaviour could be changed such that it always \n>> pulls from the remote specified in that option.\n>> \n>> This would help make the behaviour consistent in all the cases you \n>> mentioned, while also giving the option for a user to update the \n>> submodule from the remote of their choice (which may not be origin).\n> \n> I like that solution. Although, I should note that if the user has set\n> submodule.<name>.remote to something other than 'origin', they will need\n> to ensure that submodule.<name>.branch is also set, or they will still\n> hit the \"Unable to find current <remote>/HEAD revision in submodule\"\n> error that I initially stumbled on.\n> \n> How about an implementation like the following? I introduced a new \"git\n> submodule--helper\" command rather than modify \"print-default-remote\" for\n> a couple of reasons:\n> \n> 1) \"print-default-remote\" is also used for \"git submodule sync\" (I'm not\n> sure if we should change its behaviour too)\n> \n> 2) \"print-default-remote\" needs to be executed from within the\n> submodule, and takes no arguments, whereas I need to parse the\n> superproject's .git/config so need to be executed from the superproject\n> and take the submodule path as an argument\n> \n> The two functions I added are heavily based on \"git submodule--helper\n> remote-branch\". However:\n> \n> * Unlike with the branch name, I don't fall back to using the name for\n> the remote cached from when we read the .gitmodules file, if it isn't\n> found in .git/config. It doesn't make sense to me for the .gitmodules\n> file to include this information, as any new clones will only contain\n> \"origin\" remotes anyway.\n> \n> * I removed \"struct strbuf sb\" since I don't think it's used.\n> \n> Ben\n\nThanks for this. I am quite new around here, and I will be working\non porting the whole of 'submodule update' to C in the coming months.\n\nSince this would modify the behaviour of the update subcommand, I\nhave decided to CC my mentors (Christian and Shourya) who are more\nqualified than me to comment on this proposal.\n\nI personally feel that the current behaviour where the remote used\ndepends on how the submodule is checked out is odd, and I don't\nmind addressing it while doing the conversion of this functionality.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 9d505a6329..25ce3c8a1d 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2444,6 +2444,41 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,\n> \treturn 0;\n> }\n> \n> +static const char *remote_submodule_remote(const char *path)\n> +{\n> +\tconst struct submodule *sub;\n> +\tconst char *remote = NULL;\n> +\tchar *key;\n> +\n> +\tsub = submodule_from_path(the_repository, &null_oid, path);\n> +\tif (!sub)\n> +\t\treturn NULL;\n> +\n> +\tkey = xstrfmt(\"submodule.%s.remote\", sub->name);\n> +\trepo_config_get_string_tmp(the_repository, key, &remote);\n> +\tfree(key);\n> +\n> +\tif (!remote)\n> +\t\treturn \"origin\";\n> +\n> +\treturn remote;\n> +}\n> +\n> +static int resolve_remote_submodule_remote(int argc, const char **argv,\n> +\t\tconst char *prefix)\n> +{\n> +\tconst char *ret;\n> +\tif (argc != 2)\n> +\t\tdie(\"submodule--helper remote-remote takes exactly one arguments, got %d\", argc);\n> +\n> +\tret = remote_submodule_remote(argv[1]);\n> +\tif (!ret)\n> +\t\tdie(\"submodule %s doesn't exist\", argv[1]);\n> +\n> +\tprintf(\"%s\", ret);\n> +\treturn 0;\n> +}\n> +\n> static int push_check(int argc, const char **argv, const char *prefix)\n> {\n> \tstruct remote *remote;\n> @@ -2770,6 +2805,7 @@ static struct cmd_struct commands[] = {\n> \t{\"deinit\", module_deinit, 0},\n> \t{\"summary\", module_summary, SUPPORT_SUPER_PREFIX},\n> \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n> +\t{\"remote-remote\", resolve_remote_submodule_remote, 0},\n> \t{\"push-check\", push_check, 0},\n> \t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n> \t{\"is-active\", is_active, 0},\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index eb90f18229..4d0df1cf5a 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -577,7 +577,7 @@ cmd_update()\n> \t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n> \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n> \t\t\tfi\n> -\t\t\tremote_name=$(sanitize_submodule_env; cd \"$sm_path\" && git submodule--helper print-default-remote)\n> +\t\t\tremote_name=$(git submodule--helper remote-remote \"$sm_path\")\n> \t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n> \t\t\t\tgit rev-parse --verify \"${remote_name}/${branch}\") ||\n> \t\t\tdie \"$(eval_gettext \"Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n\n"},{"id":"425228","messageId":"CAP8UFD39ShCZRxUN7iHFUSQxO=EL18hhpbNN86SAnASHgxe6sQ@mail.gmail.com","threadId":"55693","inReplyTo":"FCF5A210-4042-4C1B-87A5-83D916F02FCE@gmail.com","subject":"Re: Bug in git submodule update --remote","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-05-21T18:36:23Z","receivedAt":"2021-05-21T18:36:38Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, May 21, 2021 at 1:47 PM Atharva Raykar <raykar.ath@gmail.com> wrote:\n>\n> On 19-May-2021, at 20:11, Ben Avison <bavison@riscosopen.org> wrote:\n\n> > I like that solution. Although, I should note that if the user has set\n> > submodule.<name>.remote to something other than 'origin', they will need\n> > to ensure that submodule.<name>.branch is also set, or they will still\n> > hit the \"Unable to find current <remote>/HEAD revision in submodule\"\n> > error that I initially stumbled on.\n> >\n> > How about an implementation like the following? I introduced a new \"git\n> > submodule--helper\" command rather than modify \"print-default-remote\" for\n> > a couple of reasons:\n\nFirst thanks for taking a look at improving git submodule and for\ndoing so by introducing most of the new code in submodule--helper!\n\n[...]\n\n> Since this would modify the behaviour of the update subcommand, I\n> have decided to CC my mentors (Christian and Shourya) who are more\n> qualified than me to comment on this proposal.\n\nAs commit f0a96e8d's author is Dscho (Johannes Schindelin) and as he\nwas helped by Philippe Blain, let me CC them too. They might know the\nreasons for this behavior better than us.\n"},{"id":"425338","messageId":"2c99d00c-d580-2ab9-ffda-256bbb628392@gmail.com","threadId":"55693","inReplyTo":"c4b27662-1228-a1ff-26fc-637897ffc8e7@riscosopen.org","subject":"Re: Bug in git submodule update --remote","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-05-22T18:02:49Z","receivedAt":"2021-05-22T18:02:54Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Ben,\n\nLe 2021-05-14 à 14:11, Ben Avison a écrit :\n> A recent-ish change in git 2.27.1, introduced in commit f0a96e8d, has also got me wondering about whether some related functionality is correct. I'm not sure what the best way to fix things is, so can I invite opinions?\n> \n> The scenario: I have a repository with a submodule. The submodule's upstream repository uses a fork workflow, so the submodule has two remotes, one for pulling in other people's changes, and one for uploading my own pull requests.\n> \n> $ mkdir myproject\n> $ cd myproject\n> $ git init\n> $ git submodule add https://github.com/acme/library\n> $ cd library\n> $ git remote add -f myfork https://github.com/user/library\n> $ cd ..\n> $ git add .\n> $ git commit -m \"Initial commit\"\n> $ git remote add origin https://github.com/user/myproject\n> $ git push -u origin master\n> \n> Furthermore, assume I forked https://github.com/acme/example some time ago, so that the master branches between it and my fork have diverged.\n> \n> Time passes. People hack away on both projects. I want to fix a bug or implement a new feature in the library, so I start by ensuring both are up-to-date:\n> \n> $ git pull\n> $ git submodule update --remote\n> $ cd library\n> $ git checkout -b new-feature\n> $ # hack away\n> $ git add .\n> $ git commit -m \"New feature\"\n> $ git push -u myfork new-feature\n\nStarting on a slight tangent, I know the 'git push -u $remote $branch' command\nis often mentioned in tutorials, but since you are using the forking workflow,\nI would suggest looking into the 'remote.pushDefault' / branch.$.pushRemote configs [1]. This allows you\nto track upstream/master on your feature branches (i.e. the branch into which your feature\nwill eventually be integrated), and to push directly to your fork with a simple 'git push' :)\n\n> $ cd ..\n> \n> Some more time passes, and I want to work on it again. Again, I start by ensuring I'm up-to-date. Before git 2.27.1, I could do:\n> \n> $ git submodule update --remote\n> \n> Now, I get:\n> \n> fatal: Needed a single revision\n> Unable to find current myfork/HEAD revision in submodule path 'library'\n\nThat's a shame. Dscho's f0a96e8d4c (submodule: fall back\nto remote's HEAD for missing remote.<name>.branch, 2020-06-24) did not touch\ngit-sumodule.sh, so when I took a look at it in [3] I did not think of going through\nthe code path...\n\n> \n> What's going on is that within \"git submodule update --remote\", the sha1 used is formed by looking up the submodule's ref \"<remote>/<branch>\". The change in git 2.27.1 is that if no remote tracking branch is stated in the superproject's .gitmodules file, <branch> defaults to \"HEAD\" rather than \"master\" as previously. That's fine if <remote> is \"origin\", but in practice, it depends on how the submodule is currently checked out:\n> \n> * if it's in detached HEAD state, <remote> is set to \"origin\"\n> * if its branch is not a tracking branch, \"origin\" is also used\n> * but if it's a tracking branch (as I used in my workflow above - not uncommon in pull request workflows because you might grant other people access to the branch during the review process) then it looks up the name of the remote which is being tracked\n> \n\nYes, that's indeed the behaviour of submodule--helper.c::get_default_remote,\nwhich is called by submodule--helper.c::print_default_remote, i.e.\n'git submodule--helper print-default-remote', which is called by\ngit-submodule.sh::cmd_update (line 580 as of v2.32.0-rc1).\n\n> First observation: a ref called \"myfork/HEAD\" presumably *could* have been created by the \"git remote add\" command, reflecting that remote's default branch. Should it?\n\nI think that's what happens with the '-m' switch, i.e. 'git remote add -m master myfork $url'.\nThe symref $remote/HEAD can also be created after adding the remote\nwith 'git remote set-head myfork --auto' [2], which will\nquery the remote's real default branch (i.e. the remote HEAD) and set the local symref accordingly.\nThe fact that this is not done automatically upon\n'git remote add' (or git fetch) is a historical mistake in my opinion but some others might disagree.\n\n> \n> In practice, that's not actually what I'd want, though. If I do \"git submodule update --remote\", I personally normally do so as a shortcut compared to cloning everything again. I don't expect the behaviour to change depending on whether I happen to have left one of the submodules checked out on a tracking branch or not: myfork/master is potentially quite out of date compared to origin/master.\n\nI completely agree.\n\n> \n> It also makes very little sense to me that issuing \"git submodule update --remote\" twice in quick succession would leave us in a different state, because the first call puts all the submodules into detached HEAD state, meaning that the second call always looks up the remote tracking branches from the submodule's \"origin\" remote.\n\nI also agree.\n\n> \n> One way this could be fixed is that if <branch> turns out to be \"HEAD\", we could force <remote> to be \"origin\". However, this doesn't address the equivalent problem that arises if the remote tracking branch *is* named in .gitmodules.\n> \n> I'd therefore like to propose that \"git submodule update --remote\" *always* uses the remote named \"origin\" to form the ref used to check out the submodule. However, I'm not sure whether everyone would agree that this constitutes a bugfix, or whether I'd need to introduce a new switch to enforce this behaviour.\n>\n\nI prefer introducing a new config variable, as you suggest in your patch\ndownthread. I'll comment there with more details, but I do not like the\napproach of hard-coding 'origin' with no escape hatch. Personnally I use\n'origin' for my fork and 'upstream' for the autoritative repository in all\nmy clones, and I can't do this in submodules because of the fact that 'origin'\nis hard-coded at few places in the submodule code, so let't not hard code it yet\none more time.\n\nCheers,\n\nPhilippe.\n\n\n[1] https://github.blog/2015-07-29-git-2-5-including-multiple-worktrees-and-triangular-workflows/#improved-support-for-triangular-workflows\n[2] http://git-scm.com/docs/git-remote#Documentation/git-remote.txt-emset-headem\n[3] https://lore.kernel.org/git/D2ED942B-9397-472A-B017-190016531547@gmail.com/\n"},{"id":"425341","messageId":"ae3baa2a-2f30-8e8a-f3cf-d8210e7225b0@gmail.com","threadId":"55693","inReplyTo":"a727c1af-4993-684d-d299-00bf1a47cfc7@riscosopen.org","subject":"Re: Bug in git submodule update --remote","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-05-22T19:15:51Z","receivedAt":"2021-05-22T19:15:57Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Ben,\n\nLe 2021-05-19 à 10:41, Ben Avison a écrit :\n> On 19/05/2021 11:49, Atharva Raykar wrote:\n>> If I understood you correctly, you'd prefer that the updating of the\n>> submodule should be independent of the ref that is checked out in the\n>> submodule's directory.\n>>\n>> While I am not sure of the reason why the design of 'update\n>> --remote' uses the remote-tracking branch of the submodule, I can\n>> imagine adding a switch like 'submodule.<name>.remote' that defaults\n>> to 'origin'. Then the behaviour could be changed such that it always\n>> pulls from the remote specified in that option.\n>>\n>> This would help make the behaviour consistent in all the cases you\n>> mentioned, while also giving the option for a user to update the\n>> submodule from the remote of their choice (which may not be origin).\n> \n> I like that solution. \n\nI also think a new config is a good solution.\n\n> Although, I should note that if the user has set\n> submodule.<name>.remote to something other than 'origin', they will need\n> to ensure that submodule.<name>.branch is also set, or they will still\n> hit the \"Unable to find current <remote>/HEAD revision in submodule\"\n> error that I initially stumbled on.\n> \n> How about an implementation like the following? I introduced a new \"git\n> submodule--helper\" command rather than modify \"print-default-remote\" for\n> a couple of reasons:\n> \n> 1) \"print-default-remote\" is also used for \"git submodule sync\" (I'm not\n> sure if we should change its behaviour too)\n> \n> 2) \"print-default-remote\" needs to be executed from within the\n> submodule, and takes no arguments, whereas I need to parse the\n> superproject's .git/config so need to be executed from the superproject\n> and take the submodule path as an argument\n> \n> The two functions I added are heavily based on \"git submodule--helper\n> remote-branch\". However:\n> \n> * Unlike with the branch name, I don't fall back to using the name for\n> the remote cached from when we read the .gitmodules file, if it isn't\n> found in .git/config. It doesn't make sense to me for the .gitmodules\n> file to include this information, as any new clones will only contain\n> \"origin\" remotes anyway.\n> \n> * I removed \"struct strbuf sb\" since I don't think it's used.\n> \n> Ben\n\nI think that we would want to careful examine every use of 'get_default_remote'\n(and the NEEDSWORK in submodule.c:get_next_submodule)\nand decide for each one if it makes sense to respect that new config before failing\nback to 'origin'. So I'm not sure about the approach here since it just fixes the behaviour\nfor 'git submodule update --remote' but not other scenarios where the user would like\nanother remote than 'origin' to be used for some operations in the submodule.\n\nThere was also another bug report recently [1] around the new 'clone.defaultRemoteName'\nconfig and 'git submodule update --remote', so it might be good to also keep that in\nmind when working on a solution.\n\nCheers,\n\nPhilippe.\nP.S. I've CC'ed Emily who has plans for a big overhaul of submodule functionality\nover the next months [2] so might be interested.\n\n[1] https://lore.kernel.org/git/2d58fe40-9e8c-4653-8170-5411fd3cf6f4@www.fastmail.com/t/#u\n[2] https://lore.kernel.org/git/0fc5c0f7-52f7-fb36-f654-ff5223a8809b@gmail.com/t/#u\n"}]}