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.
