[PATCH 0/2] Make gc a builtin.

[PATCH 0/2] Make gc a builtin.

[James Bowes]

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

[PATCH 1/2] run-command: Make run_command_va_opt public and add run_command_va

[James Bowes]

Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>
---
 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

[PATCH 2/2] Make gc a builtin.

[James Bowes]

Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>
---
 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

Re: [PATCH 2/2] Make gc a builtin.

[Johannes Schindelin]

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

Re: [PATCH 2/2] Make gc a builtin.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 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);

Gaaaaaaaah.

That may be valid C, but please do that as a sequence of
separate statements.

	if (we are told to pack-refs)
        	if (try to pack refs and find error)
			goto failure;

	if (try to reflog expire and find error)
		goto failure;

        ...

	return Ok;

	failure:

	return Error;

[PATCH] Make gc a builtin.

[James Bowes]

Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>
---

This patch replaces replaces my previous two. It uses
run_command_v_opt rather than changing run-command.*, and

if (command_fails)
    goto failure

for running the commands.

-James

 Makefile     |    3 +-
 builtin-gc.c |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 builtin.h    |    1 +
 git-gc.sh    |   37 ---------------------------
 git.c        |    1 +
 5 files changed, 82 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..c507bdf
--- /dev/null
+++ b/builtin-gc.c
@@ -0,0 +1,78 @@
+/*
+ * 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 const char *argv_pack_refs[] = {"pack-refs", "--prune", NULL};
+static const char *argv_reflog[] = {"reflog", "expire", "--all", NULL};
+static const char *argv_repack[] = {"repack", "-a", "-d", "-l", NULL};
+static const char *argv_prune[] = {"prune", NULL};
+static const char *argv_rerere[] = {"rerere", "gc", NULL};
+
+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;
+}
+
+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)
+	    if (run_command_v_opt(argv_pack_refs, RUN_GIT_CMD))
+            goto failure;
+
+    if (run_command_v_opt(argv_reflog, RUN_GIT_CMD))
+        goto failure;
+
+    if (run_command_v_opt(argv_repack, RUN_GIT_CMD))
+        goto failure;
+
+    if (prune)
+        if (run_command_v_opt(argv_prune, RUN_GIT_CMD))
+            goto failure;
+
+    if (run_command_v_opt(argv_rerere, RUN_GIT_CMD))
+        goto failure;
+
+    return 0;
+
+failure:
+    return -1;
+}
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

Re: [PATCH 0/2] Make gc a builtin.

[Theodore Tso]

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

Re: [PATCH 2/2] Make gc a builtin.

[Johannes Schindelin]

Hi,

On Sun, 11 Mar 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 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);
> 
> Gaaaaaaaah.
> 
> That may be valid C,

Actually, it is not. As usual, I fscked up: run_command_v_opt() is 
supposed to return 0 on _success_, so all the "&&" should be "||", and all 
the "||" should be "&&".

> 	if (we are told to pack-refs)
>         	if (try to pack refs and find error)
> 			goto failure;

I find this not very elegant. Instead, I'd do

	if (do_pack_refs && run_comand_v_opt(argv_pack_refs, RUN_GIT_CMD))
		return error("Could not run pack-refs.");

It does not only avoid the evil goto, but uses the screen estate for some 
nice error messages so that the user is not left out in the cold when 
something is wrong.

Ciao,
Dscho

Re: [PATCH 0/2] Make gc a builtin.

[Johannes Schindelin]

Hi,

On Sun, 11 Mar 2007, Theodore Tso wrote:

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

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

Ciao,
Dscho

Re: [PATCH 0/2] Make gc a builtin.

[Theodore Tso]

On Mon, Mar 12, 2007 at 12:23:41PM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 11 Mar 2007, Theodore Tso wrote:
> 
> > 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?
> 
> 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

Re: [PATCH 0/2] Make gc a builtin.

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Sun, 11 Mar 2007, Theodore Tso wrote:
> 
> > 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?
> 
> 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.

Re: [PATCH] Make gc a builtin.

[Shawn O. Pearce]

A good (second) try.

James Bowes <jbowes@dangerouslyinc.com> wrote:
> diff --git a/builtin-gc.c b/builtin-gc.c
> +
> +static int pack_refs;

Actually I think you want to use:

static int pack_refs = -1;

See below for why...

> +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;
> +}

Gaaah.  How about some curly braces around the then part of that
first if?

Actually, we typically just write this more like:

static int gc_config(const char *var, const char *value)
{
	if (!strcmp(var, "gc.packrefs")) {
		if (!strcmp(value, "notbare"))
			pack_refs = -1;
		else
			pack_refs = git_config_bool(var, value);
	}
	return git_default_config(var, value);
}

> +int cmd_gc(int argc, const char **argv, const char *prefix)
> +{
> +	int i;
> +	int prune = 0;
> +
> +	git_config(gc_config);

if (pack_refs < 0)
	pack_refs = !is_bare_repository();

The is_bare_repository function guesses until the configuration
is done parsing; once the configuration has been parsed it has a
definate answer one way or the other.  So what I'm suggesting you
do here is set pack_refs = -1 to mean use the is_bare_repository
setting, otherwise it stays what it was set to.

> +    if (pack_refs)
> +	    if (run_command_v_opt(argv_pack_refs, RUN_GIT_CMD))
> +            goto failure;
....
> +    if (prune)
> +        if (run_command_v_opt(argv_prune, RUN_GIT_CMD))
> +            goto failure;

Gaah.  Tabs-vs-spaces, not to mention that these aren't even lining
up the same way.  I too prefer what Dsco suggested already:

	if (prune && run_command_v_opt(argv_prune, RUN_GIT_CMD))
		return error("failed to run %s", argv_prune[0]);

-- 
Shawn.

Re: [PATCH 0/2] Make gc a builtin.

[Linus Torvalds]

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

Re: [PATCH 0/2] Make gc a builtin.

[Jakub Narebski]

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

Re: [PATCH 0/2] Make gc a builtin.

[Linus Torvalds]

On Tue, 13 Mar 2007, Jakub Narebski wrote:

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

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