threads / rfc / 47619

RFC patch, 2 partsAutomate updating git-completion.bash a bit

Subject: [PATCH/RFC 0/2] Automate updating git-completion.bash a bit

## tl;dr

14 messages between Jan 16, 2018 and Apr 11, 2019. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Nguyễn Thái Ngọc Duy· Jan 16, 2018, 10:36 UTC · lore

I noticed --recurse-submodules was missing from git-grep complete list. Then I found a couple more should be on the list as well and many more in future may face the same faith. Perhaps this helps remedy this situation?

This lets us extract certain information from git commands and feed it directly to git-completion.bash. Now long options by default will be complete-able (which also means it's the reviewer's and coder's responsibility to add "no complete" flag appropriately) but I think the number of new dangerous options will be much fewer than completeable ones.

This is not really a new idea. Python has argcomplete that does more or less the same thing.

This is just a proof of concept. More commands should be converted of course if it's a good thing to do.

Nguyễn Thái Ngọc Duy (2):
  parse-options: support --git-completion-helper
  git-completion: use --git-completion-helper
 builtin/grep.c                         | 13 +++++++-----
 contrib/completion/git-completion.bash | 16 +--------------
 parse-options.c                        | 37 ++++++++++++++++++++++++++++++++++
 parse-options.h                        | 14 ++++++++-----
 4 files changed, 55 insertions(+), 25 deletions(-)
-- 
2.15.1.600.g899a5f85c6
Nguyễn Thái Ngọc Duy· Jan 16, 2018, 10:36 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 1/2] parse-options: support --git-completion-helper

This option is designed to be used by git-completion.bash. For many simple cases, what we do in there is usually

    __gitcomp "lots of completion options"

which has to be manually updated when a new user-visible option is added. With support from parse-options, we can write

    __gitcomp "$(git command --git-completion-helper)"

and get that list directly from the parser for free. Dangerous/Unpopular options could be hidden with the new "NO_GITCOMP" flag.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 parse-options.c | 37 +++++++++++++++++++++++++++++++++++++
 parse-options.h |  6 ++++--
 2 files changed, 41 insertions(+), 2 deletions(-)
Show changes to 2 files +41 −2

parse-options.c, parse-options.h

diff --git a/parse-options.c b/parse-options.c
index fca7159646..6c542f44cf 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -425,6 +425,35 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	parse_options_check(options);
 }
 
+static int show_gitcomp(struct parse_opt_ctx_t *ctx, const struct option *opts)
+{
+	for (; opts->type != OPTION_END; opts++) {
+		const char *suffix;
+
+		if (!opts->long_name)
+			continue;
+		if (opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NO_GITCOMP))
+			continue;
+
+		switch (opts->type) {
+		case OPTION_GROUP:
+			continue;
+		case OPTION_STRING:
+		case OPTION_FILENAME:
+		case OPTION_INTEGER:
+		case OPTION_MAGNITUDE:
+			if (!(opts->flags & PARSE_OPT_OPTARG))
+				suffix = "=";
+			break;
+		default:
+			suffix = "";
+		}
+		printf(" --%s%s", opts->long_name, suffix);
+	}
+	fputc('\n', stdout);
+	exit(0);
+}
+
 static int usage_with_options_internal(struct parse_opt_ctx_t *,
 				       const char * const *,
 				       const struct option *, int, int);
@@ -434,6 +463,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 		       const char * const usagestr[])
 {
 	int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);
+	int internal_gitcomp = !(ctx->flags & PARSE_OPT_NO_INTERNAL_GITCOMP);
 	int err = 0;
 
 	/* we must reset ->opt, unknown short option leave it dangling */
@@ -455,6 +485,11 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 		if (internal_help && ctx->total == 1 && !strcmp(arg + 1, "h"))
 			goto show_usage;
 
+		/* lone --git-completion-helper is asked by git-completion.bash */
+		if (internal_gitcomp && ctx->total == 1 &&
+		    !strcmp(arg + 1, "-git-completion-helper"))
+			goto show_git_comp;
+
 		if (arg[1] != '-') {
 			ctx->opt = arg + 1;
 			switch (parse_short_opt(ctx, options)) {
@@ -521,6 +556,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 	err = 1;
  show_usage:
 	return usage_with_options_internal(ctx, usagestr, options, 0, err);
+ show_git_comp:
+	return show_gitcomp(ctx, options);
 }
 
 int parse_options_end(struct parse_opt_ctx_t *ctx)
diff --git a/parse-options.h b/parse-options.h
index af711227ae..8a3389b05b 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -27,7 +27,8 @@ enum parse_opt_flags {
 	PARSE_OPT_STOP_AT_NON_OPTION = 2,
 	PARSE_OPT_KEEP_ARGV0 = 4,
 	PARSE_OPT_KEEP_UNKNOWN = 8,
-	PARSE_OPT_NO_INTERNAL_HELP = 16
+	PARSE_OPT_NO_INTERNAL_HELP = 16,
+	PARSE_OPT_NO_INTERNAL_GITCOMP = 32
 };
 
 enum parse_opt_option_flags {
@@ -38,7 +39,8 @@ enum parse_opt_option_flags {
 	PARSE_OPT_LASTARG_DEFAULT = 16,
 	PARSE_OPT_NODASH = 32,
 	PARSE_OPT_LITERAL_ARGHELP = 64,
-	PARSE_OPT_SHELL_EVAL = 256
+	PARSE_OPT_SHELL_EVAL = 256,
+	PARSE_OPT_NO_GITCOMP = 512
 };
 
 struct option;

-- 
2.15.1.600.g899a5f85c6
Jacob Keller· Jan 16, 2018, 18:25 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 1/2] parse-options: support --git-completion-helper

On Tue, Jan 16, 2018 at 2:36 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 14 quoted lines
> This option is designed to be used by git-completion.bash. For many
> simple cases, what we do in there is usually
>
>     __gitcomp "lots of completion options"
>
> which has to be manually updated when a new user-visible option is
> added. With support from parse-options, we can write
>
>     __gitcomp "$(git command --git-completion-helper)"
>
> and get that list directly from the parser for free. Dangerous/Unpopular
> options could be hidden with the new "NO_GITCOMP" flag.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Was this possibly avoided in the past due to being slower than simply storing the list in the completion code itself?

I think this is a good idea myself.

Thanks, Jake

Show 101 quoted lines
> ---
>  parse-options.c | 37 +++++++++++++++++++++++++++++++++++++
>  parse-options.h |  6 ++++--
>  2 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index fca7159646..6c542f44cf 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -425,6 +425,35 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
>         parse_options_check(options);
>  }
>
> +static int show_gitcomp(struct parse_opt_ctx_t *ctx, const struct option *opts)
> +{
> +       for (; opts->type != OPTION_END; opts++) {
> +               const char *suffix;
> +
> +               if (!opts->long_name)
> +                       continue;
> +               if (opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NO_GITCOMP))
> +                       continue;
> +
> +               switch (opts->type) {
> +               case OPTION_GROUP:
> +                       continue;
> +               case OPTION_STRING:
> +               case OPTION_FILENAME:
> +               case OPTION_INTEGER:
> +               case OPTION_MAGNITUDE:
> +                       if (!(opts->flags & PARSE_OPT_OPTARG))
> +                               suffix = "=";
> +                       break;
> +               default:
> +                       suffix = "";
> +               }
> +               printf(" --%s%s", opts->long_name, suffix);
> +       }
> +       fputc('\n', stdout);
> +       exit(0);
> +}
> +
>  static int usage_with_options_internal(struct parse_opt_ctx_t *,
>                                        const char * const *,
>                                        const struct option *, int, int);
> @@ -434,6 +463,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>                        const char * const usagestr[])
>  {
>         int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);
> +       int internal_gitcomp = !(ctx->flags & PARSE_OPT_NO_INTERNAL_GITCOMP);
>         int err = 0;
>
>         /* we must reset ->opt, unknown short option leave it dangling */
> @@ -455,6 +485,11 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>                 if (internal_help && ctx->total == 1 && !strcmp(arg + 1, "h"))
>                         goto show_usage;
>
> +               /* lone --git-completion-helper is asked by git-completion.bash */
> +               if (internal_gitcomp && ctx->total == 1 &&
> +                   !strcmp(arg + 1, "-git-completion-helper"))
> +                       goto show_git_comp;
> +
>                 if (arg[1] != '-') {
>                         ctx->opt = arg + 1;
>                         switch (parse_short_opt(ctx, options)) {
> @@ -521,6 +556,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>         err = 1;
>   show_usage:
>         return usage_with_options_internal(ctx, usagestr, options, 0, err);
> + show_git_comp:
> +       return show_gitcomp(ctx, options);
>  }
>
>  int parse_options_end(struct parse_opt_ctx_t *ctx)
> diff --git a/parse-options.h b/parse-options.h
> index af711227ae..8a3389b05b 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -27,7 +27,8 @@ enum parse_opt_flags {
>         PARSE_OPT_STOP_AT_NON_OPTION = 2,
>         PARSE_OPT_KEEP_ARGV0 = 4,
>         PARSE_OPT_KEEP_UNKNOWN = 8,
> -       PARSE_OPT_NO_INTERNAL_HELP = 16
> +       PARSE_OPT_NO_INTERNAL_HELP = 16,
> +       PARSE_OPT_NO_INTERNAL_GITCOMP = 32
>  };
>
>  enum parse_opt_option_flags {
> @@ -38,7 +39,8 @@ enum parse_opt_option_flags {
>         PARSE_OPT_LASTARG_DEFAULT = 16,
>         PARSE_OPT_NODASH = 32,
>         PARSE_OPT_LITERAL_ARGHELP = 64,
> -       PARSE_OPT_SHELL_EVAL = 256
> +       PARSE_OPT_SHELL_EVAL = 256,
> +       PARSE_OPT_NO_GITCOMP = 512
>  };
>
>  struct option;
> --
> 2.15.1.600.g899a5f85c6
>
Duy Nguyen· Jan 17, 2018, 00:21 UTC · re: Jacob Keller · lore

Re: [PATCH 1/2] parse-options: support --git-completion-helper

On Wed, Jan 17, 2018 at 1:25 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
Show 18 quoted lines
> On Tue, Jan 16, 2018 at 2:36 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> This option is designed to be used by git-completion.bash. For many
>> simple cases, what we do in there is usually
>>
>>     __gitcomp "lots of completion options"
>>
>> which has to be manually updated when a new user-visible option is
>> added. With support from parse-options, we can write
>>
>>     __gitcomp "$(git command --git-completion-helper)"
>>
>> and get that list directly from the parser for free. Dangerous/Unpopular
>> options could be hidden with the new "NO_GITCOMP" flag.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> Was this possibly avoided in the past due to being slower than simply
> storing the list in the completion code itself?

At least on linux I would not worry too much about performance (but then I don't know what platforms this git-completion supports and whether this may become real performance problems for them). git-completion.bash already executes some heavy commands (getting refs for completion for example), though this makes the script call git much earlier.

There's one thing I will have to be careful about though, that to make sure that --git-completion-helper runs even if the command itself is not supposed to run, e.g. calling git-status without a worktree will fail, but calling 'git status --git-completion-helper' should still work in that condition.

-- 
Duy
Junio C Hamano· Jan 16, 2018, 23:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 1/2] parse-options: support --git-completion-helper

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 18 quoted lines
> This option is designed to be used by git-completion.bash. For many
> simple cases, what we do in there is usually
>
>     __gitcomp "lots of completion options"
>
> which has to be manually updated when a new user-visible option is
> added. With support from parse-options, we can write
>
>     __gitcomp "$(git command --git-completion-helper)"
>
> and get that list directly from the parser for free. Dangerous/Unpopular
> options could be hidden with the new "NO_GITCOMP" flag.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  parse-options.c | 37 +++++++++++++++++++++++++++++++++++++
>  parse-options.h |  6 ++++--
>  2 files changed, 41 insertions(+), 2 deletions(-)

I do not know if you need PARSE_OPT_NO_INTERNAL_GITCOMP. The only way to trigger this is by passing a rather long option (intended to be used only by scripts), so unlike PARSE_OPT_NO_INTERNAL_HELP which was an escape hatch to free a short-and-sweet "-h" to be retargetted by a selected few commands, there may not be need for it.

Some day when everybody uses parse-options, it would be trivially correct to declare that this is the right approach ;-) I am not sure how much of the current completion script's hardcoded option list can be reduced with this approach with today's code, given that the bigger ones (diff and log family) are not using parse-options API at all, but for the ones that already do use parse-options, I think this makes sense.

Thanks for a fun read ;-)
Duy Nguyen· Jan 17, 2018, 00:27 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] parse-options: support --git-completion-helper

On Wed, Jan 17, 2018 at 6:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 26 quoted lines
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> This option is designed to be used by git-completion.bash. For many
>> simple cases, what we do in there is usually
>>
>>     __gitcomp "lots of completion options"
>>
>> which has to be manually updated when a new user-visible option is
>> added. With support from parse-options, we can write
>>
>>     __gitcomp "$(git command --git-completion-helper)"
>>
>> and get that list directly from the parser for free. Dangerous/Unpopular
>> options could be hidden with the new "NO_GITCOMP" flag.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  parse-options.c | 37 +++++++++++++++++++++++++++++++++++++
>>  parse-options.h |  6 ++++--
>>  2 files changed, 41 insertions(+), 2 deletions(-)
>
> I do not know if you need PARSE_OPT_NO_INTERNAL_GITCOMP.  The only
> way to trigger this is by passing a rather long option (intended to
> be used only by scripts), so unlike PARSE_OPT_NO_INTERNAL_HELP which
> was an escape hatch to free a short-and-sweet "-h" to be retargetted
> by a selected few commands, there may not be need for it.

It's a copy/paste from internal_help. Because I was asking for comments, not really aiming to submit these changes to git.git (yet), I didn't bother to check further.

Show 7 quoted lines
> Some day when everybody uses parse-options, it would be trivially
> correct to declare that this is the right approach ;-) I am not sure
> how much of the current completion script's hardcoded option list
> can be reduced with this approach with today's code, given that the
> bigger ones (diff and log family) are not using parse-options API at
> all, but for the ones that already do use parse-options, I think
> this makes sense.

Yeah yeah the horrible handle_revision_opt() and diff_opt_parse(). I think I attempted to convert them two parse-options at some point, not sure why I dropped it, maybe because target variables are in dynamically allocated memory ('struct diff_options *' and 'struct rev_info *') which does not fit well to how we usually initialize 'struct options[]'.

-- 
Duy
Nguyễn Thái Ngọc Duy· Jan 16, 2018, 10:37 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 2/2] git-completion: use --git-completion-helper

The builtin command is updated to hide some options. The new options that are complete-able are:

--after-context= --all-match --before-context= --color --context --exclude-standard --recurse-submodules --textconv

--max-depth and --threads are already completable, but now it will complete with the "=" suffix because these always take an argument.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c                         | 13 ++++++++-----
 contrib/completion/git-completion.bash | 16 +---------------
 parse-options.h                        |  8 +++++---
 3 files changed, 14 insertions(+), 23 deletions(-)
Show changes to 3 files +14 −23

builtin/grep.c, contrib/completion/git-completion.bash, parse-options.h

diff --git a/builtin/grep.c b/builtin/grep.c
index 3ca4ac80d8..8fe5560ee8 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -832,8 +832,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_BOOL('L', "files-without-match",
 			&opt.unmatch_name_only,
 			N_("show only the names of files without match")),
-		OPT_BOOL('z', "null", &opt.null_following_name,
-			N_("print NUL after filenames")),
+		OPT_BOOL_F('z', "null", &opt.null_following_name,
+			   N_("print NUL after filenames"),
+			   PARSE_OPT_NO_GITCOMP),
 		OPT_BOOL('c', "count", &opt.count,
 			N_("show the number of matches instead of matching lines")),
 		OPT__COLOR(&opt.color, N_("highlight matches")),
@@ -884,9 +885,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_GROUP(""),
 		{ OPTION_STRING, 'O', "open-files-in-pager", &show_in_pager,
 			N_("pager"), N_("show matching files in the pager"),
-			PARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },
-		OPT_BOOL(0, "ext-grep", &external_grep_allowed__ignored,
-			 N_("allow calling of grep(1) (ignored by this build)")),
+			PARSE_OPT_OPTARG | PARSE_OPT_NO_GITCOMP,
+			NULL, (intptr_t)default_pager },
+		OPT_BOOL_F(0, "ext-grep", &external_grep_allowed__ignored,
+			   N_("allow calling of grep(1) (ignored by this build)"),
+			   PARSE_OPT_NO_GITCOMP),
 		OPT_END()
 	};
 
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3683c772c5..f0d9126fd6 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1585,21 +1585,7 @@ _git_grep ()
 
 	case "$cur" in
 	--*)
-		__gitcomp "
-			--cached
-			--text --ignore-case --word-regexp --invert-match
-			--full-name --line-number
-			--extended-regexp --basic-regexp --fixed-strings
-			--perl-regexp
-			--threads
-			--files-with-matches --name-only
-			--files-without-match
-			--max-depth
-			--count
-			--and --or --not --all-match
-			--break --heading --show-function --function-context
-			--untracked --no-index
-			"
+		__gitcomp "$(git grep --git-completion-helper)"
 		return
 		;;
 	esac
diff --git a/parse-options.h b/parse-options.h
index 8a3389b05b..2bd833a4c5 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -124,9 +124,11 @@ struct option {
 				      (h), PARSE_OPT_NOARG, NULL, (b) }
 #define OPT_COUNTUP(s, l, v, h)     { OPTION_COUNTUP, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG }
-#define OPT_SET_INT(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \
-				      (h), PARSE_OPT_NOARG, NULL, (i) }
-#define OPT_BOOL(s, l, v, h)        OPT_SET_INT(s, l, v, h, 1)
+#define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), (v), NULL, \
+					  (h), PARSE_OPT_NOARG | (f), NULL, (i) }
+#define OPT_SET_INT(s, l, v, h, i)  OPT_SET_INT_F(s, l, v, h, i, 0)
+#define OPT_BOOL_F(s, l, v, h, f)   OPT_SET_INT_F(s, l, v, h, 1, f)
+#define OPT_BOOL(s, l, v, h)        OPT_BOOL_F(s, l, v, h, 0)
 #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}
 #define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), (v), NULL, \

-- 
2.15.1.600.g899a5f85c6
SZEDER Gábor· Jan 17, 2018, 00:51 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH/RFC 0/2] Automate updating git-completion.bash a bit

On Tue, Jan 16, 2018 at 11:36 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 14 quoted lines
> I noticed --recurse-submodules was missing from git-grep complete
> list. Then I found a couple more should be on the list as well and
> many more in future may face the same faith. Perhaps this helps remedy
> this situation?
>
> This lets us extract certain information from git commands and feed it
> directly to git-completion.bash. Now long options by default will
> be complete-able (which also means it's the reviewer's and coder's
> responsibility to add "no complete" flag appropriately) but I think
> the number of new dangerous options will be much fewer than
> completeable ones.
>
> This is not really a new idea. Python has argcomplete that does more
> or less the same thing.
This has come up before for Git as well, see:
  https://public-inbox.org/git/1334140165-24958-1-git-send-email-bebarino@gmail.com/T/#u

I see that your approach solves one of the shortcomings of those older patches, namely it makes possible to omit dangerous options from completion. Great.

I also see that you want to invoke git in a subshell every time the user attempts to complete an --option. Not so great, at least for Windows users. That older thread contains a few ideas about how to do it only once by lazy-initializing a variable for each command to hold its options.

Show 16 quoted lines
> This is just a proof of concept. More commands should be converted of
> course if it's a good thing to do.
>
> Nguyễn Thái Ngọc Duy (2):
>   parse-options: support --git-completion-helper
>   git-completion: use --git-completion-helper
>
>  builtin/grep.c                         | 13 +++++++-----
>  contrib/completion/git-completion.bash | 16 +--------------
>  parse-options.c                        | 37 ++++++++++++++++++++++++++++++++++
>  parse-options.h                        | 14 ++++++++-----
>  4 files changed, 55 insertions(+), 25 deletions(-)
>
> --
> 2.15.1.600.g899a5f85c6
>
Duy Nguyen· Jan 17, 2018, 09:16 UTC · re: SZEDER Gábor · lore

Re: [PATCH/RFC 0/2] Automate updating git-completion.bash a bit

On Wed, Jan 17, 2018 at 7:51 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
Show 20 quoted lines
> On Tue, Jan 16, 2018 at 11:36 AM, Nguyễn Thái Ngọc Duy
> <pclouds@gmail.com> wrote:
>> I noticed --recurse-submodules was missing from git-grep complete
>> list. Then I found a couple more should be on the list as well and
>> many more in future may face the same faith. Perhaps this helps remedy
>> this situation?
>>
>> This lets us extract certain information from git commands and feed it
>> directly to git-completion.bash. Now long options by default will
>> be complete-able (which also means it's the reviewer's and coder's
>> responsibility to add "no complete" flag appropriately) but I think
>> the number of new dangerous options will be much fewer than
>> completeable ones.
>>
>> This is not really a new idea. Python has argcomplete that does more
>> or less the same thing.
>
> This has come up before for Git as well, see:
>
>   https://public-inbox.org/git/1334140165-24958-1-git-send-email-bebarino@gmail.com/T/#u
Thanks! I did search the archive but couldn't find this one.
Show 10 quoted lines
>
> I see that your approach solves one of the shortcomings of those older
> patches, namely it makes possible to omit dangerous options from
> completion.  Great.
>
> I also see that you want to invoke git in a subshell every time the user
> attempts to complete an --option.  Not so great, at least for Windows
> users.  That older thread contains a few ideas about how to do it only
> once by lazy-initializing a variable for each command to hold its
> options.
Noted.

I see you also pointed out the problem with running commands like "git-status" without a repository. I'll try to address this too if possible (I'm thinking about making struct options[] available outside cmd_*(); then we could handle it more like "git --version" which always works)

-- 
Duy
Duy Nguyen· Jan 17, 2018, 09:34 UTC · re: Duy Nguyen · lore

Re: [PATCH/RFC 0/2] Automate updating git-completion.bash a bit

Actually I forgot another option. What if we automate updating the script at "compile" time instead of calling git at run time? E.g. with something like below, a contributor could just run

    make update-completion

then add git-completion.bash changes to the same patch that introduces new options. If they forget, Junio could always run this near -rc0.

I know this output is a bit ugly. I probably could try to make the update work with wrapped __gitcomp lines to be friendlier to human eyes.

-- 8< --
Show changes to 3 files +11 −16

Makefile, contrib/completion/git-completion.bash, contrib/completion/update.sh

diff --git a/Makefile b/Makefile
index 1a9b23b679..05eb7c8742 100644
--- a/Makefile
+++ b/Makefile
@@ -2834,3 +2834,5 @@ cover_db: coverage-report
 cover_db_html: cover_db
 	cover -report html -outputdir cover_db_html cover_db
 
+update-completion:
+	contrib/completion/update.sh contrib/completion/git-completion.bash ./git
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3683c772c5..e8c224f486 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1585,21 +1585,7 @@ _git_grep ()
 
 	case "$cur" in
 	--*)
-		__gitcomp "
-			--cached
-			--text --ignore-case --word-regexp --invert-match
-			--full-name --line-number
-			--extended-regexp --basic-regexp --fixed-strings
-			--perl-regexp
-			--threads
-			--files-with-matches --name-only
-			--files-without-match
-			--max-depth
-			--count
-			--and --or --not --all-match
-			--break --heading --show-function --function-context
-			--untracked --no-index
-			"
+		__gitcomp_auto grep "--cached --text --ignore-case --word-regexp --invert-match --full-name --line-number --extended-regexp --basic-regexp --fixed-strings --perl-regexp --threads --files-with-matches --name-only --files-without-match --max-depth --count --and --or --not --all-match --break --heading --show-function --function-context --untracked --no-index"
 		return
 		;;
 	esac
diff --git a/contrib/completion/update.sh b/contrib/completion/update.sh
new file mode 100755
index 0000000000..99c4841152
--- /dev/null
+++ b/contrib/completion/update.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+file="$1"
+git="$2"
+
+grep __gitcomp_auto "$file" | while read a cmd b; do
+    sed -i "s/\\(.*$a $cmd \).*/\\1$("$git" $cmd --git-completion-helper)/" "$file"
+done
-- 8< --




On Wed, Jan 17, 2018 at 04:16:22PM +0700, Duy Nguyen wrote:
> On Wed, Jan 17, 2018 at 7:51 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > On Tue, Jan 16, 2018 at 11:36 AM, Nguyễn Thái Ngọc Duy
> > <pclouds@gmail.com> wrote:
> >> I noticed --recurse-submodules was missing from git-grep complete
> >> list. Then I found a couple more should be on the list as well and
> >> many more in future may face the same faith. Perhaps this helps remedy
> >> this situation?
> >>
> >> This lets us extract certain information from git commands and feed it
> >> directly to git-completion.bash. Now long options by default will
> >> be complete-able (which also means it's the reviewer's and coder's
> >> responsibility to add "no complete" flag appropriately) but I think
> >> the number of new dangerous options will be much fewer than
> >> completeable ones.
> >>
> >> This is not really a new idea. Python has argcomplete that does more
> >> or less the same thing.
> >
> > This has come up before for Git as well, see:
> >
> >   https://public-inbox.org/git/1334140165-24958-1-git-send-email-bebarino@gmail.com/T/#u
> 
> Thanks! I did search the archive but couldn't find this one.
> 
> >
> > I see that your approach solves one of the shortcomings of those older
> > patches, namely it makes possible to omit dangerous options from
> > completion.  Great.
> >
> > I also see that you want to invoke git in a subshell every time the user
> > attempts to complete an --option.  Not so great, at least for Windows
> > users.  That older thread contains a few ideas about how to do it only
> > once by lazy-initializing a variable for each command to hold its
> > options.
> 
> Noted.
> 
> I see you also pointed out the problem with running commands like
> "git-status" without a repository. I'll try to address this too if
> possible (I'm thinking about making struct options[] available outside
> cmd_*(); then we could handle it more like "git --version" which
> always works)
> -- 
> Duy
SZEDER Gábor· Jan 22, 2018, 18:03 UTC · re: Duy Nguyen · lore

Re: [PATCH/RFC 0/2] Automate updating git-completion.bash a bit

On Wed, Jan 17, 2018 at 10:34 AM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 8 quoted lines
> Actually I forgot another option. What if we automate updating the
> script at "compile" time instead of calling git at run time? E.g. with
> something like below, a contributor could just run
>
>     make update-completion
>
> then add git-completion.bash changes to the same patch that introduces
> new options. If they forget

They inevitably will :) If contributors have to remember something anyway, then they might as well remember to update the completion script in the first place.

Another alternative would be to extend t9902 with (preferably auto-generated) tests to compare the output of 'git $cmd --git-completion-helper' with 'run_completion "git $cmd --"'. Then contributors wouldn't have to remember anything, because everyone runs the full test suite every time anyway, right?

However, that would result in some code churn initially, because I suspect the options are listed in different order in the command and in the completion script.

All in all I don't think it would trump getting all --options straight from the commands themselves.

Duy Nguyen· Jan 23, 2018, 09:59 UTC · re: SZEDER Gábor · lore

Re: [PATCH/RFC 0/2] Automate updating git-completion.bash a bit

On Mon, Jan 22, 2018 at 07:03:24PM +0100, SZEDER Gábor wrote:
Show 11 quoted lines
> On Wed, Jan 17, 2018 at 10:34 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> > Actually I forgot another option. What if we automate updating the
> > script at "compile" time instead of calling git at run time? E.g. with
> > something like below, a contributor could just run
> >
> >     make update-completion
> >
> > then add git-completion.bash changes to the same patch that introduces
> > new options. If they forget
> 
> They inevitably will :)

OK. We go with the first option then (with caching to reduce overhead on Windows). I'm not going to bother you with actual code changes. The diff of completable options looks like below. It would be great if people could check if some options should NOT be completable for some reason.

A couple points
- Ignore options that are removed in the diff. They are removed for a
  reason which I will explain when real patches come.
- There are regressions where --foo= becomes --foo, I hope this is ok
  for now. We can tweak this later.
- In some commands you can complete both --foo and --foo= (now it's
  just one form, --foo). I would argue that it's pointless. It's no
  big deal to type '=' (or a space) after we have completed --foo.
- I feel --force is not treated the same way everywhere. But I guess
  that's ok since "forcing" in some command context may be less severe
  than others.
-- 8< --
Show changes to 32 files +167 −2

git-add.txt, git-am.txt, git-apply.txt, git-branch.txt, git-checkout.txt, git-cherry-pick.txt, git-clean.txt, git-clone.txt, git-commit.txt, git-config.txt, git-describe.txt, git-fsck.txt, git-gc.txt, git-grep.txt, git-init.txt, git-ls-files.txt, git-ls-remote.txt, git-mv.txt, git-name-rev.txt, git-notes_add.txt, git-notes_append.txt, git-notes_copy.txt, git-push.txt, git-remote_add.txt, git-replace.txt, git-reset.txt, git-revert.txt, git-show-branch.txt, git-status.txt, git-tag.txt, git-worktree_add.txt, git-worktree_lock.txt

diff --git a/git-add.txt b/git-add.txt
index 1c249a3..2693cc1 100644
--- a/git-add.txt
+++ b/git-add.txt
@@ -1,10 +1,15 @@
+--all
 --chmod=
 --dry-run
 --edit
 --force
 --ignore-errors
+--ignore-missing
+--ignore-removal
 --intent-to-add
 --interactive
 --patch
 --refresh
+--renormalize
 --update
+--verbose
diff --git a/git-am.txt b/git-am.txt
index b0082bf..552dc96 100644
--- a/git-am.txt
+++ b/git-am.txt
@@ -1,12 +1,24 @@
 --3way
 --committer-date-is-author-date
+--directory
+--exclude
+--gpg-sign
 --ignore-date
 --ignore-space-change
 --ignore-whitespace
+--include
 --interactive
 --keep
+--keep-cr
+--keep-non-patch
+--message-id
+--no-keep-cr
 --no-utf8
+--patch-format
+--quiet
+--reject
+--resolvemsg=
 --scissors
 --signoff
 --utf8
---whitespace=
+--whitespace
diff --git a/git-apply.txt b/git-apply.txt
index 6bf4c2f..71d53d2 100644
--- a/git-apply.txt
+++ b/git-apply.txt
@@ -1,13 +1,17 @@
+--3way
+--allow-overlap
 --apply
+--build-fake-ancestor=
 --cached
 --check
---directory=
---exclude=
+--directory
+--exclude
 --ignore-space-change
 --ignore-whitespace
 --inaccurate-eof
+--include
 --index
---index-info
+--no-add
 --no-add
 --numstat
 --recount
@@ -17,4 +21,4 @@
 --summary
 --unidiff-zero
 --verbose
---whitespace=
+--whitespace
diff --git a/git-branch.txt b/git-branch.txt
index 69594e3..9d308aa 100644
--- a/git-branch.txt
+++ b/git-branch.txt
@@ -1,10 +1,14 @@
---abbrev=
+--abbrev
+--all
 --color
 --column
 --contains
 --copy
+--create-reflog
 --delete
 --edit-description
+--format=
+--ignore-case
 --list
 --merged
 --move
@@ -15,9 +19,10 @@
 --no-merged
 --no-track
 --points-at
+--quiet
 --remotes
 --set-upstream-to=
---sort=
+--sort
 --track
 --unset-upstream
 --verbose
diff --git a/git-checkout.txt b/git-checkout.txt
index 493a1fe..75f19d2 100644
--- a/git-checkout.txt
+++ b/git-checkout.txt
@@ -1,12 +1,14 @@
 --conflict=
 --detach
+--ignore-other-worktrees
 --ignore-skip-worktree-bits
 --merge
 --no-recurse-submodules
 --no-track
---orphan
+--orphan=
 --ours
 --patch
+--progress
 --quiet
 --recurse-submodules
 --theirs
diff --git a/git-cherry-pick.txt b/git-cherry-pick.txt
index 39ba895..f8cdbce 100644
--- a/git-cherry-pick.txt
+++ b/git-cherry-pick.txt
@@ -1,5 +1,14 @@
+--abort
+--allow-empty
+--allow-empty-message
+--continue
 --edit
+--ff
+--gpg-sign
+--keep-redundant-commits
 --mainline
 --no-commit
+--quit
 --signoff
 --strategy=
+--strategy-option
diff --git a/git-clean.txt b/git-clean.txt
index 40407f7..10c6155 100644
--- a/git-clean.txt
+++ b/git-clean.txt
@@ -1,2 +1,4 @@
 --dry-run
+--exclude
+--interactive
 --quiet
diff --git a/git-clone.txt b/git-clone.txt
index f6e892b..1b6a4da 100644
--- a/git-clone.txt
+++ b/git-clone.txt
@@ -1,18 +1,29 @@
 --bare
---branch
---depth
+--branch=
+--config
+--depth=
+--dissociate
+--ipv4
+--ipv6
+--jobs=
 --local
 --mirror
 --no-checkout
 --no-hardlinks
 --no-single-branch
 --no-tags
---origin
+--origin=
+--progress
 --quiet
 --recurse-submodules
 --reference
+--reference-if-able
+--separate-git-dir=
+--shallow-exclude
+--shallow-since=
 --shallow-submodules
 --shared
 --single-branch
 --template=
---upload-pack
+--upload-pack=
+--verbose
diff --git a/git-commit.txt b/git-commit.txt
index 2f98a59..337a57e 100644
--- a/git-commit.txt
+++ b/git-commit.txt
@@ -1,20 +1,26 @@
 --all
---allow-empty
 --amend
 --author=
+--branch
 --cleanup=
---date
+--date=
 --dry-run
 --edit
 --file=
 --fixup=
+--gpg-sign
 --include
 --interactive
---message=
+--long
+--message
 --no-edit
+--no-post-rewrite
 --no-verify
+--no-verify
+--null
 --only
 --patch
+--porcelain
 --quiet
 --reedit-message=
 --reset-author
@@ -22,8 +28,7 @@
 --short
 --signoff
 --squash=
+--status
 --template=
 --untracked-files
---untracked-files=
 --verbose
---verify
diff --git a/git-config.txt b/git-config.txt
index c4bcd1e..651fc27 100644
--- a/git-config.txt
+++ b/git-config.txt
@@ -1,15 +1,28 @@
 --add
+--blob=
+--bool
+--bool-or-int
+--edit
+--expiry-date
 --file=
 --get
 --get-all
+--get-color
+--get-colorbool
 --get-regexp
+--get-urlmatch
 --global
+--includes
+--int
 --list
 --local
 --name-only
+--null
+--path
 --remove-section
 --rename-section
 --replace-all
+--show-origin
 --system
 --unset
 --unset-all
diff --git a/git-describe.txt b/git-describe.txt
index 05eeb7a..275debb 100644
--- a/git-describe.txt
+++ b/git-describe.txt
@@ -1,4 +1,4 @@
---abbrev=
+--abbrev
 --all
 --always
 --broken
diff --git a/git-fsck.txt b/git-fsck.txt
index 9732ebf..3af9964 100644
--- a/git-fsck.txt
+++ b/git-fsck.txt
@@ -1,8 +1,12 @@
 --cache
+--connectivity-only
+--dangling
 --full
 --lost-found
 --name-objects
 --no-reflogs
+--progress
+--reflogs
 --root
 --strict
 --tags
diff --git a/git-gc.txt b/git-gc.txt
index 3b7ca6f..c08e60a 100644
--- a/git-gc.txt
+++ b/git-gc.txt
@@ -1,2 +1,3 @@
 --aggressive
 --prune
+--quiet
diff --git a/git-grep.txt b/git-grep.txt
index 6bec8f7..f7770be 100644
--- a/git-grep.txt
+++ b/git-grep.txt
@@ -1,9 +1,14 @@
+--after-context=
 --all-match
 --and
 --basic-regexp
+--before-context=
 --break
 --cached
+--color
+--context
 --count
+--exclude-standard
 --extended-regexp
 --files-with-matches
 --files-without-match
@@ -14,14 +19,17 @@
 --ignore-case
 --invert-match
 --line-number
---max-depth
+--max-depth=
 --name-only
 --no-index
 --not
 --or
 --perl-regexp
+--quiet
+--recurse-submodules
 --show-function
 --text
---threads
+--textconv
+--threads=
 --untracked
 --word-regexp
diff --git a/git-init.txt b/git-init.txt
index a7d3da6..aec6a35 100644
--- a/git-init.txt
+++ b/git-init.txt
@@ -1,5 +1,5 @@
 --bare
 --quiet
+--separate-git-dir=
 --shared
---shared=
 --template=
diff --git a/git-ls-files.txt b/git-ls-files.txt
index 04d6f08..6810eb0 100644
--- a/git-ls-files.txt
+++ b/git-ls-files.txt
@@ -1,20 +1,23 @@
 --abbrev
 --cached
+--debug
 --deleted
 --directory
+--empty-directory
+--eol
 --error-unmatch
---exclude=
---exclude-from=
---exclude-per-directory
+--exclude
+--exclude-from
 --exclude-per-directory=
 --exclude-standard
 --full-name
 --ignored
---ignored
 --killed
 --modified
 --no-empty-directory
 --others
+--recurse-submodules
+--resolve-undo
 --stage
 --unmerged
 --with-tree=
diff --git a/git-ls-remote.txt b/git-ls-remote.txt
index c58c80e..381384b 100644
--- a/git-ls-remote.txt
+++ b/git-ls-remote.txt
@@ -1,5 +1,7 @@
 --get-url
 --heads
+--quiet
 --refs
 --symref
 --tags
+--upload-pack=
diff --git a/git-mv.txt b/git-mv.txt
index c26c9e6..28ae674 100644
--- a/git-mv.txt
+++ b/git-mv.txt
@@ -1 +1,2 @@
 --dry-run
+--verbose
diff --git a/git-name-rev.txt b/git-name-rev.txt
index 7b1acde..1e5044a 100644
--- a/git-name-rev.txt
+++ b/git-name-rev.txt
@@ -1,3 +1,8 @@
 --all
+--always
+--exclude
+--name-only
+--refs
 --stdin
 --tags
+--undefined
diff --git a/git-notes_add.txt b/git-notes_add.txt
index 87cb06b..4c2dfc1 100644
--- a/git-notes_add.txt
+++ b/git-notes_add.txt
@@ -1,4 +1,5 @@
---file=
---message=
---reedit-message=
---reuse-message=
+--allow-empty
+--file
+--message
+--reedit-message
+--reuse-message
diff --git a/git-notes_append.txt b/git-notes_append.txt
index 87cb06b..4c2dfc1 100644
--- a/git-notes_append.txt
+++ b/git-notes_append.txt
@@ -1,4 +1,5 @@
---file=
---message=
---reedit-message=
---reuse-message=
+--allow-empty
+--file
+--message
+--reedit-message
+--reuse-message
diff --git a/git-notes_copy.txt b/git-notes_copy.txt
index 93aa025..d088b96 100644
--- a/git-notes_copy.txt
+++ b/git-notes_copy.txt
@@ -1 +1,2 @@
+--for-rewrite=
 --stdin
diff --git a/git-push.txt b/git-push.txt
index 1f4182f..5ea1e39 100644
--- a/git-push.txt
+++ b/git-push.txt
@@ -1,16 +1,24 @@
 --all
+--atomic
 --delete
 --dry-run
+--exec=
 --follow-tags
 --force
 --force-with-lease
---force-with-lease=
+--ipv4
+--ipv6
 --mirror
+--no-verify
+--porcelain
+--progress
 --prune
+--push-option
 --quiet
 --receive-pack=
---recurse-submodules=
+--recurse-submodules
 --repo=
 --set-upstream
+--signed
 --tags
 --verbose
diff --git a/git-remote_add.txt b/git-remote_add.txt
index aed6530..4889f31 100644
--- a/git-remote_add.txt
+++ b/git-remote_add.txt
@@ -1,6 +1,6 @@
 --fetch
---master
---mirror=
+--master=
+--mirror
 --no-tags
 --tags
 --track
diff --git a/git-replace.txt b/git-replace.txt
index 53cd1aa..d494653 100644
--- a/git-replace.txt
+++ b/git-replace.txt
@@ -3,3 +3,4 @@
 --format=
 --graft
 --list
+--raw
diff --git a/git-reset.txt b/git-reset.txt
index eafefbc..840cbae 100644
--- a/git-reset.txt
+++ b/git-reset.txt
@@ -1,6 +1,9 @@
 --hard
+--intent-to-add
 --keep
 --merge
 --mixed
 --patch
+--quiet
+--recurse-submodules
 --soft
diff --git a/git-revert.txt b/git-revert.txt
index abd5c1e..ffa0d80 100644
--- a/git-revert.txt
+++ b/git-revert.txt
@@ -1,7 +1,11 @@
+--abort
+--continue
 --edit
+--gpg-sign
 --mainline
 --no-commit
 --no-edit
+--quit
 --signoff
 --strategy=
---strategy-option=
+--strategy-option
diff --git a/git-show-branch.txt b/git-show-branch.txt
index e3c3096..37c9594 100644
--- a/git-show-branch.txt
+++ b/git-show-branch.txt
@@ -5,7 +5,7 @@
 --independent
 --list
 --merge-base
---more=
+--more
 --no-color
 --no-name
 --reflog
diff --git a/git-status.txt b/git-status.txt
index 2dce372..ed86ad5 100644
--- a/git-status.txt
+++ b/git-status.txt
@@ -1,10 +1,12 @@
 --branch
---column=
+--column
 --ignored
---ignore-submodules=
+--ignore-submodules
 --long
 --no-column
+--null
 --porcelain
 --short
---untracked-files=
+--show-stash
+--untracked-files
 --verbose
diff --git a/git-tag.txt b/git-tag.txt
index b983562..a5419db 100644
--- a/git-tag.txt
+++ b/git-tag.txt
@@ -1,18 +1,21 @@
 --annotate
---cleanup
+--cleanup=
+--color
 --column
 --contains
 --create-reflog
 --delete
---file
+--file=
 --force
+--format=
+--ignore-case
 --list
---local-user
+--local-user=
 --merged
 --message
 --no-contains
 --no-merged
 --points-at
 --sign
---sort=
+--sort
 --verify
diff --git a/git-worktree_add.txt b/git-worktree_add.txt
index 9dd7f92..3cb6a7e 100644
--- a/git-worktree_add.txt
+++ b/git-worktree_add.txt
@@ -1 +1,5 @@
+--checkout
 --detach
+--guess-remote
+--lock
+--track
diff --git a/git-worktree_lock.txt b/git-worktree_lock.txt
index a0bc06b..6ad59bb 100644
--- a/git-worktree_lock.txt
+++ b/git-worktree_lock.txt
@@ -1 +1 @@
---reason
+--reason=
-- 8< --
--
Duy
Duy Nguyen· Apr 11, 2019, 11:11 UTC · re: Duy Nguyen · lore

Re: [PATCH/RFC 0/2] Automate updating git-completion.bash a bit

Sorry mistyped 'y', please ignore.
On Thu, Apr 11, 2019 at 6:10 PM Duy Nguyen <pclouds@gmail.com> wrote:
>
>
-- 
Duy

← back to recent threads