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

From: Taylor Blau <me@ttaylorr.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, peff@peff.net
Subject: Re: [PATCH 8/8] builtin/repack.c: pass `--refs-snapshot` when writing bitmaps
Date: Sat, 11 Sep 2021 12:49:12 -0400	[thread overview]
Message-ID: <YTzeCD4AKcgTnkm2@nand.local> (raw)
In-Reply-To: <871r5v9z3i.fsf@evledraar.gmail.com>

On Sat, Sep 11, 2021 at 12:27:39PM +0200, Æ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?

Sure.

> > +	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.
>
> > +		for_each_string_list_item(item, preferred) {
> > +			for_each_ref_in(item->string, midx_snapshot_ref_one, &data);
> > +		}
>
> Cheap style commenst I know, but throughout this series there's an odd
> mixture of sometimes adding braces for one-lines, sometimes not, or in
> the case of that "else ;" I commented on not for something *much* bigger
> :)

I've always been uncomfortable with brace-less for loops when the loop
is hidden behind a macro. I haven't bothered to check, but my gut
feeling is that it is pretty rare to see a for_each_something() macro
with a one-line body that doesn't have braces.

But my gut is wrong, because

    git grep 'for_each_.*($' **/*.c | wc -l

turns up 99 results! So I'm happy to be more consistent with the other
code (despite my general discomfort with it). An observation is that
even this file isn't consistent about braceless macro'd for loops, even
before this series.

So even cleaning it up will still leave this file in an inconsistent
state, but I don't think it's worth spending much time at all thinking
about cleaning it up.

>
> > +		if (midx_snapshot_refs(refs_snapshot) < 0)
> > +			die(_("could not take a snapshot of references"));
>
> Seems like this die() should be moved into the midx_snapshot_refs()
> function and that function should use die_errno(), not die(), also it
> uses close_tempfile_gently(), shouldn't save the errno on failure there,
> clean up the tempfile, and then die?

Yeah, makes sense.

Thanks,
Taylor