Re: [PATCH v2 6/8] packed-backend: add "packed-refs" entry consistency check
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Feb 3, 2025, 08:40 UTC
- Message-ID
- <Z6CA-aj7cgAxLTUI@pks.im>
- In-Reply-To
- <Z5r7Hlk_VS0jYU74@ArchLinux>
On Thu, Jan 30, 2025 at 12:07:58PM +0800, shejialuo wrote:
Show 18 quoted lines
> "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.
Show 20 quoted lines
> 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.
Show 6 quoted lines
> + 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.
Show 9 quoted lines
> @@ -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?
Patrick