{bug} warning: unable to access 'RelNotes/.gitattributes'
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. We simply used to notice and ignore any failure from open() and moved on, but we started distinguishing between ENOENT and others (in this case, we get ENOTDIR), and added a warning for non-ENOENT cases and I think that is what I am seeing. It is getting late so I won't dig it further for now, but just a heads up. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: {bug} warning: unable to access 'RelNotes/.gitattributes'
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. We simply used to notice and ignore any failure from open() and moved on, but we started distinguishing between ENOENT and others (in this case, we get ENOTDIR), and added a warning for non-ENOENT cases and I think that is what I am seeing. I can provoke such a warning by doing: git check-attr -a RelNotes/foo I haven't decided whether that's a good or bad thing. It makes sense, since the file you are asking for would get ENOTDIR, but maybe somebody is feeding junk to check-attr. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: {bug} warning: unable to access 'RelNotes/.gitattributes'
Jeff King p...@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;
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: {bug} warning: unable to access 'RelNotes/.gitattributes'
On Thu, Sep 13, 2012 at 12:40:39PM -0700, Junio C Hamano wrote:
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
Weird. I don't see any problems with that command, either (I tried it
with the current 'next'). Thinking that maybe delta reuse was getting in
the way, I also tried it with --no-reuse-delta.
- 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.
Yeah, that sounds like a pretty huge bug. But since I can't reproduce,
you're on your own for tracking it down.
- 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.
I don't think it's dubious. Imagine you had a bunch of binary files and
you did:
$ echo *.bin -delta .gitattributes
$ git repack -ad
You would expect it to affect all of the .bin files through history, no?
The real issue is that we should be much more lenient, because we have
no clue if the filename has any basis in the working tree.
While it's cool that the ENOTDIR warning has possibly found another bug,
I think in the long run we would want to ignore ENOTDIR along with
ENOENT to handle this situation (and I think it would be safe to do it
all the time, and not worry about this special case).
- 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.
That is a totally separate issue, but it might be a nice optimization.
A good start would be just running prof and seeing how much time we
spend on the attr stack now (I suspect it is really not much compared to
the actual packing, but maybe on systems with horribly slow stat() it
would be worse).
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 think that this:
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;
}
is the right thing to do. It's cool that it uncovered a bug in this
case, but it is easy to construct a non-bug case that would exhibit the
same bogus warning (just convert a directory into a file).
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
