{"thread":{"id":"54294","subject":"[PATCH] completion: complete refs after 'git restore -s'","startedAt":"2020-09-25T14:26:04Z","lastAt":"2020-09-25T20:20:40Z","messageCount":3,"participants":["Ákos Uzonyi","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"406333","messageId":"20200925142552.1656596-1-uzonyi.akos@gmail.com","threadId":"54294","inReplyTo":null,"subject":"[PATCH] completion: complete refs after 'git restore -s'","fromName":"Ákos Uzonyi","fromEmail":"uzonyi.akos@gmail.com","sentAt":"2020-09-25T14:25:52Z","receivedAt":"2020-09-25T14:26:04Z","isPatch":true,"sender":{"key":"uzonyi.akos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15818682?v=4"},"body":"From: Uzonyi Ákos \n\nCurrently only the long version (--source=) supports completion.\n\nAdd completion support to the short (-s) option too.\n\nSigned-off-by: Ákos Uzonyi \n---\n contrib/completion/git-completion.bash | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 8be4a0316e..50e6e82157 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2853,6 +2853,18 @@ _git_restore ()\n \t--*)\n \t\t__gitcomp_builtin restore\n \t\t;;\n+\t*)\n+\t\tlocal prevword prevword=\"${words[cword-1]}\"\n+\n+\t\tcase \"$prevword\" in\n+\t\t\t-s)\n+\t\t\t\t__git_complete_refs\n+\t\t\t\treturn\n+\t\t\t\t;;\n+\t\t\t*)\n+\t\t\t\t;;\n+\t\tesac\n+\t\t;;\n \tesac\n }\n \n-- \n2.28.0\n\n"},{"id":"406365","messageId":"xmqqlfgxlq6o.fsf@gitster.c.googlers.com","threadId":"54294","inReplyTo":"20200925142552.1656596-1-uzonyi.akos@gmail.com","subject":"Re: [PATCH] completion: complete refs after 'git restore -s'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-25T17:30:23Z","receivedAt":"2020-09-25T17:30:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ákos Uzonyi writes:\n\n> From: Uzonyi Ákos \n>\n> Currently only the long version (--source=) supports completion.\n>\n> Add completion support to the short (-s) option too.\n\nI am not too familiar with the completion library, but what makes\nthe \"-s\" option of restore so special? I've scanned the entire file\nand did not find that many special cases for short options that have\ntheir longer counterpart supported already.\n\nI do not know if the \"feature\" this wants to bring in is a good\nidea---we may want to try to be more systematic (e.g. perhaps it\ninvolves teaching the parse-options subsystem about equivalence of\nshort and long options, so that we can reuse existing support for\nthe the long option \"--source=\" to complete \"-s \"), if we\nwere to do something like this. Singling out \"-s\" of \"restore\"\nsmells not quite right, as the approach would not scale well.\n\n> Signed-off-by: Ákos Uzonyi \n> ---\n> contrib/completion/git-completion.bash | 12 ++++++++++++\n> 1 file changed, 12 insertions(+)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 8be4a0316e..50e6e82157 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -2853,6 +2853,18 @@ _git_restore ()\n> \t--*)\n> \t\t__gitcomp_builtin restore\n> \t\t;;\n> +\t*)\n> +\t\tlocal prevword prevword=\"${words[cword-1]}\"\n\nWhy duplicated prevword here? Did you mean \n\n\tlocal prevword=${words[cword-1]}\n\ninstead?\n\n> +\n> +\t\tcase \"$prevword\" in\n> +\t\t\t-s)\n> +\t\t\t\t__git_complete_refs\n> +\t\t\t\treturn\n> +\t\t\t\t;;\n> +\t\t\t*)\n> +\t\t\t\t;;\n> +\t\tesac\n> +\t\t;;\n\nWrong indentation. In this file, as can be seen on the line \"*)\"\nyou added at the top of this hunk, the case arms like \"-s)\" and \"*)\"\nmust align with \"case\" and \"esac\" in this file.\n\n> \tesac\n> }\n\n\nThanks.\n"},{"id":"406384","messageId":"CAJrAtOWH8Upwov5dL4OfHPAb9v3NynmooC8mr3hAVA+Yo9Yi5w@mail.gmail.com","threadId":"54294","inReplyTo":"xmqqlfgxlq6o.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] completion: complete refs after 'git restore -s'","fromName":"Ákos Uzonyi","fromEmail":"uzonyi.akos@gmail.com","sentAt":"2020-09-25T19:59:49Z","receivedAt":"2020-09-25T20:20:40Z","isPatch":true,"sender":{"key":"uzonyi.akos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/15818682?v=4"},"body":"Hi,\n\nThanks for your detailed review.\n\nOn Fri, 25 Sep 2020 at 19:30, Junio C Hamano wrote:\n> Ákos Uzonyi writes:\n>\n> > From: Uzonyi Ákos \n> >\n> > Currently only the long version (--source=) supports completion.\n> >\n> > Add completion support to the short (-s) option too.\n>\n> I am not too familiar with the completion library, but what makes\n> the \"-s\" option of restore so special? I've scanned the entire file\n> and did not find that many special cases for short options that have\n> their longer counterpart supported already.\n\nThere are multiple commands already having this kind of short-long\noption completion. The \"-c\" options of commit, switch and checkout\neach have longer counterparts, and both the short and long versions\nhave completion support for their arguments.\n\n> I do not know if the \"feature\" this wants to bring in is a good\n> idea---we may want to try to be more systematic (e.g. perhaps it\n> involves teaching the parse-options subsystem about equivalence of\n> short and long options, so that we can reuse existing support for\n> the the long option \"--source=\" to complete \"-s \"), if we\n> were to do something like this. Singling out \"-s\" of \"restore\"\n> smells not quite right, as the approach would not scale well.\n\nI think these cases are not too frequent, so it doesn't seem to be a\nbig scaling problem.\n\n> > Signed-off-by: Ákos Uzonyi \n> > ---\n> > contrib/completion/git-completion.bash | 12 ++++++++++++\n> > 1 file changed, 12 insertions(+)\n> >\n> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> > index 8be4a0316e..50e6e82157 100644\n> > --- a/contrib/completion/git-completion.bash\n> > +++ b/contrib/completion/git-completion.bash\n> > @@ -2853,6 +2853,18 @@ _git_restore ()\n> > --*)\n> > __gitcomp_builtin restore\n> > ;;\n> > + *)\n> > + local prevword prevword=\"${words[cword-1]}\"\n>\n> Why duplicated prevword here? Did you mean\n>\n> local prevword=${words[cword-1]}\n>\n> instead?\n\nThanks, I'll fix it.\n\n> > +\n> > + case \"$prevword\" in\n> > + -s)\n> > + __git_complete_refs\n> > + return\n> > + ;;\n> > + *)\n> > + ;;\n> > + esac\n> > + ;;\n>\n> Wrong indentation. In this file, as can be seen on the line \"*)\"\n> you added at the top of this hunk, the case arms like \"-s)\" and \"*)\"\n> must align with \"case\" and \"esac\" in this file.\n\nThanks, I'll fix it.\n\nBy the way, I copied this piece of code from _git_switch (it's also\nthere in _git_checkout), so these problems have to be fixed there as\nwell.\n\nAlso, reading _git_commit it looks that we already have a \"$prev\"\nvariable, so I'll use that instead of \"$prevword\".\n"}]}