git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 2/5] make credential helpers builtins
Date: Thu, 13 Aug 2020 11:08:39 -0400	[thread overview]
Message-ID: <20200813150839.GB2244@syl.lan> (raw)
In-Reply-To: <20200813145855.GB891370@coredump.intra.peff.net>

On Thu, Aug 13, 2020 at 10:58:55AM -0400, Jeff King wrote:
> 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.

> 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.

> 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.

> 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

  reply	other threads:[~2020-08-13 15:08 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-13 14:55 [PATCH 0/5] slimming down installed size Jeff King
2020-08-13 14:57 ` [PATCH 1/5] Makefile: drop builtins from MSVC pdb list Jeff King
2020-08-13 15:04   ` Taylor Blau
2020-08-13 15:08     ` Jeff King
2020-08-13 16:37       ` Derrick Stolee
2020-08-13 17:40         ` Jeff King
2020-08-14 14:18       ` Johannes Schindelin
2020-08-14 14:32         ` Jeff King
2020-08-17  4:42           ` Johannes Schindelin
2020-08-17 13:20         ` Jeff Hostetler
2020-08-13 14:58 ` [PATCH 2/5] make credential helpers builtins Jeff King
2020-08-13 15:08   ` Taylor Blau [this message]
2020-08-13 15:14     ` Jeff King
2020-08-13 17:55       ` Junio C Hamano
2020-08-13 14:59 ` [PATCH 3/5] make git-bugreport a builtin Jeff King
2020-08-13 17:01   ` Derrick Stolee
2020-08-13 17:38     ` Jeff King
2020-08-13 18:25       ` Junio C Hamano
2020-08-13 18:47         ` Junio C Hamano
2020-08-14 10:13           ` Jeff King
2020-08-14 14:25           ` Johannes Schindelin
2020-08-14 10:05         ` Jeff King
2020-08-13 18:01   ` Junio C Hamano
2020-08-14 14:28     ` Johannes Schindelin
2020-08-15  6:38     ` Jeff King
2020-08-17 12:12       ` Emily Shaffer
2020-08-17 16:58       ` Junio C Hamano
2020-08-17 21:40         ` Jeff King
2020-08-17 12:16   ` Emily Shaffer
2020-08-13 14:59 ` [PATCH 4/5] make git-fast-import " Jeff King
2020-08-13 15:00 ` [PATCH 5/5] drop vcs-svn experiment Jeff King
2020-08-13 15:11   ` Taylor Blau
2020-08-13 15:18     ` Jeff King
2020-08-14 14:39   ` Johannes Schindelin
2020-08-14 15:11     ` Jeff King
2020-08-13 15:13 ` [PATCH 0/5] slimming down installed size Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200813150839.GB2244@syl.lan \
    --to=me@ttaylorr.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --subject='Re: [PATCH 2/5] make credential helpers builtins' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).