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 _______________________________________________ Quilt-dev mailing list [email protected] http://lists.nongnu.org/mailman/listinfo/quilt-dev
