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

From: Catalin Marinas <catalin.marinas@gmail.com>
Date: Sep 14, 2008, 21:19 UTC
Message-ID: <b0943d9e0809141419q6facb21at627e658805f1d223@mail.gmail.com>
In-Reply-To: <20080914085118.GC30664@diana.vm.bytemark.co.uk>

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

Show 7 quoted lines

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

Show 7 quoted lines

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

Show 9 quoted lines

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