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

[Stefan Beller <sbeller@google.com>]

On Thu, Nov 15, 2018 at 1:33 PM Michael Forney <mforney@mforney.org> wrote:
>
> On 2018-11-15, Stefan Beller <sbeller@google.com> wrote:
> > On Wed, Nov 14, 2018 at 10:05 PM Michael Forney <mforney@mforney.org>
> > wrote:
> >> Looking at ff6f1f564c, I don't really see anything that might be
> >> related to git-add, git-reset, or git-diff, so I'm guessing that this
> >> only worked before because the submodule config wasn't getting loaded
> >> during `git add` or `git reset`. Now that the config is loaded
> >> automatically, submodule.<name>.ignore started taking effect where it
> >> shouldn't.
> >>
> >> Unfortunately, this doesn't really get me much closer to finding a fix.
> >
> > Maybe selectively unloading or overwriting the config?
> >
> > Or we can change is_submodule_ignored() in diff.c
> > to be only applied selectively whether we are running the
> > right command? For this approach we'd have to figure out the
> > set of commands to which the ignore config should apply or
> > not (and come up with a more concise documentation then)
> >
> > This approach sounds appealing to me as it would cover
> > new commands as well and we'd only have a central point
> > where the decision for ignoring is made.
>
> 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?

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

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

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

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

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