Re: [GSoC][PATCH v4 2/4] dir: add directory_exists

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 16, 2017, 16:09 UTC
Message-ID: <xmqqlgs5cify.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170316051827.97198-3-lehmacdj@gmail.com>

Devin Lehmacher <lehmacdj@gmail.com> writes:

Show 6 quoted lines

> +int directory_exists(const char *path)
> +{
> +	struct stat sb;
> +	int ret = lstat(path, &sb);
> +	return ret == 0 && S_ISDIR(sb.st_mode);
> +}

I am not a great fan of using file_exists() [*1*] on anything other than paths in the working tree (i.e. in preparation for checking things out of or into the index), as paths inside .git/ and paths outside have different requirements. One of the difference is if it makes sense to use stat(2) or lstat(2) for a check like this function does. For working tree entities, which file_exists() and friends are designed for, we absolutely should use lstat(2). The "RelNotes" symbolic link at the top level of the project must be known as a symbolic link and a question "is this a directory?" on it must be answered "no", for example.

But there is no fundamental reason ~/.git-credential-cache (or anything outside the working tree) must be S_ISDIR(). If the user wanted to have the real directory elsewhere and point at it with a symbolic link ~/.git-credential-cache, we should allow it.

If we need a helper function to see if a path in the working tree is a directory, adding this new helper function to dir.c (which is about the paths in the working tree) and use lstat(2) in it is the right thing to do. But I do not think it should be used to check if ~/.git-credential-cache directory, which is not a filesystem entity in a working tree, exists.

IOW, I do not think this patch helps the topic of this series. Drop this patch from the series and have a similar code (but use stat(2) instead of lstat(2)) directly inside get_socket_path()'s in the next patch, perhaps?

[Footnote]

*1* ... and friends, like safe_create_leading_directories().