Junio C Hamano <gitster@xxxxxxxxx> writes：
>
> shejialuo <shejialuo@xxxxxxxxx> 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").
>
Yes, I agree, will rename the function in the next version.
> >> > +{
> >> > + 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.
>
I am totally shocked by the fact that it will create the pointee file. I am
a little curious about the design here. I know when creating a new repo,
HEAD is a dangling symbolic ref. When we do first commit, it will
create the pointee file. But HEAD is a special file, it should be treated
differently. Why we treat the other symrefs the same way.
> 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.
>
Should we really improve this? From my perspective, a user will get
more information when this warn message shows up. The
"error: pathspec 'branch-3' did not match any file(s) known to git" is
less informative.
Maybe the pro git users do not use git command to write to the git file
system. However, this is a bad way to do this. We always want to
the user to use the command to handle with git file system.
Instead, we should warn about the user when using
$ git branch branch-3 master
It will create a new pointee here, I suppose we should warn the user
here except HEAD ref. The user could get the dangling symref by
deleting the pointee ref. For example:
$ git symbolic-ref refs/heads/main master
$ git checkout -b master-copy
$ git branch --delete master
Should we warn this for user in git-fsck(1)?
> So from all of these reasons, I do not think missing target should
> be treated as any error worthy event. Not even warning.
>
Yes, if git-branch implicitly creates the pointee ref file. We should not
handle such case. But I think giving a warning here except HEAD
ref is worthful.
> 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.).
>
Yes!
> > 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 for your help! I will look at these codes to implement the
"escape" check.
> Thanks.
>
