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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 11, 2021, 10:27 UTC
Message-ID: <871r5v9z3i.fsf@evledraar.gmail.com>
In-Reply-To: <6a1c52181e8c8c9fe2f0e2d7fbeb1057f68c1f3d.1631331139.git.me@ttaylorr.com>

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.

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

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