{"thread":{"id":"31765","subject":"'git grep needle rev' attempts to access 'rev:.../.gitattributes' in the worktree","startedAt":"2012-10-09T09:03:57Z","lastAt":"2012-10-17T07:33:12Z","messageCount":43,"participants":["Johannes Sixt","Nguyen Thai Ngoc Duy","Jeff King","Junio C Hamano","Nguyễn Thái Ngọc Duy","Michael Haggerty"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"200825","messageId":"5073E87D.9020100@viscovery.net","threadId":"31765","inReplyTo":null,"subject":"'git grep needle rev' attempts to access 'rev:.../.gitattributes' in the worktree","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-09T09:03:57Z","receivedAt":"2012-10-09T09:03:57Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Running 'git grep needle origin/master' on Windows gives numerous warnings\nof the kind\n\nwarning: unable to access 'origin/master:Documentation/.gitattributes':\nInvalid argument\n\nIt is worrysome that it is attempted to access a file whose name is\nprefixed by a revision.\n\n-- Hannes\n"},{"id":"200828","messageId":"CACsJy8BFmUr5_E47900T6j9Lgdvj96_dNbo2LHP5hLYq3w7ZLA@mail.gmail.com","threadId":"31765","inReplyTo":"5073E87D.9020100@viscovery.net","subject":"Re: 'git grep needle rev' attempts to access 'rev:.../.gitattributes' in the worktree","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-09T09:38:32Z","receivedAt":"2012-10-09T09:38:32Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Oct 9, 2012 at 4:03 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Running 'git grep needle origin/master' on Windows gives numerous warnings\n> of the kind\n>\n> warning: unable to access 'origin/master:Documentation/.gitattributes':\n> Invalid argument\n\nstrace confirms it. Stack trace\n\n#0  read_attr_from_file (path=0x820e818\n\"HEAD:Documentation/.gitattributes\", macro_ok=0) at attr.c:351\n#1  0x080d378d in read_attr (path=0x820e818\n\"HEAD:Documentation/.gitattributes\", macro_ok=0) at attr.c:436\n#2  0x080d3bf1 in prepare_attr_stack (path=0x820e7f0\n\"HEAD:Documentation/.gitattributes\") at attr.c:630\n#3  0x080d3f68 in collect_all_attrs (path=0x820e7f0\n\"HEAD:Documentation/.gitattributes\") at attr.c:747\n#4  0x080d3ffd in git_check_attr (path=0x820e7f0\n\"HEAD:Documentation/.gitattributes\", num=1, check=0xbfffd848) at\nattr.c:761\n#5  0x0815e736 in userdiff_find_by_path (path=0x820e7f0\n\"HEAD:Documentation/.gitattributes\") at userdiff.c:278\n#6  0x081058ca in grep_source_load_driver (gs=0xbfffd978) at grep.c:1504\n#7  0x08105907 in grep_source_is_binary (gs=0xbfffd978) at grep.c:1512\n#8  0x08104eaa in grep_source_1 (opt=0xbfffe304, gs=0xbfffd978,\ncollect_hits=0) at grep.c:1180\n\nNever touched userdiff.c before. Anybody?\n-- \nDuy\n"},{"id":"200832","messageId":"20121009120144.GA8780@do","threadId":"31765","inReplyTo":"CACsJy8BFmUr5_E47900T6j9Lgdvj96_dNbo2LHP5hLYq3w7ZLA@mail.gmail.com","subject":"Re: 'git grep needle rev' attempts to access 'rev:.../.gitattributes' in the worktree","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-09T12:01:44Z","receivedAt":"2012-10-09T12:01:44Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Oct 09, 2012 at 04:38:32PM +0700, Nguyen Thai Ngoc Duy wrote:\n> #5  0x0815e736 in userdiff_find_by_path (path=0x820e7f0\n> \"HEAD:Documentation/.gitattributes\") at userdiff.c:278\n> #6  0x081058ca in grep_source_load_driver (gs=0xbfffd978) at grep.c:1504\n\nA bandage patch may look like this. But it does not solve the real\nproblem:\n\n - we should be able to look up in-tree .gitattributes, not just\n   ignore like this patch does\n\n - gs->name seems to be prepared for human consumption, not for\n   accessing files. grep_file() with opt->relative is true can put\n   quotes in gs->name, for example.\n\nI feel like we should make this function a callback and let git-grep\ndeal with driver loading itself.\n\n-- 8< --\ndiff --git a/grep.c b/grep.c\nindex edc7776..c5ed067 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1501,7 +1501,8 @@ void grep_source_load_driver(struct grep_source *gs)\n \t\treturn;\n \n \tgrep_attr_lock();\n-\tgs->driver = userdiff_find_by_path(gs->name);\n+\tif (gs->type == GREP_SOURCE_FILE)\n+\t\tgs->driver = userdiff_find_by_path(gs->name);\n \tif (!gs->driver)\n \t\tgs->driver = userdiff_find_by_name(\"default\");\n \tgrep_attr_unlock();\n-- 8< --\n\n-- \nDuy\n"},{"id":"200834","messageId":"20121009124144.GB12465@sigill.intra.peff.net","threadId":"31765","inReplyTo":"20121009120144.GA8780@do","subject":"Re: 'git grep needle rev' attempts to access 'rev:.../.gitattributes' in the worktree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-09T12:41:44Z","receivedAt":"2012-10-09T12:41:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 09, 2012 at 07:01:44PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> On Tue, Oct 09, 2012 at 04:38:32PM +0700, Nguyen Thai Ngoc Duy wrote:\n> > #5  0x0815e736 in userdiff_find_by_path (path=0x820e7f0\n> > \"HEAD:Documentation/.gitattributes\") at userdiff.c:278\n> > #6  0x081058ca in grep_source_load_driver (gs=0xbfffd978) at grep.c:1504\n> \n> A bandage patch may look like this. But it does not solve the real\n> problem:\n> \n>  - we should be able to look up in-tree .gitattributes, not just\n>    ignore like this patch does\n> \n>  - gs->name seems to be prepared for human consumption, not for\n>    accessing files. grep_file() with opt->relative is true can put\n>    quotes in gs->name, for example.\n\nRight. For the second, you would probably want gs->identifier in the\ncase of GREP_SOURCE_FILE. But that identifier information is not\navailable at all for GREP_SOURCE_SHA1, which is what is breaking the\nfirst point.\n\n> I feel like we should make this function a callback and let git-grep\n> deal with driver loading itself.\n\nI think we just need to have callers of grep_source_init provide us with\nthe actual pathname (or NULL, in the case of GREP_SOURCE_BUF). That is\nwhere the information is lost.\n\nLike this incomplete and untested patch, which should fix the quoting\nproblem for GREP_SOURCE_FILE, but leave the sha1 bits broken (see the\nin-code comment). I'm traveling this week, so I doubt I'll have time to\nlook for a few more days. If you want to work on it, please do.\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 82530a6..be602dd 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -86,7 +86,7 @@ static void add_work(struct grep_opt *opt, enum grep_source_type type,\n static int skip_first_line;\n \n static void add_work(struct grep_opt *opt, enum grep_source_type type,\n-\t\t     const char *name, const void *id)\n+\t\t     const char *name, const char *path, const void *id)\n {\n \tgrep_lock();\n \n@@ -94,7 +94,7 @@ static void add_work(struct grep_opt *opt, enum grep_source_type type,\n \t\tpthread_cond_wait(&cond_write, &grep_mutex);\n \t}\n \n-\tgrep_source_init(&todo[todo_end].source, type, name, id);\n+\tgrep_source_init(&todo[todo_end].source, type, name, path, id);\n \tif (opt->binary != GREP_BINARY_TEXT)\n \t\tgrep_source_load_driver(&todo[todo_end].source);\n \ttodo[todo_end].done = 0;\n@@ -378,14 +378,21 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \tif (opt->relative && opt->prefix_length) {\n \t\tquote_path_relative(filename + tree_name_len, -1, &pathbuf,\n \t\t\t\t    opt->prefix);\n+\t\t/* XXX Why do we insert here instead of just putting it in\n+\t\t * first? */\n \t\tstrbuf_insert(&pathbuf, 0, filename, tree_name_len);\n \t} else {\n \t\tstrbuf_addstr(&pathbuf, filename);\n \t}\n \n+\t/* XXX We seem to get all kinds of junk via the filename field here,\n+\t * including partial filenames, sha1:path, etc. We could parse it\n+\t * ourselves, but that is probably insanity. We should ask the\n+\t * caller to break it down more for us. For now, just pass NULL. */\n+\n #ifndef NO_PTHREADS\n \tif (use_threads) {\n-\t\tadd_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, sha1);\n+\t\tadd_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, NULL, sha1);\n \t\tstrbuf_release(&pathbuf);\n \t\treturn 0;\n \t} else\n@@ -394,7 +401,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \t\tstruct grep_source gs;\n \t\tint hit;\n \n-\t\tgrep_source_init(&gs, GREP_SOURCE_SHA1, pathbuf.buf, sha1);\n+\t\tgrep_source_init(&gs, GREP_SOURCE_SHA1, pathbuf.buf, NULL, sha1);\n \t\tstrbuf_release(&pathbuf);\n \t\thit = grep_source(opt, &gs);\n \n@@ -414,7 +421,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \n #ifndef NO_PTHREADS\n \tif (use_threads) {\n-\t\tadd_work(opt, GREP_SOURCE_FILE, buf.buf, filename);\n+\t\tadd_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);\n \t\tstrbuf_release(&buf);\n \t\treturn 0;\n \t} else\n@@ -423,7 +430,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\tstruct grep_source gs;\n \t\tint hit;\n \n-\t\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename);\n+\t\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);\n \t\tstrbuf_release(&buf);\n \t\thit = grep_source(opt, &gs);\n \ndiff --git a/grep.c b/grep.c\nindex edc7776..06bc1c6 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1373,7 +1373,7 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n \tstruct grep_source gs;\n \tint r;\n \n-\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL);\n+\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);\n \tgs.buf = buf;\n \tgs.size = size;\n \n@@ -1384,10 +1384,12 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n }\n \n void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n-\t\t      const char *name, const void *identifier)\n+\t\t      const char *name, const char *path,\n+\t\t      const void *identifier)\n {\n \tgs->type = type;\n \tgs->name = name ? xstrdup(name) : NULL;\n+\tgs->path = path ? xstrdup(path) : NULL;\n \tgs->buf = NULL;\n \tgs->size = 0;\n \tgs->driver = NULL;\n@@ -1409,6 +1411,8 @@ void grep_source_clear(struct grep_source *gs)\n {\n \tfree(gs->name);\n \tgs->name = NULL;\n+\tfree(gs->path);\n+\tgs->path = NULL;\n \tfree(gs->identifier);\n \tgs->identifier = NULL;\n \tgrep_source_clear_data(gs);\ndiff --git a/grep.h b/grep.h\nindex c256ac6..c2cf57b 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -158,11 +158,13 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n \tchar *buf;\n \tunsigned long size;\n \n+\tchar *path; /* for attribute lookups */\n \tstruct userdiff_driver *driver;\n };\n \n void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n-\t\t      const char *name, const void *identifier);\n+\t\t      const char *name, const char *path,\n+\t\t      const void *identifier);\n void grep_source_clear_data(struct grep_source *gs);\n void grep_source_clear(struct grep_source *gs);\n void grep_source_load_driver(struct grep_source *gs);\n"},{"id":"200852","messageId":"7vk3uzfp3p.fsf@alter.siamese.dyndns.org","threadId":"31765","inReplyTo":"20121009124144.GB12465@sigill.intra.peff.net","subject":"Re: 'git grep needle rev' attempts to access 'rev:.../.gitattributes' in the worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-09T18:59:54Z","receivedAt":"2012-10-09T18:59:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think we just need to have callers of grep_source_init provide us with\n> the actual pathname (or NULL, in the case of GREP_SOURCE_BUF). That is\n> where the information is lost.\n\nYes.  I agree that is the right approach.\n"},{"id":"200875","messageId":"CACsJy8Cku-XTbQBDpZpPh7AMDFMoNKLKeJNs5HKCQnBy420d2w@mail.gmail.com","threadId":"31765","inReplyTo":"7vk3uzfp3p.fsf@alter.siamese.dyndns.org","subject":"Re: 'git grep needle rev' attempts to access 'rev:.../.gitattributes' in the worktree","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-10T05:17:53Z","receivedAt":"2012-10-10T05:17:53Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Oct 10, 2012 at 1:59 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> I think we just need to have callers of grep_source_init provide us with\n>> the actual pathname (or NULL, in the case of GREP_SOURCE_BUF). That is\n>> where the information is lost.\n>\n> Yes.  I agree that is the right approach.\n\nPassing full path this way means prepare_attr_stack has to walk back\nto top directory for every files (even in the same directory). If\n.gitattributes are loaded while git-grep traverses the tree, then it\ncan preload attr once per directory. But Jeff's approach looks\nsimpler.\n-- \nDuy\n"},{"id":"200877","messageId":"7vwqyydh6v.fsf@alter.siamese.dyndns.org","threadId":"31765","inReplyTo":"CACsJy8Cku-XTbQBDpZpPh7AMDFMoNKLKeJNs5HKCQnBy420d2w@mail.gmail.com","subject":"Re: 'git grep needle rev' attempts to access 'rev:.../.gitattributes' in the worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-10T05:33:44Z","receivedAt":"2012-10-10T05:33:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> On Wed, Oct 10, 2012 at 1:59 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Jeff King <peff@peff.net> writes:\n>>\n>>> I think we just need to have callers of grep_source_init provide us with\n>>> the actual pathname (or NULL, in the case of GREP_SOURCE_BUF). That is\n>>> where the information is lost.\n>>\n>> Yes.  I agree that is the right approach.\n>\n> Passing full path this way means prepare_attr_stack has to walk back\n> to top directory for every files (even in the same directory). If\n> .gitattributes are loaded while git-grep traverses the tree, then it\n> can preload attr once per directory. But Jeff's approach looks\n> simpler.\n\nWhy can't you do both?  That is, to build a full path as you\ndescend, and read per-directory .gitattributes as you go?\n\nConfused...\n"},{"id":"200879","messageId":"CACsJy8AoLvWc9NCkqX_gEiBwLZDDx7TA9_CEMmXbb=k8iDS7_Q@mail.gmail.com","threadId":"31765","inReplyTo":"7vwqyydh6v.fsf@alter.siamese.dyndns.org","subject":"Re: 'git grep needle rev' attempts to access 'rev:.../.gitattributes' in the worktree","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-10T05:45:21Z","receivedAt":"2012-10-10T05:45:21Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Oct 10, 2012 at 12:33 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n>\n>> On Wed, Oct 10, 2012 at 1:59 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Jeff King <peff@peff.net> writes:\n>>>\n>>>> I think we just need to have callers of grep_source_init provide us with\n>>>> the actual pathname (or NULL, in the case of GREP_SOURCE_BUF). That is\n>>>> where the information is lost.\n>>>\n>>> Yes.  I agree that is the right approach.\n>>\n>> Passing full path this way means prepare_attr_stack has to walk back\n>> to top directory for every files (even in the same directory). If\n>> .gitattributes are loaded while git-grep traverses the tree, then it\n>> can preload attr once per directory. But Jeff's approach looks\n>> simpler.\n>\n> Why can't you do both?  That is, to build a full path as you\n> descend, and read per-directory .gitattributes as you go?\n\nHm... I need to check attr.c code but I think it means read\n.gitattributes and prepare attr stack in builtin/grep.c (where we\ntraverse trees) and actually check the attribute in\ngrep_source_load_driver(), much further down the call stack. I'm not\nsure how we can pass the prepared attr stack down to\ngrep_source_load_driver().\n-- \nDuy\n"},{"id":"200912","messageId":"1349868894-3579-1-git-send-email-pclouds@gmail.com","threadId":"31765","inReplyTo":"7vk3uzfp3p.fsf@alter.siamese.dyndns.org","subject":"Re: 'git grep needle rev' attempts to access 'rev:.../.gitattributes' in the worktree","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-10T11:34:51Z","receivedAt":"2012-10-10T11:34:51Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Oct 10, 2012 at 1:59 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> I think we just need to have callers of grep_source_init provide us with\n>> the actual pathname (or NULL, in the case of GREP_SOURCE_BUF). That is\n>> where the information is lost.\n>\n> Yes.  I agree that is the right approach.\n\nHere we go. No additional tests as I don't know how to write them. But\nstrace shows it's ok.\n\nCurrently we don't have many options to optimize attr access. We\ncannot prepare a attr stack in advance for a specific directory, then\ncheck attributes many times using the stack. We cannot prepare attr\nstack from a tree object. As a result a normal \"grep pattern --\"\nresults in a lot of duplicate failed open(\".../.gitattributes\") calls.\n\nJeff King (1):\n  grep: pass true path name to grep machinery\n\nNguyễn Thái Ngọc Duy (2):\n  quote: let caller reset buffer for quote_path_relative()\n  grep: stop looking at random places for .gitattributes\n\n Documentation/git-grep.txt |  7 +++++--\n builtin/clean.c            |  2 ++\n builtin/grep.c             | 19 ++++++++++++-------\n builtin/ls-files.c         |  1 +\n grep.c                     | 11 ++++++++---\n grep.h                     |  4 +++-\n quote.c                    |  1 -\n wt-status.c                |  1 +\n 8 files changed, 32 insertions(+), 14 deletions(-)\n\n-- \n1.7.12.1.406.g6ab07c4\n"},{"id":"200913","messageId":"1349868894-3579-2-git-send-email-pclouds@gmail.com","threadId":"31765","inReplyTo":"1349868894-3579-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 1/3] quote: let caller reset buffer for quote_path_relative()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-10T11:34:52Z","receivedAt":"2012-10-10T11:34:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"quote_path_relative() resetting output buffer is sometimes unnecessary\nas the buffer has never been used, and some other times makes it\nharder for the caller to use (see builtin/grep.c, the caller has to\ninsert a string after quote_path_relative)\n\nMove the buffer reset back to call sites when necessary.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n The answer for Jeff's XXX in his patch, why strbuf_insert() instead\n of just adding in advance.\n\n builtin/clean.c    | 2 ++\n builtin/grep.c     | 2 +-\n builtin/ls-files.c | 1 +\n quote.c            | 1 -\n wt-status.c        | 1 +\n 5 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 69c1cda..ff633cc 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -149,6 +149,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\tstrbuf_addstr(&directory, ent->name);\n+\t\t\tstrbuf_reset(&buf);\n \t\t\tqname = quote_path_relative(directory.buf, directory.len, &buf, prefix);\n \t\t\tif (show_only && (remove_directories ||\n \t\t\t    (matches == MATCHED_EXACTLY))) {\n@@ -171,6 +172,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t} else {\n \t\t\tif (pathspec && !matches)\n \t\t\t\tcontinue;\n+\t\t\tstrbuf_reset(&buf);\n \t\t\tqname = quote_path_relative(ent->name, -1, &buf, prefix);\n \t\t\tif (show_only) {\n \t\t\t\tprintf(_(\"Would remove %s\\n\"), qname);\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 82530a6..377c904 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -376,9 +376,9 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \tstruct strbuf pathbuf = STRBUF_INIT;\n \n \tif (opt->relative && opt->prefix_length) {\n+\t\tstrbuf_add(&pathbuf, filename, tree_name_len);\n \t\tquote_path_relative(filename + tree_name_len, -1, &pathbuf,\n \t\t\t\t    opt->prefix);\n-\t\tstrbuf_insert(&pathbuf, 0, filename, tree_name_len);\n \t} else {\n \t\tstrbuf_addstr(&pathbuf, filename);\n \t}\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex b5434af..1e0cae9 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -395,6 +395,7 @@ int report_path_error(const char *ps_matched, const char **pathspec, const char\n \t\tif (found_dup)\n \t\t\tcontinue;\n \n+\t\tstrbuf_reset(&sb);\n \t\tname = quote_path_relative(pathspec[num], -1, &sb, prefix);\n \t\terror(\"pathspec '%s' did not match any file(s) known to git.\",\n \t\t      name);\ndiff --git a/quote.c b/quote.c\nindex 911229f..7e23ba9 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -381,7 +381,6 @@ char *quote_path_relative(const char *in, int len,\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *rel = path_relative(in, len, &sb, prefix, -1);\n-\tstrbuf_reset(out);\n \tquote_c_style_counted(rel, strlen(rel), out, NULL, 0);\n \tstrbuf_release(&sb);\n \ndiff --git a/wt-status.c b/wt-status.c\nindex 2a9658b..be8b600 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -690,6 +690,7 @@ static void wt_status_print_other(struct wt_status *s,\n \t\tstruct string_list_item *it;\n \t\tconst char *path;\n \t\tit = &(l->items[i]);\n+\t\tstrbuf_reset(&buf);\n \t\tpath = quote_path(it->string, strlen(it->string),\n \t\t\t\t  &buf, s->prefix);\n \t\tif (column_active(s->colopts)) {\n-- \n1.7.12.1.406.g6ab07c4\n"},{"id":"200914","messageId":"1349868894-3579-3-git-send-email-pclouds@gmail.com","threadId":"31765","inReplyTo":"1349868894-3579-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/3] grep: pass true path name to grep machinery","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-10T11:34:53Z","receivedAt":"2012-10-10T11:34:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nHaving path name (if available) may be useful. For example, if grep\nmachinery needs to look up git attributes, it'll need a good path\nname. grep_source->name is not good because it's for display purpose\nonly.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/grep.c | 17 +++++++++++------\n grep.c         |  8 ++++++--\n grep.h         |  4 +++-\n 3 files changed, 20 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 377c904..0211e35 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -86,7 +86,7 @@ static pthread_cond_t cond_result;\n static int skip_first_line;\n \n static void add_work(struct grep_opt *opt, enum grep_source_type type,\n-\t\t     const char *name, const void *id)\n+\t\t     const char *name, const char *path, const void *id)\n {\n \tgrep_lock();\n \n@@ -94,7 +94,7 @@ static void add_work(struct grep_opt *opt, enum grep_source_type type,\n \t\tpthread_cond_wait(&cond_write, &grep_mutex);\n \t}\n \n-\tgrep_source_init(&todo[todo_end].source, type, name, id);\n+\tgrep_source_init(&todo[todo_end].source, type, name, path, id);\n \tif (opt->binary != GREP_BINARY_TEXT)\n \t\tgrep_source_load_driver(&todo[todo_end].source);\n \ttodo[todo_end].done = 0;\n@@ -383,9 +383,14 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \t\tstrbuf_addstr(&pathbuf, filename);\n \t}\n \n+\t/* XXX We seem to get all kinds of junk via the filename field here,\n+\t * including partial filenames, sha1:path, etc. We could parse it\n+\t * ourselves, but that is probably insanity. We should ask the\n+\t * caller to break it down more for us. For now, just pass NULL. */\n+\n #ifndef NO_PTHREADS\n \tif (use_threads) {\n-\t\tadd_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, sha1);\n+\t\tadd_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, NULL, sha1);\n \t\tstrbuf_release(&pathbuf);\n \t\treturn 0;\n \t} else\n@@ -394,7 +399,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \t\tstruct grep_source gs;\n \t\tint hit;\n \n-\t\tgrep_source_init(&gs, GREP_SOURCE_SHA1, pathbuf.buf, sha1);\n+\t\tgrep_source_init(&gs, GREP_SOURCE_SHA1, pathbuf.buf, NULL, sha1);\n \t\tstrbuf_release(&pathbuf);\n \t\thit = grep_source(opt, &gs);\n \n@@ -414,7 +419,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \n #ifndef NO_PTHREADS\n \tif (use_threads) {\n-\t\tadd_work(opt, GREP_SOURCE_FILE, buf.buf, filename);\n+\t\tadd_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);\n \t\tstrbuf_release(&buf);\n \t\treturn 0;\n \t} else\n@@ -423,7 +428,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\tstruct grep_source gs;\n \t\tint hit;\n \n-\t\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename);\n+\t\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);\n \t\tstrbuf_release(&buf);\n \t\thit = grep_source(opt, &gs);\n \ndiff --git a/grep.c b/grep.c\nindex edc7776..06bc1c6 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1373,7 +1373,7 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n \tstruct grep_source gs;\n \tint r;\n \n-\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL);\n+\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);\n \tgs.buf = buf;\n \tgs.size = size;\n \n@@ -1384,10 +1384,12 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n }\n \n void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n-\t\t      const char *name, const void *identifier)\n+\t\t      const char *name, const char *path,\n+\t\t      const void *identifier)\n {\n \tgs->type = type;\n \tgs->name = name ? xstrdup(name) : NULL;\n+\tgs->path = path ? xstrdup(path) : NULL;\n \tgs->buf = NULL;\n \tgs->size = 0;\n \tgs->driver = NULL;\n@@ -1409,6 +1411,8 @@ void grep_source_clear(struct grep_source *gs)\n {\n \tfree(gs->name);\n \tgs->name = NULL;\n+\tfree(gs->path);\n+\tgs->path = NULL;\n \tfree(gs->identifier);\n \tgs->identifier = NULL;\n \tgrep_source_clear_data(gs);\ndiff --git a/grep.h b/grep.h\nindex c256ac6..c2cf57b 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -158,11 +158,13 @@ struct grep_source {\n \tchar *buf;\n \tunsigned long size;\n \n+\tchar *path; /* for attribute lookups */\n \tstruct userdiff_driver *driver;\n };\n \n void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n-\t\t      const char *name, const void *identifier);\n+\t\t      const char *name, const char *path,\n+\t\t      const void *identifier);\n void grep_source_clear_data(struct grep_source *gs);\n void grep_source_clear(struct grep_source *gs);\n void grep_source_load_driver(struct grep_source *gs);\n-- \n1.7.12.1.406.g6ab07c4\n"},{"id":"200915","messageId":"1349868894-3579-4-git-send-email-pclouds@gmail.com","threadId":"31765","inReplyTo":"1349868894-3579-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/3] grep: stop looking at random places for .gitattributes","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-10T11:34:54Z","receivedAt":"2012-10-10T11:34:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"grep searches for .gitattributes using \"name\" field in struct\ngrep_source but that field is not real on-disk path name. For example,\n\"grep pattern rev\" fills the field with \"rev:path\", which is\nnon-existent usually until somebody exploits it to drive git away.\n\nattr does not support looking up .gitattributes from a tree object.\nMaking \"git grep pattern <rev>\" support .gitattributes could be a big\nwork. Just note in document what we support for now. The document\nchanges in this patch are to be reverted once support is in place.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Documentation/git-grep.txt | 7 +++++--\n grep.c                     | 3 ++-\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex cfecf84..a4c66ee 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -86,7 +86,9 @@ OPTIONS\n \tfiles.\n \n -I::\n-\tDon't match the pattern in binary files.\n+\tDon't match the pattern in binary files. Note that binary\n+\tdetection via .gitattributes only works with searching files\n+\tin working directory.\n \n --max-depth <depth>::\n \tFor each <pathspec> given on command line, descend at most <depth>\n@@ -189,7 +191,8 @@ OPTIONS\n \tthe match, unless the matching line is a function name itself.\n \tThe name is determined in the same way as 'git diff' works out\n \tpatch hunk headers (see 'Defining a custom hunk-header' in\n-\tlinkgit:gitattributes[5]).\n+\tlinkgit:gitattributes[5]). Note that .gitattributes are only\n+\tsupport for searching files in working directory.\n \n -<num>::\n -C <num>::\ndiff --git a/grep.c b/grep.c\nindex 06bc1c6..e36c01b 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1505,7 +1505,8 @@ void grep_source_load_driver(struct grep_source *gs)\n \t\treturn;\n \n \tgrep_attr_lock();\n-\tgs->driver = userdiff_find_by_path(gs->name);\n+\tif (gs->path)\n+\t\tgs->driver = userdiff_find_by_path(gs->path);\n \tif (!gs->driver)\n \t\tgs->driver = userdiff_find_by_name(\"default\");\n \tgrep_attr_unlock();\n-- \n1.7.12.1.406.g6ab07c4\n"},{"id":"200916","messageId":"5075615B.8020702@viscovery.net","threadId":"31765","inReplyTo":"1349868894-3579-4-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 3/3] grep: stop looking at random places for .gitattributes","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-10T11:51:55Z","receivedAt":"2012-10-10T11:51:55Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Thanks for working on this issue!\n\nAm 10/10/2012 13:34, schrieb Nguyễn Thái Ngọc Duy:\n> +\tlinkgit:gitattributes[5]). Note that .gitattributes are only\n> +\tsupport for searching files in working directory.\n\nDoes this mean it does not work for 'git grep --cached'? That would be a\nreal loss.\n\n-- Hannes\n"},{"id":"200917","messageId":"CACsJy8BG0HJNePG-j27SrfEenEkZ5a5nCdMEuDGn=qXxq=VzhA@mail.gmail.com","threadId":"31765","inReplyTo":"5075615B.8020702@viscovery.net","subject":"Re: [PATCH 3/3] grep: stop looking at random places for .gitattributes","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-10T12:03:33Z","receivedAt":"2012-10-10T12:03:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Oct 10, 2012 at 6:51 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Thanks for working on this issue!\n>\n> Am 10/10/2012 13:34, schrieb Nguyễn Thái Ngọc Duy:\n>> +     linkgit:gitattributes[5]). Note that .gitattributes are only\n>> +     support for searching files in working directory.\n>\n> Does this mean it does not work for 'git grep --cached'? That would be a\n> real loss.\n\nI missed this case. I don't think it works correctly before as it\nreads worktree's .gitattributes instead of the index version. But at\nleast we support reading .gitattributes from index. Patches coming\nsoon.\n-- \nDuy\n"},{"id":"200918","messageId":"50756646.5060505@viscovery.net","threadId":"31765","inReplyTo":"CACsJy8BG0HJNePG-j27SrfEenEkZ5a5nCdMEuDGn=qXxq=VzhA@mail.gmail.com","subject":"Re: [PATCH 3/3] grep: stop looking at random places for .gitattributes","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-10T12:12:54Z","receivedAt":"2012-10-10T12:12:54Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/10/2012 14:03, schrieb Nguyen Thai Ngoc Duy:\n> On Wed, Oct 10, 2012 at 6:51 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n>> Thanks for working on this issue!\n>>\n>> Am 10/10/2012 13:34, schrieb Nguyễn Thái Ngọc Duy:\n>>> +     linkgit:gitattributes[5]). Note that .gitattributes are only\n>>> +     support for searching files in working directory.\n>>\n>> Does this mean it does not work for 'git grep --cached'? That would be a\n>> real loss.\n> \n> I missed this case. I don't think it works correctly before as it\n> reads worktree's .gitattributes instead of the index version. But at\n> least we support reading .gitattributes from index. Patches coming\n> soon.\n\nIs there already an established definition which the \"correct\"\n.gitattributes are? IIRC, everywhere else we are looking at the\n.gitattributes in the worktree, regardless of whether the object at the\npath in question is in the worktree, the index, or in an old commit.\n\n-- Hannes\n"},{"id":"200919","messageId":"CACsJy8CznUShZVdmvGz1bfHHpZ1ZEX3PRUPb0+NNq2ZnvZEzZg@mail.gmail.com","threadId":"31765","inReplyTo":"50756646.5060505@viscovery.net","subject":"Re: [PATCH 3/3] grep: stop looking at random places for .gitattributes","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-10T12:32:18Z","receivedAt":"2012-10-10T12:32:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Oct 10, 2012 at 7:12 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Is there already an established definition which the \"correct\"\n> .gitattributes are?\n\nIf I ask to grep the index then to me it should read only the index.\nAlthough other people can counter that they may want different\nattributes than the one stored in index, which either comes from\nworktree or $GIT_DIR.\n\n> IIRC, everywhere else we are looking at the\n> .gitattributes in the worktree, regardless of whether the object at the\n> path in question is in the worktree, the index, or in an old commit.\n\ngit-archive has --worktree-attributes to specify where attributes come\nfrom. Sparse checkout can choose to read index version first then\nworktree's or the other way around. All normal operations read\nworktree version, if not found index version.\n-- \nDuy\n"},{"id":"200920","messageId":"50756D62.5030008@viscovery.net","threadId":"31765","inReplyTo":"CACsJy8CznUShZVdmvGz1bfHHpZ1ZEX3PRUPb0+NNq2ZnvZEzZg@mail.gmail.com","subject":"Re: [PATCH 3/3] grep: stop looking at random places for .gitattributes","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-10T12:43:14Z","receivedAt":"2012-10-10T12:43:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/10/2012 14:32, schrieb Nguyen Thai Ngoc Duy:\n> git-archive has --worktree-attributes to specify where attributes come\n> from. Sparse checkout can choose to read index version first then\n> worktree's or the other way around. All normal operations read\n> worktree version, if not found index version.\n\nSo, even if I run 'git diff master~2000 master~1999', it uses the\nattributes from the worktree, and if not found from the index? (I would\nnot mind, BTW.)\n\n-- Hannes\n"},{"id":"200921","messageId":"CACsJy8A4Ef3P+qjR6eJD1bH7XUNGBKCiwiMc6hhQYskxBwz8Ag@mail.gmail.com","threadId":"31765","inReplyTo":"50756D62.5030008@viscovery.net","subject":"Re: [PATCH 3/3] grep: stop looking at random places for .gitattributes","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-10T12:51:15Z","receivedAt":"2012-10-10T12:51:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Oct 10, 2012 at 7:43 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Am 10/10/2012 14:32, schrieb Nguyen Thai Ngoc Duy:\n>> git-archive has --worktree-attributes to specify where attributes come\n>> from. Sparse checkout can choose to read index version first then\n>> worktree's or the other way around. All normal operations read\n>> worktree version, if not found index version.\n>\n> So, even if I run 'git diff master~2000 master~1999', it uses the\n> attributes from the worktree, and if not found from the index? (I would\n> not mind, BTW.)\n\nYes. Tested and verified.\n-- \nDuy\n"},{"id":"200924","messageId":"1349877544-17648-1-git-send-email-pclouds@gmail.com","threadId":"31765","inReplyTo":"1349868894-3579-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 0/2] Re: 'git grep needle rev' attempts to access 'rev:.../.gitattributes' in the worktree","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-10T13:59:02Z","receivedAt":"2012-10-10T13:59:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Round too. .gitattributes should be respected in all cases except blob\ngrepping. As Johannes pointed elsewhere in this thread, if \"git\ndiff <rev1> <rev2>\" looks up worktree's .gitattributes (and none in\nrev1/rev2), there's no reason \"git grep\" should behave differently.\n\nNguyễn Thái Ngọc Duy (2):\n  quote: let caller reset buffer for quote_path_relative()\n  grep: stop looking at random places for .gitattributes\n\n builtin/clean.c        |  2 ++\n builtin/grep.c         | 24 +++++++++++++-----------\n builtin/ls-files.c     |  1 +\n grep.c                 | 11 ++++++++---\n grep.h                 |  4 +++-\n quote.c                |  1 -\n t/t7008-grep-binary.sh | 22 ++++++++++++++++++++++\n wt-status.c            |  1 +\n 8 files changed, 50 insertions(+), 16 deletions(-)\n\n-- \n1.7.12.1.406.g6ab07c4\n"},{"id":"200923","messageId":"1349877544-17648-2-git-send-email-pclouds@gmail.com","threadId":"31765","inReplyTo":"1349877544-17648-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 1/2] quote: let caller reset buffer for quote_path_relative()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-10T13:59:03Z","receivedAt":"2012-10-10T13:59:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"quote_path_relative() resetting output buffer is sometimes unnecessary\nas the buffer has never been used, and some other times makes it\nharder for the caller to use (see builtin/grep.c, the caller has to\ninsert a string after quote_path_relative)\n\nMove the buffer reset back to call sites when necessary.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/clean.c    | 2 ++\n builtin/grep.c     | 2 +-\n builtin/ls-files.c | 1 +\n quote.c            | 1 -\n wt-status.c        | 1 +\n 5 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 69c1cda..ff633cc 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -149,6 +149,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\tstrbuf_addstr(&directory, ent->name);\n+\t\t\tstrbuf_reset(&buf);\n \t\t\tqname = quote_path_relative(directory.buf, directory.len, &buf, prefix);\n \t\t\tif (show_only && (remove_directories ||\n \t\t\t    (matches == MATCHED_EXACTLY))) {\n@@ -171,6 +172,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t} else {\n \t\t\tif (pathspec && !matches)\n \t\t\t\tcontinue;\n+\t\t\tstrbuf_reset(&buf);\n \t\t\tqname = quote_path_relative(ent->name, -1, &buf, prefix);\n \t\t\tif (show_only) {\n \t\t\t\tprintf(_(\"Would remove %s\\n\"), qname);\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 82530a6..377c904 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -376,9 +376,9 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \tstruct strbuf pathbuf = STRBUF_INIT;\n \n \tif (opt->relative && opt->prefix_length) {\n+\t\tstrbuf_add(&pathbuf, filename, tree_name_len);\n \t\tquote_path_relative(filename + tree_name_len, -1, &pathbuf,\n \t\t\t\t    opt->prefix);\n-\t\tstrbuf_insert(&pathbuf, 0, filename, tree_name_len);\n \t} else {\n \t\tstrbuf_addstr(&pathbuf, filename);\n \t}\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex b5434af..1e0cae9 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -395,6 +395,7 @@ int report_path_error(const char *ps_matched, const char **pathspec, const char\n \t\tif (found_dup)\n \t\t\tcontinue;\n \n+\t\tstrbuf_reset(&sb);\n \t\tname = quote_path_relative(pathspec[num], -1, &sb, prefix);\n \t\terror(\"pathspec '%s' did not match any file(s) known to git.\",\n \t\t      name);\ndiff --git a/quote.c b/quote.c\nindex 911229f..7e23ba9 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -381,7 +381,6 @@ char *quote_path_relative(const char *in, int len,\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *rel = path_relative(in, len, &sb, prefix, -1);\n-\tstrbuf_reset(out);\n \tquote_c_style_counted(rel, strlen(rel), out, NULL, 0);\n \tstrbuf_release(&sb);\n \ndiff --git a/wt-status.c b/wt-status.c\nindex 2a9658b..be8b600 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -690,6 +690,7 @@ static void wt_status_print_other(struct wt_status *s,\n \t\tstruct string_list_item *it;\n \t\tconst char *path;\n \t\tit = &(l->items[i]);\n+\t\tstrbuf_reset(&buf);\n \t\tpath = quote_path(it->string, strlen(it->string),\n \t\t\t\t  &buf, s->prefix);\n \t\tif (column_active(s->colopts)) {\n-- \n1.7.12.1.406.g6ab07c4\n"},{"id":"200925","messageId":"1349877544-17648-3-git-send-email-pclouds@gmail.com","threadId":"31765","inReplyTo":"1349877544-17648-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 2/2] grep: stop looking at random places for .gitattributes","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-10T13:59:04Z","receivedAt":"2012-10-10T13:59:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"grep searches for .gitattributes using \"name\" field in struct\ngrep_source but that field is not real on-disk path name. For example,\n\"grep pattern rev\" fills the field with \"rev:path\", which is\nnon-existent usually until somebody exploits it to drive git away.\n\nThis patch passes real paths down to grep_source_load_driver(). Except\ngrepping a blob, all other cases should have right paths down to\ngrep_source_load_driver(). In other words, .gitattributes are still\nrespected.\n\nInitial-work-by: Jeff King <peff@peff.net>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/grep.c         | 22 ++++++++++++----------\n grep.c                 | 11 ++++++++---\n grep.h                 |  4 +++-\n t/t7008-grep-binary.sh | 22 ++++++++++++++++++++++\n 4 files changed, 45 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 377c904..f6c5ba2 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -86,7 +86,7 @@ static pthread_cond_t cond_result;\n static int skip_first_line;\n \n static void add_work(struct grep_opt *opt, enum grep_source_type type,\n-\t\t     const char *name, const void *id)\n+\t\t     const char *name, const char *path, const void *id)\n {\n \tgrep_lock();\n \n@@ -94,7 +94,7 @@ static void add_work(struct grep_opt *opt, enum grep_source_type type,\n \t\tpthread_cond_wait(&cond_write, &grep_mutex);\n \t}\n \n-\tgrep_source_init(&todo[todo_end].source, type, name, id);\n+\tgrep_source_init(&todo[todo_end].source, type, name, path, id);\n \tif (opt->binary != GREP_BINARY_TEXT)\n \t\tgrep_source_load_driver(&todo[todo_end].source);\n \ttodo[todo_end].done = 0;\n@@ -371,7 +371,8 @@ static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type\n }\n \n static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n-\t\t     const char *filename, int tree_name_len)\n+\t\t     const char *filename, int tree_name_len,\n+\t\t     const char *path)\n {\n \tstruct strbuf pathbuf = STRBUF_INIT;\n \n@@ -385,7 +386,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \n #ifndef NO_PTHREADS\n \tif (use_threads) {\n-\t\tadd_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, sha1);\n+\t\tadd_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);\n \t\tstrbuf_release(&pathbuf);\n \t\treturn 0;\n \t} else\n@@ -394,7 +395,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \t\tstruct grep_source gs;\n \t\tint hit;\n \n-\t\tgrep_source_init(&gs, GREP_SOURCE_SHA1, pathbuf.buf, sha1);\n+\t\tgrep_source_init(&gs, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);\n \t\tstrbuf_release(&pathbuf);\n \t\thit = grep_source(opt, &gs);\n \n@@ -414,7 +415,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \n #ifndef NO_PTHREADS\n \tif (use_threads) {\n-\t\tadd_work(opt, GREP_SOURCE_FILE, buf.buf, filename);\n+\t\tadd_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);\n \t\tstrbuf_release(&buf);\n \t\treturn 0;\n \t} else\n@@ -423,7 +424,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\tstruct grep_source gs;\n \t\tint hit;\n \n-\t\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename);\n+\t\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);\n \t\tstrbuf_release(&buf);\n \t\thit = grep_source(opt, &gs);\n \n@@ -479,7 +480,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n \t\tif (cached || (ce->ce_flags & CE_VALID) || ce_skip_worktree(ce)) {\n \t\t\tif (ce_stage(ce))\n \t\t\t\tcontinue;\n-\t\t\thit |= grep_sha1(opt, ce->sha1, ce->name, 0);\n+\t\t\thit |= grep_sha1(opt, ce->sha1, ce->name, 0, ce->name);\n \t\t}\n \t\telse\n \t\t\thit |= grep_file(opt, ce->name);\n@@ -518,7 +519,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\tstrbuf_add(base, entry.path, te_len);\n \n \t\tif (S_ISREG(entry.mode)) {\n-\t\t\thit |= grep_sha1(opt, entry.sha1, base->buf, tn_len);\n+\t\t\thit |= grep_sha1(opt, entry.sha1, base->buf, tn_len,\n+\t\t\t\t\t base->buf + tn_len);\n \t\t}\n \t\telse if (S_ISDIR(entry.mode)) {\n \t\t\tenum object_type type;\n@@ -548,7 +550,7 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t       struct object *obj, const char *name)\n {\n \tif (obj->type == OBJ_BLOB)\n-\t\treturn grep_sha1(opt, obj->sha1, name, 0);\n+\t\treturn grep_sha1(opt, obj->sha1, name, 0, NULL);\n \tif (obj->type == OBJ_COMMIT || obj->type == OBJ_TREE) {\n \t\tstruct tree_desc tree;\n \t\tvoid *data;\ndiff --git a/grep.c b/grep.c\nindex edc7776..e36c01b 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1373,7 +1373,7 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n \tstruct grep_source gs;\n \tint r;\n \n-\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL);\n+\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);\n \tgs.buf = buf;\n \tgs.size = size;\n \n@@ -1384,10 +1384,12 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n }\n \n void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n-\t\t      const char *name, const void *identifier)\n+\t\t      const char *name, const char *path,\n+\t\t      const void *identifier)\n {\n \tgs->type = type;\n \tgs->name = name ? xstrdup(name) : NULL;\n+\tgs->path = path ? xstrdup(path) : NULL;\n \tgs->buf = NULL;\n \tgs->size = 0;\n \tgs->driver = NULL;\n@@ -1409,6 +1411,8 @@ void grep_source_clear(struct grep_source *gs)\n {\n \tfree(gs->name);\n \tgs->name = NULL;\n+\tfree(gs->path);\n+\tgs->path = NULL;\n \tfree(gs->identifier);\n \tgs->identifier = NULL;\n \tgrep_source_clear_data(gs);\n@@ -1501,7 +1505,8 @@ void grep_source_load_driver(struct grep_source *gs)\n \t\treturn;\n \n \tgrep_attr_lock();\n-\tgs->driver = userdiff_find_by_path(gs->name);\n+\tif (gs->path)\n+\t\tgs->driver = userdiff_find_by_path(gs->path);\n \tif (!gs->driver)\n \t\tgs->driver = userdiff_find_by_name(\"default\");\n \tgrep_attr_unlock();\ndiff --git a/grep.h b/grep.h\nindex c256ac6..c2cf57b 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -158,11 +158,13 @@ struct grep_source {\n \tchar *buf;\n \tunsigned long size;\n \n+\tchar *path; /* for attribute lookups */\n \tstruct userdiff_driver *driver;\n };\n \n void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n-\t\t      const char *name, const void *identifier);\n+\t\t      const char *name, const char *path,\n+\t\t      const void *identifier);\n void grep_source_clear_data(struct grep_source *gs);\n void grep_source_clear(struct grep_source *gs);\n void grep_source_load_driver(struct grep_source *gs);\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex fd6410f..2c85a30 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -111,6 +111,28 @@ test_expect_success 'grep respects binary diff attribute' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'grep --cached respects binary diff attribute' '\n+\tgit grep --cached text t >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --cached respects binary diff attribute (2)' '\n+\tgit add .gitattributes &&\n+\trm .gitattributes &&\n+\tgit grep --cached text t >actual &&\n+\ttest_cmp expect actual &&\n+\tgit checkout .gitattributes &&\n+\tgit rm --cached .gitattributes\n+'\n+\n+test_expect_success 'grep tree respects binary diff attribute' '\n+\tgit commit -m new &&\n+\techo \"Binary file HEAD:t matches\" >expect &&\n+\tgit grep text HEAD -- t >actual &&\n+\ttest_cmp expect actual &&\n+\tgit reset HEAD^\n+'\n+\n test_expect_success 'grep respects not-binary diff attribute' '\n \techo binQary | q_to_nul >b &&\n \tgit add b &&\n-- \n1.7.12.1.406.g6ab07c4\n"},{"id":"200926","messageId":"50758477.3030304@viscovery.net","threadId":"31765","inReplyTo":"1349877544-17648-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-10T14:21:43Z","receivedAt":"2012-10-10T14:21:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/10/2012 15:59, schrieb Nguyễn Thái Ngọc Duy:\n> This patch passes real paths down to grep_source_load_driver(). Except\n> grepping a blob, all other cases should have right paths down to\n\n... grepping a blob or tree object...\n\n> grep_source_load_driver(). In other words, .gitattributes are still\n> respected.\n...\n> +test_expect_success 'grep --cached respects binary diff attribute (2)' '\n> +\tgit add .gitattributes &&\n> +\trm .gitattributes &&\n> +\tgit grep --cached text t >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit checkout .gitattributes &&\n> +\tgit rm --cached .gitattributes\n> +'\n\nThis should perhaps be test_when_finished \"git rm --cached .gitattributes\".\n\n> +\n> +test_expect_success 'grep tree respects binary diff attribute' '\n\nI was confused by the word \"tree\" here. Isn't \"pathspec\" more correct?\n\n> +\tgit commit -m new &&\n> +\techo \"Binary file HEAD:t matches\" >expect &&\n> +\tgit grep text HEAD -- t >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit reset HEAD^\n> +'\n\nAnd in yet another test, should\n\n\tgit grep text HEAD:t\n\n/not/ respect the binary attribute?\n\nAt any rate, I don't observe the warnings anymore with this series.\n\nThanks,\n-- Hannes\n"},{"id":"200945","messageId":"7vy5jeaz81.fsf@alter.siamese.dyndns.org","threadId":"31765","inReplyTo":"50756646.5060505@viscovery.net","subject":"Re: [PATCH 3/3] grep: stop looking at random places for .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-10T19:44:46Z","receivedAt":"2012-10-10T19:44:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Is there already an established definition which the \"correct\"\n> .gitattributes are? IIRC, everywhere else we are looking at the\n> .gitattributes in the worktree, regardless of whether the object at the\n> path in question is in the worktree, the index, or in an old commit.\n\nNo, and it is deliberately kept vague while waiting for us to come\nup with a clear definition of what is \"correct\".\n\nWe could declare, from a purist's point of view, that the attribute\nshould be taken from the same place as the path in question is taken\nfrom.  When running \"git add foo.c\", we grab the contents of \"foo.c\"\nfrom the working tree, so \".gitignore\" from the working tree should\nbe applied when dealing with \"foo.c\".  Similarly, the contents of\nblob \"foo.c\" that \"git checkout foo.c\" reads from the index would\nget attributes from \".gitignore\" in the index (to find what its\nsmudging semantics is) before it gets written out to the working\ntree.  \"git diff A B\" may give the attributes from tree A to the\npreimage side while using the attributes from tree B to the\npostimage side.\n\nBut the last example has some practical issues.  Very often, people\nretroactively define attributes to correct earlier mistakes.  If an\nolder tree A forgot to declare that a path mybank.gnucash is a\nGnuCash ledger file, while a newer tree B (and the current checkout\nthat is even newer) does [*1*], it is more useful to apply the newer\ndefinition from .gitattributes to both trees in practice (and in\npractice, you are much less likely to have a check-out of ancient\ntree while running \"git diff A B\" to compare two trees that are\nnewer than the current check-out).  Using the file from the working\ntree is the best approximation of \"we want to use the newer one\",\nboth from the semantics (i.e. you are likely to have fresher tree\nchecked out) and the performance (i.e. reading from files in the\nworking tree is far more trivial than reading from historical trees)\npoint of view.\n\nSo it is not so cut-and-dried that \"take the attributes from the\nsame place\" is a good and \"correct\" definition [*2*].\n\n\n[Footnote]\n\n*1* GnuCash writes, by default, a gzip compressed xml file, so I\nhave in my .gitattributes file\n\n\t*.gnucash\tfilter=gnucash\n\nand then in my .git/config\n\n\t[filter \"gnucash\"]\n        \tclean = gzip -dc\n                smudge = gzip -c\n\nThis allows \"git diff\" to work reasonably well (if you do not mind\nreading diff between two versions of xml files, that is) and also\nhelps delta compression when packing the repository.\n\n\n*2* Besides, the attributes are primarily used to define the\nsemantics about the contents in question.  If one file is of\n\"gnucash\" kind (i.e. has \"filter=gnucash\" attribute in the previous\nexample) in one tree, and the path is of a different kind\n(e.g. \"filter=ooo\" that says \"this is an Ooo file\"), it is very\nlikely that it does not even make sense, with or without content\nfiltering, to compare them with \"git diff\", so \"take the attributes\nfrom the same place\" would have to imply \"if the attributes do not\nmatch, say something similar to 'Binary files differ'\", which is\njust as useless as applying one attribute taken from a convenient\nbut random place (i.e. the working tree).\n"},{"id":"200946","messageId":"7vtxu2ayov.fsf@alter.siamese.dyndns.org","threadId":"31765","inReplyTo":"50758477.3030304@viscovery.net","subject":"Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-10T19:56:16Z","receivedAt":"2012-10-10T19:56:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> At any rate, I don't observe the warnings anymore with this series.\n\nWhat kind of warnings have you been getting?  Earlier we had a bug\nin the jk/config-warn-on-inaccessible-paths series that made it warn\nwhen we tried to open a .gitattribute file and open() returned an\nerror other than ENOENT.  The bug was that we saw unnecessary errors\nwhen a directory that used to exist no longer exists in the working\ntree; we would instead get ENOTDIR in such a case that needs to be\nignored.\n\nThe problem was supposed to be \"fixed\" by 8e950da (attr: failure to\nopen a \".gitattributes\" file is OK with ENOTDIR, 2012-09-13); if you\nare still seeing the error, that error still may need to be\naddressed, regardless of Nguyễn's patch.\n"},{"id":"200954","messageId":"7vd30qav4m.fsf@alter.siamese.dyndns.org","threadId":"31765","inReplyTo":"1349868894-3579-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/3] quote: let caller reset buffer for quote_path_relative()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-10T21:13:13Z","receivedAt":"2012-10-10T21:13:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> quote_path_relative() resetting output buffer is sometimes unnecessary\n> as the buffer has never been used, and some other times makes it\n> harder for the caller to use (see builtin/grep.c, the caller has to\n> insert a string after quote_path_relative)\n>\n> Move the buffer reset back to call sites when necessary.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  The answer for Jeff's XXX in his patch, why strbuf_insert() instead\n>  of just adding in advance.\n\nThis sounds a lot stronger than \"let\" to me.  All existing callers\nthat assumed that buf to be emptied by the function now has to clear\nit.  \"quote: stop resetting output buffer of quote_path_relative()\"\nmay better describe what this really does.\n\nHow should this interact with the logic in the called function that\nused to say \"if we ended up returning an empty string because the\npath is the same as the base, we should give ./ back\", and what\nshould the return value of this function be?\n\nTo answer these questions, you must define the meaning of the string\nin the output buffer that already exists when the function is\ncalled.  If the caller did this:\n\n\tstrbuf_addstr(&out, \"The path relative to your HOME is: \");\n        quote_path_relative(path, pathlen, &out, \"/home/pclouds/\");\n\nthen the answers are \"We still need to add ./ but !out->len is no\nlonger a good test to decide\" and \"It should point at the first byte\nof what we added, not out->buf\".\n\nBut if the caller did this instead:\n\n\tsrcdir = \"/src/\";\n\tstrbuf_addstr(&dst, \"/dst/\");\n        quote_path_relative(path, pathlen, &dst, srcdir);\n        printf(\"cp '%s' '%s'\\n\", path, dst->buf);\n\nthen it is nonsensical to add \"./\" when out->len is not zero when\nthe function returns.\n\nSo what does it mean to have an existing string in the output buffer\nwhen calling the function?  Is it supposed to be a path to a\ndirectory, or just a general uninterpreted string (e.g. a message)?\n"},{"id":"200967","messageId":"CACsJy8CoedQBm4xyfhZX1mNTXpLMHxugygZUqOoZS8V5WVztGg@mail.gmail.com","threadId":"31765","inReplyTo":"50758477.3030304@viscovery.net","subject":"Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-11T01:49:27Z","receivedAt":"2012-10-11T01:49:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Oct 10, 2012 at 9:21 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n>> +     git commit -m new &&\n>> +     echo \"Binary file HEAD:t matches\" >expect &&\n>> +     git grep text HEAD -- t >actual &&\n>> +     test_cmp expect actual &&\n>> +     git reset HEAD^\n>> +'\n>\n> And in yet another test, should\n>\n>         git grep text HEAD:t\n>\n> /not/ respect the binary attribute?\n\nGray area. Is it ok to do that without documenting it (i.e. common\nsense)? I have something in mind that could do that, but it also makes\n\"git grep text HEAD^{tree}\" not respect attributes.\n-- \nDuy\n"},{"id":"200969","messageId":"7v8vbdaecl.fsf@alter.siamese.dyndns.org","threadId":"31765","inReplyTo":"CACsJy8CoedQBm4xyfhZX1mNTXpLMHxugygZUqOoZS8V5WVztGg@mail.gmail.com","subject":"Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-11T03:15:38Z","receivedAt":"2012-10-11T03:15:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> On Wed, Oct 10, 2012 at 9:21 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n>>> +     git commit -m new &&\n>>> +     echo \"Binary file HEAD:t matches\" >expect &&\n>>> +     git grep text HEAD -- t >actual &&\n>>> +     test_cmp expect actual &&\n>>> +     git reset HEAD^\n>>> +'\n>>\n>> And in yet another test, should\n>>\n>>         git grep text HEAD:t\n>>\n>> /not/ respect the binary attribute?\n>\n> Gray area. Is it ok to do that without documenting it (i.e. common\n> sense)? I have something in mind that could do that, but it also makes\n> \"git grep text HEAD^{tree}\" not respect attributes.\n\nPersonally, I do not think HEAD:t is worth worrying about.\n\nWe could use the get_sha1_with_context() to get \"t\" out of \"HEAD:t\",\nand we could even enhance get_sha1_with_context() to also preserve\nthe value of what came before the colon, but that would mean that\nthese three\n\n    git grep text HEAD:t/t0200\n    git grep text $(git rev-parse HEAD:t/t0200)\n    git grep text $(git rev-parse HEAD:t):t0200\n\nwould behave differently; only the first one has any chance of\napplying the correct set of \".gitattributes\".  All of them would be\nable to use the \".gitattributes\" file contained in the tree object\nthat corresponds to t/t0200 (if we updated attr.c to read from tree\nobjects, that is), but the latter two would skip \".gitattributes\"\nfiles from the top-level and \"t\" directories, still using the final\nfallback definition from $GIT_DIR/info/attributes file.\n\nIf we have to draw a line somewhere, the saner place to draw it is\nto stop at\n\n    git grep text HEAD -- t/t0200\n"},{"id":"200974","messageId":"50765CDC.8020509@viscovery.net","threadId":"31765","inReplyTo":"7vtxu2ayov.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-11T05:45:00Z","receivedAt":"2012-10-11T05:45:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/10/2012 21:56, schrieb Junio C Hamano:\n> Johannes Sixt <j.sixt@viscovery.net> writes:\n> \n>> At any rate, I don't observe the warnings anymore with this series.\n> \n> What kind of warnings have you been getting?  Earlier we had a bug\n> in the jk/config-warn-on-inaccessible-paths series that made it warn\n> when we tried to open a .gitattribute file and open() returned an\n> error other than ENOENT.  The bug was that we saw unnecessary errors\n> when a directory that used to exist no longer exists in the working\n> tree; we would instead get ENOTDIR in such a case that needs to be\n> ignored.\n\nI saw EINVAL errors when 'git grep pattern rev' was run on Windows. The\nreason is that the code attempted to access \"rev:dir/.gitattributes\" in\nthe worktree, which is an invalid path on Windows due to the colon. The\nlack of this warning indicates that the attempts to access these files are\neliminated.\n\n-- Hannes\n"},{"id":"200975","messageId":"50765F34.1070108@viscovery.net","threadId":"31765","inReplyTo":"7vy5jeaz81.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] grep: stop looking at random places for .gitattributes","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-11T05:55:00Z","receivedAt":"2012-10-11T05:55:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/10/2012 21:44, schrieb Junio C Hamano:\n> Johannes Sixt <j.sixt@viscovery.net> writes:\n> \n>> Is there already an established definition which the \"correct\"\n>> .gitattributes are?\n> \n> No, and it is deliberately kept vague while waiting for us to come\n> up with a clear definition of what is \"correct\".\n...\n> Very often, people\n> retroactively define attributes to correct earlier mistakes.\n\nAbsolutely. I have Windows resource files that are Shift-JIS encoded\nchecked in long ago, and I want to retoactively declare them with\n\"encoding=Shift-JIS\" because I prefer to see Japanese script in gitk\nrather than gibberish.\n\n-- Hannes\n"},{"id":"200977","messageId":"50766F94.4020104@alum.mit.edu","threadId":"31765","inReplyTo":"50765F34.1070108@viscovery.net","subject":"Re: [PATCH 3/3] grep: stop looking at random places for .gitattributes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-10-11T07:04:52Z","receivedAt":"2012-10-11T07:04:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/11/2012 07:55 AM, Johannes Sixt wrote:\n> Am 10/10/2012 21:44, schrieb Junio C Hamano:\n>> Johannes Sixt <j.sixt@viscovery.net> writes:\n>>\n>>> Is there already an established definition which the \"correct\"\n>>> .gitattributes are?\n>>\n>> No, and it is deliberately kept vague while waiting for us to come\n>> up with a clear definition of what is \"correct\".\n> ...\n>> Very often, people\n>> retroactively define attributes to correct earlier mistakes.\n> \n> Absolutely. I have Windows resource files that are Shift-JIS encoded\n> checked in long ago, and I want to retoactively declare them with\n> \"encoding=Shift-JIS\" because I prefer to see Japanese script in gitk\n> rather than gibberish.\n\nMaybe I'm being too much of a purist, but I don't think that git should\nretroactively reinterpret history on its own initiative in a way that\nmight not be correct (e.g., maybe your encoding changed from ASCII to\nShift-JIS sometime in the past).  It would be more appropriate for this\nto happen only if explicitly requested by the user.  For example, why\ndon't you override the incorrect historical attributes via\n.git/info/attributes?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"200979","messageId":"CACsJy8AKT4VzYg_aBrK2UNAhv=yO=ddSFPdi3sZakD9UBc388w@mail.gmail.com","threadId":"31765","inReplyTo":"50766F94.4020104@alum.mit.edu","subject":"Re: [PATCH 3/3] grep: stop looking at random places for .gitattributes","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-11T08:17:45Z","receivedAt":"2012-10-11T08:17:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 11, 2012 at 2:04 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> Maybe I'm being too much of a purist, but I don't think that git should\n> retroactively reinterpret history on its own initiative in a way that\n> might not be correct (e.g., maybe your encoding changed from ASCII to\n> Shift-JIS sometime in the past).  It would be more appropriate for this\n> to happen only if explicitly requested by the user.  For example, why\n> don't you override the incorrect historical attributes via\n> .git/info/attributes?\n\nI think git-notes is a more appropriate place to correct these things.\nIf the incorrect commits are pruned, their notes can also be pruned.\nNo poluttion in $GIT_DIR/info/attr.. And i'm your side, no checking\nworktree without user's permission.\n-- \nDuy\n"},{"id":"200986","messageId":"CACsJy8CTGpDW46-37oX14+JxLxWmoz2f4mawr4QPkqCSunF7dQ@mail.gmail.com","threadId":"31765","inReplyTo":"7vd30qav4m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] quote: let caller reset buffer for quote_path_relative()","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-11T13:04:05Z","receivedAt":"2012-10-11T13:04:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 11, 2012 at 4:13 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> This sounds a lot stronger than \"let\" to me.  All existing callers\n> that assumed that buf to be emptied by the function now has to clear\n> it.  \"quote: stop resetting output buffer of quote_path_relative()\"\n> may better describe what this really does.\n>\n> How should this interact with the logic in the called function that\n> used to say \"if we ended up returning an empty string because the\n> path is the same as the base, we should give ./ back\", and what\n> should the return value of this function be?\n> ...\n> So what does it mean to have an existing string in the output buffer\n> when calling the function?  Is it supposed to be a path to a\n> directory, or just a general uninterpreted string (e.g. a message)?\n\nI prefer the KISS principle in this case: do not interpret existing string.\n-- \nDuy\n"},{"id":"200991","messageId":"7vpq4p80sn.fsf@alter.siamese.dyndns.org","threadId":"31765","inReplyTo":"50765CDC.8020509@viscovery.net","subject":"Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-11T15:51:20Z","receivedAt":"2012-10-11T15:51:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> I saw EINVAL errors when 'git grep pattern rev' was run on Windows. The\n> reason is that the code attempted to access \"rev:dir/.gitattributes\" in\n> the worktree, which is an invalid path on Windows due to the colon. The\n> lack of this warning indicates that the attempts to access these files are\n> eliminated.\n\nIt means that whenever we ask for attributes for a tracked file that\nis inside a directory whose name contains a colon, we would get the\nsame error on Windows, no?  Perhaps Windows may not let you create\nsuch a directory in the first place, but you may still get a\nrepository of a cross platform project that contains such a path.\n\nWhat I am wondering is if we should do something similar to 8e950da\n(attr: failure to open a .gitattributes file is OK with ENOTDIR,\n2012-09-13), at least on Windows, for EINVAL.\n"},{"id":"200995","messageId":"7v8vbd7yem.fsf@alter.siamese.dyndns.org","threadId":"31765","inReplyTo":"CACsJy8CTGpDW46-37oX14+JxLxWmoz2f4mawr4QPkqCSunF7dQ@mail.gmail.com","subject":"Re: [PATCH 1/3] quote: let caller reset buffer for quote_path_relative()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-11T16:42:57Z","receivedAt":"2012-10-11T16:42:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> On Thu, Oct 11, 2012 at 4:13 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> This sounds a lot stronger than \"let\" to me.  All existing callers\n>> that assumed that buf to be emptied by the function now has to clear\n>> it.  \"quote: stop resetting output buffer of quote_path_relative()\"\n>> may better describe what this really does.\n>>\n>> How should this interact with the logic in the called function that\n>> used to say \"if we ended up returning an empty string because the\n>> path is the same as the base, we should give ./ back\", and what\n>> should the return value of this function be?\n>> ...\n>> So what does it mean to have an existing string in the output buffer\n>> when calling the function?  Is it supposed to be a path to a\n>> directory, or just a general uninterpreted string (e.g. a message)?\n>\n> I prefer the KISS principle in this case: do not interpret existing string.\n\nSorry, I do not quite understand what you mean.\n\nThe original function is about turning one path into a path that is\nrelative to the other path (e.g. make the result usable for chdir()\nwhen your cwd() is the other path to name that path), and because\nyou do not want to end up with an empty path, it adds \"./\" to the\nresult when the two paths are equal.\n\nThat is simple and stupid, in the sense that you can explain in\nsimple terms what it does even to a stupid person and make him\nunderstand.\n\nIf a patch changes one aspect of the implementation of the function,\nbut keeps other parts intact without thinking the ramifications of\ndoing so through, and ends up giving the end result unnecessarily\ncomplex semantics, the _patch_ might be simple and stupid, but the\nend result is no longer simple.\n\nThat is why I asked what it means to \"append\" and asked you to write\nit down for people who may need to decide if this function is an\nappropriate thing to call for their new code in the future.\n\nYou didn't answer that question, so I have to ask again.  What is\nthe existing string this function appends its result to?\n\n - If it is a leading path (in other words, you are transplanting a\n   hierarchy to somewhere else --- see and (re)read \"But if the\n   caller did this instead\" part from the message you are responding\n   to), \"because you do not want to end up with an empty path, it\n   adds \"./\" to the result when the two paths are equal\" does not\n   make sense at all.\n\n - If it is a message that describes the resulting relative path\n   (the use case above that \"transplant\" example in the same\n   message), it needs to add \"./\" to the result, but the logic to\n   determine it now needs to take the length of what was in the\n   buffer when the function was called into account.\n\n - Is there a third possibility?\n\nWhatever your answer is, please make sure the next person who wants\nto call this function from his code does not have to ask \"Why does\nit append \"./\" when out->len is zero?  Shouldn't the condition be\nwhen *rel is an empty string?\"\n\nThanks.\n"},{"id":"201044","messageId":"5077C7AC.9010301@viscovery.net","threadId":"31765","inReplyTo":"7vpq4p80sn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-12T07:33:00Z","receivedAt":"2012-10-12T07:33:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/11/2012 17:51, schrieb Junio C Hamano:\n> Johannes Sixt <j.sixt@viscovery.net> writes:\n> \n>> I saw EINVAL errors when 'git grep pattern rev' was run on Windows. The\n>> reason is that the code attempted to access \"rev:dir/.gitattributes\" in\n>> the worktree, which is an invalid path on Windows due to the colon. The\n>> lack of this warning indicates that the attempts to access these files are\n>> eliminated.\n> \n> It means that whenever we ask for attributes for a tracked file that\n> is inside a directory whose name contains a colon, we would get the\n> same error on Windows, no?  Perhaps Windows may not let you create\n> such a directory in the first place, but you may still get a\n> repository of a cross platform project that contains such a path.\n\nYour assessment is correct.\n\n> What I am wondering is if we should do something similar to 8e950da\n> (attr: failure to open a .gitattributes file is OK with ENOTDIR,\n> 2012-09-13), at least on Windows, for EINVAL.\n\nIt might be worth it. We already have a similar special case in\nwrite_or_die.c:maybe_flush_or_die() for Windows, although it is not about\na colon in a path name.\n\nPerhaps like this.\n\n--- 8< ---\nFrom: Johannes Sixt <j6t@kdbg.org>\nSubject: [PATCH] attr: do not warn on path with colon on Windows\n\nIn the same spirit as commit 8e950dab (attr: failure to open a\n.gitattributes file is OK with ENOTDIR), ignore EINVAL errors. On\nWindows, a path that contains a colon that is not the one after the\ndrive letter is not allowed and is reported with errno set to\nEINVAL.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n attr.c | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/attr.c b/attr.c\nindex 8010429..ac945ad 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -354,7 +354,15 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)\n \tint lineno = 0;\n \n \tif (!fp) {\n-\t\tif (errno != ENOENT && errno != ENOTDIR)\n+\t\t/*\n+\t\t * If path does not exist, it is not worth mentioning,\n+\t\t * but I/O errors and permission errors are.\n+\t\t *\n+\t\t * On Windows, EINVAL is reported if path contains a colon\n+\t\t * that is not the driver letter separator. Such a path\n+\t\t * cannot exist in the file system and is also uninteresting.\n+\t\t */\n+\t\tif (errno != ENOENT && errno != ENOTDIR && errno != EINVAL)\n \t\t\twarn_on_inaccessible(path);\n \t\treturn NULL;\n \t}\n-- \n1.8.0.rc2.1237.g5522246\n"},{"id":"201052","messageId":"1350038978-26153-1-git-send-email-pclouds@gmail.com","threadId":"31765","inReplyTo":"1349877544-17648-3-git-send-email-pclouds@gmail.com","subject":"[PATCH v3] grep: stop looking at random places for .gitattributes","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-10-12T10:49:38Z","receivedAt":"2012-10-12T10:49:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"grep searches for .gitattributes using \"name\" field in struct\ngrep_source but that field is not real on-disk path name. For example,\n\"grep pattern rev\" fills the field with \"rev:path\", and Git looks for\n.gitattributes in the (non-existent but exploitable) path \"rev:path\"\ninstead of \"path\".\n\nThis patch passes real paths down to grep_source_load_driver() when:\n\n - grep on work tree\n - grep on the index\n - grep a commit (or a tag if it points to a commit)\n\nso that these cases look up .gitattributes at proper paths.\n.gitattributes lookup is disabled in all other cases.\n\nInitial-work-by: Jeff King <peff@peff.net>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n This fixes t7008 as Johannes commented and makes \"git grep foo HEAD:t\"\n not look up worktree's .gitattributes (in fact it does not look up\n anywhere).\n\n The quote_path_relative() patch is not required, it's an independent\n design issue.\n\n builtin/grep.c         | 31 ++++++++++++++++++-------------\n grep.c                 | 11 ++++++++---\n grep.h                 |  4 +++-\n t/t7008-grep-binary.sh | 22 ++++++++++++++++++++++\n 4 files changed, 51 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 82530a6..38a17eb 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -86,7 +86,7 @@ static pthread_cond_t cond_result;\n static int skip_first_line;\n \n static void add_work(struct grep_opt *opt, enum grep_source_type type,\n-\t\t     const char *name, const void *id)\n+\t\t     const char *name, const char *path, const void *id)\n {\n \tgrep_lock();\n \n@@ -94,7 +94,7 @@ static void add_work(struct grep_opt *opt, enum grep_source_type type,\n \t\tpthread_cond_wait(&cond_write, &grep_mutex);\n \t}\n \n-\tgrep_source_init(&todo[todo_end].source, type, name, id);\n+\tgrep_source_init(&todo[todo_end].source, type, name, path, id);\n \tif (opt->binary != GREP_BINARY_TEXT)\n \t\tgrep_source_load_driver(&todo[todo_end].source);\n \ttodo[todo_end].done = 0;\n@@ -371,7 +371,8 @@ static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type\n }\n \n static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n-\t\t     const char *filename, int tree_name_len)\n+\t\t     const char *filename, int tree_name_len,\n+\t\t     const char *path)\n {\n \tstruct strbuf pathbuf = STRBUF_INIT;\n \n@@ -385,7 +386,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \n #ifndef NO_PTHREADS\n \tif (use_threads) {\n-\t\tadd_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, sha1);\n+\t\tadd_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);\n \t\tstrbuf_release(&pathbuf);\n \t\treturn 0;\n \t} else\n@@ -394,7 +395,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \t\tstruct grep_source gs;\n \t\tint hit;\n \n-\t\tgrep_source_init(&gs, GREP_SOURCE_SHA1, pathbuf.buf, sha1);\n+\t\tgrep_source_init(&gs, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);\n \t\tstrbuf_release(&pathbuf);\n \t\thit = grep_source(opt, &gs);\n \n@@ -414,7 +415,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \n #ifndef NO_PTHREADS\n \tif (use_threads) {\n-\t\tadd_work(opt, GREP_SOURCE_FILE, buf.buf, filename);\n+\t\tadd_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);\n \t\tstrbuf_release(&buf);\n \t\treturn 0;\n \t} else\n@@ -423,7 +424,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\tstruct grep_source gs;\n \t\tint hit;\n \n-\t\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename);\n+\t\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);\n \t\tstrbuf_release(&buf);\n \t\thit = grep_source(opt, &gs);\n \n@@ -479,7 +480,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n \t\tif (cached || (ce->ce_flags & CE_VALID) || ce_skip_worktree(ce)) {\n \t\t\tif (ce_stage(ce))\n \t\t\t\tcontinue;\n-\t\t\thit |= grep_sha1(opt, ce->sha1, ce->name, 0);\n+\t\t\thit |= grep_sha1(opt, ce->sha1, ce->name, 0, ce->name);\n \t\t}\n \t\telse\n \t\t\thit |= grep_file(opt, ce->name);\n@@ -497,7 +498,8 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n }\n \n static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n-\t\t     struct tree_desc *tree, struct strbuf *base, int tn_len)\n+\t\t     struct tree_desc *tree, struct strbuf *base, int tn_len,\n+\t\t     int check_attr)\n {\n \tint hit = 0;\n \tenum interesting match = entry_not_interesting;\n@@ -518,7 +520,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\tstrbuf_add(base, entry.path, te_len);\n \n \t\tif (S_ISREG(entry.mode)) {\n-\t\t\thit |= grep_sha1(opt, entry.sha1, base->buf, tn_len);\n+\t\t\thit |= grep_sha1(opt, entry.sha1, base->buf, tn_len,\n+\t\t\t\t\t check_attr ? base->buf + tn_len : NULL);\n \t\t}\n \t\telse if (S_ISDIR(entry.mode)) {\n \t\t\tenum object_type type;\n@@ -533,7 +536,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \n \t\t\tstrbuf_addch(base, '/');\n \t\t\tinit_tree_desc(&sub, data, size);\n-\t\t\thit |= grep_tree(opt, pathspec, &sub, base, tn_len);\n+\t\t\thit |= grep_tree(opt, pathspec, &sub, base, tn_len,\n+\t\t\t\t\t check_attr);\n \t\t\tfree(data);\n \t\t}\n \t\tstrbuf_setlen(base, old_baselen);\n@@ -548,7 +552,7 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t       struct object *obj, const char *name)\n {\n \tif (obj->type == OBJ_BLOB)\n-\t\treturn grep_sha1(opt, obj->sha1, name, 0);\n+\t\treturn grep_sha1(opt, obj->sha1, name, 0, NULL);\n \tif (obj->type == OBJ_COMMIT || obj->type == OBJ_TREE) {\n \t\tstruct tree_desc tree;\n \t\tvoid *data;\n@@ -571,7 +575,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t\tstrbuf_addch(&base, ':');\n \t\t}\n \t\tinit_tree_desc(&tree, data, size);\n-\t\thit = grep_tree(opt, pathspec, &tree, &base, base.len);\n+\t\thit = grep_tree(opt, pathspec, &tree, &base, base.len,\n+\t\t\t\tobj->type == OBJ_COMMIT);\n \t\tstrbuf_release(&base);\n \t\tfree(data);\n \t\treturn hit;\ndiff --git a/grep.c b/grep.c\nindex edc7776..e36c01b 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1373,7 +1373,7 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n \tstruct grep_source gs;\n \tint r;\n \n-\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL);\n+\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);\n \tgs.buf = buf;\n \tgs.size = size;\n \n@@ -1384,10 +1384,12 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n }\n \n void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n-\t\t      const char *name, const void *identifier)\n+\t\t      const char *name, const char *path,\n+\t\t      const void *identifier)\n {\n \tgs->type = type;\n \tgs->name = name ? xstrdup(name) : NULL;\n+\tgs->path = path ? xstrdup(path) : NULL;\n \tgs->buf = NULL;\n \tgs->size = 0;\n \tgs->driver = NULL;\n@@ -1409,6 +1411,8 @@ void grep_source_clear(struct grep_source *gs)\n {\n \tfree(gs->name);\n \tgs->name = NULL;\n+\tfree(gs->path);\n+\tgs->path = NULL;\n \tfree(gs->identifier);\n \tgs->identifier = NULL;\n \tgrep_source_clear_data(gs);\n@@ -1501,7 +1505,8 @@ void grep_source_load_driver(struct grep_source *gs)\n \t\treturn;\n \n \tgrep_attr_lock();\n-\tgs->driver = userdiff_find_by_path(gs->name);\n+\tif (gs->path)\n+\t\tgs->driver = userdiff_find_by_path(gs->path);\n \tif (!gs->driver)\n \t\tgs->driver = userdiff_find_by_name(\"default\");\n \tgrep_attr_unlock();\ndiff --git a/grep.h b/grep.h\nindex c256ac6..c2cf57b 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -158,11 +158,13 @@ struct grep_source {\n \tchar *buf;\n \tunsigned long size;\n \n+\tchar *path; /* for attribute lookups */\n \tstruct userdiff_driver *driver;\n };\n \n void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n-\t\t      const char *name, const void *identifier);\n+\t\t      const char *name, const char *path,\n+\t\t      const void *identifier);\n void grep_source_clear_data(struct grep_source *gs);\n void grep_source_clear(struct grep_source *gs);\n void grep_source_load_driver(struct grep_source *gs);\ndiff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh\nindex fd6410f..26f8319 100755\n--- a/t/t7008-grep-binary.sh\n+++ b/t/t7008-grep-binary.sh\n@@ -111,6 +111,28 @@ test_expect_success 'grep respects binary diff attribute' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'grep --cached respects binary diff attribute' '\n+\tgit grep --cached text t >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --cached respects binary diff attribute (2)' '\n+\tgit add .gitattributes &&\n+\trm .gitattributes &&\n+\tgit grep --cached text t >actual &&\n+\ttest_when_finished \"git rm --cached .gitattributes\" &&\n+\ttest_when_finished \"git checkout .gitattributes\" &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep revision respects binary diff attribute' '\n+\tgit commit -m new &&\n+\techo \"Binary file HEAD:t matches\" >expect &&\n+\tgit grep text HEAD -- t >actual &&\n+\ttest_when_finished \"git reset HEAD^\" &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'grep respects not-binary diff attribute' '\n \techo binQary | q_to_nul >b &&\n \tgit add b &&\n-- \n1.7.12.1.406.g6ab07c4\n"},{"id":"201066","messageId":"7vfw5j4oz8.fsf@alter.siamese.dyndns.org","threadId":"31765","inReplyTo":"1350038978-26153-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3] grep: stop looking at random places for .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-12T16:47:07Z","receivedAt":"2012-10-12T16:47:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n\n> grep searches for .gitattributes using \"name\" field in struct\n> grep_source but that field is not real on-disk path name. For example,\n> \"grep pattern rev\" fills the field with \"rev:path\", and Git looks for\n> .gitattributes in the (non-existent but exploitable) path \"rev:path\"\n> instead of \"path\".\n>\n> This patch passes real paths down to grep_source_load_driver() when:\n>\n>  - grep on work tree\n>  - grep on the index\n>  - grep a commit (or a tag if it points to a commit)\n>\n> so that these cases look up .gitattributes at proper paths.\n> .gitattributes lookup is disabled in all other cases.\n>\n> Initial-work-by: Jeff King <peff@peff.net>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n\nLooks sensible and straightforward.  Thanks.\n"},{"id":"201116","messageId":"7v4nlx3cc8.fsf@alter.siamese.dyndns.org","threadId":"31765","inReplyTo":"5077C7AC.9010301@viscovery.net","subject":"Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-14T04:29:59Z","receivedAt":"2012-10-14T04:29:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> It might be worth it. We already have a similar special case in\n> write_or_die.c:maybe_flush_or_die() for Windows, although it is not about\n> a colon in a path name.\n>\n> Perhaps like this.\n\nHrm, the \"we already have\" one b2f5e26 (Windows: Work around an\noddity when a pipe with no reader is written to., 2007-08-17) was\nwhat you added while I was looking the other way ;-) as a part of\nWindows specific pull.\n\nThat change, and this patch, seem to cover the cases to be ignored\nwith a bit too wide a net to my taste.  On other systems, and even\non Windows if the path does not have any colon, EINVAL is something\nwe would want to notice and report, as a potential problem, no?\n\n> --- 8< ---\n> From: Johannes Sixt <j6t@kdbg.org>\n> Subject: [PATCH] attr: do not warn on path with colon on Windows\n>\n> In the same spirit as commit 8e950dab (attr: failure to open a\n> .gitattributes file is OK with ENOTDIR), ignore EINVAL errors. On\n> Windows, a path that contains a colon that is not the one after the\n> drive letter is not allowed and is reported with errno set to\n> EINVAL.\n>\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n>  attr.c | 10 +++++++++-\n>  1 file changed, 9 insertions(+), 1 deletion(-)\n>\n> diff --git a/attr.c b/attr.c\n> index 8010429..ac945ad 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -354,7 +354,15 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)\n>  \tint lineno = 0;\n>  \n>  \tif (!fp) {\n> -\t\tif (errno != ENOENT && errno != ENOTDIR)\n> +\t\t/*\n> +\t\t * If path does not exist, it is not worth mentioning,\n> +\t\t * but I/O errors and permission errors are.\n> +\t\t *\n> +\t\t * On Windows, EINVAL is reported if path contains a colon\n> +\t\t * that is not the driver letter separator. Such a path\n> +\t\t * cannot exist in the file system and is also uninteresting.\n> +\t\t */\n> +\t\tif (errno != ENOENT && errno != ENOTDIR && errno != EINVAL)\n>  \t\t\twarn_on_inaccessible(path);\n>  \t\treturn NULL;\n>  \t}\n"},{"id":"201200","messageId":"507BA6F0.4090500@viscovery.net","threadId":"31765","inReplyTo":"7v4nlx3cc8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-15T06:02:24Z","receivedAt":"2012-10-15T06:02:24Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/14/2012 6:29, schrieb Junio C Hamano:\n> Johannes Sixt <j.sixt@viscovery.net> writes:\n> \n>> It might be worth it. We already have a similar special case in\n>> write_or_die.c:maybe_flush_or_die() for Windows, although it is not about\n>> a colon in a path name.\n>>\n>> Perhaps like this.\n> \n> Hrm, the \"we already have\" one b2f5e26 (Windows: Work around an\n> oddity when a pipe with no reader is written to., 2007-08-17) was\n> what you added while I was looking the other way ;-) as a part of\n> Windows specific pull.\n> \n> That change, and this patch, seem to cover the cases to be ignored\n> with a bit too wide a net to my taste.  On other systems, and even\n> on Windows if the path does not have any colon, EINVAL is something\n> we would want to noticbbe and report, as a potential problem, no?\n\nFor fopen(), EINVAL should occur only if the mode argument is wrong, which\nit isn't. For fflush() (as in write_or_die.c), EINVAL is not even listed\nas possible error code. Therefore, catching EINVAL wholesale should not be\na problem, IMO, at least not \"on other systems\".\n\nOn Windows, it is more problematic because there is a table of \"customary\"\nWindows API error codes, which are mapped to errno values, and EINVAL is\nused for all other Windows error codes (and for a few listed ones), and\nignoring EINVAL might indeed miss something worth to be reported.\n\nSooo... I don't mind if you do not pick up this patch because it handles a\nrather theoretic case, i.e., where a project with strange paths somehow\nended up on a Windows drive.\n\nBut reverting the EINVAL check from write_or_die.c is out of question,\nbecause that handles a real case.\n\n-- Hannes\n"},{"id":"201270","messageId":"7vfw5fy8tx.fsf@alter.siamese.dyndns.org","threadId":"31765","inReplyTo":"507BA6F0.4090500@viscovery.net","subject":"Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-15T16:54:34Z","receivedAt":"2012-10-15T16:54:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> For fflush() (as in write_or_die.c), EINVAL is not even listed\n> as possible error code. Therefore, catching EINVAL wholesale should not be\n> a problem, IMO, at least not \"on other systems\".\n\nI see a disturbing gap between \"EINVAL is not supposed to be even\npossible\" and \"therefore it is safe to ignore\".  Our usual attitude\ntowards catching errors is to ignore only specific things that are\nexpected to happen and known to be safe, e.g. the original code\nbefore that patch which special cased to ignore EPIPE with\n\n\tif (fflush(f)) {\n\t\tif (errno == EPIPE)\n                \texit(0);\n\t\tdie_errno(\"write failure on '%s'\", desc);\n\t}\n\nwhere we know we are often downstream of something else and\ndiagnosing EPIPE as an error is actively wrong.\n\nUnless you assume that *no* other platform has the same glitch as\nWindows has with respect to fflush(f) returning EINVAL, or any other\nplatform that may return EINVAL from fflush(f) has the exactly same\nglitch as Windows, ignoring EINVAL unconditionally everywhere is\nwrong.  Perhaps the next broken platform may return EINVAL when it\nshould return EIO or something.  Ideally, that earlier workaround\nshould have done a logica equivalent of:\n\n\n\tif (fflush(f)) {\n#ifdef WINDOWS\t\t\n\t\t/*\n\t\t * On Windows, EPIPE is returned only by the first write()\n\t\t * after the reading end has closed its handle; subsequent\n\t\t * write()s return EINVAL.\n\t\t */\n                if (errno == EINVAL && this is after a second write)\n\t\t\terrno = EPIPE;\n#endif\n\t\tif (errno == EPIPE)\n                \texit(0);\n\t\tdie_errno(\"write failure on '%s'\", desc);\n\t}\n\nand did so not in-line at the calling site but in a compat/ wrapper\nfor fflush() to eliminate the need for the ifdef.\n\n> But reverting the EINVAL check from write_or_die.c is out of question,\n> because that handles a real case.\n\nI am not saying we should not deal with EINVAL on Windows at all.  I\nam just saying ignoring EINVAL on other platforms is wrong, and\nthese two shouldn't have been mutually incompatible goals.\n"},{"id":"201315","messageId":"507D010A.8000904@viscovery.net","threadId":"31765","inReplyTo":"7vfw5fy8tx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-16T06:39:06Z","receivedAt":"2012-10-16T06:39:06Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/15/2012 18:54, schrieb Junio C Hamano:\n> Ideally, that earlier workaround\n> should have done a logica equivalent of:\n> ...\n> and did so not in-line at the calling site but in a compat/ wrapper\n> for fflush() to eliminate the need for the ifdef.\n\nFair enough.\n\n>> But reverting the EINVAL check from write_or_die.c is out of question,\n>> because that handles a real case.\n\nCorrection: I can't reproduce the error messages that this was working\naround anymore in a brief test. I'll revert the check locally and see what\nhappens.\n\n-- Hannes\n"},{"id":"201406","messageId":"507E58CF.2040803@viscovery.net","threadId":"31765","inReplyTo":"507D010A.8000904@viscovery.net","subject":"Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-17T07:05:51Z","receivedAt":"2012-10-17T07:05:51Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/16/2012 8:39, schrieb Johannes Sixt:\n> Am 10/15/2012 18:54, schrieb Junio C Hamano:\n>> Ideally, that earlier workaround\n>> should have done a logica equivalent of:\n>> ...\n>> and did so not in-line at the calling site but in a compat/ wrapper\n>> for fflush() to eliminate the need for the ifdef.\n> \n> Fair enough.\n> \n>>> But reverting the EINVAL check from write_or_die.c is out of question,\n>>> because that handles a real case.\n> \n> Correction: I can't reproduce the error messages that this was working\n> around anymore in a brief test. I'll revert the check locally and see what\n> happens.\n\nThe error is reproducible with this command on Windows:\n\n$ git rev-list HEAD | sed 1q\n42b333d8bb8709dfc5783dd4c44bdb6012c2c17d\nfatal: write failure on 'stdout': Invalid argument\n\nLet's do as you suggested.\n\n--- 8< ---\nFrom: Johannes Sixt <j6t@kdbg.org>\nSubject: [PATCH] maybe_flush_or_die: move a too-loose Windows specific error\n check to compat\n\nCommit b2f5e268 (Windows: Work around an oddity when a pipe with no reader\nis written to) introduced a check for EINVAL after fflush() to fight\nspurious \"Invalid argument\" errors on Windows when a pipe was broken. But\nthis check may hide real errors on systems that do not have the this odd\nbehavior. Introduce an fflush wrapper in compat/mingw.* so that the treatment\nis only applied on Windows.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n compat/mingw.c | 22 ++++++++++++++++++++++\n compat/mingw.h |  3 +++\n write_or_die.c |  7 +------\n 3 files changed, 26 insertions(+), 6 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex afc892d..4e63838 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -335,6 +335,28 @@ FILE *mingw_freopen (const char *filename, const char *otype, FILE *stream)\n \treturn freopen(filename, otype, stream);\n }\n \n+#undef fflush\n+int mingw_fflush(FILE *stream)\n+{\n+\tint ret = fflush(stream);\n+\n+\t/*\n+\t * write() is used behind the scenes of stdio output functions.\n+\t * Since git code does not check for errors after each stdio write\n+\t * operation, it can happen that write() is called by a later\n+\t * stdio function even if an earlier write() call failed. In the\n+\t * case of a pipe whose readable end was closed, only the first\n+\t * call to write() reports EPIPE on Windows. Subsequent write()\n+\t * calls report EINVAL. It is impossible to notice whether this\n+\t * fflush invocation triggered such a case, therefore, we have to\n+\t * catch all EINVAL errors whole-sale.\n+\t */\n+\tif (ret && errno == EINVAL)\n+\t\terrno = EPIPE;\n+\n+\treturn ret;\n+}\n+\n /*\n  * The unit of FILETIME is 100-nanoseconds since January 1, 1601, UTC.\n  * Returns the 100-nanoseconds (\"hekto nanoseconds\") since the epoch.\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 61a6521..eeb08d1 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -185,6 +185,9 @@ FILE *mingw_fopen (const char *filename, const char *otype);\n FILE *mingw_freopen (const char *filename, const char *otype, FILE *stream);\n #define freopen mingw_freopen\n \n+int mingw_fflush(FILE *stream);\n+#define fflush mingw_fflush\n+\n char *mingw_getcwd(char *pointer, int len);\n #define getcwd mingw_getcwd\n \ndiff --git a/write_or_die.c b/write_or_die.c\nindex d45b536..960f448 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -34,12 +34,7 @@ void maybe_flush_or_die(FILE *f, const char *desc)\n \t\t\treturn;\n \t}\n \tif (fflush(f)) {\n-\t\t/*\n-\t\t * On Windows, EPIPE is returned only by the first write()\n-\t\t * after the reading end has closed its handle; subsequent\n-\t\t * write()s return EINVAL.\n-\t\t */\n-\t\tif (errno == EPIPE || errno == EINVAL)\n+\t\tif (errno == EPIPE)\n \t\t\texit(0);\n \t\tdie_errno(\"write failure on '%s'\", desc);\n \t}\n-- \n1.8.0.rc2.1248.g3f5b13f\n"},{"id":"201410","messageId":"7vk3uppn7r.fsf@alter.siamese.dyndns.org","threadId":"31765","inReplyTo":"507E58CF.2040803@viscovery.net","subject":"Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-17T07:33:12Z","receivedAt":"2012-10-17T07:33:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index afc892d..4e63838 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -335,6 +335,28 @@ FILE *mingw_freopen (const char *filename, const char *otype, FILE *stream)\n>  \treturn freopen(filename, otype, stream);\n>  }\n>  \n> +#undef fflush\n> +int mingw_fflush(FILE *stream)\n> +{\n> +\tint ret = fflush(stream);\n\nThe \"#undef\" above is a bit unfortunate.\n\nWhenever I see this construct I start to wonder \"I know this is to\ndisable our own #define we have elsewhere that renames fflush() to\nmingw_fflush(), but what happens if the system include implements\nfflush() as a macro?\"\n\nA better organization might be\n\n - make \"int mingw_fflush(FILE *);\" declaration available to all the\n   callers and to this part of the file; and\n\n - make \"#define fflush(x) mingw_fflush(x)\" macro visible when\n   compiling the rest of the system, but make it invisible to the\n   implementation of the emulation function.\n\nThe latter implies that a function in the emulation layer, if it\nneeds to fflush(), would explicitly call mingw_fflush().\n\nI know you did this knowing that it is not an issue on your\nplatform, and this file is only used on your platform anyway, so I\ndo not think we should address such a reorganization right now, but\nit is something we may want to keep an eye on, as other people may\nlater try to stub away a real macro imitating this part of the code.\n\nThanks for following through.\n\nSometimes discussions on our list result in participant feeling\nsatisified with the conclusion without completing the last mile of\nproducing and applying the patch, which I find only after a few\nmonth when I'm trawling the list archive for anything we missed.\n\nNow I'll have to do my part and queue this to my tree ;-)\n"}]}