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
