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@xxxxxxxxxxx>:
>
> > 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@xxxxxxxxxxx
www.treskal.com/kalle
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
