Re: [PATCH v2 02/24] pack-bitmap-write.c: gracefully fail to write non-closed bitmaps

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 21, 2021, 17:20 UTC
Message-ID: <YPhXb9Zns8S6aIod@nand.local>
In-Reply-To: <YPft87yCjR9e+93E@coredump.intra.peff.net>

On Wed, Jul 21, 2021 at 05:50:43AM -0400, Jeff King wrote:

> The amount of error-plumbing you had to do is a little unpleasant, but I
> think is unavoidable. The only non-obvious part was this hunk:

Agreed, at least on the amount of plumbing required to get this to work ;).

Show 19 quoted lines

> > @@ -463,8 +488,11 @@ void bitmap_writer_build(struct packing_data *to_pack)
> >  		struct commit *child;
> >  		int reused = 0;
> >
> > -		fill_bitmap_commit(ent, commit, &queue, &tree_queue,
> > -				   old_bitmap, mapping);
> > +		if (fill_bitmap_commit(ent, commit, &queue, &tree_queue,
> > +				       old_bitmap, mapping) < 0) {
> > +			closed = 0;
> > +			break;
> > +		}
> >
> >  		if (ent->selected) {
> >  			store_selected(ent, commit);
>
> This is the right thing to do because we still want to free memory, stop
> progress, etc. I gave a look over what will run after breaking out of
> the loop, and compute_xor_offsets(), which you already handled, is the
> only thing we'd want to avoid running. Good.

Right. The key is that we return "closed ? 0 : -1" (of course, being careful to invert "closed" where "1" OK into a suitable return value for bitmap_writer_build, where "0" means OK, and a negative number means "error").

While I'm thinking about that inversion, we *could* call this variable "open" and set it to "0" until proven otherwise. Then the conditional becomes "if (!open)", but the return value is still "return open ? -1 : 0" (since I assume we'd want to use 0/1 values for "open" instead of -1, meaning we'd have to do some translation).

Anyway, this is definitely an annoying detail that doesn't really matter (and just rambling on my part) ;).

Thanks, Taylor