[PATCH 0/4]

[PATCH 0/4]

[Stefan Beller]

A couple days before the 2.19 release we had a bug report about
broken submodules[1] and reverted[2] the commits leading up to them.

The behavior of said bug fixed itself by taking a different approach[3],
specifically by a weaker enforcement of having `core.worktree` set in a
submodule [4].

The revert [2] was overly broad as we neared the release, such that we wanted
to rather keep the known buggy behavior of always having `core.worktree` set,
rather than figuring out how to fix the new bug of having 'git submodule update'
not working in old style repository setups.

This series re-introduces those reverted patches, with no changes in code,
but with drastically changed commit messages, as those focus on why it is safe
to re-introduce them instead of explaining the desire for the change.

[1] https://public-inbox.org/git/2659750.rG6xLiZASK@twilight
[2] f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'", 2018-09-07)
[3] 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17)
[4] 74d4731da1 (submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13)

Stefan Beller (4):
  submodule update: add regression test with old style setups
  submodule: unset core.worktree if no working tree is present
  submodule--helper: fix BUG message in ensure_core_worktree
  submodule deinit: unset core.worktree

 builtin/submodule--helper.c        |  4 +++-
 submodule.c                        | 14 ++++++++++++++
 submodule.h                        |  2 ++
 t/lib-submodule-update.sh          |  5 +++--
 t/t7400-submodule-basic.sh         |  5 +++++
 t/t7412-submodule-absorbgitdirs.sh |  7 ++++++-
 6 files changed, 33 insertions(+), 4 deletions(-)

-- 
2.20.0.rc2.403.gdbc3b29805-goog

[PATCH 1/4] submodule update: add regression test with old style setups

[Stefan Beller]

As f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'",
2018-09-07) was produced shortly before a release, nobody asked for
a regression test to be included. Add a regression test that makes sure
that the invocation of `git submodule update` on old setups doesn't
produce errors as pointed out in f178c13fda.

The place to add such a regression test may look odd in t7412, but
that is the best place as there we setup old style submodule setups
explicitly.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7412-submodule-absorbgitdirs.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
index ce74c12da2..1cfa150768 100755
--- a/t/t7412-submodule-absorbgitdirs.sh
+++ b/t/t7412-submodule-absorbgitdirs.sh
@@ -75,7 +75,12 @@ test_expect_success 're-setup nested submodule' '
 	GIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \
 		core.worktree "../../../nested" &&
 	# make sure this re-setup is correct
-	git status --ignore-submodules=none
+	git status --ignore-submodules=none &&
+
+	# also make sure this old setup does not regress
+	git submodule update --init --recursive >out 2>err &&
+	test_must_be_empty out &&
+	test_must_be_empty err
 '
 
 test_expect_success 'absorb the git dir in a nested submodule' '
-- 
2.20.0.rc2.403.gdbc3b29805-goog

Re: [PATCH 1/4] submodule update: add regression test with old style setups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> As f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'",
> 2018-09-07) was produced shortly before a release, nobody asked for
> a regression test to be included. Add a regression test that makes sure
> that the invocation of `git submodule update` on old setups doesn't
> produce errors as pointed out in f178c13fda.
>
> The place to add such a regression test may look odd in t7412, but
> that is the best place as there we setup old style submodule setups
> explicitly.

Very good first step.  Thanks.

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t7412-submodule-absorbgitdirs.sh | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
> index ce74c12da2..1cfa150768 100755
> --- a/t/t7412-submodule-absorbgitdirs.sh
> +++ b/t/t7412-submodule-absorbgitdirs.sh
> @@ -75,7 +75,12 @@ test_expect_success 're-setup nested submodule' '
>  	GIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \
>  		core.worktree "../../../nested" &&
>  	# make sure this re-setup is correct
> -	git status --ignore-submodules=none
> +	git status --ignore-submodules=none &&
> +
> +	# also make sure this old setup does not regress
> +	git submodule update --init --recursive >out 2>err &&
> +	test_must_be_empty out &&
> +	test_must_be_empty err
>  '
>  
>  test_expect_success 'absorb the git dir in a nested submodule' '

[PATCH 2/4] submodule: unset core.worktree if no working tree is present

[Stefan Beller]

This reintroduces 4fa4f90ccd (submodule: unset core.worktree if no working
tree is present, 2018-06-12), which was reverted as part of f178c13fda
(Revert "Merge branch 'sb/submodule-core-worktree'", 2018-09-07).

4fa4f90ccd was reverted as its followup commit was faulty, but without
the accompanying change of the followup, we'd have an incomplete workflow
of setting `core.worktree` again, when it is needed such as checking out
a revision that contains a submodule.

So re-introduce that commit as-is, focusing on fixing up the followup

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c               | 14 ++++++++++++++
 submodule.h               |  2 ++
 t/lib-submodule-update.sh |  3 ++-
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index 6415cc5580..d393e947e6 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1561,6 +1561,18 @@ 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();
@@ -1726,6 +1738,8 @@ 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 a680214c01..9e18e9b807 100644
--- a/submodule.h
+++ b/submodule.h
@@ -131,6 +131,8 @@ 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 016391723c..51d4555549 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -709,7 +709,8 @@ 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 -e sub1 &&
+			test_must_fail git config -f .git/modules/sub1/config core.worktree
 		)
 	'
 	# ... absorbing a .git directory along the way.
-- 
2.20.0.rc2.403.gdbc3b29805-goog

Re: [PATCH 2/4] submodule: unset core.worktree if no working tree is present

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> This reintroduces 4fa4f90ccd (submodule: unset core.worktree if no working
> tree is present, 2018-06-12), which was reverted as part of f178c13fda
> (Revert "Merge branch 'sb/submodule-core-worktree'", 2018-09-07).
>
> 4fa4f90ccd was reverted as its followup commit was faulty, but without
> the accompanying change of the followup, we'd have an incomplete workflow
> of setting `core.worktree` again, when it is needed such as checking out
> a revision that contains a submodule.
>
> So re-introduce that commit as-is, focusing on fixing up the followup

I was hoping to hear (given what 0/4 claimed) a clearer explanation
of what this change wants to achieve, but that is lacking.

No need to grumble about an earlier work was that turned out to be
inappropriate for the codebase back then.  Repeatedly saying "this
is needed" without giving further explaining why it is so, or
anything like that, would help readers.

Just pretend that the ealier commits and their reversion never
happened, and further pretend that we are doing the best thing that
should happen to our codebase.  How would we explain this change,
what the problem it tries to solve and what the solution looks like
in the larger picture?

	When removing a working tree of a submodule (e.g. we may be
	switching back to an earlier commit in the superproject that
	did not have the submodule in question yet), we failed to
	unset core.worktree of the submodule's repository.  That
	caused this and that issues, exhibited by a few new tests
	this commit adds.

	Make sure that core.worktree gets unset so that a leftover
	setting won't cause these issues.

or something like that?  I am just guessing by looking at the old
commit's text, as the above two paragraphs and one sentence does not
say much.

> diff --git a/submodule.c b/submodule.c
> index 6415cc5580..d393e947e6 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1561,6 +1561,18 @@ 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();
> @@ -1726,6 +1738,8 @@ 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 a680214c01..9e18e9b807 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -131,6 +131,8 @@ 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 016391723c..51d4555549 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -709,7 +709,8 @@ 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 -e sub1 &&
> +			test_must_fail git config -f .git/modules/sub1/config core.worktree
>  		)
>  	'
>  	# ... absorbing a .git directory along the way.

[PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree

[Stefan Beller]

Shortly after f178c13fda (Revert "Merge branch
'sb/submodule-core-worktree'", 2018-09-07), we had another series
that implemented partially the same, ensuring that core.worktree was
set in a checked out submodule, namely 74d4731da1 (submodule--helper:
replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13)

As the series 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c',
2018-09-17) has different goals than the reverted series 7e25437d35
(Merge branch 'sb/submodule-core-worktree', 2018-07-18), I'd wanted to
replay the series on top of it to reach the goal of having `core.worktree`
correctly set when the submodules worktree is present, and unset when the
worktree is not present.

The replay resulted in a strange merge conflict highlighting that
the BUG message was not changed in 74d4731da1 (submodule--helper:
replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13).

Fix the error message.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d38113a31a..31ac30cf2f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2045,7 +2045,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)
 	struct repository subrepo;
 
 	if (argc != 2)
-		BUG("submodule--helper connect-gitdir-workingtree <name> <path>");
+		BUG("submodule--helper ensure-core-worktree <path>");
 
 	path = argv[1];
 
-- 
2.20.0.rc2.403.gdbc3b29805-goog

Re: [PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Shortly after f178c13fda (Revert "Merge branch
> 'sb/submodule-core-worktree'", 2018-09-07), we had another series
> that implemented partially the same, ensuring that core.worktree was
> set in a checked out submodule, namely 74d4731da1 (submodule--helper:
> replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13)
>
> As the series 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c',
> 2018-09-17) has different goals than the reverted series 7e25437d35
> (Merge branch 'sb/submodule-core-worktree', 2018-07-18), I'd wanted to
> replay the series on top of it to reach the goal of having `core.worktree`
> correctly set when the submodules worktree is present, and unset when the
> worktree is not present.
>
> The replay resulted in a strange merge conflict highlighting that
> the BUG message was not changed in 74d4731da1 (submodule--helper:
> replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13).
>
> Fix the error message.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Unlike the step 2/4 I commented on, this does explain what this
wants to do and why, at least when looked from sideways.  Is the
above saying the same as the following two-liner?

	An ealier mistake while rebasing to produce 74d4731da1
	failed to update this BUG message.  Fix this.



>  builtin/submodule--helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d38113a31a..31ac30cf2f 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2045,7 +2045,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)
>  	struct repository subrepo;
>  
>  	if (argc != 2)
> -		BUG("submodule--helper connect-gitdir-workingtree <name> <path>");
> +		BUG("submodule--helper ensure-core-worktree <path>");
>  
>  	path = argv[1];

Re: [PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree

[Stefan Beller]

> Unlike the step 2/4 I commented on, this does explain what this
> wants to do and why, at least when looked from sideways.  Is the
> above saying the same as the following two-liner?
>
>         An ealier mistake while rebasing to produce 74d4731da1
>         failed to update this BUG message.  Fix this.

I am not sure if it was rebasing, which was executed mistakenly.
So maybe just saying "74d4731da1 contains a faulty BUG
message. Fix it." would do.

The intent of the longer message was to shed light in how I found
the BUG (ie. I did not see the BUG message, which would ask me
to actually fix a bug, but found it via code inspection), which I
thought was valuable information, too.

Re: [PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> Unlike the step 2/4 I commented on, this does explain what this
>> wants to do and why, at least when looked from sideways.  Is the
>> above saying the same as the following two-liner?
>>
>>         An ealier mistake while rebasing to produce 74d4731da1
>>         failed to update this BUG message.  Fix this.
>
> I am not sure if it was rebasing, which was executed mistakenly.
> So maybe just saying "74d4731da1 contains a faulty BUG
> message. Fix it." would do.
>
> The intent of the longer message was to shed light in how I found
> the BUG (ie. I did not see the BUG message, which would ask me
> to actually fix a bug, but found it via code inspection), which I
> thought was valuable information, too.

I guess that it could be stated in a way to make it valuable, but in
the presented text, I somehow found it was making the more important
part of the description (i.e. "this patch fixes a mistake made by
74d4731da1") buried and harder to grok.

Thanks.

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

[Stefan Beller]

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.

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 &&
-- 
2.20.0.rc2.403.gdbc3b29805-goog

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

[Junio C Hamano]

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

Re: [PATCH 0/4]

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> A couple days before the 2.19 release we had a bug report about
> broken submodules[1] and reverted[2] the commits leading up to them.
>
> The behavior of said bug fixed itself by taking a different approach[3],
> specifically by a weaker enforcement of having `core.worktree` set in a
> submodule [4].
>
> The revert [2] was overly broad as we neared the release, such that we wanted
> to rather keep the known buggy behavior of always having `core.worktree` set,
> rather than figuring out how to fix the new bug of having 'git submodule update'
> not working in old style repository setups.
>
> This series re-introduces those reverted patches, with no changes in code,
> but with drastically changed commit messages, as those focus on why it is safe
> to re-introduce them instead of explaining the desire for the change.

The above was a bit too cryptic for me to grok, so let me try
rephrasing to see if I got them all correctly.

 - three-patch series leading to 984cd77ddb were meant to fix some
   bug, but the series itself was buggy and caused problems; we got
   rid of them

 - the problem 984cd77ddb wanted to fix was fixed differently
   without reintroducing the problem three-patch series introduced.
   That fix is already with us since 4d6d6ef1fc.

 - now these three changes that were problematic in the past is
   resent without any update (other than that it has one preparatory
   patch to add tests).

Is that what is going on?  Obviously I am not getting "the other"
benefit we wanted to gain out of these three patches (because the
above description fails to explain what that is), other than to fix
the issue that was fixed by 4d6d6ef1fc.

Sorry for being puzzled...

> [1] https://public-inbox.org/git/2659750.rG6xLiZASK@twilight
> [2] f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'", 2018-09-07)
> [3] 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17)
> [4] 74d4731da1 (submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13)
>
> Stefan Beller (4):
>   submodule update: add regression test with old style setups
>   submodule: unset core.worktree if no working tree is present
>   submodule--helper: fix BUG message in ensure_core_worktree
>   submodule deinit: unset core.worktree
>
>  builtin/submodule--helper.c        |  4 +++-
>  submodule.c                        | 14 ++++++++++++++
>  submodule.h                        |  2 ++
>  t/lib-submodule-update.sh          |  5 +++--
>  t/t7400-submodule-basic.sh         |  5 +++++
>  t/t7412-submodule-absorbgitdirs.sh |  7 ++++++-
>  6 files changed, 33 insertions(+), 4 deletions(-)

Re: [PATCH 0/4]

[Stefan Beller]

On Fri, Dec 7, 2018 at 9:57 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Beller <sbeller@google.com> writes:
>
> > A couple days before the 2.19 release we had a bug report about
> > broken submodules[1] and reverted[2] the commits leading up to them.
> >
> > The behavior of said bug fixed itself by taking a different approach[3],
> > specifically by a weaker enforcement of having `core.worktree` set in a
> > submodule [4].
> >
> > The revert [2] was overly broad as we neared the release, such that we wanted
> > to rather keep the known buggy behavior of always having `core.worktree` set,
> > rather than figuring out how to fix the new bug of having 'git submodule update'
> > not working in old style repository setups.
> >
> > This series re-introduces those reverted patches, with no changes in code,
> > but with drastically changed commit messages, as those focus on why it is safe
> > to re-introduce them instead of explaining the desire for the change.
>
> The above was a bit too cryptic for me to grok, so let me try
> rephrasing to see if I got them all correctly.
>
>  - three-patch series leading to 984cd77ddb were meant to fix some
>    bug, but the series itself was buggy and caused problems; we got
>    rid of them

yes.

>  - the problem 984cd77ddb wanted to fix was fixed differently

e98317508c02*

>    without reintroducing the problem three-patch series introduced.
>    That fix is already with us since 4d6d6ef1fc.

yes.

>  - now these three changes that were problematic in the past is
>    resent without any update (other than that it has one preparatory
>    patch to add tests).

One of the three changes was problematic, (e98317508c02),
the other two are good (in company of the third).

But those two were not good on their own, which is why we
reverted all three at once.

Now that we have a different approach for the third,
we could re-introduce the two.
(4fa4f90ccd8, 984cd77ddbf0)

We do that, but with precaution (an extra test);
additional careful reading found a typo, hence
we have "a third" patch, but that is totally different
than what above was referred to "one of three".


> Is that what is going on?  Obviously I am not getting "the other"
> benefit we wanted to gain out of these three patches (because the
> above description fails to explain what that is), other than to fix
> the issue that was fixed by 4d6d6ef1fc.

The other benefit refers to
7e25437d35 (Merge branch 'sb/submodule-core-worktree', 2018-07-18)
which was reverted as a whole.
It's goal was to handle core.worktree appropriately.

(Instead of having it there all the time, only have it when
a working tree is present)

> Sorry for being puzzled...

This means I need to revamp the commit messages and
cover letter altogether.

Stefan

Re: [PATCH 0/4]

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>>  - now these three changes that were problematic in the past is
>>    resent without any update (other than that it has one preparatory
>>    patch to add tests).
>
> One of the three changes was problematic, (e98317508c02),
> the other two are good (in company of the third).

Ah, that is what I failed to read.

> This means I need to revamp the commit messages and
> cover letter altogether.

I guess that would help future readers.  Thanks.

[PATCH 0/4] submodules: unset core.worktree when no working tree present

[Stefan Beller]

v2:
I reworded the commit messages to explain the patches from the ground up
instead of only linking to the old commits, that got reverted.

> Just pretend that the ealier commits and their reversion never
> happened, and further pretend that we are doing the best thing that
> should happen to our codebase.

I disagree with that first stance (I can freely admit those commits happened),
but agree on the second point, so I explained why the code is the best
for the code base now. So I kept those pointers in there, too, to make it
easier for future code archeologists. 

v1:

A couple days before the 2.19 release we had a bug report about
broken submodules[1] and reverted[2] the commits leading up to them.

The behavior of said bug fixed itself by taking a different approach[3],
specifically by a weaker enforcement of having `core.worktree` set in a
submodule [4].

The revert [2] was overly broad as we neared the release, such that we wanted
to rather keep the known buggy behavior of always having `core.worktree` set,
rather than figuring out how to fix the new bug of having 'git submodule update'
not working in old style repository setups.

This series re-introduces those reverted patches, with no changes in code,
but with drastically changed commit messages, as those focus on why it is safe
to re-introduce them instead of explaining the desire for the change.

[1] https://public-inbox.org/git/2659750.rG6xLiZASK@twilight
[2] f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'", 2018-09-07)
[3] 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17)
[4] 74d4731da1 (submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13)
Stefan Beller (4):
  submodule update: add regression test with old style setups
  submodule: unset core.worktree if no working tree is present
  submodule--helper: fix BUG message in ensure_core_worktree
  submodule deinit: unset core.worktree

 builtin/submodule--helper.c        |  4 +++-
 submodule.c                        | 14 ++++++++++++++
 submodule.h                        |  2 ++
 t/lib-submodule-update.sh          |  5 +++--
 t/t7400-submodule-basic.sh         |  5 +++++
 t/t7412-submodule-absorbgitdirs.sh |  7 ++++++-
 6 files changed, 33 insertions(+), 4 deletions(-)

-- 
2.20.0.405.gbc1bbc6f85-goog

[PATCH 1/4] submodule update: add regression test with old style setups

[Stefan Beller]

As f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'",
2018-09-07) was produced shortly before a release, nobody asked for
a regression test to be included. Add a regression test that makes sure
that the invocation of `git submodule update` on old setups doesn't
produce errors as pointed out in f178c13fda.

The place to add such a regression test may look odd in t7412, but
that is the best place as there we setup old style submodule setups
explicitly.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7412-submodule-absorbgitdirs.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
index ce74c12da2..1cfa150768 100755
--- a/t/t7412-submodule-absorbgitdirs.sh
+++ b/t/t7412-submodule-absorbgitdirs.sh
@@ -75,7 +75,12 @@ test_expect_success 're-setup nested submodule' '
 	GIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \
 		core.worktree "../../../nested" &&
 	# make sure this re-setup is correct
-	git status --ignore-submodules=none
+	git status --ignore-submodules=none &&
+
+	# also make sure this old setup does not regress
+	git submodule update --init --recursive >out 2>err &&
+	test_must_be_empty out &&
+	test_must_be_empty err
 '
 
 test_expect_success 'absorb the git dir in a nested submodule' '
-- 
2.20.0.405.gbc1bbc6f85-goog

Re: [PATCH 1/4] submodule update: add regression test with old style setups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The place to add such a regression test may look odd in t7412, but
> that is the best place as there we setup old style submodule setups
> explicitly.

Makes sense; thanks.

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t7412-submodule-absorbgitdirs.sh | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
> index ce74c12da2..1cfa150768 100755
> --- a/t/t7412-submodule-absorbgitdirs.sh
> +++ b/t/t7412-submodule-absorbgitdirs.sh
> @@ -75,7 +75,12 @@ test_expect_success 're-setup nested submodule' '
>  	GIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \
>  		core.worktree "../../../nested" &&
>  	# make sure this re-setup is correct
> -	git status --ignore-submodules=none
> +	git status --ignore-submodules=none &&
> +
> +	# also make sure this old setup does not regress
> +	git submodule update --init --recursive >out 2>err &&
> +	test_must_be_empty out &&
> +	test_must_be_empty err
>  '
>  
>  test_expect_success 'absorb the git dir in a nested submodule' '

[PATCH 2/4] submodule: unset core.worktree if no working tree is present

[Stefan Beller]

When a submodules work tree is removed, we should unset its core.worktree
setting as the worktree is no longer present. This is not just in line
with the conceptual view of submodules, but it fixes an inconvenience
for looking at submodules that are not checked out:

    git clone --recurse-submodules git://github.com/git/git && cd git &&
    git checkout --recurse-submodules v2.13.0
    git -C .git/modules/sha1collisiondetection log
    fatal: cannot chdir to '../../../sha1collisiondetection': \
        No such file or directory

With this patch applied, the final call to git log works instead of dying
in its setup, as the checkout will unset the core.worktree setting such
that following log will be run in a bare repository.

This patch covers all commands that are in the unpack machinery, i.e.
checkout, read-tree, reset. A follow up patch will address
"git submodule deinit", which will also make use of the new function
submodule_unset_core_worktree(), which is why we expose it in this patch.

This patch was authored as 4fa4f90ccd (submodule: unset core.worktree if
no working tree is present, 2018-06-12), which was reverted as part of
f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'",
2018-09-07). The revert was needed as the nearby commit e98317508c
(submodule: ensure core.worktree is set after update, 2018-06-18) is
faulty and at the time of 7e25437d35 (Merge branch
'sb/submodule-core-worktree', 2018-07-18) we could not revert the faulty
commit only, as they were depending on each other: If core.worktree is
unset, we have to have ways to ensure that it is set again once
the working tree reappears again.

Now that 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17),
specifically 74d4731da1 (submodule--helper: replace
connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13) is
present, we already check and ensure core.worktree is set when
populating a new work tree, such that we can re-introduce the commits
that unset core.worktree when removing the worktree.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c               | 14 ++++++++++++++
 submodule.h               |  2 ++
 t/lib-submodule-update.sh |  3 ++-
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index 6415cc5580..d393e947e6 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1561,6 +1561,18 @@ 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();
@@ -1726,6 +1738,8 @@ 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 a680214c01..9e18e9b807 100644
--- a/submodule.h
+++ b/submodule.h
@@ -131,6 +131,8 @@ 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 016391723c..51d4555549 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -709,7 +709,8 @@ 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 -e sub1 &&
+			test_must_fail git config -f .git/modules/sub1/config core.worktree
 		)
 	'
 	# ... absorbing a .git directory along the way.
-- 
2.20.0.405.gbc1bbc6f85-goog

Re: [PATCH 2/4] submodule: unset core.worktree if no working tree is present

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 2018-09-07). The revert was needed as the nearby commit e98317508c
> (submodule: ensure core.worktree is set after update, 2018-06-18) is
> faulty and at the time of 7e25437d35 (Merge branch
> 'sb/submodule-core-worktree', 2018-07-18) we could not revert the faulty
> commit only, as they were depending on each other: If core.worktree is
> unset, we have to have ways to ensure that it is set again once
> the working tree reappears again.
>
> Now that 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17),
> specifically 74d4731da1 (submodule--helper: replace
> connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13) is
> present, we already check and ensure core.worktree is set when
> populating a new work tree, such that we can re-introduce the commits
> that unset core.worktree when removing the worktree.

Cleanly explained.  Will queue.  Thanks.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule.c               | 14 ++++++++++++++
>  submodule.h               |  2 ++
>  t/lib-submodule-update.sh |  3 ++-
>  3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/submodule.c b/submodule.c
> index 6415cc5580..d393e947e6 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1561,6 +1561,18 @@ 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();
> @@ -1726,6 +1738,8 @@ 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 a680214c01..9e18e9b807 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -131,6 +131,8 @@ 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 016391723c..51d4555549 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -709,7 +709,8 @@ 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 -e sub1 &&
> +			test_must_fail git config -f .git/modules/sub1/config core.worktree
>  		)
>  	'
>  	# ... absorbing a .git directory along the way.

[PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree

[Stefan Beller]

74d4731da1 (submodule--helper: replace connect-gitdir-workingtree by
ensure-core-worktree, 2018-08-13) missed to update the BUG message.
Fix it.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d38113a31a..31ac30cf2f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2045,7 +2045,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)
 	struct repository subrepo;
 
 	if (argc != 2)
-		BUG("submodule--helper connect-gitdir-workingtree <name> <path>");
+		BUG("submodule--helper ensure-core-worktree <path>");
 
 	path = argv[1];
 
-- 
2.20.0.405.gbc1bbc6f85-goog

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

[Stefan Beller]

When a submodule is deinit'd, the working tree is gone, so the setting of
core.worktree is bogus. Unset it. 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.

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.

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 &&
-- 
2.20.0.405.gbc1bbc6f85-goog