Re: [PATCH] notes: add `rm` and `delete` commands
On Friday 10 November 2017 at 12:14 pm +0900, Junio C Hamano wrote: > Eric Sunshinewrites: > > > or against the change: > > > > - synonym bloat; balloons documentation > > - steals command verbs from potential future features > > - ... > > I tend to agree with these two (and if I were to replace the "..." > with something concrete, this change is not desirable because it > will force us to add all these three when we introduce "remove" > elsewhere in the system). > > Having said that, this remids me of an age-old topic we discussed in > a distant past but stalled due to lack of strong drive, which is: > > Some git subcommands take command verb (e.g. "git notes 'remove'") > while others take command mode flags (e.g. "git branch --delete"), > and the users need to remember them, which is not ideal. > > I think the consensus back then, if we were to aim for consistency > by uniformly using only one of the two, is to use the command mode > flags, simply because existing commands that have the primary mode > and take an optional command mode flag [*1*] cannot be migrated to > the command verb UI safely and sanely. > > And then, if we are not careful when we standardize all subcommands > to take command mode flags, we might end up with a situation where > some subcommand say "--remove" while other say "--delete", and some > users will wish to rectify that situation by adding an alias defined > for these flags---I view this patch as a part of that possible > future topic ;-). My motivation was entirely "these were the commands I tried before I tried `remove` when I wanted to delete a note." I think both have precedence in Git with the likes of `git rm` and `git branch --delete`, although you're entirely correct that I should have at least added that justification in the commit message. The matter of which verbs and which flags work where is clearly a much broader concern. However at the moment Git uses `rm`, `remove` and `delete` with no apparent (at least to me) logic about which is used where, and it seems to me that adding each as synonyms of the other across the entire suite would improve usability. In particular, I don't think I buy the argument that it steals verbs: I can't imagine wanting to use multiple of those verbs in the same context with different meanings, as that seems liable to cause significant confusion. That said, I do get the argument about UI and documentation bloat. From my perspective, the cost of that is less than the benefit of having a more consistent interface, and this is something we can do to have a more consistent interface *now* without waiting for a larger future consistency project, but YMMV, and I'm very willing to bow to everyone else's expertise on this.
Re: [PATCH] notes: add `rm` and `delete` commands
Eric Sunshinewrites: > or against the change: > > - synonym bloat; balloons documentation > - steals command verbs from potential future features > - ... I tend to agree with these two (and if I were to replace the "..." with something concrete, this change is not desirable because it will force us to add all these three when we introduce "remove" elsewhere in the system). Having said that, this remids me of an age-old topic we discussed in a distant past but stalled due to lack of strong drive, which is: Some git subcommands take command verb (e.g. "git notes 'remove'") while others take command mode flags (e.g. "git branch --delete"), and the users need to remember them, which is not ideal. I think the consensus back then, if we were to aim for consistency by uniformly using only one of the two, is to use the command mode flags, simply because existing commands that have the primary mode and take an optional command mode flag [*1*] cannot be migrated to the command verb UI safely and sanely. And then, if we are not careful when we standardize all subcommands to take command mode flags, we might end up with a situation where some subcommand say "--remove" while other say "--delete", and some users will wish to rectify that situation by adding an alias defined for these flags---I view this patch as a part of that possible future topic ;-). [Footnote] *1* For example, "git branch" by default is in the branch creation mode, but it can be told to work in its branch deletion mode with "--delete". If we were to standardize on command verbs, is "git branch delete" a deprecated relic from the old world that wants to create a branch whose name is 'delete', or is it done by a script in the new world that collected names of branches to delete and asked "git branch delete $to_delete" to delete all of them, where the $to_delete variable happened to end up empty? With command mode flags, we do not have to worry about such an ambiguity during and after transition.
Re: [PATCH] notes: add `rm` and `delete` commands
On Thu, Nov 9, 2017 at 8:46 AM, Adam Dinwoodiewrote: > Add `git notes rm` and `git notes delete` as alternative ways of saying > `git notes remove`. The justification for this change seems to be missing from the commit message. One can formulate arguments for the change: - "rm" & "delete" more intuitive for Unix and DOS users - for consistency with git- command(s) - ... or against the change: - synonym bloat; balloons documentation - steals command verbs from potential future features - ... > Signed-off-by: Adam Dinwoodie
[PATCH] notes: add `rm` and `delete` commands
Add `git notes rm` and `git notes delete` as alternative ways of saying `git notes remove`. Signed-off-by: Adam Dinwoodie--- Documentation/git-notes.txt | 4 +++- builtin/notes.c | 8 +--- t/t3301-notes.sh| 6 +++--- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt index 43677297f..b1059dc85 100644 --- a/Documentation/git-notes.txt +++ b/Documentation/git-notes.txt @@ -17,7 +17,7 @@ SYNOPSIS 'git notes' merge [-v | -q] [-s ] 'git notes' merge --commit [-v | -q] 'git notes' merge --abort [-v | -q] -'git notes' remove [--ignore-missing] [--stdin] [...] +'git notes' (remove|rm|delete) [--ignore-missing] [--stdin] [...] 'git notes' prune [-n | -v] 'git notes' get-ref @@ -110,6 +110,8 @@ When done, the user can either finalize the merge with 'git notes merge --abort'. remove:: +rm:: +delete:: Remove the notes for given objects (defaults to HEAD). When giving zero or one object from the command line, this is equivalent to specifying an empty note message to diff --git a/builtin/notes.c b/builtin/notes.c index 12afdf190..cedb37535 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -32,7 +32,7 @@ static const char * const git_notes_usage[] = { N_("git notes [--ref ] merge [-v | -q] [-s ] "), N_("git notes merge --commit [-v | -q]"), N_("git notes merge --abort [-v | -q]"), - N_("git notes [--ref ] remove [...]"), + N_("git notes [--ref ] (remove|rm|delete) [...]"), N_("git notes [--ref ] prune [-n | -v]"), N_("git notes [--ref ] get-ref"), NULL @@ -77,7 +77,7 @@ static const char * const git_notes_merge_usage[] = { }; static const char * const git_notes_remove_usage[] = { - N_("git notes remove []"), + N_("git notes (remove|rm|delete) []"), NULL }; @@ -1012,7 +1012,9 @@ int cmd_notes(int argc, const char **argv, const char *prefix) result = show(argc, argv, prefix); else if (!strcmp(argv[0], "merge")) result = merge(argc, argv, prefix); - else if (!strcmp(argv[0], "remove")) + else if (!strcmp(argv[0], "remove") || +!strcmp(argv[0], "rm") || +!strcmp(argv[0], "delete")) result = remove_cmd(argc, argv, prefix); else if (!strcmp(argv[0], "prune")) result = prune(argc, argv, prefix); diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 2d200fdf3..50df9a3f9 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -353,9 +353,9 @@ test_expect_success 'create note with combination of -m and -F' ' test_cmp expect-combine_m_and_F actual ' -test_expect_success 'remove note with "git notes remove"' ' +test_expect_success 'remove note with "git notes remove/rm"' ' git notes remove HEAD^ && - git notes remove && + git notes rm && cat >expect-rm-remove <<-EOF && commit 7f9ad8836c775acb134c0a055fc55fb4cd1ba361 Author: A U Thor @@ -390,7 +390,7 @@ test_expect_success 'removing more than one' ' # We have only two -- add another and make sure it stays git notes add -m "extra" && git notes list HEAD >after-removal-expect && - git notes remove HEAD^^ HEAD^^^ && + git notes delete HEAD^^ HEAD^^^ && git notes list | sed -e "s/ .*//" >actual && test_cmp after-removal-expect actual ' -- 2.14.3