{"thread":{"id":"40255","subject":"[PATCH/RFC] Pinning of submodules","startedAt":"2015-09-02T23:34:24Z","lastAt":"2015-09-08T16:55:49Z","messageCount":7,"participants":["Anders Ro","Eric Sunshine","Jens Lehmann","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"269248","messageId":"55E78780.2040801@gmail.com","threadId":"40255","inReplyTo":"55E78522.1030107@gmail.com","subject":"[PATCH/RFC] Pinning of submodules","fromName":"Anders Ro","fromEmail":"anders.ronnbrant@gmail.com","sentAt":"2015-09-02T23:34:24Z","receivedAt":"2015-09-02T23:34:24Z","isPatch":true,"sender":{"key":"anders.ronnbrant@gmail.com","avatar":"https://gravatar.com/avatar/b0f21dd0e596dc35bdf2dc9b1df6d46e0069c96651eb4542056983f3520a4478?d=mp&s=160"},"body":"Patch to make it possible to pin submodules so that they are not\naffected by the --remote option in \"git submodule\".\n\nAnders Ro (2):\n  git-submodule.sh: pin submodule when branch name is '@'\n  t7412: add test case for pinned submodules\n\n Documentation/git-submodule.txt |  3 +-\n git-submodule.sh                |  9 ++++-\n t/t7412-submodule-pinning.sh    | 73\n+++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 83 insertions(+), 2 deletions(-)\n create mode 100755 t/t7412-submodule-pinning.sh\n\n\n\nFrom 32b25aded0c4e30c68ac3ab75f4cbb63302ca147 Mon Sep 17 00:00:00 2001\nFrom: Anders Ro <anders.ronnbrant@gmail.com>\nDate: Fri, 28 Nov 2014 01:39:37 +0100\nSubject: [PATCH 1/2] git-submodule.sh: pin submodule when branch name is '@'\n\nSetting branch name to '@' for a submodule will disable 'git submodule\nupdate --remote' calls for that specific submodule. I.e. instead of\nfollow the unspecified default choice of master, nothing is being\nupdated. This is useful when multiple submodules exist but not all\nshould follow the remote branch head.\n\nSigned-off-by: Anders Ro <anders.ronnbrant@gmail.com>\n---\n Documentation/git-submodule.txt | 3 ++-\n git-submodule.sh                | 9 ++++++++-\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex f17687e..bd0cced 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -289,7 +289,8 @@ OPTIONS\n \tThe remote branch used defaults to `master`, but the branch name may\n \tbe overridden by setting the `submodule.<name>.branch` option in\n \teither `.gitmodules` or `.git/config` (with `.git/config` taking\n-\tprecedence).\n+\tprecedence). Setting the branch name to the invalid branch name '@'\n+\tdisables this option, i.e. pins the submodule at the recorded SHA-1.\n +\n This works for any of the supported update procedures (`--checkout`,\n `--rebase`, etc.).  The only change is the source of the target SHA-1.\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 25b1ddf..1bb2bb1 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -843,7 +843,8 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\tdie \"$(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n \t\tfi\n \n-\t\tif test -n \"$remote\"\n+\t\t# Fetch latest in remote unless branch name in config is '@'\n+\t\tif test -n \"$remote\" -a \"$branch\" != \"@\"\n \t\tthen\n \t\t\tif test -z \"$nofetch\"\n \t\t\tthen\n@@ -857,6 +858,12 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\tdie \"$(eval_gettext \"Unable to find current ${remote_name}/${branch} revision in submodule path '\\$sm_path'\")\"\n \t\tfi\n \n+\t\t# Inform that the current sm is pinned and use of '--remote' ignored\n+\t\tif test -n \"$remote\" -a \"$branch\" = \"@\"\n+\t\tthen\n+\t\t\tsay \"$(eval_gettext \"Submodule path '\\$displaypath' pinned, remote update ignored\")\"\n+\t\tfi\n+\n \t\tif test \"$subsha1\" != \"$sha1\" || test -n \"$force\"\n \t\tthen\n \t\t\tsubforce=$force\n-- \n2.1.4\n\n\n\nFrom 98e6677325bada62301eba18b81f4884793d7c02 Mon Sep 17 00:00:00 2001\nFrom: Anders Ro <anders.ronnbrant@gmail.com>\nDate: Thu, 3 Sep 2015 00:03:15 +0200\nSubject: [PATCH 2/2] t7412: add test case for pinned submodules\n\nSigned-off-by: Anders Ro <anders.ronnbrant@gmail.com>\n---\n t/t7412-submodule-pinning.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 73 insertions(+)\n create mode 100755 t/t7412-submodule-pinning.sh\n\ndiff --git a/t/t7412-submodule-pinning.sh b/t/t7412-submodule-pinning.sh\nnew file mode 100755\nindex 0000000..2844b1e\n--- /dev/null\n+++ b/t/t7412-submodule-pinning.sh\n@@ -0,0 +1,73 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2015 Anders Ronnbrant\n+#\n+\n+test_description=\"Branch name '@' disables submodule update --remote calls\"\n+\n+. ./test-lib.sh\n+\n+get_sha() {\n+  cd $1 && git rev-list --max-count=1 HEAD\n+}\n+\n+equal_sha() {\n+  test \"$(get_sha $1)\" = \"$(get_sha $2)\"\n+}\n+\n+not_equal_sha() {\n+  test \"$(get_sha $1)\" != \"$(get_sha $2)\"\n+}\n+\n+test_expect_success 'setup submodule tree structure' '\n+for i in 1 2 3; do echo file$i > file$i; git add file$i; git commit -m file$i; done &&\n+test_tick &&\n+git clone . super &&\n+git clone . follow &&\n+git clone . pinned &&\n+(cd super && git submodule add -b master ../follow follow) &&\n+(cd super && git submodule add           ../pinned pinned)\n+'\n+\n+test_expect_success 'verify submodules have the same SHA' '\n+equal_sha super/follow super/pinned\n+'\n+\n+\n+test_expect_success 'switch submodule pinned to HEAD~1 and commit super' '\n+(cd super/pinned && git checkout master && git reset --hard HEAD~1) &&\n+(cd super && git add pinned && git commit -m \"Submodule pinned @ HEAD~1\") &&\n+(cd super && git submodule status)\n+'\n+\n+\n+test_expect_success 'verify submodules not have the same SHA anymore' '\n+not_equal_sha super/follow super/pinned\n+'\n+\n+\n+test_expect_success 'set branch name to \"@\" for submodule pinned' '\n+(cd super && git config --replace-all submodule.pinned.branch \"@\") &&\n+test \"$(cd super && git config --get submodule.pinned.branch)\" = \"@\"\n+'\n+\n+\n+test_expect_success 'run submodule update --remote and expect no change' '\n+(cd super && git submodule update --remote) &&\n+not_equal_sha super/follow super/pinned\n+'\n+\n+\n+test_expect_success 'remove branch name \"@\" for submodule pinned (unpin)' '\n+(cd super && git config --unset-all submodule.pinned.branch) &&\n+(cd super && git config --list)\n+'\n+\n+\n+test_expect_success 'run submodule update --remote and expect same SHA1 again' '\n+(cd super && git submodule update --remote) &&\n+equal_sha super/follow super/pinned\n+'\n+\n+\n+test_done\n-- \n2.1.4\n\n"},{"id":"269398","messageId":"CAPig+cQ6Ls1iasVsTwbpqGWt9=7gBuWjWAPmSMJCStsdPUwCgg@mail.gmail.com","threadId":"40255","inReplyTo":"55E78780.2040801@gmail.com","subject":"Re: [PATCH/RFC] Pinning of submodules","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-04T05:02:41Z","receivedAt":"2015-09-04T05:02:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 2, 2015 at 7:34 PM, Anders Ro <anders.ronnbrant@gmail.com> wrote:\n> Patch to make it possible to pin submodules so that they are not\n> affected by the --remote option in \"git submodule\".\n\nThanks for the patches. I don't use submodules, so I can't comment\nspecifically on this change, however, I can offer some general\ncomments on the patches. But first, a piece of advice...\n\nUse git-send-email to post the patches as proper emails, one email\nper patch, rather than as attachments. Reviewers are going to want to\nwrite inline comments on the patches, and they can't do so when the\npatches are attachments, so attaching patches discourages reviewers\nfrom responding.\n\n> git-submodule.sh: pin submodule when branch name is '@'\n>\n> Setting branch name to '@' for a submodule will disable 'git submodule\n> update --remote' calls for that specific submodule. I.e. instead of\n> follow the unspecified default choice of master, nothing is being\n> updated. This is useful when multiple submodules exist but not all\n> should follow the remote branch head.\n\nWith the disclaimer that I'm not a submodule user (to which the\nanswer might be obvious): What benefit is there in using a magic\nvalue like this (\"@\") over, say, an explicit configuration setting?\n\n> Signed-off-by: Anders Ro <anders.ronnbrant@gmail.com>\n> ---\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 25b1ddf..1bb2bb1 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -843,7 +843,8 @@ Maybe you want to use 'update --init'?\")\"\n>   die \"$(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n>   fi\n>\n> - if test -n \"$remote\"\n> + # Fetch latest in remote unless branch name in config is '@'\n> + if test -n \"$remote\" -a \"$branch\" != \"@\"\n\nThe -a option to 'test' is not portable[1] and is considered obsolete\nby POSIX[2]. Use \"test foo && test bar\" instead.\n\n[1]: http://www.gnu.org/software/autoconf/manual/autoconf.html#index-g_t_0040command_007btest_007d-1793\n[2]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html\n\n>   then\n>   if test -z \"$nofetch\"\n>   then\n> @@ -857,6 +858,12 @@ Maybe you want to use 'update --init'?\")\"\n>   die \"$(eval_gettext \"Unable to find current ${remote_name}/${branch} revision in submodule path '\\$sm_path'\")\"\n>   fi\n>\n> + # Inform that the current sm is pinned and use of '--remote' ignored\n> + if test -n \"$remote\" -a \"$branch\" = \"@\"\n\nDitto.\n\n> + then\n> + say \"$(eval_gettext \"Submodule path '\\$displaypath' pinned, remote update ignored\")\"\n> + fi\n> +\n\n> Subject: [PATCH 2/2] t7412: add test case for pinned submodules\n>\n> Signed-off-by: Anders Ro <anders.ronnbrant@gmail.com>\n> ---\n> diff --git a/t/t7412-submodule-pinning.sh b/t/t7412-submodule-pinning.sh\n> new file mode 100755\n> index 0000000..2844b1e\n> --- /dev/null\n> +++ b/t/t7412-submodule-pinning.sh\n> @@ -0,0 +1,73 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2015 Anders Ronnbrant\n> +#\n> +\n> +test_description=\"Branch name '@' disables submodule update --remote calls\"\n> +\n> +. ./test-lib.sh\n> +\n> +get_sha() {\n> +  cd $1 && git rev-list --max-count=1 HEAD\n> +}\n\nA few issues:\n\nIndent with tabs (width 8), not spaces. (This comment applies to the\nentire patch).\n\nStyle for shell scripts on this project is to have a space before\n\"()\".\n\nTaking a hint from t/test-lib-functions.sh:test_cmp_rev(), use \"git\nrev-parse --verify\" instead.\n\nIt's a bit ugly that this does \"cd $1\" without ever balancing it with\na return to the original directory. If someone later comes along and\nwrites:\n\n    get_sha1 fiddle >fiddle-id\n\nin a new test, then that person will be surprised to find that the\ncurrent working directory changed out from under him. The current\npatch doesn't experience this problem since it always invokes it as\n$(get_sha1 fiddle), but it could be made more robust by either\nwrapping it in a subshell so that the 'cd' is undone when the\nsubshell exits, or by using \"-C $1\".\n\nTaking the above comments into account:\n\n    get_sha () {\n        git -C \"$1\" rev-parse --verify HEAD\n    }\n\n> +equal_sha() {\n> +  test \"$(get_sha $1)\" = \"$(get_sha $2)\"\n> +}\n> +\n> +not_equal_sha() {\n> +  test \"$(get_sha $1)\" != \"$(get_sha $2)\"\n> +}\n\nStyle: space before \"()\" on these two function declarations\n\n> +test_expect_success 'setup submodule tree structure' '\n> +for i in 1 2 3; do echo file$i > file$i; git add file$i; git commit -m file$i; done &&\n\nStyle: write each command of the 'for' loop on its own line,\nincluding 'do', don't use semi-colons\n\nStyle: no space after redirection operator: >file$i\n\nKeep the &&-chain intact, and end the chain with \"|| return $?\" so\nthat the 'for' loop correctly exits if any contained command fails.\n\n    for i in 1 2 3\n    do\n        echo file$i >file$i &&\n        git add file$i &&\n        git commit -m file$i || return $?\n    done\n\n> +test_tick &&\n\nWhy is this needed? There doesn't seem to be any obvious reason for\nits presence, and the test still passes without it.\n\n> +git clone . super &&\n> +git clone . follow &&\n> +git clone . pinned &&\n> +(cd super && git submodule add -b master ../follow follow) &&\n> +(cd super && git submodule add           ../pinned pinned)\n\nStyle: it's probably better to avoid aligning columns like this since\nit can end up being a maintenance headache, and because the line with\nthe extensive blank span can be difficult to read in isolation.\n\nSince both of these execute in directory 'super', this could be\nstated more concisely as:\n\n    (\n    cd super &&\n    git submodule add ... &&\n    git submodule add ...\n    ) &&\n\nPlacement of the subshell parentheses is intentional.\n\n> +'\n> +\n> +test_expect_success 'verify submodules have the same SHA' '\n> +equal_sha super/follow super/pinned\n> +'\n\nStyle: indent the test body (using tab); this comment applies to the\nentire patch\n\n> +\n> +\n\nStyle: one blank line between tests, not two\n\n> +test_expect_success 'switch submodule pinned to HEAD~1 and commit super' '\n> +(cd super/pinned && git checkout master && git reset --hard HEAD~1) &&\n> +(cd super && git add pinned && git commit -m \"Submodule pinned @ HEAD~1\") &&\n> +(cd super && git submodule status)\n> +'\n\nStyle: one command per line; format subshell as shown in above\nexample\n\nYou can combine the two 'cd super' bits into a single subshell.\n\n> +\n> +\n> +test_expect_success 'verify submodules not have the same SHA anymore' '\n> +not_equal_sha super/follow super/pinned\n> +'\n\nEach test should be a logical unit. In prose, you might describe a\ntest as \"set condition A, make change B, and verify that A and B\nresulted in some expected state\".\n\nThis test is the \"verify A and B\" part, and the preceding test is the\n\"set condition A, make change B\" part, however, splitting them up\nlike this makes it difficult for the reader to understand how they\nare related. Worse, someone may come along and insert a new test\nbetween the two, not realizing that these two tests are logically\none, possibly breaking them.\n\nConsequently, this test should be combined with the one preceding it.\n\n> +\n> +\n> +test_expect_success 'set branch name to \"@\" for submodule pinned' '\n> +(cd super && git config --replace-all submodule.pinned.branch \"@\") &&\n> +test \"$(cd super && git config --get submodule.pinned.branch)\" = \"@\"\n\nWhat is the last line testing? It appears to be testing the behavior\nof git-config, which is outside the scope of this test script.\n\n> +'\n> +\n> +\n> +test_expect_success 'run submodule update --remote and expect no change' '\n> +(cd super && git submodule update --remote) &&\n> +not_equal_sha super/follow super/pinned\n> +'\n\nAll the above comments apply here too. These two tests are logically\none, thus should be combined.\n\nOnce combined, you can use test_config rather than git-config, since\ntest_config will ensure that the config setting is undone when the\ntest exits.\n\n> +\n> +\n> +test_expect_success 'remove branch name \"@\" for submodule pinned (unpin)' '\n> +(cd super && git config --unset-all submodule.pinned.branch) &&\n\nIf you take the above advice and use test_config in the previous\ntest, then this won't be needed.\n\n> +(cd super && git config --list)\n\nThis isn't testing anything at all, but is instead merely cluttering\nthe output (when --verbose is enabled). It looks like debugging code\nwhich should have been removed before committing.\n\n> +'\n> +\n> +\n> +test_expect_success 'run submodule update --remote and expect same SHA1 again' '\n> +(cd super && git submodule update --remote) &&\n\nFor these one-line subshells, \"-C super\" is easier and reads\nbetter. (This comment also applies to the rest of the patch.)\n\n> +equal_sha super/follow super/pinned\n> +'\n\nThis test should be combined with the previous one (although, if you\nfollow the advice regarding the previous test, then that test will\ncollapse to nothing...).\n\n> +\n> +\n> +test_done\n> --\n> 2.1.4\n"},{"id":"269547","messageId":"55ECB956.1080909@gmail.com","threadId":"40255","inReplyTo":"CAPig+cQ6Ls1iasVsTwbpqGWt9=7gBuWjWAPmSMJCStsdPUwCgg@mail.gmail.com","subject":"Re: [PATCH/RFC] Pinning of submodules","fromName":"Anders Ro","fromEmail":"anders.ronnbrant@gmail.com","sentAt":"2015-09-06T22:08:22Z","receivedAt":"2015-09-06T22:08:22Z","isPatch":true,"sender":{"key":"anders.ronnbrant@gmail.com","avatar":"https://gravatar.com/avatar/b0f21dd0e596dc35bdf2dc9b1df6d46e0069c96651eb4542056983f3520a4478?d=mp&s=160"},"body":"On 04/09/15 07:02, Eric Sunshine wrote:\n> On Wed, Sep 2, 2015 at 7:34 PM, Anders Ro <anders.ronnbrant@gmail.com> wrote:\n>> Patch to make it possible to pin submodules so that they are not\n>> affected by the --remote option in \"git submodule\".\n> \n> Thanks for the patches. I don't use submodules, so I can't comment\n> specifically on this change, however, I can offer some general\n> comments on the patches. But first, a piece of advice...\n> \n> Use git-send-email to post the patches as proper emails, one email\n> per patch, rather than as attachments. Reviewers are going to want to\n> write inline comments on the patches, and they can't do so when the\n> patches are attachments, so attaching patches discourages reviewers\n> from responding.\n\nThanks for the advice and the code comments, I knew I would get it wrong\nsomehow :). I did try to follow an IMAP Gmail guide in a hurry. I'll fix\nthe code and make sure git send-mail works the next time.\n\n>> git-submodule.sh: pin submodule when branch name is '@'\n>>\n>> Setting branch name to '@' for a submodule will disable 'git submodule\n>> update --remote' calls for that specific submodule. I.e. instead of\n>> follow the unspecified default choice of master, nothing is being\n>> updated. This is useful when multiple submodules exist but not all\n>> should follow the remote branch head.\n> \n> With the disclaimer that I'm not a submodule user (to which the\n> answer might be obvious): What benefit is there in using a magic\n> value like this (\"@\") over, say, an explicit configuration setting?\n\n>From what I have understood (not a submodule expert yet) the '@' is an\ninvalid branch name and should therefore not collide with any current\nbranches. My idea was to disable the '--remote' option when the user\nhave explicitly set an invalid branch name to not modify any current\nbehaviour. Though having an explicit option is of course more\nclarifying. The current behaviour though is that empty branch name means\n\"follow master\" which is somewhat unintuitive.\n\n>> Signed-off-by: Anders Ro <anders.ronnbrant@gmail.com>\n>> ---\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index 25b1ddf..1bb2bb1 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -843,7 +843,8 @@ Maybe you want to use 'update --init'?\")\"\n>>   die \"$(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n>>   fi\n>>\n>> - if test -n \"$remote\"\n>> + # Fetch latest in remote unless branch name in config is '@'\n>> + if test -n \"$remote\" -a \"$branch\" != \"@\"\n> \n> The -a option to 'test' is not portable[1] and is considered obsolete\n> by POSIX[2]. Use \"test foo && test bar\" instead.\n> \n> [1]: http://www.gnu.org/software/autoconf/manual/autoconf.html#index-g_t_0040command_007btest_007d-1793\n> [2]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html\n> \n>>   then\n>>   if test -z \"$nofetch\"\n>>   then\n>> @@ -857,6 +858,12 @@ Maybe you want to use 'update --init'?\")\"\n>>   die \"$(eval_gettext \"Unable to find current ${remote_name}/${branch} revision in submodule path '\\$sm_path'\")\"\n>>   fi\n>>\n>> + # Inform that the current sm is pinned and use of '--remote' ignored\n>> + if test -n \"$remote\" -a \"$branch\" = \"@\"\n> \n> Ditto.\n> \n>> + then\n>> + say \"$(eval_gettext \"Submodule path '\\$displaypath' pinned, remote update ignored\")\"\n>> + fi\n>> +\n> \n>> Subject: [PATCH 2/2] t7412: add test case for pinned submodules\n>>\n>> Signed-off-by: Anders Ro <anders.ronnbrant@gmail.com>\n>> ---\n>> diff --git a/t/t7412-submodule-pinning.sh b/t/t7412-submodule-pinning.sh\n>> new file mode 100755\n>> index 0000000..2844b1e\n>> --- /dev/null\n>> +++ b/t/t7412-submodule-pinning.sh\n>> @@ -0,0 +1,73 @@\n>> +#!/bin/sh\n>> +#\n>> +# Copyright (c) 2015 Anders Ronnbrant\n>> +#\n>> +\n>> +test_description=\"Branch name '@' disables submodule update --remote calls\"\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +get_sha() {\n>> +  cd $1 && git rev-list --max-count=1 HEAD\n>> +}\n> \n> A few issues:\n> \n> Indent with tabs (width 8), not spaces. (This comment applies to the\n> entire patch).\n> \n> Style for shell scripts on this project is to have a space before\n> \"()\".\n> \n> Taking a hint from t/test-lib-functions.sh:test_cmp_rev(), use \"git\n> rev-parse --verify\" instead.\n> \n> It's a bit ugly that this does \"cd $1\" without ever balancing it with\n> a return to the original directory. If someone later comes along and\n> writes:\n> \n>     get_sha1 fiddle >fiddle-id\n> \n> in a new test, then that person will be surprised to find that the\n> current working directory changed out from under him. The current\n> patch doesn't experience this problem since it always invokes it as\n> $(get_sha1 fiddle), but it could be made more robust by either\n> wrapping it in a subshell so that the 'cd' is undone when the\n> subshell exits, or by using \"-C $1\".\n> \n> Taking the above comments into account:\n> \n>     get_sha () {\n>         git -C \"$1\" rev-parse --verify HEAD\n>     }\n> \n>> +equal_sha() {\n>> +  test \"$(get_sha $1)\" = \"$(get_sha $2)\"\n>> +}\n>> +\n>> +not_equal_sha() {\n>> +  test \"$(get_sha $1)\" != \"$(get_sha $2)\"\n>> +}\n> \n> Style: space before \"()\" on these two function declarations\n> \n>> +test_expect_success 'setup submodule tree structure' '\n>> +for i in 1 2 3; do echo file$i > file$i; git add file$i; git commit -m file$i; done &&\n> \n> Style: write each command of the 'for' loop on its own line,\n> including 'do', don't use semi-colons\n> \n> Style: no space after redirection operator: >file$i\n> \n> Keep the &&-chain intact, and end the chain with \"|| return $?\" so\n> that the 'for' loop correctly exits if any contained command fails.\n> \n>     for i in 1 2 3\n>     do\n>         echo file$i >file$i &&\n>         git add file$i &&\n>         git commit -m file$i || return $?\n>     done\n> \n>> +test_tick &&\n> \n> Why is this needed? There doesn't seem to be any obvious reason for\n> its presence, and the test still passes without it.\n> \n>> +git clone . super &&\n>> +git clone . follow &&\n>> +git clone . pinned &&\n>> +(cd super && git submodule add -b master ../follow follow) &&\n>> +(cd super && git submodule add           ../pinned pinned)\n> \n> Style: it's probably better to avoid aligning columns like this since\n> it can end up being a maintenance headache, and because the line with\n> the extensive blank span can be difficult to read in isolation.\n> \n> Since both of these execute in directory 'super', this could be\n> stated more concisely as:\n> \n>     (\n>     cd super &&\n>     git submodule add ... &&\n>     git submodule add ...\n>     ) &&\n> \n> Placement of the subshell parentheses is intentional.\n> \n>> +'\n>> +\n>> +test_expect_success 'verify submodules have the same SHA' '\n>> +equal_sha super/follow super/pinned\n>> +'\n> \n> Style: indent the test body (using tab); this comment applies to the\n> entire patch\n> \n>> +\n>> +\n> \n> Style: one blank line between tests, not two\n> \n>> +test_expect_success 'switch submodule pinned to HEAD~1 and commit super' '\n>> +(cd super/pinned && git checkout master && git reset --hard HEAD~1) &&\n>> +(cd super && git add pinned && git commit -m \"Submodule pinned @ HEAD~1\") &&\n>> +(cd super && git submodule status)\n>> +'\n> \n> Style: one command per line; format subshell as shown in above\n> example\n> \n> You can combine the two 'cd super' bits into a single subshell.\n> \n>> +\n>> +\n>> +test_expect_success 'verify submodules not have the same SHA anymore' '\n>> +not_equal_sha super/follow super/pinned\n>> +'\n> \n> Each test should be a logical unit. In prose, you might describe a\n> test as \"set condition A, make change B, and verify that A and B\n> resulted in some expected state\".\n> \n> This test is the \"verify A and B\" part, and the preceding test is the\n> \"set condition A, make change B\" part, however, splitting them up\n> like this makes it difficult for the reader to understand how they\n> are related. Worse, someone may come along and insert a new test\n> between the two, not realizing that these two tests are logically\n> one, possibly breaking them.\n> \n> Consequently, this test should be combined with the one preceding it.\n> \n>> +\n>> +\n>> +test_expect_success 'set branch name to \"@\" for submodule pinned' '\n>> +(cd super && git config --replace-all submodule.pinned.branch \"@\") &&\n>> +test \"$(cd super && git config --get submodule.pinned.branch)\" = \"@\"\n> \n> What is the last line testing? It appears to be testing the behavior\n> of git-config, which is outside the scope of this test script.\n> \n>> +'\n>> +\n>> +\n>> +test_expect_success 'run submodule update --remote and expect no change' '\n>> +(cd super && git submodule update --remote) &&\n>> +not_equal_sha super/follow super/pinned\n>> +'\n> \n> All the above comments apply here too. These two tests are logically\n> one, thus should be combined.\n> \n> Once combined, you can use test_config rather than git-config, since\n> test_config will ensure that the config setting is undone when the\n> test exits.\n> \n>> +\n>> +\n>> +test_expect_success 'remove branch name \"@\" for submodule pinned (unpin)' '\n>> +(cd super && git config --unset-all submodule.pinned.branch) &&\n> \n> If you take the above advice and use test_config in the previous\n> test, then this won't be needed.\n> \n>> +(cd super && git config --list)\n> \n> This isn't testing anything at all, but is instead merely cluttering\n> the output (when --verbose is enabled). It looks like debugging code\n> which should have been removed before committing.\n> \n>> +'\n>> +\n>> +\n>> +test_expect_success 'run submodule update --remote and expect same SHA1 again' '\n>> +(cd super && git submodule update --remote) &&\n> \n> For these one-line subshells, \"-C super\" is easier and reads\n> better. (This comment also applies to the rest of the patch.)\n> \n>> +equal_sha super/follow super/pinned\n>> +'\n> \n> This test should be combined with the previous one (although, if you\n> follow the advice regarding the previous test, then that test will\n> collapse to nothing...).\n> \n>> +\n>> +\n>> +test_done\n>> --\n>> 2.1.4\n"},{"id":"269548","messageId":"CAPig+cRwAjTF6_rT8+nhbsZTYcZqneWSZ_LTCo9z2m5+SEGwWg@mail.gmail.com","threadId":"40255","inReplyTo":"55ECB956.1080909@gmail.com","subject":"Re: [PATCH/RFC] Pinning of submodules","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-06T23:43:35Z","receivedAt":"2015-09-06T23:43:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 6, 2015 at 6:08 PM, Anders Ro <anders.ronnbrant@gmail.com> wrote:\n> On 04/09/15 07:02, Eric Sunshine wrote:\n>> On Wed, Sep 2, 2015 at 7:34 PM, Anders Ro <anders.ronnbrant@gmail.com> wrote:\n>>> git-submodule.sh: pin submodule when branch name is '@'\n>>>\n>>> Setting branch name to '@' for a submodule will disable 'git submodule\n>>> update --remote' calls for that specific submodule. I.e. instead of\n>>> follow the unspecified default choice of master, nothing is being\n>>> updated. This is useful when multiple submodules exist but not all\n>>> should follow the remote branch head.\n>>\n>> With the disclaimer that I'm not a submodule user (to which the\n>> answer might be obvious): What benefit is there in using a magic\n>> value like this (\"@\") over, say, an explicit configuration setting?\n>\n> From what I have understood (not a submodule expert yet) the '@' is an\n> invalid branch name and should therefore not collide with any current\n> branches. My idea was to disable the '--remote' option when the user\n> have explicitly set an invalid branch name to not modify any current\n> behaviour. Though having an explicit option is of course more\n> clarifying. The current behaviour though is that empty branch name means\n> \"follow master\" which is somewhat unintuitive.\n\nMy concern in asking was that some future person might come up with\nanother scenario which also wants to use a \"magic value\" and would\nhave to invent / arrive at another \"illegal\" representation. Hence, an\nexplicit setting might be more appropriate. However, as stated, I\ndon't even use submodules, so I may be far off the mark. I've cc'd a\nfew of the submodule maintainers with the hope that they will chime\nin.\n\n>>> +test_expect_success 'set branch name to \"@\" for submodule pinned' '\n>>> +(cd super && git config --replace-all submodule.pinned.branch \"@\") &&\n>>> +test \"$(cd super && git config --get submodule.pinned.branch)\" = \"@\"\n>>\n>> What is the last line testing? It appears to be testing the behavior\n>> of git-config, which is outside the scope of this test script.\n>>\n>> Once combined, you can use test_config rather than git-config, since\n>> test_config will ensure that the config setting is undone when the\n>> test exits.\n\nIn light of this recent thread[1] which shows that\ntest_when_finished() and, consequently, test_config() are\nnon-functional in subshells, I have to retract the advice to use\ntest_config() in this situation. Instead, at this time, it probably\nwould be best to continue using \"git-config\" as you do here...\n\n>>> +test_expect_success 'remove branch name \"@\" for submodule pinned (unpin)' '\n>>> +(cd super && git config --unset-all submodule.pinned.branch) &&\n>>\n>> If you take the above advice and use test_config in the previous\n>> test, then this won't be needed.\n\nand \"git-config --unset-all\" here.\n\nLater, once [1] has landed in \"master\" (or perhaps \"next\"), this can\nbe revisited and updated to use \"test_config -C\"[2].\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/277323/focus=277370\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/277323/focus=277372\n"},{"id":"269581","messageId":"55EDEFDC.3040605@web.de","threadId":"40255","inReplyTo":"CAPig+cRwAjTF6_rT8+nhbsZTYcZqneWSZ_LTCo9z2m5+SEGwWg@mail.gmail.com","subject":"Re: [PATCH/RFC] Pinning of submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2015-09-07T20:13:16Z","receivedAt":"2015-09-07T20:13:16Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 07.09.2015 um 01:43 schrieb Eric Sunshine:\n> On Sun, Sep 6, 2015 at 6:08 PM, Anders Ro <anders.ronnbrant@gmail.com> wrote:\n>> On 04/09/15 07:02, Eric Sunshine wrote:\n>>> On Wed, Sep 2, 2015 at 7:34 PM, Anders Ro <anders.ronnbrant@gmail.com> wrote:\n>>>> git-submodule.sh: pin submodule when branch name is '@'\n>>>>\n>>>> Setting branch name to '@' for a submodule will disable 'git submodule\n>>>> update --remote' calls for that specific submodule. I.e. instead of\n>>>> follow the unspecified default choice of master, nothing is being\n>>>> updated. This is useful when multiple submodules exist but not all\n>>>> should follow the remote branch head.\n>>>\n>>> With the disclaimer that I'm not a submodule user (to which the\n>>> answer might be obvious): What benefit is there in using a magic\n>>> value like this (\"@\") over, say, an explicit configuration setting?\n>>\n>>  From what I have understood (not a submodule expert yet) the '@' is an\n>> invalid branch name and should therefore not collide with any current\n>> branches. My idea was to disable the '--remote' option when the user\n>> have explicitly set an invalid branch name to not modify any current\n>> behaviour. Though having an explicit option is of course more\n>> clarifying. The current behaviour though is that empty branch name means\n>> \"follow master\" which is somewhat unintuitive.\n>\n> My concern in asking was that some future person might come up with\n> another scenario which also wants to use a \"magic value\" and would\n> have to invent / arrive at another \"illegal\" representation. Hence, an\n> explicit setting might be more appropriate. However, as stated, I\n> don't even use submodules, so I may be far off the mark. I've cc'd a\n> few of the submodule maintainers with the hope that they will chime\n> in.\n\nAdded Trevor to the CC, who is the original author of --remote (see\n06b1abb5b).\n\nWhile I believe that adding such functionality makes perfect sense,\nI do not find it terribly obvious that setting the branch to '@' will\nmake --remote skip this submodule. I wouldn't care so much if we'd\nonly use this value internally, but this is user visible (and has to\nbe set by the user if she wants to skip a submodule in --remote).\n\nSetting the branch to an empty value feels a bit more natural, but\nI'm not so sure our config handling supports that well (we seem to\nassume in quite some places that empty equals unset). So I tend to\nprefer a new option for that.\n"},{"id":"269584","messageId":"55EE1D75.1030109@gmail.com","threadId":"40255","inReplyTo":"55EDEFDC.3040605@web.de","subject":"Re: [PATCH/RFC] Pinning of submodules","fromName":"Anders Ro","fromEmail":"anders.ronnbrant@gmail.com","sentAt":"2015-09-07T23:27:49Z","receivedAt":"2015-09-07T23:27:49Z","isPatch":true,"sender":{"key":"anders.ronnbrant@gmail.com","avatar":"https://gravatar.com/avatar/b0f21dd0e596dc35bdf2dc9b1df6d46e0069c96651eb4542056983f3520a4478?d=mp&s=160"},"body":"On 07/09/15 22:13, Jens Lehmann wrote:\n> Am 07.09.2015 um 01:43 schrieb Eric Sunshine:\n>> On Sun, Sep 6, 2015 at 6:08 PM, Anders Ro <anders.ronnbrant@gmail.com>\n>> wrote:\n>>> On 04/09/15 07:02, Eric Sunshine wrote:\n>>>> On Wed, Sep 2, 2015 at 7:34 PM, Anders Ro\n>>>> <anders.ronnbrant@gmail.com> wrote:\n>>>>> git-submodule.sh: pin submodule when branch name is '@'\n>>>>>\n>>>>> Setting branch name to '@' for a submodule will disable 'git submodule\n>>>>> update --remote' calls for that specific submodule. I.e. instead of\n>>>>> follow the unspecified default choice of master, nothing is being\n>>>>> updated. This is useful when multiple submodules exist but not all\n>>>>> should follow the remote branch head.\n>>>>\n>>>> With the disclaimer that I'm not a submodule user (to which the\n>>>> answer might be obvious): What benefit is there in using a magic\n>>>> value like this (\"@\") over, say, an explicit configuration setting?\n>>>\n>>>  From what I have understood (not a submodule expert yet) the '@' is an\n>>> invalid branch name and should therefore not collide with any current\n>>> branches. My idea was to disable the '--remote' option when the user\n>>> have explicitly set an invalid branch name to not modify any current\n>>> behaviour. Though having an explicit option is of course more\n>>> clarifying. The current behaviour though is that empty branch name means\n>>> \"follow master\" which is somewhat unintuitive.\n>>\n>> My concern in asking was that some future person might come up with\n>> another scenario which also wants to use a \"magic value\" and would\n>> have to invent / arrive at another \"illegal\" representation. Hence, an\n>> explicit setting might be more appropriate. However, as stated, I\n>> don't even use submodules, so I may be far off the mark. I've cc'd a\n>> few of the submodule maintainers with the hope that they will chime\n>> in.\n\nAgree this is not a \"future proof\" solution. Though faily a quick one. I\nstarted with a setting but realized it involved a bit more changes since\nyou should be able to tell the submodule command to pin a submodule\nright from the start etc.\n\n> \n> Added Trevor to the CC, who is the original author of --remote (see\n> 06b1abb5b).\n> \n> While I believe that adding such functionality makes perfect sense,\n> I do not find it terribly obvious that setting the branch to '@' will\n> make --remote skip this submodule. I wouldn't care so much if we'd\n> only use this value internally, but this is user visible (and has to\n> be set by the user if she wants to skip a submodule in --remote).\n> \n> Setting the branch to an empty value feels a bit more natural, but\n> I'm not so sure our config handling supports that well (we seem to\n> assume in quite some places that empty equals unset). So I tend to\n> prefer a new option for that.\n\nI started out that way, thinking that would solve my initial problem of\nhaving 4 submodules which should follow branch 'develop' and one\nsubmodule that should stay put until explicitly changed. Instead having\nempty an branch name gave me 'master' on that module, which was way off\ncurrent status of the code to say the least. This was all executed by\nJenkins which either gives you \"update remote branches\" or \"not update\nremote branches\". Thus no other choice than to change the code.\n\nThe current 'git submodule add' takes option '-b' as branch and last\ntime I tried providing '@' as branch name it did not work. That\nindicates that there should be a '-p' option for pinning the submodule\nfrom the start. Having to fiddle with the .gitmodules file after the\nfact is not really user friendly (though it have worked for me for a\nwhile). I will have a go at an explicit setting in a week or two and get\nback for comments on that. Perhaps some maintainers can give some hints\non adding a new option etc meanwhile. How about option '-p, ---pinned'\nand setting 'submodule.<branch>.pinned = true|false'?\n\nCleaned-up the test code according to Eric's comments, looks a bit more\nstylish actually :).\n"},{"id":"269609","messageId":"xmqq37yoam0a.fsf@gitster.mtv.corp.google.com","threadId":"40255","inReplyTo":"55EDEFDC.3040605@web.de","subject":"Re: [PATCH/RFC] Pinning of submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-08T16:55:49Z","receivedAt":"2015-09-08T16:55:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> Am 07.09.2015 um 01:43 schrieb Eric Sunshine:\n>\n>> My concern in asking was that some future person might come up with\n>> another scenario which also wants to use a \"magic value\" and would\n>> have to invent / arrive at another \"illegal\" representation. Hence, an\n>> explicit setting might be more appropriate. However, as stated, I\n>> don't even use submodules, so I may be far off the mark. I've cc'd a\n>> few of the submodule maintainers with the hope that they will chime\n>> in.\n>\n> Added Trevor to the CC, who is the original author of --remote (see\n> 06b1abb5b).\n>\n> While I believe that adding such functionality makes perfect sense,\n> I do not find it terribly obvious that setting the branch to '@' will\n> make --remote skip this submodule. I wouldn't care so much if we'd\n> only use this value internally, but this is user visible (and has to\n> be set by the user if she wants to skip a submodule in --remote).\n>\n> Setting the branch to an empty value feels a bit more natural, but\n> I'm not so sure our config handling supports that well (we seem to\n> assume in quite some places that empty equals unset). So I tend to\n> prefer a new option for that.\n\nAs I stare at both the code change and log message of 06b1abb5\n(submodule update: add --remote for submodule's upstream changes,\n2012-12-19), I cannot shake this feeling that the change to default\nsubmodule.$name.branch to 'master' is doubly misdesigned.\n\nThe stated goal of users of --remote is to declare \"This submodule\ndoes not care what the top-level integrator happened to have seen at\nthe tip when the integration of the history of submodule.  It always\nfollows the tip of a specific branch at the upstream.\"\n\nIf we were to use any default, the only justification would be \"the\nusers of --remote would want this mode of update to happen for all\nsubmodules, and having to specify which specific branch to be\nfollowed is too cumbersome\".  But if that is the case, defaulting to\n'master' does not make any sense---if it defaulted to 'HEAD' for\neach submodule, it may have made some sense, as that is the usual\nconvention to denote which branch is the default branch in a\nrepository.\n\nAlso Anders' proposal refutes that \"when --remote is used, all\nsubmodules should behave that way\" assumption.\n\nI wonder if it is a sensible way forward to introduce a new\nconfiguration variable 'submodule.remoteFallBackBranch' so that the\nusers can customize this line (near l.800 in git-submodule.sh)\n\n     branch=$(get_submodule_config \"$name\" branch master)\n\nThe possible values for that new configuration value would be:\n\n   - an empty string: disable \"update --remote\" for any submodule\n     '$name' for which submodule.$name.branch is not set.\n\n   - 'master': behave the same way as the current code; we can make\n     this the default, when submodule.remoteFallBackBranch is unset,\n     to ease transition.\n\n   - any user-chosen branch name. On notable example may be 'HEAD',\n     which 06b1abb5 (submodule update: add --remote for submodule's\n     upstream changes, 2012-12-19) should have chosen as the\n     default.\n\nBut I am not the target audience of \"update --remote\", so let's hear\nwhat the real-world use cases are.\n"}]}