[PATCH 0/5] Some submodule bugfixes and "reattaching detached HEADs"

The first three patches fix bugs in existing submodule code,
sort of independent from the last 2 patches, which are RFCs.



In submodules as of today you always end up with a detached HEAD,
which may be scary to people used to working on branches. (I myself
enjoy working with a detached HEAD).

The detached HEAD in a submodule is the correct in the submodule-as-a-lib
case, but in case you actively want to work inside submodules, you
may want to have proper branch support, e.g. when typing:

    git checkout --recuse-submodules master

you may want to have the submodules on the master branch as well.

There are a couple of challenges though:
* We'd probably want to pay attention to the branch configuration
  (.gitmodules submodule.NAME.branch to be "." or "foo")
* What if the submodule does not have a master branch?
  Jonathan proposed off list that we may just want to create the
  branch in the submodule. This is not implemented in this series.
* In case the branch exists in the submodule and doesn't point at the
  the object as recorded in the superproject, we may either want to 
  update the branch to point at the superproject record or we want to
  reject the "reattaching HEAD". Patch 4 provides the later.

Any other ideas on why this could be a bad idea or corner cases?

Thanks,
Stefan

Stefan Beller (5):
  submodule_move_head: fix leak of struct child_process
  submodule_move_head: prepare env for child correctly
  submodule: avoid auto-discovery in new working tree manipulator code
  submodule--helper: reattach-HEAD
  submodule_move_head: reattach submodule HEAD to branch if possible.

 builtin/submodule--helper.c | 12 ++++++
 submodule.c                 | 93 ++++++++++++++++++++++++++++++++++++++++-----
 submodule.h                 | 10 +++++
 3 files changed, 106 insertions(+), 9 deletions(-)

-- 
2.13.0.rc1.1.gbc33f0f778

While fixing the leak of `cp`, reuse it instead of having to declare
another struct child_process.

Signed-off-by: Stefan Beller <redacted>
---
 submodule.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/submodule.c b/submodule.c
index d3299e29c0..cd098cf12b 100644
--- a/submodule.c
+++ b/submodule.c

@@ -1466,17 +1466,19 @@ int submodule_move_head(const char *path,
 		goto out;
 	}
 
+	child_process_clear(&cp);
+
 	if (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {
 		if (new) {
-			struct child_process cp1 = CHILD_PROCESS_INIT;
+			child_process_init(&cp);
 			/* also set the HEAD accordingly */
-			cp1.git_cmd = 1;
-			cp1.no_stdin = 1;
-			cp1.dir = path;
+			cp.git_cmd = 1;
+			cp.no_stdin = 1;
+			cp.dir = path;
 
-			argv_array_pushl(&cp1.args, "update-ref", "HEAD", new, NULL);
+			argv_array_pushl(&cp.args, "update-ref", "HEAD", new, NULL);
 
-			if (run_command(&cp1)) {
+			if (run_command(&cp)) {
 				ret = -1;
 				goto out;
 			}

@@ -1492,6 +1494,7 @@ int submodule_move_head(const char *path,
 		}
 	}
 out:
+	child_process_clear(&cp);
 	return ret;
 }
 

-- 
2.13.0.rc1.1.gbc33f0f778

All commands that are run in a submodule, are run in a correct setup,
there is no need to prepare the environment without setting the GIT_DIR
variable. By setting the GIT_DIR variable we fix issues as discussed in
10f5c52656 (submodule: avoid auto-discovery in
prepare_submodule_repo_env(), 2016-09-01)

Signed-off-by: Stefan Beller <redacted>
---
 submodule.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/submodule.c b/submodule.c
index c7a7a33916..df03691199 100644
--- a/submodule.c
+++ b/submodule.c

@@ -1363,7 +1363,7 @@ static int submodule_has_dirty_index(const struct submodule *sub)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 
-	prepare_submodule_repo_env_no_git_dir(&cp.env_array);
+	prepare_submodule_repo_env(&cp.env_array);
 
 	cp.git_cmd = 1;
 	argv_array_pushl(&cp.args, "diff-index", "--quiet",

@@ -1380,7 +1380,7 @@ static int submodule_has_dirty_index(const struct submodule *sub)
 static void submodule_reset_index(const char *path)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
-	prepare_submodule_repo_env_no_git_dir(&cp.env_array);
+	prepare_submodule_repo_env(&cp.env_array);
 
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;

@@ -1438,7 +1438,7 @@ int submodule_move_head(const char *path,
 		}
 	}
 
-	prepare_submodule_repo_env_no_git_dir(&cp.env_array);
+	prepare_submodule_repo_env(&cp.env_array);
 
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;

-- 
2.13.0.rc1.1.gbc33f0f778

We forgot to prepare the submodule env, which is only a problem for
nested submodules. See 2e5d6503bd (ls-files: fix recurse-submodules
with nested submodules, 2017-04-13) for further explanation.

Signed-off-by: Stefan Beller <redacted>
---
 submodule.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/submodule.c b/submodule.c
index cd098cf12b..c7a7a33916 100644
--- a/submodule.c
+++ b/submodule.c

@@ -1476,6 +1476,7 @@ int submodule_move_head(const char *path,
 			cp.no_stdin = 1;
 			cp.dir = path;
 
+			prepare_submodule_repo_env(&cp.env_array);
 			argv_array_pushl(&cp.args, "update-ref", "HEAD", new, NULL);
 
 			if (run_command(&cp)) {

-- 
2.13.0.rc1.1.gbc33f0f778

Add a new command, that reattaches a detached HEAD to its configured
branch in a submodule.

In a later patch we will teach git-submodule as well as other submodule
worktree manipulators (git reset/checkout --recurse-submodules) to not
end up in a detached HEAD state in the submodules.

Signed-off-by: Stefan Beller <redacted>
---
 builtin/submodule--helper.c | 12 ++++++++
 submodule.c                 | 69 +++++++++++++++++++++++++++++++++++++++++++++
 submodule.h                 | 10 +++++++
 3 files changed, 91 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 36e4231821..645ceac999 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c

@@ -1196,6 +1196,17 @@ static int is_active(int argc, const char **argv, const char *prefix)
 	return !is_submodule_initialized(argv[1]);
 }
 
+static int cmd_reattach_HEAD(int argc, const char **argv, const char *prefix)
+{
+	if (argc != 2)
+		die("submodule--helper reattach-HEAD takes exactly 1 argument");
+
+	gitmodules_config();
+	git_config(submodule_config, NULL);
+
+	return reattach_HEAD(argv[1], REATTACH_HEAD_DIE_ON_ERROR);
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {

@@ -1217,6 +1228,7 @@ static struct cmd_struct commands[] = {
 	{"push-check", push_check, 0},
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
 	{"is-active", is_active, 0},
+	{"reattach-HEAD", cmd_reattach_HEAD, 0}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)

diff --git a/submodule.c b/submodule.c
index df03691199..4e74e38829 100644
--- a/submodule.c
+++ b/submodule.c

@@ -1499,6 +1499,75 @@ int submodule_move_head(const char *path,
 	return ret;
 }
 
+int reattach_HEAD(const char *submodule_path, int flags)
+{
+	const char *branch;
+	struct object_id sub_head_object;
+	struct object_id sub_branch_object;
+
+	const struct submodule *sub = submodule_from_path(null_sha1, submodule_path);
+
+	if (!sub) {
+		if (!(flags & REATTACH_HEAD_DIE_ON_ERROR))
+			return -1;
+		die(_("no submodule mapping found in .gitmodules for path '%s'"),
+			submodule_path);
+	}
+
+	if (!sub->branch) {
+		if (!(flags & REATTACH_HEAD_DIE_ON_ERROR))
+			return -1;
+		die(_("no branch configured to follow for submodule '%s'"),
+			sub->path);
+	}
+
+	/* lookup branch value in .gitmodules */
+	if (strcmp(".", sub->branch)) {
+		branch = sub->branch;
+	} else {
+		/* special care for '.': Is the superproject on a branch? */
+		struct object_id oid;
+		branch = resolve_refdup("HEAD", 0, oid.hash, NULL);
+		if (!branch) {
+			if (!(flags & REATTACH_HEAD_DIE_ON_ERROR))
+				return -1;
+			die(_("Not on any branch, but submodule configured to follow superprojects branch"));
+		}
+	}
+
+	if (!strcmp("HEAD", branch))
+		return 0;
+
+	resolve_gitlink_ref(sub->path, "HEAD", sub_head_object.hash);
+	resolve_gitlink_ref(sub->path, branch, sub_branch_object.hash);
+
+	if (!oidcmp(&sub_head_object, &sub_branch_object)) {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		struct strbuf reason = STRBUF_INIT;
+
+		cp.dir = sub->path;
+		prepare_submodule_repo_env(&cp.env_array);
+
+		strbuf_addf(&reason, "reattach HEAD to %s", branch);
+		argv_array_pushl(&cp.args, "git", "symbolic-ref",
+				 "-m", reason.buf, "HEAD", branch, NULL);
+		if (run_command(&cp)) {
+			if (!(flags & REATTACH_HEAD_DIE_ON_ERROR))
+				return -1;
+			die(_("could not run symbolic-ref in submodule '%s'"),
+			    sub->path);
+		}
+		strbuf_release(&reason);
+		return 0;
+	} else {
+		fprintf(stderr, "not reattaching HEAD, object ids differ:\n"
+				"HEAD is %s\n branch is %s",
+				oid_to_hex(&sub_head_object),
+				oid_to_hex(&sub_branch_object));
+		return 1;
+	}
+}
+
 static int find_first_merges(struct object_array *result, const char *path,
 		struct commit *a, struct commit *b)
 {

diff --git a/submodule.h b/submodule.h
index 1277480add..f7bb565a6d 100644
--- a/submodule.h
+++ b/submodule.h

@@ -119,6 +119,16 @@ extern int submodule_move_head(const char *path,
  */
 extern void prepare_submodule_repo_env(struct argv_array *out);
 
+/*
+ * Attach the submodules HEAD to its configured branch if the underlying
+ * object id are the same. Returns
+ * -  0 when the branch could be reattached.
+ * - +1 when the branch could not be reattached due to object name mismatch
+ * - <0 when an error occurred (e.g. missing .gitmodules file)
+ */
+#define REATTACH_HEAD_DIE_ON_ERROR (1<<0)
+extern int reattach_HEAD(const char *submodule_path, int flags);
+
 #define ABSORB_GITDIR_RECURSE_SUBMODULES (1<<0)
 extern void absorb_git_dir_into_superproject(const char *prefix,
 					     const char *path,

-- 
2.13.0.rc1.1.gbc33f0f778

Side note: We also want to call this from git submodule update

Signed-off-by: Stefan Beller <redacted>
---
 submodule.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/submodule.c b/submodule.c
index 4e74e38829..66651ffa34 100644
--- a/submodule.c
+++ b/submodule.c

@@ -1483,6 +1483,8 @@ int submodule_move_head(const char *path,
 				ret = -1;
 				goto out;
 			}
+
+			reattach_HEAD(path, 0);
 		} else {
 			struct strbuf sb = STRBUF_INIT;
 

-- 
2.13.0.rc1.1.gbc33f0f778

On 05/01, Stefan Beller wrote:

While fixing the leak of `cp`, reuse it instead of having to declare
another struct child_process.

Signed-off-by: Stefan Beller <redacted>

This shouldn't be needed as 'finish_command' does the cleanup for you by
calling 'child_prcoess_clear()'.

-- 
Brandon Williams

On 05/01, Stefan Beller wrote:

The first three patches fix bugs in existing submodule code,
sort of independent from the last 2 patches, which are RFCs.



In submodules as of today you always end up with a detached HEAD,
which may be scary to people used to working on branches. (I myself
enjoy working with a detached HEAD).

The detached HEAD in a submodule is the correct in the submodule-as-a-lib
case, but in case you actively want to work inside submodules, you
may want to have proper branch support, e.g. when typing:

    git checkout --recuse-submodules master

you may want to have the submodules on the master branch as well.

I don't know why submodules were originally designed to be in a
detached HEAD state but I much prefer working on branches (as I'm sure
many other developers do) so the prospect of this becoming the norm is
exciting! :D

There are a couple of challenges though:
* We'd probably want to pay attention to the branch configuration
  (.gitmodules submodule.NAME.branch to be "." or "foo")

Yes, I agree.  If we have a particular name of a branch configured to be
tracked, then we should respect that and try to attach HEAD onto that
branch name.

* What if the submodule does not have a master branch?
  Jonathan proposed off list that we may just want to create the
  branch in the submodule. This is not implemented in this series.

I think it would be fine to create the branch, just as long as it
doesn't already exist as I can't think of a negative impact of this
(aside from maybe having more branches in the submodules after a
prolonged time period?)

* In case the branch exists in the submodule and doesn't point at the
  the object as recorded in the superproject, we may either want to 
  update the branch to point at the superproject record or we want to
  reject the "reattaching HEAD". Patch 4 provides the later.

The later seems like the most correct thing to do.  It would be
unfortunate if I had done some work on top of the master branch in a
submodule (which wasn't reflected in the superproject), then done a
'checkout master' in the super project only to go back to the submodule
and find that all my work is gone! (Well not gone, but you'd have to go
into the scary reflog!)  It just makes sense to leave HEAD in a detached
state here as it prevents loss of work.

Any other ideas on why this could be a bad idea or corner cases?

Thanks,
Stefan

Stefan Beller (5):
  submodule_move_head: fix leak of struct child_process
  submodule_move_head: prepare env for child correctly
  submodule: avoid auto-discovery in new working tree manipulator code
  submodule--helper: reattach-HEAD
  submodule_move_head: reattach submodule HEAD to branch if possible.

 builtin/submodule--helper.c | 12 ++++++
 submodule.c                 | 93 ++++++++++++++++++++++++++++++++++++++++-----
 submodule.h                 | 10 +++++
 3 files changed, 106 insertions(+), 9 deletions(-)

-- 
2.13.0.rc1.1.gbc33f0f778

-- 
Brandon Williams

On 05/01, Stefan Beller wrote:

Add a new command, that reattaches a detached HEAD to its configured
branch in a submodule.

In a later patch we will teach git-submodule as well as other submodule
worktree manipulators (git reset/checkout --recurse-submodules) to not
end up in a detached HEAD state in the submodules.

Signed-off-by: Stefan Beller <redacted>
 
+int reattach_HEAD(const char *submodule_path, int flags)
+{
+	const char *branch;
+	struct object_id sub_head_object;
+	struct object_id sub_branch_object;
+
+	const struct submodule *sub = submodule_from_path(null_sha1, submodule_path);
+
+	if (!sub) {
+		if (!(flags & REATTACH_HEAD_DIE_ON_ERROR))
+			return -1;
+		die(_("no submodule mapping found in .gitmodules for path '%s'"),
+			submodule_path);

It may be a bit clearer to do the following:

  if (flags & REATTACH_HEAD_DIE_ON_ERROR)
    die(...);

  return -1;

It just feels weird to me to have the inverted logic, that's my opinion
though.

So this is the case where the superproject is in a detached-HEAD state?
In that case then we don't need to continue because if the superproject
isn't on a branch, then there isn't a reason the submodule should be on
a branch.

-- 
Brandon Williams

On Mon, May 1, 2017 at 11:36 AM, Brandon Williams [off-list ref] wrote:

  if (flags & REATTACH_HEAD_DIE_ON_ERROR)
    die(...);

  return -1;

It just feels weird to me to have the inverted logic, that's my opinion
though.

Yeah, me too. But my feelings were not as important as staying
under 80 characters a line. And as the error message is longer than
the "return -1", I wanted to have it not indented yet another level.

quoted

+     }
+
+     if (!sub->branch) {
+             if (!(flags & REATTACH_HEAD_DIE_ON_ERROR))
+                     return -1;
+             die(_("no branch configured to follow for submodule '%s'"),
+                     sub->path);
+     }
+
+     /* lookup branch value in .gitmodules */
+     if (strcmp(".", sub->branch)) {
+             branch = sub->branch;
+     } else {
+             /* special care for '.': Is the superproject on a branch? */
+             struct object_id oid;
+             branch = resolve_refdup("HEAD", 0, oid.hash, NULL);
+             if (!branch) {
+                     if (!(flags & REATTACH_HEAD_DIE_ON_ERROR))
+                             return -1;
+                     die(_("Not on any branch, but submodule configured to follow superprojects branch"));
+             }
+     }
+
+     if (!strcmp("HEAD", branch))
+             return 0;

So this is the case where the superproject is in a detached-HEAD state?

and the submodule config is branch='.'

In that case then we don't need to continue because if the superproject
isn't on a branch, then there isn't a reason the submodule should be on
a branch.

agreed.

v2:
 * I dropped the RFC patches for reattaching HEAD as that is not the main
   concern of this series.
 * patch1 is just about dropping cp1 to reusing cp, instead of additionally
   "fixing" mem leaks as finish_command does that for us
 * reordered the patches, former patch 3 now as patch 2 (avoid auto-discovery
   in new working tree manipulator code)
 * Regarding former patch2, I wrote:

Junio wrote:

quoted

Sounds good (if only because this makes it similar to other
codepaths).

Is this something whose breakage before the patch is easily
demonstrated with a test?

I'll try to come up with a test in a reroll.

 
done

Thanks,
Stefan


Stefan Beller (3):
  submodule_move_head: reuse child_process structure for futher commands
  submodule: avoid auto-discovery in new working tree manipulator code
  submodule: properly recurse for read-tree and checkout

 submodule.c                    | 21 +++++++++++----------
 t/lib-submodule-update.sh      |  7 +------
 t/t1013-read-tree-submodule.sh |  1 -
 t/t2013-checkout-submodule.sh  |  1 -
 4 files changed, 12 insertions(+), 18 deletions(-)

-- 
2.13.0.rc1.19.g083305f9b1

We do not need to declare another struct child_process, but we can just
reuse the existing `cp` struct.

Signed-off-by: Stefan Beller <redacted>
---
 submodule.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/submodule.c b/submodule.c
index d3299e29c0..a2377ce019 100644
--- a/submodule.c
+++ b/submodule.c

@@ -1468,15 +1468,15 @@ int submodule_move_head(const char *path,
 
 	if (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {
 		if (new) {
-			struct child_process cp1 = CHILD_PROCESS_INIT;
+			child_process_init(&cp);
 			/* also set the HEAD accordingly */
-			cp1.git_cmd = 1;
-			cp1.no_stdin = 1;
-			cp1.dir = path;
+			cp.git_cmd = 1;
+			cp.no_stdin = 1;
+			cp.dir = path;
 
-			argv_array_pushl(&cp1.args, "update-ref", "HEAD", new, NULL);
+			argv_array_pushl(&cp.args, "update-ref", "HEAD", new, NULL);
 
-			if (run_command(&cp1)) {
+			if (run_command(&cp)) {
 				ret = -1;
 				goto out;
 			}

-- 
2.13.0.rc1.19.g083305f9b1

All commands that are run in a submodule, are run in a correct setup,
there is no need to prepare the environment without setting the GIT_DIR
variable. By setting the GIT_DIR variable we fix issues as discussed in
10f5c52656 (submodule: avoid auto-discovery in
prepare_submodule_repo_env(), 2016-09-01)

Signed-off-by: Stefan Beller <redacted>
---
 submodule.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/submodule.c b/submodule.c
index a2377ce019..b0141a66dd 100644
--- a/submodule.c
+++ b/submodule.c

@@ -1363,7 +1363,7 @@ static int submodule_has_dirty_index(const struct submodule *sub)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 
-	prepare_submodule_repo_env_no_git_dir(&cp.env_array);
+	prepare_submodule_repo_env(&cp.env_array);
 
 	cp.git_cmd = 1;
 	argv_array_pushl(&cp.args, "diff-index", "--quiet",

@@ -1380,7 +1380,7 @@ static int submodule_has_dirty_index(const struct submodule *sub)
 static void submodule_reset_index(const char *path)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
-	prepare_submodule_repo_env_no_git_dir(&cp.env_array);
+	prepare_submodule_repo_env(&cp.env_array);
 
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;

@@ -1438,7 +1438,7 @@ int submodule_move_head(const char *path,
 		}
 	}
 
-	prepare_submodule_repo_env_no_git_dir(&cp.env_array);
+	prepare_submodule_repo_env(&cp.env_array);
 
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;

-- 
2.13.0.rc1.19.g083305f9b1

We forgot to prepare the submodule env, which is only a problem for
nested submodules. See 2e5d6503bd (ls-files: fix recurse-submodules
with nested submodules, 2017-04-13) for further explanation.

To come up with a proper test for this, we'd need to look at nested
submodules just as in that given commit. It turns out we're lucky
and these tests already exist, but are marked as failing. We need
to pass `--recurse-submodules` to read-tree additionally to make
these tests pass. Passing that flag alone would not make the tests
pass, such that this covers testing for the bug fix of the submodule
env as well.

Signed-off-by: Stefan Beller <redacted>
---
 submodule.c                    | 3 ++-
 t/lib-submodule-update.sh      | 7 +------
 t/t1013-read-tree-submodule.sh | 1 -
 t/t2013-checkout-submodule.sh  | 1 -
 4 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/submodule.c b/submodule.c
index b0141a66dd..b3ae642f29 100644
--- a/submodule.c
+++ b/submodule.c

@@ -1446,7 +1446,7 @@ int submodule_move_head(const char *path,
 
 	argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
 			get_super_prefix_or_empty(), path);
-	argv_array_pushl(&cp.args, "read-tree", NULL);
+	argv_array_pushl(&cp.args, "read-tree", "--recurse-submodules", NULL);
 
 	if (flags & SUBMODULE_MOVE_HEAD_DRY_RUN)
 		argv_array_push(&cp.args, "-n");

@@ -1474,6 +1474,7 @@ int submodule_move_head(const char *path,
 			cp.no_stdin = 1;
 			cp.dir = path;
 
+			prepare_submodule_repo_env(&cp.env_array);
 			argv_array_pushl(&cp.args, "update-ref", "HEAD", new, NULL);
 
 			if (run_command(&cp)) {

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index fb4f7b014e..2c17826e95 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh

@@ -787,11 +787,6 @@ test_submodule_switch_recursing () {
 	then
 		RESULTDS=failure
 	fi
-	RESULTR=success
-	if test "$KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED" = 1
-	then
-		RESULTR=failure
-	fi
 	RESULTOI=success
 	if test "$KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED" = 1
 	then

@@ -1003,7 +998,7 @@ test_submodule_switch_recursing () {
 	'
 
 	# recursing deeper than one level doesn't work yet.
-	test_expect_$RESULTR "$command: modified submodule updates submodule recursively" '
+	test_expect_success "$command: modified submodule updates submodule recursively" '
 		prolog &&
 		reset_work_tree_to_interested add_nested_sub &&
 		(

diff --git a/t/t1013-read-tree-submodule.sh b/t/t1013-read-tree-submodule.sh
index de1ba02dc5..7019d0a04f 100755
--- a/t/t1013-read-tree-submodule.sh
+++ b/t/t1013-read-tree-submodule.sh

@@ -5,7 +5,6 @@ test_description='read-tree can handle submodules'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
-KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1
 KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
 KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1
 

diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
index e8f70b806f..aa35223369 100755
--- a/t/t2013-checkout-submodule.sh
+++ b/t/t2013-checkout-submodule.sh

@@ -64,7 +64,6 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/
 '
 
 KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
-KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1
 test_submodule_switch_recursing "git checkout --recurse-submodules"
 
 test_submodule_forced_switch_recursing "git checkout -f --recurse-submodules"

-- 
2.13.0.rc1.19.g083305f9b1

On 05/02, Stefan Beller wrote:

v2:
 * I dropped the RFC patches for reattaching HEAD as that is not the main
   concern of this series.
 * patch1 is just about dropping cp1 to reusing cp, instead of additionally
   "fixing" mem leaks as finish_command does that for us
 * reordered the patches, former patch 3 now as patch 2 (avoid auto-discovery
   in new working tree manipulator code)
 * Regarding former patch2, I wrote:

quoted

Junio wrote:

quoted

Sounds good (if only because this makes it similar to other
codepaths).

Is this something whose breakage before the patch is easily
demonstrated with a test?

quoted

I'll try to come up with a test in a reroll.

 
done

Thanks,
Stefan

Changes look good.

Stefan Beller (3):
  submodule_move_head: reuse child_process structure for futher commands
  submodule: avoid auto-discovery in new working tree manipulator code
  submodule: properly recurse for read-tree and checkout

 submodule.c                    | 21 +++++++++++----------
 t/lib-submodule-update.sh      |  7 +------
 t/t1013-read-tree-submodule.sh |  1 -
 t/t2013-checkout-submodule.sh  |  1 -
 4 files changed, 12 insertions(+), 18 deletions(-)

-- 
2.13.0.rc1.19.g083305f9b1

-- 
Brandon Williams