import --no-commit and rollback

Andreas Bernauer ab-mercurial at lysium.de
Tue Mar 20 02:50:34 CDT 2012


On 3/17/12 3:27, Greg Ward wrote:
> [moving to mercurial-devel]
> 
> On 16 March 2012, Andreas Bernauer said:
>> On 3/16/12 3:08, Roman Neuhauser wrote:
>>> hello,
>>>
>>> i wouldn't expect import --no-commit to be considered rollback-worthy.
>>> is looks like a bug.  the script is attached.
>>
>>
>> I wouldn't expect that, either.  This patch against 594fc9329628 may fix it:
>>
>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>> --- a/mercurial/commands.py
>> +++ b/mercurial/commands.py
>> @@ -3598,7 +3598,10 @@
>>          try:
>>              wlock = repo.wlock()
>>              lock = repo.lock()
>> -            tr = repo.transaction('import')
>> +            if opts.get('no_commit'):
>> +                tr = None
>> +            else:
>> +                tr = repo.transaction('import')
> 
> Yuck: that duplicates the "if nocommit" case in widely separated
> places, and makes the code even more deeply nested than it already is.
> 

Yes. Another approach would be to have transaction not storing empty
transactions. Is that feasible?


> Maybe a better approach: outside the outer try, do this:
> 
>   if opts.get('no_commit'):
>       def start():
>           wlock = repo.wlock()
>       def finish():
>           release(wlock)
>   else:
>       def start():
>           wlock = repo.wlock()
>           lock = repo.lock()
>           tr = repo.transaction('import')
>       def finish():
>           if tr:
>               tr.release()
>           release(lock, wlock)
> 
> Now there is the minor implementation detail that closures cannot
> assign to variables in their containing scope. But they can mutate
> objects in the containing scope, so there's probably a way to do this
> without adding duplicate conditional logic in deeply nested and widely
> separated try/try/finally blocks.

I think you'd like to have start() return the locks and transaction
and pass it into finish(). Below is another try.

The only 'special case' is in the second hunk that checks if tr is
something. However, the same test happened in the finally clause, so if
tr could be None in the finally clause, it should have been checked in
the try-body, too. But I may be missing something. I'm willing to
improve that patch.

# HG changeset patch
# User Andreas Bernauer <ab-mercurial at lysium.de>
# Date 1331895009 -3600
# Node ID 2522c89cb7ab2b9ee7aaa525eecb2b54f70065d6
# Parent  7b9bf72430bab927974f4e9404880271e3dbd2e6
Don't create transaction when import with '--no-commit' (issue3323)

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -3619,11 +3619,24 @@
         finally:
             os.unlink(tmpname)

+    if opts.get('no_commit'):
+        def start():
+            return (repo.wlock(), None, None)
+        def finish(wlock, lock, tr):
+            release(wlock)
+    else:
+        def start():
+            return (repo.wlock(),
+                    repo.lock(),
+                    repo.transaction('import'))
+        def finish(wlock, lock, tr):
+            if tr:
+                tr.release()
+            release(lock, wlock)
+
     try:
         try:
-            wlock = repo.wlock()
-            lock = repo.lock()
-            tr = repo.transaction('import')
+            (wlock, lock, tr) = start()
             parents = repo.parents()
             for patchurl in patches:
                 if patchurl == '-':
@@ -3649,7 +3662,8 @@
                 if not haspatch:
                     raise util.Abort(_('%s: no diffs found') % patchurl)

-            tr.close()
+            if tr:
+                tr.close()
             if msgs:
                 repo.savecommitmessage('\n* * *\n'.join(msgs))
         except:
@@ -3659,9 +3673,7 @@
             repo.dirstate.invalidate()
             raise
     finally:
-        if tr:
-            tr.release()
-        release(lock, wlock)
+        finish(wlock, lock, tr)

 @command('incoming|in',
     [('f', 'force', None,


More information about the Mercurial-devel mailing list