Re: [Quilt-dev] Bugfixes: "delete" call to "pop" has wrong target; QUILT_DELETE_ARGS inherited by "pop"

[Jean Delvare]

Hi Joe,

> I've found a couple of bugs in the delete command.
> 
> When deleting the top patch by calling "quilt delete" with no
> arguments,  pop is called as "@QUILT@/pop -fq $patch".  This doesn't
> pop that patch,  it pops *to* that patch, so the operation fails.  The
> "$patch" argument  should be removed to pop the top patch.
> 
> Also, I have QUILT_DELETE_ARGS set to "-r --backup" in my .quiltrc,
> and  these are passed on to "pop", which results in an error:
> 
>     getopt: invalid option -- r
>     getopt: unrecognized option `--backup'
>     Usage: quilt pop [-afRqv] [num|patch]
> 
> The first attached patch "quilt-delete_pop.patch" fixes these
> problems.

Your analysis and fix look all correct to me. I have always wondered why
"quilt delete" was failing on me, now you've made it clear.

I've applied this first patch to quilt CVS already.

> The second patch "quilt-quilt_command.patch" creates a patchfn 
> "quilt_command()" which can be used to invoke subcommands.  I think
> it would be good to apply this patch as well for consistency in how
> subcommands are called.

I fully agree here too.

> One question on this patch is whether or not the QUILT_COMMAND
> variable should be set so that QUILT_*_ARGS are applied to the
> subcommands, or so that only the arguments passed in by the calling
> command are applied.  This version does the latter.

Sounds much safer to me the way you did.  The user could have default
options for some commands that make them behave in a non-standard way.
Imagine for example what would happen on "quilt delete" if a user had
QUILT_POP_ARGS set to "-a".  Sure this would be a silly setting to have,
but I think this illustrates why we better do NOT apply QUILT_*_ARGS to
subcommands by default.

If we ever have a case where picking QUILT_*_ARGS would be useful, we
will have to manually select which options we want.  For now, none of
the users of your new quilt_command() function seem to need this though.

I am willing to apply this second patch as well, unless Andreas (or
anyone) objects.  However, this patch is adding the patchfns inclusion
mechanism to edit.in while that change is already in CVS.  Could you
please resubmit a patch that would apply cleanly on current CVS?

Thanks,
-- 
Jean Delvare