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

"Catalin Marinas" [off-list ref] writes:

On 08/10/2007, Karl Hasselström [off-list ref] wrote:

quoted

On 2007-10-08 14:16:10 +0100, Catalin Marinas wrote:

quoted

On 14/09/2007, David Kågedal [off-list ref] wrote:

quoted

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

Yes, that was the point of the series.

quoted

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

It might be true that the assertion could be amended so that if
commit=True, then it is allowed to have top/bottom that doesn't
correspond to a commit and its parent. But it's hard to be sure
without looking at the code much harder than I did just now.  Testing
would also help.

quoted

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.

-- 
David Kågedal

On 10/10/2007, David Kågedal [off-list ref] wrote:

"Catalin Marinas" [off-list ref] writes:

quoted

On 08/10/2007, Karl Hasselström [off-list ref] wrote:

quoted

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

It might be true that the assertion could be amended so that if
commit=True, then it is allowed to have top/bottom that doesn't
correspond to a commit and its parent.

I'll fix this and add a test for pick --reverse.

-- 
Catalin