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

From: Stefan Beller <sbeller@google.com>
Date: Apr 14, 2017, 20:12 UTC
Message-ID: <CAGZ79kYQpVKc1GGj4hxw6t-6eDo8zPjwf=CtEPFTc9b4St1EOQ@mail.gmail.com>
In-Reply-To: <15b87ca2-98c4-edfc-1e7e-7a25c28bd8da@google.com>

On Fri, Apr 14, 2017 at 11:28 AM, Jonathan Tan <jonathantanmy@google.com> wrote:

Show 14 quoted lines

>> @@ -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.

Show 9 quoted lines

>>
>> +       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.

Show 25 quoted lines

>
> 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