Re: [PATCH 8/8] builtin/repack.c: pass `--refs-snapshot` when writing bitmaps

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 11, 2021, 11:19 UTC
Message-ID: <87sfyb8ihg.fsf@evledraar.gmail.com>
In-Reply-To: <871r5v9z3i.fsf@evledraar.gmail.com>

On Sat, Sep 11 2021, Ævar Arnfjörð Bjarmason wrote:

Show 14 quoted lines

> On Fri, Sep 10 2021, Taylor Blau wrote:
>
>> +struct midx_snapshot_ref_data {
>> +	struct tempfile *f;
>
> Style/readability: Spare more than one byte for a variable name in a
> struct, maybe file, or tmpfile?
>
>> +	fprintf(data->f->fp, "%s%s\n", data->preferred ? "+" : "",
>> +		oid_to_hex(oid));
>
> Just an idea: Maybe the file could be lines of "+\tOID\n" instead of
> "+OID\n"? Lends itself more naturally to extension, use with the likes
> of string_list_split() etc.

Actually, even better a format like:

    "OID[\t+]\n"

Or

    "OID[\tpreferred=1]\n"

Then you can just use the strbuf API to eat the lines, and only parse any optional \t-delimited "flags" if the length of the line you get doesn't match the hash size length.

I don't think it matters for speed or whatever, just ease of parsing & working with this new format.

The "+" v.s. "preferred=1" suggestion is just to make it future-proof for comma-delimited key-value options, which we already use in the protocol etc., so it's easy to parse, but I don't know if we're ever likely to add a new flag here (and even if we are, we can use the next "\t" positional...