login
Search
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 = ...
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
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
--
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 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 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 --
It was you who asked for in-code docs. :-) The new-infrastructure code 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 Indeed. -- Karl Hasselström, kha@treskal.com www.treskal.com/kalle --
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 =3D 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 =3D None if not options.conflict 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 ATM, I only added a --conflict option which has the "allow" meaning. --=20 Catalin
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
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 ...Ah, that's the difference. I thought that even if iw isn't passed, it 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 Automatic dependency - if two patches cannot be reorder with regards It should work since the trans.run() command without iw is equivalent OK, if you prepare the stack log, I'll merge it and have a look. Thanks. --=20 Catalin
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 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 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) 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 --
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). --=20 Catalin
When push+pop are converted to the new infrastructure, their Actually, the new infrastructure already uses apply (with fall-back to Have you tried the benchmarks I committed a while back? -- Karl Hasselström, kha@treskal.com www.treskal.com/kalle --
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
--
No, I wanted to see how some real patches behave and I'm pretty pleased with the result. -- Catalin --
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 --
OK, I thought we only had the synthetic patches and haven't bothered looking at the scripts. -- Catalin --
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
--
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
--
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 --
Hmm, strange. That's not what I thought it was supposed to do. Look at 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 --
