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