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

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

[Stefan Beller]

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

[PATCH 1/5] submodule_move_head: fix leak of struct child_process

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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

[PATCH 2/5] submodule_move_head: prepare env for child correctly

[Stefan Beller]

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 <sbeller@google.com>
---
 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

[PATCH 3/5] submodule: avoid auto-discovery in new working tree manipulator code

[Stefan Beller]

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 <sbeller@google.com>
---
 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

[RFC PATCH 4/5] submodule--helper: reattach-HEAD

[Stefan Beller]

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 <sbeller@google.com>
---
 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

[RFC PATCH 5/5] submodule_move_head: reattach submodule HEAD to branch if possible.

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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

Re: [PATCH 1/5] submodule_move_head: fix leak of struct child_process

[Brandon Williams]

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 <sbeller@google.com>

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


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

-- 
Brandon Williams

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

[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

Re: [RFC PATCH 4/5] submodule--helper: reattach-HEAD

[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 <sbeller@google.com>
>  
> +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.

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

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

-- 
Brandon Williams

Re: [RFC PATCH 4/5] submodule--helper: reattach-HEAD

[Stefan Beller]

On Mon, May 1, 2017 at 11:36 AM, Brandon Williams <bmwill@google.com> 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.

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

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

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> 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

The reason is because the superproject already records where the
HEAD in the submodule should be, when any of its commits is checked
out.  The tip of a branch (which one???) in a submodule may match
that commit, in which case there is nothing gained by having you on
that branch, or it may not match that commit, in which case it is
unclear what should happen.  Leaving your submodule on the branch
would mean the state of your tree as a whole does not match what the
checkout operation in the superproject wanted to create.  Resetting
the branch would mean you may lose the history of the branch.

Thinking of the detached HEAD state as an implicit unnamed branch
that matches the state the superproject checkout expects was
conceptually one of the cleanest choices.

But all of the above concentrates on what should happen immediately
after you do a checkout in the superproject, and it would be OK for
a sight-seer.  Once you want to work in the submodules to build on
their histories, not being on a branch does become awkward.  For one
thing, after you are done with the work in your submodule, you would
want to update the superproject and make a commit that records the
commit in the submodule, and then you would want to publish both the
superproject and the submodule because both of them are now
updated.  The commit in the superproject may be on the branch that
is currently checked out, but we don't know what branch the commit
in the submoudule should go.

The submodule.<name>.branch configuration may be a good source to
learn that piece of information, but it does not fully solve the
issue.  It is OK for the tip of that branch to be at or behind the
commit the superproject records, but it is unclear what should
happen if the local tip of that branch is ahead of the commit in the
superproject when checkout happens.

By the way, how does this topic work with the various checkout modes
that can be specified with submodule.<name>.update?

Re: [PATCH 2/5] submodule_move_head: prepare env for child correctly

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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 <sbeller@google.com>
> ---

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?

>  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)) {

Re: [PATCH 1/5] submodule_move_head: fix leak of struct child_process

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> 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 <sbeller@google.com>
>
> This shouldn't be needed as 'finish_command' does the cleanup for you by
> calling 'child_prcoess_clear()'.

Yes, indeed.  

It might not hurt to update the code so that the cp is reused in the
second run instead of using a separate instance cp1, but that is a
topic separate from fixing a non-existing leak.

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

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

[Stefan Beller]

On Mon, May 1, 2017 at 6:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>> 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
>
> The reason is because the superproject already records where the
> HEAD in the submodule should be, when any of its commits is checked
> out.  The tip of a branch (which one???)

The one as configured (submodule.NAME.branch

>  in a submodule may match
> that commit, in which case there is nothing gained by having you on
> that branch,

Being on a branch has some advantages, e.g. easier pushing, not
worrying about losing commits due to gc, an easier "name" in a literal sense.


>  or it may not match that commit, in which case it is
> unclear what should happen.

Yes, I anticipate this to be the main point of discussion.

>  Leaving your submodule on the branch
> would mean the state of your tree as a whole does not match what the
> checkout operation in the superproject wanted to create.  Resetting
> the branch would mean you may lose the history of the branch.

or telling the user via die(), that there is a mismatch.
(you may want to run git submodule update --remote to fix the situation)

> Thinking of the detached HEAD state as an implicit unnamed branch
> that matches the state the superproject checkout expects was
> conceptually one of the cleanest choices.

Assuming this is the cleanest design, we may want to change the
message of git-status, then.
Unlike the scary detached HEAD message (incl long hint), we may just
want to say

    $ git status
    In submodule 'foo'
    You're detached exactly as the superprojects wants you to be.
    Nothing to worry.



> But all of the above concentrates on what should happen immediately
> after you do a checkout in the superproject, and it would be OK for
> a sight-seer.  Once you want to work in the submodules to build on
> their histories, not being on a branch does become awkward.  For one
> thing, after you are done with the work in your submodule, you would
> want to update the superproject and make a commit that records the
> commit in the submodule, and then you would want to publish both the
> superproject and the submodule because both of them are now
> updated.  The commit in the superproject may be on the branch that
> is currently checked out, but we don't know what branch the commit
> in the submoudule should go.
>
> The submodule.<name>.branch configuration may be a good source to
> learn that piece of information,

Glad we agree up to this point.

> but it does not fully solve the
> issue.  It is OK for the tip of that branch to be at or behind the
> commit the superproject records, but it is unclear what should
> happen if the local tip of that branch is ahead of the commit in the
> superproject when checkout happens.

right. It is still unclear to me as well. I'll have to think about the
various modes of operation.

> By the way, how does this topic work with the various checkout modes
> that can be specified with submodule.<name>.update?

This series currently does not touch git-submodule, but in principle
we could just run "submodule--helper reattach-HEAD" after any operation
and then see if we can attach a HEAD (likely for "update=checkout",
but in  "merge" we may want to fast-forward the branch, and in "rebase"
we might want to reset (noff) to the tip.

I'll think about this more.
Thanks,
Stefan

Re: [PATCH 2/5] submodule_move_head: prepare env for child correctly

[Stefan Beller]

On Mon, May 1, 2017 at 7:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 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 <sbeller@google.com>
>> ---
>
> 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.

Thanks,
Stefan

Re: [PATCH 1/5] submodule_move_head: fix leak of struct child_process

[Stefan Beller]

On Mon, May 1, 2017 at 8:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>> 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 <sbeller@google.com>
>>
>> This shouldn't be needed as 'finish_command' does the cleanup for you by
>> calling 'child_prcoess_clear()'.
>
> Yes, indeed.
>
> It might not hurt to update the code so that the cp is reused in the
> second run instead of using a separate instance cp1, but that is a
> topic separate from fixing a non-existing leak.

ok, I'll drop the patch and may send another patch later for re-using cp

Thanks,
Stefan

[PATCHv2 0/3] Some submodule bugfixes

[Stefan Beller]

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

[PATCHv2 1/3] submodule_move_head: reuse child_process structure for futher commands

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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

[PATCHv2 2/3] submodule: avoid auto-discovery in new working tree manipulator code

[Stefan Beller]

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 <sbeller@google.com>
---
 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

[PATCHv2 3/3] submodule: properly recurse for read-tree and checkout

[Stefan Beller]

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 <sbeller@google.com>
---
 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

Re: [PATCHv2 0/3] Some submodule bugfixes

[Brandon Williams]

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:
> > Junio wrote:
> >> 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

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

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

[Stefan Beller]

On Mon, May 1, 2017 at 9:04 PM, Stefan Beller <sbeller@google.com> wrote:
>>
>>> 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
>>
>
> I'll think about this more.

What the current model is missing is the possibility to have
a symbolic link not just to a ref within a repository, but to the outside
of a repository (such as the superproject in this case).

So we could have a HEAD with a content like:

    "super: <superprojects git dir> [LF <path inside superproject>]"

Then we would use the HEAD to determine if the superproject
would touch a submodule at all. Example workflow:

    git -C <sub> checkout --reattach-to-superproject

    # hack away in the submodule

    # This will make a commit in <sub> and add the
    # resulting object to the index of the superproject
    # because HEAD is tracking the superproject.
    # so in order to have HEAD containing the new
    # commit we have to change the superproject:
    git -C <sub> commit -a -m "message"

    # This has also interesting consequences for
    # submodule related commands:
    git checkout --recurse-submodules <tree-ish>
    # Any submodule whose HEAD is attached to the
    # superproject would be touched, the others would
    # not.

By being directly attached to the superproject, it would be
easy to find all submodules that are changed, via a

    git -C <super> status # no need to recurse, even!




















The whole "checkout --recurse-submodules" series is based on
assumptions of the current mental model of how branches and
detached HEADs work.


A submodule would have a symref