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@xxxxxxxxxx>
---
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;