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

[Josh Boyer]

On Sun, 2005-07-31 at 22:35 +0200, Jean Delvare wrote:
> 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]

Makes sense.

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

Sure.  I really need to get a spell checker in gvim one of these days.

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

Ok, good point.

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

Ok.

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

Ok, quite a good point.  I'll work on figuring this out.  Depending on
how complicated it gets, I might go back to adding a whole new command.
It might duplicate some code, but it will probably be easier then trying
to shove an option like this into a well known command.

Thanks for the review.  I'll be sure to post again once I've fixed up
the issues you pointed out.  Probably with a regression test this time
too.

josh