Re: [Quilt-dev] [PATCH] Add drop option to pop command

[Jean Delvare]

Hi Josh,

> > As suggested a while ago, my proposed quilt drop command was so
> > similar to quilt pop that I've implemented it's functionality as an
> > option to pop instead.  Below is the patch.
> > 
> > Questions/comments always welcome.  If this is more acceptable for
> > inclusion, that's great.  And if not, that's ok too.  I can always
> > use quilt to keep it applied to my local copy ;).

Here comes my review of your patch.

> --- quilt.orig/quilt/pop.in
> +++ quilt/quilt/pop.in
> @@ -19,7 +19,7 @@ fi
>  
>  usage()
>  {
> -     printf $"Usage: quilt pop [-afRqv] [num|patch]\n"
> +     printf $"Usage: quilt pop [-adfRqv] [num|patch]\n"

I think I understand that -d and -R are mutually exclusive, so maybe
this should rather read:

Usage: quilt pop [-azqv] [-R|-d] [num|patch]

> +-d   Drop patch(es).  (Leaves them appiled to the source tree)

s/appiled/applied. Also, I think s/Leaves/Leave/ and a trailing dot
would be welcome.

> -             if [ -z "$(shopt -s nullglob ; echo "$QUILT_PC/$patch/"*)" ]
> +             if [ -n "$opt_drop" ]
>               then
> -                     printf $"Patch %s appears to be empty, removing\n" \
> -                            "$(print_patch $patch)"
> -                     status=0
> -             else
> -                     printf $"Removing patch %s\n" "$(print_patch $patch)"
> -                     @LIB@/backup-files $silent -r -t -B $QUILT_PC/$patch/ -
> +                     printf $"Dropping patch %s\n" "$(print_patch $patch)"
> +                     @LIB@/backup-files $silent -x -B $QUILT_PC/$patch/ -
>                       status=$?
> +                     remove_from_series $patch
> +             else
> +                     if [ -z "$(shopt -s nullglob ; echo 
> "$QUILT_PC/$patch/"*)" ]
> +                     then
> +                             printf $"Patch %s appears to be empty, 
> removing\n" \
> +                             "$(print_patch $patch)"
> +                             status=0
> +                     else
> +                             printf $"Removing patch %s\n" "$(print_patch 
> $patch)"
> +                             @LIB@/backup-files $silent -r -t -B 
> $QUILT_PC/$patch/ -
> +                             status=$?
> +                     fi

I'm curious what would happen if you tried -d on an empty patch. Would
it cause any problem? Even if it doesn't, it's probably still good to
let the user know, as this isn't supposed to happen.

> @@ -222,6 +232,10 @@ do
>       -a)
>               opt_all=1
>               shift ;;
> +     -d)
> +             opt_drop=1
> +             unset opt_remove
> +             shift ;;
>       -h)
>               usage -h ;;
>          --)

I don't like the fact that -d -R would do one thing and -R -d would do
something different. Also, silently ignoring an option is no good. I'd
prefer the script to exit with an error message if both -d and -R are
provided.

What would happen if a dropped patch included a file that is modified by
other patches below the dropped one? Looks to me like you would be left
in a state where you can't commit the changes to your source code
management system, nor can you pop the lower patch modifying this file
anymore, right? This doesn't sound good. As I understand it, dropping a
patch only makes sense if it affects files no patches below it affect.
Your patch should ensure that this is the case rather than blindly
trusting the user. I don't think we can accept your patch in mainline as
long as this point isn't addressed, as this would be too easy for a
random user to screw up his/her working tree.

-- 
Jean Delvare