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

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

Tian Yuchen <a3205153416@gmail.com> writes:

> Strictly enforcing 'lstat()' prevents valid '.git' symlinks.

But nobody sane would propose running one more lstat() anyway, so
how is that relevant?

>  		if (!gitdirenv) {
> -			if (die_on_error ||
> -			    error_code == READ_GITFILE_ERR_NOT_A_FILE) {
> -				/* NEEDSWORK: fail if .git is not file nor dir */
> -				if (is_git_directory(dir->buf)) {
> -					gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
> -					gitdir_path = xstrdup(dir->buf);
> -				}
> -			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
> -				return GIT_DIR_INVALID_GITFILE;

The _intent_ of the original code was to

    * do is_git_directory() thing to deal with a plain vanilla
      ".git" directory when read_gitfile_gently thing said "we found
      a directory" (NOT_A_FILE is overly coarse, which is what we
      are correcting in this topic, but the _intent_ was to do the
      is_git_directory() thing when we know it is a directory).

    * return INVALID_GITFILE on any error, but do not return when
      the reason why read_gitfile_gently thing failed was because
      there is no ".git" there (again, STAT_FAILED is overly coarse,
      which is what we are correcting in this topic, but the
      _intent_ was to return INVALID thing when we not the failure
      is not due to ENOENT).  Note that returning INVALID_GITFILE is
      done when die_on_error is not set.

> -		} else
> +			if (error_code)
> +				read_gitfile_error_die(error_code, dir->buf, NULL);

Should this be unconditional?  If our caller did not ask us to die
upon an error with die_on_error, what happens?  The original I think
returned INVALID_GITFILE for the caller to deal with.

> +			if (is_git_directory(dir->buf)) {

Should this be unconditional?  If the thing is a directory, the
original would have given us NOT_A_FILE but now it would give us
IS_A_DIR.  And that is the only case original wanted to call
is_git_directory() no?

> +				gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
> +				gitdir_path = xstrdup(dir->buf);
> +			}
> +		} else {
>  			gitfile = xstrdup(dir->buf);
> +		}
>  		/*
>  		 * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT
>  		 * to check that directory for a repository.