Patchwork [2,of,5] transaction: prevent addfilegenerator from always making backup

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Oct. 9, 2015, 6:59 p.m.
Message ID <b551f3883a6bb1285e83.1444417166@feefifofum>
Download mbox | patch
Permalink /patch/10927/
State Changes Requested
Delegated to: Pierre-Yves David
Headers show

Comments

Katsunori FUJIWARA - Oct. 9, 2015, 6:59 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1444416862 -32400
#      Sat Oct 10 03:54:22 2015 +0900
# Node ID b551f3883a6bb1285e83f257b81d10f11ab26341
# Parent  557573b9247d6bac06d253d32105acfec1253604
transaction: prevent addfilegenerator from always making backup

Before this patch, 'addfilegenerator()'-ed files are always backuped
at 'close()', and restored at rollbacking that transaction.

This works fine in many cases, but not in the corner case for
'.hg/dirstate' below:

  1. commit transaction

     At the end of this step, there are two backup files for
     'dirstate': 'undo.dirstate' and 'undo.backup.dirstate'.

  2. update the working directory to the parent not related with ones
     added at (1)

  3. rollback the transaction committed at (1)

     In this case, 'repo.rollback()' explicitly omits restoring
     dirstate from 'undo.dirstate', because the parent of the working
     directory isn't related with rollbacked revisions (= this
     situation is called as not-"parent gone").

     On the other hand, 'transaction.rollback()' restores files from
     'undo.backup.*' forcibly, and this breaks current dirstate.

To prevent 'addfilegenerator()' from always making backup, this patch
adds 'backup' flag to 'addfilegenerator()'.
Pierre-Yves David - Oct. 11, 2015, 7:11 p.m.
On 10/09/2015 11:59 AM, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1444416862 -32400
> #      Sat Oct 10 03:54:22 2015 +0900
> # Node ID b551f3883a6bb1285e83f257b81d10f11ab26341
> # Parent  557573b9247d6bac06d253d32105acfec1253604
> transaction: prevent addfilegenerator from always making backup

As said some long time ago I'm not a big fan of this. It do not feel 
like it make much sense in the Transaction API and I would like to keep 
it simple and consistent. The rollback special case does not seems 
enough to break the rules.

As far as I understand, the reason we need this is that "rollback after 
update" case. so:

- do we know what is using this case?
- do we really care of keeping the BC on rollback corner case?
- Can we achieve the same result in another way? (eg: in place hack in 
rollback)

As worse if this is really the only way to do that, we should make this 
parament "private" and discourage its usage.

What do you think?
Katsunori FUJIWARA - Oct. 12, 2015, 3:16 p.m.
At Sun, 11 Oct 2015 12:11:53 -0700,
Pierre-Yves David wrote:
> 
> On 10/09/2015 11:59 AM, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1444416862 -32400
> > #      Sat Oct 10 03:54:22 2015 +0900
> > # Node ID b551f3883a6bb1285e83f257b81d10f11ab26341
> > # Parent  557573b9247d6bac06d253d32105acfec1253604
> > transaction: prevent addfilegenerator from always making backup
> 
> As said some long time ago I'm not a big fan of this. It do not feel 
> like it make much sense in the Transaction API and I would like to keep 
> it simple and consistent. The rollback special case does not seems 
> enough to break the rules.
> 
> As far as I understand, the reason we need this is that "rollback after 
> update" case. so:
> 
> - do we know what is using this case?

"working directory status" specific files should use it, but I don't
know other callers, yet.

If changing branch also had to be invisible to external processes
while transaction is running, it would become the next caller, though.

(you would ask me whether there are other 'backup=False' callers or
not, wouldn't you ?)

> - do we really care of keeping the BC on rollback corner case?

IMHO, restoring dirstate at rollbacking in non-"parent-gone" case
confuses end users, because content of files in the working directory
may be far from parents restored.

> - Can we achieve the same result in another way? (eg: in place hack in 
> rollback)

OK, I'll post revised one, which achieves the same result by changing
'repo.rollback()' side.


> As worse if this is really the only way to do that, we should make this 
> parament "private" and discourage its usage.
> 
> What do you think?
> 
> -- 
> Pierre-Yves David
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp

Patch

diff --git a/mercurial/transaction.py b/mercurial/transaction.py
--- a/mercurial/transaction.py
+++ b/mercurial/transaction.py
@@ -249,7 +249,7 @@ 
 
     @active
     def addfilegenerator(self, genid, filenames, genfunc, order=0,
-                         location=''):
+                         location='', backup=True):
         """add a function to generates some files at transaction commit
 
         The `genfunc` argument is a function capable of generating proper
@@ -271,17 +271,22 @@ 
         The `location` arguments may be used to indicate the files are located
         outside of the the standard directory for transaction. It should match
         one of the key of the `transaction.vfsmap` dictionary.
+
+        The `backup` argument is used to decide whether original file
+        is backuped at `close()` or not. Backup is used to restore at
+        rollbacking the transaction regardless of rollbacked revisions.
         """
         # For now, we are unable to do proper backup and restore of custom vfs
         # but for bookmarks that are handled outside this mechanism.
-        self._filegenerators[genid] = (order, filenames, genfunc, location)
+        self._filegenerators[genid] = (order, filenames, genfunc, location,
+                                       backup)
 
     def _generatefiles(self, suffix=''):
         # write files registered for generation
         any = False
         for entry in sorted(self._filegenerators.values()):
             any = True
-            order, filenames, genfunc, location = entry
+            order, filenames, genfunc, location, backup = entry
             vfs = self._vfsmap[location]
             files = []
             try:
@@ -289,7 +294,7 @@ 
                     name += suffix
                     if suffix:
                         self.registertmp(name, location=location)
-                    else:
+                    elif backup:
                         self.addbackup(name, location=location)
                     files.append(vfs(name, 'w', atomictemp=True))
                 genfunc(*files)