Re: [PATCH v3 42/42] git-completion.bash: add GIT_COMPLETION_OPTIONS=all config
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
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
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
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
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=", &arg)) > + return show_gitcomp(ctx, options, arg); > > if (arg[1] != '-') { > ctx->opt = arg + 1;
[PATCH v3 42/42] git-completion.bash: add GIT_COMPLETION_OPTIONS=all config
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 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=", &arg)) + return show_gitcomp(ctx, options, arg); if (arg[1] != '-') { ctx->opt = arg + 1; -- 2.16.1.207.gedba492059