{"thread":{"id":"29322","subject":"[PATCH] gitignore: warn about pointless syntax","startedAt":"2012-01-09T15:40:46Z","lastAt":"2012-01-10T18:51:05Z","messageCount":11,"participants":["Jan Engelhardt","Jeff King","Junio C Hamano","Thomas Rast"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"182166","messageId":"1326123647-18352-1-git-send-email-jengelh@medozas.de","threadId":"29322","inReplyTo":null,"subject":"gitignore warn about ** submission","fromName":"Jan Engelhardt","fromEmail":"jengelh@medozas.de","sentAt":"2012-01-09T15:40:46Z","receivedAt":"2012-01-09T15:40:46Z","isPatch":false,"sender":{"key":"jengelh@medozas.de","avatar":null},"body":"\nThe following changes since commit eac2d83247ea0a265d923518c26873bb12c33778:\n\n Git 1.7.9-rc0 (2012-01-06 12:51:09 -0800)\n\nare available in the git repository at:\n git://dev.medozas.de/git master\n\nJan Engelhardt (1):\n gitignore: warn about pointless syntax\n\n dir.c | 10 ++++++++++\n 1 files changed, 10 insertions(+), 0 deletions(-)\n"},{"id":"182165","messageId":"1326123647-18352-2-git-send-email-jengelh@medozas.de","threadId":"29322","inReplyTo":"1326123647-18352-1-git-send-email-jengelh@medozas.de","subject":"[PATCH] gitignore: warn about pointless syntax","fromName":"Jan Engelhardt","fromEmail":"jengelh@medozas.de","sentAt":"2012-01-09T15:40:47Z","receivedAt":"2012-01-09T15:40:47Z","isPatch":true,"sender":{"key":"jengelh@medozas.de","avatar":null},"body":"Add a warning to the gitignore parser if it sees \"**\". Git, using\nfnmatch, does not consider the double-asterisk anything special like\nrsync/zsh. Remind users of that, since too many seem to be Doing It\nWrong™.\n\nSigned-off-by: Jan Engelhardt \n---\n dir.c | 10 ++++++++++\n 1 files changed, 10 insertions(+), 0 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 0a78d00..60f65cb 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -376,6 +376,15 @@ void free_excludes(struct exclude_list *el)\n \tel->excludes = NULL;\n }\n \n+static inline void check_bogus_wildcard(const char *file, const char *p)\n+{\n+\tif (strstr(p, \"**\") == NULL)\n+\t\treturn;\n+\twarning(_(\"Pattern \\\"%s\\\" from file \\\"%s\\\": Double asterisk does not \"\n+\t\t\"have a special meaning and is interpreted just like a single \"\n+\t\t\"asterisk.\\n\"), file, p);\n+}\n+\n int add_excludes_from_file_to_list(const char *fname,\n \t\t\t\t const char *base,\n \t\t\t\t int baselen,\n@@ -427,6 +436,7 @@ int add_excludes_from_file_to_list(const char *fname,\n \t\tif (buf[i] == '\\n') {\n \t\t\tif (entry != buf + i && entry[0] != '#') {\n \t\t\t\tbuf[i - (i && buf[i-1] == '\\r')] = 0;\n+\t\t\t\tcheck_bogus_wildcard(fname, entry);\n \t\t\t\tadd_exclude(entry, base, baselen, which);\n \t\t\t}\n \t\t\tentry = buf + i + 1;\n-- \n1.7.7\n"},{"id":"182167","messageId":"20120109162802.GA2374@sigill.intra.peff.net","threadId":"29322","inReplyTo":"1326123647-18352-2-git-send-email-jengelh@medozas.de","subject":"Re: [PATCH] gitignore: warn about pointless syntax","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-09T16:28:02Z","receivedAt":"2012-01-09T16:28:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 09, 2012 at 04:40:47PM +0100, Jan Engelhardt wrote:\n\n> +static inline void check_bogus_wildcard(const char *file, const char *p)\n> +{\n> +\tif (strstr(p, \"**\") == NULL)\n> +\t\treturn;\n> +\twarning(_(\"Pattern \\\"%s\\\" from file \\\"%s\\\": Double asterisk does not \"\n> +\t\t\"have a special meaning and is interpreted just like a single \"\n> +\t\t\"asterisk.\\n\"), file, p);\n\nWouldn't this also match the meaningful \"foo\\**\"?\n\n-Peff\n"},{"id":"182176","messageId":"7vhb04ek6e.fsf@alter.siamese.dyndns.org","threadId":"29322","inReplyTo":"20120109162802.GA2374@sigill.intra.peff.net","subject":"Re: [PATCH] gitignore: warn about pointless syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-09T19:43:21Z","receivedAt":"2012-01-09T19:43:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jan 09, 2012 at 04:40:47PM +0100, Jan Engelhardt wrote:\n>\n>> +static inline void check_bogus_wildcard(const char *file, const char *p)\n>> +{\n>> +\tif (strstr(p, \"**\") == NULL)\n>> +\t\treturn;\n>> +\twarning(_(\"Pattern \\\"%s\\\" from file \\\"%s\\\": Double asterisk does not \"\n>> +\t\t\"have a special meaning and is interpreted just like a single \"\n>> +\t\t\"asterisk.\\n\"), file, p);\n>\n> Wouldn't this also match the meaningful \"foo\\**\"?\n\nYes.\n\nBut trying to catch that false positive by checking one before \"**\"\nagainst a backslash is not a way to do so as it will then turn \"foo\\\\**\"\ninto a false negative, and you would end up reimplementing fnmatch if you\nreally want to avoid false positives nor negatives. At that point, you may\nbe better off implementing git_fnmatch() instead that understands the\ndouble-asterisk that works as some people may expect it to work ;-).\n"},{"id":"182186","messageId":"20120109223358.GA9902@sigill.intra.peff.net","threadId":"29322","inReplyTo":"7vhb04ek6e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] gitignore: warn about pointless syntax","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-09T22:33:59Z","receivedAt":"2012-01-09T22:33:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 09, 2012 at 11:43:21AM -0800, Junio C Hamano wrote:\n\n> >> +static inline void check_bogus_wildcard(const char *file, const char *p)\n> >> +{\n> >> +\tif (strstr(p, \"**\") == NULL)\n> >> +\t\treturn;\n> >> +\twarning(_(\"Pattern \\\"%s\\\" from file \\\"%s\\\": Double asterisk does not \"\n> >> +\t\t\"have a special meaning and is interpreted just like a single \"\n> >> +\t\t\"asterisk.\\n\"), file, p);\n> >\n> > Wouldn't this also match the meaningful \"foo\\**\"?\n> \n> Yes.\n> \n> But trying to catch that false positive by checking one before \"**\"\n> against a backslash is not a way to do so as it will then turn \"foo\\\\**\"\n> into a false negative, and you would end up reimplementing fnmatch if you\n> really want to avoid false positives nor negatives. At that point, you may\n> be better off implementing git_fnmatch() instead that understands the\n> double-asterisk that works as some people may expect it to work ;-).\n\nYou only have to implement proper backslash decoding, so I think it is\nnot as hard as reimplementing fnmatch:\n\n enum { NORMAL, QUOTED, WILDCARD } context = NORMAL;\n for (i = 0; p[i]; i++) {\n if (context == QUOTED)\n context = NORMAL;\n else if (p[i] == '\\\\')\n context = QUOTED;\n else if (p[i] == '*') {\n if (context == WILDCARD) {\n warning(...);\n return;\n }\n context = WILDCARD;\n }\n else\n context = NORMAL;\n }\n\nThat being said, if this is such a commonly-requested feature that we\nneed to be detecting and complaining about its absence, I would be much\nmore in favor of simply implementing it. Surely fnmatch is not that hard\nto write, or we could lift code from glibc or even rsync.\n\nWhich perhaps was what you are getting at, but I am happy to say it more\nexplicitly. :)\n\n-Peff\n"},{"id":"182209","messageId":"alpine.LNX.2.01.1201100639340.11534@frira.zrqbmnf.qr","threadId":"29322","inReplyTo":"20120109223358.GA9902@sigill.intra.peff.net","subject":"Re: [PATCH] gitignore: warn about pointless syntax","fromName":"Jan Engelhardt","fromEmail":"jengelh@medozas.de","sentAt":"2012-01-10T05:42:11Z","receivedAt":"2012-01-10T05:42:11Z","isPatch":true,"sender":{"key":"jengelh@medozas.de","avatar":null},"body":"\nOn Monday 2012-01-09 23:33, Jeff King wrote:\n>On Mon, Jan 09, 2012 at 11:43:21AM -0800, Junio C Hamano wrote:\n>\n>>>>+static inline void check_bogus_wildcard(const char *file, const char *p)\n>>>>+{\n>>>>+\tif (strstr(p, \"**\") == NULL)\n>>>>+\t\treturn;\n>>>>+\twarning(_(\"Pattern \\\"%s\\\" from file \\\"%s\\\": Double asterisk does not \"\n>>>>+\t\t\"have a special meaning and is interpreted just like a single \"\n>>>>+\t\t\"asterisk.\\n\"), file, p);\n>\n>You only have to implement proper backslash decoding, so I think it is\n>not as hard as reimplementing fnmatch:\n>[...]\n>\n>That being said, if this is such a commonly-requested feature\n\nWas it actually requested, or did you mean \"commonly attempted use\"?\n\nAs I see it, foo/**/*.o for example is equal to placing \"*.o\" in\nfoo/.gitignore, so the feature is already implemented, just not\nthrough the syntax people falsely assume it is. And that is the\nreason for wanting to output a warning. If it was me, I'd even make\nit use error(), because that is the only way to educate people (and\nit works), but alas, some on the list might consider that too harsh.\n"},{"id":"182213","messageId":"7vd3asayfx.fsf@alter.siamese.dyndns.org","threadId":"29322","inReplyTo":"alpine.LNX.2.01.1201100639340.11534@frira.zrqbmnf.qr","subject":"Re: [PATCH] gitignore: warn about pointless syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-10T06:01:06Z","receivedAt":"2012-01-10T06:01:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jan Engelhardt writes:\n\n> On Monday 2012-01-09 23:33, Jeff King wrote:\n>>On Mon, Jan 09, 2012 at 11:43:21AM -0800, Junio C Hamano wrote:\n>>\n>>>>>+static inline void check_bogus_wildcard(const char *file, const char *p)\n>>>>>+{\n>>>>>+\tif (strstr(p, \"**\") == NULL)\n>>>>>+\t\treturn;\n>>>>>+\twarning(_(\"Pattern \\\"%s\\\" from file \\\"%s\\\": Double asterisk does not \"\n>>>>>+\t\t\"have a special meaning and is interpreted just like a single \"\n>>>>>+\t\t\"asterisk.\\n\"), file, p);\n>>\n>>You only have to implement proper backslash decoding, so I think it is\n>>not as hard as reimplementing fnmatch:\n>>[...]\n>>\n>>That being said, if this is such a commonly-requested feature\n>\n> Was it actually requested, or did you mean \"commonly attempted use\"?\n>\n> As I see it, foo/**/*.o for example is equal to placing \"*.o\" in\n> foo/.gitignore, so the feature is already implemented, just not\n> through the syntax people falsely assume it is.\n\nYou can either adjust the people, i.e. teach that their \"false\" assumption\nis wrong and the feature they expect is available but not in a way that\nthey expect.\n\nOr you can adjust the tool to match their expectation.\n\nThe point that Peff correctly read between my lines is that in real life,\npeople are harder to train than tools and often the latter is a better\napproach, especially if it does not amount to too much more work than\ndoing the former.\n"},{"id":"182214","messageId":"alpine.LNX.2.01.1201100744340.17336@frira.zrqbmnf.qr","threadId":"29322","inReplyTo":"7vd3asayfx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] gitignore: warn about pointless syntax","fromName":"Jan Engelhardt","fromEmail":"jengelh@medozas.de","sentAt":"2012-01-10T07:01:16Z","receivedAt":"2012-01-10T07:01:16Z","isPatch":true,"sender":{"key":"jengelh@medozas.de","avatar":null},"body":"On Tuesday 2012-01-10 07:01, Junio C Hamano wrote:\n\n>>>That being said, if this is such a commonly-requested feature\n>>\n>>Was it actually requested, or did you mean \"commonly attempted use\"?\n>>As I see it, foo/**/*.o for example is equal to placing \"*.o\" in\n>>foo/.gitignore, so the feature is already implemented, just not\n>>through the syntax people falsely assume it is.\n>\n>You can either adjust the people, i.e. teach that their \"false\" assumption\n>is wrong and the feature they expect is available but not in a way that\n>they expect. Or you can adjust the tool to match their expectation.\n>[...]in real life, people are harder to train than tools[...]\n\nThough, having one more way to do things leads to a certain mess at a \nlater point, if such mess is not already present. I am thinking here of \nthe precedent iptables option parser set by removing support for \nexclamation marks in odd positions, as it was redundant (\"more than 1 \nway\"), was only supported by ~45% of all options and had to be \nexplicitly invoked at every callsite - so in fact was harder on users \nthan git would be for **. There were a few mails by people who could not \nseem to read error messages, but overall, within 6-9 months, everything \nwas quiet again. So, that's the empiric result of what teaching-the-tool \nwould do.\n"},{"id":"182215","messageId":"alpine.LNX.2.01.1201100801330.28967@frira.zrqbmnf.qr","threadId":"29322","inReplyTo":"alpine.LNX.2.01.1201100744340.17336@frira.zrqbmnf.qr","subject":"Re: [PATCH] gitignore: warn about pointless syntax","fromName":"Jan Engelhardt","fromEmail":"jengelh@medozas.de","sentAt":"2012-01-10T07:02:53Z","receivedAt":"2012-01-10T07:02:53Z","isPatch":true,"sender":{"key":"jengelh@medozas.de","avatar":null},"body":"On Tuesday 2012-01-10 08:01, Jan Engelhardt wrote:\n>>\n>>You can either adjust the people, i.e. teach that their \"false\" assumption\n>>is wrong and the feature they expect is available but not in a way that\n>>they expect. Or you can adjust the tool to match their expectation.\n>>[...]in real life, people are harder to train than tools[...]\n>\n>Though, having one more way to do things leads to a certain mess at a \n>later point, if such mess is not already present. I am thinking here of \n>the precedent iptables option parser set by removing support for \n>exclamation marks in odd positions, as it was redundant (\"more than 1 \n>way\"), was only supported by ~45% of all options and had to be \n>explicitly invoked at every callsite - so in fact was harder on users \n>than git would be for **. There were a few mails by people who could not \n>seem to read error messages, but overall, within 6-9 months, everything \n>was quiet again. So, that's the empiric result of what teaching-the-tool \n>would do.\n\n~ teaching-the-user would entail - it's factually problemfree.\n"},{"id":"182219","messageId":"87d3arnb6t.fsf@thomas.inf.ethz.ch","threadId":"29322","inReplyTo":"alpine.LNX.2.01.1201100639340.11534@frira.zrqbmnf.qr","subject":"Re: [PATCH] gitignore: warn about pointless syntax","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-01-10T09:44:58Z","receivedAt":"2012-01-10T09:44:58Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jan Engelhardt writes:\n>\n> As I see it, foo/**/*.o for example is equal to placing \"*.o\" in\n> foo/.gitignore, so the feature is already implemented, just not\n> through the syntax people falsely assume it is. And that is the\n> reason for wanting to output a warning. If it was me, I'd even make\n> it use error(),\n\nNo, please don't even think about it. Having an error() there would\nmean that git would be essentially useless in repositories that have\nthis mistake, and the user may not be in a position to fix the\nrepository!\n\n(He could drag along a local change to that effect, which is highly\nannoying if the repository is otherwise read-only for him.)\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"182266","messageId":"20120110185105.GD15273@sigill.intra.peff.net","threadId":"29322","inReplyTo":"alpine.LNX.2.01.1201100639340.11534@frira.zrqbmnf.qr","subject":"Re: [PATCH] gitignore: warn about pointless syntax","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-10T18:51:05Z","receivedAt":"2012-01-10T18:51:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 10, 2012 at 06:42:11AM +0100, Jan Engelhardt wrote:\n\n> >You only have to implement proper backslash decoding, so I think it is\n> >not as hard as reimplementing fnmatch:\n> >[...]\n> >\n> >That being said, if this is such a commonly-requested feature\n> \n> Was it actually requested, or did you mean \"commonly attempted use\"?\n\nBoth. I meant in my sentence \"if this is such a big problem that we need\nto add a check for it, then surely it is something people would like to\nbe using\". But if you peruse the list archives, you can find several\npeople mentioning that they would like it.\n\n> As I see it, foo/**/*.o for example is equal to placing \"*.o\" in\n> foo/.gitignore, so the feature is already implemented, just not\n> through the syntax people falsely assume it is. And that is the\n> reason for wanting to output a warning. If it was me, I'd even make\n> it use error(), because that is the only way to educate people (and\n> it works), but alas, some on the list might consider that too harsh.\n\nThose features aren't exactly equivalent. Off the top of my head, I can\nthink of a few reasons to prefer using the top-level:\n\n - you simply prefer it because it keeps your rules grouped in a more\n logical way\n\n - you don't control the sub-tree (e.g., it is brought in by sub-tree\n merge, or you have an agreement with other devs not to touch things\n in it. Also, I don't think .gitignores cross submodule boundaries\n currently, but it is something that could happen eventually).\n\n - you can write more complex rules with \"**\" that would otherwise\n necessitate writing multiple rules split across directories\n\nDon't get me wrong. I am not a huge proponent of \"**\", and I could\nreally care less if we implement it or not, and we have survived many\nyears without it. It just seems to me that if it's worth warning about,\nit's worth implementing.\n\n-Peff\n"}]}