Re: [PATCH v8] setup: allow cwd/.git to be a symlink to a directory

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

Tian Yuchen <a3205153416@gmail.com> writes:

> Currently, `setup_git_directory_gently_1()` fails to recognize a `.git`
> symlink pointing to a directory because `read_gitfile_gently()` strictly
> expects a regular file and returns `READ_GITFILE_ERR_NOT_A_FILE` for
> anything else, including valid directories.

I have to admit that I was not paying too much attention to this
part of the proposed log message.  We kept seeing the above (and the
title) for the series forever, but is it really a problem if we did
not allow you to make a symbolic link to somebody else's .git/
directory?  Besides, I do not think we have such a problem at all,
as read_gitfile_gently() uses stat() not lstat().

	Side note: In any case, we do not want to see "Currently".
        We give an observation on how the current system works in
        the present tense (so no need to say "Currently X is Y", or
        "Previously X was Y" to describe the state before your
        change; just "X is Y" is enough), and discuss what you
        perceive as a problem in it.

I just did this, and it worked just fine as expected.

    $ git clone https://git.kernel.org/pub/scm/git/git.git/ git
    $ mv git/.git git.git
    $ ln -s ../git.git git/.git
    $ git -C git log

Even if there were a problem in making such a symbolic link work,
these days we have a textual "gitdir: $there" file as an official
way to let you keep the repository data on a filesystem that is
different from the filesystem that hosts its working tree.

Anyway, I've always thought that this topic is about addressing the
two NEEDSWORK comments to allow us to give better filesystem problem
diagnoses.

> Fix this by distinguishing directories from regular files and other
> non-regular file types (like FIFOs or sockets) via newly introduced
> error_code.

So, "Fix this" written here does not resonate with my understanding
of what we have been discussing so far.  Puzzled.

> diff --git a/setup.c b/setup.c
> index c8336eb20e..2869d10669 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -897,10 +897,14 @@ int verify_repository_format(const struct repository_format *format,
>  void read_gitfile_error_die(int error_code, const char *path, const char *dir)
>  {
>  	switch (error_code) {
> -	case READ_GITFILE_ERR_STAT_FAILED:
> -	case READ_GITFILE_ERR_NOT_A_FILE:
> +	case READ_GITFILE_ERR_STAT_ENOENT:
> +	case READ_GITFILE_ERR_IS_A_DIR:
>  		/* non-fatal; follow return path */
>  		break;
> +	case READ_GITFILE_ERR_STAT_FAILED:
> +		die(_("error reading %s"), path);
> +	case READ_GITFILE_ERR_NOT_A_FILE:
> +		die(_("not a regular file: %s"), path);
>  	case READ_GITFILE_ERR_OPEN_FAILED:
>  		die_errno(_("error opening '%s'"), path);
>  	case READ_GITFILE_ERR_TOO_LARGE:
> @@ -941,8 +945,14 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
>  	static struct strbuf realpath = STRBUF_INIT;
>  
>  	if (stat(path, &st)) {
> -		/* NEEDSWORK: discern between ENOENT vs other errors */
> -		error_code = READ_GITFILE_ERR_STAT_FAILED;
> +		if (errno == ENOENT)
> +			error_code = READ_GITFILE_ERR_STAT_ENOENT;
> +		else
> +			error_code = READ_GITFILE_ERR_STAT_FAILED;
> +		goto cleanup_return;
> +	}
> +	if (S_ISDIR(st.st_mode)) {
> +		error_code = READ_GITFILE_ERR_IS_A_DIR;
>  		goto cleanup_return;
>  	}
>  	if (!S_ISREG(st.st_mode)) {

All of the above are exactly what I expected to see.  Nice.

> @@ -1578,20 +1588,28 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>  		if (offset > min_offset)
>  			strbuf_addch(dir, '/');
>  		strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
> -		gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?
> -						NULL : &error_code);
> +		gitdirenv = read_gitfile_gently(dir->buf, &error_code);
>  		if (!gitdirenv) {
> +			switch (error_code) {
> +			case READ_GITFILE_ERR_STAT_ENOENT:
> +				/* no .git in this directory, move on */
> +				break;
> +			case READ_GITFILE_ERR_IS_A_DIR:
>  				if (is_git_directory(dir->buf)) {
>  					gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
>  					gitdir_path = xstrdup(dir->buf);
>  				}
> +				/* otherwise, it is an empty/unrelated directory, move on */
> +				break;

This design decision may be debatable, but not tightening everything
at once may be a prudent thing to do to avoid accidental regression.

Having said that.

If you have a directory ".git/" somewhere in your working tree, and
the directory is somehow corrupt that is_git_directory() says "nope,
that is not a valid Git directory", wouldn't you rather want to know
about it as a potential problem?  Perhaps there are valid use cases
to have such a directory (you have "empty" listed here, which may or
may not have a valid use case), but it smells like falling into the
same bucket as "Gee we have .git that is a fifo here---what is going
on???", which is ...

> +			default:
> +				if (die_on_error || error_code == READ_GITFILE_ERR_NOT_A_FILE)
> +					read_gitfile_error_die(error_code, dir->buf, NULL);
> +				else
> +					return GIT_DIR_INVALID_GITFILE;

... what we do here.


So, I dunno.

> +			}
> +		} else {
>  			gitfile = xstrdup(dir->buf);
> +		}