{"thread":{"id":"31517","subject":"{bug} warning: unable to access 'RelNotes/.gitattributes'","startedAt":"2012-09-13T06:32:22Z","lastAt":"2012-09-14T01:28:32Z","messageCount":5,"participants":["Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"198900","messageId":"7vpq5qa0w9.fsf@alter.siamese.dyndns.org","threadId":"31517","inReplyTo":null,"subject":"{bug} warning: unable to access 'RelNotes/.gitattributes'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-13T06:32:22Z","receivedAt":"2012-09-13T06:32:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git repack\" started giving the above warning, and I am guessing\nthat the recent 11e50b2 (attr: warn on inaccessible attribute files,\n2012-08-21) exposed a bug where we ask stat(2) not lstat(2) by\nmistake before deciding to append .gitattributes to see if that\ndirectory has a per-directory attributes file. We simply used to\nnotice and ignore any failure from open() and moved on, but we\nstarted distinguishing between ENOENT and others (in this case, we\nget ENOTDIR), and added a warning for non-ENOENT cases and I think\nthat is what I am seeing.\n\nIt is getting late so I won't dig it further for now, but just a\nheads up.\n"},{"id":"198911","messageId":"20120913123740.GB4287@sigill.intra.peff.net","threadId":"31517","inReplyTo":"7vpq5qa0w9.fsf@alter.siamese.dyndns.org","subject":"Re: {bug} warning: unable to access 'RelNotes/.gitattributes'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-13T12:37:40Z","receivedAt":"2012-09-13T12:37:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 12, 2012 at 11:32:22PM -0700, Junio C Hamano wrote:\n\n> \"git repack\" started giving the above warning, and I am guessing\n> that the recent 11e50b2 (attr: warn on inaccessible attribute files,\n> 2012-08-21) exposed a bug where we ask stat(2) not lstat(2) by\n> mistake before deciding to append .gitattributes to see if that\n> directory has a per-directory attributes file.\n\nInteresting. I don't get any such warning on repack. And RelNotes points\nto a file, so I'm not sure why stat() would make us think it was a dir.\n\n> We simply used to notice and ignore any failure from open() and moved\n> on, but we started distinguishing between ENOENT and others (in this\n> case, we get ENOTDIR), and added a warning for non-ENOENT cases and I\n> think that is what I am seeing.\n\nI can provoke such a warning by doing:\n\n git check-attr -a RelNotes/foo\n\nI haven't decided whether that's a good or bad thing. It makes sense,\nsince the file you are asking for would get ENOTDIR, but maybe somebody\nis feeding junk to check-attr.\n\n-Peff\n"},{"id":"198960","messageId":"7vy5kd7lu0.fsf@alter.siamese.dyndns.org","threadId":"31517","inReplyTo":"20120913123740.GB4287@sigill.intra.peff.net","subject":"Re: {bug} warning: unable to access 'RelNotes/.gitattributes'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-13T19:40:39Z","receivedAt":"2012-09-13T19:40:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Sep 12, 2012 at 11:32:22PM -0700, Junio C Hamano wrote:\n>\n>> \"git repack\" started giving the above warning, and I am guessing\n>> that the recent 11e50b2 (attr: warn on inaccessible attribute files,\n>> 2012-08-21) exposed a bug where we ask stat(2) not lstat(2) by\n>> mistake before deciding to append .gitattributes to see if that\n>> directory has a per-directory attributes file.\n>\n> Interesting. I don't get any such warning on repack. And RelNotes points\n> to a file, so I'm not sure why stat() would make us think it was a dir.\n\nInteresting. The command in question is\n\n git-pack-objects --keep-true-parents --honor-pack-keep --non-empty \\\n --all --reflog --delta-base-offset > Interesting. I don't get any such warning on repack. And RelNotes points\n> > to a file, so I'm not sure why stat() would make us think it was a dir.\n> \n> Interesting. The command in question is\n> \n> git-pack-objects --keep-true-parents --honor-pack-keep --non-empty \\\n> --all --reflog --delta-base-offset - \"rev-list --object --all\" does not produce \"Relnotes/1.7.4.txt\"\n> (it does have \"Documentation/RelNotes/1.7.4.txt\", of course).\n> Somebody in this callchain is screwing the name up.\n\nYeah, that sounds like a pretty huge bug. But since I can't reproduce,\nyou're on your own for tracking it down.\n\n> - Even if the name were correct, we are looking at the path that\n> existed in the past. The value of checking the attributes file\n> in the working tree for \"delta\" attribute is dubious.\n\nI don't think it's dubious. Imagine you had a bunch of binary files and\nyou did:\n\n $ echo \"*.bin -delta\" >.gitattributes\n $ git repack -ad\n\nYou would expect it to affect all of the .bin files through history, no?\nThe real issue is that we should be much more lenient, because we have\nno clue if the filename has any basis in the working tree.\n\nWhile it's cool that the ENOTDIR warning has possibly found another bug,\nI think in the long run we would want to ignore ENOTDIR along with\nENOENT to handle this situation (and I think it would be safe to do it\nall the time, and not worry about this \"special\" case).\n\n> - This is done while traversing the commit list and enumerating\n> objects, so even if we have many incarnations of the same path in\n> different commits, the attr stack mechanism would only help\n> objects in the same directory in the same commit. Perhaps we\n> could do this after collecting all the blobs, check attributes\n> for each path only once (in a sorted order so that we can take\n> advantage of the attr stack), to reduce the cost of \"delta\"\n> attribute check.\n\nThat is a totally separate issue, but it might be a nice optimization.\nA good start would be just running \"prof\" and seeing how much time we\nspend on the attr stack now (I suspect it is really not much compared to\nthe actual packing, but maybe on systems with horribly slow stat() it\nwould be worse).\n\n> In any case, because the directory that used to exist to house the\n> blob in it may no longer exist, giving the warning on ENOTDIR that\n> your 11e50b2 (attr: warn on inaccessible attribute files,\n> 2012-08-21) is a wrong thing to do (assuming that checking the\n> current attribute setting for historical tree is a sensible thing to\n> do, that is).\n\nI think that this:\n\n> diff --git i/attr.c w/attr.c\n> index f12c83f..056d702 100644\n> --- i/attr.c\n> +++ w/attr.c\n> @@ -353,7 +353,7 @@ 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)\n> +\t\tif (errno != ENOENT && errno != ENOTDIR)\n> \t\t\twarn_on_inaccessible(path);\n> \t\treturn NULL;\n> \t}\n\nis the right thing to do. It's cool that it uncovered a bug in this\ncase, but it is easy to construct a non-bug case that would exhibit the\nsame bogus warning (just convert a directory into a file).\n\n-Peff\n"},{"id":"198986","messageId":"7vobl94clb.fsf@alter.siamese.dyndns.org","threadId":"31517","inReplyTo":"20120913211512.GA16968@sigill.intra.peff.net","subject":"Re: {bug} warning: unable to access 'RelNotes/.gitattributes'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-14T01:28:32Z","receivedAt":"2012-09-14T01:28:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Sep 13, 2012 at 12:40:39PM -0700, Junio C Hamano wrote:\n>\n>> > Interesting. I don't get any such warning on repack. And RelNotes points\n>> > to a file, so I'm not sure why stat() would make us think it was a dir.\n>> \n>> Interesting. The command in question is\n>> \n>> git-pack-objects --keep-true-parents --honor-pack-keep --non-empty \\\n>> --all --reflog --delta-base-offset \n> Weird. I don't see any problems with that command, either (I tried it\n> with the current 'next'). Thinking that maybe delta reuse was getting in\n> the way, I also tried it with --no-reuse-delta.\n>\n>> - \"rev-list --object --all\" does not produce \"Relnotes/1.7.4.txt\"\n>> (it does have \"Documentation/RelNotes/1.7.4.txt\", of course).\n>> Somebody in this callchain is screwing the name up.\n>\n> Yeah, that sounds like a pretty huge bug. But since I can't reproduce,\n> you're on your own for tracking it down.\n\nI have a remote tracking branch refs/remotes/repo/html that has the\npath RelNotes/1.7.4.txt at the top ;-) Depending on how traversal\ngoes, if the tree that represents that RelNotes directory in the html\ntree is found before the tree that represents Documentation/RelNotes \ndirectory in the main history at the corresponding commit, it is\nperfectly normal that we discover the blob as RelNotes/1.7.4.txt, so\nthere is no bug.\n\nSo among the three points I raised, the first one was a false issue,\nthe second one is real (we do look for attributes in the working\ntree for historical commit, or for a commit that does not belong to\nthe same lineage as the one that is currently checked out, hence we\nmust ignore ENOTDIR), and the third one is unrelated.\n\n> I think that this:\n>\n>> diff --git i/attr.c w/attr.c\n>> index f12c83f..056d702 100644\n>> --- i/attr.c\n>> +++ w/attr.c\n>> @@ -353,7 +353,7 @@ 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)\n>> +\t\tif (errno != ENOENT && errno != ENOTDIR)\n>> \t\t\twarn_on_inaccessible(path);\n>> \t\treturn NULL;\n>> \t}\n>\n> is the right thing to do. It's cool that it uncovered a bug in this\n> case, but it is easy to construct a non-bug case that would exhibit the\n> same bogus warning (just convert a directory into a file).\n\nYes.\n"}]}