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

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

[Michael Forney]

Hi,

In the past few months have noticed some confusing behavior with
ignored submodules. I finally got around to bisecting this to commit
5556808690ea245708fb80383be5c1afee2fb3eb (add, reset: ensure
submodules can be added or reset).

Here is a demonstration of the problem:

First some repository initialization with a submodule marked as ignored.

$ git init inner && git -C inner commit --allow-empty -m 'inner 1'
Initialized empty Git repository in /tmp/inner/.git/
[master (root-commit) ef55bed] inner 1
$ git init outer && cd outer
Initialized empty Git repository in /tmp/outer/.git/
$ git submodule add ../inner
Cloning into '/tmp/outer/inner'...
done.
$ echo 1 > foo.txt && git add foo.txt
$ git commit -m 'outer 1'
[master (root-commit) efeb85c] outer 1
 3 files changed, 5 insertions(+)
 create mode 100644 .gitmodules
 create mode 100644 foo.txt
 create mode 160000 inner
$ git config submodule.inner.ignore all
$ git -C inner commit --allow-empty -m 'inner 2'
[master 7b7f0fa] inner 2
$ git status
On branch master
nothing to commit, working tree clean
$

Up to here is all expected. However, if I go to update `foo.txt` and
commit with `git commit -a`, changes to inner get recorded
unexpectedly. What's worse is the shortstat output of `git commit -a`,
and the diff output of `git show` give no indication that the
submodule was changed.

$ echo 2 > foo.txt
$ git status
On branch master
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        modified:   foo.txt

no changes added to commit (use "git add" and/or "git commit -a")
$ git commit -a -m 'update foo.txt'
[master 6ec564c] update foo.txt
 1 file changed, 1 insertion(+), 1 deletion(-)
$ git show
commit 6ec564c15ddae099c71f01750b4c434557525653 (HEAD -> master)
Author: Michael Forney <mforney@mforney.org>
Date:   Fri Mar 16 20:18:37 2018 -0700

    update foo.txt

diff --git a/foo.txt b/foo.txt
index d00491f..0cfbf08 100644
--- a/foo.txt
+++ b/foo.txt
@@ -1 +1 @@
-1
+2
$

There have been a couple occasions where I accidentally pushed local
changes to ignored submodules because of this. Since they don't show
up in the log output, it is difficult to figure out what actually has
gone wrong.

Anyway, since the bisected commit (555680869) only mentions add and
reset, I'm assuming that this is a regression and not a deliberate
behavior change. The documentation for submodule.<name>.ignore states
that the setting should only affect `git status` and the diff family.
In terms of my expectations, I would go further and say it should only
affect `git status` and diffs against the working tree.

I took a brief look through the relevant sources, and it wasn't clear
to me how to fix this without accidentally changing the behavior of
other subcommands.

Any help with this issue is appreciated!

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

[Michael Forney]

On 2018-03-16, Michael Forney <mforney@mforney.org> wrote:
> Hi,
>
> In the past few months have noticed some confusing behavior with
> ignored submodules. I finally got around to bisecting this to commit
> 5556808690ea245708fb80383be5c1afee2fb3eb (add, reset: ensure
> submodules can be added or reset).
>
> Here is a demonstration of the problem:
>
> First some repository initialization with a submodule marked as ignored.
>
> $ git init inner && git -C inner commit --allow-empty -m 'inner 1'
> Initialized empty Git repository in /tmp/inner/.git/
> [master (root-commit) ef55bed] inner 1
> $ git init outer && cd outer
> Initialized empty Git repository in /tmp/outer/.git/
> $ git submodule add ../inner
> Cloning into '/tmp/outer/inner'...
> done.
> $ echo 1 > foo.txt && git add foo.txt
> $ git commit -m 'outer 1'
> [master (root-commit) efeb85c] outer 1
>  3 files changed, 5 insertions(+)
>  create mode 100644 .gitmodules
>  create mode 100644 foo.txt
>  create mode 160000 inner
> $ git config submodule.inner.ignore all
> $ git -C inner commit --allow-empty -m 'inner 2'
> [master 7b7f0fa] inner 2
> $ git status
> On branch master
> nothing to commit, working tree clean
> $
>
> Up to here is all expected. However, if I go to update `foo.txt` and
> commit with `git commit -a`, changes to inner get recorded
> unexpectedly. What's worse is the shortstat output of `git commit -a`,
> and the diff output of `git show` give no indication that the
> submodule was changed.
>
> $ echo 2 > foo.txt
> $ git status
> On branch master
> Changes not staged for commit:
>   (use "git add <file>..." to update what will be committed)
>   (use "git checkout -- <file>..." to discard changes in working directory)
>
>         modified:   foo.txt
>
> no changes added to commit (use "git add" and/or "git commit -a")
> $ git commit -a -m 'update foo.txt'
> [master 6ec564c] update foo.txt
>  1 file changed, 1 insertion(+), 1 deletion(-)
> $ git show
> commit 6ec564c15ddae099c71f01750b4c434557525653 (HEAD -> master)
> Author: Michael Forney <mforney@mforney.org>
> Date:   Fri Mar 16 20:18:37 2018 -0700
>
>     update foo.txt
>
> diff --git a/foo.txt b/foo.txt
> index d00491f..0cfbf08 100644
> --- a/foo.txt
> +++ b/foo.txt
> @@ -1 +1 @@
> -1
> +2
> $
>
> There have been a couple occasions where I accidentally pushed local
> changes to ignored submodules because of this. Since they don't show
> up in the log output, it is difficult to figure out what actually has
> gone wrong.
>
> Anyway, since the bisected commit (555680869) only mentions add and
> reset, I'm assuming that this is a regression and not a deliberate
> behavior change. The documentation for submodule.<name>.ignore states
> that the setting should only affect `git status` and the diff family.
> In terms of my expectations, I would go further and say it should only
> affect `git status` and diffs against the working tree.
>
> I took a brief look through the relevant sources, and it wasn't clear
> to me how to fix this without accidentally changing the behavior of
> other subcommands.
>
> Any help with this issue is appreciated!

I accidentally pushed local changes to ignored submodules again due to this.

Can anyone confirm whether this is the intended behavior of ignore? If
it is, then at least the documentation needs an update saying that
`commit -a` will commit all submodule changes, even if they are
ignored.

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

[Stefan Beller]

On Thu, Oct 25, 2018 at 11:03 AM Michael Forney <mforney@mforney.org> wrote:
>
> On 2018-03-16, Michael Forney <mforney@mforney.org> wrote:
> > Hi,
> >
> > In the past few months have noticed some confusing behavior with
> > ignored submodules. I finally got around to bisecting this to commit
> > 5556808690ea245708fb80383be5c1afee2fb3eb (add, reset: ensure
> > submodules can be added or reset).

Uh. :(

See the discussion starting at
https://public-inbox.org/git/20170725213928.125998-4-bmwill@google.com/
specifically
https://public-inbox.org/git/xmqqinieq49v.fsf@gitster.mtv.corp.google.com/


> >
> > Here is a demonstration of the problem:
> >
[...]
> > Up to here is all expected.

Makes sense.

> > However, if I go to update `foo.txt` and
> > commit with `git commit -a`, changes to inner get recorded
> > unexpectedly. What's worse is the shortstat output of `git commit -a`,
> > and the diff output of `git show` give no indication that the
> > submodule was changed.

This is really bad. git-status and git-commit share some code,
and we'll populate the commit message with a status output.
So it seems reasonable to expect the status and the commit to match,
i.e. if status tells me there is no change, then commit should not record
the submodule update.

> > $ git commit -a -m 'update foo.txt'
> > [master 6ec564c] update foo.txt
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > $ git show
> > commit 6ec564c15ddae099c71f01750b4c434557525653 (HEAD -> master)
> > Author: Michael Forney <mforney@mforney.org>
> > Date:   Fri Mar 16 20:18:37 2018 -0700
> >
> >     update foo.txt
> >
> > diff --git a/foo.txt b/foo.txt
> > index d00491f..0cfbf08 100644
> > --- a/foo.txt
> > +++ b/foo.txt
> > @@ -1 +1 @@
> > -1
> > +2
> > $
> >
> > There have been a couple occasions where I accidentally pushed local
> > changes to ignored submodules because of this. Since they don't show
> > up in the log output, it is difficult to figure out what actually has
> > gone wrong.

How was it prevented before? Just by git commit -a not picking up the
submodule change?

> >
> > Anyway, since the bisected commit (555680869) only mentions add and
> > reset, I'm assuming that this is a regression and not a deliberate
> > behavior change. The documentation for submodule.<name>.ignore states
> > that the setting should only affect `git status` and the diff family.
> > In terms of my expectations, I would go further and say it should only
> > affect `git status` and diffs against the working tree.
> >
> > I took a brief look through the relevant sources, and it wasn't clear
> > to me how to fix this without accidentally changing the behavior of
> > other subcommands.
> >
> > Any help with this issue is appreciated!

I guess reverting that commit is not a good idea now, as
I would expect something to break.

Maybe looking through the series 614ea03a71
(Merge branch 'bw/submodule-config-cleanup', 2017-08-26)
to understand why it happened in the context would be a good start.

> I accidentally pushed local changes to ignored submodules again due to this.
>
> Can anyone confirm whether this is the intended behavior of ignore? If
> it is, then at least the documentation needs an update saying that
> `commit -a` will commit all submodule changes, even if they are
> ignored.

The docs say "(but it will nonetheless show up in the output of
status and commit when it has been staged)" as well, so that commit
sounds like a regression?

Stefan

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

[Michael Forney]

On 2018-10-25, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Oct 25, 2018 at 11:03 AM Michael Forney <mforney@mforney.org>
> wrote:
>>
>> On 2018-03-16, Michael Forney <mforney@mforney.org> wrote:
>> > Hi,
>> >
>> > In the past few months have noticed some confusing behavior with
>> > ignored submodules. I finally got around to bisecting this to commit
>> > 5556808690ea245708fb80383be5c1afee2fb3eb (add, reset: ensure
>> > submodules can be added or reset).
>
> Uh. :(
>
> See the discussion starting at
> https://public-inbox.org/git/20170725213928.125998-4-bmwill@google.com/
> specifically
> https://public-inbox.org/git/xmqqinieq49v.fsf@gitster.mtv.corp.google.com/

Thanks for the links. Let me explain how I'm using
submodule.<name>.ignore. Maybe there's a better mechanism in git to
deal with this (if .ignore is a misfeature).

I have a git repository which contains a number of submodules that
refer to external repositories. Some of these repositories need to
patched in some way, so patches are stored alongside the submodules,
and are applied when building. This mostly works fine, but causes
submodules to show up as modified in `git status` and get updated with
`git commit -a`. To resolve this, I've added `ignore = all` to
.gitmodules for all the submodules that need patches applied. This
way, I can explicitly `git add` the submodule when I want to update
the base commit, but otherwise pretend that they are clean. This has
worked pretty well for me, but less so since git 2.15 when this issue
was introduced.

Of course, I could maintain and publish forks of those repositories
and use those as the remote for the submodules. However in many cases
these patches are just temporary until they get applied upstream and a
new release is made, and I don't really want to keep mirrors
unnecessarily, or keep switching the submodule URL between upstream
and my fork.

>> > However, if I go to update `foo.txt` and
>> > commit with `git commit -a`, changes to inner get recorded
>> > unexpectedly. What's worse is the shortstat output of `git commit -a`,
>> > and the diff output of `git show` give no indication that the
>> > submodule was changed.
>
> This is really bad. git-status and git-commit share some code,
> and we'll populate the commit message with a status output.
> So it seems reasonable to expect the status and the commit to match,
> i.e. if status tells me there is no change, then commit should not record
> the submodule update.

I just checked and if I don't specify a message on the command-line,
the status output in the message template *does* mention that `inner`
is getting updated.

>> > There have been a couple occasions where I accidentally pushed local
>> > changes to ignored submodules because of this. Since they don't show
>> > up in the log output, it is difficult to figure out what actually has
>> > gone wrong.
>
> How was it prevented before? Just by git commit -a not picking up the
> submodule change?

Yes. Previously, `git commit -a` would not pick up the change (unless
I added it explicitly with `git add`), and `git log` would still show
changes to ignored submodules (which is the behavior I want).

> I guess reverting that commit is not a good idea now, as
> I would expect something to break.
>
> Maybe looking through the series 614ea03a71
> (Merge branch 'bw/submodule-config-cleanup', 2017-08-26)
> to understand why it happened in the context would be a good start.

Thanks, that's a good idea. I'll take a look through that series.

>> I accidentally pushed local changes to ignored submodules again due to
>> this.
>>
>> Can anyone confirm whether this is the intended behavior of ignore? If
>> it is, then at least the documentation needs an update saying that
>> `commit -a` will commit all submodule changes, even if they are
>> ignored.
>
> The docs say "(but it will nonetheless show up in the output of
> status and commit when it has been staged)" as well, so that commit
> sounds like a regression?

I just came across someone else affected by this issue:
https://github.com/git/git/commit/55568086#commitcomment-27137460

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

[Michael Forney]

+bmwill

On 2018-11-14, Michael Forney <mforney@mforney.org> wrote:
> On 2018-10-25, Stefan Beller <sbeller@google.com> wrote:
>> I guess reverting that commit is not a good idea now, as
>> I would expect something to break.
>>
>> Maybe looking through the series 614ea03a71
>> (Merge branch 'bw/submodule-config-cleanup', 2017-08-26)
>> to understand why it happened in the context would be a good start.
>
> Thanks, that's a good idea. I'll take a look through that series.

Interesting. If I build git from master after reverting 55568086, I do
indeed observe the issue it claims to fix (unable to add ignored
submodules). However, if I build from 9ef23f91fc (the immediate parent
of 55568086), I do not see the issue.

Investigating this further, it seems that 55568086 addresses an issue
that does not appear until later on in the series in ff6f1f564c
(submodule-config: lazy-load a repository's .gitmodules file). Perhaps
this was a result of reordering commits during a rebase. In other
words, I get correct behavior until 55568086, and in
55568086..ff6f1f564c^ if I revert 55568086.

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.

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

[Stefan Beller]

On Wed, Nov 14, 2018 at 10:05 PM Michael Forney <mforney@mforney.org> wrote:
>
> +bmwill
>
> On 2018-11-14, Michael Forney <mforney@mforney.org> wrote:
> > On 2018-10-25, Stefan Beller <sbeller@google.com> wrote:
> >> I guess reverting that commit is not a good idea now, as
> >> I would expect something to break.
> >>
> >> Maybe looking through the series 614ea03a71
> >> (Merge branch 'bw/submodule-config-cleanup', 2017-08-26)
> >> to understand why it happened in the context would be a good start.
> >
> > Thanks, that's a good idea. I'll take a look through that series.
>
> Interesting. If I build git from master after reverting 55568086, I do
> indeed observe the issue it claims to fix (unable to add ignored
> submodules). However, if I build from 9ef23f91fc (the immediate parent
> of 55568086), I do not see the issue.
>
> Investigating this further, it seems that 55568086 addresses an issue
> that does not appear until later on in the series in ff6f1f564c
> (submodule-config: lazy-load a repository's .gitmodules file). Perhaps
> this was a result of reordering commits during a rebase. In other
> words, I get correct behavior until 55568086, and in
> 55568086..ff6f1f564c^ if I revert 55568086.
>
> 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.

Stefan

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

[Michael Forney]

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

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.

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;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
 	clear_pathspec(&rev.prune_data);
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)
 {
 	int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
 	if (S_ISGITLINK(ce->ce_mode)) {
 		struct diff_flags orig_flags = diffopt->flags;
-		if (!diffopt->flags.override_submodule_config)
+		if (!diffopt->flags.override_submodule_config && !exact)
 			set_diffopt_flags_from_submodule_config(diffopt, ce->name);
 		if (diffopt->flags.ignore_submodules)
 			changed = 0;
@@ -88,7 +89,7 @@ static int match_stat_with_submodule(struct
diff_options *diffopt,

 int run_diff_files(struct rev_info *revs, unsigned int option)
 {
-	int entries, i;
+	int entries, i, matched;
 	int diff_unmerged_stage = revs->max_count;
 	unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
 			      ? CE_MATCH_RACY_IS_DIRTY : 0);
@@ -110,7 +111,8 @@ int run_diff_files(struct rev_info *revs, unsigned
int option)
 		if (diff_can_quit_early(&revs->diffopt))
 			break;

-		if (!ce_path_match(istate, ce, &revs->prune_data, NULL))
+		matched = ce_path_match(istate, ce, &revs->prune_data, NULL);
+		if (!matched)
 			continue;

 		if (ce_stage(ce)) {
@@ -226,7 +228,8 @@ int run_diff_files(struct rev_info *revs, unsigned
int option)
 			}

 			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
-							    ce_option, &dirty_submodule);
+							    ce_option, &dirty_submodule,
+							    matched == MATCHED_EXACTLY);
 			newmode = ce_mode_from_stat(ce, st.st_mode);
 		}

@@ -292,7 +295,7 @@ static int get_stat_data(const struct cache_entry *ce,
 			return -1;
 		}
 		changed = match_stat_with_submodule(diffopt, ce, &st,
-						    0, dirty_submodule);
+						    0, dirty_submodule, 0);
 		if (changed) {
 			mode = ce_mode_from_stat(ce, st.st_mode);
 			oid = &null_oid;
diff --git a/diff.c b/diff.c
index e38d1ecaf..73dc75286 100644
--- a/diff.c
+++ b/diff.c
@@ -6209,24 +6209,6 @@ int diff_can_quit_early(struct diff_options *opt)
 		opt->flags.has_changes);
 }

-/*
- * Shall changes to this submodule be ignored?
- *
- * Submodule changes can be configured to be ignored separately for each path,
- * but that configuration can be overridden from the command line.
- */
-static int is_submodule_ignored(const char *path, struct diff_options *options)
-{
-	int ignored = 0;
-	struct diff_flags orig_flags = options->flags;
-	if (!options->flags.override_submodule_config)
-		set_diffopt_flags_from_submodule_config(options, path);
-	if (options->flags.ignore_submodules)
-		ignored = 1;
-	options->flags = orig_flags;
-	return ignored;
-}
-
 void diff_addremove(struct diff_options *options,
 		    int addremove, unsigned mode,
 		    const struct object_id *oid,
@@ -6235,7 +6217,7 @@ void diff_addremove(struct diff_options *options,
 {
 	struct diff_filespec *one, *two;

-	if (S_ISGITLINK(mode) && is_submodule_ignored(concatpath, options))
+	if (S_ISGITLINK(mode) && options->flags.ignore_submodules)
 		return;

 	/* This may look odd, but it is a preparation for
@@ -6285,7 +6267,7 @@ void diff_change(struct diff_options *options,
 	struct diff_filepair *p;

 	if (S_ISGITLINK(old_mode) && S_ISGITLINK(new_mode) &&
-	    is_submodule_ignored(concatpath, options))
+	    options->flags.ignore_submodules)
 		return;

 	if (options->flags.reverse_diff) {

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

[Michael Forney]

On 2018-11-15, Michael Forney <mforney@mforney.org> wrote:
> Here is a work-in-progress diff that seems to have the correct
> behavior in all cases I tried.

I was hoping that gmail wouldn't mess with the whitespace, but apparently
it has, sorry about that. Let me try again.

---
 builtin/add.c |  1 -
 diff-lib.c    | 15 +++++++++------
 diff.c        | 22 ++--------------------
 3 files changed, 11 insertions(+), 27 deletions(-)

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;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
 	clear_pathspec(&rev.prune_data);
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)
 {
 	int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
 	if (S_ISGITLINK(ce->ce_mode)) {
 		struct diff_flags orig_flags = diffopt->flags;
-		if (!diffopt->flags.override_submodule_config)
+		if (!diffopt->flags.override_submodule_config && !exact)
 			set_diffopt_flags_from_submodule_config(diffopt, ce->name);
 		if (diffopt->flags.ignore_submodules)
 			changed = 0;
@@ -88,7 +89,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
 
 int run_diff_files(struct rev_info *revs, unsigned int option)
 {
-	int entries, i;
+	int entries, i, matched;
 	int diff_unmerged_stage = revs->max_count;
 	unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
 			      ? CE_MATCH_RACY_IS_DIRTY : 0);
@@ -110,7 +111,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		if (diff_can_quit_early(&revs->diffopt))
 			break;
 
-		if (!ce_path_match(istate, ce, &revs->prune_data, NULL))
+		matched = ce_path_match(istate, ce, &revs->prune_data, NULL);
+		if (!matched)
 			continue;
 
 		if (ce_stage(ce)) {
@@ -226,7 +228,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			}
 
 			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
-							    ce_option, &dirty_submodule);
+							    ce_option, &dirty_submodule,
+							    matched == MATCHED_EXACTLY);
 			newmode = ce_mode_from_stat(ce, st.st_mode);
 		}
 
@@ -292,7 +295,7 @@ static int get_stat_data(const struct cache_entry *ce,
 			return -1;
 		}
 		changed = match_stat_with_submodule(diffopt, ce, &st,
-						    0, dirty_submodule);
+						    0, dirty_submodule, 0);
 		if (changed) {
 			mode = ce_mode_from_stat(ce, st.st_mode);
 			oid = &null_oid;
diff --git a/diff.c b/diff.c
index e38d1ecaf..73dc75286 100644
--- a/diff.c
+++ b/diff.c
@@ -6209,24 +6209,6 @@ int diff_can_quit_early(struct diff_options *opt)
 		opt->flags.has_changes);
 }
 
-/*
- * Shall changes to this submodule be ignored?
- *
- * Submodule changes can be configured to be ignored separately for each path,
- * but that configuration can be overridden from the command line.
- */
-static int is_submodule_ignored(const char *path, struct diff_options *options)
-{
-	int ignored = 0;
-	struct diff_flags orig_flags = options->flags;
-	if (!options->flags.override_submodule_config)
-		set_diffopt_flags_from_submodule_config(options, path);
-	if (options->flags.ignore_submodules)
-		ignored = 1;
-	options->flags = orig_flags;
-	return ignored;
-}
-
 void diff_addremove(struct diff_options *options,
 		    int addremove, unsigned mode,
 		    const struct object_id *oid,
@@ -6235,7 +6217,7 @@ void diff_addremove(struct diff_options *options,
 {
 	struct diff_filespec *one, *two;
 
-	if (S_ISGITLINK(mode) && is_submodule_ignored(concatpath, options))
+	if (S_ISGITLINK(mode) && options->flags.ignore_submodules)
 		return;
 
 	/* This may look odd, but it is a preparation for
@@ -6285,7 +6267,7 @@ void diff_change(struct diff_options *options,
 	struct diff_filepair *p;
 
 	if (S_ISGITLINK(old_mode) && S_ISGITLINK(new_mode) &&
-	    is_submodule_ignored(concatpath, options))
+	    options->flags.ignore_submodules)
 		return;
 
 	if (options->flags.reverse_diff) {
-- 
2.19.1

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

[Stefan Beller]

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

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

[Michael Forney]

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!

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

[Stefan Beller]

On Thu, Nov 15, 2018 at 4:31 PM Michael Forney <mforney@mforney.org> wrote:
>
> 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.

That sounds good.

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

-a as short for --all in git commit, and I presumed it to be
'--all-content', but documentation tells me it's about files
only, so there is no really an "all" rule, but rather me misunderstanding
to what it applies.

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

[Stefan Beller]

> I have a git repository which contains a number of submodules that
> refer to external repositories. Some of these repositories need to
> patched in some way, so patches are stored alongside the submodules,
> and are applied when building. This mostly works fine, but causes
> submodules to show up as modified in `git status` and get updated with
> `git commit -a`. To resolve this, I've added `ignore = all` to
> .gitmodules for all the submodules that need patches applied. This
> way, I can explicitly `git add` the submodule when I want to update
> the base commit, but otherwise pretend that they are clean. This has
> worked pretty well for me, but less so since git 2.15 when this issue
> was introduced.

> > This is really bad. git-status and git-commit share some code,
> > and we'll populate the commit message with a status output.
> > So it seems reasonable to expect the status and the commit to match,
> > i.e. if status tells me there is no change, then commit should not record
> > the submodule update.
>
> I just checked and if I don't specify a message on the command-line,
> the status output in the message template *does* mention that `inner`
> is getting updated.

That's good.

> >> > There have been a couple occasions where I accidentally pushed local
> >> > changes to ignored submodules because of this. Since they don't show
> >> > up in the log output, it is difficult to figure out what actually has
> >> > gone wrong.
> >
> > How was it prevented before? Just by git commit -a not picking up the
> > submodule change?
>
> Yes. Previously, `git commit -a` would not pick up the change (unless
> I added it explicitly with `git add`), and `git log` would still show
> changes to ignored submodules (which is the behavior I want).

and both are broken currently (commit -a will commit a submodule if
it is changed, and it will also not show that in log, but it did show that
it is committing it in the commit message template)

> I just came across someone else affected by this issue:
> https://github.com/git/git/commit/55568086#commitcomment-27137460

Point taken.