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

From: Brandon Williams <bmwill@google.com>
Date: May 1, 2017, 18:36 UTC
Message-ID: <20170501183656.GH39135@google.com>
In-Reply-To: <20170501180058.8063-5-sbeller@google.com>

On 05/01, Stefan Beller wrote:

Show 22 quoted lines

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

Show 25 quoted lines

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

Show 58 quoted lines

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