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

[Taylor Blau <me@ttaylorr.com>]

On Thu, Mar 21, 2019 at 08:06:01PM -0400, Jeff King wrote:
> 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