Re: [PATCH v2] setup: fail if .git is not a file or directory

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

Tian Yuchen <a3205153416@gmail.com> writes:

>  		if (!gitdirenv) {
>  			if (die_on_error ||
> -			    error_code == READ_GITFILE_ERR_NOT_A_FILE) {
> +				error_code == READ_GITFILE_ERR_NOT_A_FILE) {

Why this indentation change?

> -				/* NEEDSWORK: fail if .git is not file nor dir */
> -				if (is_git_directory(dir->buf)) {
> +				if (is_git_directory(dir->buf)){

Why this change (which is style violation that lack necessary SP
between "){")?

>  					gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
>  					gitdir_path = xstrdup(dir->buf);
> +				} else {
> +					/*
> +					 * It is neither a gitfile nor a git directory.
> +					 */
> +					struct stat st;
> +					if (!lstat(dir->buf, &st) &&
> +						!S_ISREG(st.st_mode) &&
> +						!S_ISDIR(st.st_mode)) {
> +						if (die_on_error)
> +							die(_("Invalid %s: not a regular file or directory"), dir->buf);
> +						else
> +							return GIT_DIR_INVALID_GITFILE;
> +					}
>  				}

Stepping back a bit, even though the NEEDSWORK comment is placed
here, I am not sure if this is the bast place to do much of the
necessary work.

When gitdirenv is NULL and die_on_error is set, we would have died
on any error other than STAT_FAILED (most often, this is because
there wasn't .git at the path in the first place) or NOT_A_FILE
(again, a happy case is that .git existed and it was a directory).
We would have already died in read_gitfile_error_die() in all other
cases.  But these two error cases are not necessarily entirely happy
and that is what the NEEDSWORK comment is about.

So, if we wanted to tighten the error checking to help users
diagnose problems in their filesystem, I wonder if it is a better
approach to refine the set of READ_GITFILE_ERR_* error codes:

 - STAT_ENOENT (new) is returned when stat failed and we got ENOENT,
   and it is not a fatal error.

 - STAT_FAILED becomes a fatal error in read_gitfile_error_die().

 - IS_A_DIR (new) is returned when stat succeeded and it is a
   directory.  It is not a fatal error.

 - NOT_A_FILE becomes a fatal error in read_gitfile_error_die().

Existing callers of the two functions, read_gitfile_gently() and
read_gitfile_error_die(), must be audited and adjusted
appropriately, but once it is done, it would become much simpler,
wouldn't it?

Thanks.