Git issues with submodules

Git issues with submodules

[Sergey Sharybin]

Hey everyone from Blender developers!

As you might already know, we've recently switched from SVN to Git to
host Blender sources. In general it works really awesome, but we've
got some issues with submodules.

in SVN we had separate repositories for addons and translations which
were attached to main tree as svn:external. The reason for this was:

1. Separate commit access between core sources and addons so nobody
accidentally breaks anything in the core.
2. Separate commit history to help tracking issues down.

For the most developers and all artists (yes, we've got loads of
artists who builds blender on their own) it makes sense to always
checkout latest versions of addons and translations when updating
working tree.

We used Git submodules as a replacement for svn:external, with some
tweaks and specific of update procedure.

Namely, we always do `git submodule update --remote` to pull all the
latest changes from submodules. This will mark checkout as modified
because submodule hash changes. To avoid infinite commits of submodule
hash we've added ignore=all to their configuration.

In most cases it works fine, but there're some circumstances when it
gives weirdo issues.

Namely, `git ls-files -m` will show addons as modified, regardless
ignore=all configuration. In the same time `git diff-index --name-only
HEAD --` will show no changes at all.

This leads to issues with Arcanist (which is a Phabricator's tool) who
considers addons as uncommited changes and either complains on this or
just adds this to commits.

This issue i might easily reproduce on my laptop with latest Git
1.8.4.3. There're also some more issues which happens to our
developers and which i can not quite reproduce.

Sometimes it happens so git checkout to another branch yields about
uncommited changes to addons and doesn't checkout to another branch.

My guess here is that submodule hash in master and branch was
different and having hash modified in master somehow prevented changes
from another branch to be checked out. In this case question would be:
what would be the proper way to checkout branches when having
submodules configured this way?

Second issue is that some developers still manages to commit changes
to submodule hash, which i have totally no idea why Git allows to
include such a changes. I could not do such a commits on purpose even.

Here're some links to help understanding what's going on:

- Blender repository browser: http://developer.blender.org/diffusion/B/
- Task in our tracker about issues we've got with Git:
http://developer.blender.org/T37528
- History of changes to addons hash:
http://developer.blender.org/diffusion/B/history/master/release/scripts/addons

We're totally new to git submodules and clarification (and maybe even
confirmed bug with ls-files -m :) would really be appreciated. We're
also open for suggestions about re-configuring our submodules so they
works in a way we'd expect this.

Thanks in advance!

-- 
With best regards, Sergey Sharybin

Re: Git issues with submodules

[Ramkumar Ramachandra]

[+CC: Jens, the goto-guy for submodules]

Sergey Sharybin wrote:
> Namely, `git ls-files -m` will show addons as modified, regardless
> ignore=all configuration. In the same time `git diff-index --name-only
> HEAD --` will show no changes at all.

This happens because diff-index handles submodules explicitly (see
diff-lib.c), while ls-files doesn't (see builtin/ls-files.c). My
opinion is that this is a bug, and git ls-files needs to be taught to
handle submodules properly.

> This leads to issues with Arcanist (which is a Phabricator's tool) who
> considers addons as uncommited changes and either complains on this or
> just adds this to commits.

Does Arcanist use `git ls-files -m` to check?

> There're also some more issues which happens to our
> developers and which i can not quite reproduce.

Do try to track down the other issues and let us know.

> Sometimes it happens so git checkout to another branch yields about
> uncommited changes to addons and doesn't checkout to another branch.

I've seldom used submodules with branches, so I'll let others chime in.

Cheers.

Re: Git issues with submodules

[Sergey Sharybin]

Hey,

Answers are inlined.


On Fri, Nov 22, 2013 at 5:16 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
>
> [+CC: Jens, the goto-guy for submodules]
>
> Sergey Sharybin wrote:
> > Namely, `git ls-files -m` will show addons as modified, regardless
> > ignore=all configuration. In the same time `git diff-index --name-only
> > HEAD --` will show no changes at all.
>
> This happens because diff-index handles submodules explicitly (see
> diff-lib.c), while ls-files doesn't (see builtin/ls-files.c). My
> opinion is that this is a bug, and git ls-files needs to be taught to
> handle submodules properly.

Shall i fire report somewhere or it's being handled by the folks
reading this ML?

> > This leads to issues with Arcanist (which is a Phabricator's tool) who
> > considers addons as uncommited changes and either complains on this or
> > just adds this to commits.
>
> Does Arcanist use `git ls-files -m` to check?

Yes, Arcanist uses `git ls-files -m` to check whether there're local
modifications. We might also contact phab developers asking to change
it to `git diff --name-only HEAD --`.  Is there a preferable way to
get list of modified files and are this command intended to output the
same results?

> > There're also some more issues which happens to our
> > developers and which i can not quite reproduce.
>
> Do try to track down the other issues and let us know.

I'm trying, but doesn't happen here on laptop yet. Will give it
another try (do have some ideas). Also directed our developers here
who experienced the issue and might give some details,

> > Sometimes it happens so git checkout to another branch yields about
> > uncommited changes to addons and doesn't checkout to another branch.
>
> I've seldom used submodules with branches, so I'll let others chime in.

Ok, thanks anyway :)

-- 
With best regards, Sergey Sharybin

Re: Git issues with submodules

[Ramkumar Ramachandra]

Sergey Sharybin wrote:
> On Fri, Nov 22, 2013 at 5:16 PM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>>
>> [+CC: Jens, the goto-guy for submodules]
>>
>> Sergey Sharybin wrote:
>> > Namely, `git ls-files -m` will show addons as modified, regardless
>> > ignore=all configuration. In the same time `git diff-index --name-only
>> > HEAD --` will show no changes at all.
>>
>> This happens because diff-index handles submodules explicitly (see
>> diff-lib.c), while ls-files doesn't (see builtin/ls-files.c). My
>> opinion is that this is a bug, and git ls-files needs to be taught to
>> handle submodules properly.
>
> Shall i fire report somewhere or it's being handled by the folks
> reading this ML?

Bugs are reported and tackled on the list.

>> > This leads to issues with Arcanist (which is a Phabricator's tool) who
>> > considers addons as uncommited changes and either complains on this or
>> > just adds this to commits.
>>
>> Does Arcanist use `git ls-files -m` to check?
>
> Yes, Arcanist uses `git ls-files -m` to check whether there're local
> modifications. We might also contact phab developers asking to change
> it to `git diff --name-only HEAD --`.  Is there a preferable way to
> get list of modified files and are this command intended to output the
> same results?

I just checked it out: it uses `git ls-files -m` to get the list of
unstaged changes; `git diff --name-only HEAD --` will list staged
changes as well.

Re: Git issues with submodules

[Jeff King]

On Fri, Nov 22, 2013 at 06:38:47PM +0530, Ramkumar Ramachandra wrote:

> >> Does Arcanist use `git ls-files -m` to check?
> >
> > Yes, Arcanist uses `git ls-files -m` to check whether there're local
> > modifications. We might also contact phab developers asking to change
> > it to `git diff --name-only HEAD --`.  Is there a preferable way to
> > get list of modified files and are this command intended to output the
> > same results?
> 
> I just checked it out: it uses `git ls-files -m` to get the list of
> unstaged changes; `git diff --name-only HEAD --` will list staged
> changes as well.

That diff command compares the working tree and HEAD; if you are trying
to match `ls-files -m`, you probably wanted just `git diff --name-only`
to compare the working tree and the index. Although in a script you'd
probably want to use the plumbing `git diff-files` instead.

-Peff

Re: Git issues with submodules

[Sergey Sharybin]

Ramkumar, not actually sure what you mean?

For me `git diff --name-only HEAD --` ignores changes to submodules
hash changes. Also apparently it became a known TODO for phabricator
developers [1].

Jeff, kinda trying to match yes. Just don't want changes to submodules
hash to be included.

So, after all is it expected behavior of ls-files or not and if not
shall i report it as a separate thread? :)

[1] https://secure.phabricator.com/rARCe62b23e67deacc24469525cc5dea2b297a5073fb


On Fri, Nov 22, 2013 at 9:11 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Nov 22, 2013 at 06:38:47PM +0530, Ramkumar Ramachandra wrote:
>
>> >> Does Arcanist use `git ls-files -m` to check?
>> >
>> > Yes, Arcanist uses `git ls-files -m` to check whether there're local
>> > modifications. We might also contact phab developers asking to change
>> > it to `git diff --name-only HEAD --`.  Is there a preferable way to
>> > get list of modified files and are this command intended to output the
>> > same results?
>>
>> I just checked it out: it uses `git ls-files -m` to get the list of
>> unstaged changes; `git diff --name-only HEAD --` will list staged
>> changes as well.
>
> That diff command compares the working tree and HEAD; if you are trying
> to match `ls-files -m`, you probably wanted just `git diff --name-only`
> to compare the working tree and the index. Although in a script you'd
> probably want to use the plumbing `git diff-files` instead.
>
> -Peff



-- 
With best regards, Sergey Sharybin

Re: Git issues with submodules

[Ramkumar Ramachandra]

Sergey Sharybin wrote:
> Ramkumar, not actually sure what you mean?
>
> For me `git diff --name-only HEAD --` ignores changes to submodules
> hash changes.

`git diff --name-only HEAD --` compares the worktree to HEAD (listing
both staged and unstaged changes); we want `git diff --name-only --`
to compare the worktree to the index (listing only unstaged changes),
as Peff notes.

> Also apparently it became a known TODO for phabricator
> developers [1].

That was me :)

> So, after all is it expected behavior of ls-files or not and if not
> shall i report it as a separate thread? :)

Actually, I doubt it's worth fixing ls-files. Your problem should be
fixed when this is merged (hopefully in a few hours):

  https://github.com/facebook/arcanist/pull/121

Cheers.

Re: Git issues with submodules

[Sergey Sharybin]

Ah, didn't notice you're the author of that pull-request Ramkumar :)

So guess issue with arc can be considered solved now. But i'm still
collecting more details about how to manage to commit change addons
hash without arc command even (it happens to Campbell Barton really
often).

Will report back when we'll know something.

On Fri, Nov 22, 2013 at 10:35 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Sergey Sharybin wrote:
>> Ramkumar, not actually sure what you mean?
>>
>> For me `git diff --name-only HEAD --` ignores changes to submodules
>> hash changes.
>
> `git diff --name-only HEAD --` compares the worktree to HEAD (listing
> both staged and unstaged changes); we want `git diff --name-only --`
> to compare the worktree to the index (listing only unstaged changes),
> as Peff notes.
>
>> Also apparently it became a known TODO for phabricator
>> developers [1].
>
> That was me :)
>
>> So, after all is it expected behavior of ls-files or not and if not
>> shall i report it as a separate thread? :)
>
> Actually, I doubt it's worth fixing ls-files. Your problem should be
> fixed when this is merged (hopefully in a few hours):
>
>   https://github.com/facebook/arcanist/pull/121
>
> Cheers.



-- 
With best regards, Sergey Sharybin

Re: Git issues with submodules

[Sergey Sharybin]

Ok, got it now.

To reproduce the issue:

- Run git submodule update --recursive to make sure their SHA is
changed. Then `git add /path/to/changed submodule` or just `git add .`
- Modify any file from the parent repository
- Neither of `git status`, `git diff` and `git diff-files --name-only`
will show changes to a submodule, only changes to that file which was
changed in parent repo.
- Make a git commit. It will not list changes to submodule as wll.
- `git show HEAD` will show changes to both file from and parent
repository (which is expected) and will also show changes to the
submodule hash (which is unexpected i'd say).

On Fri, Nov 22, 2013 at 11:01 PM, Sergey Sharybin <sergey.vfx@gmail.com> wrote:
> Ah, didn't notice you're the author of that pull-request Ramkumar :)
>
> So guess issue with arc can be considered solved now. But i'm still
> collecting more details about how to manage to commit change addons
> hash without arc command even (it happens to Campbell Barton really
> often).
>
> Will report back when we'll know something.
>
> On Fri, Nov 22, 2013 at 10:35 PM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> Sergey Sharybin wrote:
>>> Ramkumar, not actually sure what you mean?
>>>
>>> For me `git diff --name-only HEAD --` ignores changes to submodules
>>> hash changes.
>>
>> `git diff --name-only HEAD --` compares the worktree to HEAD (listing
>> both staged and unstaged changes); we want `git diff --name-only --`
>> to compare the worktree to the index (listing only unstaged changes),
>> as Peff notes.
>>
>>> Also apparently it became a known TODO for phabricator
>>> developers [1].
>>
>> That was me :)
>>
>>> So, after all is it expected behavior of ls-files or not and if not
>>> shall i report it as a separate thread? :)
>>
>> Actually, I doubt it's worth fixing ls-files. Your problem should be
>> fixed when this is merged (hopefully in a few hours):
>>
>>   https://github.com/facebook/arcanist/pull/121
>>
>> Cheers.
>
>
>
> --
> With best regards, Sergey Sharybin



-- 
With best regards, Sergey Sharybin

Re: Git issues with submodules

[Ramkumar Ramachandra]

Sergey Sharybin wrote:
> To reproduce the issue:
>
> - Run git submodule update --recursive to make sure their SHA is
> changed. Then `git add /path/to/changed submodule` or just `git add .`
> - Modify any file from the parent repository
> - Neither of `git status`, `git diff` and `git diff-files --name-only`
> will show changes to a submodule, only changes to that file which was
> changed in parent repo.
> - Make a git commit. It will not list changes to submodule as wll.
> - `git show HEAD` will show changes to both file from and parent
> repository (which is expected) and will also show changes to the
> submodule hash (which is unexpected i'd say).

Thanks Sergey; I can confirm that this is a bug. For some reason, the
`git add .` is adding the ignored submodule to the index. After that,

  $ git diff-index @

is not showing the ignored submodule. Let me see if I can dig through
this in greater detail.

Re: Git issues with submodules

[Jens Lehmann]

Am 22.11.2013 19:11, schrieb Ramkumar Ramachandra:
> Sergey Sharybin wrote:
>> To reproduce the issue:
>>
>> - Run git submodule update --recursive to make sure their SHA is
>> changed. Then `git add /path/to/changed submodule` or just `git add .`
>> - Modify any file from the parent repository
>> - Neither of `git status`, `git diff` and `git diff-files --name-only`
>> will show changes to a submodule, only changes to that file which was
>> changed in parent repo.
>> - Make a git commit. It will not list changes to submodule as wll.
>> - `git show HEAD` will show changes to both file from and parent
>> repository (which is expected) and will also show changes to the
>> submodule hash (which is unexpected i'd say).
> 
> Thanks Sergey; I can confirm that this is a bug.

Hmm, looks like git show also needs to be fixed to honor the
ignore setting from .gitmodules. It already does that for
diff.ignoreSubmodules from either .git/config or git -c and
also supports the --ignore-submodules command line option.
The following fixes this inconsistency for me:

---------------------->8-------------------
diff --git a/builtin/log.c b/builtin/log.c
index b708517..ca97cfb 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -25,6 +25,7 @@
 #include "version.h"
 #include "mailmap.h"
 #include "gpg-interface.h"
+#include "submodule.h"

 /* Set a default date-time format for git log ("log.date" config variable) */
 static const char *default_date_mode = NULL;
@@ -521,6 +522,7 @@ int cmd_show(int argc, const char **argv, const char *prefix
        int i, count, ret = 0;

        init_grep_defaults();
+       gitmodules_config();
        git_config(git_log_config, NULL);

        memset(&match_all, 0, sizeof(match_all));
---------------------->8-------------------

But the question is if that is the right thing to do: should
diff.ignoreSubmodules and submodule.<name>.ignore only affect
the diff family or also git log & friends? That would make
users blind for submodule history (which they already are
when using diff & friends, so that might be ok here too).

> For some reason, the
> `git add .` is adding the ignored submodule to the index.

The ignore setting is documented to only affect diff output
(including what checkout, commit and status show as modified).
While I agree that this behavior is confusing for Sergey and
not optimal for the floating branch model he uses, git is
currently doing exactly what it should. And for people using
the ignore setting to not having to stat submodules with huge
and/or many files that behavior is what they want: don't bother
me with what changed, but commit what I did change on purpose.
We may have to rethink what should happen for users of the
floating branch model though.

> After that,
> 
>   $ git diff-index @
> 
> is not showing the ignored submodule.

Of course it isn't, it's configured not to. You'll have to use
--ignore-submodules=dirty to override the configuration to make
it show differences in the recorded hash.

Re: Git issues with submodules

[Sergey Sharybin]

> > For some reason, the
> > `git add .` is adding the ignored submodule to the index.
>
> The ignore setting is documented to only affect diff output
> (including what checkout, commit and status show as modified).
> While I agree that this behavior is confusing for Sergey and
> not optimal for the floating branch model he uses, git is
> currently doing exactly what it should. And for people using
> the ignore setting to not having to stat submodules with huge
> and/or many files that behavior is what they want: don't bother
> me with what changed, but commit what I did change on purpose.
> We may have to rethink what should happen for users of the
> floating branch model though.
>

I totally see what's happening here and indeed current logic of `git
add .` agree is correct from how it was designed to. I could also see
why it might be useful to keep `git add .` and `git commit .` not to
respect submodule ignore flag. The only confusing thing here is that
if i stage changed submodule with this command i wouldn't see this
submodule in "changes to be committed" wen doing a commit.

So seems it's just matter of better communication of what's gonna to
be committed in "changes to be committed" section? Or maybe even make
it so `git status` will show staged changes from submdules hash
regardless ignore flag? Just an ideas how to make communication what's
going on a bit better :)

And for sure don't think suppressing stuff from git show is a nice
idea (if i understand your proposal f making submodule ignore option
affect on other commands).

-- 
With best regards, Sergey Sharybin

Re: Re: Git issues with submodules

[Heiko Voigt]

Hi,

On Fri, Nov 22, 2013 at 10:01:44PM +0100, Jens Lehmann wrote:
> Hmm, looks like git show also needs to be fixed to honor the
> ignore setting from .gitmodules. It already does that for
> diff.ignoreSubmodules from either .git/config or git -c and
> also supports the --ignore-submodules command line option.
> The following fixes this inconsistency for me:
> 
> ---------------------->8-------------------
> diff --git a/builtin/log.c b/builtin/log.c
> index b708517..ca97cfb 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -25,6 +25,7 @@
>  #include "version.h"
>  #include "mailmap.h"
>  #include "gpg-interface.h"
> +#include "submodule.h"
> 
>  /* Set a default date-time format for git log ("log.date" config variable) */
>  static const char *default_date_mode = NULL;
> @@ -521,6 +522,7 @@ int cmd_show(int argc, const char **argv, const char *prefix
>         int i, count, ret = 0;
> 
>         init_grep_defaults();
> +       gitmodules_config();
>         git_config(git_log_config, NULL);
> 
>         memset(&match_all, 0, sizeof(match_all));
> ---------------------->8-------------------
> 
> But the question is if that is the right thing to do: should
> diff.ignoreSubmodules and submodule.<name>.ignore only affect
> the diff family or also git log & friends? That would make
> users blind for submodule history (which they already are
> when using diff & friends, so that might be ok here too).
> 
> > For some reason, the
> > `git add .` is adding the ignored submodule to the index.
> 
> The ignore setting is documented to only affect diff output
> (including what checkout, commit and status show as modified).
> While I agree that this behavior is confusing for Sergey and
> not optimal for the floating branch model he uses, git is
> currently doing exactly what it should. And for people using
> the ignore setting to not having to stat submodules with huge
> and/or many files that behavior is what they want: don't bother
> me with what changed, but commit what I did change on purpose.
> We may have to rethink what should happen for users of the
> floating branch model though.

This gets more nasty. When using 'git add .' you secretly add the
submodule to the index. But it is neither shown in status nor diff
--cached. commit actually complains there is nothing to add. But then
once you add a local file to the index you can commit and secretly take
the submodule change with you.

What I think needs fixing here first is that the ignore setting should not
apply to any diffs between HEAD and index. IMO, it should only apply
to the diff between worktree and index.

When we have that the user does not see the submodule changed when
normally working. But after doing git add . the change to the submodule
should be shown in status and diff regardless of the configuration.

I will have a look at that.

After that we can discuss whether add should add submodules that are
tracked but not shown. How about commit -a ? Should it also ignore the
change? I am undecided here. There does not seem to be any good
decision. From the users point of view we should probably not add it
since its not visible in status. What do others think?

Cheers Heiko

Re: Re: Git issues with submodules

[Jonathan Nieder]

Heiko Voigt wrote:

> After that we can discuss whether add should add submodules that are
> tracked but not shown. How about commit -a ? Should it also ignore the
> change? I am undecided here. There does not seem to be any good
> decision. From the users point of view we should probably not add it
> since its not visible in status. What do others think?

I agree --- it should not add.

That leaves the question of how to add explicitly.  "git add -f"?
"git add --ignore-submodules=none"?

Thanks,
Jonathan

Re: Git issues with submodules

[Jens Lehmann]

Am 22.11.2013 23:09, schrieb Jonathan Nieder:
> Heiko Voigt wrote:
> 
>> After that we can discuss whether add should add submodules that are
>> tracked but not shown. How about commit -a ? Should it also ignore the
>> change? I am undecided here. There does not seem to be any good
>> decision. From the users point of view we should probably not add it
>> since its not visible in status. What do others think?
> 
> I agree --- it should not add.

I concur: adding a change that is hidden from the user during
the process is not a good idea.

> That leaves the question of how to add explicitly.  "git add -f"?
> "git add --ignore-submodules=none"?

I suspect "git add" and "git commit -a" have to learn the
--ignore-submodules option anyway if we go that route. There
are points in time (e.g. releasing a new version or having run
an expansive test successfully) where some users want to update
the submodules that are normally ignored to record the exact
versions involved.

Re: Re: Git issues with submodules

[Heiko Voigt]

Hi,

On Sat, Nov 23, 2013 at 09:10:44PM +0100, Jens Lehmann wrote:
> Am 22.11.2013 23:09, schrieb Jonathan Nieder:
> > Heiko Voigt wrote:
> > 
> >> After that we can discuss whether add should add submodules that are
> >> tracked but not shown. How about commit -a ? Should it also ignore the
> >> change? I am undecided here. There does not seem to be any good
> >> decision. From the users point of view we should probably not add it
> >> since its not visible in status. What do others think?
> > 
> > I agree --- it should not add.
> 
> I concur: adding a change that is hidden from the user during
> the process is not a good idea.

Here is a patch achieving that. Still missing a test which I will add.

Cheers Heiko

---8<----
Subject: [PATCH] fix 'git add' to skip submodules configured as ignored

If submodules are configured as ignore=all they are not shown by status.
Lets also ignore them when adding files to the index. This avoids that
users accidentially add ignored submodules with: git add .

We achieve this by reading the submodule config and thus correctly
initializing the infrastructure to take the ignore decision.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 builtin/add.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/builtin/add.c b/builtin/add.c
index 226f758..2d0d2ef 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -15,6 +15,7 @@
 #include "diffcore.h"
 #include "revision.h"
 #include "bulk-checkin.h"
+#include "submodule.h"
 
 static const char * const builtin_add_usage[] = {
 	N_("git add [options] [--] <pathspec>..."),
@@ -378,6 +379,10 @@ static int add_config(const char *var, const char *value, void *cb)
 		ignore_add_errors = git_config_bool(var, value);
 		return 0;
 	}
+
+	if (!prefixcmp(var, "submodule."))
+		return parse_submodule_config_option(var, value);
+
 	return git_default_config(var, value, cb);
 }
 
@@ -415,6 +420,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int implicit_dot = 0;
 	struct update_callback_data update_data;
 
+	gitmodules_config();
 	git_config(add_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, builtin_add_options,
-- 
1.8.5.rc3.1.gbe2a8c7

Re: Git issues with submodules

[Jens Lehmann]

Am 24.11.2013 01:52, schrieb Heiko Voigt:
> Hi,
> 
> On Sat, Nov 23, 2013 at 09:10:44PM +0100, Jens Lehmann wrote:
>> Am 22.11.2013 23:09, schrieb Jonathan Nieder:
>>> Heiko Voigt wrote:
>>>
>>>> After that we can discuss whether add should add submodules that are
>>>> tracked but not shown. How about commit -a ? Should it also ignore the
>>>> change? I am undecided here. There does not seem to be any good
>>>> decision. From the users point of view we should probably not add it
>>>> since its not visible in status. What do others think?
>>>
>>> I agree --- it should not add.
>>
>> I concur: adding a change that is hidden from the user during
>> the process is not a good idea.
> 
> Here is a patch achieving that. Still missing a test which I will add.

Looking good to me. Please add tests for "diff.ignoreSubmodules"
and "submodule.<name>.ignore", the latter both in .gitmodules and
.git/config. While doing some testing for this thread I found an
inconsistency in git show which currently honors the submodule
specific option only from .git/config and ignores it in the
.gitmodules file (depending on the outcome of the discussion on
what '--ignore-submodules=all' should ignore we might have to fix
that one afterwards).

I'd suggest to also add the --ignore-submodules option in another
patch on top, because the user should be able to override the
configuration either way. And what about having the '-f' option
imply '--ignore-submodules=none'?

> Cheers Heiko
> 
> ---8<----
> Subject: [PATCH] fix 'git add' to skip submodules configured as ignored
> 
> If submodules are configured as ignore=all they are not shown by status.
> Lets also ignore them when adding files to the index. This avoids that
> users accidentially add ignored submodules with: git add .
> 
> We achieve this by reading the submodule config and thus correctly
> initializing the infrastructure to take the ignore decision.
> 
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---
>  builtin/add.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/builtin/add.c b/builtin/add.c
> index 226f758..2d0d2ef 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -15,6 +15,7 @@
>  #include "diffcore.h"
>  #include "revision.h"
>  #include "bulk-checkin.h"
> +#include "submodule.h"
>  
>  static const char * const builtin_add_usage[] = {
>  	N_("git add [options] [--] <pathspec>..."),
> @@ -378,6 +379,10 @@ static int add_config(const char *var, const char *value, void *cb)
>  		ignore_add_errors = git_config_bool(var, value);
>  		return 0;
>  	}
> +
> +	if (!prefixcmp(var, "submodule."))
> +		return parse_submodule_config_option(var, value);
> +
>  	return git_default_config(var, value, cb);
>  }
>  
> @@ -415,6 +420,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  	int implicit_dot = 0;
>  	struct update_callback_data update_data;
>  
> +	gitmodules_config();
>  	git_config(add_config, NULL);
>  
>  	argc = parse_options(argc, argv, prefix, builtin_add_options,
> 

Re: Git issues with submodules

[Sergey Sharybin]

Hey!

Sorry for the delayed reply.

Am i right the intention is to make it so `git add .` and `git commit
.` doesn't include changes to submodule hash unless -f argument is
provided?

On Sun, Nov 24, 2013 at 10:29 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> Am 24.11.2013 01:52, schrieb Heiko Voigt:
>> Hi,
>>
>> On Sat, Nov 23, 2013 at 09:10:44PM +0100, Jens Lehmann wrote:
>>> Am 22.11.2013 23:09, schrieb Jonathan Nieder:
>>>> Heiko Voigt wrote:
>>>>
>>>>> After that we can discuss whether add should add submodules that are
>>>>> tracked but not shown. How about commit -a ? Should it also ignore the
>>>>> change? I am undecided here. There does not seem to be any good
>>>>> decision. From the users point of view we should probably not add it
>>>>> since its not visible in status. What do others think?
>>>>
>>>> I agree --- it should not add.
>>>
>>> I concur: adding a change that is hidden from the user during
>>> the process is not a good idea.
>>
>> Here is a patch achieving that. Still missing a test which I will add.
>
> Looking good to me. Please add tests for "diff.ignoreSubmodules"
> and "submodule.<name>.ignore", the latter both in .gitmodules and
> .git/config. While doing some testing for this thread I found an
> inconsistency in git show which currently honors the submodule
> specific option only from .git/config and ignores it in the
> .gitmodules file (depending on the outcome of the discussion on
> what '--ignore-submodules=all' should ignore we might have to fix
> that one afterwards).
>
> I'd suggest to also add the --ignore-submodules option in another
> patch on top, because the user should be able to override the
> configuration either way. And what about having the '-f' option
> imply '--ignore-submodules=none'?
>
>> Cheers Heiko
>>
>> ---8<----
>> Subject: [PATCH] fix 'git add' to skip submodules configured as ignored
>>
>> If submodules are configured as ignore=all they are not shown by status.
>> Lets also ignore them when adding files to the index. This avoids that
>> users accidentially add ignored submodules with: git add .
>>
>> We achieve this by reading the submodule config and thus correctly
>> initializing the infrastructure to take the ignore decision.
>>
>> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
>> ---
>>  builtin/add.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/builtin/add.c b/builtin/add.c
>> index 226f758..2d0d2ef 100644
>> --- a/builtin/add.c
>> +++ b/builtin/add.c
>> @@ -15,6 +15,7 @@
>>  #include "diffcore.h"
>>  #include "revision.h"
>>  #include "bulk-checkin.h"
>> +#include "submodule.h"
>>
>>  static const char * const builtin_add_usage[] = {
>>       N_("git add [options] [--] <pathspec>..."),
>> @@ -378,6 +379,10 @@ static int add_config(const char *var, const char *value, void *cb)
>>               ignore_add_errors = git_config_bool(var, value);
>>               return 0;
>>       }
>> +
>> +     if (!prefixcmp(var, "submodule."))
>> +             return parse_submodule_config_option(var, value);
>> +
>>       return git_default_config(var, value, cb);
>>  }
>>
>> @@ -415,6 +420,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>>       int implicit_dot = 0;
>>       struct update_callback_data update_data;
>>
>> +     gitmodules_config();
>>       git_config(add_config, NULL);
>>
>>       argc = parse_options(argc, argv, prefix, builtin_add_options,
>>
>



-- 
With best regards, Sergey Sharybin

Re: Re: Git issues with submodules

[Heiko Voigt]

On Mon, Nov 25, 2013 at 03:02:51PM +0600, Sergey Sharybin wrote:
> Am i right the intention is to make it so `git add .` and `git commit
> .` doesn't include changes to submodule hash unless -f argument is
> provided?

Yes thats the goal. My patch currently only disables it when ignore is
set to all. I will add another patch that implements the -f and
--submodule-ignore option to both of them so the user has an easy way to
bypass that. But having said that we changing existing behavior here so
we have to investigate carefully whether we are not breaking peoples
expectations (and script). That also applies to the other patch
that enables showing them in diff and friends again.

Cheers Heiko

Re: Re: Git issues with submodules

[Sergey Sharybin]

Heiko, yeah sure see what you mean. Changing existing behavior is pretty PITA.

Just one more question for now, are you referencing to the patch "[RFC
PATCH] disable complete ignorance of submodules for index <-> HEAD
diff"? Coz i tested it and seems it doesn't change behavior of
add/commit.

Also, i'm around to test the all patches which are related on submodules :)

On Mon, Nov 25, 2013 at 11:49 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> On Mon, Nov 25, 2013 at 03:02:51PM +0600, Sergey Sharybin wrote:
>> Am i right the intention is to make it so `git add .` and `git commit
>> .` doesn't include changes to submodule hash unless -f argument is
>> provided?
>
> Yes thats the goal. My patch currently only disables it when ignore is
> set to all. I will add another patch that implements the -f and
> --submodule-ignore option to both of them so the user has an easy way to
> bypass that. But having said that we changing existing behavior here so
> we have to investigate carefully whether we are not breaking peoples
> expectations (and script). That also applies to the other patch
> that enables showing them in diff and friends again.
>
> Cheers Heiko



-- 
With best regards, Sergey Sharybin

Re: Re: Re: Git issues with submodules

[Heiko Voigt]

On Mon, Nov 25, 2013 at 11:57:45PM +0600, Sergey Sharybin wrote:
> Heiko, yeah sure see what you mean. Changing existing behavior is pretty PITA.
> 
> Just one more question for now, are you referencing to the patch "[RFC
> PATCH] disable complete ignorance of submodules for index <-> HEAD
> diff"? Coz i tested it and seems it doesn't change behavior of
> add/commit.

Yep, that was just an RFC for status and diff. I think teaching add and
commit to skip submodules if ignored are a separate topic and thus will
be in a separate patch. I have to add tests and probably some more
commands. The logic of ignoring submodules is implemented quite deep in
the diff code. So changing it can affect quite some commands so we
have to check quite carefully what will be affected and if we can change
it without to much fallout.

> Also, i'm around to test the all patches which are related on submodules :)

Thanks, good to know. Stay tuned!

Cheers Heiko

[RFC/WIP PATCH 0/4] less ignorance of submodules for ignore=all

[Heiko Voigt]

This is my current work in progress. Sergey it would be awesome if you
could test these and tell me whether the behaviour is what you would
expect. Once that is settled I will add some tests and possibly clean up
some code.

Since nobody spoke against this change of behavior I assume that we
agree on the general approach I am taking here. If not please speak up
now so we can work something out and save me implementation time ;-)

Whats still missing is:

 * it seems reset does not care at all about the ignore settings. It
   still shows a

   M	submodule

   line even when the submodule in question was not in the index and is
   marked as ignored. Have not looked at the code yet.

 * The git diff $commit question Junio mentioned here[1] it does not yet
   show diffs of ignore=all submodules.

For testing convenience you can also find all patches applied to Junio's
current master here:

https://github.com/hvoigt/git/commits/hv/fix_ignore_all_submodules

Cheers Heiko

Heiko Voigt (4):
  disable complete ignorance of submodules for index <-> HEAD diff
  fix 'git add' to skip submodules configured as ignored
  teach add -f option for ignored submodules
  always show committed submodules in summary after commit

 builtin/add.c             | 55 ++++++++++++++++++++++++++++++++++++-----------
 builtin/commit.c          |  1 +
 builtin/diff.c            |  2 ++
 diff-lib.c                |  3 +++
 diff.h                    |  2 +-
 submodule.c               | 26 ++++++++++++++++++++--
 submodule.h               |  2 ++
 t/t4027-diff-submodule.sh | 12 ++++++++---
 t/t7508-status.sh         |  6 +++++-
 9 files changed, 90 insertions(+), 19 deletions(-)

-- 
1.8.5.1.43.gf00fb86

[RFC/WIP PATCH 1/4] disable complete ignorance of submodules for index <-> HEAD diff

[Heiko Voigt]

If the value of ignore for submodules is set to "all" we would not show
whats actually committed during status or diff. This can result in the
user committing unexpected submodule references. Lets be nicer and always
show whats in the index.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 builtin/diff.c            |  2 ++
 diff-lib.c                |  3 +++
 diff.h                    |  2 +-
 submodule.c               | 16 ++++++++++++++--
 submodule.h               |  1 +
 t/t4027-diff-submodule.sh | 12 +++++++++---
 t/t7508-status.sh         |  6 +++++-
 7 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index adb93a9..c47614d 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -162,6 +162,8 @@ static int builtin_diff_tree(struct rev_info *revs,
 	if (argc > 1)
 		usage(builtin_diff_usage);
 
+	enforce_no_complete_ignore_submodule(&revs->diffopt);
+
 	/*
 	 * We saw two trees, ent0 and ent1.  If ent1 is uninteresting,
 	 * swap them.
diff --git a/diff-lib.c b/diff-lib.c
index 346cac6..c5219cb 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -483,6 +483,9 @@ int run_diff_index(struct rev_info *revs, int cached)
 {
 	struct object_array_entry *ent;
 
+	if (cached)
+		enforce_no_complete_ignore_submodule(&revs->diffopt);
+
 	ent = revs->pending.objects;
 	if (diff_cache(revs, ent->item->sha1, ent->name, cached))
 		exit(128);
diff --git a/diff.h b/diff.h
index e342325..81561b3 100644
--- a/diff.h
+++ b/diff.h
@@ -64,7 +64,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)
 #define DIFF_OPT_FOLLOW_RENAMES      (1 <<  7)
 #define DIFF_OPT_RENAME_EMPTY        (1 <<  8)
-/* (1 <<  9) unused */
+#define DIFF_OPT_NO_IGNORE_SUBMODULE (1 <<  9)
 #define DIFF_OPT_HAS_CHANGES         (1 << 10)
 #define DIFF_OPT_QUICK               (1 << 11)
 #define DIFF_OPT_NO_INDEX            (1 << 12)
diff --git a/submodule.c b/submodule.c
index 1905d75..e0719b6 100644
--- a/submodule.c
+++ b/submodule.c
@@ -294,6 +294,16 @@ int parse_submodule_config_option(const char *var, const char *value)
 	return 0;
 }
 
+void enforce_no_complete_ignore_submodule(struct diff_options *diffopt)
+{
+	DIFF_OPT_SET(diffopt, NO_IGNORE_SUBMODULE);
+	if (DIFF_OPT_TST(diffopt, OVERRIDE_SUBMODULE_CONFIG) &&
+	    DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES)) {
+		DIFF_OPT_CLR(diffopt, IGNORE_SUBMODULES);
+		DIFF_OPT_SET(diffopt, IGNORE_DIRTY_SUBMODULES);
+	}
+}
+
 void handle_ignore_submodules_arg(struct diff_options *diffopt,
 				  const char *arg)
 {
@@ -301,9 +311,11 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt,
 	DIFF_OPT_CLR(diffopt, IGNORE_UNTRACKED_IN_SUBMODULES);
 	DIFF_OPT_CLR(diffopt, IGNORE_DIRTY_SUBMODULES);
 
-	if (!strcmp(arg, "all"))
+	if (!strcmp(arg, "all")) {
+		if (DIFF_OPT_TST(diffopt, NO_IGNORE_SUBMODULE))
+			return;
 		DIFF_OPT_SET(diffopt, IGNORE_SUBMODULES);
-	else if (!strcmp(arg, "untracked"))
+	} else if (!strcmp(arg, "untracked"))
 		DIFF_OPT_SET(diffopt, IGNORE_UNTRACKED_IN_SUBMODULES);
 	else if (!strcmp(arg, "dirty"))
 		DIFF_OPT_SET(diffopt, IGNORE_DIRTY_SUBMODULES);
diff --git a/submodule.h b/submodule.h
index 7beec48..2c8087e 100644
--- a/submodule.h
+++ b/submodule.h
@@ -20,6 +20,7 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 int submodule_config(const char *var, const char *value, void *cb);
 void gitmodules_config(void);
 int parse_submodule_config_option(const char *var, const char *value);
+void enforce_no_complete_ignore_submodule(struct diff_options *diffopt);
 void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
 int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
 void show_submodule_summary(FILE *f, const char *path,
diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 518bf95..bd84ea7 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -258,7 +258,9 @@ test_expect_success 'git diff between submodule commits' '
 	expect_from_to >expect.body $subtip $subprev &&
 	test_cmp expect.body actual.body &&
 	git diff --ignore-submodules HEAD^..HEAD >actual &&
-	! test -s actual
+	sed -e "1,/^@@/d" actual >actual.body &&
+	expect_from_to >expect.body $subtip $subprev &&
+	test_cmp expect.body actual.body
 '
 
 test_expect_success 'git diff between submodule commits [.git/config]' '
@@ -274,7 +276,9 @@ test_expect_success 'git diff between submodule commits [.git/config]' '
 	test_cmp expect.body actual.body &&
 	git config submodule.subname.ignore all &&
 	git diff HEAD^..HEAD >actual &&
-	! test -s actual &&
+	sed -e "1,/^@@/d" actual >actual.body &&
+	expect_from_to >expect.body $subtip $subprev &&
+	test_cmp expect.body actual.body &&
 	git diff --ignore-submodules=dirty HEAD^..HEAD >actual &&
 	sed -e "1,/^@@/d" actual >actual.body &&
 	expect_from_to >expect.body $subtip $subprev &&
@@ -294,7 +298,9 @@ test_expect_success 'git diff between submodule commits [.gitmodules]' '
 	test_cmp expect.body actual.body &&
 	git config -f .gitmodules submodule.subname.ignore all &&
 	git diff HEAD^..HEAD >actual &&
-	! test -s actual &&
+	sed -e "1,/^@@/d" actual >actual.body &&
+	expect_from_to >expect.body $subtip $subprev &&
+	test_cmp expect.body actual.body &&
 	git config submodule.subname.ignore dirty &&
 	git config submodule.subname.path sub &&
 	git diff  HEAD^..HEAD >actual &&
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index c987b5e..977295f 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1357,6 +1357,11 @@ test_expect_success "status (core.commentchar with two chars with submodule summ
 test_expect_success "--ignore-submodules=all suppresses submodule summary" '
 	cat > expect << EOF &&
 On branch master
+Changes to be committed:
+  (use "git reset HEAD <file>..." to unstage)
+
+	modified:   sm
+
 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)
@@ -1374,7 +1379,6 @@ Untracked files:
 	output
 	untracked
 
-no changes added to commit (use "git add" and/or "git commit -a")
 EOF
 	git status --ignore-submodules=all > output &&
 	test_i18ncmp expect output
-- 
1.8.5.1.43.gf00fb86

[RFC/WIP PATCH 2/4] fix 'git add' to skip submodules configured as ignored

[Heiko Voigt]

If submodules are configured as ignore=all they are not shown by status.
Lets also ignore them when adding files to the index. This avoids that
users accidentially add ignored submodules with: git add .

We achieve this by reading the submodule config and thus correctly
initializing the infrastructure to take the ignore decision.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 builtin/add.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/builtin/add.c b/builtin/add.c
index 226f758..2d0d2ef 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -15,6 +15,7 @@
 #include "diffcore.h"
 #include "revision.h"
 #include "bulk-checkin.h"
+#include "submodule.h"
 
 static const char * const builtin_add_usage[] = {
 	N_("git add [options] [--] <pathspec>..."),
@@ -378,6 +379,10 @@ static int add_config(const char *var, const char *value, void *cb)
 		ignore_add_errors = git_config_bool(var, value);
 		return 0;
 	}
+
+	if (!prefixcmp(var, "submodule."))
+		return parse_submodule_config_option(var, value);
+
 	return git_default_config(var, value, cb);
 }
 
@@ -415,6 +420,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int implicit_dot = 0;
 	struct update_callback_data update_data;
 
+	gitmodules_config();
 	git_config(add_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, builtin_add_options,
-- 
1.8.5.1.43.gf00fb86

[RFC/WIP PATCH 3/4] teach add -f option for ignored submodules

[Heiko Voigt]

When the user wants to bypass the ignored status configured by
submodule.<name>.ignore=all it is now allowed by using the -f option.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 builtin/add.c | 49 +++++++++++++++++++++++++++++++++++++------------
 submodule.c   | 10 ++++++++++
 submodule.h   |  1 +
 3 files changed, 48 insertions(+), 12 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 2d0d2ef..d6cab7f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -16,6 +16,7 @@
 #include "revision.h"
 #include "bulk-checkin.h"
 #include "submodule.h"
+#include "string-list.h"
 
 static const char * const builtin_add_usage[] = {
 	N_("git add [options] [--] <pathspec>..."),
@@ -37,6 +38,20 @@ struct update_callback_data {
 static const char *option_with_implicit_dot;
 static const char *short_option_with_implicit_dot;
 
+static struct lock_file lock_file;
+
+static const char ignore_error[] =
+N_("The following paths are ignored by one of your .gitignore files:\n");
+static const char submodule_ignore_error[] =
+N_("The following paths are ignored submodules:\n");
+
+static int verbose, show_only, ignored_too, refresh_only;
+static int ignore_add_errors, intent_to_add, ignore_missing;
+
+#define ADDREMOVE_DEFAULT 0 /* Change to 1 in Git 2.0 */
+static int addremove = ADDREMOVE_DEFAULT;
+static int addremove_explicit = -1; /* unspecified */
+
 static void warn_pathless_add(void)
 {
 	static int shown;
@@ -140,6 +155,9 @@ static void update_callback(struct diff_queue_struct *q,
 			warn_pathless_add();
 			continue;
 		}
+		if (is_ignored_submodule(path) && !ignored_too)
+			continue;
+
 		switch (fix_unmerged_status(p, data)) {
 		default:
 			die(_("unexpected diff status %c"), p->status);
@@ -174,6 +192,7 @@ static void update_files_in_cache(const char *prefix,
 	struct rev_info rev;
 
 	init_revisions(&rev, prefix);
+	enforce_no_complete_ignore_submodule(&rev.diffopt);
 	setup_revisions(0, NULL, &rev, NULL);
 	if (pathspec)
 		copy_pathspec(&rev.prune_data, pathspec);
@@ -332,18 +351,6 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-static struct lock_file lock_file;
-
-static const char ignore_error[] =
-N_("The following paths are ignored by one of your .gitignore files:\n");
-
-static int verbose, show_only, ignored_too, refresh_only;
-static int ignore_add_errors, intent_to_add, ignore_missing;
-
-#define ADDREMOVE_DEFAULT 0 /* Change to 1 in Git 2.0 */
-static int addremove = ADDREMOVE_DEFAULT;
-static int addremove_explicit = -1; /* unspecified */
-
 static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)
 {
 	/* if we are told to ignore, we are not adding removals */
@@ -407,6 +414,17 @@ static int add_files(struct dir_struct *dir, int flags)
 	return exit_status;
 }
 
+static void die_ignored_submodules(struct string_list *ignored_submodules)
+{
+	struct string_list_item *path;
+
+	fprintf(stderr, _(submodule_ignore_error));
+	for_each_string_list_item(path, ignored_submodules)
+		fprintf(stderr, "%s\n", path->string);
+	fprintf(stderr, _("Use -f if you really want to add them.\n"));
+	die(_("no files added"));
+}
+
 int cmd_add(int argc, const char **argv, const char *prefix)
 {
 	int exit_status = 0;
@@ -419,6 +437,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	char *seen = NULL;
 	int implicit_dot = 0;
 	struct update_callback_data update_data;
+	struct string_list ignored_submodules = STRING_LIST_INIT_NODUP;
 
 	gitmodules_config();
 	git_config(add_config, NULL);
@@ -550,6 +569,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 		for (i = 0; i < pathspec.nr; i++) {
 			const char *path = pathspec.items[i].match;
+			char path_copy[PATH_MAX];
 			if (!seen[i] &&
 			    ((pathspec.items[i].magic &
 			      (PATHSPEC_GLOB | PATHSPEC_ICASE)) ||
@@ -562,6 +582,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 					die(_("pathspec '%s' did not match any files"),
 					    pathspec.items[i].original);
 			}
+			normalize_path_copy(path_copy, path);
+			if (is_ignored_submodule(path_copy))
+				string_list_insert(&ignored_submodules, path);
 		}
 		free(seen);
 	}
@@ -583,6 +606,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	update_files_in_cache(prefix, &pathspec, &update_data);
 
 	exit_status |= !!update_data.add_errors;
+	if (!ignored_too && ignored_submodules.nr)
+		die_ignored_submodules(&ignored_submodules);
 	if (add_new_files)
 		exit_status |= add_files(&dir, flags);
 
diff --git a/submodule.c b/submodule.c
index e0719b6..c28a926 100644
--- a/submodule.c
+++ b/submodule.c
@@ -199,6 +199,16 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 	}
 }
 
+int is_ignored_submodule(const char *path)
+{
+	struct diff_options diffopt;
+	memset(&diffopt, 0, sizeof(diffopt));
+	set_diffopt_flags_from_submodule_config(&diffopt, path);
+	if (DIFF_OPT_TST(&diffopt, IGNORE_SUBMODULES))
+		return 1;
+	return 0;
+}
+
 int submodule_config(const char *var, const char *value, void *cb)
 {
 	if (!prefixcmp(var, "submodule."))
diff --git a/submodule.h b/submodule.h
index 2c8087e..e067580 100644
--- a/submodule.h
+++ b/submodule.h
@@ -17,6 +17,7 @@ int remove_path_from_gitmodules(const char *path);
 void stage_updated_gitmodules(void);
 void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 		const char *path);
+int is_ignored_submodule(const char *path);
 int submodule_config(const char *var, const char *value, void *cb);
 void gitmodules_config(void);
 int parse_submodule_config_option(const char *var, const char *value);
-- 
1.8.5.1.43.gf00fb86

Re: [RFC/WIP PATCH 3/4] teach add -f option for ignored submodules

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> When the user wants to bypass the ignored status configured by
> submodule.<name>.ignore=all it is now allowed by using the -f option.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---
>  builtin/add.c | 49 +++++++++++++++++++++++++++++++++++++------------
>  submodule.c   | 10 ++++++++++
>  submodule.h   |  1 +
>  3 files changed, 48 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 2d0d2ef..d6cab7f 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -16,6 +16,7 @@
>  #include "revision.h"
>  #include "bulk-checkin.h"
>  #include "submodule.h"
> +#include "string-list.h"
>  
>  static const char * const builtin_add_usage[] = {
>  	N_("git add [options] [--] <pathspec>..."),
> @@ -37,6 +38,20 @@ struct update_callback_data {
>  static const char *option_with_implicit_dot;
>  static const char *short_option_with_implicit_dot;
>  
> +static struct lock_file lock_file;
> +
> +static const char ignore_error[] =
> +N_("The following paths are ignored by one of your .gitignore files:\n");
> +static const char submodule_ignore_error[] =
> +N_("The following paths are ignored submodules:\n");
> +
> +static int verbose, show_only, ignored_too, refresh_only;
> +static int ignore_add_errors, intent_to_add, ignore_missing;
> +
> +#define ADDREMOVE_DEFAULT 0 /* Change to 1 in Git 2.0 */
> +static int addremove = ADDREMOVE_DEFAULT;
> +static int addremove_explicit = -1; /* unspecified */
> +
>  static void warn_pathless_add(void)
>  {
>  	static int shown;
> @@ -140,6 +155,9 @@ static void update_callback(struct diff_queue_struct *q,
>  			warn_pathless_add();
>  			continue;
>  		}
> +		if (is_ignored_submodule(path) && !ignored_too)
> +			continue;
> +
>  		switch (fix_unmerged_status(p, data)) {
>  		default:
>  			die(_("unexpected diff status %c"), p->status);
> @@ -174,6 +192,7 @@ static void update_files_in_cache(const char *prefix,
>  	struct rev_info rev;
>  
>  	init_revisions(&rev, prefix);
> +	enforce_no_complete_ignore_submodule(&rev.diffopt);
>  	setup_revisions(0, NULL, &rev, NULL);
>  	if (pathspec)
>  		copy_pathspec(&rev.prune_data, pathspec);
> @@ -332,18 +351,6 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> -static struct lock_file lock_file;
> -
> -static const char ignore_error[] =
> -N_("The following paths are ignored by one of your .gitignore files:\n");
> -
> -static int verbose, show_only, ignored_too, refresh_only;
> -static int ignore_add_errors, intent_to_add, ignore_missing;
> -
> -#define ADDREMOVE_DEFAULT 0 /* Change to 1 in Git 2.0 */
> -static int addremove = ADDREMOVE_DEFAULT;
> -static int addremove_explicit = -1; /* unspecified */
> -
>  static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)
>  {
>  	/* if we are told to ignore, we are not adding removals */
> @@ -407,6 +414,17 @@ static int add_files(struct dir_struct *dir, int flags)
>  	return exit_status;
>  }
>  
> +static void die_ignored_submodules(struct string_list *ignored_submodules)
> +{
> +	struct string_list_item *path;
> +
> +	fprintf(stderr, _(submodule_ignore_error));
> +	for_each_string_list_item(path, ignored_submodules)
> +		fprintf(stderr, "%s\n", path->string);
> +	fprintf(stderr, _("Use -f if you really want to add them.\n"));
> +	die(_("no files added"));
> +}
> +
>  int cmd_add(int argc, const char **argv, const char *prefix)
>  {
>  	int exit_status = 0;
> @@ -419,6 +437,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  	char *seen = NULL;
>  	int implicit_dot = 0;
>  	struct update_callback_data update_data;
> +	struct string_list ignored_submodules = STRING_LIST_INIT_NODUP;
>  
>  	gitmodules_config();
>  	git_config(add_config, NULL);
> @@ -550,6 +569,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  
>  		for (i = 0; i < pathspec.nr; i++) {
>  			const char *path = pathspec.items[i].match;
> +			char path_copy[PATH_MAX];
>  			if (!seen[i] &&
>  			    ((pathspec.items[i].magic &
>  			      (PATHSPEC_GLOB | PATHSPEC_ICASE)) ||
> @@ -562,6 +582,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  					die(_("pathspec '%s' did not match any files"),
>  					    pathspec.items[i].original);
>  			}
> +			normalize_path_copy(path_copy, path);
> +			if (is_ignored_submodule(path_copy))
> +				string_list_insert(&ignored_submodules, path);
>  		}
>  		free(seen);
>  	}
> @@ -583,6 +606,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  	update_files_in_cache(prefix, &pathspec, &update_data);
>  
>  	exit_status |= !!update_data.add_errors;
> +	if (!ignored_too && ignored_submodules.nr)
> +		die_ignored_submodules(&ignored_submodules);

Why is this done so late in the process?  Shouldn't it be done
immediately after we have finished iterating over the pathspecs,
checking with is_ignored_submodule() and stuffing them into
ignored_submodules string list, not waiting for plugging bulk
checkin or updating paths already tracked in the index?

>  	if (add_new_files)
>  		exit_status |= add_files(&dir, flags);
>  
> diff --git a/submodule.c b/submodule.c
> index e0719b6..c28a926 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -199,6 +199,16 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
>  	}
>  }
>  
> +int is_ignored_submodule(const char *path)
> +{
> +	struct diff_options diffopt;
> +	memset(&diffopt, 0, sizeof(diffopt));
> +	set_diffopt_flags_from_submodule_config(&diffopt, path);
> +	if (DIFF_OPT_TST(&diffopt, IGNORE_SUBMODULES))
> +		return 1;
> +	return 0;
> +}
> +
>  int submodule_config(const char *var, const char *value, void *cb)
>  {
>  	if (!prefixcmp(var, "submodule."))
> diff --git a/submodule.h b/submodule.h
> index 2c8087e..e067580 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -17,6 +17,7 @@ int remove_path_from_gitmodules(const char *path);
>  void stage_updated_gitmodules(void);
>  void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
>  		const char *path);
> +int is_ignored_submodule(const char *path);
>  int submodule_config(const char *var, const char *value, void *cb);
>  void gitmodules_config(void);
>  int parse_submodule_config_option(const char *var, const char *value);

Re: Re: [RFC/WIP PATCH 3/4] teach add -f option for ignored submodules

[Heiko Voigt]

On Fri, Dec 06, 2013 at 03:10:31PM -0800, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> > diff --git a/builtin/add.c b/builtin/add.c
> > index 2d0d2ef..d6cab7f 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -550,6 +569,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> >  
> >  		for (i = 0; i < pathspec.nr; i++) {
> >  			const char *path = pathspec.items[i].match;
> > +			char path_copy[PATH_MAX];
> >  			if (!seen[i] &&
> >  			    ((pathspec.items[i].magic &
> >  			      (PATHSPEC_GLOB | PATHSPEC_ICASE)) ||
> > @@ -562,6 +582,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> >  					die(_("pathspec '%s' did not match any files"),
> >  					    pathspec.items[i].original);
> >  			}
> > +			normalize_path_copy(path_copy, path);
> > +			if (is_ignored_submodule(path_copy))
> > +				string_list_insert(&ignored_submodules, path);
> >  		}
> >  		free(seen);
> >  	}
> > @@ -583,6 +606,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> >  	update_files_in_cache(prefix, &pathspec, &update_data);
> >  
> >  	exit_status |= !!update_data.add_errors;
> > +	if (!ignored_too && ignored_submodules.nr)
> > +		die_ignored_submodules(&ignored_submodules);
> 
> Why is this done so late in the process?  Shouldn't it be done
> immediately after we have finished iterating over the pathspecs,
> checking with is_ignored_submodule() and stuffing them into
> ignored_submodules string list, not waiting for plugging bulk
> checkin or updating paths already tracked in the index?

There was no specific reason. I just imitated the codepath for new
files (which will die in add_files() if they are ignored). This can be
moved further up. Will do so.

Cheers Heiko

[RFC/WIP PATCH 4/4] always show committed submodules in summary after commit

[Heiko Voigt]

If an ignored submodule is committed because is was registered in the
index we should always show that to the user in the printed summary
after commit.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 builtin/commit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index 6ab4605..e551566 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1361,6 +1361,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
 	strbuf_release(&committer_ident);
 
 	init_revisions(&rev, prefix);
+	enforce_no_complete_ignore_submodule(&rev.diffopt);
 	setup_revisions(0, NULL, &rev, NULL);
 
 	rev.diff = 1;
-- 
1.8.5.1.43.gf00fb86

Re: [RFC/WIP PATCH 0/4] less ignorance of submodules for ignore=all

[Heiko Voigt]

On Wed, Dec 04, 2013 at 11:16:59PM +0100, Heiko Voigt wrote:
>  * The git diff $commit question Junio mentioned here[1] it does not yet
>    show diffs of ignore=all submodules.

Forgot to add this here:

[1] http://article.gmane.org/gmane.comp.version-control.git/238348

Re: [RFC/WIP PATCH 0/4] less ignorance of submodules for ignore=all

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> This is my current work in progress. Sergey it would be awesome if you
> could test these and tell me whether the behaviour is what you would
> expect. Once that is settled I will add some tests and possibly clean up
> some code.
>
> Since nobody spoke against this change of behavior I assume that we
> agree on the general approach I am taking here. If not please speak up
> now so we can work something out and save me implementation time ;-)
>
> Whats still missing is:

Before listing what's missing, can you describe what "the general
approach" is?  After all, that is what you are assuming that has got
a silent concensus, but without getting it spelled out, others would
easily miss what they "agreed" to.

I do think that it is a good thing to make what "git add ." does and
what "git status ." reports consistent, and "git add ." that does
not add everything may be a good step in that direction (another
possible solution may be to admit that ignore=all was a mistake and
remove that special case altogether, so that "git status" will
always report a submodule that does not match what is in the HEAD
and/or index).

Re: Re: [RFC/WIP PATCH 0/4] less ignorance of submodules for ignore=all

[Heiko Voigt]

On Wed, Dec 04, 2013 at 02:32:46PM -0800, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> > This is my current work in progress. Sergey it would be awesome if you
> > could test these and tell me whether the behaviour is what you would
> > expect. Once that is settled I will add some tests and possibly clean up
> > some code.
> >
> > Since nobody spoke against this change of behavior I assume that we
> > agree on the general approach I am taking here. If not please speak up
> > now so we can work something out and save me implementation time ;-)
> >
> > Whats still missing is:
> 
> Before listing what's missing, can you describe what "the general
> approach" is?  After all, that is what you are assuming that has got
> a silent concensus, but without getting it spelled out, others would
> easily miss what they "agreed" to.

Definitely, sorry I missed that (isn't it obvious ;-)):

This series tries to achieve the following goals for the
submodule.<name>.ignore=all configuration or the --ignore-submodules=all
command line switch.

 * Make git status never ignore submodule changes that got somehow in the
   index. Currently when ignore=all is specified they are and thus
   secretly committed. Basically always show exactly what will be
   committed.

 * Make add ignore submodules that have the ignore=all configuration when
   not explicitly naming a certain submodule (i.e. using git add .).
   That way ignore=all submodules are not added to the index by default.
   That can be overridden by using the -f switch so it behaves the same
   as with untracked files specified in one of the ignore files except
   that submodules are actually tracked.

 * Let diff always show submodule changes between revisions or
   between a revision and the index. Only worktree changes should be
   ignored with ignore=all.

 * Generally speaking: Make everything that displays diffs in history,
   diffs between revisions or between a revision and the index always
   show submodules changes (only the commit ids) even if a submodule is
   specified as ignore=all.

 * If ignore=all for a submodule and a diff would usually involve the
   worktree we will show the diff of the commit ids between the current
   index and the requested revision.

> I do think that it is a good thing to make what "git add ." does and
> what "git status ." reports consistent, and "git add ." that does
> not add everything may be a good step in that direction (another
> possible solution may be to admit that ignore=all was a mistake and
> remove that special case altogether, so that "git status" will
> always report a submodule that does not match what is in the HEAD
> and/or index).

I think it was too early to add ignore=all back then when the ignoring
was implemented. We did not think through all implications. Since people
have always been requesting the floating model and as it seems started
using it I am not so sure whether there is not a valid use case. Maybe
Sergey can shed some light on their actual use case and why they do not
care about the precise revision most of the time.

For example the case that all developers always want to work with some
HEAD revision of all submodules and the build system then integrates
their changes on a regular basis. When all went well it creates commits
with the precise revisions. This way they have some stable points as
fallback or for releases. Thats at least the use case I can think of but
maybe there are others.

Cheers Heiko

Re: [RFC/WIP PATCH 0/4] less ignorance of submodules for ignore=all

[Jens Lehmann]

Am 05.12.2013 00:19, schrieb Heiko Voigt:
> On Wed, Dec 04, 2013 at 02:32:46PM -0800, Junio C Hamano wrote:
> This series tries to achieve the following goals for the
> submodule.<name>.ignore=all configuration or the --ignore-submodules=all
> command line switch.

Thanks for the summary.

>  * Make git status never ignore submodule changes that got somehow in the
>    index. Currently when ignore=all is specified they are and thus
>    secretly committed. Basically always show exactly what will be
>    committed.

Yes, what's in the index should always be shown as such even when the
user chose to ignore the work tree differences of the submodule.

>  * Make add ignore submodules that have the ignore=all configuration when
>    not explicitly naming a certain submodule (i.e. using git add .).
>    That way ignore=all submodules are not added to the index by default.
>    That can be overridden by using the -f switch so it behaves the same
>    as with untracked files specified in one of the ignore files except
>    that submodules are actually tracked.

I think we should do this part in a different series, as everybody
seems to agree that this should be fixed that way and it has nothing
to do with what is ignored in submodule history.

>  * Let diff always show submodule changes between revisions or
>    between a revision and the index. Only worktree changes should be
>    ignored with ignore=all.
> 
>  * Generally speaking: Make everything that displays diffs in history,
>    diffs between revisions or between a revision and the index always
>    show submodules changes (only the commit ids) even if a submodule is
>    specified as ignore=all.

I'm not so sure about that. Some scripts really want to ignore the
history of submodules when comparing a rev to the index:

git-filter-branch.sh:			git diff-index -r --name-only --ignore-submodules $commit &&
git-pull.sh:    git diff-index --cached --name-status -r --ignore-submodules HEAD --
git-rebase--merge.sh:	if ! git diff-index --quiet --ignore-submodules HEAD --
git-sh-setup.sh:	if ! git diff-index --cached --quiet --ignore-submodules HEAD --
git-stash.sh:	git diff-index --quiet --cached HEAD --ignore-submodules -- &&

I didn't check each site in detail, but I suspect each ignore option
was added on purpose to fix a problem. That means we still need "all"
(at least when diffing rev<->index). Unfortunately that area is not
covered well in our tests, I only got breakage from the filter-branch
tests when teaching "all" to only ignore work tree changes (see at the
end on how I did that).

So I'm currently in favor of adding a new "worktree"-value which will
only ignore the work tree changes of submodules, which seems just what
the floating submodule use case needs. But it looks like we need to
keep "all".

>  * If ignore=all for a submodule and a diff would usually involve the
>    worktree we will show the diff of the commit ids between the current
>    index and the requested revision.

I agree if we make that "ignore=worktree".

>> I do think that it is a good thing to make what "git add ." does and
>> what "git status ." reports consistent, and "git add ." that does
>> not add everything may be a good step in that direction

Yup, as written above I'd propose to start with that too.

>> (another
>> possible solution may be to admit that ignore=all was a mistake and
>> remove that special case altogether, so that "git status" will
>> always report a submodule that does not match what is in the HEAD
>> and/or index).

No, looking at the git-scripts that use it together with diff-index it
wasn't a mistake. But we might be missing a less drastic option ;-)

> I think it was too early to add ignore=all back then when the ignoring
> was implemented. We did not think through all implications. Since people
> have always been requesting the floating model and as it seems started
> using it I am not so sure whether there is not a valid use case. Maybe
> Sergey can shed some light on their actual use case and why they do not
> care about the precise revision most of the time.

You maybe right about not thinking things thoroughly through, but we
helped people that rightfully complained when the (then new) submodule
awareness broke their scripts.

> For example the case that all developers always want to work with some
> HEAD revision of all submodules and the build system then integrates
> their changes on a regular basis. When all went well it creates commits
> with the precise revisions. This way they have some stable points as
> fallback or for releases. Thats at least the use case I can think of but
> maybe there are others.

And that could be the "worktree" value.

Below is a hack that disables the diffing of rev and index, but not
that against the work tree. It breaks t4027-diff-submodule.sh,
t7003-filter-branch.sh and t7508-status.sh as expected:

---------------------->8--------------------
diff --git a/diff.c b/diff.c
index e34bf97..ed66a01 100644
--- a/diff.c
+++ b/diff.c
@@ -4813,7 +4813,7 @@ static int is_submodule_ignored(const char *path, struct d
        if (DIFF_OPT_TST(options, IGNORE_SUBMODULES))
                ignored = 1;
        options->flags = orig_flags;
-       return ignored;
+       return 0;
 }

 void diff_addremove(struct diff_options *options,

Re: Re: [RFC/WIP PATCH 0/4] less ignorance of submodules for ignore=all

[Heiko Voigt]

On Thu, Dec 05, 2013 at 09:51:31PM +0100, Jens Lehmann wrote:
> Am 05.12.2013 00:19, schrieb Heiko Voigt:
> > On Wed, Dec 04, 2013 at 02:32:46PM -0800, Junio C Hamano wrote:
> > This series tries to achieve the following goals for the
> > submodule.<name>.ignore=all configuration or the --ignore-submodules=all
> > command line switch.
> 
> Thanks for the summary.
> 
> >  * Make git status never ignore submodule changes that got somehow in the
> >    index. Currently when ignore=all is specified they are and thus
> >    secretly committed. Basically always show exactly what will be
> >    committed.
> 
> Yes, what's in the index should always be shown as such even when the
> user chose to ignore the work tree differences of the submodule.
> 
> >  * Make add ignore submodules that have the ignore=all configuration when
> >    not explicitly naming a certain submodule (i.e. using git add .).
> >    That way ignore=all submodules are not added to the index by default.
> >    That can be overridden by using the -f switch so it behaves the same
> >    as with untracked files specified in one of the ignore files except
> >    that submodules are actually tracked.
> 
> I think we should do this part in a different series, as everybody
> seems to agree that this should be fixed that way and it has nothing
> to do with what is ignored in submodule history.

So how about I put the two points above into a separate series? IMO, add
and status belong together in this case.

> >  * Let diff always show submodule changes between revisions or
> >    between a revision and the index. Only worktree changes should be
> >    ignored with ignore=all.
> > 
> >  * Generally speaking: Make everything that displays diffs in history,
> >    diffs between revisions or between a revision and the index always
> >    show submodules changes (only the commit ids) even if a submodule is
> >    specified as ignore=all.
> 
> I'm not so sure about that. Some scripts really want to ignore the
> history of submodules when comparing a rev to the index:
> 
> git-filter-branch.sh:			git diff-index -r --name-only --ignore-submodules $commit &&
> git-pull.sh:    git diff-index --cached --name-status -r --ignore-submodules HEAD --
> git-rebase--merge.sh:	if ! git diff-index --quiet --ignore-submodules HEAD --
> git-sh-setup.sh:	if ! git diff-index --cached --quiet --ignore-submodules HEAD --
> git-stash.sh:	git diff-index --quiet --cached HEAD --ignore-submodules -- &&
> 
> I didn't check each site in detail, but I suspect each ignore option
> was added on purpose to fix a problem. That means we still need "all"
> (at least when diffing rev<->index). Unfortunately that area is not
> covered well in our tests, I only got breakage from the filter-branch
> tests when teaching "all" to only ignore work tree changes (see at the
> end on how I did that).

Well all hits are on diff-index which is plumbing. If it is required for
some (internal) scripts to completely ignore submodules I think it is ok
to do so just for plumbing with a commandline option like that. But I am
not sure whether this should actually be configurable.

> So I'm currently in favor of adding a new "worktree"-value which will
> only ignore the work tree changes of submodules, which seems just what
> the floating submodule use case needs. But it looks like we need to
> keep "all".

But that will just add more complexity to the already complex topic of
submodules. There are already enough possibilities to get confused with
submodules. I would like to avoid making it more complex.

>From the feedback we get now from Sergey I take that not many users have
actually been using the 'all' option. Otherwise there would have been
more complaints. So the only thing we have to worry about are scripts
and those we could cover with plumbing commands.

> >  * If ignore=all for a submodule and a diff would usually involve the
> >    worktree we will show the diff of the commit ids between the current
> >    index and the requested revision.
> 
> I agree if we make that "ignore=worktree".
> 
> >> I do think that it is a good thing to make what "git add ." does and
> >> what "git status ." reports consistent, and "git add ." that does
> >> not add everything may be a good step in that direction
> 
> Yup, as written above I'd propose to start with that too.
> 
> >> (another
> >> possible solution may be to admit that ignore=all was a mistake and
> >> remove that special case altogether, so that "git status" will
> >> always report a submodule that does not match what is in the HEAD
> >> and/or index).
> 
> No, looking at the git-scripts that use it together with diff-index it
> wasn't a mistake. But we might be missing a less drastic option ;-)

Well, as said above diff-index is plumbing and as such allowed to do
non-user friendly stuff. For all the other commands I propose to never
hide stuff from the user. E.g. take update-index there you can actually
mark any index entry as "assume unchanged" which will make git stop
checking it for changes (like the submodule.<name>.ignore=all setting for
submodules). IMO that is a potentially problematic setting which no
normal user should do without really knowing the implications. Still I
think for plumbing that is ok since it is not really meant to be
directly used. For porcelain it is a different story and I think we
should really support/guard the users here to not hang themselves.

> > I think it was too early to add ignore=all back then when the ignoring
> > was implemented. We did not think through all implications. Since people
> > have always been requesting the floating model and as it seems started
> > using it I am not so sure whether there is not a valid use case. Maybe
> > Sergey can shed some light on their actual use case and why they do not
> > care about the precise revision most of the time.
> 
> You maybe right about not thinking things thoroughly through, but we
> helped people that rightfully complained when the (then new) submodule
> awareness broke their scripts.

Here you are confusing 'all' with 'dirty'. Before the submodule
awareness starting with 1.7.0 the behavior we now get with 'dirty' was
the default. So that would be the option that helped them.

While talking about I think we should really make 'dirty' the default
instead of 'all'. AFAIR, the goal of ignoring submodules was to take the
runtime penalty that came from recursively scanning submodules for
changes. That is already fulfilled with 'dirty'.

> Below is a hack that disables the diffing of rev and index, but not
> that against the work tree. It breaks t4027-diff-submodule.sh,
> t7003-filter-branch.sh and t7508-status.sh as expected:

I am not sure what this is for. My patch series already implements that
(and includes the needed test changes), The testsuite passes for me.
With my implementation we have fine grained control over where it is ok
to completely ignore submodule diffs and where not.

Cheers Heiko

Re: [RFC/WIP PATCH 0/4] less ignorance of submodules for ignore=all

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> I didn't check each site in detail, but I suspect each ignore option
> was added on purpose to fix a problem. That means we still need "all"
> (at least when diffing rev<->index). Unfortunately that area is not
> covered well in our tests, I only got breakage from the filter-branch
> tests when teaching "all" to only ignore work tree changes (see at the
> end on how I did that).
>
> So I'm currently in favor of adding a new "worktree"-value which will
> only ignore the work tree changes of submodules, which seems just what
> the floating submodule use case needs.

Could you help me clarify what it exactly mean to only ignore the
work tree changes?  Specifically, if I have a submodule directory
whose (1) HEAD points at a commit that is the same as the commit
that is recorded by the top-level's gitlink, (2) the index may or
may not match HEAD, and (3) the working tree contents may or may not
match the index or the HEAD, does it only have the work tree
changes?

If the HEAD in the submodule directory is different from the commit
recorded by the top-level's gitlink, but the index and the working
tree match that different HEAD, I am guessing that it no longer is
with "only the work tree changes" and shown as modified.

If that is the suggestion, it goes back to the very original Git
submodule behavour where we compare $submoduledir/.git/HEAD with the
commit object name expected by the top-level and say the submodule
does not have any change when and only when these two object names
are the same, which sounds like a very sensible default behaviour
(besides, it is very cheap to check ;-).

Re: Git issues with submodules

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Looking good to me. Please add tests for "diff.ignoreSubmodules"
> and "submodule.<name>.ignore", the latter both in .gitmodules and
> .git/config. While doing some testing for this thread I found an
> inconsistency in git show which currently honors the submodule
> specific option only from .git/config and ignores it in the
> .gitmodules file ...

Sorry, but isn't that what should happen?  .git/config is the
ultimate source of the truth, and .gitmodules is a hint to prime
that when the user does "git submodule init", no?

> I'd suggest to also add the --ignore-submodules option in another
> patch on top, because the user should be able to override the
> configuration either way. And what about having the '-f' option
> imply '--ignore-submodules=none'?

Yeah, this sudden change of semantics, which I think is going in the
right direction in the longer run, does look like it may be robbing
from those from the "want specific revision, but not want to see the
cruft in the top-level" camp to pay those in the "floating" school.
At least, with "add -f", it allows people to add such ignored ones,
just like you can "git add -f cruft" when cruft is not tracked and
marked as ignored in the .gitignore mechansim.

Re: Git issues with submodules

[Jens Lehmann]

Am 25.11.2013 22:01, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> Looking good to me. Please add tests for "diff.ignoreSubmodules"
>> and "submodule.<name>.ignore", the latter both in .gitmodules and
>> .git/config. While doing some testing for this thread I found an
>> inconsistency in git show which currently honors the submodule
>> specific option only from .git/config and ignores it in the
>> .gitmodules file ...
> 
> Sorry, but isn't that what should happen?  .git/config is the
> ultimate source of the truth, and .gitmodules is a hint to prime
> that when the user does "git submodule init", no?

"git submodule init" only copies the "update" and "url" settings
to .git/config, all others default to the value they have in the
.gitmodules file if they aren't found in .git/config. This allows
upstream to change these settings unless the user copies them to
.git/config himself.

Re: Git issues with submodules

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Am 25.11.2013 22:01, schrieb Junio C Hamano:
>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>> 
>>> Looking good to me. Please add tests for "diff.ignoreSubmodules"
>>> and "submodule.<name>.ignore", the latter both in .gitmodules and
>>> .git/config. While doing some testing for this thread I found an
>>> inconsistency in git show which currently honors the submodule
>>> specific option only from .git/config and ignores it in the
>>> .gitmodules file ...
>> 
>> Sorry, but isn't that what should happen?  .git/config is the
>> ultimate source of the truth, and .gitmodules is a hint to prime
>> that when the user does "git submodule init", no?
>
> "git submodule init" only copies the "update" and "url" settings
> to .git/config, all others default to the value they have in the
> .gitmodules file if they aren't found in .git/config. This allows
> upstream to change these settings unless the user copies them to
> .git/config himself.

I know what the code does. I was questioning if "only copies X and
Y" is a sensible thing.

Copying at init time will fix the values when copied and give the
user a stable and dependable behaviour.  I have a feeling that the
current "not copy to fix it to a stable value, but look into
.gitmodules as a fallback" was not a designed behaviour for the
other properties, but was done by accident and/or laziness.

Re: Git issues with submodules

[Jonathan Nieder]

Junio C Hamano wrote:

>                                          I have a feeling that the
> current "not copy to fix it to a stable value, but look into
> .gitmodules as a fallback" was not a designed behaviour for the
> other properties, but was done by accident and/or laziness.

It was designed.  See for example the thread surrounding [1]:

| And when you are on a superproject branch actively developing inside a
| submodule, you may want to increase fetch-activity to fetch all new
| commits in the submodule even if they aren't referenced in the
| superproject (yet), as that might be just what your fellow developers
| are about to do. And the person setting up that branch could do that
| once for all users so they don't have to repeat it in every clone. And
| when switching away from that branch all those developers cannot forget
| to reconfigure to fetch-on-demand, so not having that in .git/config is
| a plus here too.

Thanks,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/161193/focus=161357

Re: Git issues with submodules

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>>                                          I have a feeling that the
>> current "not copy to fix it to a stable value, but look into
>> .gitmodules as a fallback" was not a designed behaviour for the
>> other properties, but was done by accident and/or laziness.
>
> It was designed.  See for example the thread surrounding [1]:

OK, thanks.

>
> | And when you are on a superproject branch actively developing inside a
> | submodule, you may want to increase fetch-activity to fetch all new
> | commits in the submodule even if they aren't referenced in the
> | superproject (yet), as that might be just what your fellow developers
> | are about to do. And the person setting up that branch could do that
> | once for all users so they don't have to repeat it in every clone. And
> | when switching away from that branch all those developers cannot forget
> | to reconfigure to fetch-on-demand, so not having that in .git/config is
> | a plus here too.
>
> Thanks,
> Jonathan
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/161193/focus=161357

[RFC PATCH] disable complete ignorance of submodules for index <-> HEAD diff

[Heiko Voigt]

If the value of ignore for submodules is set to "all" we would not show
whats actually committed during status or diff. This can result in the
user committing unexpected submodule references. Lets be nicer and always
show whats in the index.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
This probably needs splitting up into two patches one for the
refactoring and one for the actual fix. It is also missing tests, but I
would first like to know what you think about this approach.

 builtin/diff.c | 43 +++++++++++++++++++++++++++----------------
 diff.h         |  2 +-
 submodule.c    |  6 ++++--
 wt-status.c    |  3 +++
 4 files changed, 35 insertions(+), 19 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index adb93a9..e9a356c 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -249,6 +249,21 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
 	return run_diff_files(revs, options);
 }
 
+static int have_cached_option(int argc, const char **argv)
+{
+	int i;
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+		if (!strcmp(arg, "--"))
+			return 0;
+		else if (!strcmp(arg, "--cached") ||
+			 !strcmp(arg, "--staged")) {
+			return 1;
+		}
+	}
+	return 0;
+}
+
 int cmd_diff(int argc, const char **argv, const char *prefix)
 {
 	int i;
@@ -259,6 +274,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	struct blobinfo blob[2];
 	int nongit;
 	int result = 0;
+	int have_cached;
 
 	/*
 	 * We could get N tree-ish in the rev.pending_objects list.
@@ -305,6 +321,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 
 	if (nongit)
 		die(_("Not a git repository"));
+
+	have_cached = have_cached_option(argc, argv);
+	if (have_cached)
+		DIFF_OPT_SET(&rev.diffopt, NO_IGNORE_SUBMODULE);
+
 	argc = setup_revisions(argc, argv, &rev, NULL);
 	if (!rev.diffopt.output_format) {
 		rev.diffopt.output_format = DIFF_FORMAT_PATCH;
@@ -319,22 +340,12 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	 * Do we have --cached and not have a pending object, then
 	 * default to HEAD by hand.  Eek.
 	 */
-	if (!rev.pending.nr) {
-		int i;
-		for (i = 1; i < argc; i++) {
-			const char *arg = argv[i];
-			if (!strcmp(arg, "--"))
-				break;
-			else if (!strcmp(arg, "--cached") ||
-				 !strcmp(arg, "--staged")) {
-				add_head_to_pending(&rev);
-				if (!rev.pending.nr) {
-					struct tree *tree;
-					tree = lookup_tree(EMPTY_TREE_SHA1_BIN);
-					add_pending_object(&rev, &tree->object, "HEAD");
-				}
-				break;
-			}
+	if (!rev.pending.nr && have_cached) {
+		add_head_to_pending(&rev);
+		if (!rev.pending.nr) {
+			struct tree *tree;
+			tree = lookup_tree(EMPTY_TREE_SHA1_BIN);
+			add_pending_object(&rev, &tree->object, "HEAD");
 		}
 	}
 
diff --git a/diff.h b/diff.h
index e342325..81561b3 100644
--- a/diff.h
+++ b/diff.h
@@ -64,7 +64,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)
 #define DIFF_OPT_FOLLOW_RENAMES      (1 <<  7)
 #define DIFF_OPT_RENAME_EMPTY        (1 <<  8)
-/* (1 <<  9) unused */
+#define DIFF_OPT_NO_IGNORE_SUBMODULE (1 <<  9)
 #define DIFF_OPT_HAS_CHANGES         (1 << 10)
 #define DIFF_OPT_QUICK               (1 << 11)
 #define DIFF_OPT_NO_INDEX            (1 << 12)
diff --git a/submodule.c b/submodule.c
index 1905d75..9d81712 100644
--- a/submodule.c
+++ b/submodule.c
@@ -301,9 +301,11 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt,
 	DIFF_OPT_CLR(diffopt, IGNORE_UNTRACKED_IN_SUBMODULES);
 	DIFF_OPT_CLR(diffopt, IGNORE_DIRTY_SUBMODULES);
 
-	if (!strcmp(arg, "all"))
+	if (!strcmp(arg, "all")) {
+		if (DIFF_OPT_TST(diffopt, NO_IGNORE_SUBMODULE))
+			return;
 		DIFF_OPT_SET(diffopt, IGNORE_SUBMODULES);
-	else if (!strcmp(arg, "untracked"))
+	} else if (!strcmp(arg, "untracked"))
 		DIFF_OPT_SET(diffopt, IGNORE_UNTRACKED_IN_SUBMODULES);
 	else if (!strcmp(arg, "dirty"))
 		DIFF_OPT_SET(diffopt, IGNORE_DIRTY_SUBMODULES);
diff --git a/wt-status.c b/wt-status.c
index b4e44ba..34be1cc 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -462,6 +462,9 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 		handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
 	}
 
+	/* for the index we need to disable complete ignorance of submodules */
+	DIFF_OPT_SET(&rev.diffopt, NO_IGNORE_SUBMODULE);
+
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = wt_status_collect_updated_cb;
 	rev.diffopt.format_callback_data = s;
-- 
1.8.5.rc3.1.gcd6363f

Re: [RFC PATCH] disable complete ignorance of submodules for index <-> HEAD diff

[Sergey Sharybin]

Hi,

Tested the patch. `git status` now shows the changes to the
submodules, which is nice :)

However, is it possible to make it so `git commit` lists submodules in
"changes to be committed" section, so you'll see what's gonna to be in
the commit while typing the commit message as well?

On Sat, Nov 23, 2013 at 7:11 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> If the value of ignore for submodules is set to "all" we would not show
> whats actually committed during status or diff. This can result in the
> user committing unexpected submodule references. Lets be nicer and always
> show whats in the index.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---
> This probably needs splitting up into two patches one for the
> refactoring and one for the actual fix. It is also missing tests, but I
> would first like to know what you think about this approach.
>
>  builtin/diff.c | 43 +++++++++++++++++++++++++++----------------
>  diff.h         |  2 +-
>  submodule.c    |  6 ++++--
>  wt-status.c    |  3 +++
>  4 files changed, 35 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/diff.c b/builtin/diff.c
> index adb93a9..e9a356c 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -249,6 +249,21 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
>         return run_diff_files(revs, options);
>  }
>
> +static int have_cached_option(int argc, const char **argv)
> +{
> +       int i;
> +       for (i = 1; i < argc; i++) {
> +               const char *arg = argv[i];
> +               if (!strcmp(arg, "--"))
> +                       return 0;
> +               else if (!strcmp(arg, "--cached") ||
> +                        !strcmp(arg, "--staged")) {
> +                       return 1;
> +               }
> +       }
> +       return 0;
> +}
> +
>  int cmd_diff(int argc, const char **argv, const char *prefix)
>  {
>         int i;
> @@ -259,6 +274,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>         struct blobinfo blob[2];
>         int nongit;
>         int result = 0;
> +       int have_cached;
>
>         /*
>          * We could get N tree-ish in the rev.pending_objects list.
> @@ -305,6 +321,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>
>         if (nongit)
>                 die(_("Not a git repository"));
> +
> +       have_cached = have_cached_option(argc, argv);
> +       if (have_cached)
> +               DIFF_OPT_SET(&rev.diffopt, NO_IGNORE_SUBMODULE);
> +
>         argc = setup_revisions(argc, argv, &rev, NULL);
>         if (!rev.diffopt.output_format) {
>                 rev.diffopt.output_format = DIFF_FORMAT_PATCH;
> @@ -319,22 +340,12 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>          * Do we have --cached and not have a pending object, then
>          * default to HEAD by hand.  Eek.
>          */
> -       if (!rev.pending.nr) {
> -               int i;
> -               for (i = 1; i < argc; i++) {
> -                       const char *arg = argv[i];
> -                       if (!strcmp(arg, "--"))
> -                               break;
> -                       else if (!strcmp(arg, "--cached") ||
> -                                !strcmp(arg, "--staged")) {
> -                               add_head_to_pending(&rev);
> -                               if (!rev.pending.nr) {
> -                                       struct tree *tree;
> -                                       tree = lookup_tree(EMPTY_TREE_SHA1_BIN);
> -                                       add_pending_object(&rev, &tree->object, "HEAD");
> -                               }
> -                               break;
> -                       }
> +       if (!rev.pending.nr && have_cached) {
> +               add_head_to_pending(&rev);
> +               if (!rev.pending.nr) {
> +                       struct tree *tree;
> +                       tree = lookup_tree(EMPTY_TREE_SHA1_BIN);
> +                       add_pending_object(&rev, &tree->object, "HEAD");
>                 }
>         }
>
> diff --git a/diff.h b/diff.h
> index e342325..81561b3 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -64,7 +64,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
>  #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)
>  #define DIFF_OPT_FOLLOW_RENAMES      (1 <<  7)
>  #define DIFF_OPT_RENAME_EMPTY        (1 <<  8)
> -/* (1 <<  9) unused */
> +#define DIFF_OPT_NO_IGNORE_SUBMODULE (1 <<  9)
>  #define DIFF_OPT_HAS_CHANGES         (1 << 10)
>  #define DIFF_OPT_QUICK               (1 << 11)
>  #define DIFF_OPT_NO_INDEX            (1 << 12)
> diff --git a/submodule.c b/submodule.c
> index 1905d75..9d81712 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -301,9 +301,11 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt,
>         DIFF_OPT_CLR(diffopt, IGNORE_UNTRACKED_IN_SUBMODULES);
>         DIFF_OPT_CLR(diffopt, IGNORE_DIRTY_SUBMODULES);
>
> -       if (!strcmp(arg, "all"))
> +       if (!strcmp(arg, "all")) {
> +               if (DIFF_OPT_TST(diffopt, NO_IGNORE_SUBMODULE))
> +                       return;
>                 DIFF_OPT_SET(diffopt, IGNORE_SUBMODULES);
> -       else if (!strcmp(arg, "untracked"))
> +       } else if (!strcmp(arg, "untracked"))
>                 DIFF_OPT_SET(diffopt, IGNORE_UNTRACKED_IN_SUBMODULES);
>         else if (!strcmp(arg, "dirty"))
>                 DIFF_OPT_SET(diffopt, IGNORE_DIRTY_SUBMODULES);
> diff --git a/wt-status.c b/wt-status.c
> index b4e44ba..34be1cc 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -462,6 +462,9 @@ static void wt_status_collect_changes_index(struct wt_status *s)
>                 handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
>         }
>
> +       /* for the index we need to disable complete ignorance of submodules */
> +       DIFF_OPT_SET(&rev.diffopt, NO_IGNORE_SUBMODULE);
> +
>         rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
>         rev.diffopt.format_callback = wt_status_collect_updated_cb;
>         rev.diffopt.format_callback_data = s;
> --
> 1.8.5.rc3.1.gcd6363f
>



-- 
With best regards, Sergey Sharybin

Re: Re: [RFC PATCH] disable complete ignorance of submodules for index <-> HEAD diff

[Heiko Voigt]

On Mon, Nov 25, 2013 at 03:01:34PM +0600, Sergey Sharybin wrote:
> Tested the patch. `git status` now shows the changes to the
> submodules, which is nice :)
> 
> However, is it possible to make it so `git commit` lists submodules in
> "changes to be committed" section, so you'll see what's gonna to be in
> the commit while typing the commit message as well?

Yes, of course that should be shown. Will add in the next iteration.
Which will hopefully be a much simpler implementation. Possibly getting
rid of this new flag.

Cheers Heiko

[RFC/WIP PATCH v2] disable complete ignorance of submodules for index <-> HEAD diff

[Heiko Voigt]

If the value of ignore for submodules is set to "all" we would not show
whats actually committed during status or diff. This can result in the
user committing unexpected submodule references. Lets be nicer and always
show whats in the index.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
On Thu, Nov 28, 2013 at 08:10:01AM +0100, Heiko Voigt wrote:
> On Mon, Nov 25, 2013 at 03:01:34PM +0600, Sergey Sharybin wrote:
> > Tested the patch. `git status` now shows the changes to the
> > submodules, which is nice :)
> > 
> > However, is it possible to make it so `git commit` lists submodules in
> > "changes to be committed" section, so you'll see what's gonna to be in
> > the commit while typing the commit message as well?
> 
> Yes, of course that should be shown. Will add in the next iteration.
> Which will hopefully be a much simpler implementation. Possibly getting
> rid of this new flag.

Here is an updated version of this patch. The code is a little bit more
simplified and I changed the existing tests to account for the new
behavior we are discussing. If everyone agrees that this a desired
change in behavior I would continue adding more tests so commit, status
and so on are more explicitly tested.

Cheers Heiko

P.S.: This is still work in progress, the complete series should contain
both my patches from this thread.

 builtin/diff.c            |  2 ++
 diff-lib.c                |  3 +++
 diff.h                    |  2 +-
 submodule.c               | 16 ++++++++++++++--
 submodule.h               |  1 +
 t/t4027-diff-submodule.sh | 12 +++++++++---
 t/t7508-status.sh         |  6 +++++-
 7 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index adb93a9..c47614d 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -162,6 +162,8 @@ static int builtin_diff_tree(struct rev_info *revs,
 	if (argc > 1)
 		usage(builtin_diff_usage);
 
+	enforce_no_complete_ignore_submodule(&revs->diffopt);
+
 	/*
 	 * We saw two trees, ent0 and ent1.  If ent1 is uninteresting,
 	 * swap them.
diff --git a/diff-lib.c b/diff-lib.c
index 346cac6..c5219cb 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -483,6 +483,9 @@ int run_diff_index(struct rev_info *revs, int cached)
 {
 	struct object_array_entry *ent;
 
+	if (cached)
+		enforce_no_complete_ignore_submodule(&revs->diffopt);
+
 	ent = revs->pending.objects;
 	if (diff_cache(revs, ent->item->sha1, ent->name, cached))
 		exit(128);
diff --git a/diff.h b/diff.h
index e342325..81561b3 100644
--- a/diff.h
+++ b/diff.h
@@ -64,7 +64,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)
 #define DIFF_OPT_FOLLOW_RENAMES      (1 <<  7)
 #define DIFF_OPT_RENAME_EMPTY        (1 <<  8)
-/* (1 <<  9) unused */
+#define DIFF_OPT_NO_IGNORE_SUBMODULE (1 <<  9)
 #define DIFF_OPT_HAS_CHANGES         (1 << 10)
 #define DIFF_OPT_QUICK               (1 << 11)
 #define DIFF_OPT_NO_INDEX            (1 << 12)
diff --git a/submodule.c b/submodule.c
index 1905d75..e0719b6 100644
--- a/submodule.c
+++ b/submodule.c
@@ -294,6 +294,16 @@ int parse_submodule_config_option(const char *var, const char *value)
 	return 0;
 }
 
+void enforce_no_complete_ignore_submodule(struct diff_options *diffopt)
+{
+	DIFF_OPT_SET(diffopt, NO_IGNORE_SUBMODULE);
+	if (DIFF_OPT_TST(diffopt, OVERRIDE_SUBMODULE_CONFIG) &&
+	    DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES)) {
+		DIFF_OPT_CLR(diffopt, IGNORE_SUBMODULES);
+		DIFF_OPT_SET(diffopt, IGNORE_DIRTY_SUBMODULES);
+	}
+}
+
 void handle_ignore_submodules_arg(struct diff_options *diffopt,
 				  const char *arg)
 {
@@ -301,9 +311,11 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt,
 	DIFF_OPT_CLR(diffopt, IGNORE_UNTRACKED_IN_SUBMODULES);
 	DIFF_OPT_CLR(diffopt, IGNORE_DIRTY_SUBMODULES);
 
-	if (!strcmp(arg, "all"))
+	if (!strcmp(arg, "all")) {
+		if (DIFF_OPT_TST(diffopt, NO_IGNORE_SUBMODULE))
+			return;
 		DIFF_OPT_SET(diffopt, IGNORE_SUBMODULES);
-	else if (!strcmp(arg, "untracked"))
+	} else if (!strcmp(arg, "untracked"))
 		DIFF_OPT_SET(diffopt, IGNORE_UNTRACKED_IN_SUBMODULES);
 	else if (!strcmp(arg, "dirty"))
 		DIFF_OPT_SET(diffopt, IGNORE_DIRTY_SUBMODULES);
diff --git a/submodule.h b/submodule.h
index 7beec48..2c8087e 100644
--- a/submodule.h
+++ b/submodule.h
@@ -20,6 +20,7 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 int submodule_config(const char *var, const char *value, void *cb);
 void gitmodules_config(void);
 int parse_submodule_config_option(const char *var, const char *value);
+void enforce_no_complete_ignore_submodule(struct diff_options *diffopt);
 void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
 int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
 void show_submodule_summary(FILE *f, const char *path,
diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 518bf95..bd84ea7 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -258,7 +258,9 @@ test_expect_success 'git diff between submodule commits' '
 	expect_from_to >expect.body $subtip $subprev &&
 	test_cmp expect.body actual.body &&
 	git diff --ignore-submodules HEAD^..HEAD >actual &&
-	! test -s actual
+	sed -e "1,/^@@/d" actual >actual.body &&
+	expect_from_to >expect.body $subtip $subprev &&
+	test_cmp expect.body actual.body
 '
 
 test_expect_success 'git diff between submodule commits [.git/config]' '
@@ -274,7 +276,9 @@ test_expect_success 'git diff between submodule commits [.git/config]' '
 	test_cmp expect.body actual.body &&
 	git config submodule.subname.ignore all &&
 	git diff HEAD^..HEAD >actual &&
-	! test -s actual &&
+	sed -e "1,/^@@/d" actual >actual.body &&
+	expect_from_to >expect.body $subtip $subprev &&
+	test_cmp expect.body actual.body &&
 	git diff --ignore-submodules=dirty HEAD^..HEAD >actual &&
 	sed -e "1,/^@@/d" actual >actual.body &&
 	expect_from_to >expect.body $subtip $subprev &&
@@ -294,7 +298,9 @@ test_expect_success 'git diff between submodule commits [.gitmodules]' '
 	test_cmp expect.body actual.body &&
 	git config -f .gitmodules submodule.subname.ignore all &&
 	git diff HEAD^..HEAD >actual &&
-	! test -s actual &&
+	sed -e "1,/^@@/d" actual >actual.body &&
+	expect_from_to >expect.body $subtip $subprev &&
+	test_cmp expect.body actual.body &&
 	git config submodule.subname.ignore dirty &&
 	git config submodule.subname.path sub &&
 	git diff  HEAD^..HEAD >actual &&
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index c987b5e..977295f 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1357,6 +1357,11 @@ test_expect_success "status (core.commentchar with two chars with submodule summ
 test_expect_success "--ignore-submodules=all suppresses submodule summary" '
 	cat > expect << EOF &&
 On branch master
+Changes to be committed:
+  (use "git reset HEAD <file>..." to unstage)
+
+	modified:   sm
+
 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)
@@ -1374,7 +1379,6 @@ Untracked files:
 	output
 	untracked
 
-no changes added to commit (use "git add" and/or "git commit -a")
 EOF
 	git status --ignore-submodules=all > output &&
 	test_i18ncmp expect output
-- 
1.8.5.rc3.1.g223caec

Re: Re: Git issues with submodules

[Ramkumar Ramachandra]

Heiko Voigt wrote:
> What I think needs fixing here first is that the ignore setting should not
> apply to any diffs between HEAD and index. IMO, it should only apply
> to the diff between worktree and index.
>
> When we have that the user does not see the submodule changed when
> normally working. But after doing git add . the change to the submodule
> should be shown in status and diff regardless of the configuration.

Yeah, I think this is a good direction.

> After that we can discuss whether add should add submodules that are
> tracked but not shown. How about commit -a ? Should it also ignore the
> change?

Here, I think ignored submodules should behave like files matched by
.gitignore: add should not add (`add -f` would be a good way to force
it), and `commit -a` should also exclude it.

Re: Git issues with submodules

[Jens Lehmann]

Am 22.11.2013 22:54, schrieb Heiko Voigt:
> What I think needs fixing here first is that the ignore setting should not
> apply to any diffs between HEAD and index. IMO, it should only apply
> to the diff between worktree and index.

Not only that. It should also apply to diffs between commits/trees
and work tree but not between commits/trees. The reason the ignore
setting was added three years ago was to avoid expensive work tree
operations when it was clear that either the information wasn't
wanted or it took too much time to determine that. And I doubt you
want to see modifications to submodules in your work tree when
diffing against HEAD but not when diffing against the index.

And this behavior happens to be just what the floating branch model
needs too. I'm not sure there isn't a use case out there that also
needs to silence diff & friends regarding submodule changes between
commits/trees and/or index too (even though I cannot come up with
one at the moment). So I propose to add "worktree" as another value
for the ignore option - which ignores submodule modifications in
the work tree - and leave "all" as it is.

Re: Re: Git issues with submodules

[Heiko Voigt]

On Sat, Nov 23, 2013 at 09:32:45PM +0100, Jens Lehmann wrote:
> Am 22.11.2013 22:54, schrieb Heiko Voigt:
> > What I think needs fixing here first is that the ignore setting should not
> > apply to any diffs between HEAD and index. IMO, it should only apply
> > to the diff between worktree and index.
> 
> Not only that. It should also apply to diffs between commits/trees
> and work tree but not between commits/trees. The reason the ignore
> setting was added three years ago was to avoid expensive work tree
> operations when it was clear that either the information wasn't
> wanted or it took too much time to determine that. And I doubt you
> want to see modifications to submodules in your work tree when
> diffing against HEAD but not when diffing against the index.
> 
> And this behavior happens to be just what the floating branch model
> needs too. I'm not sure there isn't a use case out there that also
> needs to silence diff & friends regarding submodule changes between
> commits/trees and/or index too (even though I cannot come up with
> one at the moment). So I propose to add "worktree" as another value
> for the ignore option - which ignores submodule modifications in
> the work tree - and leave "all" as it is.

I am not so sure about that. Only finding out what has changed (commit
wise) in a submodule is expensive. Just finding out whether a submodule
sha1 has changed is not expensive. Maybe we should completely stop
respecting the ignore=all setting for history and diff between index and
HEAD. AFAIK, we do not have any other setting that instruct git to
ignore specific parts of the history unless explicitly asked for by
specifying a pathspec.

And I think a user should never miss by accident that something has
changed in the repository.

Cheers Heiko

Re: Git issues with submodules

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> What I think needs fixing here first is that the ignore setting should not
> apply to any diffs between HEAD and index. IMO, it should only apply
> to the diff between worktree and index.

Hmph.  How about "git diff $commit", the diff between the worktree and
a named commit (which may or may not be HEAD)?

> When we have that the user does not see the submodule changed when
> normally working. But after doing git add . the change to the submodule
> should be shown in status and diff regardless of the configuration.

Yes, that sounds sensible.

Re: Re: Git issues with submodules

[Heiko Voigt]

On Mon, Nov 25, 2013 at 12:53:45PM -0800, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> > What I think needs fixing here first is that the ignore setting should not
> > apply to any diffs between HEAD and index. IMO, it should only apply
> > to the diff between worktree and index.
> 
> Hmph.  How about "git diff $commit", the diff between the worktree and
> a named commit (which may or may not be HEAD)?

Thats an interesting question. My first thought was that I would expect
it to not show submodules since it involves the worktree, but then I could
also argue that it should only show differences between whats in the
index and the given commit. That would make matters more complicated but
I image the use case (floating submodules) involves not caring
about submodules except for some integration points when submodule sha1's
are explicitly recorded. I would expect to only see diffs between these
integration points. But then I am not a big user (none at all at the
moment) of the floating model.

Cheers Heiko

Re: Git issues with submodules

[Ramkumar Ramachandra]

Jens Lehmann wrote:
> But the question is if that is the right thing to do: should
> diff.ignoreSubmodules and submodule.<name>.ignore only affect
> the diff family or also git log & friends? That would make
> users blind for submodule history (which they already are
> when using diff & friends, so that might be ok here too).

No, I think it's the wrong thing to do. We don't want to show false history.

> The ignore setting is documented to only affect diff output
> (including what checkout, commit and status show as modified).
> While I agree that this behavior is confusing for Sergey and
> not optimal for the floating branch model he uses, git is
> currently doing exactly what it should. And for people using
> the ignore setting to not having to stat submodules with huge
> and/or many files that behavior is what they want: don't bother
> me with what changed, but commit what I did change on purpose.
> We may have to rethink what should happen for users of the
> floating branch model though.

I'd argue that the only reason the diff-family is blind is because the
commit hash changes in the first place; if the hash didn't change (ie.
floating submodules were represented by 0s hash or something), we
wouldn't have this problem. The correct solution is to also make `git
add' blind.

Re: Git issues with submodules

[Ramkumar Ramachandra]

Jeff King wrote:
>> I just checked it out: it uses `git ls-files -m` to get the list of
>> unstaged changes; `git diff --name-only HEAD --` will list staged
>> changes as well.
>
> That diff command compares the working tree and HEAD; if you are trying
> to match `ls-files -m`, you probably wanted just `git diff --name-only`
> to compare the working tree and the index. Although in a script you'd
> probably want to use the plumbing `git diff-files` instead.

Thanks for that. It's probably not worth fixing ls-files; I'll patch
Arcanist to use diff-files instead.

Re: Git issues with submodules

[Jens Lehmann]

Am 22.11.2013 17:12, schrieb Ramkumar Ramachandra:
> Jeff King wrote:
>>> I just checked it out: it uses `git ls-files -m` to get the list of
>>> unstaged changes; `git diff --name-only HEAD --` will list staged
>>> changes as well.
>>
>> That diff command compares the working tree and HEAD; if you are trying
>> to match `ls-files -m`, you probably wanted just `git diff --name-only`
>> to compare the working tree and the index. Although in a script you'd
>> probably want to use the plumbing `git diff-files` instead.
> 
> Thanks for that. It's probably not worth fixing ls-files; I'll patch
> Arcanist to use diff-files instead.

Good to have an short term solution for Sergey, but Heiko and I
discussed this issue and agreed that we should fix ls-files. After
all the user explicitly asked not to be bothered with submodule
differences by configuring the ignore setting.