{"thread":{"id":"60697","subject":"[PATCH 0/1] completion: complete dir-type option args to am, format_patch","startedAt":"2024-01-07T21:42:10Z","lastAt":"2024-02-29T00:04:42Z","messageCount":6,"participants":["Britton Leo Kerin","Rubén Justo","Britton Kerin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"486380","messageId":"4714b88d-df5b-4e37-a5d7-af5033cfb861@smtp-relay.sendinblue.com","threadId":"60697","inReplyTo":null,"subject":"[PATCH 0/1] completion: complete dir-type option args to am, format_patch","fromName":"Britton Leo Kerin","fromEmail":"britton.kerin@gmail.com","sentAt":"2024-01-07T21:41:59Z","receivedAt":"2024-01-07T21:42:10Z","isPatch":true,"sender":{"key":"britton.kerin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7434164?v=4"},"body":"I think are a few other places this could be done as well but I wanted to\nstart with just a couple to show the idea.\n\nBritton Leo Kerin (1):\n  completion: dir-type optargs for am, format-patch\n\n contrib/completion/git-completion.bash | 38 ++++++++++++++++++++++++++\n 1 file changed, 38 insertions(+)\n\n\nbase-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa\n--\n2.43.0\n\n\n"},{"id":"486414","messageId":"d37781c3-6af2-409b-95a8-660a9b92d20b@smtp-relay.sendinblue.com","threadId":"60697","inReplyTo":"20240109005303.444932-1-britton.kerin@gmail.com","subject":"[PATCH v2 1/1] completion: dir-type optargs for am, format-patch","fromName":"Britton Leo Kerin","fromEmail":"britton.kerin@gmail.com","sentAt":"2024-01-09T00:53:03Z","receivedAt":"2024-01-09T00:53:14Z","isPatch":true,"sender":{"key":"britton.kerin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7434164?v=4"},"body":"Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>\n---\n contrib/completion/git-completion.bash | 37 ++++++++++++++++++++++++++\n 1 file changed, 37 insertions(+)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 185b47d802..2b2b6c9738 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1356,6 +1356,29 @@ __git_count_arguments ()\n \tprintf \"%d\" $c\n }\n \n+# Complete actual dir (not pathspec), respecting any -C options.\n+#\n+# Usage: __git_complete_refs [<option>]...\n+# --cur=<word>: The current dir to be completed.  Defaults to the current word.\n+__git_complete_dir ()\n+{\n+\tlocal cur_=\"$cur\"\n+\n+\twhile test $# != 0; do\n+\t\tcase \"$1\" in\n+\t\t--cur=*)\tcur_=\"${1##--cur=}\" ;;\n+\t\t*)\t\treturn 1 ;;\n+\t\tesac\n+\t\tshift\n+\tdone\n+\n+        # This rev-parse invocation amounts to a pwd which respects -C options\n+\tlocal context_dir=$(__git rev-parse --show-toplevel --show-prefix 2>/dev/null | paste -s -d '/' 2>/dev/null)\n+\t[ -d \"$context_dir\" ] || return 1\n+\n+\tCOMPREPLY=$(cd \"$context_dir\" 2>/dev/null && compgen -d -- \"$cur_\")\n+}\n+\n __git_whitespacelist=\"nowarn warn error error-all fix\"\n __git_patchformat=\"mbox stgit stgit-series hg mboxrd\"\n __git_showcurrentpatch=\"diff raw\"\n@@ -1374,6 +1397,10 @@ _git_am ()\n \t\t__gitcomp \"$__git_whitespacelist\" \"\" \"${cur##--whitespace=}\"\n \t\treturn\n \t\t;;\n+\t--directory=*)\n+\t\t__git_complete_dir --cur=\"${cur##--directory=}\"\n+\t\treturn\n+\t\t;;\n \t--patch-format=*)\n \t\t__gitcomp \"$__git_patchformat\" \"\" \"${cur##--patch-format=}\"\n \t\treturn\n@@ -1867,7 +1894,17 @@ __git_format_patch_extra_options=\"\n \n _git_format_patch ()\n {\n+\tcase \"$prev,$cur\" in\n+\t-o,*)\n+\t\t__git_complete_dir\n+\t\treturn\n+\t\t;;\n+\tesac\n \tcase \"$cur\" in\n+\t--output-directory=*)\n+\t\t__git_complete_dir --cur=\"${cur##--output-directory=}\"\n+\t\treturn\n+\t\t;;\n \t--thread=*)\n \t\t__gitcomp \"\n \t\t\tdeep shallow\n-- \n2.43.0\n\n\n"},{"id":"486415","messageId":"feae3401-b3cb-4bae-926b-4844b0b0986c@smtp-relay.sendinblue.com","threadId":"60697","inReplyTo":"4714b88d-df5b-4e37-a5d7-af5033cfb861@smtp-relay.sendinblue.com","subject":"[PATCH v2 0/1] completion: complete dir-type option args to am, format_patch","fromName":"Britton Leo Kerin","fromEmail":"britton.kerin@gmail.com","sentAt":"2024-01-09T00:53:02Z","receivedAt":"2024-01-09T00:53:15Z","isPatch":true,"sender":{"key":"britton.kerin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7434164?v=4"},"body":"This revision adds missing double quotes to improve the completion\nsituation for paths with spaces in them, and adds some comments.\n\nBritton Leo Kerin (1):\n  completion: dir-type optargs for am, format-patch\n\n contrib/completion/git-completion.bash | 37 ++++++++++++++++++++++++++\n 1 file changed, 37 insertions(+)\n\nRange-diff against v1:\n1:  2d788b0b18 ! 1:  5161304a92 completion: dir-type optargs for am, format-patch\n    @@ contrib/completion/git-completion.bash: __git_count_arguments ()\n      \tprintf \"%d\" $c\n      }\n\n    -+\n     +# Complete actual dir (not pathspec), respecting any -C options.\n     +#\n     +# Usage: __git_complete_refs [<option>]...\n    @@ contrib/completion/git-completion.bash: __git_count_arguments ()\n     +\t\tshift\n     +\tdone\n     +\n    ++        # This rev-parse invocation amounts to a pwd which respects -C options\n     +\tlocal context_dir=$(__git rev-parse --show-toplevel --show-prefix 2>/dev/null | paste -s -d '/' 2>/dev/null)\n    -+\t[ -d \"$context_dir\" ] || return\n    ++\t[ -d \"$context_dir\" ] || return 1\n     +\n    -+\tCOMPREPLY=$(cd $context_dir 2>/dev/null && compgen -d -- \"$cur_\")\n    ++\tCOMPREPLY=$(cd \"$context_dir\" 2>/dev/null && compgen -d -- \"$cur_\")\n     +}\n    -+\n     +\n      __git_whitespacelist=\"nowarn warn error error-all fix\"\n      __git_patchformat=\"mbox stgit stgit-series hg mboxrd\"\n--\n2.43.0\n\n\n"},{"id":"487874","messageId":"40c3a824-a961-490b-94d4-4eb23c8f713d@gmail.com","threadId":"60697","inReplyTo":"d37781c3-6af2-409b-95a8-660a9b92d20b@smtp-relay.sendinblue.com","subject":"Re: [PATCH v2 1/1] completion: dir-type optargs for am, format-patch","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-02-03T15:13:36Z","receivedAt":"2024-02-03T15:13:45Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 08-ene-2024 15:53:03, Britton Leo Kerin wrote:\n> Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>\n> ---\n>  contrib/completion/git-completion.bash | 37 ++++++++++++++++++++++++++\n>  1 file changed, 37 insertions(+)\n> \n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 185b47d802..2b2b6c9738 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -1356,6 +1356,29 @@ __git_count_arguments ()\n>  \tprintf \"%d\" $c\n>  }\n>  \n> +# Complete actual dir (not pathspec), respecting any -C options.\n> +#\n> +# Usage: __git_complete_refs [<option>]...\n> +# --cur=<word>: The current dir to be completed.  Defaults to the current word.\n> +__git_complete_dir ()\n> +{\n> +\tlocal cur_=\"$cur\"\n> +\n> +\twhile test $# != 0; do\n> +\t\tcase \"$1\" in\n> +\t\t--cur=*)\tcur_=\"${1##--cur=}\" ;;\n> +\t\t*)\t\treturn 1 ;;\n> +\t\tesac\n> +\t\tshift\n> +\tdone\n> +\n> +        # This rev-parse invocation amounts to a pwd which respects -C options\n> +\tlocal context_dir=$(__git rev-parse --show-toplevel --show-prefix 2>/dev/null | paste -s -d '/' 2>/dev/null)\n> +\t[ -d \"$context_dir\" ] || return 1\n> +\n> +\tCOMPREPLY=$(cd \"$context_dir\" 2>/dev/null && compgen -d -- \"$cur_\")\n\nThis assignment is problematic.\n\nFirst, COMPREPLY is expected to be an array.  Maybe a simple change can\ndo the trick:\n\n-\tCOMPREPLY=$(cd \"$context_dir\" 2>/dev/null && compgen -d -- \"$cur_\")\n+\tCOMPREPLY=( $(cd \"$context_dir\" 2>/dev/null && compgen -d -- \"$cur_\") )\n\nBut, what happens with directories that have SP's in its name?  We're\ngiving wrong options:\n\n    $ mkdir one\n    $ mkdir \"one more dir\"\n    $ git am --directory=o<TAB><TAB>\n    dir   more  one\n\nSetting IFS can help us:\n\n+       local IFS=$'\\n'\n\nNow we're returning correct options:\n\n    $ mkdir one\n    $ mkdir \"one more dir\"\n    $ git am --directory=o<TAB><TAB>\n    one       one more dir\n\nHere, the user might be expecting directory names with a trailing '/',\nas Bash do.  Again, a simple trick:\n\n-\tCOMPREPLY=( $(cd \"$context_dir\" 2>/dev/null && compgen -d -- \"$cur_\") )\n+\tCOMPREPLY=( $(cd \"$context_dir\" 2>/dev/null && compgen -d -S / -- \"$cur_\") )\n\nNow looks better, IMO:\n\n    $ git am --directory=o<TAB><TAB>\n    one/      one more dir/\n\nBut, after all of this, we're going to provoke a problematic completion due\nto the SP:\n\n    $ mkdir \"another one\"\n    $ git am --directory=anot<TAB><TAB>\n    ...\n    $ git am --directory=another one/\n\nWe should complete to:\n\n    $ git am --directory=another\\ one/\n\nHere we need a less simple trick:\n\n+       # use a hack to enable file mode in bash < 4\n+       # compopt -o filenames +o nospace 2>/dev/null ||\n+       compgen -f /non-existing-dir/ >/dev/null ||\n+       true\n\nSome commits you may find interesting:\nfea16b47b6 (git-completion.bash: add support for path completion, 2013-01-11)\n3ffa4df4b2 (completion: add hack to enable file mode in bash < 4, 2013-04-27)\n\nWell, so far, so good?  I'm afraid, not:  What happens with other\nspecial characters like quotes '\"'?\n\nI suggest you take a look at how we are already doing all of\nconsiderations for other commands, like git-add.\n\n> +}\n> +\n>  __git_whitespacelist=\"nowarn warn error error-all fix\"\n>  __git_patchformat=\"mbox stgit stgit-series hg mboxrd\"\n>  __git_showcurrentpatch=\"diff raw\"\n> @@ -1374,6 +1397,10 @@ _git_am ()\n>  \t\t__gitcomp \"$__git_whitespacelist\" \"\" \"${cur##--whitespace=}\"\n>  \t\treturn\n>  \t\t;;\n> +\t--directory=*)\n> +\t\t__git_complete_dir --cur=\"${cur##--directory=}\"\n> +\t\treturn\n> +\t\t;;\n>  \t--patch-format=*)\n>  \t\t__gitcomp \"$__git_patchformat\" \"\" \"${cur##--patch-format=}\"\n>  \t\treturn\n> @@ -1867,7 +1894,17 @@ __git_format_patch_extra_options=\"\n>  \n>  _git_format_patch ()\n>  {\n> +\tcase \"$prev,$cur\" in\n> +\t-o,*)\n> +\t\t__git_complete_dir\n> +\t\treturn\n> +\t\t;;\n> +\tesac\n>  \tcase \"$cur\" in\n> +\t--output-directory=*)\n> +\t\t__git_complete_dir --cur=\"${cur##--output-directory=}\"\n> +\t\treturn\n> +\t\t;;\n\nAdding completion for these options, and possibly opening the way for\nothers, is a nice improvement.\n\nThank you for working on this.\n"},{"id":"488498","messageId":"CAC4O8c9z4s4fFU6_h6ZRBnDhZyiTp3XR8j0DrARj+1SauLbQEQ@mail.gmail.com","threadId":"60697","inReplyTo":"40c3a824-a961-490b-94d4-4eb23c8f713d@gmail.com","subject":"Re: [PATCH v2 1/1] completion: dir-type optargs for am, format-patch","fromName":"Britton Kerin","fromEmail":"britton.kerin@gmail.com","sentAt":"2024-02-13T00:52:53Z","receivedAt":"2024-02-13T00:53:06Z","isPatch":true,"sender":{"key":"britton.kerin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7434164?v=4"},"body":"On Sat, Feb 3, 2024 at 6:13 AM Rubén Justo <rjusto@gmail.com> wrote:\n>\n> On 08-ene-2024 15:53:03, Britton Leo Kerin wrote:\n> > Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>\n> > ---\n> >  contrib/completion/git-completion.bash | 37 ++++++++++++++++++++++++++\n> >  1 file changed, 37 insertions(+)\n> >\n> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> > index 185b47d802..2b2b6c9738 100644\n> > --- a/contrib/completion/git-completion.bash\n> > +++ b/contrib/completion/git-completion.bash\n> > @@ -1356,6 +1356,29 @@ __git_count_arguments ()\n> >       printf \"%d\" $c\n> >  }\n> >\n> > +# Complete actual dir (not pathspec), respecting any -C options.\n> > +#\n> > +# Usage: __git_complete_refs [<option>]...\n> > +# --cur=<word>: The current dir to be completed.  Defaults to the current word.\n> > +__git_complete_dir ()\n> > +{\n> > +     local cur_=\"$cur\"\n> > +\n> > +     while test $# != 0; do\n> > +             case \"$1\" in\n> > +             --cur=*)        cur_=\"${1##--cur=}\" ;;\n> > +             *)              return 1 ;;\n> > +             esac\n> > +             shift\n> > +     done\n> > +\n> > +        # This rev-parse invocation amounts to a pwd which respects -C options\n> > +     local context_dir=$(__git rev-parse --show-toplevel --show-prefix 2>/dev/null | paste -s -d '/' 2>/dev/null)\n> > +     [ -d \"$context_dir\" ] || return 1\n> > +\n> > +     COMPREPLY=$(cd \"$context_dir\" 2>/dev/null && compgen -d -- \"$cur_\")\n>\n> This assignment is problematic.\n>\n> First, COMPREPLY is expected to be an array.  Maybe a simple change can\n> do the trick:\n>\n> -       COMPREPLY=$(cd \"$context_dir\" 2>/dev/null && compgen -d -- \"$cur_\")\n> +       COMPREPLY=( $(cd \"$context_dir\" 2>/dev/null && compgen -d -- \"$cur_\") )\n>\n> But, what happens with directories that have SP's in its name?  We're\n> giving wrong options:\n>\n>     $ mkdir one\n>     $ mkdir \"one more dir\"\n>     $ git am --directory=o<TAB><TAB>\n>     dir   more  one\n>\n> Setting IFS can help us:\n>\n> +       local IFS=$'\\n'\n>\n> Now we're returning correct options:\n>\n>     $ mkdir one\n>     $ mkdir \"one more dir\"\n>     $ git am --directory=o<TAB><TAB>\n>     one       one more dir\n>\n> Here, the user might be expecting directory names with a trailing '/',\n> as Bash do.  Again, a simple trick:\n>\n> -       COMPREPLY=( $(cd \"$context_dir\" 2>/dev/null && compgen -d -- \"$cur_\") )\n> +       COMPREPLY=( $(cd \"$context_dir\" 2>/dev/null && compgen -d -S / -- \"$cur_\") )\n>\n> Now looks better, IMO:\n>\n>     $ git am --directory=o<TAB><TAB>\n>     one/      one more dir/\n>\n> But, after all of this, we're going to provoke a problematic completion due\n> to the SP:\n>\n>     $ mkdir \"another one\"\n>     $ git am --directory=anot<TAB><TAB>\n>     ...\n>     $ git am --directory=another one/\n>\n> We should complete to:\n>\n>     $ git am --directory=another\\ one/\n>\n> Here we need a less simple trick:\n>\n> +       # use a hack to enable file mode in bash < 4\n> +       # compopt -o filenames +o nospace 2>/dev/null ||\n> +       compgen -f /non-existing-dir/ >/dev/null ||\n> +       true\n>\n> Some commits you may find interesting:\n> fea16b47b6 (git-completion.bash: add support for path completion, 2013-01-11)\n> 3ffa4df4b2 (completion: add hack to enable file mode in bash < 4, 2013-04-27)\n>\n> Well, so far, so good?  I'm afraid, not:  What happens with other\n> special characters like quotes '\"'?\n>\n> I suggest you take a look at how we are already doing all of\n> considerations for other commands, like git-add.\n\nThanks for all these suggestions.  Considering them and working on it\nsome more I end up with this function:\n\n\n__git_complete_dir ()\n{\n        local cur_=\"$cur\"\n\n        while test $# != 0; do\n                case \"$1\" in\n                --cur=*)        cur_=\"${1##--cur=}\" ;;\n                *)              return 1 ;;\n                esac\n                shift\n        done\n\n        # This rev-parse invocation amounts to a pwd which respects -C options\n        local context_dir=$(__git rev-parse --show-toplevel\n--show-prefix 2>/dev/null | paste -s -d '/' 2>/dev/null)\n        [ -d \"$context_dir\" ] || return 1\n\n        compopt -o noquote\n\n        local IFS=$'\\n'\n        local unescaped_candidates=($(cd \"$context_dir\" 2>/dev/null &&\ncompgen -d -S / -- \"$cur_\"))\n        for ii in \"${!unescaped_candidates[@]}\"; do\n                COMPREPLY[$ii]=$(printf \"%q\" \"${unescaped_candidates[$ii]}\")\n        done\n}\n\nThis one works for all weird characters that I've tried in bash 5.2 at\nleast, and in frameworks that do their own escaping also (e.g.\nble.sh).  Since your advice so far was so good I thought I'd ask if\nthere is anything obvious to you that is still wrong here?\n\nIf not I guess what's left is special code to make it work better with\nold versions of bash.  I'm a little sceptical that this is worth it\nsince bash 5 is already 5 years old and it's only completion code\nwe're talking about  but I guess it could be done.\n\nBritton\n"},{"id":"489644","messageId":"6683f24e-7e56-489d-be2d-8afe1fc38d2b@gmail.com","threadId":"60697","inReplyTo":"CAC4O8c9z4s4fFU6_h6ZRBnDhZyiTp3XR8j0DrARj+1SauLbQEQ@mail.gmail.com","subject":"Re: [PATCH v2 1/1] completion: dir-type optargs for am, format-patch","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-02-29T00:04:36Z","receivedAt":"2024-02-29T00:04:42Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Feb 12, 2024 at 13:52:53 -0900, Britton Kerin wrote:\n\n> __git_complete_dir ()\n> {\n>         local cur_=\"$cur\"\n> \n>         while test $# != 0; do\n>                 case \"$1\" in\n>                 --cur=*)        cur_=\"${1##--cur=}\" ;;\n>                 *)              return 1 ;;\n>                 esac\n>                 shift\n>         done\n> \n>         # This rev-parse invocation amounts to a pwd which respects -C options\n>         local context_dir=$(__git rev-parse --show-toplevel\n> --show-prefix 2>/dev/null | paste -s -d '/' 2>/dev/null)\n>         [ -d \"$context_dir\" ] || return 1\n> \n>         compopt -o noquote\n> \n>         local IFS=$'\\n'\n>         local unescaped_candidates=($(cd \"$context_dir\" 2>/dev/null &&\n> compgen -d -S / -- \"$cur_\"))\n>         for ii in \"${!unescaped_candidates[@]}\"; do\n>                 COMPREPLY[$ii]=$(printf \"%q\" \"${unescaped_candidates[$ii]}\")\n>         done\n> }\n> \n> This one works for all weird characters that I've tried in bash 5.2 at\n> least, and in frameworks that do their own escaping also (e.g.\n> ble.sh).  Since your advice so far was so good I thought I'd ask if\n> there is anything obvious to you that is still wrong here?\n\n> If not I guess what's left is special code to make it work better with\n> old versions of bash.  I'm a little sceptical that this is worth it\n> since bash 5 is already 5 years old and it's only completion code\n> we're talking about  but I guess it could be done.\n\nI don't think you need to dig too much into old Bash versions.  If it\nworks with a recent one, it's a good start.\n\nHave you considered adding some tests to t/t9902-completion.sh?\n\nIt is desirable to see some tests at least for __git_complete_dir.\nPerhaps it would also help you to polish the function.\n\nSorry for the late response.  I just found your message while reviewing\nthe topics in the 'What's cooking'.\n"}]}