Supporting `git add -a <exclude submodules>`

Supporting `git add -a <exclude submodules>`

[Joanna Wang]

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!

Re: Supporting `git add -a <exclude submodules>`

[Junio C Hamano]

Joanna Wang <jojwang@chromium.org> writes:

> 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

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.

> - add magic pathspec support to `git add`

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.

> (It looks like `git commit -a` does not work with pathspec.

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.

Re: Supporting `git add -a <exclude submodules>`

[Junio C Hamano]

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

>> (It looks like `git commit -a` does not work with pathspec.
>
> You should be able to do something like 
>
>     $ git commit . ':(exclude)t/'
>
> I think.

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.

Re: Supporting `git add -a <exclude submodules>`

[Junio C Hamano]

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

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

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.

Re: Supporting `git add -a <exclude submodules>`

[Joanna Wang]

> choose among attr:submodule, attr:type=<what>, attr:bits=<permbits>, decide what keyword to use
Whatever we choose, do we want to block these keywords from being used
by folks in their .gitattributes files?
That would break any existing usage of the keywords. Is this a concern?

Option A: To keep things working, we could add this support for attr,
but then always prioritize whatever is set in .gitattributes. So this
new behavior would only be triggered if the keywords (e.g.
"submodule", "type", "bits") aren't set in .gitattributes (or w/e list
of attributes are being read).

Option B: Or we could add this new behavior for `attr-bits:<permbits>`
or just `bits:<permbits>`.

I personally don't see big UX differences between A and B. Any opinions on this?

Re: Supporting `git add -a <exclude submodules>`

[Junio C Hamano]

Joanna Wang <jojwang@google.com> writes:

>> choose among attr:submodule, attr:type=<what>, attr:bits=<permbits>, decide what keyword to use

> Whatever we choose, do we want to block these keywords from being used
> by folks in their .gitattributes files?
> That would break any existing usage of the keywords. Is this a concern?

> Option A: To keep things working, we could add this support for attr,
> but then always prioritize whatever is set in .gitattributes. So this
> new behavior would only be triggered if the keywords (e.g.
> "submodule", "type", "bits") aren't set in .gitattributes (or w/e list
> of attributes are being read).

Without thinking things through, I think this sounds easier to
explain to the users.  I have to wonder how one would implement such
override, though.  Suppose we are doing attr:bits=160000, so we ask
git_check_attr() about "bits" attribute. In collect_some_attrs(), we
will have "bits" in the check[] array.  We prepare the attr_stack
and fill(), which would go grab whatever is defined in the
attributes system.  We'll lstat() or do the equivalent conversion
from the tree_entry.mode to permission bits only if the attributes
system has nothing to say for that "bits" attribute.  I think a few
key things that needs to be thought out are:

 (1) where in that callchain would we intercept and insert our own
     "bits" value based on the filesystem property?

 (2) does collect_some_attrs() or git_check_attr() leave enough
     information in check[] to tell between [a] the attr stack had
     no opinion on "bits" attribute and [b] the attr stack
     explicitly said "bits" attribute is unspecified?

Re: Supporting `git add -a <exclude submodules>`

[Joanna Wang]

 > (2) does collect_some_attrs() or git_check_attr() leave enough
 >   information in check[] to tell between [a] the attr stack had
 >   no opinion on "bits" attribute and [b] the attr stack
 >   explicitly said "bits" attribute is unspecified?
I will double check this but IIUC, I could change this part
(https://github.com/git/git/blob/master/attr.c#L1100-L1105),
such that:
```
if (n == ATTR__UNKNOWN && v = NULL):
  // keep n equal to ATTR_UNKNOWN
```

And here: https://github.com/git/git/blob/master/attr.c#L1238,
if value == 'ATTR__UNKNOWN', then keep it at ATTR__UNKNOWN

> (1) where in that callchain would we intercept and insert our own
>   "bits" value based on the filesystem property?
I was planning to do that somewhere around here:
https://github.com/git/git/blob/master/pathspec.c#L764-L767
 and possibly combine it with adding a new match_mode (e.g.
MATCH_BITS) which would be set by parse_pathspec().
Something like:
```
else if (ATTR_UNKNOWN(value))
    matched = (match_mode == MATCH_BITS &&
!strcmp(item->attr_match[i].value, get_mode(path)));
```

I will dive into the code more to confirm, but that's my current
high-level plan, in case you immediately see something very wrong.

Re: Supporting `git add -a <exclude submodules>`

[Junio C Hamano]

Joanna Wang <jojwang@google.com> writes:

> I will dive into the code more to confirm, but that's my current
> high-level plan, in case you immediately see something very wrong.

Nothing I immediately see glaringly wrong in there ;-)

Thanks for digging!