Re: [PATCH v9] setup: improve error diagnosis for invalid .git files

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

Tian Yuchen <a3205153416@gmail.com> writes:

>  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:

The changes to these two functions require us to audit callers of
them that are outside the call graph of the main focus of this
patch.  For example, we see the following code in submodule.c:

        void absorb_git_dir_into_superproject(const char *path,
                                              const char *super_prefix)
        {
                int err_code;
                const char *sub_git_dir;
                struct strbuf gitdir = STRBUF_INIT;

                if (validate_submodule_path(path) < 0)
                        exit(128);

                strbuf_addf(&gitdir, "%s/.git", path);
                sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);

                /* Not populated? */
                if (!sub_git_dir) {
                        const struct submodule *sub;
                        struct strbuf sub_gitdir = STRBUF_INIT;

                        if (err_code == READ_GITFILE_ERR_STAT_FAILED) {
                                /* unpopulated as expected */
                                strbuf_release(&gitdir);
                                return;
                        }

                        if (err_code != READ_GITFILE_ERR_NOT_A_REPO)
                                /* We don't know what broke here. */
                                read_gitfile_error_die(err_code, path, NULL);

Let's take a look.

ERR_STAT_FAILED used to be "If we got ENOENT, it is an unpopulated
submodule so it is OK; it is so unlikely that we get other kinds of
failure and we cannot deal with them anyway".

With the patch we have been looking at, shouldn't the above code
check for ERR_STAT_ENOENT instead, to do the same "unpopulated,
nothing to do here, happy!" return?

This is merely just one example.  All hits from "git grep" for the
functions whose semantics have been updated by the patch must be
looked at in a similar way, but I didn't look at how many releavnt
code paths there are.  It could be an easier way to find code paths
that may need to be updated to grep for ERR_STAT_FAILED and
ERR_NOT_A_FILE, the two symbols whose meaning have changed.

Thanks.