Re: [PATCH 1/2] refs/files-backend: handle packed transaction prepare failure

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 22, 2019, 14:34 UTC
Message-ID: <20190322143457.GA14595@Taylors-MacBook-Pro.local>
In-Reply-To: <20190322000601.GA32671@sigill.intra.peff.net>

On Thu, Mar 21, 2019 at 08:06:01PM -0400, Jeff King wrote:

Show 12 quoted lines

> On Thu, Mar 21, 2019 at 05:28:44AM -0400, Jeff King wrote:
>
> >   - instead of disconnecting backend_data->packed_transaction on error,
> >     we could wait to install it until we successfully prepare. That
> >     might make the flow a little simpler, but it introduces a hassle.
> >     Earlier parts of files_transaction_prepare() that encounter an error
> >     will jump to the cleanup label, and expect that cleaning up the
> >     outer transaction will clean up the packed transaction, too. We'd
> >     have to adjust those sites to clean up the packed transaction.
>
> This actually isn't too bad. Here's what it would look like as a
> cleanup patch on top. I dunno if it's worth it or not.

I am quite glad that you tried this out, since I was curious to see how it would look when you mentioned it to Michael. While I think it can often be convenient to have a local variable sharing the address of some other pointer within a struct, I find the mixed usage here somewhat confusing.

So, I think that this patch is worthwhile, but I think you should introduce _this_ as 1/3, and then the existing 1/2 and 2/2 would become 2/3 and 3/3, respectively.

Introducing this as 1/3 means that you don't have to introduce changes that immediately have the variables mentioned in them renamed in a subsequent commit. I'm not sure which you feel is preferable to you, though.

> -- >8 --
>
> [ ... ]

Thanks, Taylor