[PATCH 0/2] Make gc a builtin.

The following two patches make git-gc a builtin command.

The first patch modifies run-command.*, making two public functions that take
va_lists (one of these existed already, of course), so that less code has to be duplicated in builtin-gc.c for error handling. The second patch contains the
builtin-gc.c code itself.

-James

Signed-off-by: James Bowes <redacted>
---
 run-command.c |    7 ++++++-
 run-command.h |    2 ++
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/run-command.c b/run-command.c
index cfbad74..34fc100 100644
--- a/run-command.c
+++ b/run-command.c

@@ -52,7 +52,7 @@ int run_command_v(const char **argv)
 	return run_command_v_opt(argv, 0);
 }
 
-static int run_command_va_opt(int opt, const char *cmd, va_list param)
+int run_command_va_opt(int opt, const char *cmd, va_list param)
 {
 	int argc;
 	const char *argv[MAX_RUN_COMMAND_ARGS];

@@ -70,6 +70,11 @@ static int run_command_va_opt(int opt, const char *cmd, va_list param)
 	return run_command_v_opt(argv, opt);
 }
 
+int run_command_va(const char *cmd, va_list param)
+{
+	return run_command_va_opt(0, cmd, param);
+}
+
 int run_command_opt(int opt, const char *cmd, ...)
 {
 	va_list params;

diff --git a/run-command.h b/run-command.h
index 59c4476..3934643 100644
--- a/run-command.h
+++ b/run-command.h

@@ -16,6 +16,8 @@ enum {
 #define RUN_COMMAND_STDOUT_TO_STDERR 4
 int run_command_v_opt(const char **argv, int opt);
 int run_command_v(const char **argv);
+int run_command_va_opt(int opt, const char *cmd, va_list param);
+int run_command_va(const char *cmd, va_list param);
 int run_command_opt(int opt, const char *cmd, ...);
 int run_command(const char *cmd, ...);
 

-- 
1.5.0.2

Signed-off-by: James Bowes <redacted>
---
 Makefile     |    3 +-
 builtin-gc.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 builtin.h    |    1 +
 git-gc.sh    |   37 --------------------------
 git.c        |    1 +
 5 files changed, 85 insertions(+), 38 deletions(-)
 create mode 100644 builtin-gc.c
 delete mode 100755 git-gc.sh

diff --git a/Makefile b/Makefile
index f0fc2f8..fb17cfb 100644
--- a/Makefile
+++ b/Makefile

@@ -177,7 +177,7 @@ BASIC_LDFLAGS =
 SCRIPT_SH = \
 	git-bisect.sh git-checkout.sh \
 	git-clean.sh git-clone.sh git-commit.sh \
-	git-fetch.sh git-gc.sh \
+	git-fetch.sh \
 	git-ls-remote.sh \
 	git-merge-one-file.sh git-parse-remote.sh \
 	git-pull.sh git-rebase.sh \

@@ -297,6 +297,7 @@ BUILTIN_OBJS = \
 	builtin-fmt-merge-msg.o \
 	builtin-for-each-ref.o \
 	builtin-fsck.o \
+	builtin-gc.o \
 	builtin-grep.o \
 	builtin-init-db.o \
 	builtin-log.o \

diff --git a/builtin-gc.c b/builtin-gc.c
new file mode 100644
index 0000000..7a9332b
--- /dev/null
+++ b/builtin-gc.c

@@ -0,0 +1,81 @@
+/*
+ * git gc builtin command
+ *
+ * Cleanup unreachable files and optimize the repository.
+ *
+ * Copyright (c) 2007 James Bowes
+ *
+ * Based on git-gc.sh, which is
+ *
+ * Copyright (c) 2006 Shawn O. Pearce
+ */
+
+#include "cache.h"
+#include "run-command.h"
+
+static const char builtin_gc_usage[] = "git-gc [--prune]";
+
+static int pack_refs;
+
+static int gc_config(const char *var, const char *value)
+{
+	if (!strcmp(var, "gc.packrefs"))
+		if (strlen(value) == 0 || !strcmp(value, "notbare"))
+			pack_refs = !is_bare_repository();
+		else
+			pack_refs = git_config_bool(var, value);
+	else
+		return git_default_config(var, value);
+	return 0;
+}
+
+static void run_command_or_die(const char *cmd, ...)
+{
+	int err;
+	va_list params;
+
+	va_start(params, cmd);
+	err = run_command_va(cmd, params);
+	va_end(params);
+
+	switch (err) {
+	case 0:
+		return;
+	case -ERR_RUN_COMMAND_FORK:
+		die("unable to fork for %s", cmd);
+	case -ERR_RUN_COMMAND_EXEC:
+		die("unable to exec %s", cmd);
+	default:
+		die("%s died with strange error", cmd);
+	}
+}
+
+int cmd_gc(int argc, const char **argv, const char *prefix)
+{
+	int i;
+	int prune = 0;
+
+	git_config(gc_config);
+
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+		if (!strcmp(arg, "--prune")) {
+			prune = 1;
+			continue;
+		}
+		/* perhaps other parameters later... */
+		break;
+	}
+	if (i != argc)
+		usage(builtin_gc_usage);
+
+	if (pack_refs)
+		run_command_or_die("git-pack-refs", "--prune", NULL);
+	run_command_or_die("git-reflog", "expire", "--all", NULL);
+	run_command_or_die("git-repack", "-a", "-d", "-l", NULL);
+	if (prune)
+		run_command_or_die("git-prune", NULL);
+	run_command_or_die("git-rerere", "gc", NULL);
+
+	return 0;
+}

diff --git a/builtin.h b/builtin.h
index 1cb64b7..af203e9 100644
--- a/builtin.h
+++ b/builtin.h

@@ -37,6 +37,7 @@ extern int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix);
 extern int cmd_for_each_ref(int argc, const char **argv, const char *prefix);
 extern int cmd_format_patch(int argc, const char **argv, const char *prefix);
 extern int cmd_fsck(int argc, const char **argv, const char *prefix);
+extern int cmd_gc(int argc, const char **argv, const char *prefix);
 extern int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix);
 extern int cmd_grep(int argc, const char **argv, const char *prefix);
 extern int cmd_help(int argc, const char **argv, const char *prefix);

diff --git a/git-gc.sh b/git-gc.sh
deleted file mode 100755
index 436d7ca..0000000
--- a/git-gc.sh
+++ /dev/null

@@ -1,37 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2006, Shawn O. Pearce
-#
-# Cleanup unreachable files and optimize the repository.
-
-USAGE='[--prune]'
-SUBDIRECTORY_OK=Yes
-. git-sh-setup
-
-no_prune=:
-while case $# in 0) break ;; esac
-do
-	case "$1" in
-	--prune)
-		no_prune=
-		;;
-	--)
-		usage
-		;;
-	esac
-	shift
-done
-
-case "$(git config --get gc.packrefs)" in
-notbare|"")
-	test $(is_bare_repository) = true || pack_refs=true;;
-*)
-	pack_refs=$(git config --bool --get gc.packrefs)
-esac
-
-test "true" != "$pack_refs" ||
-git-pack-refs --prune &&
-git-reflog expire --all &&
-git-repack -a -d -l &&
-$no_prune git-prune &&
-git-rerere gc || exit

diff --git a/git.c b/git.c
index dde4d07..ed1c65e 100644
--- a/git.c
+++ b/git.c

@@ -249,6 +249,7 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
 		{ "format-patch", cmd_format_patch, RUN_SETUP },
 		{ "fsck", cmd_fsck, RUN_SETUP },
 		{ "fsck-objects", cmd_fsck, RUN_SETUP },
+		{ "gc", cmd_gc, RUN_SETUP },
 		{ "get-tar-commit-id", cmd_get_tar_commit_id },
 		{ "grep", cmd_grep, RUN_SETUP | USE_PAGER },
 		{ "help", cmd_help },

-- 
1.5.0.2

Hi,

On Sun, 11 Mar 2007, James Bowes wrote:

+	if (pack_refs)
+		run_command_or_die("git-pack-refs", "--prune", NULL);
+	run_command_or_die("git-reflog", "expire", "--all", NULL);
+	run_command_or_die("git-repack", "-a", "-d", "-l", NULL);
+	if (prune)
+		run_command_or_die("git-prune", NULL);
+	run_command_or_die("git-rerere", "gc", NULL);

Shawn recently sent a series which discourages the va_list versions of 
run_command. I think that makes sense. So, using 
run_command_v_opt(argv_pack_refs, RUN_GIT_CMD) would be better IMHO.

And instead of die()ing, I'd rather do something like

	return (pack_refs || run_command_v_opt(argv_pack_refs, RUN_GIT_CMD) &&
		run_command_v_opt(argv_reflog_expire, RUN_GIT_CMD) &&
		run_command_v_opt(argv_repack, RUN_GIT_CMD) &&
		(prune || run_command_v_opt(argv_prune, RUN_GIT_CMD) &&
		run_command_v_opt(argv_rerere, RUN_GIT_CMD);

Hmm?

Ciao,
Dscho

On Sun, Mar 11, 2007 at 06:06:56PM -0400, James Bowes wrote:

The following two patches make git-gc a builtin command.

What's the advantage in making git-gc a builtin command?  It's not
like it's going to help performance a whole lot (especially since
you're just forking separate processes to run git-prune,
git-pack-refs, et.al.), and as a shell script it's a lot easier to
explain to people what git-gc is actually doing, so there is
pedagogical value to keeping it as a shell script.

Regards,

						- Ted

Hi,

On Sun, 11 Mar 2007, Theodore Tso wrote:

On Sun, Mar 11, 2007 at 06:06:56PM -0400, James Bowes wrote:

quoted

The following two patches make git-gc a builtin command.

What's the advantage in making git-gc a builtin command?

Portability. Plus, James wanted to get involved in Git development, and 
building in gc really was the shortest path into that.

Ciao,
Dscho

On Mon, Mar 12, 2007 at 12:23:41PM +0100, Johannes Schindelin wrote:

Hi,

On Sun, 11 Mar 2007, Theodore Tso wrote:

quoted

On Sun, Mar 11, 2007 at 06:06:56PM -0400, James Bowes wrote:

quoted

The following two patches make git-gc a builtin command.

What's the advantage in making git-gc a builtin command?

Portability. Plus, James wanted to get involved in Git development, and 
building in gc really was the shortest path into that.

I'm not sure I understand the portability argument?  All of the
platforms that git currently supports will handle shell scripts,
right?  

Heck, git-commit is still a shell script, and that's a rather, ah,
fundamental command, isn't it? 

						- Ted

Johannes Schindelin [off-list ref] wrote:

On Sun, 11 Mar 2007, Theodore Tso wrote:

quoted

On Sun, Mar 11, 2007 at 06:06:56PM -0400, James Bowes wrote:

quoted

The following two patches make git-gc a builtin command.

What's the advantage in making git-gc a builtin command?

Portability. Plus, James wanted to get involved in Git development, and 
building in gc really was the shortest path into that.

Actually, git-gc.sh is pretty portable.  To POSIX systems.
Windows ain't POSIX.  Getting rid of some of those shell scripts
just makes us more portable, even to Windows.  (Yes, people really
do still get forced to use that non-operating system.)

Ted talked about git-commit.sh being more important, but Dsco
clipped it. ;-)

I think git-commit.sh and git-merge.sh should both get ported to
builtins too, as both are somewhat hairy in shell, are quite core
to the system, and would be faster on Windows if written in C
(less forking == more speed there).

But they are so core that any rewrite must be undertaken carefully.

-- 
Shawn.

On Mon, 12 Mar 2007, Theodore Tso wrote:

I'm not sure I understand the portability argument?  All of the
platforms that git currently supports will handle shell scripts,
right?  

Git "supports" MinGW, or at least wants to. And yes, you can put bash in 
there, but we'd be *so* much better off if we had no shell scripting at 
all.

Another thing I find annoying (even as a UNIX user) is that whenever I do 
any tracing for performance data, shell is absolutely horrid. It's *so* 
much nicer to do 'strace' on built-in programs that it's not even funny.

It's also sad how many performance issues we've had with shell, just 
because even something really simple (like a few hundred refs) is just too 
slow for shell scripting.

Heck, git-commit is still a shell script, and that's a rather, ah,
fundamental command, isn't it? 

Yeah, and that's probably my pet peeve. I'd love to see a built-in "git 
commit" and "git fetch". The "fetch--tool" thing in next gets rid of some 
of the latter (and apparently the worst performance problems), but it's 
sad how we have a really nice builtin "push", but our "fetch" is still 
mostly really hairy shell-code (not just "git-fetch.sh" itself, but 
"git-parse-remote.sh".

A gold star for whoever gets rid of any of of commit/clone/fetch or 
ls-remote

(ls-remote isn't that big or hairy, but I mention it because it's a user 
of "parse-remote", so making even just ls-remote built-in is probably 
going to help with fetch/clone eventually).

		Linus

Linus Torvalds wrote:

Another thing I find annoying (even as a UNIX user) is that whenever I do 
any tracing for performance data, shell is absolutely horrid. It's *so* 
much nicer to do 'strace' on built-in programs that it's not even funny.

Isn't that what GIT_TRACE was made for?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

On Tue, 13 Mar 2007, Jakub Narebski wrote:

Linus Torvalds wrote:

quoted

Another thing I find annoying (even as a UNIX user) is that whenever I do 
any tracing for performance data, shell is absolutely horrid. It's *so* 
much nicer to do 'strace' on built-in programs that it's not even funny.

Isn't that what GIT_TRACE was made for?

That just shows the high-level git commands.

If you look for performance issues or correctness issues (like when I 
tried to figure out if O_LARGEFILE was set for "git clone"), GIT_TRACE 
does nothing. You want to do "strace -f -o trace-file".

And shell scripts look horrible there, and make it much harder to follow 
things. In fact, it doesn't even need to be shell per se, but fork/exec 
already makes things harder to see, shell just tends to (a) make it even 
more so (try stracing though a shell startup, ugh) and (b) cause tons of 
fork/exec cases.

For example, when we made patch generation a built-in, it suddenly became 
*hugely* easier to follow what was going on in the traces, because it got 
much more streamlined. In general I find that "high performance" == "easy 
to trace".

		Linus