diff mbox series

[v2,2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins

Message ID patch-v2-2.6-d6c44402715-20210910T153147Z-avarab@gmail.com (mailing list archive)
State Superseded
Headers show
Series parse-options: properly align continued usage output & related | expand

Commit Message

Ævar Arnfjörð Bjarmason Sept. 10, 2021, 3:38 p.m. UTC
Change the implementation of b5dd96b70ac (make credential helpers
builtins, 2020-08-13) to declare in the "struct cmd_struct" that
NO_UNIX_SOCKETS can't be set.

This is one of two in-tree users for the empty lines in
parse_options() usage, getting rid of that is the main motivation for
this, but it also doesn't make sense to emit these sorts of usage
messages just to appease t0012-help.sh, which seemingly b5dd96b70ac
aimed to do.

I.e. these commands don't support "[options]", or "<action>" so
emitting that at the beginning is incorrect. We should just die right
away.

The existing code also had the edge case of not emitting the die()
message if a "-h" argument was given, since parse_options() will
handle the exit() itself in that case. We could feed it
PARSE_OPT_NO_INTERNAL_HELP, but this is better.

By moving this up to the "struct cmd_struct" we can also skip these in
--list-cmds=builtins instead, as noted above we shouldn't be exiting
with code 129 in these cases.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin.h                          |  6 ++++++
 builtin/credential-cache--daemon.c | 11 +----------
 builtin/credential-cache.c         | 11 +----------
 git.c                              | 15 ++++++++++++---
 t/t0012-help.sh                    | 10 ++++++++++
 5 files changed, 30 insertions(+), 23 deletions(-)

Comments

Junio C Hamano Sept. 11, 2021, 12:14 a.m. UTC | #1
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the implementation of b5dd96b70ac (make credential helpers
> builtins, 2020-08-13) to declare in the "struct cmd_struct" that
> NO_UNIX_SOCKETS can't be set.

It may happen to be that two credential-cache program are both
related to the same CPP macro NO_UNIX_SOCKETS, but I think the
pattern you are tackling with this topic is that a fallback
definition of a function that is designed to always die when invoked
misuses the parse-options API.  I do not want to see you invent a
new bit in cmd_struct for each and every conditional that lets us
define such a die-only fallback implementation.

I may be missing something obvious, but can't the following suffice,
and if not, why?

Thanks.

 builtin/credential-cache--daemon.c | 9 ---------
 builtin/credential-cache.c         | 9 ---------
 2 files changed, 18 deletions(-)

diff --git i/builtin/credential-cache--daemon.c w/builtin/credential-cache--daemon.c
index 4c6c89ab0d..f11a89a89b 100644
--- i/builtin/credential-cache--daemon.c
+++ w/builtin/credential-cache--daemon.c
@@ -304,15 +304,6 @@ int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)
 
 int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)
 {
-	const char * const usage[] = {
-		"git credential-cache--daemon [options] <action>",
-		"",
-		"credential-cache--daemon is disabled in this build of Git",
-		NULL
-	};
-	struct option options[] = { OPT_END() };
-
-	argc = parse_options(argc, argv, prefix, options, usage, 0);
 	die(_("credential-cache--daemon unavailable; no unix socket support"));
 }
 
diff --git i/builtin/credential-cache.c w/builtin/credential-cache.c
index e8a7415747..dd794f84ce 100644
--- i/builtin/credential-cache.c
+++ w/builtin/credential-cache.c
@@ -142,15 +142,6 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 
 int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 {
-	const char * const usage[] = {
-		"git credential-cache [options] <action>",
-		"",
-		"credential-cache is disabled in this build of Git",
-		NULL
-	};
-	struct option options[] = { OPT_END() };
-
-	argc = parse_options(argc, argv, prefix, options, usage, 0);
 	die(_("credential-cache unavailable; no unix socket support"));
 }
Ævar Arnfjörð Bjarmason Sept. 11, 2021, 2:50 a.m. UTC | #2
On Fri, Sep 10 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change the implementation of b5dd96b70ac (make credential helpers
>> builtins, 2020-08-13) to declare in the "struct cmd_struct" that
>> NO_UNIX_SOCKETS can't be set.
>
> It may happen to be that two credential-cache program are both
> related to the same CPP macro NO_UNIX_SOCKETS, but I think the
> pattern you are tackling with this topic is that a fallback
> definition of a function that is designed to always die when invoked
> misuses the parse-options API.  I do not want to see you invent a
> new bit in cmd_struct for each and every conditional that lets us
> define such a die-only fallback implementation.
>
> I may be missing something obvious, but can't the following suffice,
> and if not, why?

I think this is covered if you go on to read the rest of the commit
message, i.e. yes we could, but the trade-off is making the test and
users that might want to use --list-cmds=builtins hardcode these two as
special cases under the relevant prereq.

Hence doing this in the main git.c dispatch mechanism, if we can't ever
do anything useful with these it seems best to mark them as such at
compile-time in that dispatch mechanism.
Carlo Marcelo Arenas Belón Sept. 11, 2021, 3:47 a.m. UTC | #3
Sorry, if this is silly, but why is this not better (at least as a starting
point, since it obviously will need more work?

Undefined symbols for architecture x86_64:
  "_cmd_credential_cache", referenced from:
      _commands in git.o
  "_cmd_credential_cache_daemon", referenced from:
      _commands in git.o

Carlo
---- >8 ----
diff --git a/Makefile b/Makefile
index 44734f916a..2a60685c37 100644
--- a/Makefile
+++ b/Makefile
@@ -1098,8 +1098,10 @@ BUILTIN_OBJS += builtin/commit-tree.o
 BUILTIN_OBJS += builtin/commit.o
 BUILTIN_OBJS += builtin/config.o
 BUILTIN_OBJS += builtin/count-objects.o
+ifndef NO_UNIX_SOCKETS
 BUILTIN_OBJS += builtin/credential-cache--daemon.o
 BUILTIN_OBJS += builtin/credential-cache.o
+endif
 BUILTIN_OBJS += builtin/credential-store.o
 BUILTIN_OBJS += builtin/credential.o
 BUILTIN_OBJS += builtin/describe.o
Junio C Hamano Sept. 11, 2021, 6:12 a.m. UTC | #4
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> Sorry, if this is silly, but why is this not better (at least as a starting
> point, since it obviously will need more work?
>
> Undefined symbols for architecture x86_64:
>   "_cmd_credential_cache", referenced from:
>       _commands in git.o
>   "_cmd_credential_cache_daemon", referenced from:
>       _commands in git.o
>
> Carlo
> ---- >8 ----
> diff --git a/Makefile b/Makefile
> index 44734f916a..2a60685c37 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1098,8 +1098,10 @@ BUILTIN_OBJS += builtin/commit-tree.o
>  BUILTIN_OBJS += builtin/commit.o
>  BUILTIN_OBJS += builtin/config.o
>  BUILTIN_OBJS += builtin/count-objects.o
> +ifndef NO_UNIX_SOCKETS
>  BUILTIN_OBJS += builtin/credential-cache--daemon.o
>  BUILTIN_OBJS += builtin/credential-cache.o
> +endif
>  BUILTIN_OBJS += builtin/credential-store.o
>  BUILTIN_OBJS += builtin/credential.o
>  BUILTIN_OBJS += builtin/describe.o

That smells to show a much better direction to me ;-)
Ævar Arnfjörð Bjarmason Sept. 11, 2021, 7:16 a.m. UTC | #5
On Fri, Sep 10 2021, Carlo Marcelo Arenas Belón wrote:

> Sorry, if this is silly, but why is this not better (at least as a starting
> point, since it obviously will need more work?
>
> Undefined symbols for architecture x86_64:
>   "_cmd_credential_cache", referenced from:
>       _commands in git.o
>   "_cmd_credential_cache_daemon", referenced from:
>       _commands in git.o

I'm fine with it. I'm just trying to get to the end-goal of fixing the
formatting issue in "-h" output in the simplest way possible.

What you're suggesting would be effectively a revert of various parts of
b5dd96b70ac (make credential helpers builtins, 2020-08-13), which is
behavior I assumed we'd like to retain.

But yes, if changing that behavior is OK then this is simpler.

Anyway, all of this in v2 was in response to feedback on v1 to make the
v1 function in parse_options() easier, see summary at
https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/

But it does look like Junio would like to keep the only "real" in-tree
user of the current API in builtin/blame.c, so at that point removing
these by any method becomes a moot point, so I think I'l try some
alternate approach based on v1 that doesn't touch these at all again.
diff mbox series

Patch

diff --git a/builtin.h b/builtin.h
index 16ecd5586f0..66713da6a02 100644
--- a/builtin.h
+++ b/builtin.h
@@ -63,6 +63,12 @@ 
  *	more informed decision, e.g., by ignoring `pager.<cmd>` for
  *	certain subcommands.
  *
+ * `NEED_UNIX_SOCKETS`:
+ *
+ *	This built-in will not work if NO_UNIX_SOCKETS is defined. It
+ *	will be recognized for emitting error messages, but won't be
+ *	listed in --list-cmds=builtins.
+ *
  * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
  *
  * Additionally, if `foo` is a new command, there are 4 more things to do:
diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index 4c6c89ab0de..d9863287a4d 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -304,16 +304,7 @@  int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)
 
 int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)
 {
-	const char * const usage[] = {
-		"git credential-cache--daemon [options] <action>",
-		"",
-		"credential-cache--daemon is disabled in this build of Git",
-		NULL
-	};
-	struct option options[] = { OPT_END() };
-
-	argc = parse_options(argc, argv, prefix, options, usage, 0);
-	die(_("credential-cache--daemon unavailable; no unix socket support"));
+	BUG("should not be called under NO_UNIX_SOCKETS");
 }
 
 #endif /* NO_UNIX_SOCKET */
diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
index e8a74157471..22b49b265bf 100644
--- a/builtin/credential-cache.c
+++ b/builtin/credential-cache.c
@@ -142,16 +142,7 @@  int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 
 int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 {
-	const char * const usage[] = {
-		"git credential-cache [options] <action>",
-		"",
-		"credential-cache is disabled in this build of Git",
-		NULL
-	};
-	struct option options[] = { OPT_END() };
-
-	argc = parse_options(argc, argv, prefix, options, usage, 0);
-	die(_("credential-cache unavailable; no unix socket support"));
+	BUG("should not be called under NO_UNIX_SOCKETS");
 }
 
 #endif /* NO_UNIX_SOCKETS */
diff --git a/git.c b/git.c
index 18bed9a9964..6b0248841db 100644
--- a/git.c
+++ b/git.c
@@ -17,6 +17,7 @@ 
 #define SUPPORT_SUPER_PREFIX	(1<<4)
 #define DELAY_PAGER_CONFIG	(1<<5)
 #define NO_PARSEOPT		(1<<6) /* parse-options is not used */
+#define NEED_UNIX_SOCKETS	(1<<7) /* Works unless -DNO_UNIX_SOCKETS */
 
 struct cmd_struct {
 	const char *cmd;
@@ -66,6 +67,10 @@  static int list_cmds(const char *spec)
 	struct string_list list = STRING_LIST_INIT_DUP;
 	int i;
 	int nongit;
+	unsigned int exclude_option = 0;
+#ifdef NO_UNIX_SOCKETS
+	exclude_option |= NEED_UNIX_SOCKETS;
+#endif
 
 	/*
 	* Set up the repository so we can pick up any repo-level config (like
@@ -78,7 +83,7 @@  static int list_cmds(const char *spec)
 		int len = sep - spec;
 
 		if (match_token(spec, len, "builtins"))
-			list_builtins(&list, 0);
+			list_builtins(&list, exclude_option);
 		else if (match_token(spec, len, "main"))
 			list_all_main_cmds(&list);
 		else if (match_token(spec, len, "others"))
@@ -423,6 +428,10 @@  static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	const char *prefix;
 
 	prefix = NULL;
+#ifdef NO_UNIX_SOCKETS
+	if (p->option & NEED_UNIX_SOCKETS)
+		die(_("%s is unavailable; there is no UNIX socket support in this build of Git"), p->cmd);
+#endif
 	help = argc == 2 && !strcmp(argv[1], "-h");
 	if (!help) {
 		if (p->option & RUN_SETUP)
@@ -513,8 +522,8 @@  static struct cmd_struct commands[] = {
 	{ "config", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },
 	{ "count-objects", cmd_count_objects, RUN_SETUP },
 	{ "credential", cmd_credential, RUN_SETUP_GENTLY | NO_PARSEOPT },
-	{ "credential-cache", cmd_credential_cache },
-	{ "credential-cache--daemon", cmd_credential_cache_daemon },
+	{ "credential-cache", cmd_credential_cache, NEED_UNIX_SOCKETS },
+	{ "credential-cache--daemon", cmd_credential_cache_daemon, NEED_UNIX_SOCKETS },
 	{ "credential-store", cmd_credential_store },
 	{ "describe", cmd_describe, RUN_SETUP },
 	{ "diff", cmd_diff, NO_PARSEOPT },
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 5679e29c624..2d05dde4b90 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -85,4 +85,14 @@  do
 	'
 done <builtins
 
+test_expect_success UNIX_SOCKETS 'builtin list includes NEED_UNIX_SOCKETS under UNIX_SOCKETS' '
+	grep ^credential-cache$ builtins &&
+	grep ^credential-cache--daemon$ builtins
+'
+
+test_expect_success !UNIX_SOCKETS 'builtin list excludes NEED_UNIX_SOCKETS under !UNIX_SOCKETS' '
+	! grep ^credential-cache$ builtins &&
+	! grep ^credential-cache--daemon$ builtins
+'
+
 test_done