[StGit PATCH] Convert "sink" to the new infrastructure

[StGit PATCH] Convert "sink" to the new infrastructure

[Catalin Marinas]

This patch converts the sink command to use stgit.lib. The behaviour
is also changed slightly so that it only allows to sink a set of
patches if there are applied once, otherwise it is equivalent to a
push. The new implementation also allows to bring a patch forward
towards the top based on the --to argument.

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---

Before the final patch, I need to write a better test script.

I'm not sure about the conflict resolution. In this implementation, if
a conflict happens, the transaction is aborted. In case we allow
conflicts, I have to dig further on how to implement it with the new
transaction mechanism (I think "delete" does this).

An additional point - the transaction object supports functions like
pop_patches and push_patch. Should we change them for consistency and
simplicity? I.e., apart from current pop_patches with predicate add
functions that support popping a list or a single patch. The same goes
for push_patch.


stgit/commands/sink.py |   79 ++++++++++++++++++++++++++++++------------------
 1 files changed, 49 insertions(+), 30 deletions(-)

diff --git a/stgit/commands/sink.py b/stgit/commands/sink.py
index d8f79b4..cb94f99 100644
--- a/stgit/commands/sink.py
+++ b/stgit/commands/sink.py
@@ -16,13 +16,10 @@ along with this program; if not, write to the Free Software
 Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 """
 
-import sys, os
-from optparse import OptionParser, make_option
-
-from stgit.commands.common import *
-from stgit.utils import *
-from stgit import stack, git
+from optparse import make_option
 
+from stgit.commands import common
+from stgit.lib import transaction
 
 help = 'send patches deeper down the stack'
 usage = """%prog [-t <target patch>] [-n] [<patches>]
@@ -32,7 +29,7 @@ push the specified <patches> (the current patch by default), and
 then push back into place the formerly-applied patches (unless -n
 is also given)."""
 
-directory = DirectoryGotoToplevel()
+directory = common.DirectoryHasRepositoryLib()
 options = [make_option('-n', '--nopush',
                        help = 'do not push the patches back after sinking',
                        action = 'store_true'),
@@ -42,33 +39,55 @@ options = [make_option('-n', '--nopush',
 def func(parser, options, args):
     """Sink patches down the stack.
     """
+    stack = directory.repository.current_stack
 
-    check_local_changes()
-    check_conflicts()
-    check_head_top_equal(crt_series)
-
-    oldapplied = crt_series.get_applied()
-    unapplied = crt_series.get_unapplied()
-    all = unapplied + oldapplied
-
-    if options.to and not options.to in oldapplied:
-        raise CmdException('Cannot sink below %s, since it is not applied'
-                           % options.to)
+    if not stack.patchorder.applied:
+        raise common.CmdException('No patches applied')
+    if options.to and not options.to in stack.patchorder.applied:
+        raise common.CmdException('Cannot sink below %s since it is not applied'
+                                  % options.to)
 
     if len(args) > 0:
-        patches = parse_patches(args, all)
+        patches = common.parse_patches(args, stack.patchorder.all)
     else:
-        current = crt_series.get_current()
-        if not current:
-            raise CmdException('No patch applied')
-        patches = [current]
+        # current patch
+        patches = stack.patchorder.applied[-1:]
+
+    if not patches:
+        raise common.CmdException('No patches to sink')
+    if options.to and options.to in patches:
+        raise common.CmdException('Cannot have a sinked patch as target')
+
+    trans = transaction.StackTransaction(stack, 'sink')
+
+    # pop any patches to be sinked in case they are applied
+    to_push = trans.pop_patches(lambda pn: pn in patches)
+
+    if options.to:
+        if options.to in to_push:
+            # this is the case where sinking actually brings some
+            # patches forward
+            for p in to_push:
+                if p == options.to:
+                    del to_push[:to_push.index(p)]
+                    break
+                trans.push_patch(p)
+        else:
+            # target patch below patches to be sinked
+            to_pop = trans.applied[trans.applied.index(options.to):]
+            to_push = to_pop + to_push
+            trans.pop_patches(lambda pn: pn in to_pop)
+    else:
+        # pop all the remaining patches
+        to_push = trans.applied + to_push
+        trans.pop_patches(lambda pn: True)
 
-    if oldapplied:
-        crt_series.pop_patch(options.to or oldapplied[0])
-    push_patches(crt_series, patches)
+    # push the sinked patches
+    for p in patches:
+        trans.push_patch(p)
 
     if not options.nopush:
-        newapplied = crt_series.get_applied()
-        def not_reapplied_yet(p):
-            return not p in newapplied
-        push_patches(crt_series, filter(not_reapplied_yet, oldapplied))
+        for p in to_push:
+            trans.push_patch(p)
+
+    trans.run()

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Karl Hasselström]

On 2008-09-12 23:01:27 +0100, Catalin Marinas wrote:

> This patch converts the sink command to use stgit.lib. The behaviour
> is also changed slightly so that it only allows to sink a set of
> patches if there are applied once,

"if they are applied"?

> I'm not sure about the conflict resolution. In this implementation,
> if a conflict happens, the transaction is aborted. In case we allow
> conflicts, I have to dig further on how to implement it with the new
> transaction mechanism (I think "delete" does this).

goto does it too. The docstring of the StackTransaction class explains
how it works (if it doesn't, we need to improve it):

    """A stack transaction, used for making complex updates to an
    StGit stack in one single operation that will either succeed or
    fail cleanly.

    The basic theory of operation is the following:

      1. Create a transaction object.

      2. Inside a::

         try
           ...
         except TransactionHalted:
           pass

      block, update the transaction with e.g. methods like
      L{pop_patches} and L{push_patch}. This may create new git
      objects such as commits, but will not write any refs; this means
      that in case of a fatal error we can just walk away, no clean-up
      required.

      (Some operations may need to touch your index and working tree,
      though. But they are cleaned up when needed.)

      3. After the C{try} block -- wheher or not the setup ran to
      completion or halted part-way through by raising a
      L{TransactionHalted} exception -- call the transaction's L{run}
      method. This will either succeed in writing the updated state to
      your refs and index+worktree, or fail without having done
      anything."""

Not all transaction modifications need to be protected by the try
block, only those that may actually raise TransactionHalted (i.e.
those that may conflict). Specifically, in the code below, you need to
put push_patch() in a try block. Otherwise that exception will
propagate all the way up to the top level, and you will never reach
the transaction's run() call which is where refs are updated and the
new tree checked out.

> An additional point - the transaction object supports functions like
> pop_patches and push_patch. Should we change them for consistency
> and simplicity? I.e., apart from current pop_patches with predicate
> add functions that support popping a list or a single patch. The
> same goes for push_patch.

The current set of functions made sense from an implementation
perspective. But you are right that other variants would be helpful
for some callers.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Catalin Marinas]

2008/9/14 Karl Hasselström <kha@treskal.com>:
> On 2008-09-12 23:01:27 +0100, Catalin Marinas wrote:
>
>> This patch converts the sink command to use stgit.lib. The behaviour
>> is also changed slightly so that it only allows to sink a set of
>> patches if there are applied once,
>
> "if they are applied"?

Without the spelling mistakes - "if there are applied patches (ones)".
Of course, unapplied patches can be sinked but when there are no
applied patches, it is equivalent to a push and decided to make it
fail.

>> I'm not sure about the conflict resolution. In this implementation,
>> if a conflict happens, the transaction is aborted. In case we allow
>> conflicts, I have to dig further on how to implement it with the new
>> transaction mechanism (I think "delete" does this).
>
> goto does it too. The docstring of the StackTransaction class explains
> how it works (if it doesn't, we need to improve it):

I wasn't used to reading documentation in StGit files :-). Thanks for
the info, I'll repost. I'll make the default behaviour to cancel the
transaction and revert to the original state unless an option is given
to allow conflicts.

>> An additional point - the transaction object supports functions like
>> pop_patches and push_patch. Should we change them for consistency
>> and simplicity? I.e., apart from current pop_patches with predicate
>> add functions that support popping a list or a single patch. The
>> same goes for push_patch.
>
> The current set of functions made sense from an implementation
> perspective. But you are right that other variants would be helpful
> for some callers.

I can see calls to pop_patches(lambda pn: pn in patch_list). I think
we could have a helper for this. I'll try to post a patch sometime
next week.

-- 
Catalin

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Karl Hasselström]

On 2008-09-14 22:19:41 +0100, Catalin Marinas wrote:

> I wasn't used to reading documentation in StGit files :-). Thanks
> for the info, I'll repost.

It was you who asked for in-code docs. :-) The new-infrastructure code
actually looks half decent in epydoc nowadays.

> I'll make the default behaviour to cancel the transaction and revert
> to the original state unless an option is given to allow conflicts.

What I've always wanted is "sink this patch as far as it will go
without conflicting". This comes awfully close.

BTW, this kind of flag might potentially be useful in many commands
(with default value on or off depending on the command). Maybe

  --conflicts=roll-back|stop-before|allow

to indicate if the command should roll back the whole operation, stop
just before the conflicting push, or allow conflicts.

> I can see calls to pop_patches(lambda pn: pn in patch_list). I think
> we could have a helper for this.

Indeed.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Catalin Marinas]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

2008/9/15 Karl Hasselström <kha@treskal.com>:
> On 2008-09-14 22:19:41 +0100, Catalin Marinas wrote:
>
>> I wasn't used to reading documentation in StGit files :-). Thanks
>> for the info, I'll repost.
>
> It was you who asked for in-code docs. :-) The new-infrastructure code
> actually looks half decent in epydoc nowadays.

Since we are talking about this, the transactions documentation
doesn't explain when to use a iw and when to pass allow_conflicts. I
kind of figured out but I'm not convinced. At a first look, passing
allow_conflicts = True would seem that it may allow conflicts and not
revert the changes, however, this only works if I pass an "iw". But
passing it doesn't allow the default case where I want the changes
reverted.

Please have a look at the attached patch which is my last version of
the sink command rewriting. I'm not that happy (or maybe I don't
understand the reasons) with setting iw = None if not options.conflict
but that's the way I could get it to work.

>> I'll make the default behaviour to cancel the transaction and revert
>> to the original state unless an option is given to allow conflicts.
>
> What I've always wanted is "sink this patch as far as it will go
> without conflicting". This comes awfully close.

But this means that sink would try several consecutive sinks until it
can't find one. Not that it is try to implement but I wouldn't
complicate "sink" for this. I would rather add support for patch
dependency tracking (which used to be on the long term wish list). It
might be useful for other things as well like mailing a patch together
with those on which it depends (like darcs).

> BTW, this kind of flag might potentially be useful in many commands
> (with default value on or off depending on the command). Maybe
>
>  --conflicts=roll-back|stop-before|allow

ATM, I only added a --conflict option which has the "allow" meaning.

-- 
Catalin

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: convert-sink-to-the-new-infras.patch --]
[-- Type: text/x-diff; name=convert-sink-to-the-new-infras.patch, Size: 7706 bytes --]

Convert "sink" to the new infrastructure

From: Catalin Marinas <catalin.marinas@gmail.com>

This patch converts the sink command to use stgit.lib. By default, the
command doesn't allow conflicts and it cancels the operations if patches
cannot be reordered cleanly. With the --conflict options, the command
stops after the first conflict during the push operations.

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---
 stgit/commands/sink.py |   90 +++++++++++++++++++++++++++++++-----------------
 t/t1501-sink.sh        |   65 +++++++++++++++++++++++++++++------
 2 files changed, 112 insertions(+), 43 deletions(-)

diff --git a/stgit/commands/sink.py b/stgit/commands/sink.py
index d8f79b4..a799433 100644
--- a/stgit/commands/sink.py
+++ b/stgit/commands/sink.py
@@ -1,6 +1,6 @@
 
 __copyright__ = """
-Copyright (C) 2007, Yann Dirson <ydirson@altern.org>
+Copyright (C) 2008, Catalin Marinas <catalin.marinas@gmail.com>
 
 This program is free software; you can redistribute it and/or modify
 it under the terms of the GNU General Public License version 2 as
@@ -16,13 +16,10 @@ along with this program; if not, write to the Free Software
 Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 """
 
-import sys, os
-from optparse import OptionParser, make_option
-
-from stgit.commands.common import *
-from stgit.utils import *
-from stgit import stack, git
+from optparse import make_option
 
+from stgit.commands import common
+from stgit.lib import transaction
 
 help = 'send patches deeper down the stack'
 usage = """%prog [-t <target patch>] [-n] [<patches>]
@@ -32,43 +29,72 @@ push the specified <patches> (the current patch by default), and
 then push back into place the formerly-applied patches (unless -n
 is also given)."""
 
-directory = DirectoryGotoToplevel()
+directory = common.DirectoryHasRepositoryLib()
 options = [make_option('-n', '--nopush',
                        help = 'do not push the patches back after sinking',
                        action = 'store_true'),
            make_option('-t', '--to', metavar = 'TARGET',
-                       help = 'sink patches below TARGET patch')]
+                       help = 'sink patches below TARGET patch'),
+           make_option('-c', '--conflict',
+                       help = 'allow conflicts during the push operations',
+                       action = 'store_true')]
 
 def func(parser, options, args):
     """Sink patches down the stack.
     """
+    stack = directory.repository.current_stack
 
-    check_local_changes()
-    check_conflicts()
-    check_head_top_equal(crt_series)
-
-    oldapplied = crt_series.get_applied()
-    unapplied = crt_series.get_unapplied()
-    all = unapplied + oldapplied
-
-    if options.to and not options.to in oldapplied:
-        raise CmdException('Cannot sink below %s, since it is not applied'
-                           % options.to)
+    if options.to and not options.to in stack.patchorder.applied:
+        raise common.CmdException('Cannot sink below %s since it is not applied'
+                                  % options.to)
 
     if len(args) > 0:
-        patches = parse_patches(args, all)
+        patches = common.parse_patches(args, stack.patchorder.all)
     else:
-        current = crt_series.get_current()
-        if not current:
-            raise CmdException('No patch applied')
-        patches = [current]
+        # current patch
+        patches = list(stack.patchorder.applied[-1:])
 
-    if oldapplied:
-        crt_series.pop_patch(options.to or oldapplied[0])
-    push_patches(crt_series, patches)
+    if not patches:
+        raise common.CmdException('No patches to sink')
+    if options.to and options.to in patches:
+        raise common.CmdException('Cannot have a sinked patch as target')
+
+    if options.conflict:
+        iw = stack.repository.default_iw
+    else:
+        iw = None
+    trans = transaction.StackTransaction(stack, 'sink')
+
+    # pop any patches to be sinked in case they are applied
+    to_push = trans.pop_patches(lambda pn: pn in patches)
+
+    if options.to:
+        if options.to in to_push:
+            # this is the case where sinking actually brings some
+            # patches forward
+            for p in to_push:
+                if p == options.to:
+                    del to_push[:to_push.index(p)]
+                    break
+                trans.push_patch(p, iw)
+        else:
+            # target patch below patches to be sinked
+            to_pop = trans.applied[trans.applied.index(options.to):]
+            to_push = to_pop + to_push
+            trans.pop_patches(lambda pn: pn in to_pop)
+    else:
+        # pop all the remaining patches
+        to_push = trans.applied + to_push
+        trans.pop_patches(lambda pn: True)
 
+    # push the sinked and other popped patches
     if not options.nopush:
-        newapplied = crt_series.get_applied()
-        def not_reapplied_yet(p):
-            return not p in newapplied
-        push_patches(crt_series, filter(not_reapplied_yet, oldapplied))
+        patches.extend(to_push)
+    try:
+        for p in patches:
+            trans.push_patch(p, iw)
+    except transaction.TransactionHalted:
+        if not options.conflict:
+            raise
+
+    return trans.run(iw)
diff --git a/t/t1501-sink.sh b/t/t1501-sink.sh
index 32931cd..b3e2eb3 100755
--- a/t/t1501-sink.sh
+++ b/t/t1501-sink.sh
@@ -5,24 +5,67 @@ test_description='Test "stg sink"'
 . ./test-lib.sh
 
 test_expect_success 'Initialize StGit stack' '
-    echo 000 >> x &&
-    git add x &&
+    echo 0 >> f0 &&
+    git add f0 &&
     git commit -m initial &&
-    echo 000 >> y &&
-    git add y &&
-    git commit -m y &&
+    echo 1 >> f1 &&
+    git add f1 &&
+    git commit -m p1 &&
+    echo 2 >> f2 &&
+    git add f2 &&
+    git commit -m p2 &&
+    echo 3 >> f3 &&
+    git add f3 &&
+    git commit -m p3 &&
+    echo 4 >> f4 &&
+    git add f4 &&
+    git commit -m p4 &&
+    echo 22 >> f2 &&
+    git add f2 &&
+    git commit -m p22 &&
     stg init &&
-    stg uncommit &&
-    stg pop
+    stg uncommit p22 p4 p3 p2 p1 &&
+    stg pop -a
 '
 
-test_expect_success 'sink without applied patches' '
+test_expect_success 'sink default without applied patches' '
     command_error stg sink
 '
 
-test_expect_success 'sink a specific patch without applied patches' '
-    stg sink y &&
-    test $(echo $(stg series --applied --noprefix)) = "y"
+test_expect_success 'sink and reorder specified without applied patches' '
+    stg sink p2 p1 &&
+    test "$(echo $(stg series --applied --noprefix))" = "p2 p1"
+'
+
+test_expect_success 'sink patches to the bottom of the stack' '
+    stg sink p4 p3 p2 &&
+    test "$(echo $(stg series --applied --noprefix))" = "p4 p3 p2 p1"
+'
+
+test_expect_success 'sink current below a target' '
+    stg sink --to=p2 &&
+    test "$(echo $(stg series --applied --noprefix))" = "p4 p3 p1 p2"
+'
+
+test_expect_success 'bring patches forward' '
+    stg sink --to=p2 p3 p4 &&
+    test "$(echo $(stg series --applied --noprefix))" = "p1 p3 p4 p2"
+'
+
+test_expect_success 'sink specified patch below a target' '
+    stg sink --to=p3 p2 &&
+    test "$(echo $(stg series --applied --noprefix))" = "p1 p2 p3 p4"
+'
+
+test_expect_success 'sink with conflict and restore the stack' '
+    command_error stg sink --to=p2 p22 &&
+    test "$(echo $(stg series --applied --noprefix))" = "p1 p2 p3 p4"
+'
+
+test_expect_success 'sink with conflict and do not restore the stack' '
+    conflict stg sink --conflict --to=p2 p22 &&
+    test "$(echo $(stg series --applied --noprefix))" = "p1 p22" &&
+    test "$(echo $(stg status --conflict))" = "f2"
 '
 
 test_done

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Karl Hasselström]

On 2008-09-15 17:44:38 +0100, Catalin Marinas wrote:

> Since we are talking about this, the transactions documentation
> doesn't explain when to use a iw and when to pass allow_conflicts. I
> kind of figured out but I'm not convinced. At a first look, passing
> allow_conflicts = True would seem that it may allow conflicts and not
> revert the changes, however, this only works if I pass an "iw". But
> passing it doesn't allow the default case where I want the changes
> reverted.

In my experimental branch, one of the patches adds the following piece
of documentation:

+        @param allow_conflicts: Whether to allow pre-existing conflicts
+        @type allow_conflicts: bool or function of L{StackTransaction}"""

That is, allow_conflicts decides whether to abort the transaction in
case there already were conflicts -- undo and friends need to allow
existing conflicts, but most other commands just want to abort in that
case.

This should of course have been a separate patch (in kha/safe), but it
seems I was lazy ...

iw is the index+worktree object. The idea is that you provide one if
your branch is checked out, and not if not. Operations that have no
need of index+worktree, like pop, and push in case automatic merging
succeeds, will just work anyway, while operations that need
index+worktree, such as a conflicting push, will cause the whole
transaction to abort.

> Please have a look at the attached patch which is my last version of
> the sink command rewriting. I'm not that happy (or maybe I don't
> understand the reasons) with setting iw = None if not
> options.conflict but that's the way I could get it to work.

That's not the right way to do it. iw = None will tell the transaction
that you have no index+worktree, so the resulting tree will not be
checked out in the end. Since you don't change the set of applied
patches, and since all the automatic merges succeeded, you'll probably
get the exact same tree 99% of the time and not notice, but I wouldn't
recommend it.

The right way to do it, I guess, would be to add a
stop_before_conflicts flag to run(). As for implementing it, note how
the "if merge_conflict:" conditional in push_patch() delays the
recording of the final conflicting push so that the patch log can get
two entries for this transaction, one that undoes just the conflicting
push and one that undoes it all. It would probably not be hard to
teach that code to skip the conflicting push altogether.

( Oh, and note that what I just said talks about the "patch stack
  log", meaning that I'm talking about the code in kha/experimental.
  The code in kha/safe doesn't look quite the same -- in particular,
  there's no obvious place to place code that ignores the conflicting
  push. Unless you really don't want your sink changes to depend on
  the stack log stuff (e.g. because you doubt you'll be merging it
  anytime soon), I suggest we do this: I'll prepare, and ask you to
  pull, a "stacklog" branch, and once you've pulled it we won't rebase
  it anymore. You can merge it directly to your master or publish it
  as a separate development branch, whichever you feel is best. )

> 2008/9/15 Karl Hasselström <kha@treskal.com>:
>
> > What I've always wanted is "sink this patch as far as it will go
> > without conflicting". This comes awfully close.
>
> But this means that sink would try several consecutive sinks until
> it can't find one. Not that it is try to implement but I wouldn't
> complicate "sink" for this.

OK.

> I would rather add support for patch dependency tracking (which used
> to be on the long term wish list). It might be useful for other
> things as well like mailing a patch together with those on which it
> depends (like darcs).

Do you mean automatically detected dependencies, or dependencies that
the user has told us about?

> > BTW, this kind of flag might potentially be useful in many
> > commands (with default value on or off depending on the command).
> > Maybe
> >
> >  --conflicts=roll-back|stop-before|allow
>
> ATM, I only added a --conflict option which has the "allow" meaning.

OK.

> +    if options.conflict:
> +        iw = stack.repository.default_iw
> +    else:
> +        iw = None

As I said above, this doesn't (or at least isn't supposed to) work.

> +    # pop any patches to be sinked in case they are applied
> +    to_push = trans.pop_patches(lambda pn: pn in patches)

I see what made you want those utility functions ...

> +    if options.to:
> +        if options.to in to_push:
> +            # this is the case where sinking actually brings some
> +            # patches forward
> +            for p in to_push:
> +                if p == options.to:
> +                    del to_push[:to_push.index(p)]
> +                    break
> +                trans.push_patch(p, iw)
> +        else:
> +            # target patch below patches to be sinked
> +            to_pop = trans.applied[trans.applied.index(options.to):]
> +            to_push = to_pop + to_push
> +            trans.pop_patches(lambda pn: pn in to_pop)
> +    else:
> +        # pop all the remaining patches
> +        to_push = trans.applied + to_push
> +        trans.pop_patches(lambda pn: True)
>  
> +    # push the sinked and other popped patches
>      if not options.nopush:
> -        newapplied = crt_series.get_applied()
> -        def not_reapplied_yet(p):
> -            return not p in newapplied
> -        push_patches(crt_series, filter(not_reapplied_yet, oldapplied))
> +        patches.extend(to_push)
> +    try:
> +        for p in patches:
> +            trans.push_patch(p, iw)

Have you seen the reorder_patches() function last in transaction.py?
It seems you could save a lot of work here by using it.

> +    except transaction.TransactionHalted:
> +        if not options.conflict:
> +            raise

Not catching TransactionHalted will have the effect of rolling back
the whole transaction if it stops half-way through. But what you
really wanted was the new flag I described above, I think.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Catalin Marinas]

[-- Attachment #1: Type: text/plain, Size: 4494 bytes --]

2008/9/16 Karl Hasselström <kha@treskal.com>:
> On 2008-09-15 17:44:38 +0100, Catalin Marinas wrote:
>
>> Since we are talking about this, the transactions documentation
>> doesn't explain when to use a iw and when to pass allow_conflicts. I
>> kind of figured out but I'm not convinced. At a first look, passing
>> allow_conflicts = True would seem that it may allow conflicts and not
>> revert the changes, however, this only works if I pass an "iw". But
>> passing it doesn't allow the default case where I want the changes
>> reverted.
>
> In my experimental branch, one of the patches adds the following piece
> of documentation:
>
> +        @param allow_conflicts: Whether to allow pre-existing conflicts
> +        @type allow_conflicts: bool or function of L{StackTransaction}"""
>
> That is, allow_conflicts decides whether to abort the transaction in
> case there already were conflicts -- undo and friends need to allow
> existing conflicts, but most other commands just want to abort in that
> case.

OK, it is clearer now.

> iw is the index+worktree object. The idea is that you provide one if
> your branch is checked out, and not if not. Operations that have no
> need of index+worktree, like pop, and push in case automatic merging
> succeeds, will just work anyway, while operations that need
> index+worktree, such as a conflicting push, will cause the whole
> transaction to abort.

Ah, that's the difference. I thought that even if iw isn't passed, it
uses the default one.

> ( Oh, and note that what I just said talks about the "patch stack
>  log", meaning that I'm talking about the code in kha/experimental.
>  The code in kha/safe doesn't look quite the same -- in particular,
>  there's no obvious place to place code that ignores the conflicting
>  push. Unless you really don't want your sink changes to depend on
>  the stack log stuff (e.g. because you doubt you'll be merging it
>  anytime soon), I suggest we do this: I'll prepare, and ask you to
>  pull, a "stacklog" branch, and once you've pulled it we won't rebase
>  it anymore. You can merge it directly to your master or publish it
>  as a separate development branch, whichever you feel is best. )

I think we could merge your experimental branch into master. I gave it
a try and seems OK. The only issue I had was that I had an older
version of Git and it failed in really weird ways (stg pop still
busy-looping after 4 minutes and in another case it failed with broken
pipe). Once I pulled the latest Git, it was fine but we should try to
be compatible at least with the Git version in the Debian testing
distribution. It might be the patch at the top with diff-ing several
trees at once but I haven't checked.

BTW, I ran some benchmarks on stable/master/kha-experimental branches
with 300 patches from the 2.6.27-rc5-mm1 kernel. See attached for the
results. Since performance was my worry with the stack log stuff, it
turns out that there isn't a big difference with real patches. I think
pushing can be made even faster by trying a git-apply first and taking
the diff from the saved blobs in the log.

>> I would rather add support for patch dependency tracking (which used
>> to be on the long term wish list). It might be useful for other
>> things as well like mailing a patch together with those on which it
>> depends (like darcs).
>
> Do you mean automatically detected dependencies, or dependencies that
> the user has told us about?

Automatic dependency - if two patches cannot be reorder with regards
to each-other, one of the depends on the other.

>> +    if options.conflict:
>> +        iw = stack.repository.default_iw
>> +    else:
>> +        iw = None
>
> As I said above, this doesn't (or at least isn't supposed to) work.

It should work since the trans.run() command without iw is equivalent
to trans.run(iw=None).

> Have you seen the reorder_patches() function last in transaction.py?
> It seems you could save a lot of work here by using it.

No, I haven't. I'll have a look.

>> +    except transaction.TransactionHalted:
>> +        if not options.conflict:
>> +            raise
>
> Not catching TransactionHalted will have the effect of rolling back
> the whole transaction if it stops half-way through. But what you
> really wanted was the new flag I described above, I think.

OK, if you prepare the stack log, I'll merge it and have a look.

Thanks.

-- 
Catalin

[-- Attachment #2: stack-log-benchmarks.txt --]
[-- Type: text/plain, Size: 951 bytes --]

CPU: Intel Pentium 4 @ 2.5GHz
Memory: 1GB

2.6.27-rc5-mm1 kernel, 300 patches uncommitted

pop/push ran a few times to heat the caches before running the
benchmarks.


Stable stgit (v0.14.3 + some fixes)

$ time stg pop -a

real	0m1.775s
user	0m0.956s
sys	0m0.724s

$ time stg push -a (fast-forward)

real	0m5.001s
user	0m1.844s
sys	0m2.860s

$ time stg push -a (no fast-forward)

real	1m27.133s
user	0m36.998s
sys	0m34.894s


Current stgit master (no stack log):

$ time stg pop -a

real	0m1.621s
user	0m0.820s
sys	0m0.688s

$ time stg push -a (fast-forward)

real	0m27.205s
user	0m8.741s
sys	0m16.849s

$ time stg push -a (no fast-forward)

real	2m8.209s
user	0m46.031s
sys	0m57.260s


kha/experimantal stgit (with stack log):

$ time stg pop -a

real	0m2.419s
user	0m1.144s
sys	0m1.132s

$ time stg push -a (fast-forward)

real	0m29.594s
user	0m9.217s
sys	0m17.145s

$ time stg push -a (no fast-forward)

real	2m10.270s
user	0m50.919s
sys	1m2.088s

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Karl Hasselström]

On 2008-09-16 15:59:31 +0100, Catalin Marinas wrote:

> 2008/9/16 Karl Hasselström <kha@treskal.com>:
>
> > iw is the index+worktree object. The idea is that you provide one
> > if your branch is checked out, and not if not. Operations that
> > have no need of index+worktree, like pop, and push in case
> > automatic merging succeeds, will just work anyway, while
> > operations that need index+worktree, such as a conflicting push,
> > will cause the whole transaction to abort.
>
> Ah, that's the difference. I thought that even if iw isn't passed,
> it uses the default one.

It wouldn't be clean of it to do that -- it would be accessing
non-local state it had no business knowing about. I try hard to avoid
that kind of thing.

> > ( Oh, and note that what I just said talks about the "patch stack
> >   log", meaning that I'm talking about the code in
> >   kha/experimental. The code in kha/safe doesn't look quite the
> >   same -- in particular, there's no obvious place to place code
> >   that ignores the conflicting push. Unless you really don't want
> >   your sink changes to depend on the stack log stuff (e.g. because
> >   you doubt you'll be merging it anytime soon), I suggest we do
> >   this: I'll prepare, and ask you to pull, a "stacklog" branch,
> >   and once you've pulled it we won't rebase it anymore. You can
> >   merge it directly to your master or publish it as a separate
> >   development branch, whichever you feel is best. )
>
> I think we could merge your experimental branch into master. I gave
> it a try and seems OK. The only issue I had was that I had an older
> version of Git and it failed in really weird ways (stg pop still
> busy-looping after 4 minutes and in another case it failed with
> broken pipe). Once I pulled the latest Git, it was fine but we
> should try to be compatible at least with the Git version in the
> Debian testing distribution. It might be the patch at the top with
> diff-ing several trees at once but I haven't checked.

There are two patches that depend on new git versions. One needs git
1.5.6, which is in testing so I'll be pushing that to you; the other
needs Junio's master branch, and i won't even consider asking you to
take it until it's in a released git.

I hope to push it out to you tonight or tomorrow, but I have a small
pet patch I'd like to finish first, so I might be late.

> BTW, I ran some benchmarks on stable/master/kha-experimental
> branches with 300 patches from the 2.6.27-rc5-mm1 kernel. See
> attached for the results. Since performance was my worry with the
> stack log stuff, it turns out that there isn't a big difference with
> real patches. I think pushing can be made even faster by trying a
> git-apply first and taking the diff from the saved blobs in the log.

When benchmarking recent StGits, you'll want to try goto as well,
since push and pop are not yet new-infrastructure-ized (meaning
they're getting slowdowns from the stack log, but no speedups (if any)
from the new infrastructure).

> > On 2008-09-15 17:44:38 +0100, Catalin Marinas wrote:
> >
> > > +    if options.conflict:
> > > +        iw = stack.repository.default_iw
> > > +    else:
> > > +        iw = None
> >
> > As I said above, this doesn't (or at least isn't supposed to)
> > work.
>
> It should work since the trans.run() command without iw is
> equivalent to trans.run(iw=None).

But passing iw = None means telling the transaction that you have no
index+worktree, so it won't touch them. You'll get no detection of
dirty tree, or checkout of result tree in the end. Which is not what
you indended, IIUC.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Catalin Marinas]

[-- Attachment #1: Type: text/plain, Size: 2116 bytes --]

2008/9/16 Karl Hasselström <kha@treskal.com>:
> On 2008-09-16 15:59:31 +0100, Catalin Marinas wrote:
>> I think we could merge your experimental branch into master. I gave
>> it a try and seems OK. The only issue I had was that I had an older
>> version of Git and it failed in really weird ways (stg pop still
>> busy-looping after 4 minutes and in another case it failed with
>> broken pipe). Once I pulled the latest Git, it was fine but we
>> should try to be compatible at least with the Git version in the
>> Debian testing distribution. It might be the patch at the top with
>> diff-ing several trees at once but I haven't checked.
>
> There are two patches that depend on new git versions. One needs git
> 1.5.6, which is in testing so I'll be pushing that to you;

OK.

> the other
> needs Junio's master branch, and i won't even consider asking you to
> take it until it's in a released git.

Correct :-)

> I hope to push it out to you tonight or tomorrow, but I have a small
> pet patch I'd like to finish first, so I might be late.

OK, no problem. I won't have much time before the weekend anyway.

>> BTW, I ran some benchmarks on stable/master/kha-experimental
>> branches with 300 patches from the 2.6.27-rc5-mm1 kernel. See
>> attached for the results. Since performance was my worry with the
>> stack log stuff, it turns out that there isn't a big difference with
>> real patches. I think pushing can be made even faster by trying a
>> git-apply first and taking the diff from the saved blobs in the log.
>
> When benchmarking recent StGits, you'll want to try goto as well,
> since push and pop are not yet new-infrastructure-ized (meaning
> they're getting slowdowns from the stack log, but no speedups (if any)
> from the new infrastructure).

Indeed, goto is faster even than the stable branch. I attached the new
figures. I think it could go even faster if pushing attempts a "git
apply" first before the index merge. With the stack log, the patch
diff should be saved already so no need for a "git diff" (as in the
stable branch).

-- 
Catalin

[-- Attachment #2: stack-log-benchmarks.txt --]
[-- Type: text/plain, Size: 1474 bytes --]

CPU: Intel Pentium 4 @ 2.5GHz
Memory: 1GB

2.6.27-rc5-mm1 kernel, 300 patches uncommitted

pop/push ran a few times to heat the caches before running the
benchmarks.


Stable stgit (v0.14.3 + some fixes)

$ time stg pop -a

real	0m1.775s
user	0m0.956s
sys	0m0.724s

$ time stg push -a (fast-forward)

real	0m5.001s
user	0m1.844s
sys	0m2.860s

$ time stg push -a (no fast-forward)

real	1m27.133s
user	0m36.998s
sys	0m34.894s

$ time stg goto top-patch (fast-forward)

real	0m5.314s
user	0m1.920s
sys	0m2.768s

$ time stg goto top-patch (no fast-forward)

real	1m39.040s
user	0m37.022s
sys	0m35.666s


Current stgit master (no stack log):

$ time stg pop -a

real	0m1.621s
user	0m0.820s
sys	0m0.688s

$ time stg push -a (fast-forward)

real	0m27.205s
user	0m8.741s
sys	0m16.849s

$ time stg push -a (no fast-forward)

real	2m8.209s
user	0m46.031s
sys	0m57.260s

$ time stg goto top-patch (fast-forward)

real	0m10.437s
user	0m2.160s
sys	0m2.464s

$ time stg goto top-patch (no fast-forward)

real	1m23.244s
user	0m38.158s
sys	0m36.086s


kha/experimantal stgit (with stack log):

$ time stg pop -a

real	0m2.419s
user	0m1.144s
sys	0m1.132s

$ time stg push -a (fast-forward)

real	0m29.594s
user	0m9.217s
sys	0m17.145s

$ time stg push -a (no fast-forward)

real	2m10.270s
user	0m50.919s
sys	1m2.088s

$ time stg goto top-patch (fast-forward)

real	0m2.170s
user	0m1.084s
sys	0m0.460s

$ time stg goto top-patch (no fast-forward)

real	1m18.271s
user	0m39.026s
sys	0m31.938s

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Karl Hasselström]

On 2008-09-17 12:55:39 +0100, Catalin Marinas wrote:

> 2008/9/16 Karl Hasselström <kha@treskal.com>:
>
> > When benchmarking recent StGits, you'll want to try goto as well,
> > since push and pop are not yet new-infrastructure-ized (meaning
> > they're getting slowdowns from the stack log, but no speedups (if
> > any) from the new infrastructure).
>
> Indeed, goto is faster even than the stable branch.

When push+pop are converted to the new infrastructure, their
performance should be identical to goto's.

> I attached the new figures. I think it could go even faster if
> pushing attempts a "git apply" first before the index merge. With
> the stack log, the patch diff should be saved already so no need for
> a "git diff" (as in the stable branch).

Actually, the new infrastructure already uses apply (with fall-back to
merge-recursive). It doesn't use the saved diff, though.

> 2.6.27-rc5-mm1 kernel, 300 patches uncommitted
>
> pop/push ran a few times to heat the caches before running the
> benchmarks.

Have you tried the benchmarks I committed a while back?

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Karl Hasselström]

On 2008-09-17 15:04:32 +0200, Karl Hasselström wrote:

> Actually, the new infrastructure already uses apply (with fall-back
> to merge-recursive).

Specifically, since

  bc1ecd0b (Do simple in-index merge with diff+apply instead of
            read-tree)

and

  afa3f9b9 (Reuse the same temp index in a transaction)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Catalin Marinas]

2008/9/17 Karl Hasselström <kha@treskal.com>:
> Have you tried the benchmarks I committed a while back?

No, I wanted to see how some real patches behave and I'm pretty
pleased with the result.

-- 
Catalin

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Karl Hasselström]

On 2008-09-17 17:01:22 +0100, Catalin Marinas wrote:

> 2008/9/17 Karl Hasselström <kha@treskal.com>:
>
> > Have you tried the benchmarks I committed a while back?
>
> No, I wanted to see how some real patches behave and I'm pretty
> pleased with the result.

In addition to the synthetic patch series you seem to have in mind,
there is also a more than 1000 patches long series from the kernel
history. Try running the setup.sh and take a look (it takes a few
minutes to run, but you'll only have to do it once because the
performance test script is careful not to wreck the repo it works on).

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Catalin Marinas]

2008/9/18 Karl Hasselström <kha@treskal.com>:
> On 2008-09-17 17:01:22 +0100, Catalin Marinas wrote:
>
>> 2008/9/17 Karl Hasselström <kha@treskal.com>:
>>
>> > Have you tried the benchmarks I committed a while back?
>>
>> No, I wanted to see how some real patches behave and I'm pretty
>> pleased with the result.
>
> In addition to the synthetic patch series you seem to have in mind,
> there is also a more than 1000 patches long series from the kernel
> history. Try running the setup.sh and take a look (it takes a few
> minutes to run, but you'll only have to do it once because the
> performance test script is careful not to wreck the repo it works on).

OK, I thought we only had the synthetic patches and haven't bothered
looking at the scripts.

-- 
Catalin

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Catalin Marinas]

2008/9/16 Karl Hasselström <kha@treskal.com>:
> On 2008-09-16 15:59:31 +0100, Catalin Marinas wrote:
>
>> 2008/9/16 Karl Hasselström <kha@treskal.com>:
>>
>> > iw is the index+worktree object. The idea is that you provide one
>> > if your branch is checked out, and not if not. Operations that
>> > have no need of index+worktree, like pop, and push in case
>> > automatic merging succeeds, will just work anyway, while
>> > operations that need index+worktree, such as a conflicting push,
>> > will cause the whole transaction to abort.
>>
>> Ah, that's the difference. I thought that even if iw isn't passed,
>> it uses the default one.
>
> It wouldn't be clean of it to do that -- it would be accessing
> non-local state it had no business knowing about. I try hard to avoid
> that kind of thing.

I'm still confused by this and I don't think your new flag would help.
The meaning of stop_before_conflict is that it won't push the
conflicting patch but actually leave the stack with several patches
pushed or popped.

What I want for sink (and float afterwards) is by default to cancel
the whole transaction if there is a conflict and revert the stack to
it's original state prior to the "stg sink" command. What I have in my
code:

    iw = stack.repository.default_iw
    trans = transaction.StackTransaction(stack, 'sink')

    try:
        trans.reorder_patches(applied, unapplied, hidden, iw)
    except transaction.TransactionHalted:
        if not options.conflict:
            ??? here it needs to check out the previous iw
            raise

    return trans.run(iw)

It runs as expected if --conflict is given but in the default case, if
there is a conflict, it keeps the original patchorder (as expected)
but the worktree isn't clean. What do I replace ??? with to clean the
work tree?

BTW, much shorter with reorder_patches.

-- 
Catalin

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Karl Hasselström]

On 2008-09-17 17:09:46 +0100, Catalin Marinas wrote:

> I'm still confused by this and I don't think your new flag would
> help. The meaning of stop_before_conflict is that it won't push the
> conflicting patch but actually leave the stack with several patches
> pushed or popped.
>
> What I want for sink (and float afterwards) is by default to cancel
> the whole transaction if there is a conflict and revert the stack to
> it's original state prior to the "stg sink" command.

Ah, OK. Then I think you want something like this:

  try:
      trans.reorder_patches(applied, unapplied, hidden, iw)
  except transaction.TransactionHalted:
      if not options.conflict:
          trans.abort(iw)
          raise common.CmdException(
              'Operation rolled back -- would result in conflicts')
  return trans.run(iw)

But with a better error message ...

StackTransaction.abort() doesn't have much in the way of
documentation, unfortunately, but what it does is to check out the
tree we started with. (Nothing else is necessary, since we never touch
any refs and stuff until the end of StackTransaction.run(). And the
only case where we touch the tree is when we need to fall back to
merge-recursive.)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Catalin Marinas]

2008/9/18 Karl Hasselström <kha@treskal.com>:
> On 2008-09-17 17:09:46 +0100, Catalin Marinas wrote:
>
>> I'm still confused by this and I don't think your new flag would
>> help. The meaning of stop_before_conflict is that it won't push the
>> conflicting patch but actually leave the stack with several patches
>> pushed or popped.
>>
>> What I want for sink (and float afterwards) is by default to cancel
>> the whole transaction if there is a conflict and revert the stack to
>> it's original state prior to the "stg sink" command.
>
> Ah, OK. Then I think you want something like this:
>
>  try:
>      trans.reorder_patches(applied, unapplied, hidden, iw)
>  except transaction.TransactionHalted:
>      if not options.conflict:
>          trans.abort(iw)
>          raise common.CmdException(
>              'Operation rolled back -- would result in conflicts')
>  return trans.run(iw)

I tried this before but trans.abort(iw) seems to check out the iw
index which is the one immediately after the push conflict, though the
stack is unmodified, i.e. stg status shows some missing files (which
are added by subsequent patches after the conflicting one) and a
conflict.

What I would need is a way to save the original iw and and run
trans.abort(iw_original).

Or simply give up on the --conflict option and always stop after the
conflict (catch the exception and don't re-raise it). This way we
don't have to bother with checking out the initial state. With the
"undo" command in your branch, people could simply revert the stack to
the state prior to the sink command. Maybe that's a good idea so that
we don't complicate commands further with different conflict
behaviours.

-- 
Catalin

Re: [StGit PATCH] Convert "sink" to the new infrastructure

[Karl Hasselström]

On 2008-09-18 12:31:35 +0100, Catalin Marinas wrote:

> 2008/9/18 Karl Hasselström <kha@treskal.com>:
>
> > Ah, OK. Then I think you want something like this:
> >
> >  try:
> >      trans.reorder_patches(applied, unapplied, hidden, iw)
> >  except transaction.TransactionHalted:
> >      if not options.conflict:
> >          trans.abort(iw)
> >          raise common.CmdException(
> >              'Operation rolled back -- would result in conflicts')
> >  return trans.run(iw)
>
> I tried this before but trans.abort(iw) seems to check out the iw
> index which is the one immediately after the push conflict, though
> the stack is unmodified, i.e. stg status shows some missing files
> (which are added by subsequent patches after the conflicting one)
> and a conflict.

Hmm, strange. That's not what I thought it was supposed to do. Look at
how coalesce uses it, for example.

> Or simply give up on the --conflict option and always stop after the
> conflict (catch the exception and don't re-raise it). This way we
> don't have to bother with checking out the initial state. With the
> "undo" command in your branch, people could simply revert the stack
> to the state prior to the sink command. Maybe that's a good idea so
> that we don't complicate commands further with different conflict
> behaviours.

Yes, this is what every other command does, so it makes sense
consistency-wise.

But I liked the idea of your "roll-back-in-case-of-conflicts" flag; it
would be nice to have in many commands.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle