{"thread":{"id":"33049","subject":"[PATCH] Make !pattern in .gitattributes non-fatal","startedAt":"2013-03-01T20:06:17Z","lastAt":"2013-03-03T06:30:24Z","messageCount":8,"participants":["Thomas Rast","Junio C Hamano","Duy Nguyen","Nguyễn Thái Ngọc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"210472","messageId":"a1679159dbe67c34675bbe016e131260310db189.1362168366.git.trast@student.ethz.ch","threadId":"33049","inReplyTo":null,"subject":"[PATCH] Make !pattern in .gitattributes non-fatal","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2013-03-01T20:06:17Z","receivedAt":"2013-03-01T20:06:17Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Before 82dce99 (attr: more matching optimizations from .gitignore,\n2012-10-15), .gitattributes did not have any special treatment of a\nleading '!'. The docs, however, always said\n\n The rules how the pattern matches paths are the same as in\n `.gitignore` files; see linkgit:gitignore[5].\n\nBy those rules, leading '!' means pattern negation. So 82dce99\ncorrectly determined that this kind of line makes no sense and should\nbe disallowed.\n\nHowever, users who actually had a rule for files starting with a '!'\nare in a bad position: before 82dce99 '!' matched that literal\ncharacter, so it is conceivable that users have .gitattributes with\nsuch lines in them. After 82dce99 the unescaped version was\ndisallowed in such a way that git outright refuses to run(!) most\ncommands in the presence of such a .gitattributes. It therefore\nbecomes very hard to fix, let alone work with, such repositories.\n\nLet's at least allow the users to fix their repos: change the fatal\nerror into a warning.\n\nReported-by: mathstuf@gmail.com\nSigned-off-by: Thomas Rast \n---\n attr.c | 8 +++++---\n t/t0003-attributes.sh | 3 ++-\n 2 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 4657cc2..e2f9377 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -255,9 +255,11 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\t\t\t &res->u.pat.patternlen,\n \t\t\t\t &res->u.pat.flags,\n \t\t\t\t &res->u.pat.nowildcardlen);\n-\t\tif (res->u.pat.flags & EXC_FLAG_NEGATIVE)\n-\t\t\tdie(_(\"Negative patterns are forbidden in git attributes\\n\"\n-\t\t\t \"Use '\\\\!' for literal leading exclamation.\"));\n+\t\tif (res->u.pat.flags & EXC_FLAG_NEGATIVE) {\n+\t\t\twarning(_(\"Negative patterns are ignored in git attributes\\n\"\n+\t\t\t\t \"Use '\\\\!' for literal leading exclamation.\"));\n+\t\t\treturn NULL;\n+\t\t}\n \t}\n \tres->is_macro = is_macro;\n \tres->num_attr = num_attr;\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 43b2513..0b98b6f 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -198,7 +198,8 @@ test_expect_success 'root subdir attribute test' '\n \n test_expect_success 'negative patterns' '\n \techo \"!f test=bar\" >.gitattributes &&\n-\ttest_must_fail git check-attr test -- f\n+\tgit check-attr test -- '\"'\"'!f'\"'\"' 2>errors &&\n+\ttest_i18ngrep \"Negative patterns are ignored\" errors\n '\n \n test_expect_success 'patterns starting with exclamation' '\n-- \n1.8.2.rc1.392.gf57d6b8.dirty\n"},{"id":"210473","messageId":"7vk3pqg973.fsf@alter.siamese.dyndns.org","threadId":"33049","inReplyTo":"a1679159dbe67c34675bbe016e131260310db189.1362168366.git.trast@student.ethz.ch","subject":"Re: [PATCH] Make !pattern in .gitattributes non-fatal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-01T20:24:16Z","receivedAt":"2013-03-01T20:24:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Makes sense. Duy?\n"},{"id":"210474","messageId":"7vfw0eg90e.fsf@alter.siamese.dyndns.org","threadId":"33049","inReplyTo":"a1679159dbe67c34675bbe016e131260310db189.1362168366.git.trast@student.ethz.ch","subject":"Re: [PATCH] Make !pattern in .gitattributes non-fatal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-01T20:28:17Z","receivedAt":"2013-03-01T20:28:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Before 82dce99 (attr: more matching optimizations from .gitignore,\n> 2012-10-15), .gitattributes did not have any special treatment of a\n> leading '!'. The docs, however, always said\n>\n> The rules how the pattern matches paths are the same as in\n> `.gitignore` files; see linkgit:gitignore[5].\n>\n> By those rules, leading '!' means pattern negation. So 82dce99\n> correctly determined that this kind of line makes no sense and should\n> be disallowed.\n>\n> However, users who actually had a rule for files starting with a '!'\n> are in a bad position: before 82dce99 '!' matched that literal\n> character, so it is conceivable that users have .gitattributes with\n> such lines in them. After 82dce99 the unescaped version was\n> disallowed in such a way that git outright refuses to run(!) most\n> commands in the presence of such a .gitattributes. It therefore\n> becomes very hard to fix, let alone work with, such repositories.\n\nFixing the working tree is easy, but when we read from a history\nthat already records such an entry in an attribute file, it would\nbecome somewhat cumbersome. I wouldn't use \"very hard to fix\" to\ndescribe such a case.\n\nBut the demotion to warning does make sense; let's do that in\nv1.8.1.5.\n"},{"id":"210475","messageId":"87mwumx36m.fsf@pctrast.inf.ethz.ch","threadId":"33049","inReplyTo":"7vfw0eg90e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Make !pattern in .gitattributes non-fatal","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2013-03-01T20:42:09Z","receivedAt":"2013-03-01T20:42:09Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano writes:\n\n> Thomas Rast writes:\n>\n>> Before 82dce99 (attr: more matching optimizations from .gitignore,\n>> 2012-10-15), .gitattributes did not have any special treatment of a\n>> leading '!'. The docs, however, always said\n>>\n>> The rules how the pattern matches paths are the same as in\n>> `.gitignore` files; see linkgit:gitignore[5].\n>>\n>> By those rules, leading '!' means pattern negation. So 82dce99\n>> correctly determined that this kind of line makes no sense and should\n>> be disallowed.\n>>\n>> However, users who actually had a rule for files starting with a '!'\n>> are in a bad position: before 82dce99 '!' matched that literal\n>> character, so it is conceivable that users have .gitattributes with\n>> such lines in them. After 82dce99 the unescaped version was\n>> disallowed in such a way that git outright refuses to run(!) most\n>> commands in the presence of such a .gitattributes. It therefore\n>> becomes very hard to fix, let alone work with, such repositories.\n>\n> Fixing the working tree is easy, but when we read from a history\n> that already records such an entry in an attribute file, it would\n> become somewhat cumbersome. I wouldn't use \"very hard to fix\" to\n> describe such a case.\n\nWell, I'm sorry if I hurt any feelings there, but...\n\n ~/tmp/badattr(master)$ git show bad:.gitattributes\n !bad text\n ~/tmp/badattr(master)$ ~/g/git-checkout bad # a git without my patch\n fatal: Negative patterns are forbidden in git attributes\n Use '\\!' for literal leading exclamation.\n ~/tmp/badattr(master)$ git status\n # On branch master\n nothing to commit (use -u to show untracked files)\n\nNotice how it remains on master. I suppose with enough knowledge of the\ninternals I could manage, but after seeing how hard it was to *build*\nsuch broken history with a git that dies, I don't really want to try.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"210478","messageId":"7vbob2g524.fsf@alter.siamese.dyndns.org","threadId":"33049","inReplyTo":"87mwumx36m.fsf@pctrast.inf.ethz.ch","subject":"Re: [PATCH] Make !pattern in .gitattributes non-fatal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-01T21:53:39Z","receivedAt":"2013-03-01T21:53:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Junio C Hamano writes:\n>\n>> Thomas Rast writes:\n>>\n>>> Before 82dce99 (attr: more matching optimizations from .gitignore,\n>>> 2012-10-15), .gitattributes did not have any special treatment of a\n>>> leading '!'. The docs, however, always said\n>>>\n>>> The rules how the pattern matches paths are the same as in\n>>> `.gitignore` files; see linkgit:gitignore[5].\n>>>\n>>> By those rules, leading '!' means pattern negation. So 82dce99\n>>> correctly determined that this kind of line makes no sense and should\n>>> be disallowed.\n>>>\n>>> However, users who actually had a rule for files starting with a '!'\n>>> are in a bad position: before 82dce99 '!' matched that literal\n>>> character, so it is conceivable that users have .gitattributes with\n>>> such lines in them. After 82dce99 the unescaped version was\n>>> disallowed in such a way that git outright refuses to run(!) most\n>>> commands in the presence of such a .gitattributes. It therefore\n>>> becomes very hard to fix, let alone work with, such repositories.\n>>\n>> Fixing the working tree is easy, but when we read from a history\n>> that already records such an entry in an attribute file, it would\n>> become somewhat cumbersome. I wouldn't use \"very hard to fix\" to\n>> describe such a case.\n>\n> Well, I'm sorry if I hurt any feelings there, but...\n>\n> ~/tmp/badattr(master)$ git show bad:.gitattributes\n> !bad text\n> ~/tmp/badattr(master)$ ~/g/git-checkout bad # a git without my patch\n> fatal: Negative patterns are forbidden in git attributes\n> Use '\\!' for literal leading exclamation.\n> ~/tmp/badattr(master)$ git status\n> # On branch master\n> nothing to commit (use -u to show untracked files)\n>\n> Notice how it remains on master. I suppose with enough knowledge of the\n> internals I could manage,...\n\nThat would have been nicer to have in the log message.\n\nIn any case, 1.8.1.5 will go out with this fix (this is v1.8.1\nregression IIUC) this afternoon. Thanks for a fix.\n"},{"id":"210501","messageId":"CACsJy8CO1i1_QpMbLvfys=wsLDPTK69_9JanQYq2D3-fboK_WA@mail.gmail.com","threadId":"33049","inReplyTo":"a1679159dbe67c34675bbe016e131260310db189.1362168366.git.trast@student.ethz.ch","subject":"Re: [PATCH] Make !pattern in .gitattributes non-fatal","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-02T03:50:14Z","receivedAt":"2013-03-02T03:50:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 2, 2013 at 3:06 AM, Thomas Rast wrote:\n> Before 82dce99 (attr: more matching optimizations from .gitignore,\n> 2012-10-15), .gitattributes did not have any special treatment of a\n> leading '!'. The docs, however, always said\n>\n> The rules how the pattern matches paths are the same as in\n> `.gitignore` files; see linkgit:gitignore[5].\n>\n> By those rules, leading '!' means pattern negation. So 82dce99\n> correctly determined that this kind of line makes no sense and should\n> be disallowed.\n>\n> However, users who actually had a rule for files starting with a '!'\n> are in a bad position: before 82dce99 '!' matched that literal\n> character, so it is conceivable that users have .gitattributes with\n> such lines in them. After 82dce99 the unescaped version was\n> disallowed in such a way that git outright refuses to run(!) most\n> commands in the presence of such a .gitattributes. It therefore\n> becomes very hard to fix, let alone work with, such repositories.\n>\n> Let's at least allow the users to fix their repos: change the fatal\n> error into a warning.\n\nI agree we should not break existing .gitattributes. So yes, die()ing\nhere sounds bad. But..\n\n> @@ -255,9 +255,11 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n> &res->u.pat.patternlen,\n> &res->u.pat.flags,\n> &res->u.pat.nowildcardlen);\n> - if (res->u.pat.flags & EXC_FLAG_NEGATIVE)\n> - die(_(\"Negative patterns are forbidden in git attributes\\n\"\n> - \"Use '\\\\!' for literal leading exclamation.\"));\n> + if (res->u.pat.flags & EXC_FLAG_NEGATIVE) {\n> + warning(_(\"Negative patterns are ignored in git attributes\\n\"\n> + \"Use '\\\\!' for literal leading exclamation.\"));\n> + return NULL;\n> + }\n\nThis \"return NULL;\" means we ignore \"!blah\" pattern, which is a\nregression, isn't it? Should we treat '!' as literal here?\n-- \nDuy\n"},{"id":"210502","messageId":"1362197913-313-1-git-send-email-pclouds@gmail.com","threadId":"33049","inReplyTo":"a1679159dbe67c34675bbe016e131260310db189.1362168366.git.trast@student.ethz.ch","subject":"[PATCH] attr: always treat the leading exclamation mark as literal","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-02T04:18:33Z","receivedAt":"2013-03-02T04:18:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"8b1bd02 (Make !pattern in .gitattributes non-fatal - 2013-03-01)\nraises the fact that even though '!' should have been a negative\npattern indicator, people have been misusing for plain '!' and old Git\nversions accept that. This makes the leading '!' a de facto\nliteral. We can't ever make it negative indicator again because we\ncan't distinguish the literal '!' in existing repos and the negative\n'!' in future repos.\n\nAcknowledge it in document and code that the leading '!' is forever\nliteral.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Perhaps this on top of Thomas' patch? I see no way else that we can\n change the meaning of '!' again if such patterns are already stored\n in repositories.\n\n Documentation/gitattributes.txt | 3 ++-\n attr.c | 8 ++------\n dir.c | 7 ++++---\n dir.h | 6 +++++-\n t/t0003-attributes.sh | 5 ++---\n 5 files changed, 15 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 2698f63..02bff9d 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -56,7 +56,8 @@ When more than one pattern matches the path, a later line\n overrides an earlier line. This overriding is done per\n attribute. The rules how the pattern matches paths are the\n same as in `.gitignore` files; see linkgit:gitignore[5].\n-Unlike `.gitignore`, negative patterns are forbidden.\n+Unlike `.gitignore`, the leading `!` is treated as a literal\n+exclamation, not an indicator of negative patterns.\n \n When deciding what attributes are assigned to a path, git\n consults `$GIT_DIR/info/attributes` file (which has the highest\ndiff --git a/attr.c b/attr.c\nindex d181d98..838ec21 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -254,12 +254,8 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\tparse_exclude_pattern(&res->u.pat.pattern,\n \t\t\t\t &res->u.pat.patternlen,\n \t\t\t\t &res->u.pat.flags,\n-\t\t\t\t &res->u.pat.nowildcardlen);\n-\t\tif (res->u.pat.flags & EXC_FLAG_NEGATIVE) {\n-\t\t\twarning(_(\"Negative patterns are ignored in git attributes\\n\"\n-\t\t\t\t \"Use '\\\\!' for literal leading exclamation.\"));\n-\t\t\treturn NULL;\n-\t\t}\n+\t\t\t\t &res->u.pat.nowildcardlen,\n+\t\t\t\t 1);\n \t}\n \tres->is_macro = is_macro;\n \tres->num_attr = num_attr;\ndiff --git a/dir.c b/dir.c\nindex a473ca2..609ab99 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -311,13 +311,14 @@ static int no_wildcard(const char *string)\n void parse_exclude_pattern(const char **pattern,\n \t\t\t int *patternlen,\n \t\t\t int *flags,\n-\t\t\t int *nowildcardlen)\n+\t\t\t int *nowildcardlen,\n+\t\t\t int gitattributes)\n {\n \tconst char *p = *pattern;\n \tsize_t i, len;\n \n \t*flags = 0;\n-\tif (*p == '!') {\n+\tif (!gitattributes && *p == '!') {\n \t\t*flags |= EXC_FLAG_NEGATIVE;\n \t\tp++;\n \t}\n@@ -354,7 +355,7 @@ void add_exclude(const char *string, const char *base,\n \tint flags;\n \tint nowildcardlen;\n \n-\tparse_exclude_pattern(&string, &patternlen, &flags, &nowildcardlen);\n+\tparse_exclude_pattern(&string, &patternlen, &flags, &nowildcardlen, 0);\n \tif (flags & EXC_FLAG_MUSTBEDIR) {\n \t\tchar *s;\n \t\tx = xmalloc(sizeof(*x) + patternlen + 1);\ndiff --git a/dir.h b/dir.h\nindex f5c89e3..3aeb519 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -107,7 +107,11 @@ extern int path_excluded(struct path_exclude_check *, const char *, int namelen,\n extern int add_excludes_from_file_to_list(const char *fname, const char *base, int baselen,\n \t\t\t\t\t char **buf_p, struct exclude_list *which, int check_index);\n extern void add_excludes_from_file(struct dir_struct *, const char *fname);\n-extern void parse_exclude_pattern(const char **string, int *patternlen, int *flags, int *nowildcardlen);\n+extern void parse_exclude_pattern(const char **string,\n+\t\t\t\t int *patternlen,\n+\t\t\t\t int *flags,\n+\t\t\t\t int *nowildcardlen,\n+\t\t\t\t int gitattributes);\n extern void add_exclude(const char *string, const char *base,\n \t\t\tint baselen, struct exclude_list *which);\n extern void free_excludes(struct exclude_list *el);\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 1035a14..7ca283f 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -197,9 +197,8 @@ test_expect_success 'root subdir attribute test' '\n '\n \n test_expect_success 'negative patterns' '\n-\techo \"!f test=bar\" >.gitattributes &&\n-\tgit check-attr test -- '\"'\"'!f'\"'\"' 2>errors &&\n-\ttest_i18ngrep \"Negative patterns are ignored\" errors\n+\techo \"!f test=foo\" >.gitattributes &&\n+\tattr_check \"!f\" foo\n '\n \n test_expect_success 'patterns starting with exclamation' '\n-- \n1.8.1.2.536.gf441e6d\n"},{"id":"210518","messageId":"7vppzhc7wf.fsf@alter.siamese.dyndns.org","threadId":"33049","inReplyTo":"CACsJy8CO1i1_QpMbLvfys=wsLDPTK69_9JanQYq2D3-fboK_WA@mail.gmail.com","subject":"Re: [PATCH] Make !pattern in .gitattributes non-fatal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-03T06:30:24Z","receivedAt":"2013-03-03T06:30:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> This \"return NULL;\" means we ignore \"!blah\" pattern, which is a\n> regression, isn't it? Should we treat '!' as literal here?\n\nProbably not. Can you point to a project everybody has heard of\nthat keeps track of a path that begins with an exclamation point?\n\nWith clarification to the documentation that says \"you cannot use !\nto negate\" and your \"die on such an entry\", we have been going in\nthe direction that forbids the use of such an entry, and making it\nmean literal exclamation point is going sideways in the middle of\nthe road.\n\nBesides, if you want to match a path that begins with an exclam, you\ncan always say \"[!]\", no?\n"}]}