> > + iter = dir_iterator_begin(sb.buf, 0);
> > +
> > + if (!iter) {
> > + ret = error_errno("cannot open directory %s", sb.buf);
> > + goto out;
> > + }
>
> The error message should probably be marked as translatable. Also, I'd
> personally remove the newline between `iter = ...` and the error check
> as those are a logical unit.
>
Yes, I will improve this in the next version.
> > + for (size_t i = 0; fsck_refs_fns[i]; i++) {
> > + if (fsck_refs_fns[i](o, gitdir, refs_check_dir, iter))
> > + ret = -1;
> > + }
> > + } else {
> > + ret = error(_("unexpected file type for '%s'"),
> > + iter->basename);
>
> Instead of printing this as an error directly, shouldn't we report it
> via the `fsck_refs_report` interface?
>
Yes, exactly we should use this interface. I accidentally ignored this.
Thanks.
> > +out:
> > + strbuf_release(&sb);
> > + return ret;
> > +}
> > +
> > +static int files_fsck_refs(struct ref_store *ref_store,
> > + struct fsck_options *o)
> > +{
> > + files_fsck_refs_fn fsck_refs_fns[]= {
> > + NULL
>
> The last member should also end with a comma.
>
I will improve this in the next version.
> > + };
> > +
> > + if (o->verbose)
> > + fprintf_ln(stderr, "Checking references consistency");
> > +
> > + return files_fsck_refs_dir(ref_store, o, "refs", fsck_refs_fns);
> > +
>
> This newline should be removed.
>
OK.
> > +}
> > +
> > static int files_fsck(struct ref_store *ref_store,
> > struct fsck_options *o)
> > {
> > struct files_ref_store *refs =
> > files_downcast(ref_store, REF_STORE_READ, "fsck");
> >
> > - return refs->packed_ref_store->be->fsck(refs->packed_ref_store, o);
> > + return refs->packed_ref_store->be->fsck(refs->packed_ref_store, o) |
> > + files_fsck_refs(ref_store, o);
>
> I'd think we should first check loose files and then continue to check
> the packed ref store. That's really only a gut feeling though, and I
> cannot exactly say why that feels more natural.
>
It would feel more natural. Because packed-ref will point to the
loose ref. So we should first check the loose refs. For example. If a
regular ref is bad, we will first report the problem. And suppose the
packed-ref have recorded this regular ref. When checking packed-ref, we
could not check the regular ref itself. We only need to check one thing.
Whether the pointee exists under the "refs/" directory.
And we do not need to check regular ref again because we have checked in
the loose refs part.
> Patrick
