{"thread":{"id":"48063","subject":"Confusing behavior with ignored submodules and `git commit -a`","startedAt":"2018-03-17T03:57:12Z","lastAt":"2018-11-27T00:03:21Z","messageCount":12,"participants":["Michael Forney","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"341971","messageId":"CAGw6cBvLDNtYT6vfHcxmX0S_SS1vmYVCEkSD_ixah6cGKJ4H9w@mail.gmail.com","threadId":"48063","inReplyTo":null,"subject":"Confusing behavior with ignored submodules and `git commit -a`","fromName":"Michael Forney","fromEmail":"mforney@mforney.org","sentAt":"2018-03-17T03:57:05Z","receivedAt":"2018-03-17T03:57:12Z","isPatch":false,"sender":{"key":"mforney@mforney.org","avatar":"https://avatars.githubusercontent.com/u/52851?v=4"},"body":"Hi,\n\nIn the past few months have noticed some confusing behavior with\nignored submodules. I finally got around to bisecting this to commit\n5556808690ea245708fb80383be5c1afee2fb3eb (add, reset: ensure\nsubmodules can be added or reset).\n\nHere is a demonstration of the problem:\n\nFirst some repository initialization with a submodule marked as ignored.\n\n$ git init inner && git -C inner commit --allow-empty -m 'inner 1'\nInitialized empty Git repository in /tmp/inner/.git/\n[master (root-commit) ef55bed] inner 1\n$ git init outer && cd outer\nInitialized empty Git repository in /tmp/outer/.git/\n$ git submodule add ../inner\nCloning into '/tmp/outer/inner'...\ndone.\n$ echo 1 > foo.txt && git add foo.txt\n$ git commit -m 'outer 1'\n[master (root-commit) efeb85c] outer 1\n 3 files changed, 5 insertions(+)\n create mode 100644 .gitmodules\n create mode 100644 foo.txt\n create mode 160000 inner\n$ git config submodule.inner.ignore all\n$ git -C inner commit --allow-empty -m 'inner 2'\n[master 7b7f0fa] inner 2\n$ git status\nOn branch master\nnothing to commit, working tree clean\n$\n\nUp to here is all expected. However, if I go to update `foo.txt` and\ncommit with `git commit -a`, changes to inner get recorded\nunexpectedly. What's worse is the shortstat output of `git commit -a`,\nand the diff output of `git show` give no indication that the\nsubmodule was changed.\n\n$ echo 2 > foo.txt\n$ git status\nOn branch master\nChanges not staged for commit:\n  (use \"git add <file>...\" to update what will be committed)\n  (use \"git checkout -- <file>...\" to discard changes in working directory)\n\n        modified:   foo.txt\n\nno changes added to commit (use \"git add\" and/or \"git commit -a\")\n$ git commit -a -m 'update foo.txt'\n[master 6ec564c] update foo.txt\n 1 file changed, 1 insertion(+), 1 deletion(-)\n$ git show\ncommit 6ec564c15ddae099c71f01750b4c434557525653 (HEAD -> master)\nAuthor: Michael Forney <mforney@mforney.org>\nDate:   Fri Mar 16 20:18:37 2018 -0700\n\n    update foo.txt\n\ndiff --git a/foo.txt b/foo.txt\nindex d00491f..0cfbf08 100644\n--- a/foo.txt\n+++ b/foo.txt\n@@ -1 +1 @@\n-1\n+2\n$\n\nThere have been a couple occasions where I accidentally pushed local\nchanges to ignored submodules because of this. Since they don't show\nup in the log output, it is difficult to figure out what actually has\ngone wrong.\n\nAnyway, since the bisected commit (555680869) only mentions add and\nreset, I'm assuming that this is a regression and not a deliberate\nbehavior change. The documentation for submodule.<name>.ignore states\nthat the setting should only affect `git status` and the diff family.\nIn terms of my expectations, I would go further and say it should only\naffect `git status` and diffs against the working tree.\n\nI took a brief look through the relevant sources, and it wasn't clear\nto me how to fix this without accidentally changing the behavior of\nother subcommands.\n\nAny help with this issue is appreciated!\n"},{"id":"361536","messageId":"CAGw6cBvaC+TEOM9Tjdbs5zkz2hzW4649=4rsAo58cNOVHOQS=Q@mail.gmail.com","threadId":"48063","inReplyTo":"CAGw6cBvLDNtYT6vfHcxmX0S_SS1vmYVCEkSD_ixah6cGKJ4H9w@mail.gmail.com","subject":"Re: Confusing behavior with ignored submodules and `git commit -a`","fromName":"Michael Forney","fromEmail":"mforney@mforney.org","sentAt":"2018-10-25T18:03:39Z","receivedAt":"2018-10-25T18:03:44Z","isPatch":false,"sender":{"key":"mforney@mforney.org","avatar":"https://avatars.githubusercontent.com/u/52851?v=4"},"body":"On 2018-03-16, Michael Forney <mforney@mforney.org> wrote:\n> Hi,\n>\n> In the past few months have noticed some confusing behavior with\n> ignored submodules. I finally got around to bisecting this to commit\n> 5556808690ea245708fb80383be5c1afee2fb3eb (add, reset: ensure\n> submodules can be added or reset).\n>\n> Here is a demonstration of the problem:\n>\n> First some repository initialization with a submodule marked as ignored.\n>\n> $ git init inner && git -C inner commit --allow-empty -m 'inner 1'\n> Initialized empty Git repository in /tmp/inner/.git/\n> [master (root-commit) ef55bed] inner 1\n> $ git init outer && cd outer\n> Initialized empty Git repository in /tmp/outer/.git/\n> $ git submodule add ../inner\n> Cloning into '/tmp/outer/inner'...\n> done.\n> $ echo 1 > foo.txt && git add foo.txt\n> $ git commit -m 'outer 1'\n> [master (root-commit) efeb85c] outer 1\n>  3 files changed, 5 insertions(+)\n>  create mode 100644 .gitmodules\n>  create mode 100644 foo.txt\n>  create mode 160000 inner\n> $ git config submodule.inner.ignore all\n> $ git -C inner commit --allow-empty -m 'inner 2'\n> [master 7b7f0fa] inner 2\n> $ git status\n> On branch master\n> nothing to commit, working tree clean\n> $\n>\n> Up to here is all expected. However, if I go to update `foo.txt` and\n> commit with `git commit -a`, changes to inner get recorded\n> unexpectedly. What's worse is the shortstat output of `git commit -a`,\n> and the diff output of `git show` give no indication that the\n> submodule was changed.\n>\n> $ echo 2 > foo.txt\n> $ git status\n> On branch master\n> Changes not staged for commit:\n>   (use \"git add <file>...\" to update what will be committed)\n>   (use \"git checkout -- <file>...\" to discard changes in working directory)\n>\n>         modified:   foo.txt\n>\n> no changes added to commit (use \"git add\" and/or \"git commit -a\")\n> $ git commit -a -m 'update foo.txt'\n> [master 6ec564c] update foo.txt\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> $ git show\n> commit 6ec564c15ddae099c71f01750b4c434557525653 (HEAD -> master)\n> Author: Michael Forney <mforney@mforney.org>\n> Date:   Fri Mar 16 20:18:37 2018 -0700\n>\n>     update foo.txt\n>\n> diff --git a/foo.txt b/foo.txt\n> index d00491f..0cfbf08 100644\n> --- a/foo.txt\n> +++ b/foo.txt\n> @@ -1 +1 @@\n> -1\n> +2\n> $\n>\n> There have been a couple occasions where I accidentally pushed local\n> changes to ignored submodules because of this. Since they don't show\n> up in the log output, it is difficult to figure out what actually has\n> gone wrong.\n>\n> Anyway, since the bisected commit (555680869) only mentions add and\n> reset, I'm assuming that this is a regression and not a deliberate\n> behavior change. The documentation for submodule.<name>.ignore states\n> that the setting should only affect `git status` and the diff family.\n> In terms of my expectations, I would go further and say it should only\n> affect `git status` and diffs against the working tree.\n>\n> I took a brief look through the relevant sources, and it wasn't clear\n> to me how to fix this without accidentally changing the behavior of\n> other subcommands.\n>\n> Any help with this issue is appreciated!\n\nI accidentally pushed local changes to ignored submodules again due to this.\n\nCan anyone confirm whether this is the intended behavior of ignore? If\nit is, then at least the documentation needs an update saying that\n`commit -a` will commit all submodule changes, even if they are\nignored.\n"},{"id":"361537","messageId":"CAGZ79ka=tkKYNkPmSjhomcfAPbEg6PQPSRtpe3uq2B45fNoyjg@mail.gmail.com","threadId":"48063","inReplyTo":"CAGw6cBvaC+TEOM9Tjdbs5zkz2hzW4649=4rsAo58cNOVHOQS=Q@mail.gmail.com","subject":"Re: Confusing behavior with ignored submodules and `git commit -a`","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-25T18:26:46Z","receivedAt":"2018-10-25T18:27:01Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 25, 2018 at 11:03 AM Michael Forney <mforney@mforney.org> wrote:\n>\n> On 2018-03-16, Michael Forney <mforney@mforney.org> wrote:\n> > Hi,\n> >\n> > In the past few months have noticed some confusing behavior with\n> > ignored submodules. I finally got around to bisecting this to commit\n> > 5556808690ea245708fb80383be5c1afee2fb3eb (add, reset: ensure\n> > submodules can be added or reset).\n\nUh. :(\n\nSee the discussion starting at\nhttps://public-inbox.org/git/20170725213928.125998-4-bmwill@google.com/\nspecifically\nhttps://public-inbox.org/git/xmqqinieq49v.fsf@gitster.mtv.corp.google.com/\n\n\n> >\n> > Here is a demonstration of the problem:\n> >\n[...]\n> > Up to here is all expected.\n\nMakes sense.\n\n> > However, if I go to update `foo.txt` and\n> > commit with `git commit -a`, changes to inner get recorded\n> > unexpectedly. What's worse is the shortstat output of `git commit -a`,\n> > and the diff output of `git show` give no indication that the\n> > submodule was changed.\n\nThis is really bad. git-status and git-commit share some code,\nand we'll populate the commit message with a status output.\nSo it seems reasonable to expect the status and the commit to match,\ni.e. if status tells me there is no change, then commit should not record\nthe submodule update.\n\n> > $ git commit -a -m 'update foo.txt'\n> > [master 6ec564c] update foo.txt\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> > $ git show\n> > commit 6ec564c15ddae099c71f01750b4c434557525653 (HEAD -> master)\n> > Author: Michael Forney <mforney@mforney.org>\n> > Date:   Fri Mar 16 20:18:37 2018 -0700\n> >\n> >     update foo.txt\n> >\n> > diff --git a/foo.txt b/foo.txt\n> > index d00491f..0cfbf08 100644\n> > --- a/foo.txt\n> > +++ b/foo.txt\n> > @@ -1 +1 @@\n> > -1\n> > +2\n> > $\n> >\n> > There have been a couple occasions where I accidentally pushed local\n> > changes to ignored submodules because of this. Since they don't show\n> > up in the log output, it is difficult to figure out what actually has\n> > gone wrong.\n\nHow was it prevented before? Just by git commit -a not picking up the\nsubmodule change?\n\n> >\n> > Anyway, since the bisected commit (555680869) only mentions add and\n> > reset, I'm assuming that this is a regression and not a deliberate\n> > behavior change. The documentation for submodule.<name>.ignore states\n> > that the setting should only affect `git status` and the diff family.\n> > In terms of my expectations, I would go further and say it should only\n> > affect `git status` and diffs against the working tree.\n> >\n> > I took a brief look through the relevant sources, and it wasn't clear\n> > to me how to fix this without accidentally changing the behavior of\n> > other subcommands.\n> >\n> > Any help with this issue is appreciated!\n\nI guess reverting that commit is not a good idea now, as\nI would expect something to break.\n\nMaybe looking through the series 614ea03a71\n(Merge branch 'bw/submodule-config-cleanup', 2017-08-26)\nto understand why it happened in the context would be a good start.\n\n> I accidentally pushed local changes to ignored submodules again due to this.\n>\n> Can anyone confirm whether this is the intended behavior of ignore? If\n> it is, then at least the documentation needs an update saying that\n> `commit -a` will commit all submodule changes, even if they are\n> ignored.\n\nThe docs say \"(but it will nonetheless show up in the output of\nstatus and commit when it has been staged)\" as well, so that commit\nsounds like a regression?\n\nStefan\n"},{"id":"363413","messageId":"CAGw6cBvJSswpvrMwKU9b+ANEHO4tWjWVhLL54nUyod2NoHJe1w@mail.gmail.com","threadId":"48063","inReplyTo":"CAGZ79ka=tkKYNkPmSjhomcfAPbEg6PQPSRtpe3uq2B45fNoyjg@mail.gmail.com","subject":"Re: Confusing behavior with ignored submodules and `git commit -a`","fromName":"Michael Forney","fromEmail":"mforney@mforney.org","sentAt":"2018-11-15T05:12:18Z","receivedAt":"2018-11-15T05:12:21Z","isPatch":false,"sender":{"key":"mforney@mforney.org","avatar":"https://avatars.githubusercontent.com/u/52851?v=4"},"body":"On 2018-10-25, Stefan Beller <sbeller@google.com> wrote:\n> On Thu, Oct 25, 2018 at 11:03 AM Michael Forney <mforney@mforney.org>\n> wrote:\n>>\n>> On 2018-03-16, Michael Forney <mforney@mforney.org> wrote:\n>> > Hi,\n>> >\n>> > In the past few months have noticed some confusing behavior with\n>> > ignored submodules. I finally got around to bisecting this to commit\n>> > 5556808690ea245708fb80383be5c1afee2fb3eb (add, reset: ensure\n>> > submodules can be added or reset).\n>\n> Uh. :(\n>\n> See the discussion starting at\n> https://public-inbox.org/git/20170725213928.125998-4-bmwill@google.com/\n> specifically\n> https://public-inbox.org/git/xmqqinieq49v.fsf@gitster.mtv.corp.google.com/\n\nThanks for the links. Let me explain how I'm using\nsubmodule.<name>.ignore. Maybe there's a better mechanism in git to\ndeal with this (if .ignore is a misfeature).\n\nI have a git repository which contains a number of submodules that\nrefer to external repositories. Some of these repositories need to\npatched in some way, so patches are stored alongside the submodules,\nand are applied when building. This mostly works fine, but causes\nsubmodules to show up as modified in `git status` and get updated with\n`git commit -a`. To resolve this, I've added `ignore = all` to\n.gitmodules for all the submodules that need patches applied. This\nway, I can explicitly `git add` the submodule when I want to update\nthe base commit, but otherwise pretend that they are clean. This has\nworked pretty well for me, but less so since git 2.15 when this issue\nwas introduced.\n\nOf course, I could maintain and publish forks of those repositories\nand use those as the remote for the submodules. However in many cases\nthese patches are just temporary until they get applied upstream and a\nnew release is made, and I don't really want to keep mirrors\nunnecessarily, or keep switching the submodule URL between upstream\nand my fork.\n\n>> > However, if I go to update `foo.txt` and\n>> > commit with `git commit -a`, changes to inner get recorded\n>> > unexpectedly. What's worse is the shortstat output of `git commit -a`,\n>> > and the diff output of `git show` give no indication that the\n>> > submodule was changed.\n>\n> This is really bad. git-status and git-commit share some code,\n> and we'll populate the commit message with a status output.\n> So it seems reasonable to expect the status and the commit to match,\n> i.e. if status tells me there is no change, then commit should not record\n> the submodule update.\n\nI just checked and if I don't specify a message on the command-line,\nthe status output in the message template *does* mention that `inner`\nis getting updated.\n\n>> > There have been a couple occasions where I accidentally pushed local\n>> > changes to ignored submodules because of this. Since they don't show\n>> > up in the log output, it is difficult to figure out what actually has\n>> > gone wrong.\n>\n> How was it prevented before? Just by git commit -a not picking up the\n> submodule change?\n\nYes. Previously, `git commit -a` would not pick up the change (unless\nI added it explicitly with `git add`), and `git log` would still show\nchanges to ignored submodules (which is the behavior I want).\n\n> I guess reverting that commit is not a good idea now, as\n> I would expect something to break.\n>\n> Maybe looking through the series 614ea03a71\n> (Merge branch 'bw/submodule-config-cleanup', 2017-08-26)\n> to understand why it happened in the context would be a good start.\n\nThanks, that's a good idea. I'll take a look through that series.\n\n>> I accidentally pushed local changes to ignored submodules again due to\n>> this.\n>>\n>> Can anyone confirm whether this is the intended behavior of ignore? If\n>> it is, then at least the documentation needs an update saying that\n>> `commit -a` will commit all submodule changes, even if they are\n>> ignored.\n>\n> The docs say \"(but it will nonetheless show up in the output of\n> status and commit when it has been staged)\" as well, so that commit\n> sounds like a regression?\n\nI just came across someone else affected by this issue:\nhttps://github.com/git/git/commit/55568086#commitcomment-27137460\n"},{"id":"363414","messageId":"CAGw6cBth+j+vAjhrQutxBXAkuJrBfHKG4GdCu1jpvAAXOwudEA@mail.gmail.com","threadId":"48063","inReplyTo":"CAGw6cBvJSswpvrMwKU9b+ANEHO4tWjWVhLL54nUyod2NoHJe1w@mail.gmail.com","subject":"Re: Confusing behavior with ignored submodules and `git commit -a`","fromName":"Michael Forney","fromEmail":"mforney@mforney.org","sentAt":"2018-11-15T06:05:05Z","receivedAt":"2018-11-15T06:05:08Z","isPatch":false,"sender":{"key":"mforney@mforney.org","avatar":"https://avatars.githubusercontent.com/u/52851?v=4"},"body":"+bmwill\n\nOn 2018-11-14, Michael Forney <mforney@mforney.org> wrote:\n> On 2018-10-25, Stefan Beller <sbeller@google.com> wrote:\n>> I guess reverting that commit is not a good idea now, as\n>> I would expect something to break.\n>>\n>> Maybe looking through the series 614ea03a71\n>> (Merge branch 'bw/submodule-config-cleanup', 2017-08-26)\n>> to understand why it happened in the context would be a good start.\n>\n> Thanks, that's a good idea. I'll take a look through that series.\n\nInteresting. If I build git from master after reverting 55568086, I do\nindeed observe the issue it claims to fix (unable to add ignored\nsubmodules). However, if I build from 9ef23f91fc (the immediate parent\nof 55568086), I do not see the issue.\n\nInvestigating this further, it seems that 55568086 addresses an issue\nthat does not appear until later on in the series in ff6f1f564c\n(submodule-config: lazy-load a repository's .gitmodules file). Perhaps\nthis was a result of reordering commits during a rebase. In other\nwords, I get correct behavior until 55568086, and in\n55568086..ff6f1f564c^ if I revert 55568086.\n\nLooking at ff6f1f564c, I don't really see anything that might be\nrelated to git-add, git-reset, or git-diff, so I'm guessing that this\nonly worked before because the submodule config wasn't getting loaded\nduring `git add` or `git reset`. Now that the config is loaded\nautomatically, submodule.<name>.ignore started taking effect where it\nshouldn't.\n\nUnfortunately, this doesn't really get me much closer to finding a fix.\n"},{"id":"363447","messageId":"CAGZ79kb+KSt_verjDR0yMj3y_QjsS5npX151iKP5hZL0HvUs+g@mail.gmail.com","threadId":"48063","inReplyTo":"CAGw6cBvJSswpvrMwKU9b+ANEHO4tWjWVhLL54nUyod2NoHJe1w@mail.gmail.com","subject":"Re: Confusing behavior with ignored submodules and `git commit -a`","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-15T19:39:19Z","receivedAt":"2018-11-15T19:39:33Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> I have a git repository which contains a number of submodules that\n> refer to external repositories. Some of these repositories need to\n> patched in some way, so patches are stored alongside the submodules,\n> and are applied when building. This mostly works fine, but causes\n> submodules to show up as modified in `git status` and get updated with\n> `git commit -a`. To resolve this, I've added `ignore = all` to\n> .gitmodules for all the submodules that need patches applied. This\n> way, I can explicitly `git add` the submodule when I want to update\n> the base commit, but otherwise pretend that they are clean. This has\n> worked pretty well for me, but less so since git 2.15 when this issue\n> was introduced.\n\n> > This is really bad. git-status and git-commit share some code,\n> > and we'll populate the commit message with a status output.\n> > So it seems reasonable to expect the status and the commit to match,\n> > i.e. if status tells me there is no change, then commit should not record\n> > the submodule update.\n>\n> I just checked and if I don't specify a message on the command-line,\n> the status output in the message template *does* mention that `inner`\n> is getting updated.\n\nThat's good.\n\n> >> > There have been a couple occasions where I accidentally pushed local\n> >> > changes to ignored submodules because of this. Since they don't show\n> >> > up in the log output, it is difficult to figure out what actually has\n> >> > gone wrong.\n> >\n> > How was it prevented before? Just by git commit -a not picking up the\n> > submodule change?\n>\n> Yes. Previously, `git commit -a` would not pick up the change (unless\n> I added it explicitly with `git add`), and `git log` would still show\n> changes to ignored submodules (which is the behavior I want).\n\nand both are broken currently (commit -a will commit a submodule if\nit is changed, and it will also not show that in log, but it did show that\nit is committing it in the commit message template)\n\n> I just came across someone else affected by this issue:\n> https://github.com/git/git/commit/55568086#commitcomment-27137460\n\nPoint taken.\n"},{"id":"363450","messageId":"CAGZ79kYiWnciitwTQCXR5bHOj7nhHWr40xBiS5sPCH5W4_yQ5w@mail.gmail.com","threadId":"48063","inReplyTo":"CAGw6cBth+j+vAjhrQutxBXAkuJrBfHKG4GdCu1jpvAAXOwudEA@mail.gmail.com","subject":"Re: Confusing behavior with ignored submodules and `git commit -a`","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-15T20:03:06Z","receivedAt":"2018-11-15T20:03:21Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 14, 2018 at 10:05 PM Michael Forney <mforney@mforney.org> wrote:\n>\n> +bmwill\n>\n> On 2018-11-14, Michael Forney <mforney@mforney.org> wrote:\n> > On 2018-10-25, Stefan Beller <sbeller@google.com> wrote:\n> >> I guess reverting that commit is not a good idea now, as\n> >> I would expect something to break.\n> >>\n> >> Maybe looking through the series 614ea03a71\n> >> (Merge branch 'bw/submodule-config-cleanup', 2017-08-26)\n> >> to understand why it happened in the context would be a good start.\n> >\n> > Thanks, that's a good idea. I'll take a look through that series.\n>\n> Interesting. If I build git from master after reverting 55568086, I do\n> indeed observe the issue it claims to fix (unable to add ignored\n> submodules). However, if I build from 9ef23f91fc (the immediate parent\n> of 55568086), I do not see the issue.\n>\n> Investigating this further, it seems that 55568086 addresses an issue\n> that does not appear until later on in the series in ff6f1f564c\n> (submodule-config: lazy-load a repository's .gitmodules file). Perhaps\n> this was a result of reordering commits during a rebase. In other\n> words, I get correct behavior until 55568086, and in\n> 55568086..ff6f1f564c^ if I revert 55568086.\n>\n> Looking at ff6f1f564c, I don't really see anything that might be\n> related to git-add, git-reset, or git-diff, so I'm guessing that this\n> only worked before because the submodule config wasn't getting loaded\n> during `git add` or `git reset`. Now that the config is loaded\n> automatically, submodule.<name>.ignore started taking effect where it\n> shouldn't.\n>\n> Unfortunately, this doesn't really get me much closer to finding a fix.\n\nMaybe selectively unloading or overwriting the config?\n\nOr we can change is_submodule_ignored() in diff.c\nto be only applied selectively whether we are running the\nright command? For this approach we'd have to figure out the\nset of commands to which the ignore config should apply or\nnot (and come up with a more concise documentation then)\n\nThis approach sounds appealing to me as it would cover\nnew commands as well and we'd only have a central point\nwhere the decision for ignoring is made.\n\nStefan\n"},{"id":"363452","messageId":"CAGw6cBvLGZKcf1em0d47hcCuKau2QVbX4wfb0yN+m4umbNLaRg@mail.gmail.com","threadId":"48063","inReplyTo":"CAGZ79kYiWnciitwTQCXR5bHOj7nhHWr40xBiS5sPCH5W4_yQ5w@mail.gmail.com","subject":"Re: Confusing behavior with ignored submodules and `git commit -a`","fromName":"Michael Forney","fromEmail":"mforney@mforney.org","sentAt":"2018-11-15T21:33:57Z","receivedAt":"2018-11-15T21:34:01Z","isPatch":false,"sender":{"key":"mforney@mforney.org","avatar":"https://avatars.githubusercontent.com/u/52851?v=4"},"body":"On 2018-11-15, Stefan Beller <sbeller@google.com> wrote:\n> On Wed, Nov 14, 2018 at 10:05 PM Michael Forney <mforney@mforney.org>\n> wrote:\n>> Looking at ff6f1f564c, I don't really see anything that might be\n>> related to git-add, git-reset, or git-diff, so I'm guessing that this\n>> only worked before because the submodule config wasn't getting loaded\n>> during `git add` or `git reset`. Now that the config is loaded\n>> automatically, submodule.<name>.ignore started taking effect where it\n>> shouldn't.\n>>\n>> Unfortunately, this doesn't really get me much closer to finding a fix.\n>\n> Maybe selectively unloading or overwriting the config?\n>\n> Or we can change is_submodule_ignored() in diff.c\n> to be only applied selectively whether we are running the\n> right command? For this approach we'd have to figure out the\n> set of commands to which the ignore config should apply or\n> not (and come up with a more concise documentation then)\n>\n> This approach sounds appealing to me as it would cover\n> new commands as well and we'd only have a central point\n> where the decision for ignoring is made.\n\nWell, currently the submodule config can be disabled in diff_flags by\nsetting override_submodule_config=1. However, I'm thinking it may be\nsimpler to selectively *enable* the submodule config in diff_flags\nwhere it is needed instead of disabling it everywhere else (i.e.\nuse_submodule_config instead of override_submodule_config).\n\nI'm also starting to see why this is tricky. The only difference that\ndiff.c:run_diff_files sees between `git add inner` and `git add --all`\nis whether the index entry matched the pathspec exactly or not.\n\nHere is a work-in-progress diff that seems to have the correct\nbehavior in all cases I tried. Can you think of any cases that it\nbreaks? I'm not quite sure of the consequences of having diff_change\nand diff_addremove always ignore the submodule config; git-diff and\ngit-status still seem to work correctly.\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex f65c17229..9902f7742 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix,\n \trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = update_callback;\n \trev.diffopt.format_callback_data = &data;\n-\trev.diffopt.flags.override_submodule_config = 1;\n \trev.max_count = 0; /* do not compare unmerged paths with stage #2 */\n \trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n \tclear_pathspec(&rev.prune_data);\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 83fce5151..fbb048cca 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry\n*ce, struct stat *st)\n static int match_stat_with_submodule(struct diff_options *diffopt,\n \t\t\t\t     const struct cache_entry *ce,\n \t\t\t\t     struct stat *st, unsigned ce_option,\n-\t\t\t\t     unsigned *dirty_submodule)\n+\t\t\t\t     unsigned *dirty_submodule,\n+\t\t\t\t     int exact)\n {\n \tint changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);\n \tif (S_ISGITLINK(ce->ce_mode)) {\n \t\tstruct diff_flags orig_flags = diffopt->flags;\n-\t\tif (!diffopt->flags.override_submodule_config)\n+\t\tif (!diffopt->flags.override_submodule_config && !exact)\n \t\t\tset_diffopt_flags_from_submodule_config(diffopt, ce->name);\n \t\tif (diffopt->flags.ignore_submodules)\n \t\t\tchanged = 0;\n@@ -88,7 +89,7 @@ static int match_stat_with_submodule(struct\ndiff_options *diffopt,\n\n int run_diff_files(struct rev_info *revs, unsigned int option)\n {\n-\tint entries, i;\n+\tint entries, i, matched;\n \tint diff_unmerged_stage = revs->max_count;\n \tunsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)\n \t\t\t      ? CE_MATCH_RACY_IS_DIRTY : 0);\n@@ -110,7 +111,8 @@ int run_diff_files(struct rev_info *revs, unsigned\nint option)\n \t\tif (diff_can_quit_early(&revs->diffopt))\n \t\t\tbreak;\n\n-\t\tif (!ce_path_match(istate, ce, &revs->prune_data, NULL))\n+\t\tmatched = ce_path_match(istate, ce, &revs->prune_data, NULL);\n+\t\tif (!matched)\n \t\t\tcontinue;\n\n \t\tif (ce_stage(ce)) {\n@@ -226,7 +228,8 @@ int run_diff_files(struct rev_info *revs, unsigned\nint option)\n \t\t\t}\n\n \t\t\tchanged = match_stat_with_submodule(&revs->diffopt, ce, &st,\n-\t\t\t\t\t\t\t    ce_option, &dirty_submodule);\n+\t\t\t\t\t\t\t    ce_option, &dirty_submodule,\n+\t\t\t\t\t\t\t    matched == MATCHED_EXACTLY);\n \t\t\tnewmode = ce_mode_from_stat(ce, st.st_mode);\n \t\t}\n\n@@ -292,7 +295,7 @@ static int get_stat_data(const struct cache_entry *ce,\n \t\t\treturn -1;\n \t\t}\n \t\tchanged = match_stat_with_submodule(diffopt, ce, &st,\n-\t\t\t\t\t\t    0, dirty_submodule);\n+\t\t\t\t\t\t    0, dirty_submodule, 0);\n \t\tif (changed) {\n \t\t\tmode = ce_mode_from_stat(ce, st.st_mode);\n \t\t\toid = &null_oid;\ndiff --git a/diff.c b/diff.c\nindex e38d1ecaf..73dc75286 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6209,24 +6209,6 @@ int diff_can_quit_early(struct diff_options *opt)\n \t\topt->flags.has_changes);\n }\n\n-/*\n- * Shall changes to this submodule be ignored?\n- *\n- * Submodule changes can be configured to be ignored separately for each path,\n- * but that configuration can be overridden from the command line.\n- */\n-static int is_submodule_ignored(const char *path, struct diff_options *options)\n-{\n-\tint ignored = 0;\n-\tstruct diff_flags orig_flags = options->flags;\n-\tif (!options->flags.override_submodule_config)\n-\t\tset_diffopt_flags_from_submodule_config(options, path);\n-\tif (options->flags.ignore_submodules)\n-\t\tignored = 1;\n-\toptions->flags = orig_flags;\n-\treturn ignored;\n-}\n-\n void diff_addremove(struct diff_options *options,\n \t\t    int addremove, unsigned mode,\n \t\t    const struct object_id *oid,\n@@ -6235,7 +6217,7 @@ void diff_addremove(struct diff_options *options,\n {\n \tstruct diff_filespec *one, *two;\n\n-\tif (S_ISGITLINK(mode) && is_submodule_ignored(concatpath, options))\n+\tif (S_ISGITLINK(mode) && options->flags.ignore_submodules)\n \t\treturn;\n\n \t/* This may look odd, but it is a preparation for\n@@ -6285,7 +6267,7 @@ void diff_change(struct diff_options *options,\n \tstruct diff_filepair *p;\n\n \tif (S_ISGITLINK(old_mode) && S_ISGITLINK(new_mode) &&\n-\t    is_submodule_ignored(concatpath, options))\n+\t    options->flags.ignore_submodules)\n \t\treturn;\n\n \tif (options->flags.reverse_diff) {\n"},{"id":"363454","messageId":"20181115215339.28289-1-mforney@mforney.org","threadId":"48063","inReplyTo":"CAGw6cBvLGZKcf1em0d47hcCuKau2QVbX4wfb0yN+m4umbNLaRg@mail.gmail.com","subject":"Re: Confusing behavior with ignored submodules and `git commit -a`","fromName":"Michael Forney","fromEmail":"mforney@mforney.org","sentAt":"2018-11-15T21:53:39Z","receivedAt":"2018-11-15T21:53:43Z","isPatch":false,"sender":{"key":"mforney@mforney.org","avatar":"https://avatars.githubusercontent.com/u/52851?v=4"},"body":"On 2018-11-15, Michael Forney <mforney@mforney.org> wrote:\n> Here is a work-in-progress diff that seems to have the correct\n> behavior in all cases I tried.\n\nI was hoping that gmail wouldn't mess with the whitespace, but apparently\nit has, sorry about that. Let me try again.\n\n---\n builtin/add.c |  1 -\n diff-lib.c    | 15 +++++++++------\n diff.c        | 22 ++--------------------\n 3 files changed, 11 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex f65c17229..9902f7742 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix,\n \trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = update_callback;\n \trev.diffopt.format_callback_data = &data;\n-\trev.diffopt.flags.override_submodule_config = 1;\n \trev.max_count = 0; /* do not compare unmerged paths with stage #2 */\n \trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n \tclear_pathspec(&rev.prune_data);\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 83fce5151..fbb048cca 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry *ce, struct stat *st)\n static int match_stat_with_submodule(struct diff_options *diffopt,\n \t\t\t\t     const struct cache_entry *ce,\n \t\t\t\t     struct stat *st, unsigned ce_option,\n-\t\t\t\t     unsigned *dirty_submodule)\n+\t\t\t\t     unsigned *dirty_submodule,\n+\t\t\t\t     int exact)\n {\n \tint changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);\n \tif (S_ISGITLINK(ce->ce_mode)) {\n \t\tstruct diff_flags orig_flags = diffopt->flags;\n-\t\tif (!diffopt->flags.override_submodule_config)\n+\t\tif (!diffopt->flags.override_submodule_config && !exact)\n \t\t\tset_diffopt_flags_from_submodule_config(diffopt, ce->name);\n \t\tif (diffopt->flags.ignore_submodules)\n \t\t\tchanged = 0;\n@@ -88,7 +89,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n \n int run_diff_files(struct rev_info *revs, unsigned int option)\n {\n-\tint entries, i;\n+\tint entries, i, matched;\n \tint diff_unmerged_stage = revs->max_count;\n \tunsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)\n \t\t\t      ? CE_MATCH_RACY_IS_DIRTY : 0);\n@@ -110,7 +111,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tif (diff_can_quit_early(&revs->diffopt))\n \t\t\tbreak;\n \n-\t\tif (!ce_path_match(istate, ce, &revs->prune_data, NULL))\n+\t\tmatched = ce_path_match(istate, ce, &revs->prune_data, NULL);\n+\t\tif (!matched)\n \t\t\tcontinue;\n \n \t\tif (ce_stage(ce)) {\n@@ -226,7 +228,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t}\n \n \t\t\tchanged = match_stat_with_submodule(&revs->diffopt, ce, &st,\n-\t\t\t\t\t\t\t    ce_option, &dirty_submodule);\n+\t\t\t\t\t\t\t    ce_option, &dirty_submodule,\n+\t\t\t\t\t\t\t    matched == MATCHED_EXACTLY);\n \t\t\tnewmode = ce_mode_from_stat(ce, st.st_mode);\n \t\t}\n \n@@ -292,7 +295,7 @@ static int get_stat_data(const struct cache_entry *ce,\n \t\t\treturn -1;\n \t\t}\n \t\tchanged = match_stat_with_submodule(diffopt, ce, &st,\n-\t\t\t\t\t\t    0, dirty_submodule);\n+\t\t\t\t\t\t    0, dirty_submodule, 0);\n \t\tif (changed) {\n \t\t\tmode = ce_mode_from_stat(ce, st.st_mode);\n \t\t\toid = &null_oid;\ndiff --git a/diff.c b/diff.c\nindex e38d1ecaf..73dc75286 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6209,24 +6209,6 @@ int diff_can_quit_early(struct diff_options *opt)\n \t\topt->flags.has_changes);\n }\n \n-/*\n- * Shall changes to this submodule be ignored?\n- *\n- * Submodule changes can be configured to be ignored separately for each path,\n- * but that configuration can be overridden from the command line.\n- */\n-static int is_submodule_ignored(const char *path, struct diff_options *options)\n-{\n-\tint ignored = 0;\n-\tstruct diff_flags orig_flags = options->flags;\n-\tif (!options->flags.override_submodule_config)\n-\t\tset_diffopt_flags_from_submodule_config(options, path);\n-\tif (options->flags.ignore_submodules)\n-\t\tignored = 1;\n-\toptions->flags = orig_flags;\n-\treturn ignored;\n-}\n-\n void diff_addremove(struct diff_options *options,\n \t\t    int addremove, unsigned mode,\n \t\t    const struct object_id *oid,\n@@ -6235,7 +6217,7 @@ void diff_addremove(struct diff_options *options,\n {\n \tstruct diff_filespec *one, *two;\n \n-\tif (S_ISGITLINK(mode) && is_submodule_ignored(concatpath, options))\n+\tif (S_ISGITLINK(mode) && options->flags.ignore_submodules)\n \t\treturn;\n \n \t/* This may look odd, but it is a preparation for\n@@ -6285,7 +6267,7 @@ void diff_change(struct diff_options *options,\n \tstruct diff_filepair *p;\n \n \tif (S_ISGITLINK(old_mode) && S_ISGITLINK(new_mode) &&\n-\t    is_submodule_ignored(concatpath, options))\n+\t    options->flags.ignore_submodules)\n \t\treturn;\n \n \tif (options->flags.reverse_diff) {\n-- \n2.19.1\n\n"},{"id":"363456","messageId":"CAGZ79ka-8a5Uqe21SdHiSG-8eQdbey60R_G=A6th64ow=vqfNg@mail.gmail.com","threadId":"48063","inReplyTo":"CAGw6cBvLGZKcf1em0d47hcCuKau2QVbX4wfb0yN+m4umbNLaRg@mail.gmail.com","subject":"Re: Confusing behavior with ignored submodules and `git commit -a`","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-15T22:23:00Z","receivedAt":"2018-11-15T22:23:16Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 15, 2018 at 1:33 PM Michael Forney <mforney@mforney.org> wrote:\n>\n> On 2018-11-15, Stefan Beller <sbeller@google.com> wrote:\n> > On Wed, Nov 14, 2018 at 10:05 PM Michael Forney <mforney@mforney.org>\n> > wrote:\n> >> Looking at ff6f1f564c, I don't really see anything that might be\n> >> related to git-add, git-reset, or git-diff, so I'm guessing that this\n> >> only worked before because the submodule config wasn't getting loaded\n> >> during `git add` or `git reset`. Now that the config is loaded\n> >> automatically, submodule.<name>.ignore started taking effect where it\n> >> shouldn't.\n> >>\n> >> Unfortunately, this doesn't really get me much closer to finding a fix.\n> >\n> > Maybe selectively unloading or overwriting the config?\n> >\n> > Or we can change is_submodule_ignored() in diff.c\n> > to be only applied selectively whether we are running the\n> > right command? For this approach we'd have to figure out the\n> > set of commands to which the ignore config should apply or\n> > not (and come up with a more concise documentation then)\n> >\n> > This approach sounds appealing to me as it would cover\n> > new commands as well and we'd only have a central point\n> > where the decision for ignoring is made.\n>\n> Well, currently the submodule config can be disabled in diff_flags by\n> setting override_submodule_config=1. However, I'm thinking it may be\n> simpler to selectively *enable* the submodule config in diff_flags\n> where it is needed instead of disabling it everywhere else (i.e.\n> use_submodule_config instead of override_submodule_config).\n\nThis sounds like undoing the good(?) part of the series that introduced\nthis regression, as before that we selectively loaded the submodule\nconfig, which lead to confusion when you forgot it. Selectively *enabling*\nthe submodule config sounds like that state before?\n\nOr do we *only* talk about enabling the ignore flag, while loading the\nrest of the submodule config automatic?\n\n> I'm also starting to see why this is tricky. The only difference that\n> diff.c:run_diff_files sees between `git add inner` and `git add --all`\n> is whether the index entry matched the pathspec exactly or not.\n\nUnrelated to the trickiness, I think we'd need to document the behavior\nof the -a flag in git-add and git-commit better as adding the diff below\nwill depart from the \"all\" rule again, which I thought was a strong\nmotivator for Brandons series (IIRC).\n\n> Here is a work-in-progress diff that seems to have the correct\n> behavior in all cases I tried. Can you think of any cases that it\n> breaks? I'm not quite sure of the consequences of having diff_change\n> and diff_addremove always ignore the submodule config; git-diff and\n> git-status still seem to work correctly.\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index f65c17229..9902f7742 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix,\n>         rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n>         rev.diffopt.format_callback = update_callback;\n>         rev.diffopt.format_callback_data = &data;\n> -       rev.diffopt.flags.override_submodule_config = 1;\n\nThis line partially reverts 5556808, taking 02f2f56bc377c28\ninto account.\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 83fce5151..fbb048cca 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry\n> *ce, struct stat *st)\n>  static int match_stat_with_submodule(struct diff_options *diffopt,\n>                                      const struct cache_entry *ce,\n>                                      struct stat *st, unsigned ce_option,\n> -                                    unsigned *dirty_submodule)\n> +                                    unsigned *dirty_submodule,\n> +                                    int exact)\n> [...];\n\nThis is an interesting take so far as it is all about *detecting* change\nhere via stat information and not like the previous (before the regression)\nwhere it was about correcting output.\n\nmatch_stat_with_submodule would grow its documentation to be\nslightly more complicated as a result.\n\n> diff --git a/diff.c b/diff.c\n> index e38d1ecaf..73dc75286 100644\n> --- a/diff.c\n> +++ b/diff.c\n> [...]\n> -static int is_submodule_ignored(const char *path, struct diff_options *options)\n> -{\n> [...]\n> -       if (S_ISGITLINK(mode) && is_submodule_ignored(concatpath, options))\n> +       if (S_ISGITLINK(mode) && options->flags.ignore_submodules)\n>                 return;\n\nThis basically inlines the function is_submodule_ignored,\nexcept for the part:\n\n    if (!options->flags.override_submodule_config)\n        set_diffopt_flags_from_submodule_config(options, path);\n\nbut that was taken care off in match_stat_with_submodule in diff-lib?\n\nThis WIP looks really promising, thanks for looking into this!\nStefan\n"},{"id":"363457","messageId":"CAGw6cBvsiW7nwkJ_DBhXOYkgh7ba+rm+pwd4tOAt1ohRvcCY=A@mail.gmail.com","threadId":"48063","inReplyTo":"CAGZ79ka-8a5Uqe21SdHiSG-8eQdbey60R_G=A6th64ow=vqfNg@mail.gmail.com","subject":"Re: Confusing behavior with ignored submodules and `git commit -a`","fromName":"Michael Forney","fromEmail":"mforney@mforney.org","sentAt":"2018-11-16T00:31:56Z","receivedAt":"2018-11-16T00:32:01Z","isPatch":false,"sender":{"key":"mforney@mforney.org","avatar":"https://avatars.githubusercontent.com/u/52851?v=4"},"body":"On 2018-11-15, Stefan Beller <sbeller@google.com> wrote:\n> On Thu, Nov 15, 2018 at 1:33 PM Michael Forney <mforney@mforney.org> wrote:\n>> Well, currently the submodule config can be disabled in diff_flags by\n>> setting override_submodule_config=1. However, I'm thinking it may be\n>> simpler to selectively *enable* the submodule config in diff_flags\n>> where it is needed instead of disabling it everywhere else (i.e.\n>> use_submodule_config instead of override_submodule_config).\n>\n> This sounds like undoing the good(?) part of the series that introduced\n> this regression, as before that we selectively loaded the submodule\n> config, which lead to confusion when you forgot it. Selectively *enabling*\n> the submodule config sounds like that state before?\n>\n> Or do we *only* talk about enabling the ignore flag, while loading the\n> rest of the submodule config automatic?\n\nYes, that is what I meant. I believe the automatic loading of\nsubmodule config is the right thing to do, it just uncovered cases\nwhere the current handling of override_submodule_config is not quite\nsufficient.\n\nMy suggestion of replacing override_submodule_config with\nuse_submodule_config is because it seems like there are fewer places\nwhere we want to apply the ignore config than not. I think it should\nonly apply in diffs against the working tree and when staging changes\nto the index (unless specified explicitly). The documentation just\nmentions the \"diff family\", but all but one of the possible values for\nsubmodule.<name>.ignore (\"all\") don't make sense unless comparing with\nthe working tree. This is also how show/log -p behaved in git <2.15.\nSo I think that clarifying that it is about modifications *to the\nworking tree* would be a good idea.\n\n>> I'm also starting to see why this is tricky. The only difference that\n>> diff.c:run_diff_files sees between `git add inner` and `git add --all`\n>> is whether the index entry matched the pathspec exactly or not.\n>\n> Unrelated to the trickiness, I think we'd need to document the behavior\n> of the -a flag in git-add and git-commit better as adding the diff below\n> will depart from the \"all\" rule again, which I thought was a strong\n> motivator for Brandons series (IIRC).\n\nCan you explain what you mean by the \"all\" rule?\n\n>> Here is a work-in-progress diff that seems to have the correct\n>> behavior in all cases I tried. Can you think of any cases that it\n>> breaks? I'm not quite sure of the consequences of having diff_change\n>> and diff_addremove always ignore the submodule config; git-diff and\n>> git-status still seem to work correctly.\n>>\n>> diff --git a/builtin/add.c b/builtin/add.c\n>> index f65c17229..9902f7742 100644\n>> --- a/builtin/add.c\n>> +++ b/builtin/add.c\n>> @@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix,\n>>         rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n>>         rev.diffopt.format_callback = update_callback;\n>>         rev.diffopt.format_callback_data = &data;\n>> -       rev.diffopt.flags.override_submodule_config = 1;\n>\n> This line partially reverts 5556808, taking 02f2f56bc377c28\n> into account.\n\nCorrect. The problem with 55568086 is that add_files_to_cache is used\nby both git-add and git-commit, regardless of whether --all was\nspecified. So, while it made it possible to stage ignored submodules,\nit also made it so the submodule ignore config was overridden in all\nuses of git-add and git-commit.\n\nSo, this diff attempts to make it so the ignore config is only applied\nwhen the pathspec matches exactly rather than just always overriding\nthe ignore config.\n\n>> diff --git a/diff-lib.c b/diff-lib.c\n>> index 83fce5151..fbb048cca 100644\n>> --- a/diff-lib.c\n>> +++ b/diff-lib.c\n>> @@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry\n>> *ce, struct stat *st)\n>>  static int match_stat_with_submodule(struct diff_options *diffopt,\n>>                                      const struct cache_entry *ce,\n>>                                      struct stat *st, unsigned ce_option,\n>> -                                    unsigned *dirty_submodule)\n>> +                                    unsigned *dirty_submodule,\n>> +                                    int exact)\n>> [...];\n>\n> This is an interesting take so far as it is all about *detecting* change\n> here via stat information and not like the previous (before the regression)\n> where it was about correcting output.\n>\n> match_stat_with_submodule would grow its documentation to be\n> slightly more complicated as a result.\n\nYes, this is one part I'm not quite happy with. I wonder if instead\nmatch_stat_with_submodule could be made simpler by moving the\nie_match_stat call up to the two call sites, and then it could be\nselectively called by run_diff_files depending on the value of\nmatched.\n\n>> diff --git a/diff.c b/diff.c\n>> index e38d1ecaf..73dc75286 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> [...]\n>> -static int is_submodule_ignored(const char *path, struct diff_options\n>> *options)\n>> -{\n>> [...]\n>> -       if (S_ISGITLINK(mode) && is_submodule_ignored(concatpath,\n>> options))\n>> +       if (S_ISGITLINK(mode) && options->flags.ignore_submodules)\n>>                 return;\n>\n> This basically inlines the function is_submodule_ignored,\n> except for the part:\n>\n>     if (!options->flags.override_submodule_config)\n>         set_diffopt_flags_from_submodule_config(options, path);\n>\n> but that was taken care off in match_stat_with_submodule in diff-lib?\n\nYeah, since run_diff_files already skips ignored submodules (except if\ngiven by name), and we still want the changes to be applied to the\nindex. Like I said, I'm not really sure if there are any other uses of\ndiff_change and diff_addremove where we actually do want the .ignore\nconfig applied.\n\nBut, maybe if I instead left\n`rev.diffopt.flags.override_submodule_config = 1` in builtin/add.c and\nchanged the condition in match_stat_with_submodule from `&& !exact` to\n`|| !exact`, these functions would not need to be changed. In other\nwords, we relax the behavior when override_submodule_config is\nspecified, rather than making it stricter when it is not specified.\n\nTesting this out, it looks like it fixes the behavior with add/commit\n--all, but still omits ignored submodules in diffs between commits\n(which is not the behavior I expect, see above). So, without the\nchanges to diff_change/diff_addremove we probably would need to add\noverride_submodule_config=1 to even more places.\n\nToo many negatives are making this difficult to reason about.\n\n> This WIP looks really promising, thanks for looking into this!\n\nThanks for taking the time to help me!\n"},{"id":"364108","messageId":"CAGZ79kax6NpHP-rouezE-A8a2jVN+sC1WnouFWDcaCqYWhs_9w@mail.gmail.com","threadId":"48063","inReplyTo":"CAGw6cBvsiW7nwkJ_DBhXOYkgh7ba+rm+pwd4tOAt1ohRvcCY=A@mail.gmail.com","subject":"Re: Confusing behavior with ignored submodules and `git commit -a`","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-27T00:03:06Z","receivedAt":"2018-11-27T00:03:21Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 15, 2018 at 4:31 PM Michael Forney <mforney@mforney.org> wrote:\n>\n> On 2018-11-15, Stefan Beller <sbeller@google.com> wrote:\n> > On Thu, Nov 15, 2018 at 1:33 PM Michael Forney <mforney@mforney.org> wrote:\n> >> Well, currently the submodule config can be disabled in diff_flags by\n> >> setting override_submodule_config=1. However, I'm thinking it may be\n> >> simpler to selectively *enable* the submodule config in diff_flags\n> >> where it is needed instead of disabling it everywhere else (i.e.\n> >> use_submodule_config instead of override_submodule_config).\n> >\n> > This sounds like undoing the good(?) part of the series that introduced\n> > this regression, as before that we selectively loaded the submodule\n> > config, which lead to confusion when you forgot it. Selectively *enabling*\n> > the submodule config sounds like that state before?\n> >\n> > Or do we *only* talk about enabling the ignore flag, while loading the\n> > rest of the submodule config automatic?\n>\n> Yes, that is what I meant. I believe the automatic loading of\n> submodule config is the right thing to do, it just uncovered cases\n> where the current handling of override_submodule_config is not quite\n> sufficient.\n\nThat sounds good.\n\n>\n> My suggestion of replacing override_submodule_config with\n> use_submodule_config is because it seems like there are fewer places\n> where we want to apply the ignore config than not. I think it should\n> only apply in diffs against the working tree and when staging changes\n> to the index (unless specified explicitly). The documentation just\n> mentions the \"diff family\", but all but one of the possible values for\n> submodule.<name>.ignore (\"all\") don't make sense unless comparing with\n> the working tree. This is also how show/log -p behaved in git <2.15.\n> So I think that clarifying that it is about modifications *to the\n> working tree* would be a good idea.\n>\n> >> I'm also starting to see why this is tricky. The only difference that\n> >> diff.c:run_diff_files sees between `git add inner` and `git add --all`\n> >> is whether the index entry matched the pathspec exactly or not.\n> >\n> > Unrelated to the trickiness, I think we'd need to document the behavior\n> > of the -a flag in git-add and git-commit better as adding the diff below\n> > will depart from the \"all\" rule again, which I thought was a strong\n> > motivator for Brandons series (IIRC).\n>\n> Can you explain what you mean by the \"all\" rule?\n\n-a as short for --all in git commit, and I presumed it to be\n'--all-content', but documentation tells me it's about files\nonly, so there is no really an \"all\" rule, but rather me misunderstanding\nto what it applies.\n"}]}