Re: [StGit PATCH 09/13] Clear up the semantics of Series.new_patch

From: Catalin Marinas <catalin.marinas@gmail.com>
Date: Oct 9, 2007, 21:01 UTC
Message-ID: <b0943d9e0710091401s280b3a12md9e700fb3ae007bf@mail.gmail.com>
In-Reply-To: <20071008132524.GA11253@diana.vm.bytemark.co.uk>

On 08/10/2007, Karl Hasselström <kha@treskal.com> wrote:

Show 23 quoted lines

> On 2007-10-08 14:16:10 +0100, Catalin Marinas wrote:
>
> > On 14/09/2007, David Kågedal <davidk@lysator.liu.se> wrote:
> >
> > > +        assert commit or (top and bottom)
> > > +        assert not before_existing or (top and bottom)
> > > +        assert not (commit and before_existing)
> > > +        assert (top and bottom) or (not top and not bottom)
> > > +        assert not top or (bottom == git.get_commit(top).get_parent())
> >
> > The last assertion here prevents the use of 'stg pick --reverse'.
> > This command creates an unapplied patch with top and bottom reversed
> > and pushes it to force a three-way merge.
> >
> > It seems to work OK if I comment it out but I wonder whether it will
> > break in the future with the planned removal of the top and bottom
> > files.
>
> I think the assert represents a real constraint, namely that there has
> to be a 1:1 correspondance between patches and commits.
>
> Couldn't "stg pick --reverse" create a new commit and use that? That
> is, given that we want to revert commit C, create a new commit C* with

Series.new_patch already creates a commit, why should we move the functionality to 'pick'? The only call to new_patch with commit=False seems to be from 'uncommit' (and it makes sense indeed).

> And shouldn't there be a test for this? :-)

Yes :-). I think there are many other tests needed. It would be useful to do a code coverage with the existing tests to see what we are missing. Unit testing might be useful as well but we all have limited spare time.

-- 
Catalin