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

From: Jeff King <peff@peff.net>
Date: Jul 23, 2021, 07:37 UTC
Message-ID: <YPpx0KoGUX0KfdSw@coredump.intra.peff.net>
In-Reply-To: <YPhXb9Zns8S6aIod@nand.local>

On Wed, Jul 21, 2021 at 01:20:47PM -0400, Taylor Blau wrote:

Show 30 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).

I thought about suggesting that it be called "err" or "ret" or something. And then we do not have to care that fill_bitmap_commit() only returns an error in the non-closed state. We are simply propagating its error-return back up the stack.

And then you can just write:

  ret = fill_bitmap_commit(...);
  if (ret < 0)
	break;

  ...
  return ret;

without an extra conversion. I don't care that much either way, though (but if you like it and are re-rolling anyway... :) ).

-Peff