Re: [PATCH 2/5] refs/reftable: add fsck check for checking the table name

[shejialuo <shejialuo@gmail.com>]

On Mon, Sep 01, 2025 at 06:33:24AM -0700, Karthik Nayak wrote:
> shejialuo <shejialuo@gmail.com> writes:
> 
> > On Tue, Aug 19, 2025 at 02:21:01PM +0200, Karthik Nayak wrote:
> >> The `git refs verify` command is used to run fsck checks on the
> >> reference backends. This command is also invoked when users run 'git
> >> fsck'. While the files-backend has some fsck checks added, the reftable
> >> backend lacks such checks. Let's add the required infrastructure and a
> >> check to test for the table names in the 'tables.list' of reftables.
> >>
> >> For the infrastructure, since the reftable library is treated as an
> >> independent library we should ensure that the library code works
> >> independently without knowledge about Git's internals. To do this,
> >> add both 'reftable/fsck.c' and 'reftable/reftable-fsck.h'. Which
> >
> > A design question here, we name the "fsck.c" for the source code but for
> > the header, we use "reftable-fsck.h", it is a little strange. Why not
> > just "fsck.h" instead of "reftable-fsck.h".
> >
> 
> Since the reftable code is treated as an external library, all
> 'reftable-.*.h' headers are treated as headers which expose APIs for the
> libraries users. We would have defined 'reftable/fsck.h' if there were
> internal users of the 'fsck.c' code. But there are none.
> 

I understand the design. Thanks for the explanation.

[snip]

> >> +		uint32_t rnd;
> >> +		/*
> >> +		 * We want to match the tail '.ref'. One extra byte to ensure
> >> +		 * that there is no unexpected extra character and one byte for
> >> +		 * the null terminator added by sscanf.
> >> +		 */
> >> +		char tail[6];
> >> +
> >> +		if (sscanf(names[i], "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x%5s",
> >> +			   &min, &max, &rnd, tail) != 4) {
> >> +			err = report_fn(info, cb_data);
> >
> > I think we could just pass pointer to avoid unnecessary copy operations.
> > Besides that, I think here we report two different kinds of problem. But
> > we would give report the user always the same message `invalid reftable
> > name`. This is too vague.
> >
> 
> Not sure what you mean by 'unnecessary copy operations', could you
> elaborate?
> 

In `report_fn`, we would copy the `info` value for each call. That's my
meaning.

Thanks,
Jialuo