Re: [PATCH v4 4/8] packed-backend: add "packed-refs" header consistency check

[shejialuo <shejialuo@gmail.com>]

On Fri, Feb 14, 2025 at 02:30:45AM -0800, Karthik Nayak wrote:
> shejialuo <shejialuo@gmail.com> writes:

[snip]

> > 2. If the header content does not start with "# packed-ref with: ", we
> >    should report an error just like what "create_snapshot" does. So,
> >    create a new fsck message "badPackedRefHeader(ERROR)" for this.
> > 3. If the header content is not the same as the constant string
> >    "PACKED_REFS_HEADER". This is expected because we make it extensible
> >    intentionally. So, there is no need to report.
> 
> Do you think it's worthwhile adding a warning/info here? This would
> allow users to re-run 'git pack-refs' to ensure that they have a more
> up-to date version of 'packed-refs'.
> 

I somehow agree with you here. But Junio worries about the
compatibility. You could see [1] about this discussion:

[1] https://lore.kernel.org/git/xmqq1pwkdt7r.fsf@gitster.g/

[snip]

> > +static int packed_fsck_ref_content(struct fsck_options *o,
> > +				   const char *start, const char *eof)
> > +{
> > +	unsigned long line_number = 1;
> > +	const char *eol;
> > +	int ret = 0;
> > +
> > +	ret |= packed_fsck_ref_next_line(o, line_number, start, eof, &eol);
> > +	if (*start == '#') {
> > +		ret |= packed_fsck_ref_header(o, start, eol);
> > +
> > +		start = eol + 1;
> > +		line_number++;
> 
> Why do we increment `line_number` here? There is no usage beyond this.
> 

We will use this variable when iterating the next line (ref entries). It
will be used in next patch.

> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  static int packed_fsck(struct ref_store *ref_store,
> >  		       struct fsck_options *o,
> >  		       struct worktree *wt)
> >  {
> >  	struct packed_ref_store *refs = packed_downcast(ref_store,
> >  							REF_STORE_READ, "fsck");
> > +	struct strbuf packed_ref_content = STRBUF_INIT;
> >  	int ret = 0;
> >  	int fd;
> >
> > @@ -1786,7 +1850,16 @@ static int packed_fsck(struct ref_store *ref_store,
> >  		goto cleanup;
> >  	}
> >
> > +	if (strbuf_read(&packed_ref_content, fd, 0) < 0) {
> > +		ret = error_errno(_("unable to read %s"), refs->path);
> > +		goto cleanup;
> > +	}
> > +
> 
> So we want to parse the whole ref content to a buffer, wonder if it
> makes more sense to use `strbuf_read_line()` here instead. But let's
> carry on.
> 

We may use `strbuf_read_line`. But I don't want to do this. My check
logic is the same as the parse logic ("create_snapshot" and "next_record").
I want to keep the logic nearly the same. So maybe one day, we may
refactor the code to make the parse and check use the same code. But at
now, this is difficult.

Thanks,
Jialuo