Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree

[Sam McKelvie <sammck@gmail.com>]

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@pobox.com> wrote:
> 
> Sam McKelvie <sammck@gmail.com> 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@gmail.com>
>> ---
>> 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.