Re: [PATCH v5 10/11] setup_git_directory_gently_1(): avoid die()ing

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <johannes.schindelin@gmx.de> 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.

> +			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?

> +			} 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).

By the way, is the "error_code &&" needed?  Unless the original path
given to read_gitfile_gently() is a NULL pointer, the only time its
return value is NULL is when it has non-zero error_code.


>  		strbuf_setlen(dir, offset);
>  		if (gitdirenv) {
>  			strbuf_addstr(gitdir, gitdirenv);
> @@ -934,7 +944,7 @@ const char *discover_git_directory(struct strbuf *gitdir)
>  		return NULL;
>  
>  	cwd_len = dir.len;
> -	if (setup_git_directory_gently_1(&dir, gitdir) <= 0) {
> +	if (setup_git_directory_gently_1(&dir, gitdir, 0) <= 0) {
>  		strbuf_release(&dir);
>  		return NULL;
>  	}
> @@ -994,7 +1004,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  		die_errno(_("Unable to read current working directory"));
>  	strbuf_addbuf(&dir, &cwd);
>  
> -	switch (setup_git_directory_gently_1(&dir, &gitdir)) {
> +	switch (setup_git_directory_gently_1(&dir, &gitdir, 1)) {
>  	case GIT_DIR_NONE:
>  		prefix = NULL;
>  		break;