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

Tian Yuchen <a3205153416@gmail.com> writes:

v12 does work differently from what we have been aiming for, but I find that it arguably is a much safer approach.

Even though the updated read_gitfile_gently() returns finer-grained READ_GITFILE_ERR_* codes than the original, read_gitfile_error_die() does not change behaviour from the original. Any caller that use read_gitfile(path), which is read_gitfile_gently(path, NULL), like the setup_explicit_git_dir() codepath we have been looking at lately, lets read_gitfile_error_die() react to the error code, which is to behave exactly as what the code did before this patch.

So, I dunno. After all, these two NEEDSWORK comments have been with us for quite some time, and reminded us that we may want to consider if we need to do anything differently. I do not think we mind if we conclude negatively, taking "no, it is of dubious value to tighten error checking in these code paths" as an answer to these NEEDSWORK comments. v12 is slightly less defeatest than that stance in that we are only allowing the callers that care about what kind of errors they are getting and and want to decide how to react to them, while keeping the default error behaviour the same for those who do not ask with &error_code what kind of errors we saw.

The patch makes the behaviour change for callers that pass an &error_code pointer to read_gitfile_gently() and act on the returned error code itself, like the discovery code path. As long as these callers are audited and adjusted as necessary, we have very little risk of regression.

I won't be doing a full audit in this message, but just to give taste of what is expected ...

$ git grep -n -e 'read_gitfile_gently('

builtin/init-db.c:212:		p = read_gitfile_gently(git_dir, &err);

This caller gives &err but it never looks at what is in it after the call returns, so there shouldn't be any behaviour change.

setup.c:465:	if (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf))

This is followed by 

		ret = 1;
        if (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||
            gitfile_error == READ_GITFILE_ERR_READ_FAILED)
                ret = 1;

I do not offhand know if this list of "error codes that should result in returning 1 from this function" needs to be tweaked to adjust for the change in this patch.

worktree.c:390:	path = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));

This is followed by code that reacts to path being NULL and shows the contents of err in an error message. Should be benign.

Thanks.