Re: [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 2, 2018, 19:43 UTC
Message-ID: <xmqqbmichws2.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171227225705.73235-5-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 14 quoted lines

> diff --git a/unpack-trees.c b/unpack-trees.c
> index bf8b602901..ac59524a7f 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
>  			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
>  				update |= CE_UPDATE;
>  		}
> +		if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
> +		    !verify_uptodate(old, o))
> +			update |= CE_UPDATE;
>  		add_entry(o, old, update, 0);
>  		return 0;
>  	}

This is more sensible than the previous one that broke same() by unconditionally checking the working tree state, even when the machinery is told not to look at the working tree.

The code in the pre-context of this hunk, (i.e. the original one you are half-way mimicking), we realize that the original cache entry and the cache entry we are checking out are exactly the same and we overwrite when we know that the path in the working tree is dirty, and we are not told to "skip" that path in the working tree (i.e. sparse checkout). That happens only when we are told to o->update and o->reset.

Shouldn't we be setting the update bit only when o->update is in effect, just like we can see in the pre-context of this hunk? I do not know if we need to require o->reset and/or need to pay attention to the skip worktree bit in this new case.