Re: [PATCH v2 1/2] Check DB_ENVIRONMENT using is_directory()

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 1, 2017, 17:24 UTC
Message-ID: <xmqqr2y0nk88.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170701125057.16169-1-tboegi@web.de>

tboegi@web.de writes:

Show 11 quoted lines

> From: Torsten Bögershausen <tboegi@web.de>
>
> In setup.c is_git_directory() checks a Git directory using access(X_OK).
> This does not check, if path is a file or a directory.
> Check path with is_directory() instead.
> ---
> After all the discussions (and lots of tests) I found that this patch
> works for my setup.
> All in all could the error reporting be improvved for is_git_directory(),
> as there may be "access denied", or "not a directory" or others, but
> that is for another day.

Wouldn't this be a slight regression, though?

We used to ignore an unsearchable directory but now we blindly say "ah, it is a directory".

Checking is_directory() in addition to the existing access() would be making a progress by fixing one bug (i.e. we no longer are confused by an executable file there); skipping that access() based on the filesystem quirks can be left for another day, of course.

Show 16 quoted lines

> setup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/setup.c b/setup.c
> index 358fbc2..5a7ee2e 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -321,7 +321,7 @@ int is_git_directory(const char *suspect)
>  
>  	/* Check non-worktree-related signatures */
>  	if (getenv(DB_ENVIRONMENT)) {
> -		if (access(getenv(DB_ENVIRONMENT), X_OK))
> +		if (!is_directory(getenv(DB_ENVIRONMENT)))
>  			goto done;
>  	}
>  	else {