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

> On Tue, Sep 14, 2021 at 02:55:03PM -0400, Jeff King wrote:
>> On Sat, Sep 11, 2021 at 12:51:24PM -0400, Taylor Blau wrote:
>>
>> > > >> +	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"
>> >
>> > Sure, but I admit that I'm a little torn on this suggestion. I don't
>> > want to be naive and say that we're never going to change this format
>> > and paint ourselves into a corner.
>> >
>> > On the other hand, changing it does seem extremely unlikely to me, and
>> > this tab-delimited thing feels like overkill compared to how simple the
>> > '+' format is.
>> >
>> > So, I don't know. It's certainly easy enough to change now before we
>> > lock it in, so I guess we should.
>>
>> I'm not sure I really see the point of making this infinitely
>> extensible. This is mostly an internal interface between two Git
>> programs. Sure, it's exposed to the user in the sense that they can use
>> --refs-snapshot themselves. But if some writer wants to add a "foo"
>> flag, do they really want to be able to do it in a way that they're
>> _syntactically_ compatible with the older versions of Git, yet have no
>> clue if their option was actually recognized and accepted?
>
> I like this perspective, and tend to agree.
>
> (I'm basically repeating what you're saying but) it seems to me that
> trying to make the interface of `--refs-snapshot` compatible across
> versions of Git is stretching what we consider to be our compatibility
> guarantee given that:
>
>   - the interface is basically expected to be private between `repack`
>     and `multi-pack-index`, and
>
>   - an infinitely-extensible version of `--refs-snapshot` would still
>     have no way to tell the caller whether or not those new flags were
>     accepted
>
> So I tend to agree the existing format works fine and we shouldn't spend
> a ton of time trying to over-engineer a solution.
>
> (FWIW, the `+` vs `-` thing is intentional; `--stdin-packs` uses `-` to
> _exclude_ packs, but `+` here means "this one is special" not "exclude
> this ref").