Re: [PATCH v2 6/8] packed-backend: add "packed-refs" entry consistency check

[shejialuo <shejialuo@gmail.com>]

On Mon, Feb 03, 2025 at 09:40:25AM +0100, Patrick Steinhardt wrote:
> On Thu, Jan 30, 2025 at 12:07:58PM +0800, shejialuo wrote:
> > "packed-backend.c::next_record" will parse the ref entry to check the
> > consistency. This function has already checked the following things:
> > 
> > 1. Parse the main line of the ref entry, if the oid is not correct. It
> >    will die the program. And then it will check whether the next
> >    character of the oid is space. Then it will check whether the refname
> >    is correct.
> > 2. If the next line starts with '^', it will continue to parse the oid
> >    of the peeled oid content and check whether the last character is
> >    '\n'.
> > 
> > We can iterate each line by using the "packed_fsck_ref_next_line"
> > function. Then, create a new fsck message "badPackedRefEntry(ERROR)" to
> > report to the user when something is wrong.
> > 
> > Create two new functions "packed_fsck_ref_main_line" and
> > "packed_fsck_ref_peeled_line" for case 1 and case 2 respectively. Last,
> > update the unit test to exercise the code.
> 
> I think this message is going into too much detail about _how_ you are
> doing things compared to _what_ you are doing and what the intent is.
> 

I think I have caused some confusion here. The reason why I mention what
"next_record" does is that I want to port these two checks. Let me
improve this in the next version. I will highlight more about the
motivation.

> > diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> > index 870c8e7aaa..271c740728 100644
> > --- a/refs/packed-backend.c
> > +++ b/refs/packed-backend.c
> > @@ -1819,10 +1819,86 @@ static int packed_fsck_ref_header(struct fsck_options *o, const char *start, con
> >  	return 0;
> >  }
> >  
> > +static int packed_fsck_ref_peeled_line(struct fsck_options *o,
> > +				       struct ref_store *ref_store,
> > +				       struct strbuf *packed_entry,
> > +				       const char *start, const char *eol)
> > +{
> > +	struct fsck_ref_report report = { 0 };
> > +	struct object_id peeled;
> > +	const char *p;
> > +
> > +	report.path = packed_entry->buf;
> > +
> > +	start++;
> 
> It's a bit weird that we increment `start` here, as it is very intimate
> with how the caller calls us. Might be easier to reason about when the
> caller did this for us.
> 

For each ref entry, we have two pointers, one is the `start` which is
used to indicate the start of the line and `eol` is the end of the line.

Let's see how we call this function:

		if (start < eof && *start == '^') {
			strbuf_reset(&packed_entry);
			strbuf_addf(&packed_entry, "packed-refs line %d", line_number);
			ret |= packed_fsck_ref_next_line(o, &packed_entry, start, eof, &eol);
			ret |= packed_fsck_ref_peeled_line(o, ref_store, &packed_entry,
							   start, eol);
			start = eol + 1;
			line_number++;
		}

The reason why we do this is that we need to skip the '^' character. I
don't do this in the `if` statement. This is because I want to make the
semantics of the `start` variable unchanged.

I would add a comment here to explain why we need to execute "start++".

> > +	if (parse_oid_hex_algop(start, &peeled, &p, ref_store->repo->hash_algo)) {
> > +		return fsck_report_ref(o, &report,
> > +				       FSCK_MSG_BAD_PACKED_REF_ENTRY,
> > +				       "'%.*s' has invalid peeled oid",
> > +				       (int)(eol - start), start);
> > +	}
> 
> All the braces around those single-line return statements can go away.
> 

I see. So, I have misunderstanding here. I have thought that we should
add braces because we have split this single statement into multiple
lines. Let me update this in the next version.

> > @@ -1843,6 +1919,26 @@ static int packed_fsck_ref_content(struct fsck_options *o,
> >  				       "missing header line");
> >  	}
> >  
> > +	while (start < eof) {
> > +		strbuf_reset(&packed_entry);
> > +		strbuf_addf(&packed_entry, "packed-refs line %d", line_number);
> > +		ret |= packed_fsck_ref_next_line(o, &packed_entry, start, eof, &eol);
> > +		ret |= packed_fsck_ref_main_line(o, ref_store, &packed_entry, &refname, start, eol);
> 
> Don't we have to stop in case `next_line()` returns an error?
> 

No, we don't have to stop. We will continue to check the last ref entry,
this is intentional, we still need to check the last ref entry even
though there is no newline. I don't think we should ignore this part.

Thanks,
Jialuo