From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 13 Sep 2012 19:40:39 GMT
Subject: Re: {bug} warning: unable to access 'RelNotes/.gitattributes'
Message-ID: <7vy5kd7lu0.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120913123740.GB4287@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Wed, Sep 12, 2012 at 11:32:22PM -0700, Junio C Hamano wrote:
>
>> "git repack" started giving the above warning, and I am guessing
>> that the recent 11e50b2 (attr: warn on inaccessible attribute files,
>> 2012-08-21) exposed a bug where we ask stat(2) not lstat(2) by
>> mistake before deciding to append .gitattributes to see if that
>> directory has a per-directory attributes file.
>
> Interesting. I don't get any such warning on repack. And RelNotes points
> to a file, so I'm not sure why stat() would make us think it was a dir.

Interesting.  The command in question is

 git-pack-objects --keep-true-parents --honor-pack-keep --non-empty \
    --all --reflog --delta-base-offset </dev/null .junk-pack

And pack-objects.c::no_try_delta() is given "RelNotes/1.7.4.txt" as
a path (which is very strange), and is trying to see if "-delta" is
set for the path.

Three problems:

 - "rev-list --object --all" does not produce "Relnotes/1.7.4.txt"
   (it does have "Documentation/RelNotes/1.7.4.txt", of course).
   Somebody in this callchain is screwing the name up.

 - Even if the name were correct, we are looking at the path that
   existed in the past.  The value of checking the attributes file
   in the working tree for "delta" attribute is dubious.

 - This is done while traversing the commit list and enumerating
   objects, so even if we have many incarnations of the same path in
   different commits, the attr stack mechanism would only help
   objects in the same directory in the same commit.  Perhaps we
   could do this after collecting all the blobs, check attributes
   for each path only once (in a sorted order so that we can take
   advantage of the attr stack), to reduce the cost of "delta"
   attribute check.

In any case, because the directory that used to exist to house the
blob in it may no longer exist, giving the warning on ENOTDIR that
your 11e50b2 (attr: warn on inaccessible attribute files,
2012-08-21) is a wrong thing to do (assuming that checking the
current attribute setting for historical tree is a sensible thing to
do, that is).

I could check for ENOTDIR to squelch the warning, but I think your
patch uncovered a lot deeper issues.


diff --git i/attr.c w/attr.c
index f12c83f..056d702 100644
--- i/attr.c
+++ w/attr.c
@@ -353,7 +353,7 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
 	int lineno = 0;
 
 	if (!fp) {
-		if (errno != ENOENT)
+		if (errno != ENOENT && errno != ENOTDIR)
 			warn_on_inaccessible(path);
 		return NULL;
 	}