patchgitignore: warn about pointless syntax

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(-)

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(-)

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

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

-Peff

Jeff King <peff@peff.net> writes:

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

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

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

On Monday 2012-01-09 23:33, Jeff King wrote:

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.

Jan Engelhardt <jengelh@medozas.de> writes:

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.

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

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.

On Tuesday 2012-01-10 08:01, Jan Engelhardt wrote:

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

Jan Engelhardt <jengelh@medozas.de> writes:

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

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

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.

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