[patch] gitignore: warn about pointless syntax

[patch] gitignore: warn about pointless syntax

[Jan Engelhardt]

parent eac2d83247ea0a265d923518c26873bb12c33778 (v1.7.9-rc0)
commit b629bde461aeb178b257ab7e0f6c180f69f98cb0
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Mon Jan 9 12:30:07 2012 +0100

gitignore: warn about pointless syntax

Add a warning to the gitignore parser if it sees "**". Git, using
fnmatch, does not consider the double-asterisk anything special like
rsync/zsh. Remind users of that, since too many seem to be Doing It
Wrong™.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 dir.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/dir.c b/dir.c
index 0a78d00..60f65cb 100644
--- a/dir.c
+++ b/dir.c
@@ -376,6 +376,15 @@ void free_excludes(struct exclude_list *el)
 	el->excludes = NULL;
 }
 
+static inline void check_bogus_wildcard(const char *file, const char *p)
+{
+	if (strstr(p, "**") == NULL)
+		return;
+	warning(_("Pattern \"%s\" from file \"%s\": Double asterisk does not "
+		"have a special meaning and is interpreted just like a single "
+		"asterisk.\n"), file, p);
+}
+
 int add_excludes_from_file_to_list(const char *fname,
 				   const char *base,
 				   int baselen,
@@ -427,6 +436,7 @@ int add_excludes_from_file_to_list(const char *fname,
 		if (buf[i] == '\n') {
 			if (entry != buf + i && entry[0] != '#') {
 				buf[i - (i && buf[i-1] == '\r')] = 0;
+				check_bogus_wildcard(fname, entry);
 				add_exclude(entry, base, baselen, which);
 			}
 			entry = buf + i + 1;
-- 
# Created with git-export-patch

Re: [patch] gitignore: warn about pointless syntax

[Thomas Rast]

Jan Engelhardt <jengelh@medozas.de> writes:

> parent eac2d83247ea0a265d923518c26873bb12c33778 (v1.7.9-rc0)
> commit b629bde461aeb178b257ab7e0f6c180f69f98cb0
> Author: Jan Engelhardt <jengelh@medozas.de>
> Date:   Mon Jan 9 12:30:07 2012 +0100
>
> gitignore: warn about pointless syntax
[...]
> -- 
> # Created with git-export-patch

Are you the author of this tool?  The format is bogus in so far as it
causes git-am to insert the above lines into the git commit message, as
in:

    $ git am -3 < gitignore-patch.mbox
    Applying: gitignore: warn about pointless syntax
    $ git show
    commit 59bea4d9a1de3b3b9c0139de4298ffd9d9431457
    Author: Jan Engelhardt <jengelh@medozas.de>
    Date:   Mon Jan 9 12:34:12 2012 +0100

        gitignore: warn about pointless syntax
        
        parent eac2d83247ea0a265d923518c26873bb12c33778 (v1.7.9-rc0)
        commit b629bde461aeb178b257ab7e0f6c180f69f98cb0
        Author: Jan Engelhardt <jengelh@medozas.de>
        Date:   Mon Jan 9 12:30:07 2012 +0100
        
        gitignore: warn about pointless syntax
    [...]

So using this tool over format-patch is just a pointless cause of manual
fixup work.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

gitignore warn about ** submission

[Jan Engelhardt]

The following changes since commit eac2d83247ea0a265d923518c26873bb12c33778:

  Git 1.7.9-rc0 (2012-01-06 12:51:09 -0800)

are available in the git repository at:
  git://dev.medozas.de/git master

Jan Engelhardt (1):
      gitignore: warn about pointless syntax

 dir.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

[PATCH] gitignore: warn about pointless syntax

[Jan Engelhardt]

Add a warning to the gitignore parser if it sees "**". Git, using
fnmatch, does not consider the double-asterisk anything special like
rsync/zsh. Remind users of that, since too many seem to be Doing It
Wrong™.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 dir.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/dir.c b/dir.c
index 0a78d00..60f65cb 100644
--- a/dir.c
+++ b/dir.c
@@ -376,6 +376,15 @@ void free_excludes(struct exclude_list *el)
 	el->excludes = NULL;
 }
 
+static inline void check_bogus_wildcard(const char *file, const char *p)
+{
+	if (strstr(p, "**") == NULL)
+		return;
+	warning(_("Pattern \"%s\" from file \"%s\": Double asterisk does not "
+		"have a special meaning and is interpreted just like a single "
+		"asterisk.\n"), file, p);
+}
+
 int add_excludes_from_file_to_list(const char *fname,
 				   const char *base,
 				   int baselen,
@@ -427,6 +436,7 @@ int add_excludes_from_file_to_list(const char *fname,
 		if (buf[i] == '\n') {
 			if (entry != buf + i && entry[0] != '#') {
 				buf[i - (i && buf[i-1] == '\r')] = 0;
+				check_bogus_wildcard(fname, entry);
 				add_exclude(entry, base, baselen, which);
 			}
 			entry = buf + i + 1;
-- 
1.7.7

Re: [PATCH] gitignore: warn about pointless syntax

[Jeff King]

On Mon, Jan 09, 2012 at 04:40:47PM +0100, Jan Engelhardt wrote:

> +static inline void check_bogus_wildcard(const char *file, const char *p)
> +{
> +	if (strstr(p, "**") == NULL)
> +		return;
> +	warning(_("Pattern \"%s\" from file \"%s\": Double asterisk does not "
> +		"have a special meaning and is interpreted just like a single "
> +		"asterisk.\n"), file, p);

Wouldn't this also match the meaningful "foo\**"?

-Peff

Re: [PATCH] gitignore: warn about pointless syntax

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jan 09, 2012 at 04:40:47PM +0100, Jan Engelhardt wrote:
>
>> +static inline void check_bogus_wildcard(const char *file, const char *p)
>> +{
>> +	if (strstr(p, "**") == NULL)
>> +		return;
>> +	warning(_("Pattern \"%s\" from file \"%s\": Double asterisk does not "
>> +		"have a special meaning and is interpreted just like a single "
>> +		"asterisk.\n"), file, p);
>
> Wouldn't this also match the meaningful "foo\**"?

Yes.

But trying to catch that false positive by checking one before "**"
against a backslash is not a way to do so as it will then turn "foo\\**"
into a false negative, and you would end up reimplementing fnmatch if you
really want to avoid false positives nor negatives. At that point, you may
be better off implementing git_fnmatch() instead that understands the
double-asterisk that works as some people may expect it to work ;-).

Re: [PATCH] gitignore: warn about pointless syntax

[Jeff King]

On Mon, Jan 09, 2012 at 11:43:21AM -0800, Junio C Hamano wrote:

> >> +static inline void check_bogus_wildcard(const char *file, const char *p)
> >> +{
> >> +	if (strstr(p, "**") == NULL)
> >> +		return;
> >> +	warning(_("Pattern \"%s\" from file \"%s\": Double asterisk does not "
> >> +		"have a special meaning and is interpreted just like a single "
> >> +		"asterisk.\n"), file, p);
> >
> > Wouldn't this also match the meaningful "foo\**"?
> 
> Yes.
> 
> But trying to catch that false positive by checking one before "**"
> against a backslash is not a way to do so as it will then turn "foo\\**"
> into a false negative, and you would end up reimplementing fnmatch if you
> really want to avoid false positives nor negatives. At that point, you may
> be better off implementing git_fnmatch() instead that understands the
> double-asterisk that works as some people may expect it to work ;-).

You only have to implement proper backslash decoding, so I think it is
not as hard as reimplementing fnmatch:

  enum { NORMAL, QUOTED, WILDCARD } context = NORMAL;
  for (i = 0; p[i]; i++) {
          if (context == QUOTED)
                  context = NORMAL;
          else if (p[i] == '\\')
                  context = QUOTED;
          else if (p[i] == '*') {
                  if (context == WILDCARD) {
                        warning(...);
                        return;
                  }
                  context = WILDCARD;
          }
          else
                  context = NORMAL;
  }

That being said, if this is such a commonly-requested feature that we
need to be detecting and complaining about its absence, I would be much
more in favor of simply implementing it. Surely fnmatch is not that hard
to write, or we could lift code from glibc or even rsync.

Which perhaps was what you are getting at, but I am happy to say it more
explicitly. :)

-Peff

Re: [PATCH] gitignore: warn about pointless syntax

[Jan Engelhardt]

On Monday 2012-01-09 23:33, Jeff King wrote:
>On Mon, Jan 09, 2012 at 11:43:21AM -0800, Junio C Hamano wrote:
>
>>>>+static inline void check_bogus_wildcard(const char *file, const char *p)
>>>>+{
>>>>+	if (strstr(p, "**") == NULL)
>>>>+		return;
>>>>+	warning(_("Pattern \"%s\" from file \"%s\": Double asterisk does not "
>>>>+		"have a special meaning and is interpreted just like a single "
>>>>+		"asterisk.\n"), file, p);
>
>You only have to implement proper backslash decoding, so I think it is
>not as hard as reimplementing fnmatch:
>[...]
>
>That being said, if this is such a commonly-requested feature

Was it actually requested, or did you mean "commonly attempted use"?

As I see it, foo/**/*.o for example is equal to placing "*.o" in
foo/.gitignore, so the feature is already implemented, just not
through the syntax people falsely assume it is. And that is the
reason for wanting to output a warning. If it was me, I'd even make
it use error(), because that is the only way to educate people (and
it works), but alas, some on the list might consider that too harsh.

Re: [PATCH] gitignore: warn about pointless syntax

[Junio C Hamano]

Jan Engelhardt <jengelh@medozas.de> writes:

> On Monday 2012-01-09 23:33, Jeff King wrote:
>>On Mon, Jan 09, 2012 at 11:43:21AM -0800, Junio C Hamano wrote:
>>
>>>>>+static inline void check_bogus_wildcard(const char *file, const char *p)
>>>>>+{
>>>>>+	if (strstr(p, "**") == NULL)
>>>>>+		return;
>>>>>+	warning(_("Pattern \"%s\" from file \"%s\": Double asterisk does not "
>>>>>+		"have a special meaning and is interpreted just like a single "
>>>>>+		"asterisk.\n"), file, p);
>>
>>You only have to implement proper backslash decoding, so I think it is
>>not as hard as reimplementing fnmatch:
>>[...]
>>
>>That being said, if this is such a commonly-requested feature
>
> Was it actually requested, or did you mean "commonly attempted use"?
>
> As I see it, foo/**/*.o for example is equal to placing "*.o" in
> foo/.gitignore, so the feature is already implemented, just not
> through the syntax people falsely assume it is.

You can either adjust the people, i.e. teach that their "false" assumption
is wrong and the feature they expect is available but not in a way that
they expect.

Or you can adjust the tool to match their expectation.

The point that Peff correctly read between my lines is that in real life,
people are harder to train than tools and often the latter is a better
approach, especially if it does not amount to too much more work than
doing the former.

Re: [PATCH] gitignore: warn about pointless syntax

[Jan Engelhardt]

On Tuesday 2012-01-10 07:01, Junio C Hamano wrote:

>>>That being said, if this is such a commonly-requested feature
>>
>>Was it actually requested, or did you mean "commonly attempted use"?
>>As I see it, foo/**/*.o for example is equal to placing "*.o" in
>>foo/.gitignore, so the feature is already implemented, just not
>>through the syntax people falsely assume it is.
>
>You can either adjust the people, i.e. teach that their "false" assumption
>is wrong and the feature they expect is available but not in a way that
>they expect. Or you can adjust the tool to match their expectation.
>[...]in real life, people are harder to train than tools[...]

Though, having one more way to do things leads to a certain mess at a 
later point, if such mess is not already present. I am thinking here of 
the precedent iptables option parser set by removing support for 
exclamation marks in odd positions, as it was redundant ("more than 1 
way"), was only supported by ~45% of all options and had to be 
explicitly invoked at every callsite - so in fact was harder on users 
than git would be for **. There were a few mails by people who could not 
seem to read error messages, but overall, within 6-9 months, everything 
was quiet again. So, that's the empiric result of what teaching-the-tool 
would do.

Re: [PATCH] gitignore: warn about pointless syntax

[Jan Engelhardt]

On Tuesday 2012-01-10 08:01, Jan Engelhardt wrote:
>>
>>You can either adjust the people, i.e. teach that their "false" assumption
>>is wrong and the feature they expect is available but not in a way that
>>they expect. Or you can adjust the tool to match their expectation.
>>[...]in real life, people are harder to train than tools[...]
>
>Though, having one more way to do things leads to a certain mess at a 
>later point, if such mess is not already present. I am thinking here of 
>the precedent iptables option parser set by removing support for 
>exclamation marks in odd positions, as it was redundant ("more than 1 
>way"), was only supported by ~45% of all options and had to be 
>explicitly invoked at every callsite - so in fact was harder on users 
>than git would be for **. There were a few mails by people who could not 
>seem to read error messages, but overall, within 6-9 months, everything 
>was quiet again. So, that's the empiric result of what teaching-the-tool 
>would do.

~ teaching-the-user would entail - it's factually problemfree.

Re: [PATCH] gitignore: warn about pointless syntax

[Thomas Rast]

Jan Engelhardt <jengelh@medozas.de> writes:
>
> As I see it, foo/**/*.o for example is equal to placing "*.o" in
> foo/.gitignore, so the feature is already implemented, just not
> through the syntax people falsely assume it is. And that is the
> reason for wanting to output a warning. If it was me, I'd even make
> it use error(),

No, please don't even think about it.  Having an error() there would
mean that git would be essentially useless in repositories that have
this mistake, and the user may not be in a position to fix the
repository!

(He could drag along a local change to that effect, which is highly
annoying if the repository is otherwise read-only for him.)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] gitignore: warn about pointless syntax

[Jeff King]

On Tue, Jan 10, 2012 at 06:42:11AM +0100, Jan Engelhardt wrote:

> >You only have to implement proper backslash decoding, so I think it is
> >not as hard as reimplementing fnmatch:
> >[...]
> >
> >That being said, if this is such a commonly-requested feature
> 
> Was it actually requested, or did you mean "commonly attempted use"?

Both. I meant in my sentence "if this is such a big problem that we need
to add a check for it, then surely it is something people would like to
be using". But if you peruse the list archives, you can find several
people mentioning that they would like it.

> As I see it, foo/**/*.o for example is equal to placing "*.o" in
> foo/.gitignore, so the feature is already implemented, just not
> through the syntax people falsely assume it is. And that is the
> reason for wanting to output a warning. If it was me, I'd even make
> it use error(), because that is the only way to educate people (and
> it works), but alas, some on the list might consider that too harsh.

Those features aren't exactly equivalent. Off the top of my head, I can
think of a few reasons to prefer using the top-level:

  - you simply prefer it because it keeps your rules grouped in a more
    logical way

  - you don't control the sub-tree (e.g., it is brought in by sub-tree
    merge, or you have an agreement with other devs not to touch things
    in it. Also, I don't think .gitignores cross submodule boundaries
    currently, but it is something that could happen eventually).

  - you can write more complex rules with "**" that would otherwise
    necessitate writing multiple rules split across directories

Don't get me wrong. I am not a huge proponent of "**", and I could
really care less if we implement it or not, and we have survived many
years without it. It just seems to me that if it's worth warning about,
it's worth implementing.

-Peff