{"thread":{"id":"15507","subject":"[StGit PATCH] Convert \"sink\" to the new infrastructure","startedAt":"2008-09-12T22:01:27Z","lastAt":"2008-09-18T15:47:57Z","messageCount":18,"participants":["Catalin Marinas","Karl Hasselström"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"90608","messageId":"20080912215613.10270.20599.stgit@localhost.localdomain","threadId":"15507","inReplyTo":null,"subject":"[StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-09-12T22:01:27Z","receivedAt":"2008-09-12T22:01:27Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"This patch converts the sink command to use stgit.lib. The behaviour\nis also changed slightly so that it only allows to sink a set of\npatches if there are applied once, otherwise it is equivalent to a\npush. The new implementation also allows to bring a patch forward\ntowards the top based on the --to argument.\n\nSigned-off-by: Catalin Marinas \n---\n\nBefore the final patch, I need to write a better test script.\n\nI'm not sure about the conflict resolution. In this implementation, if\na conflict happens, the transaction is aborted. In case we allow\nconflicts, I have to dig further on how to implement it with the new\ntransaction mechanism (I think \"delete\" does this).\n\nAn additional point - the transaction object supports functions like\npop_patches and push_patch. Should we change them for consistency and\nsimplicity? I.e., apart from current pop_patches with predicate add\nfunctions that support popping a list or a single patch. The same goes\nfor push_patch.\n\n\nstgit/commands/sink.py | 79 ++++++++++++++++++++++++++++++------------------\n 1 files changed, 49 insertions(+), 30 deletions(-)\n\ndiff --git a/stgit/commands/sink.py b/stgit/commands/sink.py\nindex d8f79b4..cb94f99 100644\n--- a/stgit/commands/sink.py\n+++ b/stgit/commands/sink.py\n@@ -16,13 +16,10 @@ along with this program; if not, write to the Free Software\n Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA\n \"\"\"\n \n-import sys, os\n-from optparse import OptionParser, make_option\n-\n-from stgit.commands.common import *\n-from stgit.utils import *\n-from stgit import stack, git\n+from optparse import make_option\n \n+from stgit.commands import common\n+from stgit.lib import transaction\n \n help = 'send patches deeper down the stack'\n usage = \"\"\"%prog [-t ] [-n] []\n@@ -32,7 +29,7 @@ push the specified (the current patch by default), and\n then push back into place the formerly-applied patches (unless -n\n is also given).\"\"\"\n \n-directory = DirectoryGotoToplevel()\n+directory = common.DirectoryHasRepositoryLib()\n options = [make_option('-n', '--nopush',\n help = 'do not push the patches back after sinking',\n action = 'store_true'),\n@@ -42,33 +39,55 @@ options = [make_option('-n', '--nopush',\n def func(parser, options, args):\n \"\"\"Sink patches down the stack.\n \"\"\"\n+ stack = directory.repository.current_stack\n \n- check_local_changes()\n- check_conflicts()\n- check_head_top_equal(crt_series)\n-\n- oldapplied = crt_series.get_applied()\n- unapplied = crt_series.get_unapplied()\n- all = unapplied + oldapplied\n-\n- if options.to and not options.to in oldapplied:\n- raise CmdException('Cannot sink below %s, since it is not applied'\n- % options.to)\n+ if not stack.patchorder.applied:\n+ raise common.CmdException('No patches applied')\n+ if options.to and not options.to in stack.patchorder.applied:\n+ raise common.CmdException('Cannot sink below %s since it is not applied'\n+ % options.to)\n \n if len(args) > 0:\n- patches = parse_patches(args, all)\n+ patches = common.parse_patches(args, stack.patchorder.all)\n else:\n- current = crt_series.get_current()\n- if not current:\n- raise CmdException('No patch applied')\n- patches = [current]\n+ # current patch\n+ patches = stack.patchorder.applied[-1:]\n+\n+ if not patches:\n+ raise common.CmdException('No patches to sink')\n+ if options.to and options.to in patches:\n+ raise common.CmdException('Cannot have a sinked patch as target')\n+\n+ trans = transaction.StackTransaction(stack, 'sink')\n+\n+ # pop any patches to be sinked in case they are applied\n+ to_push = trans.pop_patches(lambda pn: pn in patches)\n+\n+ if options.to:\n+ if options.to in to_push:\n+ # this is the case where sinking actually brings some\n+ # patches forward\n+ for p in to_push:\n+ if p == options.to:\n+ del to_push[:to_push.index(p)]\n+ break\n+ trans.push_patch(p)\n+ else:\n+ # target patch below patches to be sinked\n+ to_pop = trans.applied[trans.applied.index(options.to):]\n+ to_push = to_pop + to_push\n+ trans.pop_patches(lambda pn: pn in to_pop)\n+ else:\n+ # pop all the remaining patches\n+ to_push = trans.applied + to_push\n+ trans.pop_patches(lambda pn: True)\n \n- if oldapplied:\n- crt_series.pop_patch(options.to or oldapplied[0])\n- push_patches(crt_series, patches)\n+ # push the sinked patches\n+ for p in patches:\n+ trans.push_patch(p)\n \n if not options.nopush:\n- newapplied = crt_series.get_applied()\n- def not_reapplied_yet(p):\n- return not p in newapplied\n- push_patches(crt_series, filter(not_reapplied_yet, oldapplied))\n+ for p in to_push:\n+ trans.push_patch(p)\n+\n+ trans.run()\n"},{"id":"90655","messageId":"20080914085118.GC30664@diana.vm.bytemark.co.uk","threadId":"15507","inReplyTo":"20080912215613.10270.20599.stgit@localhost.localdomain","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-09-14T08:51:18Z","receivedAt":"2008-09-14T08:51:18Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-09-12 23:01:27 +0100, Catalin Marinas wrote:\n\n> This patch converts the sink command to use stgit.lib. The behaviour\n> is also changed slightly so that it only allows to sink a set of\n> patches if there are applied once,\n\n\"if they are applied\"?\n\n> I'm not sure about the conflict resolution. In this implementation,\n> if a conflict happens, the transaction is aborted. In case we allow\n> conflicts, I have to dig further on how to implement it with the new\n> transaction mechanism (I think \"delete\" does this).\n\ngoto does it too. The docstring of the StackTransaction class explains\nhow it works (if it doesn't, we need to improve it):\n\n \"\"\"A stack transaction, used for making complex updates to an\n StGit stack in one single operation that will either succeed or\n fail cleanly.\n\n The basic theory of operation is the following:\n\n 1. Create a transaction object.\n\n 2. Inside a::\n\n try\n ...\n except TransactionHalted:\n pass\n\n block, update the transaction with e.g. methods like\n L{pop_patches} and L{push_patch}. This may create new git\n objects such as commits, but will not write any refs; this means\n that in case of a fatal error we can just walk away, no clean-up\n required.\n\n (Some operations may need to touch your index and working tree,\n though. But they are cleaned up when needed.)\n\n 3. After the C{try} block -- wheher or not the setup ran to\n completion or halted part-way through by raising a\n L{TransactionHalted} exception -- call the transaction's L{run}\n method. This will either succeed in writing the updated state to\n your refs and index+worktree, or fail without having done\n anything.\"\"\"\n\nNot all transaction modifications need to be protected by the try\nblock, only those that may actually raise TransactionHalted (i.e.\nthose that may conflict). Specifically, in the code below, you need to\nput push_patch() in a try block. Otherwise that exception will\npropagate all the way up to the top level, and you will never reach\nthe transaction's run() call which is where refs are updated and the\nnew tree checked out.\n\n> An additional point - the transaction object supports functions like\n> pop_patches and push_patch. Should we change them for consistency\n> and simplicity? I.e., apart from current pop_patches with predicate\n> add functions that support popping a list or a single patch. The\n> same goes for push_patch.\n\nThe current set of functions made sense from an implementation\nperspective. But you are right that other variants would be helpful\nfor some callers.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"90696","messageId":"b0943d9e0809141419q6facb21at627e658805f1d223@mail.gmail.com","threadId":"15507","inReplyTo":"20080914085118.GC30664@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-09-14T21:19:41Z","receivedAt":"2008-09-14T21:19:41Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/9/14 Karl Hasselström :\n> On 2008-09-12 23:01:27 +0100, Catalin Marinas wrote:\n>\n>> This patch converts the sink command to use stgit.lib. The behaviour\n>> is also changed slightly so that it only allows to sink a set of\n>> patches if there are applied once,\n>\n> \"if they are applied\"?\n\nWithout the spelling mistakes - \"if there are applied patches (ones)\".\nOf course, unapplied patches can be sinked but when there are no\napplied patches, it is equivalent to a push and decided to make it\nfail.\n\n>> I'm not sure about the conflict resolution. In this implementation,\n>> if a conflict happens, the transaction is aborted. In case we allow\n>> conflicts, I have to dig further on how to implement it with the new\n>> transaction mechanism (I think \"delete\" does this).\n>\n> goto does it too. The docstring of the StackTransaction class explains\n> how it works (if it doesn't, we need to improve it):\n\nI wasn't used to reading documentation in StGit files :-). Thanks for\nthe info, I'll repost. I'll make the default behaviour to cancel the\ntransaction and revert to the original state unless an option is given\nto allow conflicts.\n\n>> An additional point - the transaction object supports functions like\n>> pop_patches and push_patch. Should we change them for consistency\n>> and simplicity? I.e., apart from current pop_patches with predicate\n>> add functions that support popping a list or a single patch. The\n>> same goes for push_patch.\n>\n> The current set of functions made sense from an implementation\n> perspective. But you are right that other variants would be helpful\n> for some callers.\n\nI can see calls to pop_patches(lambda pn: pn in patch_list). I think\nwe could have a helper for this. I'll try to post a patch sometime\nnext week.\n\n-- \nCatalin\n"},{"id":"90718","messageId":"20080915075740.GB14452@diana.vm.bytemark.co.uk","threadId":"15507","inReplyTo":"b0943d9e0809141419q6facb21at627e658805f1d223@mail.gmail.com","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-09-15T07:57:40Z","receivedAt":"2008-09-15T07:57:40Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-09-14 22:19:41 +0100, Catalin Marinas wrote:\n\n> I wasn't used to reading documentation in StGit files :-). Thanks\n> for the info, I'll repost.\n\nIt was you who asked for in-code docs. :-) The new-infrastructure code\nactually looks half decent in epydoc nowadays.\n\n> I'll make the default behaviour to cancel the transaction and revert\n> to the original state unless an option is given to allow conflicts.\n\nWhat I've always wanted is \"sink this patch as far as it will go\nwithout conflicting\". This comes awfully close.\n\nBTW, this kind of flag might potentially be useful in many commands\n(with default value on or off depending on the command). Maybe\n\n --conflicts=roll-back|stop-before|allow\n\nto indicate if the command should roll back the whole operation, stop\njust before the conflicting push, or allow conflicts.\n\n> I can see calls to pop_patches(lambda pn: pn in patch_list). I think\n> we could have a helper for this.\n\nIndeed.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"90755","messageId":"b0943d9e0809150944o71acafe7ndeda500b1fba97df@mail.gmail.com","threadId":"15507","inReplyTo":"20080915075740.GB14452@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-09-15T16:44:38Z","receivedAt":"2008-09-15T16:44:38Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/9/15 Karl Hasselström :\n> On 2008-09-14 22:19:41 +0100, Catalin Marinas wrote:\n>\n>> I wasn't used to reading documentation in StGit files :-). Thanks\n>> for the info, I'll repost.\n>\n> It was you who asked for in-code docs. :-) The new-infrastructure code\n> actually looks half decent in epydoc nowadays.\n\nSince we are talking about this, the transactions documentation\ndoesn't explain when to use a iw and when to pass allow_conflicts. I\nkind of figured out but I'm not convinced. At a first look, passing\nallow_conflicts = True would seem that it may allow conflicts and not\nrevert the changes, however, this only works if I pass an \"iw\". But\npassing it doesn't allow the default case where I want the changes\nreverted.\n\nPlease have a look at the attached patch which is my last version of\nthe sink command rewriting. I'm not that happy (or maybe I don't\nunderstand the reasons) with setting iw = None if not options.conflict\nbut that's the way I could get it to work.\n\n>> I'll make the default behaviour to cancel the transaction and revert\n>> to the original state unless an option is given to allow conflicts.\n>\n> What I've always wanted is \"sink this patch as far as it will go\n> without conflicting\". This comes awfully close.\n\nBut this means that sink would try several consecutive sinks until it\ncan't find one. Not that it is try to implement but I wouldn't\ncomplicate \"sink\" for this. I would rather add support for patch\ndependency tracking (which used to be on the long term wish list). It\nmight be useful for other things as well like mailing a patch together\nwith those on which it depends (like darcs).\n\n> BTW, this kind of flag might potentially be useful in many commands\n> (with default value on or off depending on the command). Maybe\n>\n> --conflicts=roll-back|stop-before|allow\n\nATM, I only added a --conflict option which has the \"allow\" meaning.\n\n-- \nCatalin\n\n\nConvert \"sink\" to the new infrastructure\n\nFrom: Catalin Marinas \n\nThis patch converts the sink command to use stgit.lib. By default, the\ncommand doesn't allow conflicts and it cancels the operations if patches\ncannot be reordered cleanly. With the --conflict options, the command\nstops after the first conflict during the push operations.\n\nSigned-off-by: Catalin Marinas \n---\n stgit/commands/sink.py | 90 +++++++++++++++++++++++++++++++-----------------\n t/t1501-sink.sh | 65 +++++++++++++++++++++++++++++------\n 2 files changed, 112 insertions(+), 43 deletions(-)\n\ndiff --git a/stgit/commands/sink.py b/stgit/commands/sink.py\nindex d8f79b4..a799433 100644\n--- a/stgit/commands/sink.py\n+++ b/stgit/commands/sink.py\n@@ -1,6 +1,6 @@\n \n __copyright__ = \"\"\"\n-Copyright (C) 2007, Yann Dirson \n+Copyright (C) 2008, Catalin Marinas \n \n This program is free software; you can redistribute it and/or modify\n it under the terms of the GNU General Public License version 2 as\n@@ -16,13 +16,10 @@ along with this program; if not, write to the Free Software\n Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA\n \"\"\"\n \n-import sys, os\n-from optparse import OptionParser, make_option\n-\n-from stgit.commands.common import *\n-from stgit.utils import *\n-from stgit import stack, git\n+from optparse import make_option\n \n+from stgit.commands import common\n+from stgit.lib import transaction\n \n help = 'send patches deeper down the stack'\n usage = \"\"\"%prog [-t ] [-n] []\n@@ -32,43 +29,72 @@ push the specified (the current patch by default), and\n then push back into place the formerly-applied patches (unless -n\n is also given).\"\"\"\n \n-directory = DirectoryGotoToplevel()\n+directory = common.DirectoryHasRepositoryLib()\n options = [make_option('-n', '--nopush',\n help = 'do not push the patches back after sinking',\n action = 'store_true'),\n make_option('-t', '--to', metavar = 'TARGET',\n- help = 'sink patches below TARGET patch')]\n+ help = 'sink patches below TARGET patch'),\n+ make_option('-c', '--conflict',\n+ help = 'allow conflicts during the push operations',\n+ action = 'store_true')]\n \n def func(parser, options, args):\n \"\"\"Sink patches down the stack.\n \"\"\"\n+ stack = directory.repository.current_stack\n \n- check_local_changes()\n- check_conflicts()\n- check_head_top_equal(crt_series)\n-\n- oldapplied = crt_series.get_applied()\n- unapplied = crt_series.get_unapplied()\n- all = unapplied + oldapplied\n-\n- if options.to and not options.to in oldapplied:\n- raise CmdException('Cannot sink below %s, since it is not applied'\n- % options.to)\n+ if options.to and not options.to in stack.patchorder.applied:\n+ raise common.CmdException('Cannot sink below %s since it is not applied'\n+ % options.to)\n \n if len(args) > 0:\n- patches = parse_patches(args, all)\n+ patches = common.parse_patches(args, stack.patchorder.all)\n else:\n- current = crt_series.get_current()\n- if not current:\n- raise CmdException('No patch applied')\n- patches = [current]\n+ # current patch\n+ patches = list(stack.patchorder.applied[-1:])\n \n- if oldapplied:\n- crt_series.pop_patch(options.to or oldapplied[0])\n- push_patches(crt_series, patches)\n+ if not patches:\n+ raise common.CmdException('No patches to sink')\n+ if options.to and options.to in patches:\n+ raise common.CmdException('Cannot have a sinked patch as target')\n+\n+ if options.conflict:\n+ iw = stack.repository.default_iw\n+ else:\n+ iw = None\n+ trans = transaction.StackTransaction(stack, 'sink')\n+\n+ # pop any patches to be sinked in case they are applied\n+ to_push = trans.pop_patches(lambda pn: pn in patches)\n+\n+ if options.to:\n+ if options.to in to_push:\n+ # this is the case where sinking actually brings some\n+ # patches forward\n+ for p in to_push:\n+ if p == options.to:\n+ del to_push[:to_push.index(p)]\n+ break\n+ trans.push_patch(p, iw)\n+ else:\n+ # target patch below patches to be sinked\n+ to_pop = trans.applied[trans.applied.index(options.to):]\n+ to_push = to_pop + to_push\n+ trans.pop_patches(lambda pn: pn in to_pop)\n+ else:\n+ # pop all the remaining patches\n+ to_push = trans.applied + to_push\n+ trans.pop_patches(lambda pn: True)\n \n+ # push the sinked and other popped patches\n if not options.nopush:\n- newapplied = crt_series.get_applied()\n- def not_reapplied_yet(p):\n- return not p in newapplied\n- push_patches(crt_series, filter(not_reapplied_yet, oldapplied))\n+ patches.extend(to_push)\n+ try:\n+ for p in patches:\n+ trans.push_patch(p, iw)\n+ except transaction.TransactionHalted:\n+ if not options.conflict:\n+ raise\n+\n+ return trans.run(iw)\ndiff --git a/t/t1501-sink.sh b/t/t1501-sink.sh\nindex 32931cd..b3e2eb3 100755\n--- a/t/t1501-sink.sh\n+++ b/t/t1501-sink.sh\n@@ -5,24 +5,67 @@ test_description='Test \"stg sink\"'\n . ./test-lib.sh\n \n test_expect_success 'Initialize StGit stack' '\n- echo 000 >> x &&\n- git add x &&\n+ echo 0 >> f0 &&\n+ git add f0 &&\n git commit -m initial &&\n- echo 000 >> y &&\n- git add y &&\n- git commit -m y &&\n+ echo 1 >> f1 &&\n+ git add f1 &&\n+ git commit -m p1 &&\n+ echo 2 >> f2 &&\n+ git add f2 &&\n+ git commit -m p2 &&\n+ echo 3 >> f3 &&\n+ git add f3 &&\n+ git commit -m p3 &&\n+ echo 4 >> f4 &&\n+ git add f4 &&\n+ git commit -m p4 &&\n+ echo 22 >> f2 &&\n+ git add f2 &&\n+ git commit -m p22 &&\n stg init &&\n- stg uncommit &&\n- stg pop\n+ stg uncommit p22 p4 p3 p2 p1 &&\n+ stg pop -a\n '\n \n-test_expect_success 'sink without applied patches' '\n+test_expect_success 'sink default without applied patches' '\n command_error stg sink\n '\n \n-test_expect_success 'sink a specific patch without applied patches' '\n- stg sink y &&\n- test $(echo $(stg series --applied --noprefix)) = \"y\"\n+test_expect_success 'sink and reorder specified without applied patches' '\n+ stg sink p2 p1 &&\n+ test \"$(echo $(stg series --applied --noprefix))\" = \"p2 p1\"\n+'\n+\n+test_expect_success 'sink patches to the bottom of the stack' '\n+ stg sink p4 p3 p2 &&\n+ test \"$(echo $(stg series --applied --noprefix))\" = \"p4 p3 p2 p1\"\n+'\n+\n+test_expect_success 'sink current below a target' '\n+ stg sink --to=p2 &&\n+ test \"$(echo $(stg series --applied --noprefix))\" = \"p4 p3 p1 p2\"\n+'\n+\n+test_expect_success 'bring patches forward' '\n+ stg sink --to=p2 p3 p4 &&\n+ test \"$(echo $(stg series --applied --noprefix))\" = \"p1 p3 p4 p2\"\n+'\n+\n+test_expect_success 'sink specified patch below a target' '\n+ stg sink --to=p3 p2 &&\n+ test \"$(echo $(stg series --applied --noprefix))\" = \"p1 p2 p3 p4\"\n+'\n+\n+test_expect_success 'sink with conflict and restore the stack' '\n+ command_error stg sink --to=p2 p22 &&\n+ test \"$(echo $(stg series --applied --noprefix))\" = \"p1 p2 p3 p4\"\n+'\n+\n+test_expect_success 'sink with conflict and do not restore the stack' '\n+ conflict stg sink --conflict --to=p2 p22 &&\n+ test \"$(echo $(stg series --applied --noprefix))\" = \"p1 p22\" &&\n+ test \"$(echo $(stg status --conflict))\" = \"f2\"\n '\n \n test_done\n"},{"id":"90813","messageId":"20080916074024.GA2454@diana.vm.bytemark.co.uk","threadId":"15507","inReplyTo":"b0943d9e0809150944o71acafe7ndeda500b1fba97df@mail.gmail.com","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-09-16T07:40:24Z","receivedAt":"2008-09-16T07:40:24Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-09-15 17:44:38 +0100, Catalin Marinas wrote:\n\n> Since we are talking about this, the transactions documentation\n> doesn't explain when to use a iw and when to pass allow_conflicts. I\n> kind of figured out but I'm not convinced. At a first look, passing\n> allow_conflicts = True would seem that it may allow conflicts and not\n> revert the changes, however, this only works if I pass an \"iw\". But\n> passing it doesn't allow the default case where I want the changes\n> reverted.\n\nIn my experimental branch, one of the patches adds the following piece\nof documentation:\n\n+ @param allow_conflicts: Whether to allow pre-existing conflicts\n+ @type allow_conflicts: bool or function of L{StackTransaction}\"\"\"\n\nThat is, allow_conflicts decides whether to abort the transaction in\ncase there already were conflicts -- undo and friends need to allow\nexisting conflicts, but most other commands just want to abort in that\ncase.\n\nThis should of course have been a separate patch (in kha/safe), but it\nseems I was lazy ...\n\niw is the index+worktree object. The idea is that you provide one if\nyour branch is checked out, and not if not. Operations that have no\nneed of index+worktree, like pop, and push in case automatic merging\nsucceeds, will just work anyway, while operations that need\nindex+worktree, such as a conflicting push, will cause the whole\ntransaction to abort.\n\n> Please have a look at the attached patch which is my last version of\n> the sink command rewriting. I'm not that happy (or maybe I don't\n> understand the reasons) with setting iw = None if not\n> options.conflict but that's the way I could get it to work.\n\nThat's not the right way to do it. iw = None will tell the transaction\nthat you have no index+worktree, so the resulting tree will not be\nchecked out in the end. Since you don't change the set of applied\npatches, and since all the automatic merges succeeded, you'll probably\nget the exact same tree 99% of the time and not notice, but I wouldn't\nrecommend it.\n\nThe right way to do it, I guess, would be to add a\nstop_before_conflicts flag to run(). As for implementing it, note how\nthe \"if merge_conflict:\" conditional in push_patch() delays the\nrecording of the final conflicting push so that the patch log can get\ntwo entries for this transaction, one that undoes just the conflicting\npush and one that undoes it all. It would probably not be hard to\nteach that code to skip the conflicting push altogether.\n\n( Oh, and note that what I just said talks about the \"patch stack\n log\", meaning that I'm talking about the code in kha/experimental.\n The code in kha/safe doesn't look quite the same -- in particular,\n there's no obvious place to place code that ignores the conflicting\n push. Unless you really don't want your sink changes to depend on\n the stack log stuff (e.g. because you doubt you'll be merging it\n anytime soon), I suggest we do this: I'll prepare, and ask you to\n pull, a \"stacklog\" branch, and once you've pulled it we won't rebase\n it anymore. You can merge it directly to your master or publish it\n as a separate development branch, whichever you feel is best. )\n\n> 2008/9/15 Karl Hasselström :\n>\n> > What I've always wanted is \"sink this patch as far as it will go\n> > without conflicting\". This comes awfully close.\n>\n> But this means that sink would try several consecutive sinks until\n> it can't find one. Not that it is try to implement but I wouldn't\n> complicate \"sink\" for this.\n\nOK.\n\n> I would rather add support for patch dependency tracking (which used\n> to be on the long term wish list). It might be useful for other\n> things as well like mailing a patch together with those on which it\n> depends (like darcs).\n\nDo you mean automatically detected dependencies, or dependencies that\nthe user has told us about?\n\n> > BTW, this kind of flag might potentially be useful in many\n> > commands (with default value on or off depending on the command).\n> > Maybe\n> >\n> > --conflicts=roll-back|stop-before|allow\n>\n> ATM, I only added a --conflict option which has the \"allow\" meaning.\n\nOK.\n\n> + if options.conflict:\n> + iw = stack.repository.default_iw\n> + else:\n> + iw = None\n\nAs I said above, this doesn't (or at least isn't supposed to) work.\n\n> + # pop any patches to be sinked in case they are applied\n> + to_push = trans.pop_patches(lambda pn: pn in patches)\n\nI see what made you want those utility functions ...\n\n> + if options.to:\n> + if options.to in to_push:\n> + # this is the case where sinking actually brings some\n> + # patches forward\n> + for p in to_push:\n> + if p == options.to:\n> + del to_push[:to_push.index(p)]\n> + break\n> + trans.push_patch(p, iw)\n> + else:\n> + # target patch below patches to be sinked\n> + to_pop = trans.applied[trans.applied.index(options.to):]\n> + to_push = to_pop + to_push\n> + trans.pop_patches(lambda pn: pn in to_pop)\n> + else:\n> + # pop all the remaining patches\n> + to_push = trans.applied + to_push\n> + trans.pop_patches(lambda pn: True)\n> \n> + # push the sinked and other popped patches\n> if not options.nopush:\n> - newapplied = crt_series.get_applied()\n> - def not_reapplied_yet(p):\n> - return not p in newapplied\n> - push_patches(crt_series, filter(not_reapplied_yet, oldapplied))\n> + patches.extend(to_push)\n> + try:\n> + for p in patches:\n> + trans.push_patch(p, iw)\n\nHave you seen the reorder_patches() function last in transaction.py?\nIt seems you could save a lot of work here by using it.\n\n> + except transaction.TransactionHalted:\n> + if not options.conflict:\n> + raise\n\nNot catching TransactionHalted will have the effect of rolling back\nthe whole transaction if it stops half-way through. But what you\nreally wanted was the new flag I described above, I think.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"90838","messageId":"b0943d9e0809160759w5c9be510t3b33d5d983bff5a7@mail.gmail.com","threadId":"15507","inReplyTo":"20080916074024.GA2454@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-09-16T14:59:31Z","receivedAt":"2008-09-16T14:59:31Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/9/16 Karl Hasselström :\n> On 2008-09-15 17:44:38 +0100, Catalin Marinas wrote:\n>\n>> Since we are talking about this, the transactions documentation\n>> doesn't explain when to use a iw and when to pass allow_conflicts. I\n>> kind of figured out but I'm not convinced. At a first look, passing\n>> allow_conflicts = True would seem that it may allow conflicts and not\n>> revert the changes, however, this only works if I pass an \"iw\". But\n>> passing it doesn't allow the default case where I want the changes\n>> reverted.\n>\n> In my experimental branch, one of the patches adds the following piece\n> of documentation:\n>\n> + @param allow_conflicts: Whether to allow pre-existing conflicts\n> + @type allow_conflicts: bool or function of L{StackTransaction}\"\"\"\n>\n> That is, allow_conflicts decides whether to abort the transaction in\n> case there already were conflicts -- undo and friends need to allow\n> existing conflicts, but most other commands just want to abort in that\n> case.\n\nOK, it is clearer now.\n\n> iw is the index+worktree object. The idea is that you provide one if\n> your branch is checked out, and not if not. Operations that have no\n> need of index+worktree, like pop, and push in case automatic merging\n> succeeds, will just work anyway, while operations that need\n> index+worktree, such as a conflicting push, will cause the whole\n> transaction to abort.\n\nAh, that's the difference. I thought that even if iw isn't passed, it\nuses the default one.\n\n> ( Oh, and note that what I just said talks about the \"patch stack\n> log\", meaning that I'm talking about the code in kha/experimental.\n> The code in kha/safe doesn't look quite the same -- in particular,\n> there's no obvious place to place code that ignores the conflicting\n> push. Unless you really don't want your sink changes to depend on\n> the stack log stuff (e.g. because you doubt you'll be merging it\n> anytime soon), I suggest we do this: I'll prepare, and ask you to\n> pull, a \"stacklog\" branch, and once you've pulled it we won't rebase\n> it anymore. You can merge it directly to your master or publish it\n> as a separate development branch, whichever you feel is best. )\n\nI think we could merge your experimental branch into master. I gave it\na try and seems OK. The only issue I had was that I had an older\nversion of Git and it failed in really weird ways (stg pop still\nbusy-looping after 4 minutes and in another case it failed with broken\npipe). Once I pulled the latest Git, it was fine but we should try to\nbe compatible at least with the Git version in the Debian testing\ndistribution. It might be the patch at the top with diff-ing several\ntrees at once but I haven't checked.\n\nBTW, I ran some benchmarks on stable/master/kha-experimental branches\nwith 300 patches from the 2.6.27-rc5-mm1 kernel. See attached for the\nresults. Since performance was my worry with the stack log stuff, it\nturns out that there isn't a big difference with real patches. I think\npushing can be made even faster by trying a git-apply first and taking\nthe diff from the saved blobs in the log.\n\n>> I would rather add support for patch dependency tracking (which used\n>> to be on the long term wish list). It might be useful for other\n>> things as well like mailing a patch together with those on which it\n>> depends (like darcs).\n>\n> Do you mean automatically detected dependencies, or dependencies that\n> the user has told us about?\n\nAutomatic dependency - if two patches cannot be reorder with regards\nto each-other, one of the depends on the other.\n\n>> + if options.conflict:\n>> + iw = stack.repository.default_iw\n>> + else:\n>> + iw = None\n>\n> As I said above, this doesn't (or at least isn't supposed to) work.\n\nIt should work since the trans.run() command without iw is equivalent\nto trans.run(iw=None).\n\n> Have you seen the reorder_patches() function last in transaction.py?\n> It seems you could save a lot of work here by using it.\n\nNo, I haven't. I'll have a look.\n\n>> + except transaction.TransactionHalted:\n>> + if not options.conflict:\n>> + raise\n>\n> Not catching TransactionHalted will have the effect of rolling back\n> the whole transaction if it stops half-way through. But what you\n> really wanted was the new flag I described above, I think.\n\nOK, if you prepare the stack log, I'll merge it and have a look.\n\nThanks.\n\n-- \nCatalin\n\n\nCPU: Intel Pentium 4 @ 2.5GHz\nMemory: 1GB\n\n2.6.27-rc5-mm1 kernel, 300 patches uncommitted\n\npop/push ran a few times to heat the caches before running the\nbenchmarks.\n\n\nStable stgit (v0.14.3 + some fixes)\n\n$ time stg pop -a\n\nreal\t0m1.775s\nuser\t0m0.956s\nsys\t0m0.724s\n\n$ time stg push -a (fast-forward)\n\nreal\t0m5.001s\nuser\t0m1.844s\nsys\t0m2.860s\n\n$ time stg push -a (no fast-forward)\n\nreal\t1m27.133s\nuser\t0m36.998s\nsys\t0m34.894s\n\n\nCurrent stgit master (no stack log):\n\n$ time stg pop -a\n\nreal\t0m1.621s\nuser\t0m0.820s\nsys\t0m0.688s\n\n$ time stg push -a (fast-forward)\n\nreal\t0m27.205s\nuser\t0m8.741s\nsys\t0m16.849s\n\n$ time stg push -a (no fast-forward)\n\nreal\t2m8.209s\nuser\t0m46.031s\nsys\t0m57.260s\n\n\nkha/experimantal stgit (with stack log):\n\n$ time stg pop -a\n\nreal\t0m2.419s\nuser\t0m1.144s\nsys\t0m1.132s\n\n$ time stg push -a (fast-forward)\n\nreal\t0m29.594s\nuser\t0m9.217s\nsys\t0m17.145s\n\n$ time stg push -a (no fast-forward)\n\nreal\t2m10.270s\nuser\t0m50.919s\nsys\t1m2.088s\n"},{"id":"90859","messageId":"20080916193647.GA12513@diana.vm.bytemark.co.uk","threadId":"15507","inReplyTo":"b0943d9e0809160759w5c9be510t3b33d5d983bff5a7@mail.gmail.com","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-09-16T19:36:47Z","receivedAt":"2008-09-16T19:36:47Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-09-16 15:59:31 +0100, Catalin Marinas wrote:\n\n> 2008/9/16 Karl Hasselström :\n>\n> > iw is the index+worktree object. The idea is that you provide one\n> > if your branch is checked out, and not if not. Operations that\n> > have no need of index+worktree, like pop, and push in case\n> > automatic merging succeeds, will just work anyway, while\n> > operations that need index+worktree, such as a conflicting push,\n> > will cause the whole transaction to abort.\n>\n> Ah, that's the difference. I thought that even if iw isn't passed,\n> it uses the default one.\n\nIt wouldn't be clean of it to do that -- it would be accessing\nnon-local state it had no business knowing about. I try hard to avoid\nthat kind of thing.\n\n> > ( Oh, and note that what I just said talks about the \"patch stack\n> > log\", meaning that I'm talking about the code in\n> > kha/experimental. The code in kha/safe doesn't look quite the\n> > same -- in particular, there's no obvious place to place code\n> > that ignores the conflicting push. Unless you really don't want\n> > your sink changes to depend on the stack log stuff (e.g. because\n> > you doubt you'll be merging it anytime soon), I suggest we do\n> > this: I'll prepare, and ask you to pull, a \"stacklog\" branch,\n> > and once you've pulled it we won't rebase it anymore. You can\n> > merge it directly to your master or publish it as a separate\n> > development branch, whichever you feel is best. )\n>\n> I think we could merge your experimental branch into master. I gave\n> it a try and seems OK. The only issue I had was that I had an older\n> version of Git and it failed in really weird ways (stg pop still\n> busy-looping after 4 minutes and in another case it failed with\n> broken pipe). Once I pulled the latest Git, it was fine but we\n> should try to be compatible at least with the Git version in the\n> Debian testing distribution. It might be the patch at the top with\n> diff-ing several trees at once but I haven't checked.\n\nThere are two patches that depend on new git versions. One needs git\n1.5.6, which is in testing so I'll be pushing that to you; the other\nneeds Junio's master branch, and i won't even consider asking you to\ntake it until it's in a released git.\n\nI hope to push it out to you tonight or tomorrow, but I have a small\npet patch I'd like to finish first, so I might be late.\n\n> BTW, I ran some benchmarks on stable/master/kha-experimental\n> branches with 300 patches from the 2.6.27-rc5-mm1 kernel. See\n> attached for the results. Since performance was my worry with the\n> stack log stuff, it turns out that there isn't a big difference with\n> real patches. I think pushing can be made even faster by trying a\n> git-apply first and taking the diff from the saved blobs in the log.\n\nWhen benchmarking recent StGits, you'll want to try goto as well,\nsince push and pop are not yet new-infrastructure-ized (meaning\nthey're getting slowdowns from the stack log, but no speedups (if any)\nfrom the new infrastructure).\n\n> > On 2008-09-15 17:44:38 +0100, Catalin Marinas wrote:\n> >\n> > > + if options.conflict:\n> > > + iw = stack.repository.default_iw\n> > > + else:\n> > > + iw = None\n> >\n> > As I said above, this doesn't (or at least isn't supposed to)\n> > work.\n>\n> It should work since the trans.run() command without iw is\n> equivalent to trans.run(iw=None).\n\nBut passing iw = None means telling the transaction that you have no\nindex+worktree, so it won't touch them. You'll get no detection of\ndirty tree, or checkout of result tree in the end. Which is not what\nyou indended, IIUC.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"90928","messageId":"b0943d9e0809170455m53eaf677t87e9ade3f001d044@mail.gmail.com","threadId":"15507","inReplyTo":"20080916193647.GA12513@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-09-17T11:55:39Z","receivedAt":"2008-09-17T11:55:39Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/9/16 Karl Hasselström :\n> On 2008-09-16 15:59:31 +0100, Catalin Marinas wrote:\n>> I think we could merge your experimental branch into master. I gave\n>> it a try and seems OK. The only issue I had was that I had an older\n>> version of Git and it failed in really weird ways (stg pop still\n>> busy-looping after 4 minutes and in another case it failed with\n>> broken pipe). Once I pulled the latest Git, it was fine but we\n>> should try to be compatible at least with the Git version in the\n>> Debian testing distribution. It might be the patch at the top with\n>> diff-ing several trees at once but I haven't checked.\n>\n> There are two patches that depend on new git versions. One needs git\n> 1.5.6, which is in testing so I'll be pushing that to you;\n\nOK.\n\n> the other\n> needs Junio's master branch, and i won't even consider asking you to\n> take it until it's in a released git.\n\nCorrect :-)\n\n> I hope to push it out to you tonight or tomorrow, but I have a small\n> pet patch I'd like to finish first, so I might be late.\n\nOK, no problem. I won't have much time before the weekend anyway.\n\n>> BTW, I ran some benchmarks on stable/master/kha-experimental\n>> branches with 300 patches from the 2.6.27-rc5-mm1 kernel. See\n>> attached for the results. Since performance was my worry with the\n>> stack log stuff, it turns out that there isn't a big difference with\n>> real patches. I think pushing can be made even faster by trying a\n>> git-apply first and taking the diff from the saved blobs in the log.\n>\n> When benchmarking recent StGits, you'll want to try goto as well,\n> since push and pop are not yet new-infrastructure-ized (meaning\n> they're getting slowdowns from the stack log, but no speedups (if any)\n> from the new infrastructure).\n\nIndeed, goto is faster even than the stable branch. I attached the new\nfigures. I think it could go even faster if pushing attempts a \"git\napply\" first before the index merge. With the stack log, the patch\ndiff should be saved already so no need for a \"git diff\" (as in the\nstable branch).\n\n-- \nCatalin\n\n\nCPU: Intel Pentium 4 @ 2.5GHz\nMemory: 1GB\n\n2.6.27-rc5-mm1 kernel, 300 patches uncommitted\n\npop/push ran a few times to heat the caches before running the\nbenchmarks.\n\n\nStable stgit (v0.14.3 + some fixes)\n\n$ time stg pop -a\n\nreal\t0m1.775s\nuser\t0m0.956s\nsys\t0m0.724s\n\n$ time stg push -a (fast-forward)\n\nreal\t0m5.001s\nuser\t0m1.844s\nsys\t0m2.860s\n\n$ time stg push -a (no fast-forward)\n\nreal\t1m27.133s\nuser\t0m36.998s\nsys\t0m34.894s\n\n$ time stg goto top-patch (fast-forward)\n\nreal\t0m5.314s\nuser\t0m1.920s\nsys\t0m2.768s\n\n$ time stg goto top-patch (no fast-forward)\n\nreal\t1m39.040s\nuser\t0m37.022s\nsys\t0m35.666s\n\n\nCurrent stgit master (no stack log):\n\n$ time stg pop -a\n\nreal\t0m1.621s\nuser\t0m0.820s\nsys\t0m0.688s\n\n$ time stg push -a (fast-forward)\n\nreal\t0m27.205s\nuser\t0m8.741s\nsys\t0m16.849s\n\n$ time stg push -a (no fast-forward)\n\nreal\t2m8.209s\nuser\t0m46.031s\nsys\t0m57.260s\n\n$ time stg goto top-patch (fast-forward)\n\nreal\t0m10.437s\nuser\t0m2.160s\nsys\t0m2.464s\n\n$ time stg goto top-patch (no fast-forward)\n\nreal\t1m23.244s\nuser\t0m38.158s\nsys\t0m36.086s\n\n\nkha/experimantal stgit (with stack log):\n\n$ time stg pop -a\n\nreal\t0m2.419s\nuser\t0m1.144s\nsys\t0m1.132s\n\n$ time stg push -a (fast-forward)\n\nreal\t0m29.594s\nuser\t0m9.217s\nsys\t0m17.145s\n\n$ time stg push -a (no fast-forward)\n\nreal\t2m10.270s\nuser\t0m50.919s\nsys\t1m2.088s\n\n$ time stg goto top-patch (fast-forward)\n\nreal\t0m2.170s\nuser\t0m1.084s\nsys\t0m0.460s\n\n$ time stg goto top-patch (no fast-forward)\n\nreal\t1m18.271s\nuser\t0m39.026s\nsys\t0m31.938s\n"},{"id":"90933","messageId":"20080917130432.GA26365@diana.vm.bytemark.co.uk","threadId":"15507","inReplyTo":"b0943d9e0809170455m53eaf677t87e9ade3f001d044@mail.gmail.com","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-09-17T13:04:32Z","receivedAt":"2008-09-17T13:04:32Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-09-17 12:55:39 +0100, Catalin Marinas wrote:\n\n> 2008/9/16 Karl Hasselström :\n>\n> > When benchmarking recent StGits, you'll want to try goto as well,\n> > since push and pop are not yet new-infrastructure-ized (meaning\n> > they're getting slowdowns from the stack log, but no speedups (if\n> > any) from the new infrastructure).\n>\n> Indeed, goto is faster even than the stable branch.\n\nWhen push+pop are converted to the new infrastructure, their\nperformance should be identical to goto's.\n\n> I attached the new figures. I think it could go even faster if\n> pushing attempts a \"git apply\" first before the index merge. With\n> the stack log, the patch diff should be saved already so no need for\n> a \"git diff\" (as in the stable branch).\n\nActually, the new infrastructure already uses apply (with fall-back to\nmerge-recursive). It doesn't use the saved diff, though.\n\n> 2.6.27-rc5-mm1 kernel, 300 patches uncommitted\n>\n> pop/push ran a few times to heat the caches before running the\n> benchmarks.\n\nHave you tried the benchmarks I committed a while back?\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"90934","messageId":"20080917130923.GB26365@diana.vm.bytemark.co.uk","threadId":"15507","inReplyTo":"20080917130432.GA26365@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-09-17T13:09:23Z","receivedAt":"2008-09-17T13:09:23Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-09-17 15:04:32 +0200, Karl Hasselström wrote:\n\n> Actually, the new infrastructure already uses apply (with fall-back\n> to merge-recursive).\n\nSpecifically, since\n\n bc1ecd0b (Do simple in-index merge with diff+apply instead of\n read-tree)\n\nand\n\n afa3f9b9 (Reuse the same temp index in a transaction)\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"90954","messageId":"b0943d9e0809170901o15027408w439af4436cfea67c@mail.gmail.com","threadId":"15507","inReplyTo":"20080917130432.GA26365@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-09-17T16:01:22Z","receivedAt":"2008-09-17T16:01:22Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/9/17 Karl Hasselström :\n> Have you tried the benchmarks I committed a while back?\n\nNo, I wanted to see how some real patches behave and I'm pretty\npleased with the result.\n\n-- \nCatalin\n"},{"id":"90955","messageId":"b0943d9e0809170909j4fce34acr8f0b844d0cb5281d@mail.gmail.com","threadId":"15507","inReplyTo":"20080916193647.GA12513@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-09-17T16:09:46Z","receivedAt":"2008-09-17T16:09:46Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/9/16 Karl Hasselström :\n> On 2008-09-16 15:59:31 +0100, Catalin Marinas wrote:\n>\n>> 2008/9/16 Karl Hasselström :\n>>\n>> > iw is the index+worktree object. The idea is that you provide one\n>> > if your branch is checked out, and not if not. Operations that\n>> > have no need of index+worktree, like pop, and push in case\n>> > automatic merging succeeds, will just work anyway, while\n>> > operations that need index+worktree, such as a conflicting push,\n>> > will cause the whole transaction to abort.\n>>\n>> Ah, that's the difference. I thought that even if iw isn't passed,\n>> it uses the default one.\n>\n> It wouldn't be clean of it to do that -- it would be accessing\n> non-local state it had no business knowing about. I try hard to avoid\n> that kind of thing.\n\nI'm still confused by this and I don't think your new flag would help.\nThe meaning of stop_before_conflict is that it won't push the\nconflicting patch but actually leave the stack with several patches\npushed or popped.\n\nWhat I want for sink (and float afterwards) is by default to cancel\nthe whole transaction if there is a conflict and revert the stack to\nit's original state prior to the \"stg sink\" command. What I have in my\ncode:\n\n iw = stack.repository.default_iw\n trans = transaction.StackTransaction(stack, 'sink')\n\n try:\n trans.reorder_patches(applied, unapplied, hidden, iw)\n except transaction.TransactionHalted:\n if not options.conflict:\n ??? here it needs to check out the previous iw\n raise\n\n return trans.run(iw)\n\nIt runs as expected if --conflict is given but in the default case, if\nthere is a conflict, it keeps the original patchorder (as expected)\nbut the worktree isn't clean. What do I replace ??? with to clean the\nwork tree?\n\nBTW, much shorter with reorder_patches.\n\n-- \nCatalin\n"},{"id":"91000","messageId":"20080918071020.GA12550@diana.vm.bytemark.co.uk","threadId":"15507","inReplyTo":"b0943d9e0809170901o15027408w439af4436cfea67c@mail.gmail.com","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-09-18T07:10:20Z","receivedAt":"2008-09-18T07:10:20Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-09-17 17:01:22 +0100, Catalin Marinas wrote:\n\n> 2008/9/17 Karl Hasselström :\n>\n> > Have you tried the benchmarks I committed a while back?\n>\n> No, I wanted to see how some real patches behave and I'm pretty\n> pleased with the result.\n\nIn addition to the synthetic patch series you seem to have in mind,\nthere is also a more than 1000 patches long series from the kernel\nhistory. Try running the setup.sh and take a look (it takes a few\nminutes to run, but you'll only have to do it once because the\nperformance test script is careful not to wreck the repo it works on).\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"91004","messageId":"20080918072450.GB12550@diana.vm.bytemark.co.uk","threadId":"15507","inReplyTo":"b0943d9e0809170909j4fce34acr8f0b844d0cb5281d@mail.gmail.com","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-09-18T07:24:50Z","receivedAt":"2008-09-18T07:24:50Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-09-17 17:09:46 +0100, Catalin Marinas wrote:\n\n> I'm still confused by this and I don't think your new flag would\n> help. The meaning of stop_before_conflict is that it won't push the\n> conflicting patch but actually leave the stack with several patches\n> pushed or popped.\n>\n> What I want for sink (and float afterwards) is by default to cancel\n> the whole transaction if there is a conflict and revert the stack to\n> it's original state prior to the \"stg sink\" command.\n\nAh, OK. Then I think you want something like this:\n\n try:\n trans.reorder_patches(applied, unapplied, hidden, iw)\n except transaction.TransactionHalted:\n if not options.conflict:\n trans.abort(iw)\n raise common.CmdException(\n 'Operation rolled back -- would result in conflicts')\n return trans.run(iw)\n\nBut with a better error message ...\n\nStackTransaction.abort() doesn't have much in the way of\ndocumentation, unfortunately, but what it does is to check out the\ntree we started with. (Nothing else is necessary, since we never touch\nany refs and stuff until the end of StackTransaction.run(). And the\nonly case where we touch the tree is when we need to fall back to\nmerge-recursive.)\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"91022","messageId":"b0943d9e0809180424s61eff16cl8e9911a04e1cca42@mail.gmail.com","threadId":"15507","inReplyTo":"20080918071020.GA12550@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-09-18T11:24:41Z","receivedAt":"2008-09-18T11:24:41Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/9/18 Karl Hasselström :\n> On 2008-09-17 17:01:22 +0100, Catalin Marinas wrote:\n>\n>> 2008/9/17 Karl Hasselström :\n>>\n>> > Have you tried the benchmarks I committed a while back?\n>>\n>> No, I wanted to see how some real patches behave and I'm pretty\n>> pleased with the result.\n>\n> In addition to the synthetic patch series you seem to have in mind,\n> there is also a more than 1000 patches long series from the kernel\n> history. Try running the setup.sh and take a look (it takes a few\n> minutes to run, but you'll only have to do it once because the\n> performance test script is careful not to wreck the repo it works on).\n\nOK, I thought we only had the synthetic patches and haven't bothered\nlooking at the scripts.\n\n-- \nCatalin\n"},{"id":"91023","messageId":"b0943d9e0809180431x30c8f751g374732ee861ffe61@mail.gmail.com","threadId":"15507","inReplyTo":"20080918072450.GB12550@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-09-18T11:31:35Z","receivedAt":"2008-09-18T11:31:35Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/9/18 Karl Hasselström :\n> On 2008-09-17 17:09:46 +0100, Catalin Marinas wrote:\n>\n>> I'm still confused by this and I don't think your new flag would\n>> help. The meaning of stop_before_conflict is that it won't push the\n>> conflicting patch but actually leave the stack with several patches\n>> pushed or popped.\n>>\n>> What I want for sink (and float afterwards) is by default to cancel\n>> the whole transaction if there is a conflict and revert the stack to\n>> it's original state prior to the \"stg sink\" command.\n>\n> Ah, OK. Then I think you want something like this:\n>\n> try:\n> trans.reorder_patches(applied, unapplied, hidden, iw)\n> except transaction.TransactionHalted:\n> if not options.conflict:\n> trans.abort(iw)\n> raise common.CmdException(\n> 'Operation rolled back -- would result in conflicts')\n> return trans.run(iw)\n\nI tried this before but trans.abort(iw) seems to check out the iw\nindex which is the one immediately after the push conflict, though the\nstack is unmodified, i.e. stg status shows some missing files (which\nare added by subsequent patches after the conflicting one) and a\nconflict.\n\nWhat I would need is a way to save the original iw and and run\ntrans.abort(iw_original).\n\nOr simply give up on the --conflict option and always stop after the\nconflict (catch the exception and don't re-raise it). This way we\ndon't have to bother with checking out the initial state. With the\n\"undo\" command in your branch, people could simply revert the stack to\nthe state prior to the sink command. Maybe that's a good idea so that\nwe don't complicate commands further with different conflict\nbehaviours.\n\n-- \nCatalin\n"},{"id":"91032","messageId":"20080918154757.GA19868@diana.vm.bytemark.co.uk","threadId":"15507","inReplyTo":"b0943d9e0809180431x30c8f751g374732ee861ffe61@mail.gmail.com","subject":"Re: [StGit PATCH] Convert \"sink\" to the new infrastructure","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-09-18T15:47:57Z","receivedAt":"2008-09-18T15:47:57Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-09-18 12:31:35 +0100, Catalin Marinas wrote:\n\n> 2008/9/18 Karl Hasselström :\n>\n> > Ah, OK. Then I think you want something like this:\n> >\n> > try:\n> > trans.reorder_patches(applied, unapplied, hidden, iw)\n> > except transaction.TransactionHalted:\n> > if not options.conflict:\n> > trans.abort(iw)\n> > raise common.CmdException(\n> > 'Operation rolled back -- would result in conflicts')\n> > return trans.run(iw)\n>\n> I tried this before but trans.abort(iw) seems to check out the iw\n> index which is the one immediately after the push conflict, though\n> the stack is unmodified, i.e. stg status shows some missing files\n> (which are added by subsequent patches after the conflicting one)\n> and a conflict.\n\nHmm, strange. That's not what I thought it was supposed to do. Look at\nhow coalesce uses it, for example.\n\n> Or simply give up on the --conflict option and always stop after the\n> conflict (catch the exception and don't re-raise it). This way we\n> don't have to bother with checking out the initial state. With the\n> \"undo\" command in your branch, people could simply revert the stack\n> to the state prior to the sink command. Maybe that's a good idea so\n> that we don't complicate commands further with different conflict\n> behaviours.\n\nYes, this is what every other command does, so it makes sense\nconsistency-wise.\n\nBut I liked the idea of your \"roll-back-in-case-of-conflicts\" flag; it\nwould be nice to have in many commands.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"}]}