Re: [PATCH 1/4] entry.c: submodule recursing: respect force flag correctly

[Jonathan Tan <jonathantanmy@google.com>]

On 04/11/2017 04:49 PM, Stefan Beller wrote:
> In case of a non-forced worktree update, the submodule movement is tested
> in a dry run first, such that it doesn't matter if the actual update is
> done via the force flag. However for correctness, we want to give the
> flag is specified by the user. All callers have been inspected and updated
> if needed.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  entry.c        | 8 ++++----
>  unpack-trees.c | 7 ++++++-
>  2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index d2b512da90..d6b263f78e 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -208,7 +208,8 @@ static int write_entry(struct cache_entry *ce,
>  		sub = submodule_from_ce(ce);
>  		if (sub)
>  			return submodule_move_head(ce->name,
> -				NULL, oid_to_hex(&ce->oid), SUBMODULE_MOVE_HEAD_FORCE);
> +				NULL, oid_to_hex(&ce->oid),
> +				state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);
>  		break;
>  	default:
>  		return error("unknown file mode for %s in index", path);
> @@ -282,12 +283,11 @@ int checkout_entry(struct cache_entry *ce,
>  					unlink_or_warn(ce->name);
>
>  				return submodule_move_head(ce->name,
> -					NULL, oid_to_hex(&ce->oid),
> -					SUBMODULE_MOVE_HEAD_FORCE);
> +					NULL, oid_to_hex(&ce->oid), 0);

Should we be consistent (with the "else" block below and with the 
existing code) to use "state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0" 
instead of 0? (I glanced briefly through the code and 
SUBMODULE_MOVE_HEAD_FORCE might have no effect anyway if "old" is NULL, 
but it's probably still better to be consistent.)

>  			} else
>  				return submodule_move_head(ce->name,
>  					"HEAD", oid_to_hex(&ce->oid),
> -					SUBMODULE_MOVE_HEAD_FORCE);
> +					state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);
>  		}
>
>  		if (!changed)
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 8333da2cc9..7316ca99c2 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -252,14 +252,18 @@ static int check_submodule_move_head(const struct cache_entry *ce,
>  				     const char *new_id,
>  				     struct unpack_trees_options *o)
>  {
> +	unsigned flags = SUBMODULE_MOVE_HEAD_DRY_RUN;
>  	const struct submodule *sub = submodule_from_ce(ce);
>  	if (!sub)
>  		return 0;
>
> +	if (o->reset)
> +		flags |= SUBMODULE_MOVE_HEAD_FORCE;

It seems to me that this is independent of the entry.c change, and might 
be better in its own patch. (Or if it is not, maybe the subject should 
be "entry, unpack-trees: propagate force when submodule recursing" or 
something like that, containing the names of both modified components.)

Also, you mentioned in the parent message that this patch is required 
for patch 3. Is only the entry.c part required, or unpack-trees.c, or both?

> +
>  	switch (sub->update_strategy.type) {
>  	case SM_UPDATE_UNSPECIFIED:
>  	case SM_UPDATE_CHECKOUT:
> -		if (submodule_move_head(ce->name, old_id, new_id, SUBMODULE_MOVE_HEAD_DRY_RUN))
> +		if (submodule_move_head(ce->name, old_id, new_id, flags))
>  			return o->gently ? -1 :
>  				add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);
>  		return 0;
> @@ -308,6 +312,7 @@ static void unlink_entry(const struct cache_entry *ce)
>  		case SM_UPDATE_CHECKOUT:
>  		case SM_UPDATE_REBASE:
>  		case SM_UPDATE_MERGE:
> +			/* state.force is set at the caller. */

I don't understand the relevance of this comment - it is indeed set 
there, but "state" is not used there until after the invocation to 
unlink_entry so it doesn't seem related.

>  			submodule_move_head(ce->name, "HEAD", NULL,
>  					    SUBMODULE_MOVE_HEAD_FORCE);
>  			break;
>