Re: [PATCH 15/18] read-cache, remove_marked_cache_entries: wipe selected submodules.

[Stefan Beller <sbeller@google.com>]

On Tue, Mar 7, 2017 at 5:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> submodule_from_ce returns always NULL, when such flag is not given.
>> From 10/18:
>>
>> +const struct submodule *submodule_from_ce(const struct cache_entry *ce)
>> +{
>> +       if (!S_ISGITLINK(ce->ce_mode))
>> +               return NULL;
>> +
>> +       if (!should_update_submodules())
>> +               return NULL;
>> +
>> +       return submodule_from_path(null_sha1, ce->name);
>> +}
>>
>> should_update_submodules is always false if such a flag is not set,
>> such that we end up in the else case which is literally the same as
>> the removed lines (they are just indented).
>
> I see.
>
> I didn't think a function this deep in the callchain that does not
> take any parameter could possibly change the behaviour based on the
> end-user input.  I was expecting that such a state (i.e. are we
> recursive?  are we allowed to forcibly update the working tree
> files?) would be kept part of something like "struct checkout" and
> passed around the callchain.
>
> That was why I didn't look at how that function answers "should
> update?" question, and got puzzled.  Because it would imply there is
> some hidden state that is accessible by everybody--a global variable
> or something--which would point at a deeper design issue.

Well it is just as deep as e.g. reacting on some bits of
struct unpack_trees_options in unpack-trees.c ?

But I can see how this is an issue with design.
I also think my previous answer was a straw man.

We only need to differentiate between 'ce' being a submodule
or not, because it should not be marked CE_REMOVE in
the non-recursive state.

Side-question:
Is there some doc (commit message), that explains the difference
between CE_REMOVE and CE_WT_REMOVE ?

Thanks,
Stefan