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

["Catalin Marinas" <catalin.marinas@gmail.com>]

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