"git status" should warn/error when it cannot lists a directory

"git status" should warn/error when it cannot lists a directory

[Andrew Wong]

When "git status" recurses a directory that isn't readable (but
executable), it should print out a warning/error. Currently, if there
are untracked files in these directories, git wouldn't be able to
discover them. Ideally, "git status" should return a non-zero exit
code as well.

The problem seems to be In read_directory_recursive() from dir.c. When
opendir() returns null, we continue on ignoring any error. Is there a
scenario where returning null is expected? We can simply call perror()
here, but it would be nice if we can propagate the error to the exit
code too. How would we do that?

Re: "git status" should warn/error when it cannot lists a directory

[Jeff King]

On Mon, Feb 02, 2015 at 11:58:33AM -0500, Andrew Wong wrote:

> When "git status" recurses a directory that isn't readable (but
> executable), it should print out a warning/error. Currently, if there
> are untracked files in these directories, git wouldn't be able to
> discover them. Ideally, "git status" should return a non-zero exit
> code as well.

Also, I think "git add ." would silently ignore such directories, which
is probably a bad thing if you are relying on it to capture the whole
directory's state. Similarly, I think we would ignore transient
errors (like EMFILE) or other EACCES problems (like a mode 0700
directory owned by somebody else).

> The problem seems to be In read_directory_recursive() from dir.c. When
> opendir() returns null, we continue on ignoring any error. Is there a
> scenario where returning null is expected? We can simply call perror()
> here, but it would be nice if we can propagate the error to the exit
> code too. How would we do that?

I think we should report an error on EACCES. Perhaps somebody is happy
that "git add" ignores unreadable directories, but the right solution is
for them to put those directories in their .gitignore (and/or use
"--ignore-errors").

People may want to ignore ENOENT in this situation, though. That is a
sign that somebody is racily modifying the directory while git is
running. That's generally a bad idea, but it is not a big deal for us to
skip such a directory (after all, we might racily have missed its
existence in the first place, so all bets are off).

>From a cursory look, I'd agree that hitting the opendir() in
read_directory_recursive is the right place to start. I'd silently
ignore ENOENT, and propagate the rest.

That code is too low-level to call die() directly, I think, so you will
need to propagate the error back. Adding a new error-value to the "enum
path_treatment" could work, but it will probably be rather clumsy
getting it all the way back up to the original caller. It will probably
be much easier to:

  1. Give dir_struct an error flag, and set it whenever the traversal
     sees an error. Callers can check the flag at the appropriate level
     and ignore or die() as appropriate.

  2. Teach dir_struct a "quiet" flag. If not set, emit a warning()
     deep in the code. Alternatively, you could collect a set of
     error-producing pathnames (along with their errno values), and
     the caller could decide whether to print them itself (this is
     similar to how DIR_COLLECT_IGNORED works).

-Peff