From: David Kågedal Date: Wed, 10 Oct 2007 07:43:25 GMT Subject: Re: [StGit PATCH 09/13] Clear up the semantics of Series.new_patch Message-ID: <87prznxvmq.fsf@lysator.liu.se> In-Reply-To: "Catalin Marinas" writes: > On 08/10/2007, Karl Hasselström wrote: >> On 2007-10-08 14:16:10 +0100, Catalin Marinas wrote: >> >> > On 14/09/2007, David Kågedal 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. Yes, that was the point of the series. >> 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. >> 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