[PATCH] doc: clarify non-recursion for ignore paths like `foo/`

[PATCH] doc: clarify non-recursion for ignore paths like `foo/`

[Dakota Hawkins]

>> At any rate, would it at least be a good idea to make the "trailing
>> slash halts recursion, won't consider nested .gitignore files"
>> explicit in the `.gitignore` doc? Unless I'm missing it, I don't think
>> that behavior is called out (or at least not called out concisely/in
>> one place). It looks like this is all there is:
>
> Yeah, it's definitely come up multiple times over the years. I don't
> know what all is in gitignore(5), but if it's not mentioned it probably
> should be.
>
>>     "If the pattern ends with a slash, it is removed for the purpose
>> of the following description, but it would only find a match with a
>> directory. In other words, foo/ will match a directory foo and paths
>> underneath it, but will not match a regular file or a symbolic link
>> foo (this is consistent with the way how pathspec works in general in
>> Git)."
>>
>> Also, I'm not sure what the "following description" is in "it is
>> removed for the purpose of the following description". Is that trying
>> to imply "excluded from the rest of the doc"?
>
> I think it means "for the rest of the description of how the patterns
> work". I.e., "foo/" matches as "foo" when the rest of the matching rules
> are applied. I agree it's a bit awkward. Patches welcome. :)

I hope this is correct. I tried to follow the instructions. Please let
me know if I need to fix something with how I'm sending this!

-- >8 --
Subject: [PATCH] doc: clarify non-recursion for ignore paths like `foo/`

The behavior of .gitignore patterns ending with trailing slashes is
unclear. The user may expect subsequent matching patterns to matter, while
they do not. For example:

  foo/       # Ignores `foo` directories and everything inside of them
  !foo/*.txt # Does nothing

Explain this behavior (and its implications) more explicitly.

Signed-off-by: Dakota Hawkins <daktoahawkins@gmail.com>
---
 Documentation/gitignore.txt | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index ff5d7f9ed..e9c34c1d5 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt
@@ -89,12 +89,17 @@ PATTERN FORMAT
    Put a backslash ("`\`") in front of the first "`!`" for patterns
    that begin with a literal "`!`", for example, "`\!important!.txt`".

- - If the pattern ends with a slash, it is removed for the
-   purpose of the following description, but it would only find
-   a match with a directory.  In other words, `foo/` will match a
-   directory `foo` and paths underneath it, but will not match a
-   regular file or a symbolic link `foo` (this is consistent
-   with the way how pathspec works in general in Git).
+ - If the pattern ends with a slash it will match directories but prevent
+   further recursion into subdirectories. In other words, `foo/` will match a
+   directory `foo`, excluding files and paths underneath it, but will not match
+   a regular file or a symbolic link `foo` (this is consistent with the way
+   that pathspec works in general in Git). Consequently, `foo/` will prevent
+   consideration of subsequent matches, including exclusions (for example,
+   `!foo/*.noignore`). In order to match `foo/` directories while allowing for
+   possible later exclusions, consider using a trailing wildcard (`foo/*`).
+   Note that matching directories with a trailing wildcard incurs some
+   additional performance cost, since it requires recursion into
+   subdirectories.

  - If the pattern does not contain a slash '/', Git treats it as
    a shell glob pattern and checks for a match against the
-- 
2.16.2.windows.1

Re: [PATCH] doc: clarify non-recursion for ignore paths like `foo/`

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 21 2018, Dakota Hawkins jotted:

>>> At any rate, would it at least be a good idea to make the "trailing
>>> slash halts recursion, won't consider nested .gitignore files"
>>> explicit in the `.gitignore` doc? Unless I'm missing it, I don't think
>>> that behavior is called out (or at least not called out concisely/in
>>> one place). It looks like this is all there is:
>>
>> Yeah, it's definitely come up multiple times over the years. I don't
>> know what all is in gitignore(5), but if it's not mentioned it probably
>> should be.
>>
>>>     "If the pattern ends with a slash, it is removed for the purpose
>>> of the following description, but it would only find a match with a
>>> directory. In other words, foo/ will match a directory foo and paths
>>> underneath it, but will not match a regular file or a symbolic link
>>> foo (this is consistent with the way how pathspec works in general in
>>> Git)."
>>>
>>> Also, I'm not sure what the "following description" is in "it is
>>> removed for the purpose of the following description". Is that trying
>>> to imply "excluded from the rest of the doc"?
>>
>> I think it means "for the rest of the description of how the patterns
>> work". I.e., "foo/" matches as "foo" when the rest of the matching rules
>> are applied. I agree it's a bit awkward. Patches welcome. :)
>
> I hope this is correct. I tried to follow the instructions. Please let
> me know if I need to fix something with how I'm sending this!
>
> -- >8 --
> Subject: [PATCH] doc: clarify non-recursion for ignore paths like `foo/`
>
> The behavior of .gitignore patterns ending with trailing slashes is
> unclear. The user may expect subsequent matching patterns to matter, while
> they do not. For example:
>
>   foo/       # Ignores `foo` directories and everything inside of them
>   !foo/*.txt # Does nothing
>
> Explain this behavior (and its implications) more explicitly.
>
> Signed-off-by: Dakota Hawkins <daktoahawkins@gmail.com>
> ---
>  Documentation/gitignore.txt | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
> index ff5d7f9ed..e9c34c1d5 100644
> --- a/Documentation/gitignore.txt
> +++ b/Documentation/gitignore.txt
> @@ -89,12 +89,17 @@ PATTERN FORMAT
>     Put a backslash ("`\`") in front of the first "`!`" for patterns
>     that begin with a literal "`!`", for example, "`\!important!.txt`".
>
> - - If the pattern ends with a slash, it is removed for the
> -   purpose of the following description, but it would only find
> -   a match with a directory.  In other words, `foo/` will match a
> -   directory `foo` and paths underneath it, but will not match a
> -   regular file or a symbolic link `foo` (this is consistent
> -   with the way how pathspec works in general in Git).
> + - If the pattern ends with a slash it will match directories but prevent
> +   further recursion into subdirectories. In other words, `foo/` will match a
> +   directory `foo`, excluding files and paths underneath it, but will not match
> +   a regular file or a symbolic link `foo` (this is consistent with the way
> +   that pathspec works in general in Git). Consequently, `foo/` will prevent
> +   consideration of subsequent matches, including exclusions (for example,
> +   `!foo/*.noignore`). In order to match `foo/` directories while allowing for
> +   possible later exclusions, consider using a trailing wildcard (`foo/*`).
> +   Note that matching directories with a trailing wildcard incurs some
> +   additional performance cost, since it requires recursion into
> +   subdirectories.
>
>   - If the pattern does not contain a slash '/', Git treats it as
>     a shell glob pattern and checks for a match against the

Just before the context your patch quotes, we have this in gitignore(5)
already:

    [...]It is not possible to re-include a file if a parent directory
    of that file is excluded. Git doesn’t list excluded directories for
    performance reasons, so any patterns on contained files have no
    effect, no matter where they are defined.[...]

I can't see how your change to the documentation doesn't just re-state
what we already have documented, which is not to say the docs can't be
improved, but then we should clearly state this in one place, not
re-state it within the span of a few paragraphs.

Re: [PATCH] doc: clarify non-recursion for ignore paths like `foo/`

[Dakota Hawkins]

First, apologies since I didn't get the in-reply-to correct in my
email header. I'm not sure how to do that (using gmail/gsuite).

Meant to reply to:
https://public-inbox.org/git/20180321075041.GA24701@sigill.intra.peff.net/

> Just before the context your patch quotes, we have this in gitignore(5)
> already:
>
>     [...]It is not possible to re-include a file if a parent directory
>     of that file is excluded. Git doesn’t list excluded directories for
>     performance reasons, so any patterns on contained files have no
>     effect, no matter where they are defined.[...]
>
> I can't see how your change to the documentation doesn't just re-state
> what we already have documented, which is not to say the docs can't be
> improved, but then we should clearly state this in one place, not
> re-state it within the span of a few paragraphs.

Context:

This came up originally because of confusion with .gitattributes
patterns, since gitattributes doesn't have the same `foo/` matching
behavior. Jeff King was kind enough to prepare a patch for that
documentation here:
https://public-inbox.org/git/20180320041454.GA15213@sigill.intra.peff.net/

Re-reading the section you quoted again a couple of times you're
correct, but somehow that wasn't clear to me despite reading/searching
for what I wanted to see several times.

While what I wrote may need improvement, I think there are a couple of
valid concerns with the way this behavior is documented currently:

  - Generally: Reading about pattern matching for .gitignore is
awkward on its face, since positive matches have negative consequences
(in other words `include = !match`).
  - Specifically: This behavior that is specific to `foo/` matches is
documented in the section for `!foo` matches. If you're trying to find
the implications of `foo/` you may not have read about `!foo` as
carefully.

Since this behavior is practically applicable to both pattern formats,
and since patterns in the sum of a repo's .gitignore files can get
somewhat complicated, I think it would be a good idea to either:

  - Do this and basically explain the same behavior twice in two
pattern format sections, or
  - Pull the documentation for this behavior out into another section
where users would be likely to find and understand it if they're
looking into either pattern format

Does that make sense?

What do you think?

Thanks for the feedback,

- Dakota

Re: [PATCH] doc: clarify non-recursion for ignore paths like `foo/`

[Dakota Hawkins]

One additional note, I think I was was wrong about this:

  "In order to match `foo/` directories while allowing for
possible later exclusions, consider using a trailing wildcard (`foo/*`).
Note that matching directories with a trailing wildcard incurs some
additional performance cost, since it requires recursion into
subdirectories."

`foo/*` will let `!foo/file` work but still seems to prevent `!foo/bar/file`.

On Wed, Mar 21, 2018 at 7:53 AM, Dakota Hawkins
<dakota@dakotahawkins.com> wrote:
> First, apologies since I didn't get the in-reply-to correct in my
> email header. I'm not sure how to do that (using gmail/gsuite).
>
> Meant to reply to:
> https://public-inbox.org/git/20180321075041.GA24701@sigill.intra.peff.net/
>
>> Just before the context your patch quotes, we have this in gitignore(5)
>> already:
>>
>>     [...]It is not possible to re-include a file if a parent directory
>>     of that file is excluded. Git doesn’t list excluded directories for
>>     performance reasons, so any patterns on contained files have no
>>     effect, no matter where they are defined.[...]
>>
>> I can't see how your change to the documentation doesn't just re-state
>> what we already have documented, which is not to say the docs can't be
>> improved, but then we should clearly state this in one place, not
>> re-state it within the span of a few paragraphs.
>
> Context:
>
> This came up originally because of confusion with .gitattributes
> patterns, since gitattributes doesn't have the same `foo/` matching
> behavior. Jeff King was kind enough to prepare a patch for that
> documentation here:
> https://public-inbox.org/git/20180320041454.GA15213@sigill.intra.peff.net/
>
> Re-reading the section you quoted again a couple of times you're
> correct, but somehow that wasn't clear to me despite reading/searching
> for what I wanted to see several times.
>
> While what I wrote may need improvement, I think there are a couple of
> valid concerns with the way this behavior is documented currently:
>
>   - Generally: Reading about pattern matching for .gitignore is
> awkward on its face, since positive matches have negative consequences
> (in other words `include = !match`).
>   - Specifically: This behavior that is specific to `foo/` matches is
> documented in the section for `!foo` matches. If you're trying to find
> the implications of `foo/` you may not have read about `!foo` as
> carefully.
>
> Since this behavior is practically applicable to both pattern formats,
> and since patterns in the sum of a repo's .gitignore files can get
> somewhat complicated, I think it would be a good idea to either:
>
>   - Do this and basically explain the same behavior twice in two
> pattern format sections, or
>   - Pull the documentation for this behavior out into another section
> where users would be likely to find and understand it if they're
> looking into either pattern format
>
> Does that make sense?
>
> What do you think?
>
> Thanks for the feedback,
>
> - Dakota

Re: [PATCH] doc: clarify non-recursion for ignore paths like `foo/`

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 21 2018, Dakota Hawkins wrote:

> Re-reading the section you quoted again a couple of times you're
> correct, but somehow that wasn't clear to me despite reading/searching
> for what I wanted to see several times.

FWIW I just knew this because I'd run into this the other day, so it was
fresh in my mind.

> While what I wrote may need improvement, I think there are a couple of
> valid concerns with the way this behavior is documented currently:
>
>   - Generally: Reading about pattern matching for .gitignore is
> awkward on its face, since positive matches have negative consequences
> (in other words `include = !match`).
>   - Specifically: This behavior that is specific to `foo/` matches is
> documented in the section for `!foo` matches. If you're trying to find
> the implications of `foo/` you may not have read about `!foo` as
> carefully.
>
> Since this behavior is practically applicable to both pattern formats,
> and since patterns in the sum of a repo's .gitignore files can get
> somewhat complicated, I think it would be a good idea to either:

I think it makes more sense to document it where it's documented now,
i.e. under how "!" works in general, since this is an edge case with
negative matching.

Whether you specify "foo" or "foo/" is then just an unrelated edge case
in how we match directories v.s. files, but doesn't per-se have anything
to do with how the inversion rules work, so I think it makes sense to
document it where we document the inversion rules.

I.e. you'd get the same for all of (assuming a directory "foo"):

    f*
    !*.txt

    foo
    !*.txt

    foo/
    !*.txt

So what we subsequently exclude just because it's deeper in the tree has
nothing to do with how the disambiguation syntax for matching the
directory looks like.

>   - Do this and basically explain the same behavior twice in two
> pattern format sections, or
>   - Pull the documentation for this behavior out into another section
> where users would be likely to find and understand it if they're
> looking into either pattern format

I think it can certainly be made clearer, but maybe with a practical
example (per above) where we also show the dir structure that
would/wouldn't be matched.

I just chimed in on this one because your patch says the docs are
"unclear" then "Explain this behavior (and its implications) more
explicitly" without a reference to the existing adjacent docs. I think
whatever we do we should be unifying these docs if they're amended, not
stating this twice.

Re: [PATCH] doc: clarify non-recursion for ignore paths like `foo/`

[Dakota Hawkins]

> I think it makes more sense to document it where it's documented now,
> i.e. under how "!" works in general, since this is an edge case with
> negative matching.

My reasoning is/was that while yes, that's true, I think it's likely
that the positive matches would be added before (in terms of git
history) the exclusions. In other words it may not occur to the user
writing an early version of `.gitignore` to think about exclusions if
they don't need to exclude anything yet -- the eventual symptoms of
this problem may be "far away" from the cause, making it harder to
diagnose (I'm living proof of that!)

> I think it can certainly be made clearer, but maybe with a practical
> example (per above) where we also show the dir structure that
> would/wouldn't be matched.
>
> I just chimed in on this one because your patch says the docs are
> "unclear" then "Explain this behavior (and its implications) more
> explicitly" without a reference to the existing adjacent docs. I think
> whatever we do we should be unifying these docs if they're amended, not
> stating this twice.

I think my inability to see it despite knowing exactly what I was
looking for (this time) makes it "unclear", at least to dummies me, so
I feel like that statement is at least somewhat defensible :)

If the explanation were unified, would the idiomatic way to do that be
to add some section way down the document with a couple of `(see SOME
DETAILS ABOUT THIS below)` and some examples in the examples section?