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

Ævar Arnfjörð Bjarmason  [off-list ref] 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"));
 }
 

On Fri, Sep 10 2021, Junio C Hamano wrote:

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

quoted

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.

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

Carlo Marcelo Arenas Belón [off-list ref] writes:

That smells to show a much better direction to me ;-)

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.