patch, 5 partsSome submodule bugfixes and "reattaching detached HEADs"

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

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

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

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

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

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

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

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

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

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

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

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

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