Re: Confusing behavior with ignored submodules and `git commit -a`

[Michael Forney <mforney@mforney.org>]

On 2018-11-15, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Nov 15, 2018 at 1:33 PM Michael Forney <mforney@mforney.org> wrote:
>> Well, currently the submodule config can be disabled in diff_flags by
>> setting override_submodule_config=1. However, I'm thinking it may be
>> simpler to selectively *enable* the submodule config in diff_flags
>> where it is needed instead of disabling it everywhere else (i.e.
>> use_submodule_config instead of override_submodule_config).
>
> This sounds like undoing the good(?) part of the series that introduced
> this regression, as before that we selectively loaded the submodule
> config, which lead to confusion when you forgot it. Selectively *enabling*
> the submodule config sounds like that state before?
>
> Or do we *only* talk about enabling the ignore flag, while loading the
> rest of the submodule config automatic?

Yes, that is what I meant. I believe the automatic loading of
submodule config is the right thing to do, it just uncovered cases
where the current handling of override_submodule_config is not quite
sufficient.

My suggestion of replacing override_submodule_config with
use_submodule_config is because it seems like there are fewer places
where we want to apply the ignore config than not. I think it should
only apply in diffs against the working tree and when staging changes
to the index (unless specified explicitly). The documentation just
mentions the "diff family", but all but one of the possible values for
submodule.<name>.ignore ("all") don't make sense unless comparing with
the working tree. This is also how show/log -p behaved in git <2.15.
So I think that clarifying that it is about modifications *to the
working tree* would be a good idea.

>> I'm also starting to see why this is tricky. The only difference that
>> diff.c:run_diff_files sees between `git add inner` and `git add --all`
>> is whether the index entry matched the pathspec exactly or not.
>
> Unrelated to the trickiness, I think we'd need to document the behavior
> of the -a flag in git-add and git-commit better as adding the diff below
> will depart from the "all" rule again, which I thought was a strong
> motivator for Brandons series (IIRC).

Can you explain what you mean by the "all" rule?

>> Here is a work-in-progress diff that seems to have the correct
>> behavior in all cases I tried. Can you think of any cases that it
>> breaks? I'm not quite sure of the consequences of having diff_change
>> and diff_addremove always ignore the submodule config; git-diff and
>> git-status still seem to work correctly.
>>
>> diff --git a/builtin/add.c b/builtin/add.c
>> index f65c17229..9902f7742 100644
>> --- a/builtin/add.c
>> +++ b/builtin/add.c
>> @@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix,
>>         rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
>>         rev.diffopt.format_callback = update_callback;
>>         rev.diffopt.format_callback_data = &data;
>> -       rev.diffopt.flags.override_submodule_config = 1;
>
> This line partially reverts 5556808, taking 02f2f56bc377c28
> into account.

Correct. The problem with 55568086 is that add_files_to_cache is used
by both git-add and git-commit, regardless of whether --all was
specified. So, while it made it possible to stage ignored submodules,
it also made it so the submodule ignore config was overridden in all
uses of git-add and git-commit.

So, this diff attempts to make it so the ignore config is only applied
when the pathspec matches exactly rather than just always overriding
the ignore config.

>> diff --git a/diff-lib.c b/diff-lib.c
>> index 83fce5151..fbb048cca 100644
>> --- a/diff-lib.c
>> +++ b/diff-lib.c
>> @@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry
>> *ce, struct stat *st)
>>  static int match_stat_with_submodule(struct diff_options *diffopt,
>>                                      const struct cache_entry *ce,
>>                                      struct stat *st, unsigned ce_option,
>> -                                    unsigned *dirty_submodule)
>> +                                    unsigned *dirty_submodule,
>> +                                    int exact)
>> [...];
>
> This is an interesting take so far as it is all about *detecting* change
> here via stat information and not like the previous (before the regression)
> where it was about correcting output.
>
> match_stat_with_submodule would grow its documentation to be
> slightly more complicated as a result.

Yes, this is one part I'm not quite happy with. I wonder if instead
match_stat_with_submodule could be made simpler by moving the
ie_match_stat call up to the two call sites, and then it could be
selectively called by run_diff_files depending on the value of
matched.

>> diff --git a/diff.c b/diff.c
>> index e38d1ecaf..73dc75286 100644
>> --- a/diff.c
>> +++ b/diff.c
>> [...]
>> -static int is_submodule_ignored(const char *path, struct diff_options
>> *options)
>> -{
>> [...]
>> -       if (S_ISGITLINK(mode) && is_submodule_ignored(concatpath,
>> options))
>> +       if (S_ISGITLINK(mode) && options->flags.ignore_submodules)
>>                 return;
>
> This basically inlines the function is_submodule_ignored,
> except for the part:
>
>     if (!options->flags.override_submodule_config)
>         set_diffopt_flags_from_submodule_config(options, path);
>
> but that was taken care off in match_stat_with_submodule in diff-lib?

Yeah, since run_diff_files already skips ignored submodules (except if
given by name), and we still want the changes to be applied to the
index. Like I said, I'm not really sure if there are any other uses of
diff_change and diff_addremove where we actually do want the .ignore
config applied.

But, maybe if I instead left
`rev.diffopt.flags.override_submodule_config = 1` in builtin/add.c and
changed the condition in match_stat_with_submodule from `&& !exact` to
`|| !exact`, these functions would not need to be changed. In other
words, we relax the behavior when override_submodule_config is
specified, rather than making it stricter when it is not specified.

Testing this out, it looks like it fixes the behavior with add/commit
--all, but still omits ignored submodules in diffs between commits
(which is not the behavior I expect, see above). So, without the
changes to diff_change/diff_addremove we probably would need to add
override_submodule_config=1 to even more places.

Too many negatives are making this difficult to reason about.

> This WIP looks really promising, thanks for looking into this!

Thanks for taking the time to help me!