git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/5] make credential helpers builtins

From
Taylor Blau <me@ttaylorr.com>
Date
Aug 13, 2020, 15:08 UTC
Message-ID
<20200813150839.GB2244@syl.lan>
In-Reply-To
<20200813145855.GB891370@coredump.intra.peff.net>
On Thu, Aug 13, 2020 at 10:58:55AM -0400, Jeff King wrote:
Show 5 quoted lines
> There's no real reason for credential helpers to be separate binaries. I
> did them this way originally under the notion that helper don't _need_
> to be part of Git, and so can be built totally separately (and indeed,
> the ones in contrib/credential are). But the ones in our main Makefile
> build on libgit.a, and the resulting binaries are reasonably large.

Could you clarify which helpers you mean here? Git's own credential-cache and store make sense to convert, but the helpers in contrib definitely don't.

For what it's worth, I'm almost positive that you mean the in-tree helpers (where in-tree means "in git.git but not in contrib"), in which case I'm in favor of this direcftion.

Show 6 quoted lines
> We can slim down our total disk footprint by just making them builtins.
> This reduces the size of:
>
>   make strip install
>
> from 29MB to 24MB on my Debian system.
Great.
Show 16 quoted lines
> Note that credential-cache can't operate without support for Unix
> sockets. Currently we just don't build it at all when NO_UNIX_SOCKETS is
> set. We could continue that with conditionals in the Makefile and our
> list of builtins. But instead, let's build a dummy implementation that
> dies with an informative message. That has two advantages:
>
>   - it's simpler, because the conditional bits are all kept inside
>     the credential-cache source
>
>   - a user who is expecting it to exist will be told _why_ they can't
>     use it, rather than getting the "credential-cache is not a git
>     command" error which makes it look like the Git install is broken.
>
> Note that our dummy implementation does still respond to "-h" in order
> to appease t0012 (and this may be a little friendlier for users, as
> well).
Yep, that makes sense and I think this approach seems fine.
Show 287 quoted lines
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Makefile                                      |  8 ++---
>  builtin.h                                     |  3 ++
>  .../credential-cache--daemon.c                | 29 ++++++++++++++++---
>  .../credential-cache.c                        | 29 ++++++++++++++++---
>  .../credential-store.c                        |  6 ++--
>  contrib/buildsystems/CMakeLists.txt           | 20 +------------
>  git.c                                         |  3 ++
>  7 files changed, 63 insertions(+), 35 deletions(-)
>  rename credential-cache--daemon.c => builtin/credential-cache--daemon.c (91%)
>  rename credential-cache.c => builtin/credential-cache.c (83%)
>  rename credential-store.c => builtin/credential-store.c (96%)
>
> diff --git a/Makefile b/Makefile
> index 271b96348e..5b43c0fafb 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -672,7 +672,6 @@ EXTRA_PROGRAMS =
>  PROGRAMS += $(EXTRA_PROGRAMS)
>
>  PROGRAM_OBJS += bugreport.o
> -PROGRAM_OBJS += credential-store.o
>  PROGRAM_OBJS += daemon.o
>  PROGRAM_OBJS += fast-import.o
>  PROGRAM_OBJS += http-backend.o
> @@ -1052,6 +1051,9 @@ BUILTIN_OBJS += builtin/checkout-index.o
>  BUILTIN_OBJS += builtin/checkout.o
>  BUILTIN_OBJS += builtin/clean.o
>  BUILTIN_OBJS += builtin/clone.o
> +BUILTIN_OBJS += builtin/credential-cache.o
> +BUILTIN_OBJS += builtin/credential-cache--daemon.o
> +BUILTIN_OBJS += builtin/credential-store.o
>  BUILTIN_OBJS += builtin/column.o
>  BUILTIN_OBJS += builtin/commit-graph.o
>  BUILTIN_OBJS += builtin/commit-tree.o
> @@ -1634,11 +1636,8 @@ ifdef NO_INET_PTON
>  endif
>  ifdef NO_UNIX_SOCKETS
>  	BASIC_CFLAGS += -DNO_UNIX_SOCKETS
> -	EXCLUDED_PROGRAMS += git-credential-cache git-credential-cache--daemon
>  else
>  	LIB_OBJS += unix-socket.o
> -	PROGRAM_OBJS += credential-cache.o
> -	PROGRAM_OBJS += credential-cache--daemon.o
>  endif
>
>  ifdef NO_ICONV
> @@ -2901,7 +2900,6 @@ ifdef MSVC
>  	# because it is just a copy/hardlink of git.exe, rather than a unique binary.
>  	$(INSTALL) git.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
>  	$(INSTALL) git-shell.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
> -	$(INSTALL) git-credential-store.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
>  	$(INSTALL) git-daemon.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
>  	$(INSTALL) git-fast-import.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
>  	$(INSTALL) git-http-backend.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
> diff --git a/builtin.h b/builtin.h
> index a5ae15bfe5..4a0aed5448 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -138,6 +138,9 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix);
>  int cmd_config(int argc, const char **argv, const char *prefix);
>  int cmd_count_objects(int argc, const char **argv, const char *prefix);
>  int cmd_credential(int argc, const char **argv, const char *prefix);
> +int cmd_credential_cache(int argc, const char **argv, const char *prefix);
> +int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix);
> +int cmd_credential_store(int argc, const char **argv, const char *prefix);
>  int cmd_describe(int argc, const char **argv, const char *prefix);
>  int cmd_diff_files(int argc, const char **argv, const char *prefix);
>  int cmd_diff_index(int argc, const char **argv, const char *prefix);
> diff --git a/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> similarity index 91%
> rename from credential-cache--daemon.c
> rename to builtin/credential-cache--daemon.c
> index ec1271f89c..c61f123a3b 100644
> --- a/credential-cache--daemon.c
> +++ b/builtin/credential-cache--daemon.c
> @@ -1,9 +1,12 @@
> -#include "cache.h"
> +#include "builtin.h"
> +#include "parse-options.h"
> +
> +#ifndef NO_UNIX_SOCKETS
> +
>  #include "config.h"
>  #include "tempfile.h"
>  #include "credential.h"
>  #include "unix-socket.h"
> -#include "parse-options.h"
>
>  struct credential_cache_entry {
>  	struct credential item;
> @@ -257,7 +260,7 @@ static void init_socket_directory(const char *path)
>  	free(path_copy);
>  }
>
> -int cmd_main(int argc, const char **argv)
> +int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)
>  {
>  	struct tempfile *socket_file;
>  	const char *socket_path;
> @@ -275,7 +278,7 @@ int cmd_main(int argc, const char **argv)
>
>  	git_config_get_bool("credentialcache.ignoresighup", &ignore_sighup);
>
> -	argc = parse_options(argc, argv, NULL, options, usage, 0);
> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
>  	socket_path = argv[0];
>
>  	if (!socket_path)
> @@ -295,3 +298,21 @@ int cmd_main(int argc, const char **argv)
>
>  	return 0;
>  }
> +
> +#else
> +
> +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"));
> +}
> +
> +#endif /* NO_UNIX_SOCKET */
> diff --git a/credential-cache.c b/builtin/credential-cache.c
> similarity index 83%
> rename from credential-cache.c
> rename to builtin/credential-cache.c
> index 1cccc3a0b9..d0fafdeb9e 100644
> --- a/credential-cache.c
> +++ b/builtin/credential-cache.c
> @@ -1,7 +1,10 @@
> -#include "cache.h"
> +#include "builtin.h"
> +#include "parse-options.h"
> +
> +#ifndef NO_UNIX_SOCKETS
> +
>  #include "credential.h"
>  #include "string-list.h"
> -#include "parse-options.h"
>  #include "unix-socket.h"
>  #include "run-command.h"
>
> @@ -96,7 +99,7 @@ static char *get_socket_path(void)
>  	return socket;
>  }
>
> -int cmd_main(int argc, const char **argv)
> +int cmd_credential_cache(int argc, const char **argv, const char *prefix)
>  {
>  	char *socket_path = NULL;
>  	int timeout = 900;
> @@ -113,7 +116,7 @@ int cmd_main(int argc, const char **argv)
>  		OPT_END()
>  	};
>
> -	argc = parse_options(argc, argv, NULL, options, usage, 0);
> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
>  	if (!argc)
>  		usage_with_options(usage, options);
>  	op = argv[0];
> @@ -134,3 +137,21 @@ int cmd_main(int argc, const char **argv)
>
>  	return 0;
>  }
> +
> +#else
> +
> +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"));
> +}
> +
> +#endif /* NO_UNIX_SOCKETS */
> diff --git a/credential-store.c b/builtin/credential-store.c
> similarity index 96%
> rename from credential-store.c
> rename to builtin/credential-store.c
> index 294e771681..5331ab151a 100644
> --- a/credential-store.c
> +++ b/builtin/credential-store.c
> @@ -1,4 +1,4 @@
> -#include "cache.h"
> +#include "builtin.h"
>  #include "lockfile.h"
>  #include "credential.h"
>  #include "string-list.h"
> @@ -143,7 +143,7 @@ static void lookup_credential(const struct string_list *fns, struct credential *
>  			return; /* Found credential */
>  }
>
> -int cmd_main(int argc, const char **argv)
> +int cmd_credential_store(int argc, const char **argv, const char *prefix)
>  {
>  	const char * const usage[] = {
>  		"git credential-store [<options>] <action>",
> @@ -161,7 +161,7 @@ int cmd_main(int argc, const char **argv)
>
>  	umask(077);
>
> -	argc = parse_options(argc, (const char **)argv, NULL, options, usage, 0);
> +	argc = parse_options(argc, (const char **)argv, prefix, options, usage, 0);
>  	if (argc != 1)
>  		usage_with_options(usage, options);
>  	op = argv[0];
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 47215df25b..4be61247e5 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -501,15 +501,9 @@ unset(CMAKE_REQUIRED_INCLUDES)
>
>  #programs
>  set(PROGRAMS_BUILT
> -	git git-bugreport git-credential-store git-daemon git-fast-import git-http-backend git-sh-i18n--envsubst
> +	git git-bugreport git-daemon git-fast-import git-http-backend git-sh-i18n--envsubst
>  	git-shell git-remote-testsvn)
>
> -if(NO_UNIX_SOCKETS)
> -	list(APPEND excluded_progs git-credential-cache git-credential-cache--daemon)
> -else()
> -	list(APPEND PROGRAMS_BUILT git-credential-cache git-credential-cache--daemon)
> -endif()
> -
>  if(NOT CURL_FOUND)
>  	list(APPEND excluded_progs git-http-fetch git-http-push)
>  	add_compile_definitions(NO_CURL)
> @@ -633,9 +627,6 @@ target_link_libraries(git common-main)
>  add_executable(git-bugreport ${CMAKE_SOURCE_DIR}/bugreport.c)
>  target_link_libraries(git-bugreport common-main)
>
> -add_executable(git-credential-store ${CMAKE_SOURCE_DIR}/credential-store.c)
> -target_link_libraries(git-credential-store common-main)
> -
>  add_executable(git-daemon ${CMAKE_SOURCE_DIR}/daemon.c)
>  target_link_libraries(git-daemon common-main)
>
> @@ -672,15 +663,6 @@ endif()
>  add_executable(git-remote-testsvn ${CMAKE_SOURCE_DIR}/remote-testsvn.c)
>  target_link_libraries(git-remote-testsvn common-main vcs-svn)
>
> -if(NOT NO_UNIX_SOCKETS)
> -	add_executable(git-credential-cache ${CMAKE_SOURCE_DIR}/credential-cache.c)
> -	target_link_libraries(git-credential-cache common-main)
> -
> -	add_executable(git-credential-cache--daemon ${CMAKE_SOURCE_DIR}/credential-cache--daemon.c)
> -	target_link_libraries(git-credential-cache--daemon common-main)
> -endif()
> -
> -
>  set(git_builtin_extra
>  	cherry cherry-pick format-patch fsck-objects
>  	init merge-subtree restore show
> diff --git a/git.c b/git.c
> index 8bd1d7551d..39a160fa52 100644
> --- a/git.c
> +++ b/git.c
> @@ -499,6 +499,9 @@ 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-store", cmd_credential_store },
>  	{ "describe", cmd_describe, RUN_SETUP },
>  	{ "diff", cmd_diff, NO_PARSEOPT },
>  	{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
> --
> 2.28.0.573.gec6564704b
>

Thanks, Taylor

Previous: Jeff KingNext: Jeff King
Message 12 of 36 in “slimming down installed size”
  1. 0/5 slimming down installed sizeJeff King, Aug 13, 2020
  2. 1/5 Makefile: drop builtins from MSVC pdb listJeff King, Aug 13, 2020
  3. Taylor BlauAug 13, 2020
  4. Jeff KingAug 13, 2020
  5. Derrick StoleeAug 13, 2020
  6. Jeff KingAug 13, 2020
  7. Johannes SchindelinAug 14, 2020
  8. Jeff KingAug 14, 2020
  9. Johannes SchindelinAug 17, 2020
  10. Jeff HostetlerAug 17, 2020
  11. 2/5 make credential helpers builtinsJeff King, Aug 13, 2020
  12. Taylor BlauAug 13, 2020
  13. Jeff KingAug 13, 2020
  14. Junio C HamanoAug 13, 2020
  15. 3/5 make git-bugreport a builtinJeff King, Aug 13, 2020
  16. Derrick StoleeAug 13, 2020
  17. Jeff KingAug 13, 2020
  18. Junio C HamanoAug 13, 2020
  19. Junio C HamanoAug 13, 2020
  20. Jeff KingAug 14, 2020
  21. Johannes SchindelinAug 14, 2020
  22. Jeff KingAug 14, 2020
  23. Junio C HamanoAug 13, 2020
  24. Johannes SchindelinAug 14, 2020
  25. Jeff KingAug 15, 2020
  26. Emily ShafferAug 17, 2020
  27. Junio C HamanoAug 17, 2020
  28. Jeff KingAug 17, 2020
  29. Emily ShafferAug 17, 2020
  30. 4/5 make git-fast-import a builtinJeff King, Aug 13, 2020
  31. 5/5 drop vcs-svn experimentJeff King, Aug 13, 2020
  32. Taylor BlauAug 13, 2020
  33. Jeff KingAug 13, 2020
  34. Johannes SchindelinAug 14, 2020
  35. Jeff KingAug 14, 2020
  36. Taylor BlauAug 13, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.