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:

quoted

quoted

quoted

quoted

+	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").

Thanks,
Taylor