Supporting `git add -a <exclude submodules>`

Hi, We're looking for feedback on the idea of adding git support for users to express `git add -a <exclude submodules>`.

We recently added submodules to our project. A lot of our users don't like that when their submodules are on commits that do not match what is pinned in the root superproject, running `git add -a` and `git commit -a` (which they are so used to), will now include those submodules that 99% of the time they do not want to include.

So we think supporting `git add -a <exclude submodules>` and `git commit -a <exclude submodules>` might be good to do. (We know `git -c diff.ignoreSubmodules=all commit -a` is an option, but this does not work for `git add`. However this support gets implemented for `git add`, I assume it's ideal for the UX of "don't include submodules" to be the same for both `git commit` and `git add`, though I don't think this is a requirement)

In a separate high-level conversation, Junio brought up going about
this with magic pathsepc.
So the work would entail:
- adding 'native' submodule attribute support, so `git add -a
':(exclude,attr:submodule)' can work without having to add 'submodule'
for every submodule path in .gitattributes
- add magic pathspec support to `git add`

(It looks like `git commit -a` does not work with pathspec. In this case I think that would be fine, we can just tell users they can set aliases for `git -c diff.ignoreSubmodules=all commit -a` and `git add -a ':(exclude,attr:submodule)' if they want)

Any thoughts from folks on all of the above? Thanks!

Joanna Wang <jojwang@chromium.org> writes:

I personally do not think it is a great interface that allows you to only say "all submodules in my tree, are equally special in the same way, and I do not want any of them" without allowing you to say "paths in this directory, be they files, symlinks, or submodules, are special and I do not want any of them". The former smells like a very narrow hack that does not generally extend. So compared to "git add -a --exclude-submodules", a solution using magic pathspec mechanism and syntax would be a great improvement.

The spelling of the attribute (if we were to use the "attr" magic, that is) may want to allow a bit more flexibility, e.g., "attr:type=gitlink", or even "attr:bits=160000", that would allow natural extension to cover symbolic links and other oddities.

Yup, I do not offhand remember why "git add" does not want to take the magic pathspec with an "attr" component. Whoever wants to tip in to this effort may want to first try what breaks when it is enabled without changing anything, write tests, and then fix the fallouts, which can be done separately and in parallel with an effort to design how the way to express "does this path represent a submodule? a regular blob? a symbolic link? something else?" with the magic pathspec syntax (i.e., choose among attr:submodule, attr:type=<what>, attr:bits=<permbits>, decide what keyword to use), and implement that design.

You should be able to do something like 

    $ git commit . ':(exclude)t/'

I think.

By the way, I am surprised that "diff.ignoreSubmodules=all" is abused by anybody that way. It depends on the implementation detail that we internally use the diff machinery to find which paths are not modified, and I would even say it is a BUG that "commit -a" pays attention to the configuration variable that way. I would recommend strongly to stay away from that approach or your tools will get broken when the bug gets fixed.

Also, I have doubts on the higher level workflow issue that makes your people want to keep their submodules (or any subdirectories for that matter) dirty and out of sync with the superproject, and commit their work product from that state. It means whatever their local building and testing gave them cannot be trusted when evaluating the resulting commit at the superproject level.

Junio C Hamano <gitster@pobox.com> writes:

Correction. You would need the "-i(nclude)" option if you do the above, as the default is "-o(nly)", which will deliberately ignore what has been added to the index.

Junio C Hamano <gitster@pobox.com> writes:

Sorry, but I take this part back. This is an explicitly documented feature of the variable, so users and scripts alike should be able to depend on. It still feels like a layering violation, but it is too late to "fix" it now.