Re: [PATCH] Add a testcase for the safety of pull-policy='pull'.

From: Yann Dirson <ydirson@altern.org>
Date: Mar 1, 2007, 20:10 UTC
Message-ID: <20070301201047.GD4149@nan92-1-81-57-214-146.fbx.proxad.net>
In-Reply-To: <20070228214851.GC4149@nan92-1-81-57-214-146.fbx.proxad.net>

On Wed, Feb 28, 2007 at 10:48:51PM +0100, Yann Dirson wrote:

Show 16 quoted lines

> On Tue, Feb 27, 2007 at 11:38:45PM +0000, Catalin Marinas wrote:
> > >> BTW, push --undo now requires a status --reset beforehand.
> > >
> > >Oh, I had not noticed that.  Why so ?  It's not like if "push --undo"
> > >would lose any valuable data...
> > 
> > I added it so that one cannot remove the local changes by doing a
> > "push --undo" in error. You would have to explicitly ask for local
> > changes removing with status --reset.
> 
> At least, the message in that case should probably be made better,
> when undoing to avoid having to resolve a conflict: the message says I
> cannot undo because I have a conflict, whereas it is the exact reason
> why I want to undo.
> Especially, since the conflict markers are now auto-recorded, we could
> simply allow undo in that case.

Actually, I find the behaviour to be much worse than before: forcing the user to run "status --reset" before "push --undo" indeed brings the patch that conflicted in a state where it is partly committed. That is, if the user is interrupted in her work after resetting, and later comes back, it is not unlikely that she forget that an operation has not been finished, and eg. run "stg pop", in which case the patch would be left in an unwanted state. Eg, I just caused the problem when pushing those patches of mine you integrated: the "editor" patch, which is later modified by one of yours, caused a conflict on puch, which causes an empty patch to be recorded (no idea why there was no conflict markers recorded, BTW).

Indeed, the problem may well be that we should not commit the unresolved conflict. Why it has some value, recording it as if the user had refreshed the patch is probably not a good idea at all, precisely because we're mid-way in the push, and that the operation can be aborted without stgit knowing.

Maybe with transactions we could manage to make something sensible here, but even then, I'm not sure if we would be able to detect that the user's actions should abort the push transaction and rollback to the previous state. Sounds really too much DWIM in the end.

Maybe instead we should write such a commit, but not as if it was a refresh. Maybe under some .../patches/name.conflict ref, leaving the patch itself untouched, but causing the stack to be blocked until the push gets undone (in which case things look simple), or the patch gets resolved+refreshed (in which case, both the conflict and its resolution can appear in the patchlog, but things should be made so "push --undo" would rollback the 2 ones as a whole).

Does that sound sensible ?

Best regards,

-- 
Yann.