{"thread":{"id":"47993","subject":"Re: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default","startedAt":"2018-03-06T20:23:00Z","lastAt":"2018-04-19T17:52:50Z","messageCount":29,"participants":["Jonathan Nieder","Eddy Petrișor","Thomas Gummerer","Eric Sunshine","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"341150","messageId":"20180306202149.GA160269@aiede.svl.corp.google.com","threadId":"47993","inReplyTo":"1520366804-28233-1-git-send-email-eddy.petrisor@gmail.com","subject":"Re: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-03-06T20:21:49Z","receivedAt":"2018-03-06T20:23:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(cc list snipped)\nHi,\n\nEddy Petrișor wrote:\n\n> Cc: [a lot of people]\n\nCan you say a little about how this cc list was created? E.g. should\n\"git send-email\" get a feature to warn about long cc lists?\n\n> Signed-off-by: Eddy Petrișor \n> ---\n>\n> There are projects such as llvm/clang which use several repositories, and they\n> might be forked for providing support for various features such as adding Redox\n> awareness to the toolchain. This typically means the superproject will use\n> another branch than master, occasionally even use an old commit from that\n> non-master branch.\n>\n> Combined with the fact that when incorporating such a hierachy of repositories\n> usually the user is interested in just the exact commit specified in the\n> submodule info, it follows that a desireable usecase is to be also able to\n> provide '--depth 1' to avoid waiting for ages for the clone operation to\n> finish.\n\nSome previous discussion is at\nhttps://public-inbox.org/git/CAGZ79ka6UXKyVLmdLg_M5-sB1x96g8FRzRZy=ENy5aJBQf9_QA@mail.gmail.com/.\n\nIn theory this should be straightforward: Git protocol allows fetching\nan arbitrary commit, so \"git submodule update\" and similar commands\ncould fetch the submodule commit by SHA-1 instead of by refname. Poof!\nProblem gone.\n\nIn practice, some complications:\n\n - some servers do not permit fetch-by-sha1. For example, github does\n not permit it. This is governed by the\n uploadpack.allowReachableSHA1InWant / uploadpack.allowAnySHA1InWant\n configuration items.\n\n That should be surmountable by making the behavior conditional, but\n it's a complication.\n\n - When the user passes --depth=, do they mean that to apply to\n the superproject, to the submodules, or both? Documentation should\n make the behavior clear.\n\n Fortunately I believe this complication has been takencare of using\n the --shallow-submodules option.\n\n> Git submodule seems to be very stubborn and cloning master, although the\n> wrapper script and the gitmodules-helper could work together to clone directly\n> the branch specified in the .gitmodules file, if specified.\n\nThis could make sense. For the same reason as --depth in the\nsuperproject gives ambiguous signals about what should happen in\nsubmodules, --single-branch in the superproject gives ambiguous\nsignals about what branch to fetch in submodules.\n\n> Another wrinkle is that when the commit is not the tip of the branch, the depth\n> parameter should somehow be stored in the .gitmodules info, but any change in\n> the submodule will break the supermodule submodule depth info sooner or later,\n> which is definitly frigile.\n\nHm, this seems to go in another direction. I don't think we should\nstore the depth parameter in the .gitmodules file, since different\nusers are likely to have different preferences about what to make\nshallow. If we make --depth easy enough to use at the superproject\nlevel then the user can specify what they want there.\n\n> I tried digging into this section of the code and debugging with bashdb to see\n> where --depth might fit, but I got stuck on the shell-to-helper interaction and\n> the details of the submodule implementation, so I want to lay out this first\n> patch as starting point for the discussion in the hope somebody else picks it\n> up or can provide some inputs. I have the feeling there are multiple code paths\n> that are being ran, depending on the moment (initial clone, submodule\n> recursive, post-clone update etc.) and I have a gut feeling there shouldn't be\n> any code duplication just because the operation is different.\n>\n> This first patch is only trying to use a non-master branch, I have some changes\n> for the --depth part, but I stopped working on it due to the \"default depth\"\n> issue above.\n>\n> Does any of this sound reasonable?\n> Is this patch idea usable or did I managed to touch the part of the code that\n> should not be touched?\n\nI agree with the goal. As mentioned above, I'm not confident about\nthe particular mechanism --- e.g. something using fetch-by-sha1 seems\nlikely to be more intuitive.\n\nToday, the 'branch' setting in .gitmodules is only for \"git submodule\nupdate --remote\". This patch would be a significant expansion in\nscope for it. Hopefully others on the list can talk more about how\nthat fits into various workflows and whether it would work out well.\n\nThanks and hope that helps,\nJonathan\n\n> git-submodule.sh | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 2491496..370f19e 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -589,8 +589,11 @@ cmd_update()\n> \t\t\tbranch=$(git submodule--helper remote-branch \"$sm_path\")\n> \t\t\tif test -z \"$nofetch\"\n> \t\t\tthen\n> +\t\t\t\t# non-default branch\n> +\t\t\t\trbranch=$(git config -f .gitmodules submodule.$sm_path.branch)\n> +\t\t\t\tbr_refspec=${rbanch:+\"refs/heads/$rbranch:refs/heads/$rbranch\"}\n> \t\t\t\t# Fetch remote before determining tracking $sha1\n> -\t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n> +\t\t\t\tfetch_in_submodule \"$sm_path\" $depth $br_refspec ||\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\" && get_default_remote)\n"},{"id":"341309","messageId":"CAK0XTWdY6rfKu_s8Am6dh9njcFHqSAz_54PhW6V09DuGwE-h0g@mail.gmail.com","threadId":"47993","inReplyTo":"20180306202149.GA160269@aiede.svl.corp.google.com","subject":"Re: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@gmail.com","sentAt":"2018-03-08T19:29:25Z","receivedAt":"2018-03-08T19:29:31Z","isPatch":true,"sender":{"key":"eddy.petrisor@gmail.com","avatar":"https://gravatar.com/avatar/e13eef6cecbe95604b4667c7bddf5aee2bb4af684c5fdceaf9e2aebfead3e9f5?d=mp&s=160"},"body":"2018-03-06 22:21 GMT+02:00 Jonathan Nieder :\n>\n> (cc list snipped)\n> Hi,\n>\n> Eddy Petrișor wrote:\n>\n> > Cc: [a lot of people]\n>\n> Can you say a little about how this cc list was created? E.g. should\n> \"git send-email\" get a feature to warn about long cc lists?\n\n\nI did it as advised by the documentation, git blame:\n\nhttps://github.com/git/git/blob/master/Documentation/SubmittingPatches#L264\n\nI was suprised that there is no specialized script that does this, as\nit is for the kernel or u-boot, so I ran first\n\n git log --pretty=format:'%an <%ae>' git-submodule.sh | sort -u >\nmail-list-submodule\n\nthen configure git to use that custom output and ignore the patch\nsince it was trying to convert every line of it into a email address:\n\n git config sendemail.ccCmd 'cat mail-list-submodule # '\n\nThen \"git send-email 0001...\" did the rest.\n\n\n>\n> > Signed-off-by: Eddy Petrișor \n> > ---\n> >\n> > There are projects such as llvm/clang which use several repositories, and they\n> > might be forked for providing support for various features such as adding Redox\n> > awareness to the toolchain. This typically means the superproject will use\n> > another branch than master, occasionally even use an old commit from that\n> > non-master branch.\n> >\n> > Combined with the fact that when incorporating such a hierachy of repositories\n> > usually the user is interested in just the exact commit specified in the\n> > submodule info, it follows that a desireable usecase is to be also able to\n> > provide '--depth 1' to avoid waiting for ages for the clone operation to\n> > finish.\n>\n> Some previous discussion is at\n> https://public-inbox.org/git/CAGZ79ka6UXKyVLmdLg_M5-sB1x96g8FRzRZy=ENy5aJBQf9_QA@mail.gmail.com/.\n>\n> In theory this should be straightforward: Git protocol allows fetching\n> an arbitrary commit, so \"git submodule update\" and similar commands\n> could fetch the submodule commit by SHA-1 instead of by refname. Poof!\n> Problem gone.\n>\n> In practice, some complications:\n>\n> - some servers do not permit fetch-by-sha1. For example, github does\n> not permit it. This is governed by the\n> uploadpack.allowReachableSHA1InWant / uploadpack.allowAnySHA1InWant\n> configuration items.\n\n\nThat is the problem I have faced since I tried to clone this repo\nwhich has at lest 2 levels of submodules:\nhttps://github.com/redox-os/redox\n\nThe problematic modules are:\nrust @ https://github.com/redox-os/rust/tree/81c2bf4e51647295d3d92952dbb0464b460df0c3\n- first level\n\nand\n\nrust/src/llvm @\nhttps://github.com/rust-lang/llvm/tree/ba2edd794c7def715007931fcd1b4ce62aa711c8\n\n\n>\n> That should be surmountable by making the behavior conditional, but\n> it's a complication.\n\n\nWhich forced me to try to fetch a branch on which that commit exists,\nbut also consider providing --depth for the submodule checkout,\neffectively minimizing the amount of brought in history.\n\n>\n>\n> - When the user passes --depth=, do they mean that to apply to\n> the superproject, to the submodules, or both? Documentation should\n> make the behavior clear.\n\n\nThe supermodule clone already exists and that works OK; I was after\nproviding something like 'git submodule update --depth 20 --recursive'\nor evn providing that 'depth' info via the .gitmodules parameters\nsince 'shallow' is already used somehow, yet that is a bool value, not\nan integer, like depth expects:\n\n\neddy@feodora:~/usr/src/redox/rust-eddy$ git config -f .gitmodules\n--list | egrep '(depth|shallow)'\nsubmodule.src/llvm.shallow=true\nsubmodule.src/rt/hoedown.shallow=true\nsubmodule.src/jemalloc.shallow=true\nsubmodule.src/liblibc.shallow=true\nsubmodule.src/doc/nomicon.shallow=true\nsubmodule.src/tools/cargo.shallow=true\nsubmodule.src/doc/reference.shallow=true\nsubmodule.src/doc/book.shallow=true\nsubmodule.src/tools/rls.shallow=true\nsubmodule.src/libcompiler_builtins.shallow=true\nsubmodule.src/tools/clippy.shallow=true\nsubmodule.src/tools/rustfmt.shallow=true\nsubmodule.src/tools/miri.shallow=true\nsubmodule.src/dlmalloc.shallow=true\nsubmodule.src/binaryen.shallow=true\nsubmodule.src/doc/rust-by-example.shallow=true\nsubmodule.src/llvm-emscripten.shallow=true\nsubmodule.src/tools/rust-installer.shallow=true\n\n\n> > Git submodule seems to be very stubborn and cloning master, although the\n> > wrapper script and the gitmodules-helper could work together to clone directly\n> > the branch specified in the .gitmodules file, if specified.\n>\n> This could make sense. For the same reason as --depth in the\n> superproject gives ambiguous signals about what should happen in\n> submodules, --single-branch in the superproject gives ambiguous\n> signals about what branch to fetch in submodules.\n\n\nI never thought of providing the branch in the command line, since\nthat's not versionable info, but to provide git-submodule a hint in\nthe .gitsubmodule config about on which branch the hash in question\nshould be found, like this:\n\neddy@feodora:~/usr/src/redox/rust-eddy$ git config -f .gitmodules\n--list | egrep branch\nsubmodule.src/llvm.branch=rust-llvm-release-4-0-1\nsubmodule.src/rt/hoedown.branch=rust-2015-09-21-do-not-delete\n\n>\n> > Another wrinkle is that when the commit is not the tip of the branch, the depth\n> > parameter should somehow be stored in the .gitmodules info, but any change in\n> > the submodule will break the supermodule submodule depth info sooner or later,\n> > which is definitly frigile.\n>\n> Hm, this seems to go in another direction. I don't think we should\n> store the depth parameter in the .gitmodules file, since different\n> users are likely to have different preferences about what to make\n> shallow. If we make --depth easy enough to use at the superproject\n> level then the user can specify what they want there.\n\n\nYes, but\na) if the commit is not on the tip of that branch,\nb) the server does not support sha1 fetching and\nc) it is not expected to typically work in the submodules from the\nsuperproject checkout (e.g submodule is under another team's control)\n\nthe typical usecase could be that the code is question is read-only\nand it should be OK to typically fetch a shallow clone of the\nsubmodule (by default). The only compromise at that point is to either\nchoose a afe margin for the defalt depth in case the module branch is\nbeing modified.\n\nI solved a similar problem at work where having 2 shallow branches\nwhich had to be merged; since when trying to merge two branches\nwithout apprent common history git will do a merge commit, when a\nsimple ff woudl wor, I wanted to find the merge-base.\nSInce the brances were shallow and they had to have a merge-base, the\nsolution was to was to simply increase the --depth parameter in\npredifined steps (of 10 commits) and fetch both branches, check if the\n'git merge-base' command manged to find a commit; if not the depthc\nparameter woudl be increased and the whole process would be started\nuntil that merge-base was found.\n\nI wonder if the same couldn't be done for the submodules that have\nsubmodule..shallow setting, making the depth parameter hardcoding\nunnecessary, and guranteeing the submodule could be cloned at any\ngiven time, as long as the branch specified by submodule..branch\nis not removed from the submodule repo.\n\n>\n> > I tried digging into this section of the code and debugging with bashdb to see\n> > where --depth might fit, but I got stuck on the shell-to-helper interaction and\n> > the details of the submodule implementation, so I want to lay out this first\n> > patch as starting point for the discussion in the hope somebody else picks it\n> > up or can provide some inputs. I have the feeling there are multiple code paths\n> > that are being ran, depending on the moment (initial clone, submodule\n> > recursive, post-clone update etc.) and I have a gut feeling there shouldn't be\n> > any code duplication just because the operation is different.\n> >\n> > This first patch is only trying to use a non-master branch, I have some changes\n> > for the --depth part, but I stopped working on it due to the \"default depth\"\n> > issue above.\n> >\n> > Does any of this sound reasonable?\n> > Is this patch idea usable or did I managed to touch the part of the code that\n> > should not be touched?\n>\n> I agree with the goal. As mentioned above, I'm not confident about\n> the particular mechanism --- e.g. something using fetch-by-sha1 seems\n> likely to be more intuitive.\n>\n> Today, the 'branch' setting in .gitmodules is only for \"git submodule\n> update --remote\". This patch would be a significant expansion in\n> scope for it. Hopefully others on the list can talk more about how\n> that fits into various workflows and whether it would work out well.\n\n\nThanks for the input, it was helpful anyway.\n\nNow that I think of it, I think the \"fetch more and more from the\nbranch until we find the sha1\" could work; in the worst case you would\nend up going to the beginning of the repo in case the submodule\nhistory was rewritten or if the branch specified in the supermodule\nsubmodule..branch was removed.\n\nI am aware this is not ellegant at all (what is the default \"step\"?),\nyet the submodule part of the code already struggles in that area\nbecause it clearly looks like an afterthought, anyway.\n\n>\n> Thanks and hope that helps,\n> Jonathan\n\n\n-- \nEddy Petrișor\n"},{"id":"341311","messageId":"CAK0XTWepPHe__qO6foFrh8-5mVJgWFzUsSw_4GZ89MFOmTrXbA@mail.gmail.com","threadId":"47993","inReplyTo":"CAK0XTWdY6rfKu_s8Am6dh9njcFHqSAz_54PhW6V09DuGwE-h0g@mail.gmail.com","subject":"Re: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@gmail.com","sentAt":"2018-03-08T19:41:24Z","receivedAt":"2018-03-08T19:47:40Z","isPatch":true,"sender":{"key":"eddy.petrisor@gmail.com","avatar":"https://gravatar.com/avatar/e13eef6cecbe95604b4667c7bddf5aee2bb4af684c5fdceaf9e2aebfead3e9f5?d=mp&s=160"},"body":"2018-03-08 21:29 GMT+02:00 Eddy Petrișor :\n> 2018-03-06 22:21 GMT+02:00 Jonathan Nieder :\n>>\n>> (cc list snipped)\n>> Hi,\n>>\n>> Eddy Petrișor wrote:\n>>\n>> > Cc: [a lot of people]\n>>\n>> Can you say a little about how this cc list was created? E.g. should\n>> \"git send-email\" get a feature to warn about long cc lists?\n>\n>\n> I did it as advised by the documentation, git blame:\n>\n> https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L264\n>\n> I was suprised that there is no specialized script that does this, as\n> it is for the kernel or u-boot, so I ran first\n>\n> git log --pretty=format:'%an <%ae>' git-submodule.sh | sort -u >\n> mail-list-submodule\n>\n> then configure git to use that custom output and ignore the patch\n> since it was trying to convert every line of it into a email address:\n>\n> git config sendemail.ccCmd 'cat mail-list-submodule # '\n>\n> Then \"git send-email 0001...\" did the rest.\n>\n>\n>>\n>> > Signed-off-by: Eddy Petrișor \n>> > ---\n>> >\n>> > There are projects such as llvm/clang which use several repositories, and they\n>> > might be forked for providing support for various features such as adding Redox\n>> > awareness to the toolchain. This typically means the superproject will use\n>> > another branch than master, occasionally even use an old commit from that\n>> > non-master branch.\n>> >\n>> > Combined with the fact that when incorporating such a hierachy of repositories\n>> > usually the user is interested in just the exact commit specified in the\n>> > submodule info, it follows that a desireable usecase is to be also able to\n>> > provide '--depth 1' to avoid waiting for ages for the clone operation to\n>> > finish.\n>>\n>> Some previous discussion is at\n>> https://public-inbox.org/git/CAGZ79ka6UXKyVLmdLg_M5-sB1x96g8FRzRZy=ENy5aJBQf9_QA@mail.gmail.com/.\n>>\n>> In theory this should be straightforward: Git protocol allows fetching\n>> an arbitrary commit, so \"git submodule update\" and similar commands\n>> could fetch the submodule commit by SHA-1 instead of by refname. Poof!\n>> Problem gone.\n>>\n>> In practice, some complications:\n>>\n>> - some servers do not permit fetch-by-sha1. For example, github does\n>> not permit it. This is governed by the\n>> uploadpack.allowReachableSHA1InWant / uploadpack.allowAnySHA1InWant\n>> configuration items.\n>\n>\n> That is the problem I have faced since I tried to clone this repo\n> which has at lest 2 levels of submodules:\n> https://github.com/redox-os/redox\n>\n> The problematic modules are:\n> rust @ https://github.com/redox-os/rust/tree/81c2bf4e51647295d3d92952dbb0464b460df0c3\n> - first level\n>\n> and\n>\n> rust/src/llvm @\n> https://github.com/rust-lang/llvm/tree/ba2edd794c7def715007931fcd1b4ce62aa711c8\n>\n>\n>>\n>> That should be surmountable by making the behavior conditional, but\n>> it's a complication.\n>\n>\n> Which forced me to try to fetch a branch on which that commit exists,\n> but also consider providing --depth for the submodule checkout,\n> effectively minimizing the amount of brought in history.\n>\n>>\n>>\n>> - When the user passes --depth=, do they mean that to apply to\n>> the superproject, to the submodules, or both? Documentation should\n>> make the behavior clear.\n>\n>\n> The supermodule clone already exists and that works OK; I was after\n> providing something like 'git submodule update --depth 20 --recursive'\n> or evn providing that 'depth' info via the .gitmodules parameters\n> since 'shallow' is already used somehow, yet that is a bool value, not\n> an integer, like depth expects:\n>\n>\n> eddy@feodora:~/usr/src/redox/rust-eddy$ git config -f .gitmodules\n> --list | egrep '(depth|shallow)'\n> submodule.src/llvm.shallow=true\n> submodule.src/rt/hoedown.shallow=true\n> submodule.src/jemalloc.shallow=true\n> submodule.src/liblibc.shallow=true\n> submodule.src/doc/nomicon.shallow=true\n> submodule.src/tools/cargo.shallow=true\n> submodule.src/doc/reference.shallow=true\n> submodule.src/doc/book.shallow=true\n> submodule.src/tools/rls.shallow=true\n> submodule.src/libcompiler_builtins.shallow=true\n> submodule.src/tools/clippy.shallow=true\n> submodule.src/tools/rustfmt.shallow=true\n> submodule.src/tools/miri.shallow=true\n> submodule.src/dlmalloc.shallow=true\n> submodule.src/binaryen.shallow=true\n> submodule.src/doc/rust-by-example.shallow=true\n> submodule.src/llvm-emscripten.shallow=true\n> submodule.src/tools/rust-installer.shallow=true\n>\n>\n>> > Git submodule seems to be very stubborn and cloning master, although the\n>> > wrapper script and the gitmodules-helper could work together to clone directly\n>> > the branch specified in the .gitmodules file, if specified.\n>>\n>> This could make sense. For the same reason as --depth in the\n>> superproject gives ambiguous signals about what should happen in\n>> submodules, --single-branch in the superproject gives ambiguous\n>> signals about what branch to fetch in submodules.\n>\n>\n> I never thought of providing the branch in the command line, since\n> that's not versionable info, but to provide git-submodule a hint in\n> the .gitsubmodule config about on which branch the hash in question\n> should be found, like this:\n>\n> eddy@feodora:~/usr/src/redox/rust-eddy$ git config -f .gitmodules\n> --list | egrep branch\n> submodule.src/llvm.branch=rust-llvm-release-4-0-1\n> submodule.src/rt/hoedown.branch=rust-2015-09-21-do-not-delete\n>\n>>\n>> > Another wrinkle is that when the commit is not the tip of the branch, the depth\n>> > parameter should somehow be stored in the .gitmodules info, but any change in\n>> > the submodule will break the supermodule submodule depth info sooner or later,\n>> > which is definitly frigile.\n>>\n>> Hm, this seems to go in another direction. I don't think we should\n>> store the depth parameter in the .gitmodules file, since different\n>> users are likely to have different preferences about what to make\n>> shallow. If we make --depth easy enough to use at the superproject\n>> level then the user can specify what they want there.\n>\n>\n> Yes, but\n> a) if the commit is not on the tip of that branch,\n> b) the server does not support sha1 fetching and\n> c) it is not expected to typically work in the submodules from the\n> superproject checkout (e.g submodule is under another team's control)\n>\n> the typical usecase could be that the code is question is read-only\n> and it should be OK to typically fetch a shallow clone of the\n> submodule (by default). The only compromise at that point is to either\n> choose a afe margin for the defalt depth in case the module branch is\n> being modified.\n>\n> I solved a similar problem at work where having 2 shallow branches\n> which had to be merged; since when trying to merge two branches\n> without apprent common history git will do a merge commit, when a\n> simple ff woudl wor, I wanted to find the merge-base.\n> SInce the brances were shallow and they had to have a merge-base, the\n> solution was to was to simply increase the --depth parameter in\n> predifined steps (of 10 commits) and fetch both branches, check if the\n> 'git merge-base' command manged to find a commit; if not the depthc\n> parameter woudl be increased and the whole process would be started\n> until that merge-base was found.\n\nAnother optimization could be for submodule..shallow to imply\n--single-branch, too.\n\n> I wonder if the same couldn't be done for the submodules that have\n> submodule..shallow setting, making the depth parameter hardcoding\n> unnecessary, and guranteeing the submodule could be cloned at any\n> given time, as long as the branch specified by submodule..branch\n> is not removed from the submodule repo.\n>\n>>\n>> > I tried digging into this section of the code and debugging with bashdb to see\n>> > where --depth might fit, but I got stuck on the shell-to-helper interaction and\n>> > the details of the submodule implementation, so I want to lay out this first\n>> > patch as starting point for the discussion in the hope somebody else picks it\n>> > up or can provide some inputs. I have the feeling there are multiple code paths\n>> > that are being ran, depending on the moment (initial clone, submodule\n>> > recursive, post-clone update etc.) and I have a gut feeling there shouldn't be\n>> > any code duplication just because the operation is different.\n>> >\n>> > This first patch is only trying to use a non-master branch, I have some changes\n>> > for the --depth part, but I stopped working on it due to the \"default depth\"\n>> > issue above.\n>> >\n>> > Does any of this sound reasonable?\n>> > Is this patch idea usable or did I managed to touch the part of the code that\n>> > should not be touched?\n>>\n>> I agree with the goal. As mentioned above, I'm not confident about\n>> the particular mechanism --- e.g. something using fetch-by-sha1 seems\n>> likely to be more intuitive.\n>>\n>> Today, the 'branch' setting in .gitmodules is only for \"git submodule\n>> update --remote\". This patch would be a significant expansion in\n>> scope for it. Hopefully others on the list can talk more about how\n>> that fits into various workflows and whether it would work out well.\n>\n>\n> Thanks for the input, it was helpful anyway.\n>\n> Now that I think of it, I think the \"fetch more and more from the\n> branch until we find the sha1\" could work; in the worst case you would\n> end up going to the beginning of the repo in case the submodule\n> history was rewritten or if the branch specified in the supermodule\n> submodule..branch was removed.\n\nActually, the branch removal in the submodule would lead to a failure\nto fetch from the get go, so the worst case is if history is rewritten\nbecause the sha1 would be missing fromt he branch.\nAntoher non-fatal perfomance issue would be if the desired commit is\nvery close to the initial commit of the repo.\n\n> I am aware this is not ellegant at all (what is the default \"step\"?),\n> yet the submodule part of the code already struggles in that area\n> because it clearly looks like an afterthought, anyway.\n\n\n-- \nEddy Petrișor\n"},{"id":"341948","messageId":"20180316213323.GC2224@hank","threadId":"47993","inReplyTo":"CAK0XTWdY6rfKu_s8Am6dh9njcFHqSAz_54PhW6V09DuGwE-h0g@mail.gmail.com","subject":"Re: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-03-16T21:33:23Z","receivedAt":"2018-03-16T21:30:15Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/08, Eddy Petrișor wrote:\n> 2018-03-06 22:21 GMT+02:00 Jonathan Nieder :\n> >\n> > (cc list snipped)\n> > Hi,\n> >\n> > Eddy Petrișor wrote:\n> >\n> > > Cc: [a lot of people]\n> >\n> > Can you say a little about how this cc list was created? E.g. should\n> > \"git send-email\" get a feature to warn about long cc lists?\n> \n> \n> I did it as advised by the documentation, git blame:\n> \n> https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L264\n> \n> I was suprised that there is no specialized script that does this, as\n> it is for the kernel or u-boot, so I ran first\n> \n> git log --pretty=format:'%an <%ae>' git-submodule.sh | sort -u >\n> mail-list-submodule\n> \n> then configure git to use that custom output and ignore the patch\n> since it was trying to convert every line of it into a email address:\n> \n> git config sendemail.ccCmd 'cat mail-list-submodule # '\n> \n> Then \"git send-email 0001...\" did the rest.\n\nThere's a 'git contacts' command which we've been carrying in\n\"contrib/\" for a while. Maybe we could advertise that in\n\"Documentation/SubmittingPatches\" instead of 'git blame' and 'git\nshortlog'?\n\nMaybe something like the patch below?\n\n--- >8 ---\nSubject: [PATCH] SubmittingPatches: mention the git contacts command\n\nInstead of just mentioning 'git blame' and 'git shortlog', which make\nit harder than necessary for new contributors to pick out the\nappropriate list of people to cc on their patch series, mention the\n'git contacts' utility, which should make it much easier to get a\nreasonable list of contacts for a change.\n\nSigned-off-by: Thomas Gummerer \n---\n Documentation/SubmittingPatches | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex a1d0feca36..945f8edb46 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -260,8 +260,8 @@ that starts with `-----BEGIN PGP SIGNED MESSAGE-----`. That is\n not a text/plain, it's something else.\n \n Send your patch with \"To:\" set to the mailing list, with \"cc:\" listing\n-people who are involved in the area you are touching (the output from\n-`git blame $path` and `git shortlog --no-merges $path` would help to\n+people who are involved in the area you are touching (the `git\n+contacts` command in `contrib/contacts/` can help to\n identify them), to solicit comments and reviews.\n \n :1: footnote:[The current maintainer: gitster@pobox.com]\n-- \n2.16.2.804.g6dcf76e11\n\n"},{"id":"341953","messageId":"CAPig+cR5ncp9eMZ8krgy19A6g1Uqjx9dXTkus4r1QimD5Uwk0A@mail.gmail.com","threadId":"47993","inReplyTo":"20180316213323.GC2224@hank","subject":"Re: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-16T21:44:16Z","receivedAt":"2018-03-16T21:44:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 16, 2018 at 5:33 PM, Thomas Gummerer wrote:\n> Subject: [PATCH] SubmittingPatches: mention the git contacts command\n>\n> Instead of just mentioning 'git blame' and 'git shortlog', which make\n> it harder than necessary for new contributors to pick out the\n> appropriate list of people to cc on their patch series, mention the\n> 'git contacts' utility, which should make it much easier to get a\n> reasonable list of contacts for a change.\n>\n> Signed-off-by: Thomas Gummerer \n> ---\n> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\n> @@ -260,8 +260,8 @@ that starts with `-----BEGIN PGP SIGNED MESSAGE-----`. That is\n> Send your patch with \"To:\" set to the mailing list, with \"cc:\" listing\n> -people who are involved in the area you are touching (the output from\n> -`git blame $path` and `git shortlog --no-merges $path` would help to\n> +people who are involved in the area you are touching (the `git\n> +contacts` command in `contrib/contacts/` can help to\n> identify them), to solicit comments and reviews.\n\nIt may be a disservice to remove mention of git-blame and git-shortlog\nsince git-contacts may not be suitable for everyone. Instead, perhaps\nadvertise git-contacts as a potentially simpler alternative to the\nalready-mentioned git-blamd & git-shortlog?\n\nAlso, would it make sense to mention Felipe's git-related[1] which is\nthe original (and now more configurable) script from which\ngit-contacts functionality was derived?\n\n[1]: https://github.com/felipec/git-related\n"},{"id":"342042","messageId":"20180317191138.GE2224@hank","threadId":"47993","inReplyTo":"CAK0XTWcNySGgwgFgCPDnZ+m=2hfEgswHbJKYeu+LQfuQ9_=shQ@mail.gmail.com","subject":"Re: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-03-17T19:11:38Z","receivedAt":"2018-03-17T19:08:29Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/17, Eddy Petrișor wrote:\n> vin., 16 mar. 2018, 23:44 Eric Sunshine a scris:\n> \n> > On Fri, Mar 16, 2018 at 5:33 PM, Thomas Gummerer \n> > wrote:\n> > > a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\n> > > @@ -260,8 +260,8 @@ that starts with `-----BEGIN PGP SIGNED\n> > MESSAGE-----`. That is\n> > > Send your patch with \"To:\" set to the mailing list, with \"cc:\" listing\n> > > -people who are involved in the area you are touching (the output from\n> > > -`git blame $path` and `git shortlog --no-merges $path` would help to\n> > > +people who are involved in the area you are touching (the `git\n> > > +contacts` command in `contrib/contacts/` can help to\n> > > identify them), to solicit comments and reviews.\n> >\n> > It may be a disservice to remove mention of git-blame and git-shortlog\n> > since git-contacts may not be suitable for everyone. Instead, perhaps\n> > advertise git-contacts as a potentially simpler alternative to the\n> > already-mentioned git-blamd & git-shortlog?\n\nNot sure how much of a disservice it would be. I think of\nSubmittingPatches as mostly a document for new git contributors, for\nwho I think we should make it as easy as possible to start\ncontributing. Interpreting the output of 'git blame' and 'git\nshortlog' feels like an extra hurdle for new contributors, especially\nif someone is not familiar with the mailing list workflow. I do\nremember wondering exactly how I should interpret this when I sent my\nfirst patches.\n\n> As a \"victim\" of the current documentation, I would advise to have the\n> order reversed. For a new contributor, judging if git blame and shortlog\n> are \"more suitable\" than git contracts or git related is definitely over\n> the reasonable knowledge expectation. Why is that more suitable than this?\n> How is suitability determined?\n> \n> A new person needs a straight forward way to focus on submitting the patch\n> in the right form. With experience adding more people in cc will come\n> naturally and those contacts might be aware of the contributor, too.\n\nThis is also my experience, as I am getting involved with longer with\nthe project I get more of an intuition who is involved where, and\n'blame' and 'shortlog' start helping me confirm that and come up with\na reasonable list of contacts (although I'm still not always sure\nwhether I got the correct people or not).\n\nBut I don't know if people that are getting involved for longer read\nthis document much anymore. So I feel like having the commands\nmentioned here comes at the expense of new contributors, so I'm not\nsure it's worth it.\n\n> > Also, would it make sense to mention Felipe's git-related[1] which is\n> > the original (and now more configurable) script from which\n> > git-contacts functionality was derived?\n\nThe reason I chose 'git contacts' over git-related is mainly that it\ncomes available with git. Mentioning both again makes things harder\non new contributors who already have enough to think about when\nsubmitting the patch.\n\nI guess in the end it comes down to who we think the target of the\ndocument is. To me it was always people new to the project, which is\nwhy I think the single command there makes sense.\n\n> > [1]: https://github.com/felipec/git-related\n> >\n"},{"id":"342070","messageId":"CAPig+cRPkjDhedmvzuABCLD6+umsHP1w6ptw5BrezDx-gnWDfQ@mail.gmail.com","threadId":"47993","inReplyTo":"20180317191138.GE2224@hank","subject":"Re: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-18T01:43:03Z","receivedAt":"2018-03-18T01:53:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 17, 2018 at 3:11 PM, Thomas Gummerer wrote:\n> On 03/17, Eddy Petrișor wrote:\n>> vin., 16 mar. 2018, 23:44 Eric Sunshine a scris:\n>> > It may be a disservice to remove mention of git-blame and git-shortlog\n>> > since git-contacts may not be suitable for everyone. Instead, perhaps\n>> > advertise git-contacts as a potentially simpler alternative to the\n>> > already-mentioned git-blamd & git-shortlog?\n>\n> Not sure how much of a disservice it would be. I think of\n> SubmittingPatches as mostly a document for new git contributors, for\n> who I think we should make it as easy as possible to start\n> contributing. Interpreting the output of 'git blame' and 'git\n> shortlog' feels like an extra hurdle for new contributors, especially\n> if someone is not familiar with the mailing list workflow. I do\n> remember wondering exactly how I should interpret this when I sent my\n> first patches.\n\nOkay. Mentioning those commands (in addition to git-contacts) is an\nopportunity to educate newcomers to Git the tool (not just to git.git\nthe project) about additional ways to engage in project spelunking. By\n\"disservice\", I meant that that educational opportunity is lost.\nEddy's suggestion of reversing the order, thus mentioning git-contacts\nfirst is a good alternative.\n\nHowever, perhaps this idea of educating newcomers to Git is misplaced\nin this context; such spelunking advice may be better suited to a\ngeneral Git tutorial than to SubmittingPatches which is indeed\nspecific to git.git. Given that reasoning, then my \"disservice\" view\nmay be wrong.\n\n>> > Also, would it make sense to mention Felipe's git-related[1] which is\n>> > the original (and now more configurable) script from which\n>> > git-contacts functionality was derived?\n>\n> The reason I chose 'git contacts' over git-related is mainly that it\n> comes available with git. Mentioning both again makes things harder\n> on new contributors who already have enough to think about when\n> submitting the patch.\n>\n> I guess in the end it comes down to who we think the target of the\n> document is. To me it was always people new to the project, which is\n> why I think the single command there makes sense.\n\nFair enough.\n"},{"id":"343087","messageId":"CAGZ79kb4Ea7t5j9XA0key1f99w5xRDwyRhMder1FMgdiZot3Tg@mail.gmail.com","threadId":"47993","inReplyTo":"1520366804-28233-1-git-send-email-eddy.petrisor@gmail.com","subject":"Re: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-03-26T23:06:54Z","receivedAt":"2018-03-26T23:07:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"[snipped the cc list as well]\n\nOn Tue, Mar 6, 2018 at 12:06 PM Eddy Petrișor \nwrote:\n\n> Signed-off-by: Eddy Petrișor \n> ---\n\nDid this go anywhere?\n(I just came back from a longer vacation, sorry for the delay on my site)\n\n> There are projects such as llvm/clang which use several repositories, and\nthey\n> might be forked for providing support for various features such as adding\nRedox\n> awareness to the toolchain. This typically means the superproject will use\n> another branch than master, occasionally even use an old commit from that\n> non-master branch.\n\n> Combined with the fact that when incorporating such a hierachy of\nrepositories\n> usually the user is interested in just the exact commit specified in the\n> submodule info, it follows that a desireable usecase is to be also able to\n> provide '--depth 1' to avoid waiting for ages for the clone operation to\n> finish.\n\nVery sensible.\n\n> Git submodule seems to be very stubborn and cloning master, although the\n> wrapper script and the gitmodules-helper could work together to clone\ndirectly\n> the branch specified in the .gitmodules file, if specified.\n\nAlso very sensible.\n\nSo far so good, could you move these paragraphs before the triple dashed\nline\nand sign off so we record it as the commit message?\n\n> Another wrinkle is that when the commit is not the tip of the branch, the\ndepth\n> parameter should somehow be stored in the .gitmodules info, but any\nchange in\n> the submodule will break the supermodule submodule depth info sooner or\nlater,\n> which is definitly frigile.\n\n... which is why I would not include that.\n\ngit-fetch knows about --shallow-since or even better\nshallow-exclude which could be set to the (depth+1)-th commit\n(the boundary commit) recorded in the shallow information.\n\n> I tried digging into this section of the code and debugging with bashdb\nto see\n> where --depth might fit, but I got stuck on the shell-to-helper\ninteraction and\n> the details of the submodule implementation, so I want to lay out this\nfirst\n> patch as starting point for the discussion in the hope somebody else\npicks it\n> up or can provide some inputs. I have the feeling there are multiple code\npaths\n> that are being ran, depending on the moment (initial clone, submodule\n> recursive, post-clone update etc.) and I have a gut feeling there\nshouldn't be\n> any code duplication just because the operation is different.\n\n> This first patch is only trying to use a non-master branch, I have some\nchanges\n> for the --depth part, but I stopped working on it due to the \"default\ndepth\"\n> issue above.\n\n> Does any of this sound reasonable?\n> Is this patch idea usable or did I managed to touch the part of the code\nthat\n> should not be touched?\n\nThis sounds reasonable. Thanks for writing the patch!\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 2491496..370f19e 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -589,8 +589,11 @@ cmd_update()\n> branch=$(git submodule--helper remote-branch\n\"$sm_path\")\n> if test -z \"$nofetch\"\n> then\n> + # non-default branch\n> + rbranch=$(git config -f .gitmodules\nsubmodule.$sm_path.branch)\n> +\nbr_refspec=${rbanch:+\"refs/heads/$rbranch:refs/heads/$rbranch\"}\n\nWouldn't we want to fetch into a remote tracking branch instead?\nInstead of computing all this by yourself, these two lines could be\n\n br_refspec=$(git submodule--helper remote-branch $sm_path)\n\nI would think.\n\n> # Fetch remote before determining\ntracking $sha1\n> - fetch_in_submodule \"$sm_path\" $depth ||\n> + fetch_in_submodule \"$sm_path\" $depth\n$br_refspec ||\n> die \"$(eval_gettext \"Unable to fetch in\nsubmodule path '\\$sm_path'\")\"\n> fi\n> remote_name=$(sanitize_submodule_env; cd\n\"$sm_path\" && get_default_remote)\n\nIt would be awesome if you could write a little test for this feature, too.\nLook for the tests in regarding --remote in t7406 (in the t/ directory) as\na starting point, please.\n\nThanks!\nStefan\n"},{"id":"343370","messageId":"CAK0XTWdxsUwWB7932yqo8+4PQqtEVJ8W_R9b3RK1oLVA6ZWkOg@mail.gmail.com","threadId":"47993","inReplyTo":"CAK0XTWd7QGtVDwm8FDXejZfbgVH6-1NprGY0xxAnC33QH8aCCQ@mail.gmail.com","subject":"Fwd: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@gmail.com","sentAt":"2018-03-29T20:54:06Z","receivedAt":"2018-03-29T20:54:14Z","isPatch":true,"sender":{"key":"eddy.petrisor@gmail.com","avatar":"https://gravatar.com/avatar/e13eef6cecbe95604b4667c7bddf5aee2bb4af684c5fdceaf9e2aebfead3e9f5?d=mp&s=160"},"body":"mar., 27 mar. 2018, 02:07 Stefan Beller a scris:\n>\n> [snipped the cc list as well]\n>\n> On Tue, Mar 6, 2018 at 12:06 PM Eddy Petrișor \n> wrote:\n>\n> > Signed-off-by: Eddy Petrișor \n> > ---\n>\n> Did this go anywhere?\n> (I just came back from a longer vacation, sorry for the delay on my site)\n\n\nNot really. I am still unsure how is best to proceed. Details below.\n\n> > There are projects such as llvm/clang which use several repositories, and\n> they\n> > might be forked for providing support for various features such as adding\n> Redox\n> > awareness to the toolchain. This typically means the superproject will use\n> > another branch than master, occasionally even use an old commit from that\n> > non-master branch.\n>\n> > Combined with the fact that when incorporating such a hierachy of\n> repositories\n> > usually the user is interested in just the exact commit specified in the\n> > submodule info, it follows that a desireable usecase is to be also able to\n> > provide '--depth 1' to avoid waiting for ages for the clone operation to\n> > finish.\n>\n> Very sensible.\n\n\nThe only change is that I realized that hard coding the depth is not\nnecessary because the client can fetch more and more from the branch\nuntil the commit hash is found or the entire history was fetched and\nit wasn't found.\n\nThis is more robust but has a variable performance penalty and is\nprobably slower than single branch fetching from the start.\n\n> > Git submodule seems to be very stubborn and cloning master, although the\n> > wrapper script and the gitmodules-helper could work together to clone\n> directly\n> > the branch specified in the .gitmodules file, if specified.\n>\n> Also very sensible.\n>\n> So far so good, could you move these paragraphs before the triple dashed\n> line\n> and sign off so we record it as the commit message?\n\n\nSure, as long as the implementation and design makes sense.\n\n> > Another wrinkle is that when the commit is not the tip of the branch, the\n> depth\n> > parameter should somehow be stored in the .gitmodules info, but any\n> change in\n> > the submodule will break the supermodule submodule depth info sooner or\n> later,\n> > which is definitly frigile.\n>\n> ... which is why I would not include that.\n>\n> git-fetch knows about --shallow-since or even better\n> shallow-exclude which could be set to the (depth+1)-th commit\n> (the boundary commit) recorded in the shallow information.\n\n\nI am unsure what that means. Without yet looking in the docs, would\nthis --shallow-since be better than the try-until-found algorithm\nexplained above?\n\n> > I tried digging into this section of the code and debugging with bashdb\n> to see\n> > where --depth might fit, but I got stuck on the shell-to-helper\n> interaction and\n> > the details of the submodule implementation, so I want to lay out this\n> first\n> > patch as starting point for the discussion in the hope somebody else\n> picks it\n> > up or can provide some inputs. I have the feeling there are multiple code\n> paths\n> > that are being ran, depending on the moment (initial clone, submodule\n> > recursive, post-clone update etc.) and I have a gut feeling there\n> shouldn't be\n> > any code duplication just because the operation is different.\n>\n> > This first patch is only trying to use a non-master branch, I have some\n> changes\n> > for the --depth part, but I stopped working on it due to the \"default\n> depth\"\n> > issue above.\n>\n> > Does any of this sound reasonable?\n> > Is this patch idea usable or did I managed to touch the part of the code\n> that\n> > should not be touched?\n>\n> This sounds reasonable. Thanks for writing the patch!\n\n\nOK. Now I need to make it good, which is the hard part :)\n\n> > diff --git a/git-submodule.sh b/git-submodule.sh\n> > index 2491496..370f19e 100755\n> > --- a/git-submodule.sh\n> > +++ b/git-submodule.sh\n> > @@ -589,8 +589,11 @@ cmd_update()\n> > branch=$(git submodule--helper remote-branch\n> \"$sm_path\")\n> > if test -z \"$nofetch\"\n> > then\n> > + # non-default branch\n> > + rbranch=$(git config -f .gitmodules\n> submodule.$sm_path.branch)\n> > +\n> br_refspec=${rbanch:+\"refs/heads/$rbranch:refs/heads/$rbranch\"}\n>\n> Wouldn't we want to fetch into a remote tracking branch instead?\n> Instead of computing all this by yourself, these two lines could be\n>\n> br_refspec=$(git submodule--helper remote-branch $sm_path)\n>\n> I would think.\n\n\nI wasn't aware of this, will implement I the next version and see what happens.\n>\n>\n> > # Fetch remote before determining\n> tracking $sha1\n> > - fetch_in_submodule \"$sm_path\" $depth ||\n> > + fetch_in_submodule \"$sm_path\" $depth\n> $br_refspec ||\n> > die \"$(eval_gettext \"Unable to fetch in\n> submodule path '\\$sm_path'\")\"\n> > fi\n> > remote_name=$(sanitize_submodule_env; cd\n> \"$sm_path\" && get_default_remote)\n>\n> It would be awesome if you could write a little test for this feature, too.\n> Look for the tests in regarding --remote in t7406 (in the t/ directory) as\n> a starting point, please.\n\n\nComing up with a test case is probably a better way to explain what I\nwant the behaviour to be. Thanks for pointing out the test case area\nto look into.\n\n> Thanks!\n> Stefan\n\n-- \nEddy Petrișor\n"},{"id":"343383","messageId":"CAK0XTWdpKnMbXs5sq2qsERxQO20sQW6YwL=2kfBHmSwHMmunBg@mail.gmail.com","threadId":"47993","inReplyTo":"20180329225502.20112-1-eddy.petrisor@gmail.com","subject":"Re: [RFC PATCH v2] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@gmail.com","sentAt":"2018-03-29T22:59:50Z","receivedAt":"2018-03-29T23:00:04Z","isPatch":true,"sender":{"key":"eddy.petrisor@gmail.com","avatar":"https://gravatar.com/avatar/e13eef6cecbe95604b4667c7bddf5aee2bb4af684c5fdceaf9e2aebfead3e9f5?d=mp&s=160"},"body":"(+list)\n\n2018-03-30 1:55 GMT+03:00 :\n> From: Eddy Petrișor \n>\n> There are projects such as llvm/clang which use several repositories, and they\n> might be forked for providing support for various features such as adding Redox\n> awareness to the toolchain. This typically means the superproject will use\n> another branch than master, occasionally even use an old commit from that\n> non-master branch.\n>\n> Combined with the fact that when incorporating such a hierachy of repositories\n> usually the user is interested in just the exact commit specified in the\n> submodule info, it follows that a desireable usecase is to be also able to\n> provide '--depth 1' or at least have a shallow clone to avoid waiting for ages\n> for the clone operation to finish.\n>\n> In theory, this should be straightforward since the git protocol allows\n> fetching an arbitary commit, but, in practice, some servers do not permit\n> fetch-by-sha1.\n>\n> Git submodule seems to be very stubborn and cloning master, although the\n> wrapper script and the gitmodules-helper could work together to clone directly\n> the branch specified in the .gitmodules file, if specified.\n>\n> Signed-off-by: Eddy Petrișor \n> ---\n> git-submodule.sh | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 24914963c..65e3af08b 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -589,8 +589,10 @@ cmd_update()\n> branch=$(git submodule--helper remote-branch \"$sm_path\")\n> if test -z \"$nofetch\"\n> then\n> + # non-default branch refspec\n> + br_refspec=$(git submodule-helper remote-branch $sm_path)\n> # Fetch remote before determining tracking $sha1\n> - fetch_in_submodule \"$sm_path\" $depth ||\n> + fetch_in_submodule \"$sm_path\" $depth $br_refspec ||\n> die \"$(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n> fi\n> remote_name=$(sanitize_submodule_env; cd \"$sm_path\" && get_default_remote)\n> --\n> 2.16.2\n>\n\nI am planning to write a test case in the next few days, between the\nfew drops of free time I have.\nI am still trying to figure out my way through the test code.\n\n-- \nEddy Petrișor\n"},{"id":"343771","messageId":"20180403222053.23132-1-eddy.petrisor@codeaurora.org","threadId":"47993","inReplyTo":"CAK0XTWdpKnMbXs5sq2qsERxQO20sQW6YwL=2kfBHmSwHMmunBg@mail.gmail.com","subject":"[RFC PATCH v3 1/2] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@codeaurora.org","sentAt":"2018-04-03T22:20:52Z","receivedAt":"2018-04-03T22:21:24Z","isPatch":true,"sender":{"key":"eddy.petrisor@codeaurora.org","avatar":null},"body":"From: Eddy Petrișor \n\nThere are projects such as llvm/clang which use several repositories, and they\nmight be forked for providing support for various features such as adding Redox\nawareness to the toolchain. This typically means the superproject will use\nanother branch than master, occasionally even use an old commit from that\nnon-master branch.\n\nCombined with the fact that when incorporating such a hierachy of repositories\nusually the user is interested in just the exact commit specified in the\nsubmodule info, it follows that a desireable usecase is to be also able to\nprovide '--depth 1' or at least have a shallow clone to avoid waiting for ages\nfor the clone operation to finish.\n\nIn theory, this should be straightforward since the git protocol allows\nfetching an arbitary commit, but, in practice, some servers do not permit\nfetch-by-sha1.\n\nGit submodule seems to be very stubborn and cloning master, although the\nwrapper script and the gitmodules-helper could work together to clone directly\nthe branch specified in the .gitmodules file, if specified.\n\nSigned-off-by: Eddy Petrișor \n---\n git-submodule.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 24914963c..65e3af08b 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -589,8 +589,10 @@ cmd_update()\n \t\t\tbranch=$(git submodule--helper remote-branch \"$sm_path\")\n \t\t\tif test -z \"$nofetch\"\n \t\t\tthen\n+\t\t\t\t# non-default branch refspec\n+\t\t\t\tbr_refspec=$(git submodule-helper remote-branch $sm_path)\n \t\t\t\t# Fetch remote before determining tracking $sha1\n-\t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n+\t\t\t\tfetch_in_submodule \"$sm_path\" $depth $br_refspec ||\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\" && get_default_remote)\n-- \n2.16.2\n\n"},{"id":"343772","messageId":"20180403222053.23132-2-eddy.petrisor@codeaurora.org","threadId":"47993","inReplyTo":"20180403222053.23132-1-eddy.petrisor@codeaurora.org","subject":"[RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@codeaurora.org","sentAt":"2018-04-03T22:20:53Z","receivedAt":"2018-04-03T22:21:26Z","isPatch":true,"sender":{"key":"eddy.petrisor@codeaurora.org","avatar":null},"body":"From: Eddy Petrișor \n\nIf a submodule uses a non-default branch and the branch info is versioned, on\nsubmodule update --recursive --init the correct branch should be checked out.\n\nSigned-off-by: Eddy Petrișor \n---\n t/t7406-submodule-update.sh | 54 +++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 54 insertions(+)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 6f083c4d6..7b65f1dd1 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -259,6 +259,60 @@ test_expect_success 'submodule update --remote should fetch upstream changes wit\n \t)\n '\n \n+test_expect_success 'submodule update --remote --recursive --init should fetch module branch from .gitmodules' '\n+\tgit clone . super5 &&\n+\tgit clone super5 submodl2b2 &&\n+\tgit clone super5 submodl1b1 &&\n+\tcd submodl2b2 &&\n+\techo linel2b2 > l2b2 &&\n+\tgit checkout -b b2 &&\n+\tgit add l2b2 &&\n+\ttest_tick &&\n+\tgit commit -m \"commit on b2 branch in l2\" &&\n+\tgit rev-parse --verify HEAD >../expectl2 &&\n+\tgit checkout master &&\n+\tcd ../submodl1b1 &&\n+\tgit checkout -b b1 &&\n+\techo linel1b1 > l1b1 &&\n+\tgit add l1b1 &&\n+\ttest_tick &&\n+\tgit commit -m \"commit on b1 branch in l1\" &&\n+\tgit submodule add ../submodl2b2 submodl2b2 &&\n+\tgit config -f .gitmodules submodule.\"submodl2b2\".branch b2 &&\n+\tgit add .gitmodules &&\n+\ttest_tick &&\n+\tgit commit -m \"add l2 module with branch b2 in l1 module in branch b1\" &&\n+\tgit submodule init submodl2b2 &&\n+\tgit rev-parse --verify HEAD >../expectl1 &&\n+\tgit checkout master &&\n+\tcd ../super5 &&\n+\techo super_with_2_chained_modules > super5 &&\n+\tgit add super5 &&\n+\ttest_tick &&\n+\tgit commit -m \"commit on default branch in super5\" &&\n+\tgit submodule add ../submodl1b1 submodl1b1 &&\n+\tgit config -f .gitmodules submodule.\"submodl1b1\".branch b1 &&\n+\tgit add .gitmodules &&\n+\ttest_tick &&\n+\tgit commit -m \"add l1 module with branch b1 in super5\" &&\n+\tgit submodule init submodl1b1 &&\n+\tgit clone super5 super &&\n+\t(\n+\t\tcd super &&\n+\t\tgit submodule update --recursive --init\n+\t) &&\n+\t(\n+\t\tcd submodl1b1 &&\n+\t\tgit rev-parse --verify HEAD >../../actuall1 &&\n+\t\ttest_cmp ../../expectl1 ../../actuall1\n+\t) &&\n+\t(\n+\t\tcd submodl2b2 &&\n+\t\tgit rev-parse --verify HEAD >../../../actuall2 &&\n+\t\ttest_cmp ../../../expectl2 ../../../actuall2\n+\t)\n+'\n+\n test_expect_success 'local config should override .gitmodules branch' '\n \t(cd submodule &&\n \t git checkout test-branch &&\n-- \n2.16.2\n\n"},{"id":"343774","messageId":"CAK0XTWf=ubDK-4=FknLSFObZrShg=7UDgR5c6iNSjrwiDngayA@mail.gmail.com","threadId":"47993","inReplyTo":"20180403222053.23132-2-eddy.petrisor@codeaurora.org","subject":"Re: [RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@gmail.com","sentAt":"2018-04-03T22:26:19Z","receivedAt":"2018-04-03T22:26:31Z","isPatch":true,"sender":{"key":"eddy.petrisor@gmail.com","avatar":"https://gravatar.com/avatar/e13eef6cecbe95604b4667c7bddf5aee2bb4af684c5fdceaf9e2aebfead3e9f5?d=mp&s=160"},"body":"Notes:\n I am aware this test is not probably the best one and maybe I\nshould have first one test that does a one level non-default, before\ntrying a test with 2 levels of submodules, but I wanted to express the\ngoal of the patch.\n\n Currently the test fails, so I am obviously missing something.\nHelp would be appreciated.\n\n\n2018-04-04 1:20 GMT+03:00 Eddy Petrișor :\n> From: Eddy Petrișor \n>\n> If a submodule uses a non-default branch and the branch info is versioned, on\n> submodule update --recursive --init the correct branch should be checked out.\n>\n> Signed-off-by: Eddy Petrișor \n> ---\n> t/t7406-submodule-update.sh | 54 +++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 54 insertions(+)\n>\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index 6f083c4d6..7b65f1dd1 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -259,6 +259,60 @@ test_expect_success 'submodule update --remote should fetch upstream changes wit\n> )\n> '\n>\n> +test_expect_success 'submodule update --remote --recursive --init should fetch module branch from .gitmodules' '\n> + git clone . super5 &&\n> + git clone super5 submodl2b2 &&\n> + git clone super5 submodl1b1 &&\n> + cd submodl2b2 &&\n> + echo linel2b2 > l2b2 &&\n> + git checkout -b b2 &&\n> + git add l2b2 &&\n> + test_tick &&\n> + git commit -m \"commit on b2 branch in l2\" &&\n> + git rev-parse --verify HEAD >../expectl2 &&\n> + git checkout master &&\n> + cd ../submodl1b1 &&\n> + git checkout -b b1 &&\n> + echo linel1b1 > l1b1 &&\n> + git add l1b1 &&\n> + test_tick &&\n> + git commit -m \"commit on b1 branch in l1\" &&\n> + git submodule add ../submodl2b2 submodl2b2 &&\n> + git config -f .gitmodules submodule.\"submodl2b2\".branch b2 &&\n> + git add .gitmodules &&\n> + test_tick &&\n> + git commit -m \"add l2 module with branch b2 in l1 module in branch b1\" &&\n> + git submodule init submodl2b2 &&\n> + git rev-parse --verify HEAD >../expectl1 &&\n> + git checkout master &&\n> + cd ../super5 &&\n> + echo super_with_2_chained_modules > super5 &&\n> + git add super5 &&\n> + test_tick &&\n> + git commit -m \"commit on default branch in super5\" &&\n> + git submodule add ../submodl1b1 submodl1b1 &&\n> + git config -f .gitmodules submodule.\"submodl1b1\".branch b1 &&\n> + git add .gitmodules &&\n> + test_tick &&\n> + git commit -m \"add l1 module with branch b1 in super5\" &&\n> + git submodule init submodl1b1 &&\n> + git clone super5 super &&\n> + (\n> + cd super &&\n> + git submodule update --recursive --init\n> + ) &&\n> + (\n> + cd submodl1b1 &&\n> + git rev-parse --verify HEAD >../../actuall1 &&\n> + test_cmp ../../expectl1 ../../actuall1\n> + ) &&\n> + (\n> + cd submodl2b2 &&\n> + git rev-parse --verify HEAD >../../../actuall2 &&\n> + test_cmp ../../../expectl2 ../../../actuall2\n> + )\n> +'\n> +\n> test_expect_success 'local config should override .gitmodules branch' '\n> (cd submodule &&\n> git checkout test-branch &&\n> --\n> 2.16.2\n>\n\n\n\n-- \nEddy Petrișor\n"},{"id":"343801","messageId":"CAGZ79kYu-9vzee=R7uE4fhhrRj19ZT0Z5+7fPLwOpXhEr7aqUw@mail.gmail.com","threadId":"47993","inReplyTo":"CAK0XTWf=ubDK-4=FknLSFObZrShg=7UDgR5c6iNSjrwiDngayA@mail.gmail.com","subject":"Re: [RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-04T18:36:31Z","receivedAt":"2018-04-04T18:36:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Apr 3, 2018 at 3:26 PM, Eddy Petrișor wrote:\n> Notes:\n> I am aware this test is not probably the best one and maybe I\n> should have first one test that does a one level non-default, before\n> trying a test with 2 levels of submodules, but I wanted to express the\n> goal of the patch.\n\nThis patch only contains the test, I presume this goes on top of\nhttps://public-inbox.org/git/20180403222053.23132-1-eddy.petrisor@codeaurora.org/\nwhich you plan to later submit as one patch including both the change as well as\nthe test.\n\n> Currently the test fails, so I am obviously missing something.\n> Help would be appreciated.\n>\n>\n> 2018-04-04 1:20 GMT+03:00 Eddy Petrișor :\n>> From: Eddy Petrișor \n>>\n>> If a submodule uses a non-default branch and the branch info is versioned, on\n>> submodule update --recursive --init the correct branch should be checked out.\n>>\n>> Signed-off-by: Eddy Petrișor \n>> ---\n>> t/t7406-submodule-update.sh | 54 +++++++++++++++++++++++++++++++++++++++++++++\n>> 1 file changed, 54 insertions(+)\n>>\n>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n>> index 6f083c4d6..7b65f1dd1 100755\n>> --- a/t/t7406-submodule-update.sh\n>> +++ b/t/t7406-submodule-update.sh\n>> @@ -259,6 +259,60 @@ test_expect_success 'submodule update --remote should fetch upstream changes wit\n>> )\n>> '\n>>\n>> +test_expect_success 'submodule update --remote --recursive --init should fetch module branch from .gitmodules' '\n>> + git clone . super5 &&\n\nI found adding \"test_pause &&\"\nto be a great helper as it will drop you in a shell where\nyou can inspect the repository.\n\n\n>> + git clone super5 submodl2b2 &&\n>> + git clone super5 submodl1b1 &&\n\nWe may want to cleanup after the test is done:\n\n test_when_finished \"rm submodl2b2\" &&\n test_when_finished \"rm submodl1b2\" &&\n\n>> + cd submodl2b2 &&\n\nI'd think the test suite prefers subshells to operate in other dirs\ninstead of direct cd's, just like at the end of the test.\nFor short parts, we make heavy use of the -C option,\nSo for example the code below\n (\n cd super &&\n git submodule update --recursive --init\n ) &&\n\ncan be written as\n\n git -C super submodule update --recursive --init\n\nwhich is shorter.\n\n>> + echo linel2b2 > l2b2 &&\n\nstyle: The Git test suite prefers to have the redirect adjacent to the\nfile name:\n echo hello >world\n\n>> + git checkout -b b2 &&\n>> + git add l2b2 &&\n>> + test_tick &&\n>> + git commit -m \"commit on b2 branch in l2\" &&\n>> + git rev-parse --verify HEAD >../expectl2 &&\n\nSo until now we made a commit in a submodule on branch b2\nand wrote it out to an expect file.\n\n\n>> + git checkout master &&\n>> + cd ../submodl1b1 &&\n>> + git checkout -b b1 &&\n>> + echo linel1b1 > l1b1 &&\n>> + git add l1b1 &&\n>> + test_tick &&\n>> + git commit -m \"commit on b1 branch in l1\" &&\n\nvery similar to above, just in another repo\ninstead of making a commit yourself, you may want to use\n test_commit \nthen you don't need to call echo/add/commit yourself.\n\n>> + git submodule add ../submodl2b2 submodl2b2 &&\n>> + git config -f .gitmodules submodule.\"submodl2b2\".branch b2 &&\n>> + git add .gitmodules &&\n>> + test_tick &&\n>> + git commit -m \"add l2 module with branch b2 in l1 module in branch b1\" &&\n\nSo one submodule is made to be a submodule of the other\nwith a specific branch (b2)\n\n>> + git submodule init submodl2b2 &&\n\ngit submodule add ought to have initialized that submodule\nalready, I am not sure we need the explicit init here.\n\n>> + git rev-parse --verify HEAD >../expectl1 &&\n>> + git checkout master &&\n\nWe go back to master, which doesn't have the nested submodule?\n\n>> + cd ../super5 &&\n>> + echo super_with_2_chained_modules > super5 &&\n>> + git add super5 &&\n>> + test_tick &&\n>> + git commit -m \"commit on default branch in super5\" &&\n>> + git submodule add ../submodl1b1 submodl1b1 &&\n>> + git config -f .gitmodules submodule.\"submodl1b1\".branch b1 &&\n>> + git add .gitmodules &&\n>> + test_tick &&\n>> + git commit -m \"add l1 module with branch b1 in super5\" &&\n\nnow we do this again without the nested submodule, just one repo\nas a submodule?\n\n>> + git submodule init submodl1b1 &&\n>> + git clone super5 super &&\n\ndoes super exist here already? (I did not check, but IIRC\nsuper and super{1-4} are there as we count upwards to\nfind a name that is ok.\n\n>> + (\n>> + cd super &&\n>> + git submodule update --recursive --init\n>> + ) &&\n>> + (\n>> + cd submodl1b1 &&\n>> + git rev-parse --verify HEAD >../../actuall1 &&\n>> + test_cmp ../../expectl1 ../../actuall1\n>> + ) &&\n>> + (\n>> + cd submodl2b2 &&\n>> + git rev-parse --verify HEAD >../../../actuall2 &&\n>> + test_cmp ../../../expectl2 ../../../actuall2\n>> + )\n\nStefan\n"},{"id":"343815","messageId":"CAK0XTWdSQmfqo2-WuiPtOcn_4cf60jNZ_j9Uh_HpzxUwSpA7bw@mail.gmail.com","threadId":"47993","inReplyTo":"CAGZ79kYu-9vzee=R7uE4fhhrRj19ZT0Z5+7fPLwOpXhEr7aqUw@mail.gmail.com","subject":"Re: [RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@gmail.com","sentAt":"2018-04-04T20:37:55Z","receivedAt":"2018-04-04T20:38:02Z","isPatch":true,"sender":{"key":"eddy.petrisor@gmail.com","avatar":"https://gravatar.com/avatar/e13eef6cecbe95604b4667c7bddf5aee2bb4af684c5fdceaf9e2aebfead3e9f5?d=mp&s=160"},"body":"2018-04-04 21:36 GMT+03:00 Stefan Beller :\n> On Tue, Apr 3, 2018 at 3:26 PM, Eddy Petrișor wrote:\n>> Notes:\n>> I am aware this test is not probably the best one and maybe I\n>> should have first one test that does a one level non-default, before\n>> trying a test with 2 levels of submodules, but I wanted to express the\n>> goal of the patch.\n>\n> This patch only contains the test, I presume this goes on top of\n> https://public-inbox.org/git/20180403222053.23132-1-eddy.petrisor@codeaurora.org/\n> which you plan to later submit as one patch including both the change as well as\n> the test.\n\nYes, not sure why the emails were not linked together, I specified the\nin-reply-to mesage id when I \"git format-patch\"-ed the patches.\n\nI wasn't actually planning to squash them in a single patch, will do,\nbut I guess for now it helps to focus the discussion around the test\nsince the implementation is still lacking, see below 2 lines comment.\n\n>> Currently the test fails, so I am obviously missing something.\n>> Help would be appreciated.\n>>\n>>\n>> 2018-04-04 1:20 GMT+03:00 Eddy Petrișor :\n>>> From: Eddy Petrișor \n[..]\n>>> --- a/t/t7406-submodule-update.sh\n>>> +++ b/t/t7406-submodule-update.sh\n>>> @@ -259,6 +259,60 @@ test_expect_success 'submodule update --remote should fetch upstream changes wit\n>>> )\n>>> '\n>>>\n>>> +test_expect_success 'submodule update --remote --recursive --init should fetch module branch from .gitmodules' '\n>>> + git clone . super5 &&\n>\n> I found adding \"test_pause &&\"\n> to be a great helper as it will drop you in a shell where\n> you can inspect the repository.\n\nThanks for the pointer, I only looked at the post-failure state after\nadding --debug -i --verbose, but having \"test_pause\" to stop and\ninspect the interim stage should help a lot with debugging.\n\n>\n>>> + git clone super5 submodl2b2 &&\n>>> + git clone super5 submodl1b1 &&\n>\n> We may want to cleanup after the test is done:\n>\n> test_when_finished \"rm submodl2b2\" &&\n> test_when_finished \"rm submodl1b2\" &&\n\nSure, will do.\n\n>>> + cd submodl2b2 &&\n>\n> I'd think the test suite prefers subshells to operate in other dirs\n> instead of direct cd's, just like at the end of the test.\n\nAfter looking at other tests I was under the impression that the setup\nphase (e.g. creating the \"server\" side repos) of the test was done in\nthe main context, while the test case (i.e. the client side, or what\nthe user would do) is in subshells.\n\n> For short parts, we make heavy use of the -C option,\n> So for example the code below\n> (\n> cd super &&\n> git submodule update --recursive --init\n> ) &&\n>\n> can be written as\n>\n> git -C super submodule update --recursive --init\n>\n> which is shorter.\n\nNice.\n\n>>> + echo linel2b2 > l2b2 &&\n>\n> style: The Git test suite prefers to have the redirect adjacent to the\n> file name:\n> echo hello >world\n\nI wasn't aware of that, it seems there are some inconsistencies,\nincluding in the modified test:\n\neddy@feodora:~/usr/src/git/t$ grep '> ' -c t* 2>/dev/null | grep -v\n':0$' | grep 7406\nt7406-submodule-update.sh:24\neddy@feodora:~/usr/src/git/t$ grep '> ' -c t* 2>/dev/null | grep -v\n':0$' | wc -l\n325\n\nI suspect that a minor patch correcting these inconsistencies would be\nfaily fast reviewed adn accepted, of course, disconnected from this\nmodification.\n\n>>> + git checkout -b b2 &&\n>>> + git add l2b2 &&\n>>> + test_tick &&\n>>> + git commit -m \"commit on b2 branch in l2\" &&\n>>> + git rev-parse --verify HEAD >../expectl2 &&\n>\n> So until now we made a commit in a submodule on branch b2\n> and wrote it out to an expect file.\n\nYes, I was trying to replicate the redox-os case which has this structure:\n\nredox-os (master branch)\n + rust (@some commit on a non-default)\n + src/llvm (@some commit on a non-default branch)\n\nThis section is making sure the b2 branch has other content than the\ndefault one and setting the expectation for level2 submodule branch,\nlater.\n\n>>> + git checkout master &&\n>>> + cd ../submodl1b1 &&\n>>> + git checkout -b b1 &&\n>>> + echo linel1b1 > l1b1 &&\n>>> + git add l1b1 &&\n>>> + test_tick &&\n>>> + git commit -m \"commit on b1 branch in l1\" &&\n>\n> very similar to above, just in another repo\n> instead of making a commit yourself, you may want to use\n> test_commit \n> then you don't need to call echo/add/commit yourself.\n\nthanks for the pointer, will change that since my purpose for the\ncommit was to make sure default branch (master) and b1 branch are\ndifferent\n\n>>> + git submodule add ../submodl2b2 submodl2b2 &&\n>>> + git config -f .gitmodules submodule.\"submodl2b2\".branch b2 &&\n>>> + git add .gitmodules &&\n>>> + test_tick &&\n>>> + git commit -m \"add l2 module with branch b2 in l1 module in branch b1\" &&\n>\n> So one submodule is made to be a submodule of the other\n> with a specific branch (b2)\n\ncorrect\n\n>>> + git submodule init submodl2b2 &&\n>\n> git submodule add ought to have initialized that submodule\n> already, I am not sure we need the explicit init here.\n\nI think that from my tests I saw that without it the submodule wasn't\nthere, but I might be mistaken, will check.\n\n>>> + git rev-parse --verify HEAD >../expectl1 &&\n>>> + git checkout master &&\n>\n> We go back to master, which doesn't have the nested submodule?\n\nexactly, since the desired behaviour is to have in the nested\nsubmodule the b2 branch.\nI guess I could have added the level2 submodule@master on l1's master\nbranch, but I didn't see much point in that since it should be enough\nnow.\n\nIt might make more sense if master and b2, and, respectively master\nand l1 have diverging histories, but for now that is probably overkill\nand it will make sense in the context of shallow cloning of\nsubmodules, which I haven't yet tackled except presenting the idea.\n\n>>> + cd ../super5 &&\n>>> + echo super_with_2_chained_modules > super5 &&\n>>> + git add super5 &&\n>>> + test_tick &&\n>>> + git commit -m \"commit on default branch in super5\" &&\n>>> + git submodule add ../submodl1b1 submodl1b1 &&\n>>> + git config -f .gitmodules submodule.\"submodl1b1\".branch b1 &&\n>>> + git add .gitmodules &&\n>>> + test_tick &&\n>>> + git commit -m \"add l1 module with branch b1 in super5\" &&\n>\n> now we do this again without the nested submodule, just one repo\n> as a submodule?\n\nMy intention was to have super5 -> submodl1b1@b1 -> submodl2b2@b2 on\nthe \"server\" side.\nBut are you saying I just implemented super5 -> sbmodl1b1@master due\nto the previous master checkout in submodl1b1?\n\n>>> + git submodule init submodl1b1 &&\n>>> + git clone super5 super &&\n>\n> does super exist here already? (I did not check, but IIRC\n> super and super{1-4} are there as we count upwards to\n> find a name that is ok.\n\nI created it in the first step of the test with the intention to have\nsuper5 as the \"server\" and \"super\" as the client clone.\n\n>\n>>> + (\n>>> + cd super &&\n>>> + git submodule update --recursive --init\n>>> + ) &&\n>>> + (\n>>> + cd submodl1b1 &&\n>>> + git rev-parse --verify HEAD >../../actuall1 &&\n>>> + test_cmp ../../expectl1 ../../actuall1\n>>> + ) &&\n>>> + (\n>>> + cd submodl2b2 &&\n>>> + git rev-parse --verify HEAD >../../../actuall2 &&\n>>> + test_cmp ../../../expectl2 ../../../actuall2\n>>> + )\n\nAs a general idea for a test, does it look sane?\n\nDo you think I should I start with a just one level of submodule with\na non-default branch (super -> l1@b1), or it this OK?\nIn my view, having 2 levels makes sure the recursive part is also\naddressed and verified.\n\n-- \nEddy Petrișor\n"},{"id":"343821","messageId":"CAGZ79kbT+AnVu-AJ9=OSt=Z2ErM5ocfoLfgx=qmHFfbnkqJqwg@mail.gmail.com","threadId":"47993","inReplyTo":"CAK0XTWdSQmfqo2-WuiPtOcn_4cf60jNZ_j9Uh_HpzxUwSpA7bw@mail.gmail.com","subject":"Re: [RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-04T21:41:55Z","receivedAt":"2018-04-04T21:42:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Apr 4, 2018 at 1:37 PM, Eddy Petrișor wrote:\n\n>> This patch only contains the test, I presume this goes on top of\n>> https://public-inbox.org/git/20180403222053.23132-1-eddy.petrisor@codeaurora.org/\n>> which you plan to later submit as one patch including both the change as well as\n>> the test.\n>\n> Yes, not sure why the emails were not linked together, I specified the\n> in-reply-to mesage id when I \"git format-patch\"-ed the patches.\n\nThey are linked in public-inbox, but I noticed too late. (in gmail\nthey were not)\n\n> Thanks for the pointer, I only looked at the post-failure state after\n> adding --debug -i --verbose, but having \"test_pause\" to stop and\n> inspect the interim stage should help a lot with debugging.\n\nI also like the -x flag that stops at the failing test, but as this is the\nlast test of this script it is of limited use here.\n\n> After looking at other tests I was under the impression that the setup\n> phase (e.g. creating the \"server\" side repos) of the test was done in\n> the main context, while the test case (i.e. the client side, or what\n> the user would do) is in subshells.\n\nThis roughly coincides, as the client is usually in a new clone. :)\n\n>> style: The Git test suite prefers to have the redirect adjacent to the\n>> file name:\n>> echo hello >world\n>\n> I wasn't aware of that, it seems there are some inconsistencies,\n> including in the modified test:\n>\n> eddy@feodora:~/usr/src/git/t$ grep '> ' -c t* 2>/dev/null | grep -v\n> ':0$' | grep 7406\n> t7406-submodule-update.sh:24\n> eddy@feodora:~/usr/src/git/t$ grep '> ' -c t* 2>/dev/null | grep -v\n> ':0$' | wc -l\n> 325\n\nThanks for digging up numbers!\n\n> I suspect that a minor patch correcting these inconsistencies would be\n> faily fast reviewed adn accepted, of course, disconnected from this\n> modification.\n\nThere are two schools of thought, one of them is to fix things up\nwhenever you see fit. I am very sympathetic to this one.\n\nThe other is found in Documentation/CodingGuidelines:\n\n - Fixing style violations while working on a real change as a\n preparatory clean-up step is good, but otherwise avoid useless code\n churn for the sake of conforming to the style.\n\n \"Once it _is_ in the tree, it's not really worth the patch noise to\n go and fix it up.\"\n Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html\n\n> Yes, I was trying to replicate the redox-os case which has this structure:\n>\n> redox-os (master branch)\n> + rust (@some commit on a non-default)\n> + src/llvm (@some commit on a non-default branch)\n>\n> This section is making sure the b2 branch has other content than the\n> default one and setting the expectation for level2 submodule branch,\n> later.\n\nOh, cool!\n\n>\n>>>> + git rev-parse --verify HEAD >../expectl1 &&\n>>>> + git checkout master &&\n>>\n>> We go back to master, which doesn't have the nested submodule?\n>\n> exactly, since the desired behaviour is to have in the nested\n> submodule the b2 branch.\n\nWell if the nested submodule doesn't exist at HEAD, then git should\nnot change branches in there, only on checking out/ updating to a state\nthat has the nested sub? (I may missunderstand things here)\n\n> I guess I could have added the level2 submodule@master on l1's master\n> branch, but I didn't see much point in that since it should be enough\n> now.\n>\n> It might make more sense if master and b2, and, respectively master\n> and l1 have diverging histories, but for now that is probably overkill\n> and it will make sense in the context of shallow cloning of\n> submodules, which I haven't yet tackled except presenting the idea.\n\nok.\n\n>\n>>>> + cd ../super5 &&\n>>>> + echo super_with_2_chained_modules > super5 &&\n>>>> + git add super5 &&\n>>>> + test_tick &&\n>>>> + git commit -m \"commit on default branch in super5\" &&\n>>>> + git submodule add ../submodl1b1 submodl1b1 &&\n>>>> + git config -f .gitmodules submodule.\"submodl1b1\".branch b1 &&\n>>>> + git add .gitmodules &&\n>>>> + test_tick &&\n>>>> + git commit -m \"add l1 module with branch b1 in super5\" &&\n>>\n>> now we do this again without the nested submodule, just one repo\n>> as a submodule?\n>\n> My intention was to have super5 -> submodl1b1@b1 -> submodl2b2@b2 on\n> the \"server\" side.\n> But are you saying I just implemented super5 -> sbmodl1b1@master due\n> to the previous master checkout in submodl1b1?\n\nNo. I was a little confused about the code.\n\n>\n>>>> + git submodule init submodl1b1 &&\n>>>> + git clone super5 super &&\n>>\n>> does super exist here already? (I did not check, but IIRC\n>> super and super{1-4} are there as we count upwards to\n>> find a name that is ok.\n>\n> I created it in the first step of the test with the intention to have\n> super5 as the \"server\" and \"super\" as the client clone.\n\noh, ok.\n\n>\n>>\n>>>> + (\n>>>> + cd super &&\n>>>> + git submodule update --recursive --init\n>>>> + ) &&\n>>>> + (\n>>>> + cd submodl1b1 &&\n>>>> + git rev-parse --verify HEAD >../../actuall1 &&\n>>>> + test_cmp ../../expectl1 ../../actuall1\n>>>> + ) &&\n>>>> + (\n>>>> + cd submodl2b2 &&\n>>>> + git rev-parse --verify HEAD >../../../actuall2 &&\n>>>> + test_cmp ../../../expectl2 ../../../actuall2\n>>>> + )\n>\n> As a general idea for a test, does it look sane?\n\nYes, I think it is a sane approach. Thanks for writing such a test!\n\n> Do you think I should I start with a just one level of submodule with\n> a non-default branch (super -> l1@b1), or it this OK?\n> In my view, having 2 levels makes sure the recursive part is also\n> addressed and verified.\n\nI totally agree.\n\nStefan\n"},{"id":"345031","messageId":"20180418223552.18345-1-eddy.petrisor@codeaurora.org","threadId":"47993","inReplyTo":"CAGZ79kbT+AnVu-AJ9=OSt=Z2ErM5ocfoLfgx=qmHFfbnkqJqwg@mail.gmail.com","subject":"[RFC PATCH v4 1/9] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@codeaurora.org","sentAt":"2018-04-18T22:35:44Z","receivedAt":"2018-04-18T22:36:50Z","isPatch":true,"sender":{"key":"eddy.petrisor@codeaurora.org","avatar":null},"body":"From: Eddy Petrișor \n\nThere are projects such as llvm/clang which use several repositories, and they\nmight be forked for providing support for various features such as adding Redox\nawareness to the toolchain. This typically means the superproject will use\nanother branch than master, occasionally even use an old commit from that\nnon-master branch.\n\nCombined with the fact that when incorporating such a hierachy of repositories\nusually the user is interested in just the exact commit specified in the\nsubmodule info, it follows that a desireable usecase is to be also able to\nprovide '--depth 1' or at least have a shallow clone to avoid waiting for ages\nfor the clone operation to finish.\n\nIn theory, this should be straightforward since the git protocol allows\nfetching an arbitary commit, but, in practice, some servers do not permit\nfetch-by-sha1.\n\nGit submodule seems to be very stubborn and cloning master, although the\nwrapper script and the gitmodules-helper could work together to clone directly\nthe branch specified in the .gitmodules file, if specified.\n\nSigned-off-by: Eddy Petrișor \n---\n git-submodule.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 24914963c..65e3af08b 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -589,8 +589,10 @@ cmd_update()\n \t\t\tbranch=$(git submodule--helper remote-branch \"$sm_path\")\n \t\t\tif test -z \"$nofetch\"\n \t\t\tthen\n+\t\t\t\t# non-default branch refspec\n+\t\t\t\tbr_refspec=$(git submodule-helper remote-branch $sm_path)\n \t\t\t\t# Fetch remote before determining tracking $sha1\n-\t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n+\t\t\t\tfetch_in_submodule \"$sm_path\" $depth $br_refspec ||\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\" && get_default_remote)\n-- \n2.16.2\n\n"},{"id":"345032","messageId":"20180418223552.18345-2-eddy.petrisor@codeaurora.org","threadId":"47993","inReplyTo":"20180418223552.18345-1-eddy.petrisor@codeaurora.org","subject":"[RFC PATCH v4 2/9] t7406: add test for non-default branch in submodule","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@codeaurora.org","sentAt":"2018-04-18T22:35:45Z","receivedAt":"2018-04-18T22:36:57Z","isPatch":true,"sender":{"key":"eddy.petrisor@codeaurora.org","avatar":null},"body":"From: Eddy Petrișor \n\nIf a submodule uses a non-default branch and the branch info is versioned, on\nsubmodule update --recursive --init the correct branch should be checked out.\n\nSigned-off-by: Eddy Petrișor \n---\n t/t7406-submodule-update.sh | 54 +++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 54 insertions(+)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 6f083c4d6..7b65f1dd1 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -259,6 +259,60 @@ test_expect_success 'submodule update --remote should fetch upstream changes wit\n \t)\n '\n \n+test_expect_success 'submodule update --remote --recursive --init should fetch module branch from .gitmodules' '\n+\tgit clone . super5 &&\n+\tgit clone super5 submodl2b2 &&\n+\tgit clone super5 submodl1b1 &&\n+\tcd submodl2b2 &&\n+\techo linel2b2 > l2b2 &&\n+\tgit checkout -b b2 &&\n+\tgit add l2b2 &&\n+\ttest_tick &&\n+\tgit commit -m \"commit on b2 branch in l2\" &&\n+\tgit rev-parse --verify HEAD >../expectl2 &&\n+\tgit checkout master &&\n+\tcd ../submodl1b1 &&\n+\tgit checkout -b b1 &&\n+\techo linel1b1 > l1b1 &&\n+\tgit add l1b1 &&\n+\ttest_tick &&\n+\tgit commit -m \"commit on b1 branch in l1\" &&\n+\tgit submodule add ../submodl2b2 submodl2b2 &&\n+\tgit config -f .gitmodules submodule.\"submodl2b2\".branch b2 &&\n+\tgit add .gitmodules &&\n+\ttest_tick &&\n+\tgit commit -m \"add l2 module with branch b2 in l1 module in branch b1\" &&\n+\tgit submodule init submodl2b2 &&\n+\tgit rev-parse --verify HEAD >../expectl1 &&\n+\tgit checkout master &&\n+\tcd ../super5 &&\n+\techo super_with_2_chained_modules > super5 &&\n+\tgit add super5 &&\n+\ttest_tick &&\n+\tgit commit -m \"commit on default branch in super5\" &&\n+\tgit submodule add ../submodl1b1 submodl1b1 &&\n+\tgit config -f .gitmodules submodule.\"submodl1b1\".branch b1 &&\n+\tgit add .gitmodules &&\n+\ttest_tick &&\n+\tgit commit -m \"add l1 module with branch b1 in super5\" &&\n+\tgit submodule init submodl1b1 &&\n+\tgit clone super5 super &&\n+\t(\n+\t\tcd super &&\n+\t\tgit submodule update --recursive --init\n+\t) &&\n+\t(\n+\t\tcd submodl1b1 &&\n+\t\tgit rev-parse --verify HEAD >../../actuall1 &&\n+\t\ttest_cmp ../../expectl1 ../../actuall1\n+\t) &&\n+\t(\n+\t\tcd submodl2b2 &&\n+\t\tgit rev-parse --verify HEAD >../../../actuall2 &&\n+\t\ttest_cmp ../../../expectl2 ../../../actuall2\n+\t)\n+'\n+\n test_expect_success 'local config should override .gitmodules branch' '\n \t(cd submodule &&\n \t git checkout test-branch &&\n-- \n2.16.2\n\n"},{"id":"345033","messageId":"20180418223552.18345-3-eddy.petrisor@codeaurora.org","threadId":"47993","inReplyTo":"20180418223552.18345-1-eddy.petrisor@codeaurora.org","subject":"[RFC PATCH v4 3/9] fixup:t7406: use test_commit instead of echo/add/commit as suggested by Stefan Beller","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@codeaurora.org","sentAt":"2018-04-18T22:35:46Z","receivedAt":"2018-04-18T22:37:00Z","isPatch":true,"sender":{"key":"eddy.petrisor@codeaurora.org","avatar":null},"body":"From: Eddy Petrișor \n\nSigned-off-by: Eddy Petrișor \n---\n t/t7406-submodule-update.sh | 15 +++------------\n 1 file changed, 3 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 7b65f1dd1..7fb370991 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -264,19 +264,13 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m\n \tgit clone super5 submodl2b2 &&\n \tgit clone super5 submodl1b1 &&\n \tcd submodl2b2 &&\n-\techo linel2b2 > l2b2 &&\n \tgit checkout -b b2 &&\n-\tgit add l2b2 &&\n-\ttest_tick &&\n-\tgit commit -m \"commit on b2 branch in l2\" &&\n+\ttest_commit \"l2_on_b2\" &&\n \tgit rev-parse --verify HEAD >../expectl2 &&\n \tgit checkout master &&\n \tcd ../submodl1b1 &&\n \tgit checkout -b b1 &&\n-\techo linel1b1 > l1b1 &&\n-\tgit add l1b1 &&\n-\ttest_tick &&\n-\tgit commit -m \"commit on b1 branch in l1\" &&\n+\ttest_commit \"l1_on_b1\" &&\n \tgit submodule add ../submodl2b2 submodl2b2 &&\n \tgit config -f .gitmodules submodule.\"submodl2b2\".branch b2 &&\n \tgit add .gitmodules &&\n@@ -286,10 +280,7 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m\n \tgit rev-parse --verify HEAD >../expectl1 &&\n \tgit checkout master &&\n \tcd ../super5 &&\n-\techo super_with_2_chained_modules > super5 &&\n-\tgit add super5 &&\n-\ttest_tick &&\n-\tgit commit -m \"commit on default branch in super5\" &&\n+\ttest_commit super5_with_2_chained_modules_on_default_branch &&\n \tgit submodule add ../submodl1b1 submodl1b1 &&\n \tgit config -f .gitmodules submodule.\"submodl1b1\".branch b1 &&\n \tgit add .gitmodules &&\n-- \n2.16.2\n\n"},{"id":"345034","messageId":"20180418223552.18345-5-eddy.petrisor@codeaurora.org","threadId":"47993","inReplyTo":"20180418223552.18345-1-eddy.petrisor@codeaurora.org","subject":"[RFC PATCH v4 5/9] fixup:t7406:cleanup chained submodules after test is done","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@codeaurora.org","sentAt":"2018-04-18T22:35:48Z","receivedAt":"2018-04-18T22:37:04Z","isPatch":true,"sender":{"key":"eddy.petrisor@codeaurora.org","avatar":null},"body":"From: Eddy Petrișor \n\nSigned-off-by: Eddy Petrișor \n---\n t/t7406-submodule-update.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 974f949df..c5b412c46 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -298,7 +298,9 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m\n \t\tcd submodl2b2 &&\n \t\tgit rev-parse --verify HEAD >../../../actuall2 &&\n \t\ttest_cmp ../../../expectl2 ../../../actuall2\n-\t)\n+\t) &&\n+\ttest_when_finished \"rm submodl2b2\" &&\n+\ttest_when_finished \"rm submodl1b1\"\n '\n \n test_expect_success 'local config should override .gitmodules branch' '\n-- \n2.16.2\n\n"},{"id":"345035","messageId":"20180418223552.18345-4-eddy.petrisor@codeaurora.org","threadId":"47993","inReplyTo":"20180418223552.18345-1-eddy.petrisor@codeaurora.org","subject":"[RFC PATCH v4 4/9] fixup:t7404:use 'git -C' instead of cd .. && git","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@codeaurora.org","sentAt":"2018-04-18T22:35:47Z","receivedAt":"2018-04-18T22:37:06Z","isPatch":true,"sender":{"key":"eddy.petrisor@codeaurora.org","avatar":null},"body":"From: Eddy Petrișor \n\nSigned-off-by: Eddy Petrișor \n---\n t/t7406-submodule-update.sh | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 7fb370991..974f949df 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -288,10 +288,7 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m\n \tgit commit -m \"add l1 module with branch b1 in super5\" &&\n \tgit submodule init submodl1b1 &&\n \tgit clone super5 super &&\n-\t(\n-\t\tcd super &&\n-\t\tgit submodule update --recursive --init\n-\t) &&\n+\tgit -C super submodule update --recursive --init &&\n \t(\n \t\tcd submodl1b1 &&\n \t\tgit rev-parse --verify HEAD >../../actuall1 &&\n-- \n2.16.2\n\n"},{"id":"345036","messageId":"20180418223552.18345-6-eddy.petrisor@codeaurora.org","threadId":"47993","inReplyTo":"20180418223552.18345-1-eddy.petrisor@codeaurora.org","subject":"[RFC PATCH v4 6/9] fixup:t7406:don't call init after add, is redundant","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@codeaurora.org","sentAt":"2018-04-18T22:35:49Z","receivedAt":"2018-04-18T22:37:08Z","isPatch":true,"sender":{"key":"eddy.petrisor@codeaurora.org","avatar":null},"body":"From: Eddy Petrișor \n\nSigned-off-by: Eddy Petrișor \n---\n t/t7406-submodule-update.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex c5b412c46..32995e272 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -275,8 +275,7 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m\n \tgit config -f .gitmodules submodule.\"submodl2b2\".branch b2 &&\n \tgit add .gitmodules &&\n \ttest_tick &&\n-\tgit commit -m \"add l2 module with branch b2 in l1 module in branch b1\" &&\n-\tgit submodule init submodl2b2 &&\n+\tgit commit -m \"add l2 (on b2) in l1 (on b1)\" &&\n \tgit rev-parse --verify HEAD >../expectl1 &&\n \tgit checkout master &&\n \tcd ../super5 &&\n-- \n2.16.2\n\n"},{"id":"345037","messageId":"20180418223552.18345-7-eddy.petrisor@codeaurora.org","threadId":"47993","inReplyTo":"20180418223552.18345-1-eddy.petrisor@codeaurora.org","subject":"[RFC PATCH v4 7/9] fixup:t7406:supr5 commit is done before submodules chaining","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@codeaurora.org","sentAt":"2018-04-18T22:35:50Z","receivedAt":"2018-04-18T22:37:10Z","isPatch":true,"sender":{"key":"eddy.petrisor@codeaurora.org","avatar":null},"body":"From: Eddy Petrișor \n\nSigned-off-by: Eddy Petrișor \n---\n t/t7406-submodule-update.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 32995e272..18328be73 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -279,7 +279,7 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m\n \tgit rev-parse --verify HEAD >../expectl1 &&\n \tgit checkout master &&\n \tcd ../super5 &&\n-\ttest_commit super5_with_2_chained_modules_on_default_branch &&\n+\ttest_commit super5_commit_before_2_chained_modules_on_default_branch &&\n \tgit submodule add ../submodl1b1 submodl1b1 &&\n \tgit config -f .gitmodules submodule.\"submodl1b1\".branch b1 &&\n \tgit add .gitmodules &&\n-- \n2.16.2\n\n"},{"id":"345038","messageId":"20180418223552.18345-9-eddy.petrisor@codeaurora.org","threadId":"47993","inReplyTo":"20180418223552.18345-1-eddy.petrisor@codeaurora.org","subject":"[RFC PATCH v4 9/9] fixup:t7406:change branches in submodules after the link is done","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@codeaurora.org","sentAt":"2018-04-18T22:35:52Z","receivedAt":"2018-04-18T22:37:11Z","isPatch":true,"sender":{"key":"eddy.petrisor@codeaurora.org","avatar":null},"body":"From: Eddy Petrișor \n\nSigned-off-by: Eddy Petrișor \n---\n t/t7406-submodule-update.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex f44872143..68c25ce0f 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -267,17 +267,16 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m\n \tgit checkout -b b2 &&\n \ttest_commit \"l2_on_b2\" &&\n \tgit rev-parse --verify HEAD >../expectl2 &&\n-\tgit checkout master &&\n \tcd ../submodl1b1 &&\n \tgit checkout -b b1 &&\n \ttest_commit \"l1_on_b1\" &&\n \tgit submodule add ../submodl2b2 submodl2b2 &&\n \tgit config -f .gitmodules submodule.\"submodl2b2\".branch b2 &&\n \tgit add .gitmodules &&\n+\tgit -C ../submodl2b2 checkout master &&\n \ttest_tick &&\n \tgit commit -m \"add l2 (on b2) in l1 (on b1)\" &&\n \tgit rev-parse --verify HEAD >../expectl1 &&\n-\tgit checkout master &&\n \tcd ../super5 &&\n \ttest_commit super5_commit_before_2_chained_modules_on_default_branch &&\n \tgit submodule add ../submodl1b1 submodl1b1 &&\n@@ -285,6 +284,7 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m\n \tgit add .gitmodules &&\n \ttest_tick &&\n \tgit commit -m \"add l1 module with branch b1 in super5\" &&\n+\tgit -C ../submodl1b1 checkout master &&\n \tgit submodule init submodl1b1 &&\n \tcd .. &&\n \tgit clone super5 super_w &&\n-- \n2.16.2\n\n"},{"id":"345039","messageId":"20180418223552.18345-8-eddy.petrisor@codeaurora.org","threadId":"47993","inReplyTo":"20180418223552.18345-1-eddy.petrisor@codeaurora.org","subject":"[RFC PATCH v4 8/9] fixup:t7406:use super_w instead of the existing super","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@codeaurora.org","sentAt":"2018-04-18T22:35:51Z","receivedAt":"2018-04-18T22:37:13Z","isPatch":true,"sender":{"key":"eddy.petrisor@codeaurora.org","avatar":null},"body":"From: Eddy Petrișor \n\nSigned-off-by: Eddy Petrișor \n---\n t/t7406-submodule-update.sh | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 18328be73..f44872143 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -286,10 +286,11 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m\n \ttest_tick &&\n \tgit commit -m \"add l1 module with branch b1 in super5\" &&\n \tgit submodule init submodl1b1 &&\n-\tgit clone super5 super &&\n-\tgit -C super submodule update --recursive --init &&\n+\tcd .. &&\n+\tgit clone super5 super_w &&\n+\tgit -C super_w submodule update --recursive --init &&\n \t(\n-\t\tcd submodl1b1 &&\n+\t\tcd super_w/submodl1b1 &&\n \t\tgit rev-parse --verify HEAD >../../actuall1 &&\n \t\ttest_cmp ../../expectl1 ../../actuall1\n \t) &&\n@@ -298,6 +299,7 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m\n \t\tgit rev-parse --verify HEAD >../../../actuall2 &&\n \t\ttest_cmp ../../../expectl2 ../../../actuall2\n \t) &&\n+\ttest_when_finished \"rm super_w\" &&\n \ttest_when_finished \"rm submodl2b2\" &&\n \ttest_when_finished \"rm submodl1b1\"\n '\n-- \n2.16.2\n\n"},{"id":"345043","messageId":"CAGZ79kZL_+AZx2kFyoOokUPsqRzQFT32aj1XqHf8o7OJFwsABA@mail.gmail.com","threadId":"47993","inReplyTo":"20180418223552.18345-1-eddy.petrisor@codeaurora.org","subject":"Re: [RFC PATCH v4 1/9] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-18T23:53:21Z","receivedAt":"2018-04-19T00:59:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi Eddy,\n\nall the following patches 3-9 are touching the test as added in patch\n2, which would go best with this patch.\nCould you squash all commits into one?\n\nThere are a couple ways to do it:\n\n git reset --soft\n git commit -a --reuse-commit-message=<...>\n\nor using\n\n git rebase --interactive origin/master\n # and then marking all but the first as \"fixup\"\n\nI think the end result looks good, but that is best reviewed as one\npiece instead of 9 patches.\n\nThanks,\nStefan\n"},{"id":"345065","messageId":"CAK0XTWcOpb5DS5c0MAomfaS2G0TPDTDtchm9FhkOSDBS5r5j+g@mail.gmail.com","threadId":"47993","inReplyTo":"CAGZ79kZL_+AZx2kFyoOokUPsqRzQFT32aj1XqHf8o7OJFwsABA@mail.gmail.com","subject":"Re: [RFC PATCH v4 1/9] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@gmail.com","sentAt":"2018-04-19T05:43:38Z","receivedAt":"2018-04-19T05:43:47Z","isPatch":true,"sender":{"key":"eddy.petrisor@gmail.com","avatar":"https://gravatar.com/avatar/e13eef6cecbe95604b4667c7bddf5aee2bb4af684c5fdceaf9e2aebfead3e9f5?d=mp&s=160"},"body":"2018-04-19 2:53 GMT+03:00 Stefan Beller :\n> Hi Eddy,\n>\n> all the following patches 3-9 are touching the test as added in patch\n> 2, which would go best with this patch.\n> Could you squash all commits into one?\n\nYes,\n\nI did not have time yesterday to put all changes into a single commit\nwith an associated note (because note managment seems to be a huge\npain), so I preferred small commits.\n\nBut I wanted to get your feedback on something, I'll reply in thread\narm where you actually suspected the problem.\n\n> There are a couple ways to do it:\n>\n> git reset --soft\n> git commit -a --reuse-commit-message=<...>\n>\n> or using\n>\n> git rebase --interactive origin/master\n> # and then marking all but the first as \"fixup\"\n\nI am aware of git rebase -i and use it regularly, that's why patches\n3-9 have the 'fixup' prefix.\n\n> I think the end result looks good, but that is best reviewed as one\n> piece instead of 9 patches.\n\n\n-- \nEddy Petrișor\n"},{"id":"345066","messageId":"CAK0XTWecZASWULXYXGauV5WKhYinD7HXyXSTJntocQt3vzr4AA@mail.gmail.com","threadId":"47993","inReplyTo":"CAGZ79kbT+AnVu-AJ9=OSt=Z2ErM5ocfoLfgx=qmHFfbnkqJqwg@mail.gmail.com","subject":"Re: [RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule","fromName":"Eddy Petrișor","fromEmail":"eddy.petrisor@gmail.com","sentAt":"2018-04-19T06:07:51Z","receivedAt":"2018-04-19T06:07:57Z","isPatch":true,"sender":{"key":"eddy.petrisor@gmail.com","avatar":"https://gravatar.com/avatar/e13eef6cecbe95604b4667c7bddf5aee2bb4af684c5fdceaf9e2aebfead3e9f5?d=mp&s=160"},"body":"2018-04-05 0:41 GMT+03:00 Stefan Beller :\n> On Wed, Apr 4, 2018 at 1:37 PM, Eddy Petrișor wrote:\n>\n>>> you plan to later submit as one patch including both the change as well as\n>>> the test.\n>>\n>> Yes,\n\nI did not forget about having a single patch. Will do it once the\ndetails are ironed out.\n\n>>>>> + cd ../super5 &&\n>>>>> + echo super_with_2_chained_modules > super5 &&\n>>>>> + git add super5 &&\n>>>>> + test_tick &&\n>>>>> + git commit -m \"commit on default branch in super5\" &&\n>>>>> + git submodule add ../submodl1b1 submodl1b1 &&\n>>>>> + git config -f .gitmodules submodule.\"submodl1b1\".branch b1 &&\n>>>>> + git add .gitmodules &&\n>>>>> + test_tick &&\n>>>>> + git commit -m \"add l1 module with branch b1 in super5\" &&\n>>>\n>>> now we do this again without the nested submodule, just one repo\n>>> as a submodule?\n>>\n>> My intention was to have super5 -> submodl1b1@b1 -> submodl2b2@b2 on\n>> the \"server\" side.\n>> But are you saying I just implemented super5 -> sbmodl1b1@master due\n>> to the previous master checkout in submodl1b1?\n>\n> No. I was a little confused about the code.\n\nActually you were 100% correct. In order to link to submodl1b1@b1 I\nhad to move the master branch checkout after the submobl2@b2 is added.\n\nOtherwise the submodule is added with the last commit on master, not\nthe last one on b1 an b2, respectively.\n\nI suspect that in the tests, because the \"server side\" repos are\nlocal, the git fetch-by-sha1/cloning by hash will be done correctly,\nwithout the need of a branch hint, but the problem will still exist\nfor servers such as github which do not support fetch-by-sha1.\nIn case I realize that a server-side repo that doesn't support\nfetch-by-sha1 is needed, is there a mechanism to set that up in the\ntest case, or do I have to rethink my approach?\n\n>>>>> + git submodule init submodl1b1 &&\n>>>>> + git clone super5 super &&\n>>>\n>>> does super exist here already? (I did not check, but IIRC\n>>> super and super{1-4} are there as we count upwards to\n>>> find a name that is ok.\n>>\n>> I created it in the first step of the test with the intention to have\n>> super5 as the \"server\" and \"super\" as the client clone.\n>\n> oh, ok.\n\nAfter using test_pause I realized 'super' is left over by some other\ntest cases, so in my v4 (unjustifibly) long series I switch to using\nsuper_w because I was getting all sorts of issues and wanted to not\ninterfere with the other tests.\n\n>> As a general idea for a test, does it look sane?\n>\n> Yes, I think it is a sane approach. Thanks for writing such a test!\n\nOK, thanks for the feedback.\n\n>> Do you think I should I start with a just one level of submodule with\n>> a non-default branch (super -> l1@b1), or it this OK?\n>> In my view, having 2 levels makes sure the recursive part is also\n>> addressed and verified.\n>\n> I totally agree.\n\n\n-- \nEddy Petrișor\n"},{"id":"345117","messageId":"CAGZ79kaCW=iK3-Aq41Z5=WOz8hgrbo41XDCmnTVsAeqKwOuydA@mail.gmail.com","threadId":"47993","inReplyTo":"CAK0XTWecZASWULXYXGauV5WKhYinD7HXyXSTJntocQt3vzr4AA@mail.gmail.com","subject":"Re: [RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-19T17:52:46Z","receivedAt":"2018-04-19T17:52:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi Eddy,\n\n> I suspect that in the tests, because the \"server side\" repos are\n> local, the git fetch-by-sha1/cloning by hash will be done correctly,\n> without the need of a branch hint, but the problem will still exist\n> for servers such as github which do not support fetch-by-sha1.\n> In case I realize that a server-side repo that doesn't support\n> fetch-by-sha1 is needed, is there a mechanism to set that up in the\n> test case, or do I have to rethink my approach?\n\nYou can force a clone (at least, not sure about fetch) to use the\ngit protocol by --no-local, and then you can set\nuploadpack.{allowTipSHA1InWant, allowReachableSHA1InWant,\nallowAnySHA1InWant} as neded by the test.\n\nFrom the fetch man page:\nFor local repositories, also supported by Git natively,\nthe following syntaxes may be used:\n\n · /path/to/repo.git/\n\n · file:///path/to/repo.git/\n\nThese two syntaxes are mostly equivalent, except\nwhen cloning, when the former implies --local option.\n\nThanks,\nStefan\n"}]}