Re: [GSoC][PATCH v13 10/10] fsck: add ref content check for files backend

> On Mon, Jul 29, 2024 at 09:27:56PM +0800, shejialuo wrote:
> > Enhance the git-fsck(1) command by adding a check for reference content
> > in the files backend. The new functionality ensures that symrefs, real
> > symbolic link and regular refs are validated correctly.
> > 
> > In order to check the trailing content of the regular refs, add a new
> > parameter `trailing` to `parse_loose_ref_contents`.
> > 
> > For symrefs, `parse_loose_ref_contents` will set the "referent".
> > However, symbolic link could be either absolute or relative. Use
> > "strbuf_add_real_path" to read the symbolic link and convert the
> > relative path to absolute path. Then use "skip_prefix" to make it align
> > with symref "referent".
> > 
> > Thus, the symrefs and symbolic links could share the same interface. Add
> > a new function "files_fsck_symref_target" which aims at checking the
> > following things:
> > 
> > 1. whether the pointee is under the `refs/` directory.
> > 2. whether the pointee name is correct.
> > 3. whether the pointee path is a wrong type in filesystem.
> > 
> > Last, add the following FSCK MESSAGEs:
> > 
> > 1. "badRefContent(ERROR)": A ref has a bad content
> > 2. "badSymrefPointee(ERROR)": The pointee of a symref is bad.
> > 3. "trailingRefContent(WARN)": A ref content has trailing contents.
> 
> I think it would have been fine to stop at the preceding commit as it
> clearly demonstrates how the whole infrastructure is supposed to work.
> Additional checks like those you add here would then be a good candidate
> for a separate patch series. This would help you get the first patch
> series landed faster as you really only have to focus on setting up the
> baseline infrastructure.
> 
> Feel free to keep or drop this patch as you prefer though, I don't want
> to discourage you aiming higher. Just keep in mind that the more you add
> on top the longer it takes to land a patch series :)
> 

> > +	/*
> > +	 * If the ref is a symref, we need to check the destination name and
> > +	 * connectivity.
> > +	 */
> > +	if (referent.len && (type & REF_ISSYMREF)) {
> > +		strbuf_addf(&pointee_path, "%s/%s", gitdir, referent.buf);
> > +		strbuf_rtrim(&referent);
> > +
> > +		ret = files_fsck_symref_target(o, &info, refname.buf,
> > +					       referent.buf, pointee_path.buf);
> > +		goto clean;
> > +	} else {
> > +		if (trailing && (*trailing != '\0' && *trailing != '\n')) {
> 
> In case the ref ends with a newline, should we check that the next
> character is `\0`? Otherwise, it may contain multiple lines, which is
> not allowed for a normal ref.
> 
> Also, shouldn't the ref always end with a newline?
>