Re: [PATCH] Revert "Merge branch 'sb/submodule-core-worktree'" (was Re: Old submodules broken in 2.19rc1 and 2.19rc2)

From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: Stefan Beller <sbeller@google.com>,
	allan.jensen@qt.io, git <git@vger.kernel.org>
Subject: Re: [PATCH] Revert "Merge branch 'sb/submodule-core-worktree'" (was Re: Old submodules broken in 2.19rc1 and 2.19rc2)
Date: Fri, 07 Sep 2018 19:04:01 -0700	[thread overview]
Message-ID: <xmqqmussvj72.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180908000946.GA225427@aiede.svl.corp.google.com> (Jonathan Nieder's message of "Fri, 7 Sep 2018 17:09:46 -0700")

Jonathan Nieder <jrnieder@gmail.com> writes:

> It is late in the release cycle, so revert the whole 3-patch series.
> We can try again later for 2.20.
>
> Reported-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
> Helped-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Stefan Beller wrote:
>> Jonathan Nieder wrote:
>
>>> I think we
>>> should revert e98317508c0 in "master" (for 2.19) and keep making use
>>> of that 'second try' in "next" (for 2.20).
>>
>> Actually I'd rather revert the whole topic leading up to
>> 7e25437d35a (Merge branch 'sb/submodule-core-worktree', 2018-07-18)
>> as the last patch in there doesn't work well without e98317508c0 IIRC.
>>
>> And having only the first patch would bring an inconsistent state as
>> then different commands behave differently w.r.t. setting core.worktree.
>
> Like this (generated using "git revert -m1)?

OK.  Thanks for taking care of it.

>
>  builtin/submodule--helper.c | 26 --------------------------
>  git-submodule.sh            |  5 -----
>  submodule.c                 | 14 --------------
>  submodule.h                 |  2 --
>  t/lib-submodule-update.sh   |  5 ++---
>  t/t7400-submodule-basic.sh  |  5 -----
>  6 files changed, 2 insertions(+), 55 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index b56028ba9d..f6fb8991f3 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1123,8 +1123,6 @@ 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);
>  	}
>  
> @@ -2005,29 +2003,6 @@ static int check_name(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> -static int connect_gitdir_workingtree(int argc, const char **argv, const char *prefix)
> -{
> -	struct strbuf sb = STRBUF_INIT;
> -	const char *name, *path;
> -	char *sm_gitdir;
> -
> -	if (argc != 3)
> -		BUG("submodule--helper connect-gitdir-workingtree <name> <path>");
> -
> -	name = argv[1];
> -	path = argv[2];
> -
> -	strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
> -	sm_gitdir = absolute_pathdup(sb.buf);
> -
> -	connect_work_tree_and_git_dir(path, sm_gitdir, 0);
> -
> -	strbuf_release(&sb);
> -	free(sm_gitdir);
> -
> -	return 0;
> -}
> -
>  #define SUPPORT_SUPER_PREFIX (1<<0)
>  
>  struct cmd_struct {
> @@ -2041,7 +2016,6 @@ static struct cmd_struct commands[] = {
>  	{"name", module_name, 0},
>  	{"clone", module_clone, 0},
>  	{"update-clone", update_clone, 0},
> -	{"connect-gitdir-workingtree", connect_gitdir_workingtree, 0},
>  	{"relative-path", resolve_relative_path, 0},
>  	{"resolve-relative-url", resolve_relative_url, 0},
>  	{"resolve-relative-url-test", resolve_relative_url_test, 0},
> diff --git a/git-submodule.sh b/git-submodule.sh
> index f7fd80345c..1cb2c0a31b 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -580,11 +580,6 @@ cmd_update()
>  			die "$(eval_gettext "Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
>  		fi
>  
> -		if ! $(git config -f "$(git rev-parse --git-common-dir)/modules/$name/config" core.worktree) 2>/dev/null
> -		then
> -			git submodule--helper connect-gitdir-workingtree "$name" "$sm_path"
> -		fi
> -
>  		if test "$subsha1" != "$sha1" || test -n "$force"
>  		then
>  			subforce=$force
> diff --git a/submodule.c b/submodule.c
> index 50cbf5f13e..a2b266fbfa 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1534,18 +1534,6 @@ int bad_to_remove_submodule(const char *path, unsigned flags)
>  	return ret;
>  }
>  
> -void submodule_unset_core_worktree(const struct submodule *sub)
> -{
> -	char *config_path = xstrfmt("%s/modules/%s/config",
> -				    get_git_common_dir(), sub->name);
> -
> -	if (git_config_set_in_file_gently(config_path, "core.worktree", NULL))
> -		warning(_("Could not unset core.worktree setting in submodule '%s'"),
> -			  sub->path);
> -
> -	free(config_path);
> -}
> -
>  static const char *get_super_prefix_or_empty(void)
>  {
>  	const char *s = get_super_prefix();
> @@ -1711,8 +1699,6 @@ int submodule_move_head(const char *path,
>  
>  			if (is_empty_dir(path))
>  				rmdir_or_warn(path);
> -
> -			submodule_unset_core_worktree(sub);
>  		}
>  	}
>  out:
> diff --git a/submodule.h b/submodule.h
> index 7d476cefa7..e452919aa4 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -127,8 +127,6 @@ int submodule_move_head(const char *path,
>  			const char *new_head,
>  			unsigned flags);
>  
> -void submodule_unset_core_worktree(const struct submodule *sub);
> -
>  /*
>   * Prepare the "env_array" parameter of a "struct child_process" for executing
>   * a submodule by clearing any repo-specific environment variables, but
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index 5b56b23166..016391723c 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
> -		# core.worktree is unset for sub2 as it is not checked out
> +		GIT_WORK_TREE=. git -C submodule_update/.git/modules/sub1/modules/sub2 config --unset core.worktree
>  	fi &&
>  	# indicate we are interested in the submodule:
>  	git -C submodule_update config submodule.sub1.url "bogus" &&
> @@ -709,8 +709,7 @@ test_submodule_recursing_with_args_common() {
>  			git branch -t remove_sub1 origin/remove_sub1 &&
>  			$command remove_sub1 &&
>  			test_superproject_content origin/remove_sub1 &&
> -			! test -e sub1 &&
> -			test_must_fail git config -f .git/modules/sub1/config core.worktree
> +			! test -e sub1
>  		)
>  	'
>  	# ... absorbing a .git directory along the way.
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 7d3d984210..c0ffc1022a 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -984,11 +984,6 @@ 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 &&