Re: [GSoC][PATCH v13 08/10] files-backend: add unified interface for refs scanning

From: shejialuo <shejialuo@gmail.com>
Date: Jul 30, 2024, 16:10 UTC
Message-ID: <ZqkQbjtDFawIwlvf@ArchLinux>
In-Reply-To: <Zqik73qynJvOLctw@tanuki>

Show 11 quoted lines

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

Show 11 quoted lines

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

Show 13 quoted lines

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

Show 10 quoted lines

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

Show 16 quoted lines

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