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

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

Tian Yuchen <a3205153416@gmail.com> writes:

> Unfortunately, looking back now, my implementation barely qualifies as 
> “functional” and actually undermines the purpose of setup_git..() 
> itself. It's a mess — I rushed into it without properly reviewing the 
> context (like how other cases are handled) :(((

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.