[Quilt-dev] Re: [PATCH] Don't 'quilt delete' an unrefreshed patch

[Don Mullis]

Martin Panter <address@hidden> writes:
> On 28/12/2009, Don Mullis <address@hidden> wrote:
>> Modify the functionality of `quilt delete' to exit with an error
>>  if it detects unrefreshed changes.
>>
>>  To access the old functionality, a `quilt delete -f' option is
>>  introduced. . . .
>
> Nice. I've got this patch that does pretty much the same thing, but it
> also adds the "-R" from "pop" to "delete". Not that I've ever used
> that option, but it seemed appropriate to include it because it also
> affects how the files are checked for changes. Perhaps you might want
> to include some of my changes in your patch.

Merging in your tidy argument-forwarding code, I come up with the patch
appended (applies against the upstream git tree).  Want to review it?

I do find myself doubting the design tradeoff that has us duplicating so
much of the UI of `quilt pop' in `quilt delete'.  A simpler alternative
would be to limit `delete' to acting only on unapplied patches in the
stack.  If the user really wants to be able to delete the topmost
applied patch with a single command, he can easily enough code up a
private "porcelain-layer" script to do something like:

    quilt pop [-fR] && quilt delete -n

I have written such an alternative patch ... of course, it breaks some
of the existing tests.  And it may also break some Ubuntu packaging
scripts.

Note that Randy Dunlap's RFC "quilt delete any_nonconflicting_patch"
goes in the opposite direction, and rather enlarges the set of applied
patches that `quilt delete' can handle.

Don
---


Modify the functionality of `quilt delete' to exit with an error
if it detects unrefreshed changes.

To access the old functionality, a `quilt delete -f' option is
introduced.  Note that this brings the paradigm of
`quilt delete'/`quilt delete -f' into accord with that of
`quilt pop'/`quilt pop -f'.  For either command, nothing will be
done if there are unrefreshed patches, unless '-f' is specified.
The consistency eliminates a detail that the user formerly had to
remember.

There is a potential to break scripts that might have unwittingly relied
on the old behavior.  A backward-compatible alternative is possible,
but would complicate the UI.

To help motivate this UI change, consider the following misadventure
that arose in a real-life interactive quilt session of an actual user
(who wishes to remain anonymous).

Suppose you're working on a patch p1, then realize that some changes
you're about to add really belong in a separate patch.  So you do

        <save buffers and kill editor with p1 changes>
        quilt new p2
        <add files to p2 and edit them>
        quilt refresh
        quilt pop

Looking again at p1, you realize that it too belongs somewhere else in
the stack, so as the first step in moving it you do ...

         quilt delete

... and all your unrefreshed changes to p1 are lost, irretrievably,
because there was no `quilt refresh' of p1 along the way.
---
 quilt/delete.in  |   13 +++++++++++--
 test/delete.test |   47 ++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 53 insertions(+), 7 deletions(-)

Index: quilt/test/delete.test
===================================================================
--- quilt.orig/test/delete.test 2010-01-03 21:11:05.000000000 -0800
+++ quilt/test/delete.test      2010-01-03 21:15:05.000000000 -0800
@@ -77,9 +77,11 @@ Test the delete command.
        $ touch .pc/test3/dir/file
        $ chmod a-rx .pc/test3/dir
 
-       $ quilt delete "test3"
-       > Removing patch %{P}test3
-       > .pc/test3/dir: Permission denied
+       # `quilt delete' does not specify -f to `quilt pop' now, and the
+       # the latter writes an absolute path into its message on stderr.
+       # Deal with this by throwing stderr away.
+       $ quilt delete "test3" 2>/dev/null || echo "delete failed as expected"
+       > delete failed as expected
 
        $ chmod a+rx .pc/test3/dir
 
@@ -89,16 +91,51 @@ Test the delete command.
        > .pc/test3/dir/file
 
        $ quilt applied
-       > No patches applied
+       > patches/test3
 
        $ quilt series
        > %{P}test3
 
        $ quilt delete
+       > Removing patch patches/test3
        > No patches applied
+       > Removed patch patches/test3
 
        $ quilt delete test3
-       > Removed patch %{P}test3
+       > No patches in series
+
+       $ quilt series
+
+       # Do not allow deletion of an unrefreshed patch
+       $ quilt new test4
+       > Patch %{P}test4 is now on top
+       $ quilt add file
+       > File file added to patch patches/test4
+       $ echo "x" >file
+       $ quilt delete
+       > Patch %{P}test4 does not remove cleanly (refresh it or enforce with 
-f)
+       $ quilt refresh
+       > Refreshed patch %{P}test4
+
+       $ quilt delete
+       > Removing patch patches/test4
+       > No patches applied
+       > Removed patch %{P}test4
+
+       $ quilt series
+
+       # Do not allow deletion of an unrefreshed patch, unless "-f" is 
specified
+       $ quilt new test4
+       > Patch %{P}test4 is now on top
+       $ quilt add file
+       > File file added to patch patches/test4
+       $ echo "x" >file
+       $ quilt delete -f
+       > Removing patch %{P}test4
+       > No patches applied
+       > Removed patch %{P}test4
 
        $ cd ..
        $ rm -rf d
+
+
Index: quilt/quilt/delete.in
===================================================================
--- quilt.orig/quilt/delete.in  2010-01-03 21:21:50.000000000 -0800
+++ quilt/quilt/delete.in       2010-01-03 21:22:06.000000000 -0800
@@ -35,6 +35,12 @@ topmost patch can be removed right now.)
 --backup
        Rename the patch file to patch~ rather than deleting it.
        Ignored if not used with \`-r'.
+
+-f     Force remove. If patch was applied, the previous state will
+       be restored from backup files, and any unrefreshed changes lost.
+
+-R     Always verify if the patch is refreshed; don't rely on timestamp
+       checks.
 "
 
                exit 0
@@ -43,7 +49,7 @@ topmost patch can be removed right now.)
        fi
 }
 
-options=`getopt -o nrh --long backup -- "$@"`
+options=`getopt -o nrhfR --long backup -- "$@"`
 
 if [ $? -ne 0 ]
 then
@@ -66,6 +72,9 @@ do
        --backup)
                QUILT_BACKUP=1
                shift ;;
+       -f | -R)
+               opt_pop="${opt_pop+$opt_pop }$1"
+               shift ;;
        --)
                shift
                break ;;
@@ -103,7 +112,7 @@ if is_applied "$patch"; then
                       "$(print_patch "$patch")" >&2
                exit 1
        fi
-       if ! quilt_command pop -fq
+       if ! quilt_command pop -q $opt_pop
        then
                exit 1
        fi