RE: [PATCH 13/16] submodule: teach unpack_trees() to update submodules

[David Turner <David.Turner@twosigma.com>]

[I've reviewed up-to and including 13; I'll look at 14-16 tomorrow-ish]

> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Tuesday, November 15, 2016 6:07 PM
> Cc: git@vger.kernel.org; bmwill@google.com; gitster@pobox.com;
> jrnieder@gmail.com; mogulguy10@gmail.com; David Turner; Stefan Beller
> Subject: [PATCH 13/16] submodule: teach unpack_trees() to update
> submodules
...
>  	msgs[ERROR_NOT_UPTODATE_DIR] =
>  		_("Updating the following directories would lose untracked
> files in it:\n%s");
> +	msgs[ERROR_NOT_UPTODATE_SUBMODULE] =
> +		_("Updating the following submodules would lose modifications
> in
> +it:\n%s");

s/it/them/

>  	if (!strcmp(cmd, "checkout"))
>  		msg = advice_commit_before_merge
> @@ -1315,19 +1320,18 @@ static int verify_uptodate_1(const struct
> cache_entry *ce,
>  		return 0;
> 
>  	if (!lstat(ce->name, &st)) {
> -		int flags =
> CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;
> -		unsigned changed = ie_match_stat(o->src_index, ce, &st,
> flags);
> -		if (!changed)
> -			return 0;
> -		/*
> -		 * NEEDSWORK: the current default policy is to allow
> -		 * submodule to be out of sync wrt the superproject
> -		 * index.  This needs to be tightened later for
> -		 * submodules that are marked to be automatically
> -		 * checked out.
> -		 */
> -		if (S_ISGITLINK(ce->ce_mode))
> -			return 0;
> +		if (!S_ISGITLINK(ce->ce_mode)) {

I generally prefer to avoid if (!x) { A } else { B } -- I would rather just see if (x) { B } else { A }.

> +		if (!changed) {
> +			/* old is always a submodule */
> +			if (S_ISGITLINK(new->ce_mode)) {
> +				/*
> +				 * new is also a submodule, so check if we care
> +				 * and then if can checkout the new sha1 safely
> +				 */
> +				if (submodule_is_interesting(old->name, null_sha1)
> +				    && is_submodule_checkout_safe(new->name, &new-
> >oid))
> +					return 0;
> +			} else {
> +				/*
> +				 * new is not a submodule any more, so only
> +				 * care if we care:
> +				 */
> +				if (submodule_is_interesting(old->name, null_sha1)
> +				    && ok_to_remove_submodule(old->name))
> +					return 0;
> +			}

Do we need a return 1 in here somewhere?  Because otherwise, we fall through and return 0 later.