[Updated] D9237: transaction: only keep file names in-memory for journal [WIP]

[indygreg (Gregory Szorc)]

indygreg added a comment.


  I was just attempting to read the transactions code as part of D9274 <https://phab.mercurial-scm.org/D9274>.
  
  I was skeptical of the need to maintain the in-memory copy of entries to the transaction journal files. It feels like something we shouldn't need to do and my kneejerk reaction is we should rip out this complexity and read from the file handle (as this patch does) instead. Although I would like to go diving into the history to find out why the in-memory copy exists as it may have been introduced for a good reason (performance?). Note that we do unlink the journal files a bit early in transaction commit/rollback code - earlier than I think is reasonable. And our low-level testing around these transaction primitives may be lacking. There be many dragons in this code...
  
  Please proceed with rewriting this as smaller patches. And don't be scared to rename existing methods along the way to make things more clear. e.g. `add()` should probably be something like `record_file_append()`. Yes, we can use `_` in function names now. And if we are changing the API of a function, I would prefer it be renamed at the same time to improve readability. You are already making an API break, so you might as well improve the name...

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D9237/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D9237

To: joerg.sonnenberger, indygreg, #hg-reviewers
Cc: martinvonz, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mercurial-scm.org/pipermail/mercurial-patches/attachments/20201107/5697b770/attachment.html>