Re: [PATCH v3 42/42] git-completion.bash: add GIT_COMPLETION_OPTIONS=all config

[Duy Nguyen]

On Sun, Feb 11, 2018 at 8:59 AM, Eric Sunshine  wrote:
> On Fri, Feb 9, 2018 at 6:02 AM, Nguyễn Thái Ngọc Duy  
> wrote:
>> By default, some option names (mostly --force, scripting related or for
>> internal use) are not completable for various reasons. When
>> GIT_COMPLETION_OPTIONS is set to all, all options (except hidden ones)
>> are completable.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>> diff --git a/contrib/completion/git-completion.bash 
>> b/contrib/completion/git-completion.bash
>> @@ -36,6 +36,10 @@
>> +#
>> +#   GIT_COMPLETION_OPTIONS
>> +#
>> +# When set to "all", complete all possible options
>> @@ -303,7 +307,7 @@ __gitcomp_builtin ()
>> if [ -z "$options" ]; then
>> # leading and trailing spaces are significant to make
>> # option removal work correctly.
>> -   options=" $(__git ${cmd/_/ } --git-completion-helper) $incl "
>> +   options=" $(__git ${cmd/_/ } 
>> --git-completion-helper=$GIT_COMPLETION_OPTIONS) $incl "
>
> This approach is rather different from what I had envisioned. Rather
> than asking --git-completion-helper to do the filtering, my thought
> was that it should unconditionally dump _all_ options but annotate
> them, and then git-completion.bash can filter however it sees fit. For
> instance, --git-completion-helper might annotate "dangerous" options
> with a "!" ("!--force" or "--force!" or "--force:!" or whatever).
>
> The benefit of this approach is that it more easily supports future
> enhancements. For instance, options which only make sense in certain
> contexts could be annotated to indicate such. An example are the
> --continue, --abort, --skip options for git-rebase which only make
> sense when a rebase session is active. One could imagine these options
> being annotated something like this:
>
> --abort:rebasing
> --continue:rebasing
> --skip:rebasing
>
> where git-completion.bash understands the "rebasing" annotation as
> meaning that these options only make sense when "rebase-apply" is
> present. (In fact, the annotation could be more expressive, such as
> "--abort:exists(rebase-apply)", but that might be overkill.)

I agree. I went a bit off track with this ...=all. But yes some form
of annotation will be needed long term to describe these options in
details. We haven't gotten the annotation in struct option[] to this
level yet, it's a bit hard to see what will be needed here. Let's drop
42/42 for now. I will study git-completion.bash more to see what it
needs and revisit this at some point in future.
-- 
Duy

Re: [PATCH v3 42/42] git-completion.bash: add GIT_COMPLETION_OPTIONS=all config

[Ævar Arnfjörð Bjarmason]

On Sat, Feb 10 2018, Duy Nguyen jotted:

> On Fri, Feb 09, 2018 at 03:19:57PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Fri, Feb 09 2018, Nguyễn Thái Ngọc Duy jotted:
>>
>> > By default, some option names (mostly --force, scripting related or for
>> > internal use) are not completable for various reasons. When
>> > GIT_COMPLETION_OPTIONS is set to all, all options (except hidden ones)
>> > are completable.
>> >
>> > Signed-off-by: Nguyễn Thái Ngọc Duy 
>> > ---
>> >  contrib/completion/git-completion.bash |  6 +-
>> >  parse-options.c| 11 +++
>> >  2 files changed, 12 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/contrib/completion/git-completion.bash 
>> > b/contrib/completion/git-completion.bash
>> > index 0ddf40063b..0cfa489a8e 100644
>> > --- a/contrib/completion/git-completion.bash
>> > +++ b/contrib/completion/git-completion.bash
>> > @@ -36,6 +36,10 @@
>> >  #
>> >  # When set to "1", do not include "DWIM" suggestions in git-checkout
>> >  # completion (e.g., completing "foo" when "origin/foo" exists).
>> > +#
>> > +#   GIT_COMPLETION_OPTIONS
>> > +#
>> > +# When set to "all", complete all possible options
>>
>> I was going to suggest some wording like:
>>
>> When set to "all", include options considered unsafe such as --force
>> in the completion.
>>
>> However per your cover letter it's not just used for that:
>>
>>  10 --force
>>   4 --rerere-autoupdate
>>   1 --unsafe-paths
>>   1 --thin
>>   1 --overwrite-ignore
>>   1 --open-files-in-pager
>>   1 --null
>>   1 --ext-grep
>>   1 --exit-code
>>   1 --auto
>>
>> I wonder if we shouldn't just make this only about --force, I don't see
>> why "git grep --o" should only show --or but not
>> --open-files-in-pager, and e.g. "git grep --" is already verbose so
>> we're not saving much by excluding those.
>>
>> Then this could just become:
>>
>> GIT_COMPLETION_SHOWUNSAFEOPTIONS=1
>>
>> Or other similar boolean variable, for consistency with all the "*SHOW*
>> variables already in git-completion.bash.
>
> No. You're asking for a second default. I'm not adding plenty of
> GIT_COMPLETION_* variables for that. You either have all options, or
> you convince people that --force should be part of the current
> default.

No sorry, I mean that IMO the current patch you have could be simplified
where instead of saying "=all" there's just another variable that only
hides "dangerous" options, i.e. only "--force" (unless I've missed
another supposedly dangerous one).

But as previously discussed I think it just makes sense to stop doing
this conditionally and include --force too, the only stuff we should
hide is stuff like rebase --continue when not in an interactive rebase.

> Or you could push for a generic mechanism that allows you to customize
> your own default. Something like the below patch could give you what
> you want with:
>
> GIT_COMPLETION_OPTIONS=all
> GIT_COMPLETION_EXCLUDES=--open-files-in-pager # and some more
> . /path/to/git-completion.bash
>
> I'm not going to make a real patch for this since people may want to
> ignore --foo in one command and complete --foo in others... I'm just
> not interested in trying to cover all cases.
>
> -- 8< --
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 0cfa489a8e..9ca0d80cd7 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -40,6 +40,10 @@
>  #   GIT_COMPLETION_OPTIONS
>  #
>  # When set to "all", complete all possible options
> +#
> +#   GIT_COMPLETION_EXCLUDES
> +#
> +# Exclude some options from the complete list
>
>  case "$COMP_WORDBREAKS" in
>  *:*) : great ;;
> @@ -298,7 +302,7 @@ __gitcomp_builtin ()
>   # commands, e.g. "git remote add" becomes remote_add.
>   local cmd="$1"
>   local incl="$2"
> - local excl="$3"
> + local excl="$3 $GIT_COMPLETION_EXCLUDES"
>
>   local var=__gitcomp_builtin_"${cmd/-/_}"
>   local options
> -- 8< --

Re: [PATCH v3 42/42] git-completion.bash: add GIT_COMPLETION_OPTIONS=all config

[Eric Sunshine]

On Fri, Feb 9, 2018 at 6:02 AM, Nguyễn Thái Ngọc Duy  wrote:
> By default, some option names (mostly --force, scripting related or for
> internal use) are not completable for various reasons. When
> GIT_COMPLETION_OPTIONS is set to all, all options (except hidden ones)
> are completable.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> @@ -36,6 +36,10 @@
> +#
> +#   GIT_COMPLETION_OPTIONS
> +#
> +# When set to "all", complete all possible options
> @@ -303,7 +307,7 @@ __gitcomp_builtin ()
> if [ -z "$options" ]; then
> # leading and trailing spaces are significant to make
> # option removal work correctly.
> -   options=" $(__git ${cmd/_/ } --git-completion-helper) $incl "
> +   options=" $(__git ${cmd/_/ } 
> --git-completion-helper=$GIT_COMPLETION_OPTIONS) $incl "

This approach is rather different from what I had envisioned. Rather
than asking --git-completion-helper to do the filtering, my thought
was that it should unconditionally dump _all_ options but annotate
them, and then git-completion.bash can filter however it sees fit. For
instance, --git-completion-helper might annotate "dangerous" options
with a "!" ("!--force" or "--force!" or "--force:!" or whatever).

The benefit of this approach is that it more easily supports future
enhancements. For instance, options which only make sense in certain
contexts could be annotated to indicate such. An example are the
--continue, --abort, --skip options for git-rebase which only make
sense when a rebase session is active. One could imagine these options
being annotated something like this:

--abort:rebasing
--continue:rebasing
--skip:rebasing

where git-completion.bash understands the "rebasing" annotation as
meaning that these options only make sense when "rebase-apply" is
present. (In fact, the annotation could be more expressive, such as
"--abort:exists(rebase-apply)", but that might be overkill.)

Re: [PATCH v3 42/42] git-completion.bash: add GIT_COMPLETION_OPTIONS=all config

[Duy Nguyen]

On Fri, Feb 09, 2018 at 03:19:57PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Feb 09 2018, Nguyễn Thái Ngọc Duy jotted:
> 
> > By default, some option names (mostly --force, scripting related or for
> > internal use) are not completable for various reasons. When
> > GIT_COMPLETION_OPTIONS is set to all, all options (except hidden ones)
> > are completable.
> >
> > Signed-off-by: Nguyễn Thái Ngọc Duy 
> > ---
> >  contrib/completion/git-completion.bash |  6 +-
> >  parse-options.c| 11 +++
> >  2 files changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/contrib/completion/git-completion.bash 
> > b/contrib/completion/git-completion.bash
> > index 0ddf40063b..0cfa489a8e 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -36,6 +36,10 @@
> >  #
> >  # When set to "1", do not include "DWIM" suggestions in git-checkout
> >  # completion (e.g., completing "foo" when "origin/foo" exists).
> > +#
> > +#   GIT_COMPLETION_OPTIONS
> > +#
> > +# When set to "all", complete all possible options
> 
> I was going to suggest some wording like:
> 
> When set to "all", include options considered unsafe such as --force
> in the completion.
> 
> However per your cover letter it's not just used for that:
> 
>  10 --force
>   4 --rerere-autoupdate
>   1 --unsafe-paths
>   1 --thin
>   1 --overwrite-ignore
>   1 --open-files-in-pager
>   1 --null
>   1 --ext-grep
>   1 --exit-code
>   1 --auto
> 
> I wonder if we shouldn't just make this only about --force, I don't see
> why "git grep --o" should only show --or but not
> --open-files-in-pager, and e.g. "git grep --" is already verbose so
> we're not saving much by excluding those.
> 
> Then this could just become:
> 
> GIT_COMPLETION_SHOWUNSAFEOPTIONS=1
> 
> Or other similar boolean variable, for consistency with all the "*SHOW*
> variables already in git-completion.bash.

No. You're asking for a second default. I'm not adding plenty of
GIT_COMPLETION_* variables for that. You either have all options, or
you convince people that --force should be part of the current
default.

Or you could push for a generic mechanism that allows you to customize
your own default. Something like the below patch could give you what
you want with:

GIT_COMPLETION_OPTIONS=all
GIT_COMPLETION_EXCLUDES=--open-files-in-pager # and some more
. /path/to/git-completion.bash

I'm not going to make a real patch for this since people may want to
ignore --foo in one command and complete --foo in others... I'm just
not interested in trying to cover all cases.

-- 8< --
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 0cfa489a8e..9ca0d80cd7 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -40,6 +40,10 @@
 #   GIT_COMPLETION_OPTIONS
 #
 # When set to "all", complete all possible options
+#
+#   GIT_COMPLETION_EXCLUDES
+#
+# Exclude some options from the complete list
 
 case "$COMP_WORDBREAKS" in
 *:*) : great ;;
@@ -298,7 +302,7 @@ __gitcomp_builtin ()
# commands, e.g. "git remote add" becomes remote_add.
local cmd="$1"
local incl="$2"
-   local excl="$3"
+   local excl="$3 $GIT_COMPLETION_EXCLUDES"
 
local var=__gitcomp_builtin_"${cmd/-/_}"
local options
-- 8< --

Re: [PATCH v3 42/42] git-completion.bash: add GIT_COMPLETION_OPTIONS=all config

[Ævar Arnfjörð Bjarmason]

On Fri, Feb 09 2018, Nguyễn Thái Ngọc Duy jotted:

> By default, some option names (mostly --force, scripting related or for
> internal use) are not completable for various reasons. When
> GIT_COMPLETION_OPTIONS is set to all, all options (except hidden ones)
> are completable.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  contrib/completion/git-completion.bash |  6 +-
>  parse-options.c| 11 +++
>  2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 0ddf40063b..0cfa489a8e 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -36,6 +36,10 @@
>  #
>  # When set to "1", do not include "DWIM" suggestions in git-checkout
>  # completion (e.g., completing "foo" when "origin/foo" exists).
> +#
> +#   GIT_COMPLETION_OPTIONS
> +#
> +# When set to "all", complete all possible options

I was going to suggest some wording like:

When set to "all", include options considered unsafe such as --force
in the completion.

However per your cover letter it's not just used for that:

 10 --force
  4 --rerere-autoupdate
  1 --unsafe-paths
  1 --thin
  1 --overwrite-ignore
  1 --open-files-in-pager
  1 --null
  1 --ext-grep
  1 --exit-code
  1 --auto

I wonder if we shouldn't just make this only about --force, I don't see
why "git grep --o" should only show --or but not
--open-files-in-pager, and e.g. "git grep --" is already verbose so
we're not saving much by excluding those.

Then this could just become:

GIT_COMPLETION_SHOWUNSAFEOPTIONS=1

Or other similar boolean variable, for consistency with all the "*SHOW*
variables already in git-completion.bash.

>  case "$COMP_WORDBREAKS" in
>  *:*) : great ;;
> @@ -303,7 +307,7 @@ __gitcomp_builtin ()
>   if [ -z "$options" ]; then
>   # leading and trailing spaces are significant to make
>   # option removal work correctly.
> - options=" $(__git ${cmd/_/ } --git-completion-helper) $incl "
> + options=" $(__git ${cmd/_/ } 
> --git-completion-helper=$GIT_COMPLETION_OPTIONS) $incl "
>   for i in $excl; do
>   options="${options/ $i / }"
>   done
> diff --git a/parse-options.c b/parse-options.c
> index 979577ba2c..5b8b2b376e 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -430,14 +430,17 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
>   * many options that do not suppress it properly.
>   */
>  static int show_gitcomp(struct parse_opt_ctx_t *ctx,
> - const struct option *opts)
> + const struct option *opts,
> + const char *arg)
>  {
>   for (; opts->type != OPTION_END; opts++) {
>   const char *suffix = "";
>
>   if (!opts->long_name)
>   continue;
> - if (opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE))
> + if (opts->flags & PARSE_OPT_HIDDEN)
> + continue;
> + if ((opts->flags & PARSE_OPT_NOCOMPLETE) && strcmp(arg, "all"))
>   continue;
>
>   switch (opts->type) {
> @@ -498,8 +501,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>   goto show_usage;
>
>   /* lone --git-completion-helper is asked by git-completion.bash 
> */
> - if (ctx->total == 1 && !strcmp(arg + 1, 
> "-git-completion-helper"))
> - return show_gitcomp(ctx, options);
> + if (ctx->total == 1 && skip_prefix(arg + 1, 
> "-git-completion-helper=", ))
> + return show_gitcomp(ctx, options, arg);
>
>   if (arg[1] != '-') {
>   ctx->opt = arg + 1;