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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 11, 2021, 16:51 UTC
Message-ID: <YTzejKyYvqZu4v16@nand.local>
In-Reply-To: <87sfyb8ihg.fsf@evledraar.gmail.com>

On Sat, Sep 11, 2021 at 01:19:27PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 25 quoted lines

>
> On Sat, Sep 11 2021, Ævar Arnfjörð Bjarmason wrote:
>
> > 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"

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.

Thanks, Taylor