Incomplete bash completion on git commit: --allow-empty-message and --allow-empty

Incomplete bash completion on git commit: --allow-empty-message and --allow-empty

[Hadi Safari]

Hi!

I'm wondering why there isn't --allow-empty and --allow-empty-message in 
completeion list of git commit command. I'm getting only following flags 
from v2.18.0 on `git commit --`:

--ahead-behind      --include           --reedit-message=
--all               --interactive       --reset-author
--amend             --long              --reuse-message=
--author=           --message=          --short
--branch            --no-edit           --signoff
--cleanup=          --no-post-rewrite   --squash=
--date=             --no-verify         --status
--dry-run           --null              --template=
--edit              --only              --untracked-files
--file=             --patch             --verbose
--fixup=            --porcelain         --verify
--gpg-sign          --quiet

Besides, is there any way to allow empty commit message for a repo, e.g. 
by adding something to `.git/config`? I couldn't find any in docs.

-- 
Hadi

Re: Incomplete bash completion on git commit: --allow-empty-message and --allow-empty

[Duy Nguyen]

On Thu, Aug 16, 2018 at 3:50 PM Hadi Safari <hadi@hadisafari.ir> wrote:
>
> Hi!
>
> I'm wondering why there isn't --allow-empty and --allow-empty-message in
> completeion list of git commit command.

This is because they are marked "hidden" in the code. If you do "git
commit -h", they will not show up either. Ævar provided the reason for
hiding them in c9b5fde759 (Add option to git-commit to allow empty log
messages - 2010-04-06), basically "not for normal use" so it makes
sense to not complete them. If they are used often on command line
now, then of course we need to reconsider to stop hiding them.
-- 
Duy

[PATCH] completion: include PARSE_OPT_HIDDEN in completion output

[Ævar Arnfjörð Bjarmason]

The PARSE_OPT_HIDDEN is, per the documentation of the "option" struct
in option parse-options.h, only supposed to affect -h output, not
completion. That's what the PARSE_OPT_NOCOMPLETE flag is supposed to
be for.

Since 2e29dca66a ("completion: use __gitcomp_builtin in _git_commit",
2018-02-09) we've been using e.g. "git commit --git-completion-helper"
to get the bash completion for the git-commit command. Due to
PARSE_OPT_HIDDEN this excluded e.g. --allow-empty and
--allow-empty-message.

Now, this wasn't a behavior change in that commit. Before that we had
a hardcoded list of options, removed in 2e29dca66a ("completion: use
__gitcomp_builtin in _git_commit", 2018-02-09). This list didn't
contain those two options.

But as noted in the follow-up discussion to c9b5fde759 ("Add option to
git-commit to allow empty log messages", 2010-04-06) in
https://public-inbox.org/git/20100406055530.GE3901@coredump.intra.peff.net/
the motivation for PARSE_OPT_HIDDEN was to keep the "git commit -h"
output to a minimum, not to hide it from completion.

I think it makes sense to exclude options like these from -h output,
but for the completion the user is usually not trying to complete "git
commit --<TAB>", but e.g. "git commit --allo<TAB>", and because of
this behavior we don't show these options at all there.

However, manually going over:

    git grep -e OPT_HIDDEN_BOOL -e PARSE_OPT_HIDDEN

Shows many options we don't want to show in completion either,
e.g. "git am --binary" or "git branch -l". Many of these are internal,
deprecated, or no-ops. There's also things like "git difftool
--prompt" (the default behavior) which are arguably pointless to add,
we just have "--no-prompt" to inverse the default.

The options we'll now show on completion, that we didn't show before,
are:

OPT_HIDDEN_BOOL:

 * checkout: --guess (no idea how this works, not documented, but it's
   not deprecated and is there..)
 * commit: --allow-empty and --allow-empty-message
 * help: --exclude-guides (because why not?)
 * receive-pack: All three (non --quiet) options it supports. It
   doesn't have any completion now, but if we ever add it why not
   complete these?

PARSE_OPT_HIDDEN (without PARSE_OPT_NOCOMPLETE):

 * fetch: --recurse-submodules-default (a legitimate documented
   option, but perhaps we should blacklist this because it's rarely
   used and interferes with --recurse-submodules?).
 * ls-remote: --exec (as with "fetch" this is a synonym for
   --upload-pack, but unlike "fetch" it wasn't documented. Document it
   while I'm at it).

I don't know if that "o->flags |= PARSE_OPT_HIDDEN" line in
cmd_parseopt() in builtin/rev-parse.c should also be setting
PARSE_OPT_NOCOMPLETE.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Thu, Aug 16, 2018 at 9:46 AM, Hadi Safari <hadi@hadisafari.ir> wrote:
>
> Hi!
>
> I'm wondering why there isn't --allow-empty and
> --allow-empty-message in completeion list of git commit command. I'm
> getting only following flags from v2.18.0 on `git commit --`:

It's because we've been conflating the desire to include something in
"git <cmd> -h" output, and having "git <cmd> --some-rare-option" work
or not. Here's an attempt to fix it.

 Documentation/git-ls-remote.txt | 3 +++
 archive.c                       | 3 ++-
 builtin/add.c                   | 5 +++--
 builtin/am.c                    | 8 ++++----
 builtin/branch.c                | 5 +++--
 builtin/clone.c                 | 8 ++++----
 builtin/difftool.c              | 4 ++--
 builtin/fetch.c                 | 3 ++-
 builtin/fmt-merge-msg.c         | 3 ++-
 builtin/grep.c                  | 2 +-
 builtin/help.c                  | 2 +-
 builtin/name-rev.c              | 3 ++-
 builtin/pull.c                  | 2 +-
 builtin/show-ref.c              | 4 ++--
 builtin/write-tree.c            | 4 ++--
 parse-options.c                 | 4 ++--
 parse-options.h                 | 3 +++
 t/t9902-completion.sh           | 1 +
 18 files changed, 40 insertions(+), 27 deletions(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index b9fd3770a6..39192f4e2a 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -43,6 +43,9 @@ OPTIONS
 	SSH and where the SSH daemon does not use the PATH configured by the
 	user.
 
+--exec=<git-upload-pack>::
+	Same as --upload-pack=<git-upload-pack>.
+
 --exit-code::
 	Exit with status "2" when no matching refs are found in the remote
 	repository. Usually the command exits with status "0" to indicate
diff --git a/archive.c b/archive.c
index 78b0a398a0..d5e02127a1 100644
--- a/archive.c
+++ b/archive.c
@@ -414,7 +414,8 @@ static void parse_treeish_arg(const char **argv,
 #define OPT__COMPR(s, v, h, p) \
 	OPT_SET_INT_F(s, NULL, v, h, p, PARSE_OPT_NONEG)
 #define OPT__COMPR_HIDDEN(s, v, p) \
-	OPT_SET_INT_F(s, NULL, v, "", p, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN)
+	OPT_SET_INT_F(s, NULL, v, "", p, \
+	PARSE_OPT_NONEG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE)
 
 static int parse_archive_args(int argc, const char **argv,
 		const struct archiver **ar, struct archiver_args *args,
diff --git a/builtin/add.c b/builtin/add.c
index 8a155dd41e..f283eda6d8 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -305,8 +305,9 @@ static struct option builtin_add_options[] = {
 	OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")),
 	OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")),
 	OPT_STRING( 0 , "chmod", &chmod_arg, N_("(+/-)x"), N_("override the executable bit of the listed files")),
-	OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo,
-			N_("warn when adding an embedded repository")),
+	OPT_HIDDEN_NOCOMPLETE_BOOL(0, "warn-embedded-repo",
+				   &warn_on_embedded_repo,
+				   N_("warn when adding an embedded repository")),
 	OPT_END(),
 };
 
diff --git a/builtin/am.c b/builtin/am.c
index 2c19e69f58..d5353c5954 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2215,8 +2215,8 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOL('i', "interactive", &state.interactive,
 			N_("run interactively")),
-		OPT_HIDDEN_BOOL('b', "binary", &binary,
-			N_("historical option -- no-op")),
+		OPT_HIDDEN_NOCOMPLETE_BOOL('b', "binary", &binary,
+					   N_("historical option -- no-op")),
 		OPT_BOOL('3', "3way", &state.threeway,
 			N_("allow fall back on 3way merging if needed")),
 		OPT__QUIET(&state.quiet, N_("be quiet")),
@@ -2298,8 +2298,8 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 		{ OPTION_STRING, 'S', "gpg-sign", &state.sign_commit, N_("key-id"),
 		  N_("GPG-sign commits"),
 		  PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
-		OPT_HIDDEN_BOOL(0, "rebasing", &state.rebasing,
-			N_("(internal use for git-rebase)")),
+		OPT_HIDDEN_NOCOMPLETE_BOOL(0, "rebasing", &state.rebasing,
+					   N_("(internal use for git-rebase)")),
 		OPT_END()
 	};
 
diff --git a/builtin/branch.c b/builtin/branch.c
index 4fc55c3508..366ffb175d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -598,7 +598,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT('t', "track",  &track, N_("set up tracking mode (see git-pull(1))"),
 			BRANCH_TRACK_EXPLICIT),
 		OPT_SET_INT_F(0, "set-upstream", &track, N_("do not use"),
-			BRANCH_TRACK_OVERRIDE, PARSE_OPT_HIDDEN),
+			      BRANCH_TRACK_OVERRIDE,
+			      PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE),
 		OPT_STRING('u', "set-upstream-to", &new_upstream, N_("upstream"), N_("change the upstream info")),
 		OPT_BOOL(0, "unset-upstream", &unset_upstream, N_("Unset the upstream info")),
 		OPT__COLOR(&branch_use_color, N_("use colored output")),
@@ -624,7 +625,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		{
 			OPTION_CALLBACK, 'l', NULL, &reflog, NULL,
 			N_("deprecated synonym for --create-reflog"),
-			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
+			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,
 			deprecated_reflog_option_cb
 		},
 		OPT_BOOL(0, "edit-description", &edit_description,
diff --git a/builtin/clone.c b/builtin/clone.c
index fd2c3ef090..27d2a1a459 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -87,8 +87,8 @@ static struct option builtin_clone_options[] = {
 	OPT_BOOL('n', "no-checkout", &option_no_checkout,
 		 N_("don't create a checkout")),
 	OPT_BOOL(0, "bare", &option_bare, N_("create a bare repository")),
-	OPT_HIDDEN_BOOL(0, "naked", &option_bare,
-			N_("create a bare repository")),
+	OPT_HIDDEN_NOCOMPLETE_BOOL(0, "naked", &option_bare,
+				   N_("create a bare repository")),
 	OPT_BOOL(0, "mirror", &option_mirror,
 		 N_("create a mirror repository (implies bare)")),
 	OPT_BOOL('l', "local", &option_local,
@@ -99,8 +99,8 @@ static struct option builtin_clone_options[] = {
 		    N_("setup as shared repository")),
 	{ OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules,
 	  N_("pathspec"), N_("initialize submodules in the clone"),
-	  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,
-	  (intptr_t)"." },
+	  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,
+	  recurse_submodules_cb, (intptr_t)"." },
 	{ OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,
 	  N_("pathspec"), N_("initialize submodules in the clone"),
 	  PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 3018e61d04..f63e866dd7 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -699,8 +699,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT_F('y', "no-prompt", &prompt,
 			N_("do not prompt before launching a diff tool"),
 			0, PARSE_OPT_NONEG),
-		OPT_SET_INT_F(0, "prompt", &prompt, NULL,
-			1, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),
+		OPT_SET_INT_F(0, "prompt", &prompt, NULL, 1,
+			      PARSE_OPT_NONEG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE),
 		OPT_BOOL(0, "symlinks", &symlinks,
 			 N_("use symlinks in dir-diff mode")),
 		OPT_STRING('t', "tool", &difftool_cmd, N_("<tool>"),
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 61bec5d213..3f11d743ee 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -153,7 +153,8 @@ static struct option builtin_fetch_options[] = {
 		   &recurse_submodules_default, N_("on-demand"),
 		   N_("default for recursive fetching of submodules "
 		      "(lower priority than config files)"),
-		   PARSE_OPT_HIDDEN, option_fetch_parse_recurse_submodules },
+		   PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,
+		   option_fetch_parse_recurse_submodules },
 	OPT_BOOL(0, "update-shallow", &update_shallow,
 		 N_("accept refs that update .git/shallow")),
 	{ OPTION_CALLBACK, 0, "refmap", NULL, N_("refmap"),
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index f35ff1612b..ba0a8426a0 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -672,7 +672,8 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 		  PARSE_OPT_OPTARG, NULL, DEFAULT_MERGE_LOG_LEN },
 		{ OPTION_INTEGER, 0, "summary", &shortlog_len, N_("n"),
 		  N_("alias for --log (deprecated)"),
-		  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, NULL,
+		  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,
+		  NULL,
 		  DEFAULT_MERGE_LOG_LEN },
 		OPT_STRING('m', "message", &message, N_("text"),
 			N_("use <text> as start of message")),
diff --git a/builtin/grep.c b/builtin/grep.c
index ee5a1bd355..2ecd941101 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -892,7 +892,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			N_("show only matches from files that match all patterns")),
 		OPT_SET_INT_F(0, "debug", &opt.debug,
 			      N_("show parse tree for grep expression"),
-			      1, PARSE_OPT_HIDDEN),
+			      1, PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE),
 		OPT_GROUP(""),
 		{ OPTION_STRING, 'O', "open-files-in-pager", &show_in_pager,
 			N_("pager"), N_("show matching files in the pager"),
diff --git a/builtin/help.c b/builtin/help.c
index 8d4f6dd301..90c7a85cba 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -47,7 +47,7 @@ static struct option builtin_help_options[] = {
 	OPT_HIDDEN_BOOL(0, "exclude-guides", &exclude_guides, N_("exclude guides")),
 	OPT_BOOL('g', "guides", &show_guides, N_("print list of useful guides")),
 	OPT_BOOL('c', "config", &show_config, N_("print all configuration variable names")),
-	OPT_SET_INT_F(0, "config-for-completion", &show_config, "", 2, PARSE_OPT_HIDDEN),
+	OPT_SET_INT_F(0, "config-for-completion", &show_config, "", 2, PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE),
 	OPT_SET_INT('m', "man", &help_format, N_("show man page"), HELP_FORMAT_MAN),
 	OPT_SET_INT('w', "web", &help_format, N_("show manual in web browser"),
 			HELP_FORMAT_WEB),
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index f1cb45c227..0a63d23761 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -426,7 +426,8 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 			/* A Hidden OPT_BOOL */
 			OPTION_SET_INT, 0, "peel-tag", &peel_tag, NULL,
 			N_("dereference tags in the input (internal use)"),
-			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1,
+			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,
+			NULL, 1,
 		},
 		OPT_END(),
 	};
diff --git a/builtin/pull.c b/builtin/pull.c
index 4e78935392..cdc67cd17f 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -146,7 +146,7 @@ static struct option pull_options[] = {
 		PARSE_OPT_NOARG),
 	OPT_PASSTHRU(0, "summary", &opt_diffstat, NULL,
 		N_("(synonym to --stat)"),
-		PARSE_OPT_NOARG | PARSE_OPT_HIDDEN),
+		PARSE_OPT_NOARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE),
 	OPT_PASSTHRU(0, "log", &opt_log, N_("n"),
 		N_("add (at most <n>) entries from shortlog to merge commit message"),
 		PARSE_OPT_OPTARG),
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 2f13f1316f..afd916fa06 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -161,8 +161,8 @@ static const struct option show_ref_options[] = {
 	OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")),
 	OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
 		    "requires exact ref path")),
-	OPT_HIDDEN_BOOL('h', NULL, &show_head,
-			N_("show the HEAD reference, even if it would be filtered out")),
+	OPT_HIDDEN_NOCOMPLETE_BOOL('h', NULL, &show_head,
+				   N_("show the HEAD reference, even if it would be filtered out")),
 	OPT_BOOL(0, "head", &show_head,
 	  N_("show the HEAD reference, even if it would be filtered out")),
 	OPT_BOOL('d', "dereference", &deref_tags,
diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index c9d3c544e7..489a5e46b0 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -29,8 +29,8 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 		  PARSE_OPT_LITERAL_ARGHELP },
 		{ OPTION_BIT, 0, "ignore-cache-tree", &flags, NULL,
 		  N_("only useful for debugging"),
-		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, NULL,
-		  WRITE_TREE_IGNORE_CACHE_TREE },
+		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG | PARSE_OPT_NOCOMPLETE,
+		  NULL, WRITE_TREE_IGNORE_CACHE_TREE },
 		OPT_END()
 	};
 
diff --git a/parse-options.c b/parse-options.c
index 7db84227ab..a9c82e518a 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -437,7 +437,7 @@ static void show_negated_gitcomp(const struct option *opts, int nr_noopts)
 
 		if (!opts->long_name)
 			continue;
-		if (opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE))
+		if (opts->flags & PARSE_OPT_NOCOMPLETE)
 			continue;
 		if (opts->flags & PARSE_OPT_NONEG)
 			continue;
@@ -485,7 +485,7 @@ static int show_gitcomp(struct parse_opt_ctx_t *ctx,
 
 		if (!opts->long_name)
 			continue;
-		if (opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE))
+		if (opts->flags & PARSE_OPT_NOCOMPLETE)
 			continue;
 
 		switch (opts->type) {
diff --git a/parse-options.h b/parse-options.h
index dd14911a29..5f78e2e164 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -139,6 +139,9 @@ struct option {
 #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_HIDDEN_NOCOMPLETE_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
+						 (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE, \
+						NULL, 1}
 #define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }
 #define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), N_("n"), (h) }
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 5ff43b9cbb..2e0b9c78e7 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1423,6 +1423,7 @@ test_expect_success 'double dash "git checkout"' '
 	test_completion "git checkout --" <<-\EOF
 	--quiet Z
 	--detach Z
+	--guess Z
 	--track Z
 	--orphan=Z
 	--ours Z
-- 
2.18.0.865.gffc8e1a3cd6

Re: [PATCH] completion: include PARSE_OPT_HIDDEN in completion output

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> However, manually going over:
>
>     git grep -e OPT_HIDDEN_BOOL -e PARSE_OPT_HIDDEN
>
> Shows many options we don't want to show in completion either,
> e.g. "git am --binary" or "git branch -l". Many of these are internal,
> deprecated, or no-ops. There's also things like "git difftool
> --prompt" (the default behavior) which are arguably pointless to add,
> we just have "--no-prompt" to inverse the default.

Yeah, and I believe some hidden ones are hidden because they
encourage bad workflows (like --allow-empty-message) especially when
used interactively, and they aren't marked with nocomplete only
because there wasn't any such bit back when they were marked hidden.

In any case, those that are hidden for such a reason now need to be
marked with both hidden and nocomplete, which is a small one-time
price to pay to make the meaning of these two bits saner.  So I
quite like the direction in which this patch is taking the
underlying mechanism.

A "blind" translation that is far safer than your patch may first

 * update the code so that ones with hidden-bit are completed

 * update the data so that ones currently have hidden bit but not
   nocomplete bit gain nocomplete bit as well.

That would give us a saner mechanism without changing the behaviour.

And then we can make policy decisions for each option separately for
the merit of keeping it hidden (i.e. excluding from short help to
unclutter) and/or keeping it not completed (i.e. discouraging its
use in an interactive session).

As I think some of the hidden ones also have nocomplete and others
do not have nocomplete merely by historical accident, the way this
patch changes behaviour for some options (namely, the hidden ones
that did not have nocomplete not because they wanted to be completed
but because there wasn't such an option to exclude them from
completion previously and because it was sufficient to mark them as
hidden to exclude them from completion) means making policy
decisions while updating the mechanism that allows us to express our
policy decisions.  I do not think we should conflate the two in the
same patch.

Re: [PATCH] completion: include PARSE_OPT_HIDDEN in completion output

[Duy Nguyen]

On Thu, Aug 16, 2018 at 8:42 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> The PARSE_OPT_HIDDEN is, per the documentation of the "option" struct
> in option parse-options.h, only supposed to affect -h output, not
> completion. That's what the PARSE_OPT_NOCOMPLETE flag is supposed to
> be for.
>
> Since 2e29dca66a ("completion: use __gitcomp_builtin in _git_commit",
> 2018-02-09) we've been using e.g. "git commit --git-completion-helper"
> to get the bash completion for the git-commit command. Due to
> PARSE_OPT_HIDDEN this excluded e.g. --allow-empty and
> --allow-empty-message.
>
> Now, this wasn't a behavior change in that commit. Before that we had
> a hardcoded list of options, removed in 2e29dca66a ("completion: use
> __gitcomp_builtin in _git_commit", 2018-02-09). This list didn't
> contain those two options.
>
> But as noted in the follow-up discussion to c9b5fde759 ("Add option to
> git-commit to allow empty log messages", 2010-04-06) in
> https://public-inbox.org/git/20100406055530.GE3901@coredump.intra.peff.net/
> the motivation for PARSE_OPT_HIDDEN was to keep the "git commit -h"
> output to a minimum, not to hide it from completion.
>
> I think it makes sense to exclude options like these from -h output,
> but for the completion the user is usually not trying to complete "git
> commit --<TAB>",

Actually I do :)

> but e.g. "git commit --allo<TAB>", and because of this behavior we don't show these options at all there.

And it would be great if these options do not show up at --<tab> but
do when with --a<tab>.

We already do something similar to this with --no-<tab>. So this could
be another option.
-- 
Duy