Re: [Quilt-dev] [PATCH] quilt drop command

[Josh Boyer]

On Sat, 2005-05-28 at 18:26 +0200, Jean Delvare wrote:
> Here are a few random comments on your "drop" patch.
> 
> 1* There are still a number of references to the original "commit" name,
> both in the comments before the actual patch, and in the patch itself.

Yes, seems I missed a few.

> 
> 2* The code of your "drop" command is so similar to the "pop" command
> that I really wonder if it wouldn't make more sense to add a --drop
> option to the "pop" command instead. The only real differences, as far
> as I can see, are the flags passed to backup-files, and the extra call
> to remove_from_series. Could you try implementing the "drop"
> functionality as an option to the pop command, so that we can see which
> approach is the best?

Yes, I can do that.  I originally had it implemented like that but
thought a new command was better.  The "pop" command's sole purpose is
to un-apply the patch.  It just seemed "drop" went against "pop".

> 
> 3* Indentation is broken in the command-line options handling part of
> your code (lines 195-217).

Probably tabs vs. spaces or something.  gvim is so wonderful at hiding
things like that.  I'll fix it.

> 
> 4* You did not properly discard the -R option, there are still two
> occurences of $opt_remove in your patch.

Good catch.

> 
> Note that I am not entirely convinced by the "drop" name. When I hear
> "drop" I somehow think of it as a synonymous for "discard", which is the
> exact opposite of what the command does. I expect it to confuse some
> users as well if we keep it. Not that I have any obvious replacement
> name (except for the random list I posted last week)... Maybe if it
> becomes an option to the "pop" command it will bring new ideas for a
> better name.
> 

As I said before, I don't particularly care about the name although I'm
inclined to agree with you.

The more I think about it, the more I like your "graft" name.  It fits
what the command does fairly well.  If we went with that, I'd like to
keep it a separate command though, not an option to "pop".

Thanks for the comments.  I'll work on this a bit more in the next
couple of days and get it fixed up.

thx,
josh