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

[Junio C Hamano <gitster@pobox.com>]

Sam McKelvie <sammck@gmail.com> writes:

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

s/Alllow/allow/;

> 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 ...

>  	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.

> +	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.

> +	git -C super/dir/sub rev-parse --show-superproject-working-tree >out &&
>  	test_cmp expect out
>  '

Thanks.