Re: [PATCH 3/5] make git-bugreport a builtin

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> There's no reason that bugreport has to be a separate binary. And since
> it links against libgit.a, it has a rather large disk footprint. Let's
> make it a builtin, which reduces the size of a stripped installation
> from 24MB to 22MB.
>
> This also simplifies our Makefile a bit. And we can take advantage of
> builtin niceties like RUN_SETUP_GENTLY.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Makefile                            |  6 +-----
>  builtin.h                           |  1 +
>  bugreport.c => builtin/bugreport.c  | 10 +++-------
>  contrib/buildsystems/CMakeLists.txt |  5 +----
>  git.c                               |  1 +
>  5 files changed, 7 insertions(+), 16 deletions(-)
>  rename bugreport.c => builtin/bugreport.c (96%)

I am on the fence, as bugreport does not seem to be fully completed
part of the system.  The original thinking was that it may soon want
to grow by linking with platform specific libraries for lower-level
system characteristic identification, at which time we'd not want to
have it in builtins and "we can take advantage of builtin niceties"
will cause us regrets.  The only reason that hasn't happened as far
as I can see is because its development speed is rather slow.

So, I dunno.


> diff --git a/Makefile b/Makefile
> index 5b43c0fafb..acaff6968b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -671,7 +671,6 @@ EXTRA_PROGRAMS =
>  # ... and all the rest that could be moved out of bindir to gitexecdir
>  PROGRAMS += $(EXTRA_PROGRAMS)
>  
> -PROGRAM_OBJS += bugreport.o
>  PROGRAM_OBJS += daemon.o
>  PROGRAM_OBJS += fast-import.o
>  PROGRAM_OBJS += http-backend.o
> @@ -1041,6 +1040,7 @@ BUILTIN_OBJS += builtin/archive.o
>  BUILTIN_OBJS += builtin/bisect--helper.o
>  BUILTIN_OBJS += builtin/blame.o
>  BUILTIN_OBJS += builtin/branch.o
> +BUILTIN_OBJS += builtin/bugreport.o
>  BUILTIN_OBJS += builtin/bundle.o
>  BUILTIN_OBJS += builtin/cat-file.o
>  BUILTIN_OBJS += builtin/check-attr.o
> @@ -2458,10 +2458,6 @@ endif
>  git-%$X: %.o GIT-LDFLAGS $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
>  
> -git-bugreport$X: bugreport.o GIT-LDFLAGS $(GITLIBS)
> -	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> -		$(LIBS)
> -
>  git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
>  		$(IMAP_SEND_LDFLAGS) $(LIBS)
> diff --git a/builtin.h b/builtin.h
> index 4a0aed5448..1e78d6c142 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -119,6 +119,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix);
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix);
>  int cmd_blame(int argc, const char **argv, const char *prefix);
>  int cmd_branch(int argc, const char **argv, const char *prefix);
> +int cmd_bugreport(int argc, const char **argv, const char *prefix);
>  int cmd_bundle(int argc, const char **argv, const char *prefix);
>  int cmd_cat_file(int argc, const char **argv, const char *prefix);
>  int cmd_checkout(int argc, const char **argv, const char *prefix);
> diff --git a/bugreport.c b/builtin/bugreport.c
> similarity index 96%
> rename from bugreport.c
> rename to builtin/bugreport.c
> index 09579e268d..9c920cc065 100644
> --- a/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -1,4 +1,4 @@
> -#include "cache.h"
> +#include "builtin.h"
>  #include "parse-options.h"
>  #include "strbuf.h"
>  #include "help.h"
> @@ -119,16 +119,14 @@ static void get_header(struct strbuf *buf, const char *title)
>  	strbuf_addf(buf, "\n\n[%s]\n", title);
>  }
>  
> -int cmd_main(int argc, const char **argv)
> +int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  {
>  	struct strbuf buffer = STRBUF_INIT;
>  	struct strbuf report_path = STRBUF_INIT;
>  	int report = -1;
>  	time_t now = time(NULL);
>  	char *option_output = NULL;
>  	char *option_suffix = "%Y-%m-%d-%H%M";
> -	int nongit_ok = 0;
> -	const char *prefix = NULL;
>  	const char *user_relative_path = NULL;
>  
>  	const struct option bugreport_options[] = {
> @@ -139,8 +137,6 @@ int cmd_main(int argc, const char **argv)
>  		OPT_END()
>  	};
>  
> -	prefix = setup_git_directory_gently(&nongit_ok);
> -
>  	argc = parse_options(argc, argv, prefix, bugreport_options,
>  			     bugreport_usage, 0);
>  
> @@ -170,7 +166,7 @@ int cmd_main(int argc, const char **argv)
>  	get_system_info(&buffer);
>  
>  	get_header(&buffer, _("Enabled Hooks"));
> -	get_populated_hooks(&buffer, nongit_ok);
> +	get_populated_hooks(&buffer, !startup_info->have_repository);
>  
>  	/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */
>  	report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 4be61247e5..3e211606fd 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -501,7 +501,7 @@ unset(CMAKE_REQUIRED_INCLUDES)
>  
>  #programs
>  set(PROGRAMS_BUILT
> -	git git-bugreport git-daemon git-fast-import git-http-backend git-sh-i18n--envsubst
> +	git git-daemon git-fast-import git-http-backend git-sh-i18n--envsubst
>  	git-shell git-remote-testsvn)
>  
>  if(NOT CURL_FOUND)
> @@ -624,9 +624,6 @@ list(TRANSFORM git_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
>  add_executable(git ${CMAKE_SOURCE_DIR}/git.c ${git_SOURCES})
>  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-daemon ${CMAKE_SOURCE_DIR}/daemon.c)
>  target_link_libraries(git-daemon common-main)
>  
> diff --git a/git.c b/git.c
> index 39a160fa52..bf790e7f4f 100644
> --- a/git.c
> +++ b/git.c
> @@ -479,6 +479,7 @@ static struct cmd_struct commands[] = {
>  	{ "bisect--helper", cmd_bisect__helper, RUN_SETUP },
>  	{ "blame", cmd_blame, RUN_SETUP },
>  	{ "branch", cmd_branch, RUN_SETUP | DELAY_PAGER_CONFIG },
> +	{ "bugreport", cmd_bugreport, RUN_SETUP_GENTLY },
>  	{ "bundle", cmd_bundle, RUN_SETUP_GENTLY | NO_PARSEOPT },
>  	{ "cat-file", cmd_cat_file, RUN_SETUP },
>  	{ "check-attr", cmd_check_attr, RUN_SETUP },