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").
Thanks,
Taylor
