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:

> In order to check the trailing content, add a new parameter
> "trailing" to "parse_loose_ref_contents" function.

About this one.

>  int parse_loose_ref_contents(const char *buf, struct object_id *oid,
>  			     struct strbuf *referent, unsigned int *type,
> -			     int *failure_errno)
> +			     int *failure_errno, unsigned int *trailing)
>  {
>  	const char *p;
>  	if (skip_prefix(buf, "ref:", &buf)) {
> @@ -607,6 +607,10 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,
>  		*failure_errno = EINVAL;
>  		return -1;
>  	}
> +
> +	if (trailing && (*p != '\0' && *p != '\n'))
> +		*trailing = 1;
> +
>  	return 0;
>  }

We know what the garbage looked like at this point.  The caller owns
the "buf" pointer and we are pointing into that buffer with the
pointer p, and the garbage is right there.

So I am not sure if losing information by using "uint *" is a good
idea.  Wouldn't it make more sense to take "const char **trailing"
as a parameter and tell the caller where the trailing junk begins?

> +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.

> +{
> +	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?

> +	if (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode)) {
> +		ret = fsck_refs_report(o, refname->buf,
> +				       FSCK_MSG_DANGLING_SYMREF,
> +				       "point to invalid object");
> +		goto out;

The use of "object" here is highly misleading.  Yes, you can call a
filesystem entity like "a regular file", "a directory", etc. "an
object", but the word can refer to many other kinds of "object".  In
fact, I originally read this to mean "we are referring to an object
in the object database that is corrupt" or something, but of course
that is not what we are complaining about. We are complaining that
the symbolic link points at a file of wrong type (like a directory).

So, in short, missing is probably OK.  Pointing at a wrong thing
(like a directory or block device) is not.  Where, if any, do we
catch a symbolic ref that tries to escape the refs/* hierarchy
(e.g. ".git/refs/heads/my-crazy-ref" that is a symbolic link that
points at "../../../../else/where" that is not even part of the
repository), by the way?

Thanks.