{"thread":{"id":"49440","subject":"[PATCH] submodule: Alllow staged changes for get_superproject_working_tree","startedAt":"2018-09-27T18:12:35Z","lastAt":"2018-09-28T22:25:56Z","messageCount":9,"participants":["Sam McKelvie","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"359080","messageId":"20180927181054.25802-1-sammck@gmail.com","threadId":"49440","inReplyTo":null,"subject":"[PATCH] submodule: Alllow staged changes for get_superproject_working_tree","fromName":"Sam McKelvie","fromEmail":"sammck@gmail.com","sentAt":"2018-09-27T18:10:54Z","receivedAt":"2018-09-27T18:12:35Z","isPatch":true,"sender":{"key":"sammck@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7035152?v=4"},"body":"Invoking 'git rev-parse --show-superproject-working-tree' exits with\n\n    \"fatal: BUG: returned path string doesn't match cwd?\"\n\nwhen the superproject has an unmerged entry for the current submodule,\ninstead of displaying the superproject's working tree.\n\nThe problem is due to the fact that when a merge of the submodule reference\nis in progress, \"git ls-files --stage —full-name <submodule-relative-path>”\nreturns three seperate entries for the submodule (one for each stage) rather\nthan a single entry; e.g.,\n\n$ git ls-files --stage --full-name submodule-child-test\n160000 dbbd2766fa330fa741ea59bb38689fcc2d283ac5 1       submodule-child-test\n160000 f174d1dbfe863a59692c3bdae730a36f2a788c51 2       submodule-child-test\n160000 e6178f3a58b958543952e12824aa2106d560f21d 3       submodule-child-test\n\nThe code in get_superproject_working_tree() expected exactly one entry to\nbe returned; this patch makes it use the first entry if multiple entries\nare returned.\n\nTest t1500-rev-parse is extended to cover this case.\n\nSigned-off-by: Sam McKelvie <sammck@gmail.com>\n---\n submodule.c          |  2 +-\n t/t1500-rev-parse.sh | 17 ++++++++++++++++-\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 33de6ee5f..5b9d5ad7e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1885,7 +1885,7 @@ const char *get_superproject_working_tree(void)\n \t\t * We're only interested in the name after the tab.\n \t\t */\n \t\tsuper_sub = strchr(sb.buf, '\\t') + 1;\n-\t\tsuper_sub_len = sb.buf + sb.len - super_sub - 1;\n+\t\tsuper_sub_len = strlen(super_sub);\n \n \t\tif (super_sub_len > cwd_len ||\n \t\t    strcmp(&cwd[cwd_len - super_sub_len], super_sub))\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 5c715fe2c..b774cafc5 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -134,7 +134,6 @@ test_expect_success 'rev-parse --is-shallow-repository in non-shallow repo' '\n test_expect_success 'showing the superproject correctly' '\n \tgit rev-parse --show-superproject-working-tree >out &&\n \ttest_must_be_empty out &&\n-\n \ttest_create_repo super &&\n \ttest_commit -C super test_commit &&\n \ttest_create_repo sub &&\n@@ -142,6 +141,22 @@ test_expect_success 'showing the superproject correctly' '\n \tgit -C super submodule add ../sub dir/sub &&\n \techo $(pwd)/super >expect  &&\n \tgit -C super/dir/sub rev-parse --show-superproject-working-tree >out &&\n+\ttest_cmp expect out &&\n+\ttest_commit -C super submodule_add &&\n+\tgit -C super checkout -b branch1 &&\n+\tgit -C super/dir/sub checkout -b branch1 &&\n+\ttest_commit -C super/dir/sub branch1_commit &&\n+\tgit -C super add dir/sub &&\n+\ttest_commit -C super branch1_commit &&\n+\tgit -C super checkout master &&\n+\tgit -C super checkout -b branch2 &&\n+\tgit -C super/dir/sub checkout master &&\n+\tgit -C super/dir/sub checkout -b branch2 &&\n+\ttest_commit -C super/dir/sub branch2_commit &&\n+\tgit -C super add dir/sub &&\n+\ttest_commit -C super branch2_commit &&\n+\ttest_must_fail git -C super merge branch1 &&\n+\tgit -C super/dir/sub rev-parse --show-superproject-working-tree >out &&\n \ttest_cmp expect out\n '\n \n-- \n2.17.1\n\n"},{"id":"359109","messageId":"CAGZ79kaRYra+2Ue2TSS74tE_gdg8ppSfNtVQK4LhOrQTj8td8Q@mail.gmail.com","threadId":"49440","inReplyTo":"20180927181054.25802-1-sammck@gmail.com","subject":"Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-27T19:42:15Z","receivedAt":"2018-09-27T19:42:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 27, 2018 at 11:12 AM Sam McKelvie <sammck@gmail.com> wrote:\n>\n> Invoking 'git rev-parse --show-superproject-working-tree' exits with\n>\n>     \"fatal: BUG: returned path string doesn't match cwd?\"\n>\n> when the superproject has an unmerged entry for the current submodule,\n> instead of displaying the superproject's working tree.\n>\n> The problem is due to the fact that when a merge of the submodule reference\n> is in progress, \"git ls-files --stage —full-name <submodule-relative-path>”\n> returns three seperate entries for the submodule (one for each stage) rather\n> than a single entry; e.g.,\n>\n> $ git ls-files --stage --full-name submodule-child-test\n> 160000 dbbd2766fa330fa741ea59bb38689fcc2d283ac5 1       submodule-child-test\n> 160000 f174d1dbfe863a59692c3bdae730a36f2a788c51 2       submodule-child-test\n> 160000 e6178f3a58b958543952e12824aa2106d560f21d 3       submodule-child-test\n>\n> The code in get_superproject_working_tree() expected exactly one entry to\n> be returned; this patch makes it use the first entry if multiple entries\n> are returned.\n>\n> Test t1500-rev-parse is extended to cover this case.\n>\n> Signed-off-by: Sam McKelvie <sammck@gmail.com>\n\nThanks for following up, this patch is\nReviewed-by: Stefan Beller <sbeller@google.com>\n\nThanks for adding the test as well!\nStefan\n"},{"id":"359148","messageId":"xmqqbm8ifvka.fsf@gitster-ct.c.googlers.com","threadId":"49440","inReplyTo":"20180927181054.25802-1-sammck@gmail.com","subject":"Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-27T22:02:29Z","receivedAt":"2018-09-27T22:02:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sam McKelvie <sammck@gmail.com> writes:\n\n> Subject: Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree\n\ns/Alllow/allow/;\n\n> Invoking 'git rev-parse --show-superproject-working-tree' exits with\n>\n>     \"fatal: BUG: returned path string doesn't match cwd?\"\n>\n> when the superproject has an unmerged entry for the current submodule,\n> instead of displaying the superproject's working tree.\n>\n> The problem is due to the fact that when a merge of the submodule reference\n> is in progress, \"git ls-files --stage —full-name <submodule-relative-path>”\n> returns three seperate entries for the submodule (one for each stage) rather\n> than a single entry; e.g.,\n>\n> $ git ls-files --stage --full-name submodule-child-test\n> 160000 dbbd2766fa330fa741ea59bb38689fcc2d283ac5 1       submodule-child-test\n> 160000 f174d1dbfe863a59692c3bdae730a36f2a788c51 2       submodule-child-test\n> 160000 e6178f3a58b958543952e12824aa2106d560f21d 3       submodule-child-test\n>\n> The code in get_superproject_working_tree() expected exactly one entry to\n> be returned; this patch makes it use the first entry if multiple entries\n> are returned.\n>\n> Test t1500-rev-parse is extended to cover this case.\n>\n> Signed-off-by: Sam McKelvie <sammck@gmail.com>\n> ---\n>  submodule.c          |  2 +-\n>  t/t1500-rev-parse.sh | 17 ++++++++++++++++-\n>  2 files changed, 17 insertions(+), 2 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 33de6ee5f..5b9d5ad7e 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1885,7 +1885,7 @@ const char *get_superproject_working_tree(void)\n>  \t\t * We're only interested in the name after the tab.\n>  \t\t */\n>  \t\tsuper_sub = strchr(sb.buf, '\\t') + 1;\n> -\t\tsuper_sub_len = sb.buf + sb.len - super_sub - 1;\n> +\t\tsuper_sub_len = strlen(super_sub);\n\nAs we are reading from \"ls-files -z -s\", we know that the name is\nterminated with NUL, so we can just use strlen().  Good.\n>  \n>  \t\tif (super_sub_len > cwd_len ||\n>  \t\t    strcmp(&cwd[cwd_len - super_sub_len], super_sub))\n> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\n> index 5c715fe2c..b774cafc5 100755\n> --- a/t/t1500-rev-parse.sh\n> +++ b/t/t1500-rev-parse.sh\n> @@ -134,7 +134,6 @@ test_expect_success 'rev-parse --is-shallow-repository in non-shallow repo' '\n>  test_expect_success 'showing the superproject correctly' '\n>  \tgit rev-parse --show-superproject-working-tree >out &&\n>  \ttest_must_be_empty out &&\n> -\n\nI have a feeling that this break made the series of tests in this\nblock easier to follow.  Shouldn't we be moving in the other\ndirection, namely ...\n\n>  \ttest_create_repo super &&\n>  \ttest_commit -C super test_commit &&\n>  \ttest_create_repo sub &&\n> @@ -142,6 +141,22 @@ test_expect_success 'showing the superproject correctly' '\n>  \tgit -C super submodule add ../sub dir/sub &&\n>  \techo $(pwd)/super >expect  &&\n>  \tgit -C super/dir/sub rev-parse --show-superproject-working-tree >out &&\n> +\ttest_cmp expect out &&\n\nHere is an end of one subtest, deserves to have a break like the above.\n\n> +\ttest_commit -C super submodule_add &&\n> +\tgit -C super checkout -b branch1 &&\n> +\tgit -C super/dir/sub checkout -b branch1 &&\n> +\ttest_commit -C super/dir/sub branch1_commit &&\n> +\tgit -C super add dir/sub &&\n> +\ttest_commit -C super branch1_commit &&\n> +\tgit -C super checkout master &&\n> +\tgit -C super checkout -b branch2 &&\n> +\tgit -C super/dir/sub checkout master &&\n> +\tgit -C super/dir/sub checkout -b branch2 &&\n> +\ttest_commit -C super/dir/sub branch2_commit &&\n> +\tgit -C super add dir/sub &&\n> +\ttest_commit -C super branch2_commit &&\n> +\ttest_must_fail git -C super merge branch1 &&\n\nand all of the above is just a set-up for another subtest, so a\nsolid block of text like we see in the above is good.\n\n\tSide note: there are a few of\n\n\t\tgit -C $there checkout $onebranch &&\n\t\tgit -C $there checkout -b $anotherbranch &&\n\n\tas recurring pattern.  Shouldn't they be more like a single\n\tliner\n\n\t\tgit -C $there checkout -b $anotherbranch $onebranch &&\n\n\t?  It wasn't clear if the split was an attempt to hide some\n\tbreakage (e.g. \"checkout -b B A\" did not work but \"checkout\n\tA && checkout -b B\" did) or just being verbose because the\n\tauthor is not used to \"checkout -b B A\" form.\n\n> +\tgit -C super/dir/sub rev-parse --show-superproject-working-tree >out &&\n>  \ttest_cmp expect out\n>  '\n\nThanks.\n"},{"id":"359166","messageId":"D09E2583-BD1D-4C56-9C19-6B1E97CEEDAA@gmail.com","threadId":"49440","inReplyTo":"xmqqbm8ifvka.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree","fromName":"Sam McKelvie","fromEmail":"sammck@gmail.com","sentAt":"2018-09-28T00:30:21Z","receivedAt":"2018-09-28T00:30:27Z","isPatch":true,"sender":{"key":"sammck@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7035152?v=4"},"body":"All of your comments seem reasonable; however, since the patch was signed off by Stefan it\nIs unclear to me whether I should submit another patch or what. I apologize for not being\nfacile with the patching workflow.\n\n> On Sep 27, 2018, at 3:02 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> \n> Sam McKelvie <sammck@gmail.com> writes:\n> \n>> Subject: Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree\n> \n> s/Alllow/allow/;\n> \n\nOk, no caps on first letter of subject.\n\n>> Invoking 'git rev-parse --show-superproject-working-tree' exits with\n>> \n>>    \"fatal: BUG: returned path string doesn't match cwd?\"\n>> \n>> when the superproject has an unmerged entry for the current submodule,\n>> instead of displaying the superproject's working tree.\n>> \n>> The problem is due to the fact that when a merge of the submodule reference\n>> is in progress, \"git ls-files --stage —full-name <submodule-relative-path>”\n>> returns three seperate entries for the submodule (one for each stage) rather\n>> than a single entry; e.g.,\n>> \n>> $ git ls-files --stage --full-name submodule-child-test\n>> 160000 dbbd2766fa330fa741ea59bb38689fcc2d283ac5 1       submodule-child-test\n>> 160000 f174d1dbfe863a59692c3bdae730a36f2a788c51 2       submodule-child-test\n>> 160000 e6178f3a58b958543952e12824aa2106d560f21d 3       submodule-child-test\n>> \n>> The code in get_superproject_working_tree() expected exactly one entry to\n>> be returned; this patch makes it use the first entry if multiple entries\n>> are returned.\n>> \n>> Test t1500-rev-parse is extended to cover this case.\n>> \n>> Signed-off-by: Sam McKelvie <sammck@gmail.com>\n>> ---\n>> submodule.c          |  2 +-\n>> t/t1500-rev-parse.sh | 17 ++++++++++++++++-\n>> 2 files changed, 17 insertions(+), 2 deletions(-)\n>> \n>> diff --git a/submodule.c b/submodule.c\n>> index 33de6ee5f..5b9d5ad7e 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -1885,7 +1885,7 @@ const char *get_superproject_working_tree(void)\n>> \t\t * We're only interested in the name after the tab.\n>> \t\t */\n>> \t\tsuper_sub = strchr(sb.buf, '\\t') + 1;\n>> -\t\tsuper_sub_len = sb.buf + sb.len - super_sub - 1;\n>> +\t\tsuper_sub_len = strlen(super_sub);\n> \n> As we are reading from \"ls-files -z -s\", we know that the name is\n> terminated with NUL, so we can just use strlen().  Good.\n>> \n>> \t\tif (super_sub_len > cwd_len ||\n>> \t\t    strcmp(&cwd[cwd_len - super_sub_len], super_sub))\n>> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\n>> index 5c715fe2c..b774cafc5 100755\n>> --- a/t/t1500-rev-parse.sh\n>> +++ b/t/t1500-rev-parse.sh\n>> @@ -134,7 +134,6 @@ test_expect_success 'rev-parse --is-shallow-repository in non-shallow repo' '\n>> test_expect_success 'showing the superproject correctly' '\n>> \tgit rev-parse --show-superproject-working-tree >out &&\n>> \ttest_must_be_empty out &&\n>> -\n> \n> I have a feeling that this break made the series of tests in this\n> block easier to follow.  Shouldn't we be moving in the other\n> direction, namely …\n> \nThat’s fair.\n\n\n>> \ttest_create_repo super &&\n>> \ttest_commit -C super test_commit &&\n>> \ttest_create_repo sub &&\n>> @@ -142,6 +141,22 @@ test_expect_success 'showing the superproject correctly' '\n>> \tgit -C super submodule add ../sub dir/sub &&\n>> \techo $(pwd)/super >expect  &&\n>> \tgit -C super/dir/sub rev-parse --show-superproject-working-tree >out &&\n>> +\ttest_cmp expect out &&\n> \n> Here is an end of one subtest, deserves to have a break like the above.\n\nOK\n\n> \n>> +\ttest_commit -C super submodule_add &&\n>> +\tgit -C super checkout -b branch1 &&\n>> +\tgit -C super/dir/sub checkout -b branch1 &&\n>> +\ttest_commit -C super/dir/sub branch1_commit &&\n>> +\tgit -C super add dir/sub &&\n>> +\ttest_commit -C super branch1_commit &&\n>> +\tgit -C super checkout master &&\n>> +\tgit -C super checkout -b branch2 &&\n>> +\tgit -C super/dir/sub checkout master &&\n>> +\tgit -C super/dir/sub checkout -b branch2 &&\n>> +\ttest_commit -C super/dir/sub branch2_commit &&\n>> +\tgit -C super add dir/sub &&\n>> +\ttest_commit -C super branch2_commit &&\n>> +\ttest_must_fail git -C super merge branch1 &&\n> \n> and all of the above is just a set-up for another subtest, so a\n> solid block of text like we see in the above is good.\n> \n> \tSide note: there are a few of\n> \n> \t\tgit -C $there checkout $onebranch &&\n> \t\tgit -C $there checkout -b $anotherbranch &&\n> \n> \tas recurring pattern.  Shouldn't they be more like a single\n> \tliner\n> \n> \t\tgit -C $there checkout -b $anotherbranch $onebranch &&\n> \n> \t?  It wasn't clear if the split was an attempt to hide some\n> \tbreakage (e.g. \"checkout -b B A\" did not work but \"checkout\n> \tA && checkout -b B\" did) or just being verbose because the\n> \tauthor is not used to \"checkout -b B A\" form.\n\nYou’re right, the two forms are equivalent and the single-line version is simpler.\n\n> \n>> +\tgit -C super/dir/sub rev-parse --show-superproject-working-tree >out &&\n>> \ttest_cmp expect out\n>> '\n> \n> Thanks.\n\nThank you.\n\n"},{"id":"359174","messageId":"xmqqmus2e6rp.fsf@gitster-ct.c.googlers.com","threadId":"49440","inReplyTo":"D09E2583-BD1D-4C56-9C19-6B1E97CEEDAA@gmail.com","subject":"Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-28T01:43:22Z","receivedAt":"2018-09-28T01:43:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sam McKelvie <sammck@gmail.com> writes:\n\n>>> Subject: Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree\n>> \n>> s/Alllow/allow/;\n>> \n>\n> Ok, no caps on first letter of subject.\n\nAh, that, too.  I meant to correct triple ell, though ;-)\n\nWhen one reviewer says Reviewed-by: but you later found that the\npatch was not good enough to deserve a redoing, feel free to redo\nthe patch and do not add the Reviewed-by: you got for the old one\nto your second submission.  The difference between the one that was\nreviewed and the one you updated invalidates the stale Reviewed-by:,\nessentially.\n\nSome reviewers explicitly state \"With this and that nit corrected or\nleft as-is, the patch is Reviewed-by: me\" to tell you that as long\nas the difference between the version reviewed and the updated one\nis limited within the named issues, you can add Reviewed-by: to your\nrerolled patch.\n\nIn this case, I think the nits are pretty small and I do not mind\ntweaking the version we are discussing on my end, without having you\nto send an updated one.\n\nHere is what I'd squash into your commit, with title corrected, if\nyou are OK with that plan.  In the meantime, I'll keep the following\nas a separate patch on top when I merge your fix to 'pu'.\n\nThanks.\n\n t/t1500-rev-parse.sh | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex b774cafc5d..01abee533d 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -134,6 +134,7 @@ test_expect_success 'rev-parse --is-shallow-repository in non-shallow repo' '\n test_expect_success 'showing the superproject correctly' '\n \tgit rev-parse --show-superproject-working-tree >out &&\n \ttest_must_be_empty out &&\n+\n \ttest_create_repo super &&\n \ttest_commit -C super test_commit &&\n \ttest_create_repo sub &&\n@@ -142,20 +143,20 @@ test_expect_success 'showing the superproject correctly' '\n \techo $(pwd)/super >expect  &&\n \tgit -C super/dir/sub rev-parse --show-superproject-working-tree >out &&\n \ttest_cmp expect out &&\n+\n \ttest_commit -C super submodule_add &&\n \tgit -C super checkout -b branch1 &&\n \tgit -C super/dir/sub checkout -b branch1 &&\n \ttest_commit -C super/dir/sub branch1_commit &&\n \tgit -C super add dir/sub &&\n \ttest_commit -C super branch1_commit &&\n-\tgit -C super checkout master &&\n-\tgit -C super checkout -b branch2 &&\n-\tgit -C super/dir/sub checkout master &&\n-\tgit -C super/dir/sub checkout -b branch2 &&\n+\tgit -C super checkout -b branch2 master &&\n+\tgit -C super/dir/sub checkout -b branch2 master &&\n \ttest_commit -C super/dir/sub branch2_commit &&\n \tgit -C super add dir/sub &&\n \ttest_commit -C super branch2_commit &&\n \ttest_must_fail git -C super merge branch1 &&\n+\n \tgit -C super/dir/sub rev-parse --show-superproject-working-tree >out &&\n \ttest_cmp expect out\n '\n"},{"id":"359178","messageId":"37DC7525-209F-49CC-90AA-D55463456D20@gmail.com","threadId":"49440","inReplyTo":"xmqqmus2e6rp.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree","fromName":"Sam McKelvie","fromEmail":"sammck@gmail.com","sentAt":"2018-09-28T03:29:40Z","receivedAt":"2018-09-28T03:29:45Z","isPatch":true,"sender":{"key":"sammck@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7035152?v=4"},"body":"> On Sep 27, 2018, at 6:43 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> \n> Sam McKelvie <sammck@gmail.com> writes:\n> \n>>>> Subject: Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree\n>>> \n>>> s/Alllow/allow/;\n>>> \n>> \n>> Ok, no caps on first letter of subject.\n> \n> Ah, that, too.  I meant to correct triple ell, though ;-)\n> \n> When one reviewer says Reviewed-by: but you later found that the\n> patch was not good enough to deserve a redoing, feel free to redo\n> the patch and do not add the Reviewed-by: you got for the old one\n> to your second submission.  The difference between the one that was\n> reviewed and the one you updated invalidates the stale Reviewed-by:,\n> essentially.\n> \n> Some reviewers explicitly state \"With this and that nit corrected or\n> left as-is, the patch is Reviewed-by: me\" to tell you that as long\n> as the difference between the version reviewed and the updated one\n> is limited within the named issues, you can add Reviewed-by: to your\n> rerolled patch.\n> \n> In this case, I think the nits are pretty small and I do not mind\n> tweaking the version we are discussing on my end, without having you\n> to send an updated one.\n> \n> Here is what I'd squash into your commit, with title corrected, if\n> you are OK with that plan.  In the meantime, I'll keep the following\n> as a separate patch on top when I merge your fix to 'pu'.\n> \n> Thanks.\n\nI wholeheartedly approve of that plan and your tweaking commit below. Thank you, Junio.\n\n~Sam\n\n> \n> t/t1500-rev-parse.sh | 9 +++++----\n> 1 file changed, 5 insertions(+), 4 deletions(-)\n> \n> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\n> index b774cafc5d..01abee533d 100755\n> --- a/t/t1500-rev-parse.sh\n> +++ b/t/t1500-rev-parse.sh\n> @@ -134,6 +134,7 @@ test_expect_success 'rev-parse --is-shallow-repository in non-shallow repo' '\n> test_expect_success 'showing the superproject correctly' '\n> \tgit rev-parse --show-superproject-working-tree >out &&\n> \ttest_must_be_empty out &&\n> +\n> \ttest_create_repo super &&\n> \ttest_commit -C super test_commit &&\n> \ttest_create_repo sub &&\n> @@ -142,20 +143,20 @@ test_expect_success 'showing the superproject correctly' '\n> \techo $(pwd)/super >expect  &&\n> \tgit -C super/dir/sub rev-parse --show-superproject-working-tree >out &&\n> \ttest_cmp expect out &&\n> +\n> \ttest_commit -C super submodule_add &&\n> \tgit -C super checkout -b branch1 &&\n> \tgit -C super/dir/sub checkout -b branch1 &&\n> \ttest_commit -C super/dir/sub branch1_commit &&\n> \tgit -C super add dir/sub &&\n> \ttest_commit -C super branch1_commit &&\n> -\tgit -C super checkout master &&\n> -\tgit -C super checkout -b branch2 &&\n> -\tgit -C super/dir/sub checkout master &&\n> -\tgit -C super/dir/sub checkout -b branch2 &&\n> +\tgit -C super checkout -b branch2 master &&\n> +\tgit -C super/dir/sub checkout -b branch2 master &&\n> \ttest_commit -C super/dir/sub branch2_commit &&\n> \tgit -C super add dir/sub &&\n> \ttest_commit -C super branch2_commit &&\n> \ttest_must_fail git -C super merge branch1 &&\n> +\n> \tgit -C super/dir/sub rev-parse --show-superproject-working-tree >out &&\n> \ttest_cmp expect out\n> '\n\n"},{"id":"359227","messageId":"xmqqftxtcxjb.fsf@gitster-ct.c.googlers.com","threadId":"49440","inReplyTo":"37DC7525-209F-49CC-90AA-D55463456D20@gmail.com","subject":"Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-28T18:00:24Z","receivedAt":"2018-09-28T18:00:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sam McKelvie <sammck@gmail.com> writes:\n\n>> Ah, that, too.  I meant to correct triple ell, though ;-)\n>>  ...\n>\n> I wholeheartedly approve of that plan and your tweaking commit below. Thank you, Junio.\n\nThanks for a fix.  But now I re-read the title and think about it,\nthis is mistitled.  The word 'stage' in \"ls-files --stage\" is not\nabout 'stage' people use when they talk about \"staged changes\" at\nall.  The latter is what \"diff --cached\" is about---what's different\nbetween HEAD and the index.  The 'stage' \"ls-files --stage\" talks\nabout is \"which parent the cache entry came from, among common\nancestor, us, or the other branch being merged\".\n\nAlso we are not really \"allowing\" with this change; \"allowing\" makes\nit sound as if we were earlier \"forbidding\" with a good reason and\nthe change is lifting the limitation.\n\nWe used to incorrectly die when superproject is resolving a conflict\nfor the submodule we are currently in, and that is what the patch\nfixed.\n\nsubmodule: parse output of conflicted ls-files in superproject correctly\n\nis the shortest I could come up with, while touching all the points\nthat need to be touched and still being technically not-incorrect.\n\nOr perhaps\n\nrev-parse: --show-superproject-working-tree should work during a merge\n\nmay be more to the point.  It does not hint the root cause of the\nbug like the other one, but is more direct how the breakage would\nhave been observed by the end users.\n\n"},{"id":"359254","messageId":"2A26D56C-59E6-4F69-B0CA-4849EBE7EBC9@gmail.com","threadId":"49440","inReplyTo":"xmqqftxtcxjb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree","fromName":"Sam McKelvie","fromEmail":"sammck@gmail.com","sentAt":"2018-09-28T21:59:15Z","receivedAt":"2018-09-28T21:59:20Z","isPatch":true,"sender":{"key":"sammck@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7035152?v=4"},"body":"\n\n> On Sep 28, 2018, at 11:00 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> \n> Sam McKelvie <sammck@gmail.com> writes:\n> \n>>> Ah, that, too.  I meant to correct triple ell, though ;-)\n>>> ...\n>> \n>> I wholeheartedly approve of that plan and your tweaking commit below. Thank you, Junio.\n> \n> Thanks for a fix.  But now I re-read the title and think about it,\n> this is mistitled.  The word 'stage' in \"ls-files --stage\" is not\n> about 'stage' people use when they talk about \"staged changes\" at\n> all.  The latter is what \"diff --cached\" is about---what's different\n> between HEAD and the index.  The 'stage' \"ls-files --stage\" talks\n> about is \"which parent the cache entry came from, among common\n> ancestor, us, or the other branch being merged\".\n> \n> Also we are not really \"allowing\" with this change; \"allowing\" makes\n> it sound as if we were earlier \"forbidding\" with a good reason and\n> the change is lifting the limitation.\n> \n> We used to incorrectly die when superproject is resolving a conflict\n> for the submodule we are currently in, and that is what the patch\n> fixed.\n> \n> submodule: parse output of conflicted ls-files in superproject correctly\n> \n> is the shortest I could come up with, while touching all the points\n> that need to be touched and still being technically not-incorrect.\n> \n> Or perhaps\n> \n> rev-parse: --show-superproject-working-tree should work during a merge\n> \n> may be more to the point.  It does not hint the root cause of the\n> bug like the other one, but is more direct how the breakage would\n> have been observed by the end users.\n> \nHaha, that is closer to my original title that Stefan wanted to change:\n\nsubmodule.c: Make get_superproject_working_tree() work when superproject has unmerged changes of the submodule reference\n\nThough I could see why mine was too long.\n\nI’m really happy with both your suggestions. If you still think you can squash it with your own tweaks, great. Let me know\nIf you’d prefer another patch.\n\n"},{"id":"359260","messageId":"xmqqh8i99s40.fsf@gitster-ct.c.googlers.com","threadId":"49440","inReplyTo":"2A26D56C-59E6-4F69-B0CA-4849EBE7EBC9@gmail.com","subject":"Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-28T22:25:51Z","receivedAt":"2018-09-28T22:25:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sam McKelvie <sammck@gmail.com> writes:\n\n>> Or perhaps\n>> \n>> rev-parse: --show-superproject-working-tree should work during a merge\n>> \n>> may be more to the point.  It does not hint the root cause of the\n>> bug like the other one, but is more direct how the breakage would\n>> have been observed by the end users.\n>> \n> Haha, that is closer to my original title that Stefan wanted to change:\n>\n> submodule.c: Make get_superproject_working_tree() work when superproject has unmerged changes of the submodule reference\n>\n> Though I could see why mine was too long.\n>\n> I’m really happy with both your suggestions\n\nI've pushed out with the \"rev-parse: ...\" as the title.\n\nThanks for the fix.\n\n"}]}