Hi Junio,
PLEASE NOTE: the purpose of this patch series is to allow the same
function (setup_git_directory_gently_1()) to be the work horse for
setup_git_directory() as before, but also for the new
discover_git_directory() function that is introduced to fix
read_early_config() to avoid hardconfig .git/config.
The purpose is *not* to change any current behavior. Please do not ask me
to do that in this patch series.
Now to your comments:
On Fri, 10 Mar 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
>
> > @@ -890,14 +892,22 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
> > if (one_filesystem)
> > current_device = get_device_or_die(dir->buf, NULL, 0);
> > for (;;) {
> > - int offset = dir->len;
> > + int offset = dir->len, error_code = 0;
> >
> > if (offset > min_offset)
> > strbuf_addch(dir, '/');
> > strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
> > - gitdirenv = read_gitfile(dir->buf);
> > - if (!gitdirenv && is_git_directory(dir->buf))
> > - gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
> > + gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?
> > + NULL : &error_code);
> > + if (!gitdirenv) {
>
> We tried to read ".git" as a regular file, but couldn't. There are
> some cases but I am having trouble to convince myself all cases are
> covered correctly here, so let me follow the code aloud.
It is subtle and finicky, I agree.
> > + if (die_on_error ||
> > + error_code == READ_GITFILE_ERR_NOT_A_FILE) {
> > + if (is_git_directory(dir->buf))
> > + gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
>
> If we are told to die on error, but if it is a Git directory, then
> we do not have to (and want to) die and instead report that
> directory as discovered.
>
> If we are to report the failure status instead of dying, we also do
> want to recover when the read_gitfile_gentrly() failed only because
> it was a real Git directory. READ_GITFILE_ERR_NOT_A_FILE could be a
> signal for that, and we recover after making sure it is a Git
> directory.
>
> Among the READ_GITFILE_ERR_* codes, ERR_NOT_A_FILE is the only one
> we attempt this recovery. All others just take the "else if" thing
> below.
>
> What happens when is_git_directory() test here does not pass,
> though? Let's say stat() in read_gitfile_gently() found a FIFO
> there, it gives us ERR_NOT_A_FILE, is_git_directory() would say
> "Nah", and then ...? Don't we want the other side for this if()
> statement that returns failure?
The current code as per `master` detects the NOT_A_FILE condition, and as
the parameter return_error_code was passed as NULL (because read_gitfile is
actually #define'd in cache.h to call read_gitfile_gently with NULL as
second parameter), it calls read_gitfile_error_die(). That function
*ignores* the NOT_A_FILE error condition, and as a consequence returns to
read_gitfile_gently() which then returns NULL because there *was* an
error_code.
That means that setup_git_directory_gently_1() will retrieve a NULL from
the read_gitfile() call, which means it then goes on to test whether it
is looking at a .git/ directory via is_git_directory() and if that returns
false, the loop will continue to look at the *parent* directory.
That, in turn, means that what you are asking for is a change in behavior,
and as I am unwilling to introduce a change in behavior with this patch
series, my patch does the exact right thing.
> > + } else if (error_code &&
> > + error_code != READ_GITFILE_ERR_STAT_FAILED)
> > + return GIT_DIR_INVALID_GITFILE;
> > + }
>
> On the other hand, if read_gitfile_gently() didn't say "we found
> something that is not a regular file" we come here. If the error
> came because there wasn't ".git" in the directory we are looking at,
> i.e. stat(2) in read_gitfile_gently() gave ENOENT, it is perfectly
> normal and we just want to go one level up.
>
> But shouldn't read_gitfile_gently() be inspecting errno when it sees
> a stat() failure? If there _is_ ".git" but we failed to stat it for
> whatever reason (EACCES, ELOOP,...), we do not want to go up but
> give the INVALID_GITFILE from here, just like other cases, no?
> For that I imagine that ERR_STAT_FAILED needs to be split into two,
> one for true ERR_STAT_FAILED (from which we cannot recover) and the
> other for ERR_ENOENT to signal us that there is nothing there (which
> allows us to go up).
True. But again, you are asking for a *change in behavior*, which is not
the purpose of this patch series.
> By the way, is the "error_code &&" needed?
It is not! I had the order of the if/else blocks completely differently
originally [*1*] and just overlooked that the error_code could no longer
be 0 in that condition.
Ciao,
Dscho
Footnote *1*: Yes, I wanted to construct an English sentence with three
-ly words in a row.