Re: [GSoC][PATCH v2 7/7] fsck: add ref content check for files backend

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

shejialuo <shejialuo@gmail.com> writes:

>> > +static int files_fsck_symref(struct fsck_refs_options *o,
>> > +			     struct strbuf *refname,
>> > +			     struct strbuf *path)
>> 
>> This does not take things like HEAD or refs/remotes/origin/HEAD to
>> validate.  Instead, the caller is responsible for either doing a
>> readlink on a symbolic link, or reading a textual symref and
>> stripping "ref: " prefix from it, before calling this function.
>> The "refname" parameter is not HEAD or refs/remotes/origin/HEAD but
>> the pointee of the symref.
>> 
>> So I'd imagine that renaming it to fsck_symref_target or along that
>> line to clarify that we are not checking the symref, but the target
>> of a symref, would be a good idea.
>
> That's not correct. The "refname" parameter is EXACTLY the symref
> itself.

Yeah, but the story is the same.  We are not really checking
anything about the symref (i.e. the thing in "refname") being funny.
We are checking what is in "path" (like "does it exist?") and the
"refname" is there only for reporting purposes (i.e. "we have a
symbolic ref REFNAME, that points at PATH which is a wrong thing").

>> > +{
>> > +	struct stat st;
>> > +	int ret = 0;
>> > +
>> > +	if (lstat(path->buf, &st) < 0) {
>> > +		ret = fsck_refs_report(o, refname->buf,
>> > +				       FSCK_MSG_DANGLING_SYMREF,
>> > +				       "point to non-existent ref");
>> > +		goto out;
>> > +	}
>> 
>> Is that an error?  Just like being on an unborn branch is not an
>> error, it could be argued that a symref that points at a branch yet
>> to be born wouldn't be an error, either, no?
>> 
>
> The reason why I choose "danglingSymref" and warn severity is that I let
> the code be align with "git checkout". When we use "git checkout" for a
> dangling symref. It would produce the following output:
>
>   $ git checkout branch-3
>   warning: ignoring dangling symref refs/heads/branch-3
>   error: pathspec 'branch-3' did not match any file(s) known to git
>
> So I prefer to warn severity.

If you do this from that situation, 

    $ git branch branch-3 master

what happens is that the pointee of branch-3 is created at the
commit pointed at by 'master'.  No error.  No warnings.

In a freshly created respository, HEAD is a dangling symbolic ref,
and that is not an error.  You can create a root commit from there
just fine.

If there is anything that needs improvement in your example, it is
that "checkout branch-3" should be taught to either (1) not warn
about dangling symbolic link and just give the error, which is in
line with how "git checkout HEAD" in a freshly created repository
behaves, or (2) just like unborn 'master' pointed at by 'HEAD' is
perfectly happy to be checked out, allow the unborn 'branch-3' to be
pointed at by 'HEAD', and arrange the first commit (which will be a
root commit) you create in that state to be pointed by the ref
'branch-3' points at.

So from all of these reasons, I do not think missing target should
be treated as any error worthy event.  Not even warning.

On the other hand, the target of the symref in "path" must be
checked, even if it does not currently exist, for its validity, the
same way an existing ref gets checked (lives inside refs/, passes
check-ref-format, etc.).

> I intentionally ignored the "escape" situation. Actually, the path could
> be either absolute or relative. It may be a little complicated. I will
> find a way to support this in the next version.

Yes, if this wants to claim to be part of "FSCK", it should catch
all the errors the regular runtime would complain about, and
"escape" thing is one of the first things that you need to get
right.  Whatever refs.c:read_ref_internal() for S_ISLNK(st.st_mode)
case takes as legit should be considered legit, the "fallback - read
through the symlink as if it were a non-symlink" case probably wants
to be warned.  refs.c:resolve_ref_unsafe(), which is used at the low
level from object-name.c:get_oid_basic() via refs.c:repo_dwim_ref(),
has further checks to see if a refname that a symref resolves to is
valid and the runtime sanity relies on these checks.

Thanks.