{"thread":{"id":"42950","subject":"[PATCH 0/2] submodule update: allow '.' for branch value","startedAt":"2016-07-28T17:26:53Z","lastAt":"2016-07-28T20:02:43Z","messageCount":11,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"292431","messageId":"20160728172641.8376-1-sbeller@google.com","threadId":"42950","inReplyTo":null,"subject":"[PATCH 0/2] submodule update: allow '.' for branch value","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-28T17:26:39Z","receivedAt":"2016-07-28T17:26:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The meat is in patch 2 and helps Git and Gerrit work well together.\n\npatch 1 looks unrelated but it is not, as when left out the broken test, breaks\nwith patch 2. This is because we add more commits in the submodule.\n\nThanks,\nStefan\n\nStefan Beller (2):\n  t7406: correct depth test in shallow test\n  submodule update: allow '.' for branch value\n\n git-submodule.sh            |  7 +++++++\n t/t7406-submodule-update.sh | 37 ++++++++++++++++++++++++++++++++++---\n 2 files changed, 41 insertions(+), 3 deletions(-)\n\n-- \n2.9.2.466.g8c6d1f9.dirty\n\n"},{"id":"292432","messageId":"20160728172641.8376-2-sbeller@google.com","threadId":"42950","inReplyTo":"20160728172641.8376-1-sbeller@google.com","subject":"[PATCH 1/2] t7406: correct depth test in shallow test","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-28T17:26:40Z","receivedAt":"2016-07-28T17:26:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We used to ask for 3 changes and tested for having 1, so the test\nseems broken.\n\nCorrect the test by using test_line_count that exists in the test suite.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t7406-submodule-update.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 88e9750..bd261ac 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -846,9 +846,10 @@ test_expect_success 'submodule update clone shallow submodule' '\n \t(cd super3 &&\n \t sed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n \t mv -f .gitmodules.tmp .gitmodules &&\n-\t git submodule update --init --depth=3\n+\t git submodule update --init --depth=2\n \t (cd submodule &&\n-\t  test 1 = $(git log --oneline | wc -l)\n+\t  git log --oneline >lines\n+\t  test_line_count = 2 lines\n \t )\n )\n '\n-- \n2.9.2.466.g8c6d1f9.dirty\n\n"},{"id":"292433","messageId":"20160728172641.8376-3-sbeller@google.com","threadId":"42950","inReplyTo":"20160728172641.8376-1-sbeller@google.com","subject":"[PATCH 2/2] submodule update: allow '.' for branch value","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-28T17:26:41Z","receivedAt":"2016-07-28T17:27:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Gerrit has a \"superproject subscription\" feature[1], that triggers a\ncommit in a superproject that is subscribed to its submodules.\nConceptually this Gerrit feature can be done on the client side with\nGit via:\n\n    git -C <superproject> submodule update --remote --force\n    git -C <superproject> commit -a -m \"Update submodules\"\n    git -C <superproject> push\n\nfor each branch in the superproject. To ease the configuration in Gerrit\na special value of \".\" has been introduced for the submodule.<name>.branch\nto mean the same branch as the superproject[2], such that you can create a\nnew branch on both superproject and the submodule and this feature\ncontinues to work on that new branch.\n\nNow we have find projects in the wild with such a .gitmodules file.\nTo have Git working well with these, we imitate the behavior and\nlook up the superprojects branch name if the submodules branch is\nconfigured to \".\". In projects that do not use Gerrit, this value\nwhould be never configured as \".\" is not a valid branch name.\n\n[1] introduced around 2012-01, e.g.\n    https://gerrit-review.googlesource.com/#/c/30810/\n[2] excerpt from [1]:\n > The branch value could be '.' if the submodule project branch\n > has the same name as the destination branch of the commit having\n > gitlinks/.gitmodules file.\n\nCC: Avery Pennarun <apenwarr@gmail.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n git-submodule.sh            |  7 +++++++\n t/t7406-submodule-update.sh | 32 +++++++++++++++++++++++++++++++-\n 2 files changed, 38 insertions(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 4ec7546..12bb9e8 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -591,6 +591,13 @@ cmd_update()\n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tbranch=$(get_submodule_config \"$name\" branch master)\n+\t\tif test \"$branch\" = \".\"\n+\t\tthen\n+\t\t\tif ! branch=$(git symbolic-ref --short -q HEAD)\n+\t\t\tthen\n+\t\t\t\tdie \"$(eval_gettext \"submodule branch configured to inherit branch from superproject, but it's not on any branch\")\"\n+\t\t\tfi\n+\t\tfi\n \t\tif ! test -z \"$update\"\n \t\tthen\n \t\t\tupdate_module=$update\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex bd261ac..41d69e4 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -209,9 +209,39 @@ test_expect_success 'submodule update --remote should fetch upstream changes' '\n \t)\n '\n \n-test_expect_success 'local config should override .gitmodules branch' '\n+test_expect_success 'submodule update --remote should fetch upstream changes with .' '\n+\t(cd super &&\n+\t git config -f .gitmodules submodule.\"submodule\".branch \".\" &&\n+\t git add .gitmodules &&\n+\t git commit -m \"submodules: update from the respective superproject branch\"\n+\t) &&\n \t(cd submodule &&\n+\t echo line4a >> file &&\n+\t git add file &&\n+\t test_tick &&\n+\t git commit -m \"upstream line4a\" &&\n \t git checkout -b test-branch &&\n+\t test_commit on-test-branch\n+\t) &&\n+\t(cd super &&\n+\t git submodule update --remote --force submodule &&\n+\t (cd submodule &&\n+\t  test \"$(git log -1 --oneline)\" = \"$(GIT_DIR=../../submodule/.git git log -1 --oneline master)\"\n+\t ) &&\n+\t git checkout -b test-branch &&\n+\t git submodule update --remote --force submodule &&\n+\t (cd submodule &&\n+\t  test \"$(git log -1 --oneline)\" = \"$(GIT_DIR=../../submodule/.git git log -1 --oneline test-branch)\"\n+\t ) &&\n+\t git checkout master &&\n+\t git branch -d test-branch &&\n+\t git revert HEAD\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 \t echo line5 >> file &&\n \t git add file &&\n \t test_tick &&\n-- \n2.9.2.466.g8c6d1f9.dirty\n\n"},{"id":"292436","messageId":"20160728182132.25088-1-sbeller@google.com","threadId":"42950","inReplyTo":"20160728172641.8376-3-sbeller@google.com","subject":"[PATCHv2 2/2] submodule update: allow '.' for branch value","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-28T18:21:32Z","receivedAt":"2016-07-28T18:21:41Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Gerrit has a \"superproject subscription\" feature[1], that triggers a\ncommit in a superproject that is subscribed to its submodules.\nConceptually this Gerrit feature can be done on the client side with\nGit via:\n\n    git -C <superproject> submodule update --remote --force\n    git -C <superproject> commit -a -m \"Update submodules\"\n    git -C <superproject> push\n\nfor each branch in the superproject. To ease the configuration in Gerrit\na special value of \".\" has been introduced for the submodule.<name>.branch\nto mean the same branch as the superproject[2], such that you can create a\nnew branch on both superproject and the submodule and this feature\ncontinues to work on that new branch.\n\nNow we have find projects in the wild with such a .gitmodules file.\nTo have Git working well with these, we imitate the behavior and\nlook up the superprojects branch name if the submodules branch is\nconfigured to \".\". In projects that do not use Gerrit, this value\nwhould be never configured as \".\" is not a valid branch name.\n\n[1] introduced around 2012-01, e.g.\n    https://gerrit-review.googlesource.com/#/c/30810/\n[2] excerpt from [1]:\n > The branch value could be '.' if the submodule project branch\n > has the same name as the destination branch of the commit having\n > gitlinks/.gitmodules file.\n\nCC: Avery Pennarun <apenwarr@gmail.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n This comes with another test that I run into while using this code.\n Please replace patch 2 with this v2.\n \n Thanks,\n Stefan\n\n git-submodule.sh            |  9 ++++++++-\n t/t7406-submodule-update.sh | 42 +++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 49 insertions(+), 2 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 4ec7546..1eb33ad 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -590,7 +590,6 @@ cmd_update()\n \n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n-\t\tbranch=$(get_submodule_config \"$name\" branch master)\n \t\tif ! test -z \"$update\"\n \t\tthen\n \t\t\tupdate_module=$update\n@@ -616,6 +615,14 @@ cmd_update()\n \n \t\tif test -n \"$remote\"\n \t\tthen\n+\t\t\tbranch=$(get_submodule_config \"$name\" branch master)\n+\t\t\tif test \"$branch\" = \".\"\n+\t\t\tthen\n+\t\t\t\tif ! branch=$(git symbolic-ref --short -q HEAD)\n+\t\t\t\tthen\n+\t\t\t\t\tdie \"$(eval_gettext \"submodule branch configured to inherit branch from superproject, but it's not on any branch\")\"\n+\t\t\t\tfi\n+\t\t\tfi\n \t\t\tif test -z \"$nofetch\"\n \t\t\tthen\n \t\t\t\t# Fetch remote before determining tracking $sha1\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex bd261ac..953c486 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -209,9 +209,49 @@ test_expect_success 'submodule update --remote should fetch upstream changes' '\n \t)\n '\n \n-test_expect_success 'local config should override .gitmodules branch' '\n+test_expect_success 'submodule update --remote should fetch upstream changes with .' '\n+\t(cd super &&\n+\t git config -f .gitmodules submodule.\"submodule\".branch \".\" &&\n+\t git add .gitmodules &&\n+\t git commit -m \"submodules: update from the respective superproject branch\"\n+\t) &&\n \t(cd submodule &&\n+\t echo line4a >> file &&\n+\t git add file &&\n+\t test_tick &&\n+\t git commit -m \"upstream line4a\" &&\n+\t git checkout -b test-branch &&\n+\t test_commit on-test-branch\n+\t) &&\n+\t(cd super &&\n+\t git submodule update --remote --force submodule &&\n+\t (cd submodule &&\n+\t  test \"$(git log -1 --oneline)\" = \"$(GIT_DIR=../../submodule/.git git log -1 --oneline master)\"\n+\t ) &&\n \t git checkout -b test-branch &&\n+\t git submodule update --remote --force submodule &&\n+\t (cd submodule &&\n+\t  test \"$(git log -1 --oneline)\" = \"$(GIT_DIR=../../submodule/.git git log -1 --oneline test-branch)\"\n+\t ) &&\n+\t git checkout master &&\n+\t git branch -d test-branch\n+\t)\n+'\n+\n+test_expect_success 'branch = . does not confuse the rest of update' '\n+\t(cd super &&\n+\t git checkout --detach &&\n+\t # update is not confused by branch=\".\" even if the the superproject\n+\t # is not on any branch currently\n+\t git submodule update &&\n+\t git revert HEAD &&\n+\t git checkout master\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 \t echo line5 >> file &&\n \t git add file &&\n \t test_tick &&\n-- \n2.9.2.468.g3d9025b\n\n"},{"id":"292438","messageId":"xmqqeg6dsj33.fsf@gitster.mtv.corp.google.com","threadId":"42950","inReplyTo":"20160728172641.8376-2-sbeller@google.com","subject":"Re: [PATCH 1/2] t7406: correct depth test in shallow test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-28T18:39:28Z","receivedAt":"2016-07-28T18:39:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> We used to ask for 3 changes and tested for having 1, so the test\n> seems broken.\n\nI am not sure what to think of \"seems broken\".\n\nAsking for 3 and having 1 is broken in what way?  Should we be\nexpecting for 3 because we asked for that many?  Should we expect\nless than three even though we asked for three because the upstream\nside does not even have that many?  If the current test that asks\nfor 3 and gets only 1 is not failing, why should we expect that\nasking for 2 would get 2?  In other words, why is it sane that\nasking for fewer number of commits gives us more?\n\nAlso most of the lines in this subshell seem to be breaking\n&&-chain.\n\n\n\n> Correct the test by using test_line_count that exists in the test suite.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  t/t7406-submodule-update.sh | 5 +++--\n>  1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index 88e9750..bd261ac 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -846,9 +846,10 @@ test_expect_success 'submodule update clone shallow submodule' '\n>  \t(cd super3 &&\n>  \t sed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n>  \t mv -f .gitmodules.tmp .gitmodules &&\n> -\t git submodule update --init --depth=3\n> +\t git submodule update --init --depth=2\n>  \t (cd submodule &&\n> -\t  test 1 = $(git log --oneline | wc -l)\n> +\t  git log --oneline >lines\n> +\t  test_line_count = 2 lines\n>  \t )\n>  )\n>  '\n"},{"id":"292440","messageId":"CAGZ79ka5n1_5CM3DOGHcd3W=4KJb9d-sb=E1pyvzP=XWGVXE3g@mail.gmail.com","threadId":"42950","inReplyTo":"xmqqeg6dsj33.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] t7406: correct depth test in shallow test","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-28T18:47:12Z","receivedAt":"2016-07-28T18:47:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 28, 2016 at 11:39 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> We used to ask for 3 changes and tested for having 1, so the test\n>> seems broken.\n>\n> I am not sure what to think of \"seems broken\".\n\nWhen asking for depth 3, I would expect a result of 1,2, or 3 commits.\n\nBut when testing the depth argument with a history less than 3, and then\nimplying: \"I got 1, which is less than 3, so the depth works\", seems\nto be a logical shortcut to me.\n\nI would have expected a history of >3, then ask for 3 and confirm we did not\nget 4 or 5 or 6, but 3 only.\n\n>\n> Asking for 3 and having 1 is broken in what way?  Should we be\n> expecting for 3 because we asked for that many?  Should we expect\n> less than three even though we asked for three because the upstream\n> side does not even have that many?  If the current test that asks\n> for 3 and gets only 1 is not failing, why should we expect that\n> asking for 2 would get 2?  In other words, why is it sane that\n> asking for fewer number of commits gives us more?\n\nI think there is a subtle thing going on, that I did not examine properly but\nit is hidden in the modernization from\n\n    test 1 = $(something)\n to test_line_count = 2\n\nI'll investigate the actual reason.\n\n>\n> Also most of the lines in this subshell seem to be breaking\n> &&-chain.\n\nThanks for pointing that out, will fix while at it.\n\n>\n>\n>\n>> Correct the test by using test_line_count that exists in the test suite.\n>>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>>  t/t7406-submodule-update.sh | 5 +++--\n>>  1 file changed, 3 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n>> index 88e9750..bd261ac 100755\n>> --- a/t/t7406-submodule-update.sh\n>> +++ b/t/t7406-submodule-update.sh\n>> @@ -846,9 +846,10 @@ test_expect_success 'submodule update clone shallow submodule' '\n>>       (cd super3 &&\n>>        sed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n>>        mv -f .gitmodules.tmp .gitmodules &&\n>> -      git submodule update --init --depth=3\n>> +      git submodule update --init --depth=2\n>>        (cd submodule &&\n>> -       test 1 = $(git log --oneline | wc -l)\n>> +       git log --oneline >lines\n>> +       test_line_count = 2 lines\n>>        )\n>>  )\n>>  '\n"},{"id":"292442","messageId":"xmqq7fc5shnl.fsf@gitster.mtv.corp.google.com","threadId":"42950","inReplyTo":"20160728182132.25088-1-sbeller@google.com","subject":"Re: [PATCHv2 2/2] submodule update: allow '.' for branch value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-28T19:10:22Z","receivedAt":"2016-07-28T19:10:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Gerrit has a \"superproject subscription\" feature[1], that triggers a\n> commit in a superproject that is subscribed to its submodules.\n> Conceptually this Gerrit feature can be done on the client side with\n> Git via:\n>\n>     git -C <superproject> submodule update --remote --force\n>     git -C <superproject> commit -a -m \"Update submodules\"\n>     git -C <superproject> push\n>\n> for each branch in the superproject.\n\nWhich I imagine would be useful if you only have one submodule.  If\nyou work on submodules A and B and then want to give the updated\nsuperproject that binds the latest in both A and B as an atomic\nupdate, the three lines above would not quite work, no?\n\n> To ease the configuration in Gerrit\n> a special value of \".\" has been introduced for the submodule.<name>.branch\n> to mean the same branch as the superproject[2], such that you can create a\n> new branch on both superproject and the submodule and this feature\n> continues to work on that new branch.\n>\n> Now we have find projects in the wild with such a .gitmodules file.\n\nThat's annoying.\n\n> To have Git working well with these, we imitate the behavior and\n> look up the superprojects branch name if the submodules branch is\n> configured to \".\". In projects that do not use Gerrit, this value\n> whould be never configured as \".\" is not a valid branch name.\n\nI find that the last sentence is somewhat misleading.  I agree it is\njustifiable that using \".\" as the name to trigger a new (to us)\nfeature is safe, because such a setting wouldn't have meant anything\nuseful without this change, but I initially misread it and thought\nyou added \"are we using Gerrit?  Error out if we are not\" logic,\nwhich is not the case here.\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 4ec7546..1eb33ad 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -590,7 +590,6 @@ cmd_update()\n>  \n>  \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n>  \t\turl=$(git config submodule.\"$name\".url)\n> -\t\tbranch=$(get_submodule_config \"$name\" branch master)\n>  \t\tif ! test -z \"$update\"\n>  \t\tthen\n>  \t\t\tupdate_module=$update\n> @@ -616,6 +615,14 @@ cmd_update()\n>  \n>  \t\tif test -n \"$remote\"\n>  \t\tthen\n> +\t\t\tbranch=$(get_submodule_config \"$name\" branch master)\n> +\t\t\tif test \"$branch\" = \".\"\n> +\t\t\tthen\n> +\t\t\t\tif ! branch=$(git symbolic-ref --short -q HEAD)\n> +\t\t\t\tthen\n> +\t\t\t\t\tdie \"$(eval_gettext \"submodule branch configured to inherit branch from superproject, but it's not on any branch\")\"\n> +\t\t\t\tfi\n> +\t\t\tfi\n\nI see that you narrowed the scope of \"$branch\" (which is only used\nwhen $remote exists), but it is a bit annoying to see that change\nmixed with \"now a dot means something different\" change.\n\nI wonder if the above 8-line block wants to be encapsulated to\nbecome a part of \"git submodule--helper\" interface, though.  IOW,\nbranch=$(git submodule--helper branch \"$name\") or something?\n\n> +test_expect_success 'submodule update --remote should fetch upstream changes with .' '\n> +\t(cd super &&\n> +\t git config -f .gitmodules submodule.\"submodule\".branch \".\" &&\n> +\t git add .gitmodules &&\n> +\t git commit -m \"submodules: update from the respective superproject branch\"\n> +\t) &&\n>  \t(cd submodule &&\n> +\t echo line4a >> file &&\n> +\t git add file &&\n> +\t test_tick &&\n> +\t git commit -m \"upstream line4a\" &&\n> +\t git checkout -b test-branch &&\n> +\t test_commit on-test-branch\n> +\t) &&\n> +\t(cd super &&\n> +\t git submodule update --remote --force submodule &&\n> +\t (cd submodule &&\n> +\t  test \"$(git log -1 --oneline)\" = \"$(GIT_DIR=../../submodule/.git git log -1 --oneline master)\"\n\nA few issues:\n\n  * A crash in \"git log\" would not be noticed with this.  Perhaps\n\n\tgit log -1 --oneline $one_way_to_invoke >expect &&\n        git log -1 --oneline $the_other_way >actual &&\n        test_cmp expect actual\n\n    would be better?\n\n  * What exactly is this testing?  The current branch (in submodule)\n    pointing at the same commit as the tip of 'master'?  Or the\n    current branch _is_ 'master'?\n\n  * What exactly is the reason why one has GIT_DIR=... and the other\n    does not?  I do not think this a place to test that \"gitdir: \"\n    in .git points at the right place, so it must be testing\n    something else, but I cannot guess.\n\n> +\t ) &&\n>  \t git checkout -b test-branch &&\n> +\t git submodule update --remote --force submodule &&\n> +\t (cd submodule &&\n> +\t  test \"$(git log -1 --oneline)\" = \"$(GIT_DIR=../../submodule/.git git log -1 --oneline test-branch)\"\n> +\t ) &&\n> +\t git checkout master &&\n> +\t git branch -d test-branch\n> +\t)\n> +'\n> +\n> +test_expect_success 'branch = . does not confuse the rest of update' '\n> +\t(cd super &&\n> +\t git checkout --detach &&\n> +\t # update is not confused by branch=\".\" even if the the superproject\n> +\t # is not on any branch currently\n> +\t git submodule update &&\n> +\t git revert HEAD &&\n\n\"revert\" is rather unusual thing to see in the test.  Also I am not\nsure why cmd_update that now has an explicit check to die when\nbranch is set to \".\" and the head is detached is expected \"not\" to\nbe confused.  Perhaps I misread the main part of the patch?\n\nPuzzled.\n\n> +\t git checkout master\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>  \t echo line5 >> file &&\n>  \t git add file &&\n>  \t test_tick &&\n"},{"id":"292443","messageId":"CAGZ79kZfXKC66uokqOXeWND=V+53YfAkfd1NUK7A4jM2UAea7A@mail.gmail.com","threadId":"42950","inReplyTo":"CAGZ79ka5n1_5CM3DOGHcd3W=4KJb9d-sb=E1pyvzP=XWGVXE3g@mail.gmail.com","subject":"Re: [PATCH 1/2] t7406: correct depth test in shallow test","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-28T19:24:03Z","receivedAt":"2016-07-28T19:24:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 28, 2016 at 11:47 AM, Stefan Beller <sbeller@google.com> wrote:\n> On Thu, Jul 28, 2016 at 11:39 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> We used to ask for 3 changes and tested for having 1, so the test\n>>> seems broken.\n>>\n>> I am not sure what to think of \"seems broken\".\n>\n> When asking for depth 3, I would expect a result of 1,2, or 3 commits.\n>\n> But when testing the depth argument with a history less than 3, and then\n> implying: \"I got 1, which is less than 3, so the depth works\", seems\n> to be a logical shortcut to me.\n>\n> I would have expected a history of >3, then ask for 3 and confirm we did not\n> get 4 or 5 or 6, but 3 only.\n>\n>>\n>> Asking for 3 and having 1 is broken in what way?  Should we be\n>> expecting for 3 because we asked for that many?  Should we expect\n>> less than three even though we asked for three because the upstream\n>> side does not even have that many?  If the current test that asks\n>> for 3 and gets only 1 is not failing, why should we expect that\n>> asking for 2 would get 2?  In other words, why is it sane that\n>> asking for fewer number of commits gives us more?\n>\n> I think there is a subtle thing going on, that I did not examine properly but\n> it is hidden in the modernization from\n>\n>     test 1 = $(something)\n>  to test_line_count = 2\n>\n> I'll investigate the actual reason.\n\nSo when I place a test_pause just before that check for the lines\nwe have the upstream submodule:\n$ git log --oneline\n6355002 upstream line4\n820877d upstream line3\n4301fd3 Commit 2\n0c90624 upstream\n\nwhich then allows fetching\n6355002 upstream line4\n820877d upstream line3\n4301fd3 Commit 2\n\nand \"Commit 2\" is recorded as the sha1, i.e.\nwhen checking out \"Commit 2\" and running\n(git log --oneline | wc -l) we get 1 as it cuts\noff after that.\n\nWhen adding a test (in the next patch) that adds\nmore commits to the submodule upstream, we\nwill fetch with depth 3 but will no longer see the sha1,\nso we try a different approach. Current approach:\ntry fetching again with no depth, and then try again with sha1\ngiven.\n\nSo one could say there is a bug as the fetching should\nuse the depth argument as well.\n\nThe depth of 2 I chose originally turns out to be a lucky\naccident too, as the depth from \"Commit 2\" is 2,\nso that we would observe the same depth no matter if\na --depth 2 was given and working or not.\n\nI'll redo this test (as 2 tests, one is testing the situation as\nwe have now, i.e. the desired tip is reachable from within\nthe depth argument, the second test will test if it is not\nreachable.)\n\n>\n>>\n>> Also most of the lines in this subshell seem to be breaking\n>> &&-chain.\n>\n> Thanks for pointing that out, will fix while at it.\n>\n>>\n>>\n>>\n>>> Correct the test by using test_line_count that exists in the test suite.\n>>>\n>>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>>> ---\n>>>  t/t7406-submodule-update.sh | 5 +++--\n>>>  1 file changed, 3 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n>>> index 88e9750..bd261ac 100755\n>>> --- a/t/t7406-submodule-update.sh\n>>> +++ b/t/t7406-submodule-update.sh\n>>> @@ -846,9 +846,10 @@ test_expect_success 'submodule update clone shallow submodule' '\n>>>       (cd super3 &&\n>>>        sed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n>>>        mv -f .gitmodules.tmp .gitmodules &&\n>>> -      git submodule update --init --depth=3\n>>> +      git submodule update --init --depth=2\n>>>        (cd submodule &&\n>>> -       test 1 = $(git log --oneline | wc -l)\n>>> +       git log --oneline >lines\n>>> +       test_line_count = 2 lines\n>>>        )\n>>>  )\n>>>  '\n"},{"id":"292445","messageId":"CAGZ79kactn4VZ5i+fCrBGa77dzFiTngAgVU3R1orz2=EMAZ1Jg@mail.gmail.com","threadId":"42950","inReplyTo":"xmqq7fc5shnl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2 2/2] submodule update: allow '.' for branch value","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-28T19:44:28Z","receivedAt":"2016-07-28T19:44:35Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 28, 2016 at 12:10 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Gerrit has a \"superproject subscription\" feature[1], that triggers a\n>> commit in a superproject that is subscribed to its submodules.\n>> Conceptually this Gerrit feature can be done on the client side with\n>> Git via:\n>>\n>>     git -C <superproject> submodule update --remote --force\n>>     git -C <superproject> commit -a -m \"Update submodules\"\n>>     git -C <superproject> push\n>>\n>> for each branch in the superproject.\n>\n> Which I imagine would be useful if you only have one submodule.  If\n> you work on submodules A and B and then want to give the updated\n> superproject that binds the latest in both A and B as an atomic\n> update, the three lines above would not quite work, no?\n\nWhen using Gerrit you can submit A,B together bound by a \"topic\"\nand that will trigger a superproject update that contains one\natomic commit updating A and B at the same time.\n\nTo fully emulate that Gerrit feature you would need to put\nthe 3 lines above in an infinite loop that polls the remote\nsubmodules all the time.\n\nIf you wanted to do that without that Gerrit feature,\nthis becomes racy as \"submodule update --remote\"\nfetches the submodules racily (by design) and it may end\nup putting A and B in the same commit or not.\n\n\n>\n>> To ease the configuration in Gerrit\n>> a special value of \".\" has been introduced for the submodule.<name>.branch\n>> to mean the same branch as the superproject[2], such that you can create a\n>> new branch on both superproject and the submodule and this feature\n>> continues to work on that new branch.\n>>\n>> Now we have find projects in the wild with such a .gitmodules file.\n\nI'll fix the typo. (delete have or find)\n\n>\n> That's annoying.\n>\n>> To have Git working well with these, we imitate the behavior and\n>> look up the superprojects branch name if the submodules branch is\n>> configured to \".\". In projects that do not use Gerrit, this value\n>> whould be never configured as \".\" is not a valid branch name.\n>\n> I find that the last sentence is somewhat misleading.  I agree it is\n> justifiable that using \".\" as the name to trigger a new (to us)\n> feature is safe, because such a setting wouldn't have meant anything\n> useful without this change, but I initially misread it and thought\n> you added \"are we using Gerrit?  Error out if we are not\" logic,\n> which is not the case here.\n\nWell I wanted to express:\n\n  The .gitmodules used in these Gerrit projects do not conform\n  to Gits understanding of how .gitmodules should look like.\n  Let's make Git understand this Gerrit corner case (which you\n  would only need when using Gerrit).\n\n  Adding special treatment of the \".\" value is safe as this is an\n  invalid branch name in Git.\n\n\n>\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index 4ec7546..1eb33ad 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -590,7 +590,6 @@ cmd_update()\n>>\n>>               name=$(git submodule--helper name \"$sm_path\") || exit\n>>               url=$(git config submodule.\"$name\".url)\n>> -             branch=$(get_submodule_config \"$name\" branch master)\n>>               if ! test -z \"$update\"\n>>               then\n>>                       update_module=$update\n>> @@ -616,6 +615,14 @@ cmd_update()\n>>\n>>               if test -n \"$remote\"\n>>               then\n>> +                     branch=$(get_submodule_config \"$name\" branch master)\n>> +                     if test \"$branch\" = \".\"\n>> +                     then\n>> +                             if ! branch=$(git symbolic-ref --short -q HEAD)\n>> +                             then\n>> +                                     die \"$(eval_gettext \"submodule branch configured to inherit branch from superproject, but it's not on any branch\")\"\n>> +                             fi\n>> +                     fi\n>\n> I see that you narrowed the scope of \"$branch\" (which is only used\n> when $remote exists), but it is a bit annoying to see that change\n> mixed with \"now a dot means something different\" change.\n>\n> I wonder if the above 8-line block wants to be encapsulated to\n> become a part of \"git submodule--helper\" interface, though.  IOW,\n> branch=$(git submodule--helper branch \"$name\") or something?\n\nThere is already get_submodule_config that we implement in shell,\nwhich is also used in cmd_summary for reading the .ignore\nfield.\n\nSo having the \".\" check in that method (whether in shell or in C)\ndoesn't make sense to me.\n\nWe could of course have our own method specific for branches,\nthat take the \".\" into account. I think we can go with that.\n\n>\n>> +test_expect_success 'submodule update --remote should fetch upstream changes with .' '\n>> +     (cd super &&\n>> +      git config -f .gitmodules submodule.\"submodule\".branch \".\" &&\n>> +      git add .gitmodules &&\n>> +      git commit -m \"submodules: update from the respective superproject branch\"\n>> +     ) &&\n>>       (cd submodule &&\n>> +      echo line4a >> file &&\n>> +      git add file &&\n>> +      test_tick &&\n>> +      git commit -m \"upstream line4a\" &&\n>> +      git checkout -b test-branch &&\n>> +      test_commit on-test-branch\n>> +     ) &&\n>> +     (cd super &&\n>> +      git submodule update --remote --force submodule &&\n>> +      (cd submodule &&\n>> +       test \"$(git log -1 --oneline)\" = \"$(GIT_DIR=../../submodule/.git git log -1 --oneline master)\"\n>\n> A few issues:\n>\n>   * A crash in \"git log\" would not be noticed with this.  Perhaps\n>\n>         git log -1 --oneline $one_way_to_invoke >expect &&\n>         git log -1 --oneline $the_other_way >actual &&\n>         test_cmp expect actual\n>\n>     would be better?\n\nOf course. I tried to blend in with the code after looking at the surrounding\ncode. Maybe I need to modernize that whole test file as a preparatory step.\n\n>\n>   * What exactly is this testing?  The current branch (in submodule)\n>     pointing at the same commit as the tip of 'master'?  Or the\n>     current branch _is_ 'master'?\n>\n>   * What exactly is the reason why one has GIT_DIR=... and the other\n>     does not?  I do not think this a place to test that \"gitdir: \"\n>     in .git points at the right place, so it must be testing\n>     something else, but I cannot guess.\n\nIt is testing that the local state is at the same commit as the\nmaster branch on the remote side.\n(GIT_DIR=../../submodule/.git is saying to be \"remote\", and \"master\"\nto check that branch. We need to have master here as we configured\n\".\" and have the master branch checked out in the superproject.)\n\nAt least that is my understanding from the existing tests for\nthe  \"--remote\" flag\n\n>> +     (cd super &&\n>> +      git checkout --detach &&\n>> +      # update is not confused by branch=\".\" even if the the superproject\n>> +      # is not on any branch currently\n>> +      git submodule update &&\n>> +      git revert HEAD &&\n>\n> \"revert\" is rather unusual thing to see in the test.\n\nThe tests are so long that I tried to get back in a state that is as least\ndifferent from before to not break the following tests.\n(And except for the depth this worked well)\n\n> Also I am not\n> sure why cmd_update that now has an explicit check to die when\n> branch is set to \".\" and the head is detached is expected \"not\" to\n> be confused.  Perhaps I misread the main part of the patch?\n\nWell you *only* explicitly die(..) when you ask for --remote.\nIf the superproject is on a detached HEAD and just wants the\nsha1s as recorded (--no-remote), it doesn't care about the recorded\nbranch value and should *not* die.\n"},{"id":"292446","messageId":"xmqq37mtsfov.fsf@gitster.mtv.corp.google.com","threadId":"42950","inReplyTo":"CAGZ79kZfXKC66uokqOXeWND=V+53YfAkfd1NUK7A4jM2UAea7A@mail.gmail.com","subject":"Re: [PATCH 1/2] t7406: correct depth test in shallow test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-28T19:52:48Z","receivedAt":"2016-07-28T19:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The depth of 2 I chose originally turns out to be a lucky\n> accident too, as the depth from \"Commit 2\" is 2,\n> so that we would observe the same depth no matter if\n> a --depth 2 was given and working or not.\n>\n> I'll redo this test (as 2 tests, one is testing the situation as\n> we have now, i.e. the desired tip is reachable from within\n> the depth argument, the second test will test if it is not\n> reachable.)\n\nGood that I was puzzled ;-) Looking forward to see an improved test.\n\nThanks.\n"},{"id":"292447","messageId":"xmqqy44lr0o4.fsf@gitster.mtv.corp.google.com","threadId":"42950","inReplyTo":"CAGZ79kactn4VZ5i+fCrBGa77dzFiTngAgVU3R1orz2=EMAZ1Jg@mail.gmail.com","subject":"Re: [PATCHv2 2/2] submodule update: allow '.' for branch value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-28T20:02:35Z","receivedAt":"2016-07-28T20:02:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Well I wanted to express:\n>\n>   The .gitmodules used in these Gerrit projects do not conform\n>   to Gits understanding of how .gitmodules should look like.\n>   Let's make Git understand this Gerrit corner case (which you\n>   would only need when using Gerrit).\n>\n>   Adding special treatment of the \".\" value is safe as this is an\n>   invalid branch name in Git.\n\nYup, I got it after reading it twice.  My point was that you\nshouldn't have to read it twice to get it.\n\n>> I wonder if the above 8-line block wants to be encapsulated to\n>> become a part of \"git submodule--helper\" interface, though.  IOW,\n>> branch=$(git submodule--helper branch \"$name\") or something?\n>\n> There is already get_submodule_config that we implement in shell,\n> which is also used in cmd_summary for reading the .ignore\n> field.\n>\n> So having the \".\" check in that method (whether in shell or in C)\n> doesn't make sense to me.\n\nThat's an excuse from the helper implementor's side, isn't it?  I\nwas coming from the opposite direction, i.e. potential caller of a\nhelper.  Whenever I want to know \"is there a branch configured for\nthis submodule, and if so what is it?\", wouldn't I be entitled to a\nhelper that consistently gets the real branch name with the magic\n\".\" resolved for me?\n\n>>> +       test \"$(git log -1 --oneline)\" = \"$(GIT_DIR=../../submodule/.git git log -1 --oneline master)\"\n>>\n>> A few issues:\n>>\n>>   * A crash in \"git log\" would not be noticed with this.  Perhaps\n>>\n>>         git log -1 --oneline $one_way_to_invoke >expect &&\n>>         git log -1 --oneline $the_other_way >actual &&\n>>         test_cmp expect actual\n>>\n>>     would be better?\n>\n> Of course. I tried to blend in with the code after looking at the surrounding\n> code. Maybe I need to modernize that whole test file as a preparatory step.\n>>\n>>   * What exactly is this testing?  The current branch (in submodule)\n>>     pointing at the same commit as the tip of 'master'?  Or the\n>>     current branch _is_ 'master'?\n>>\n>>   * What exactly is the reason why one has GIT_DIR=... and the other\n>>     does not?  I do not think this a place to test that \"gitdir: \"\n>>     in .git points at the right place, so it must be testing\n>>     something else, but I cannot guess.\n>\n> It is testing that the local state is at the same commit as the\n> master branch on the remote side.\n\nAhh, OK, I totally misread that.  \"git -C ../../submodule log\" would\nhave been the more modern way to say that, I would guess, but now it\nmakes sense.\n\n>>> +      # update is not confused by branch=\".\" even if the the superproject\n>>> +      # is not on any branch currently\n>>> +      git submodule update &&\n>>> +      git revert HEAD &&\n>>\n>> \"revert\" is rather unusual thing to see in the test.\n>\n> The tests are so long that I tried to get back in a state that is as least\n> different from before to not break the following tests.\n\nI guessed that much; I just expected to see \"git reset --hard\n$some_old_state\" if you want to rewind to the previous state the\nnext test expects and \"revert\" looked unusual.\n\n>> Also I am not\n>> sure why cmd_update that now has an explicit check to die when\n>> branch is set to \".\" and the head is detached is expected \"not\" to\n>> be confused.  Perhaps I misread the main part of the patch?\n>\n> Well you *only* explicitly die(..) when you ask for --remote.\n\nOK, I _did_ misread the patch, then.  It would help to have \"when\ngiving no --remote, git submodule\" before the comment that begins\nwith \"update is not confused\" to avoid the same confusion.\n\nThanks.\n\n"}]}