Re: [PATCH 1/4] entry.c: submodule recursing: respect force flag correctly
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Apr 14, 2017, 18:28 UTC
- Message-ID
- <15b87ca2-98c4-edfc-1e7e-7a25c28bd8da@google.com>
- In-Reply-To
- <20170411234923.1860-2-sbeller@google.com>
On 04/11/2017 04:49 PM, Stefan Beller wrote:
Show 33 quoted lines
> 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.)
Show 23 quoted lines
> } 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?
Show 14 quoted lines
> +
> 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; >