On Fri, Apr 14, 2017 at 11:28 AM, Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote: >> @@ -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.) ok, will do. >> >> + 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.) eh. I realize the patch evolved after writing the commit message initially. Maybe: fix all submodule_move_head force flags Audit all callers of submodule_move_head and make sure the force flag is handled correctly. > > 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. Well we would have wanted to put state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0 here, but state is not passed into this function, so just make a comment why we keep it at force all the time. Thanks, Stefan