Re: [patch] _hg: completion for 'hg bookmarks'

[Christoph Mathys <eraserix@xxxxxxxxx>]

Hi Daniel,

Thank you for the quick response.

On Thu, Sep 24, 2015 at 6:05 PM, Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> wrote:

> 1. The function should be defined further up in the file (_hg_cmd_* are
> defined alphabetically).

Fixed.

> 2. --rev and --message need to become --rev= and --message= to allow
> both '--rev ARG' and '--rev=ARG' (which both work).

Fixed for --rev.

> 3. Why do you have a leading '-' on the first and last line?  Is it
> intentional?

They were intentional. I wanted to create groups of mutually exclusive
arguments. But on typing the reply I realized that it is very well
possible to call '--inactive <bookmark>', so the patch was wrong. I
have done away with the '-' in this version of the patch and it should
be more straightforward like this.

> 4. How should positional arguments (i.e., 'hg bookmarks <TAB>') be
> completed?  They are currently completed as files; presumably they
> should be completed as _hg_bookmarks_internal?

Should now always be _hg_bookmarks_internal.

> 5. Your mailer munged whitespace in the patch.  You should be able to
> avoid this by sending the patch as an attachment named *.txt.

Done.

thx,
Christophq

diff --git a/Completion/Unix/Command/_hg b/Completion/Unix/Command/_hg
index 9dd0236..c84faf3 100644
--- a/Completion/Unix/Command/_hg
+++ b/Completion/Unix/Command/_hg
@@ -460,6 +460,16 @@ _hg_cmd_bisect() {
   '(--command -c --noupdate -U)'{-U,--noupdate}'[do not update to target]'
 }
 
+_hg_cmd_bookmarks() {
+  _arguments -s -w : $_hg_global_opts \
+  '(--force -f)'{-f,--force}'[force]' \
+  '(--rev -r)'{-r+,--rev=}'[set bookmark at revision]:revision:_hg_tags' \
+  '(--delete -d)'{-d,--delete}'[delete a given bookmark]' \
+  '(--rename -m)'{-m+,--rename}'[rename given bookmark]:bookmark:_hg_bookmarks_internal' \
+  '(--inactive -i)'{-i,--inactive}'[mark a bookmark inactive]' \
+  ':bookmark:_hg_bookmarks_internal'
+}
+
 _hg_cmd_branch() {
   _arguments -s -w : $_hg_global_opts \
   '(--force -f)'{-f,--force}'[set branch name even if it shadows an existing branch]' \