All of your comments seem reasonable; however, since the patch was signed off by Stefan it Is unclear to me whether I should submit another patch or what. I apologize for not being facile with the patching workflow. > On Sep 27, 2018, at 3:02 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Sam McKelvie <sammck@xxxxxxxxx> writes: > >> Subject: Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree > > s/Alllow/allow/; > Ok, no caps on first letter of subject. >> Invoking 'git rev-parse --show-superproject-working-tree' exits with >> >> "fatal: BUG: returned path string doesn't match cwd?" >> >> when the superproject has an unmerged entry for the current submodule, >> instead of displaying the superproject's working tree. >> >> The problem is due to the fact that when a merge of the submodule reference >> is in progress, "git ls-files --stage —full-name <submodule-relative-path>” >> returns three seperate entries for the submodule (one for each stage) rather >> than a single entry; e.g., >> >> $ git ls-files --stage --full-name submodule-child-test >> 160000 dbbd2766fa330fa741ea59bb38689fcc2d283ac5 1 submodule-child-test >> 160000 f174d1dbfe863a59692c3bdae730a36f2a788c51 2 submodule-child-test >> 160000 e6178f3a58b958543952e12824aa2106d560f21d 3 submodule-child-test >> >> The code in get_superproject_working_tree() expected exactly one entry to >> be returned; this patch makes it use the first entry if multiple entries >> are returned. >> >> Test t1500-rev-parse is extended to cover this case. >> >> Signed-off-by: Sam McKelvie <sammck@xxxxxxxxx> >> --- >> submodule.c | 2 +- >> t/t1500-rev-parse.sh | 17 ++++++++++++++++- >> 2 files changed, 17 insertions(+), 2 deletions(-) >> >> diff --git a/submodule.c b/submodule.c >> index 33de6ee5f..5b9d5ad7e 100644 >> --- a/submodule.c >> +++ b/submodule.c >> @@ -1885,7 +1885,7 @@ const char *get_superproject_working_tree(void) >> * We're only interested in the name after the tab. >> */ >> super_sub = strchr(sb.buf, '\t') + 1; >> - super_sub_len = sb.buf + sb.len - super_sub - 1; >> + super_sub_len = strlen(super_sub); > > As we are reading from "ls-files -z -s", we know that the name is > terminated with NUL, so we can just use strlen(). Good. >> >> if (super_sub_len > cwd_len || >> strcmp(&cwd[cwd_len - super_sub_len], super_sub)) >> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh >> index 5c715fe2c..b774cafc5 100755 >> --- a/t/t1500-rev-parse.sh >> +++ b/t/t1500-rev-parse.sh >> @@ -134,7 +134,6 @@ test_expect_success 'rev-parse --is-shallow-repository in non-shallow repo' ' >> test_expect_success 'showing the superproject correctly' ' >> git rev-parse --show-superproject-working-tree >out && >> test_must_be_empty out && >> - > > I have a feeling that this break made the series of tests in this > block easier to follow. Shouldn't we be moving in the other > direction, namely … > That’s fair. >> test_create_repo super && >> test_commit -C super test_commit && >> test_create_repo sub && >> @@ -142,6 +141,22 @@ test_expect_success 'showing the superproject correctly' ' >> git -C super submodule add ../sub dir/sub && >> echo $(pwd)/super >expect && >> git -C super/dir/sub rev-parse --show-superproject-working-tree >out && >> + test_cmp expect out && > > Here is an end of one subtest, deserves to have a break like the above. OK > >> + test_commit -C super submodule_add && >> + git -C super checkout -b branch1 && >> + git -C super/dir/sub checkout -b branch1 && >> + test_commit -C super/dir/sub branch1_commit && >> + git -C super add dir/sub && >> + test_commit -C super branch1_commit && >> + git -C super checkout master && >> + git -C super checkout -b branch2 && >> + git -C super/dir/sub checkout master && >> + git -C super/dir/sub checkout -b branch2 && >> + test_commit -C super/dir/sub branch2_commit && >> + git -C super add dir/sub && >> + test_commit -C super branch2_commit && >> + test_must_fail git -C super merge branch1 && > > and all of the above is just a set-up for another subtest, so a > solid block of text like we see in the above is good. > > Side note: there are a few of > > git -C $there checkout $onebranch && > git -C $there checkout -b $anotherbranch && > > as recurring pattern. Shouldn't they be more like a single > liner > > git -C $there checkout -b $anotherbranch $onebranch && > > ? It wasn't clear if the split was an attempt to hide some > breakage (e.g. "checkout -b B A" did not work but "checkout > A && checkout -b B" did) or just being verbose because the > author is not used to "checkout -b B A" form. You’re right, the two forms are equivalent and the single-line version is simpler. > >> + git -C super/dir/sub rev-parse --show-superproject-working-tree >out && >> test_cmp expect out >> ' > > Thanks. Thank you.
