patchConvert "sink" to the new infrastructure

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(-)

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

"if they are applied"?

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.

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

2008/9/14 Karl Hasselström <kha@treskal.com>:

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 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.

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.

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

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

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.

Indeed.

2008/9/15 Karl Hasselström <kha@treskal.com>:

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.

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).

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

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

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.

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. )

OK.

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

OK.

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

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

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

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.

2008/9/16 Karl Hasselström <kha@treskal.com>:

OK, it is clearer now.

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

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.

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

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

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

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

Thanks.

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

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.

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.

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).

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.

2008/9/16 Karl Hasselström <kha@treskal.com>:

OK.

Correct :-)

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

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).

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

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

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

Have you tried the benchmarks I committed a while back?

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

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)

2008/9/17 Karl Hasselström <kha@treskal.com>:

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

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

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).

2008/9/18 Karl Hasselström <kha@treskal.com>:

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

2008/9/16 Karl Hasselström <kha@treskal.com>:

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.

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

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.)

2008/9/18 Karl Hasselström <kha@treskal.com>:

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.

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

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

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.