Re: [PATCH 4/4] submodule deinit: unset core.worktree

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

Stefan Beller <sbeller@google.com> writes:

> This re-introduces 984cd77ddb (submodule deinit: unset core.worktree,
> 2018-06-18), which was reverted as part of f178c13fda (Revert "Merge
> branch 'sb/submodule-core-worktree'", 2018-09-07)
>
> The whole series was reverted as the offending commit e98317508c
> (submodule: ensure core.worktree is set after update, 2018-06-18)
> was relied on by other commits such as 984cd77ddb.
>
> Keep the offending commit reverted, but its functionality came back via
> 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17), such
> that we can reintroduce 984cd77ddb now.

None of the above three explains the most important thing directly,
so readers fail to grasp what the main theme of the three-patch
series is, without looking at the commits that were reverted
already.

Is the theme of the overall series to make sure core.worktree is set
to point at the working tree when submodule's working tree is
instantiated, and unset it when it is not?

2/4 was also explained (in the original) that it wants to unset and
did so when "move_head" gets called.  This one does the unset when a
submodule is deinited.  Are these the only two cases a submodule
loses its working tree?  If so, the log message for this step should
declare victory by ending with something like

	... as we covered the only other case in which a submodule
	loses its working tree in the earlier step (i.e. switching
	branches of top-level project to move to a commit that did
	not have the submodule), this makes the code always maintain
	core.worktree correctly unset when there is no working tree
	for a submodule.

Thanks.  I think I agree with what the series wants to do (if I read
what it wants to do correctly, that is).

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/submodule--helper.c | 2 ++
>  t/lib-submodule-update.sh   | 2 +-
>  t/t7400-submodule-basic.sh  | 5 +++++
>  3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 31ac30cf2f..672b74db89 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1131,6 +1131,8 @@ static void deinit_submodule(const char *path, const char *prefix,
>  		if (!(flags & OPT_QUIET))
>  			printf(format, displaypath);
>  
> +		submodule_unset_core_worktree(sub);
> +
>  		strbuf_release(&sb_rm);
>  	}
>  
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index 51d4555549..5b56b23166 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -235,7 +235,7 @@ reset_work_tree_to_interested () {
>  	then
>  		mkdir -p submodule_update/.git/modules/sub1/modules &&
>  		cp -r submodule_update_repo/.git/modules/sub1/modules/sub2 submodule_update/.git/modules/sub1/modules/sub2
> -		GIT_WORK_TREE=. git -C submodule_update/.git/modules/sub1/modules/sub2 config --unset core.worktree
> +		# core.worktree is unset for sub2 as it is not checked out
>  	fi &&
>  	# indicate we are interested in the submodule:
>  	git -C submodule_update config submodule.sub1.url "bogus" &&
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 76a7cb0af7..aba2d4d6ee 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -984,6 +984,11 @@ test_expect_success 'submodule deinit should remove the whole submodule section
>  	rmdir init
>  '
>  
> +test_expect_success 'submodule deinit should unset core.worktree' '
> +	test_path_is_file .git/modules/example/config &&
> +	test_must_fail git config -f .git/modules/example/config core.worktree
> +'
> +
>  test_expect_success 'submodule deinit from subdirectory' '
>  	git submodule update --init &&
>  	git config submodule.example.foo bar &&