Re: [PATCH v4 2/5] ref: port git-fsck(1) regular refs check for files backend

[shejialuo <shejialuo@gmail.com>]

On Wed, Sep 18, 2024 at 11:59:45AM -0700, Junio C Hamano wrote:

[snip]

> The above reads as if you are, in preparation to "port" the checks
> we have in "fsck" to elsewhere (presumably to "refs verify"), you
> are removing the checks that _will_ become redundant from "fsck".
> 
> But that does not seem to be what is happening.  Let me try to
> paraphrase, in order to check my understanding of what you wanted to
> say:
> 
>     "git-fsck(1) has some consistency checks for regular refs.  As
>     we want to align the checks "git refs verify" performs with
>     them (and eventually call the unified code that checks refs from
>     both), port the logic "git fsck" has to "git refs verify".
> 

Thanks, I have re-read my words, I did not explain this thing well.

> > +	if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) {
> > +		ret = error_errno(_("unable to read ref '%s/%s'"),
> > +				  refs_check_dir, iter->relative_path);
> 
> Is there a reason why we cannot to use report.path aka refname.buf,
> and instead we have to recompute the same path again?
> 

Thanks for pointing out this, because this part I wrote a long time ago
and I think it's unrelated to the fsck part. So, I forgot to change.

> Should this error be propagated back to the caller, not just to the
> end-user, by a call to fsck_report_ref(), like you do for a ref file
> that has questionable contents?  If ref iteration (like for-each-ref)
> claims there is this ref, and you cannot read its value when you try
> to use it, it is just as bad as having a loose ref file that has
> unusable contents, isn't it?
> 

I agree. The initial motivation for this design is that I think this is
OS-specific issue (It may be read successfully in the next time). So, I
don't put it into the fsck part. But It make senses that we should
report this.

> It is a separate matter if such a failure mode deserves its own
> error code (FSCK_MSG_UNREADABLE_REF) or can be rolled into the same
> FSCK_MSG_BAD_REF_CONTENT.  I can see arguments for both sides and
> offhand have no strong preference either way.
> 

We could just use "FSCK_MSG_BAD_REF_CONTENT" and add a message "cannot
open this file". I guess this should be enough.