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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 11, 2021, 16:49 UTC
Message-ID: <YTzeCD4AKcgTnkm2@nand.local>
In-Reply-To: <871r5v9z3i.fsf@evledraar.gmail.com>

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

Show 8 quoted lines

>
> 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.

Show 15 quoted lines

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

Show 8 quoted lines

>
> > +		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