[PATCH] Make gc a builtin.

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

[PATCH] Make gc a builtin.

[James Bowes]

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

Take 3. The changes are pretty much all of Shawn's suggestions. If a command
fails this code just returns -1, rather than calling error(), so that two
duplicate error messages aren't printed out.

-James

 Makefile     |    3 +-
 builtin-gc.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 builtin.h    |    1 +
 git-gc.sh    |   37 ----------------------------
 git.c        |    1 +
 5 files changed, 80 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..d86f07c
--- /dev/null
+++ b/builtin-gc.c
@@ -0,0 +1,76 @@
+/*
+ * 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 = -1;
+
+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 (!strcmp(value, "notbare"))
+			pack_refs = -1;
+		else
+			pack_refs = git_config_bool(var, value);
+		return 0;
+	}
+	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();
+
+	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_v_opt(argv_pack_refs, RUN_GIT_CMD))
+		return -1;
+
+	if (run_command_v_opt(argv_reflog, RUN_GIT_CMD))
+		return -1;
+
+	if (run_command_v_opt(argv_repack, RUN_GIT_CMD))
+		return -1;
+
+	if (prune && run_command_v_opt(argv_prune, RUN_GIT_CMD))
+		return -1;
+
+	if (run_command_v_opt(argv_rerere, RUN_GIT_CMD))
+		return -1;
+
+	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] Make gc a builtin.

[Johannes Schindelin]

Hi,

On Tue, 13 Mar 2007, James Bowes wrote:

> Take 3. The changes are pretty much all of Shawn's suggestions. If a 
> command fails this code just returns -1, rather than calling error(), so 
> that two duplicate error messages aren't printed out.

If you say "return error(...);", there is _no_ way that multiple error 
messages are printed out.

If you say "return -1;", however, the user is likely to _never_ know that 
git-gc failed. (I, for one, do not check $? after running a program which 
does not say _anything_.)

Ciao,
Dscho

Re: [PATCH] Make gc a builtin.

[Brian Gernhardt]

On Mar 13, 2007, at 9:05 PM, Johannes Schindelin wrote:

> If you say "return error(...);", there is _no_ way that multiple error
> messages are printed out.

Except that cmd_gc() is littered with run_command* calls, which fork  
off a subprocess to do the heavy lifting.  So if git-repack fails an  
error will be printed by that process, making the error() call  
redundant.  (If I'm understanding things correctly.)

~~ Brian

[PATCH] Make gc a builtin.

[James Bowes]

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

On 3/13/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> If you say "return error(...);", there is _no_ way that multiple error
> messages are printed out.

Yeah, I wasn't testing well enough. If you pass the name of a non-existant
command to run_command, then it will print out a message about not being able
to exec. That's not going to help when the command runs but does something bad.
So here's the patch with error().

-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 9b31565..1ccd52f 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..3b1f8c2
--- /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"
+
+#define FAILED_RUN "failed to run %s"
+
+static const char builtin_gc_usage[] = "git-gc [--prune]";
+
+static int pack_refs = -1;
+
+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 (!strcmp(value, "notbare"))
+			pack_refs = -1;
+		else
+			pack_refs = git_config_bool(var, value);
+		return 0;
+	}
+	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();
+
+	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_v_opt(argv_pack_refs, RUN_GIT_CMD))
+		return error(FAILED_RUN, argv_pack_refs[0]);
+
+	if (run_command_v_opt(argv_reflog, RUN_GIT_CMD))
+		return error(FAILED_RUN, argv_reflog[0]);
+
+	if (run_command_v_opt(argv_repack, RUN_GIT_CMD))
+		return error(FAILED_RUN, argv_repack[0]);
+
+	if (prune && run_command_v_opt(argv_prune, RUN_GIT_CMD))
+		return error(FAILED_RUN, argv_prune[0]);
+
+	if (run_command_v_opt(argv_rerere, RUN_GIT_CMD))
+		return error(FAILED_RUN, argv_rerere[0]);
+
+	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] Make gc a builtin.

[Shawn O. Pearce]

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

ACK.  Very nicely done.

...
> +	if (pack_refs && run_command_v_opt(argv_pack_refs, RUN_GIT_CMD))
> +		return error(FAILED_RUN, argv_pack_refs[0]);
> +
> +	if (run_command_v_opt(argv_reflog, RUN_GIT_CMD))
> +		return error(FAILED_RUN, argv_reflog[0]);
> +
> +	if (run_command_v_opt(argv_repack, RUN_GIT_CMD))
> +		return error(FAILED_RUN, argv_repack[0]);
> +
> +	if (prune && run_command_v_opt(argv_prune, RUN_GIT_CMD))
> +		return error(FAILED_RUN, argv_prune[0]);
> +
> +	if (run_command_v_opt(argv_rerere, RUN_GIT_CMD))
> +		return error(FAILED_RUN, argv_rerere[0]);

And isn't the above so much more readable than this mess?

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

Which is why I like builtins, and why I think Dscho does too.

-- 
Shawn.

Re: [PATCH] Make gc a builtin.

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> James Bowes <jbowes@dangerouslyinc.com> wrote:
>> Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>
>
> ACK.  Very nicely done.

Perhaps.  But we lost another sample script which made an entry
barrier higher to a new person.

>> +	if (run_command_v_opt(argv_rerere, RUN_GIT_CMD))
>> +		return error(FAILED_RUN, argv_rerere[0]);
>
> And isn't the above so much more readable than this mess?
>
>> -test "true" != "$pack_refs" ||
>> -git-pack-refs --prune &&
>> -git-reflog expire --all &&
>> -git-repack -a -d -l &&
>> ...

I do not necessarily think so.  This is not even a performance
critical part of the system, so if there _were_ no other
constraints, I would rather keep scripts like this as scripts.

For things like this, scripts are much easier to read,
understand and futz with, and command lists chained with && in
shell scripts are very nice and compact way to express what is
going on.

This is especially true if you have some specialized needs, if
you do not expect you need to keep that change forever, and if
you are lazy.  For example, if you have a repository that you
for some reason need to keep available to older dumb transport
clients for now, you would disable "git-pack-refs --prune" line
from your copy of the script version.  No need to recompile.

Another example is git-repack script.  When you have a
specialized repacking needs (say, repack from a specific
revision to make a .keep pack to avoid future excessive
repacking), being able to check how the plumbing is used in
git-repack script and run customized version of it is very
handy.  Once you rewrite it to sequence of

	if (run_command_v_opt(blech, RUN_GIT_CMD))
        	...

it becomes much harder to learn what the shell command
equivalent that would suit your needs would be, and we would
lose another command that would serve as a good example.

We are doing built-in _only_ because people on some platforms
cannot sanely use POSIX shell scripts.  I do not reject these
"make X built-in" patches (when X is perfectly fine as a shell
script) because I sympathize with people stuck on Windows, not
because I think built-in is easier to read nor work with than
scripts.  There is a downside.

Re: [PATCH] Make gc a builtin.

[Theodore Tso]

On Wed, Mar 14, 2007 at 12:19:07AM -0700, Junio C Hamano wrote:
> >> -test "true" != "$pack_refs" ||
> >> -git-pack-refs --prune &&
> >> -git-reflog expire --all &&
> >> -git-repack -a -d -l &&
> >> ...
> 
> I do not necessarily think so.  This is not even a performance
> critical part of the system, so if there _were_ no other
> constraints, I would rather keep scripts like this as scripts.

I agree with Junio; I think the scripts are much more readable and
easier to understand; In fact, it would be nice if the script were
preserved somewhere, perhaps as comments in the .c file.

						- Ted

Re: [PATCH] Make gc a builtin.

[Santi Béjar]

On 3/14/07, Theodore Tso <tytso@mit.edu> wrote:
> On Wed, Mar 14, 2007 at 12:19:07AM -0700, Junio C Hamano wrote:
> > >> -test "true" != "$pack_refs" ||
> > >> -git-pack-refs --prune &&
> > >> -git-reflog expire --all &&
> > >> -git-repack -a -d -l &&
> > >> ...
> >
> > I do not necessarily think so.  This is not even a performance
> > critical part of the system, so if there _were_ no other
> > constraints, I would rather keep scripts like this as scripts.
>
> I agree with Junio; I think the scripts are much more readable and
> easier to understand; In fact, it would be nice if the script were
> preserved somewhere, perhaps as comments in the .c file.
>

Or move them to contrib/examples, as was done with git-resolve.sh.

Santi

Re: [PATCH] Make gc a builtin.

[Junio C Hamano]

"Santi Béjar" <sbejar@gmail.com> writes:

> On 3/14/07, Theodore Tso <tytso@mit.edu> wrote:
>> On Wed, Mar 14, 2007 at 12:19:07AM -0700, Junio C Hamano wrote:
>> > >> -test "true" != "$pack_refs" ||
>> > >> -git-pack-refs --prune &&
>> > >> -git-reflog expire --all &&
>> > >> -git-repack -a -d -l &&
>> > >> ...
>> >
>> > I do not necessarily think so.  This is not even a performance
>> > critical part of the system, so if there _were_ no other
>> > constraints, I would rather keep scripts like this as scripts.
>>
>> I agree with Junio; I think the scripts are much more readable and
>> easier to understand; In fact, it would be nice if the script were
>> preserved somewhere, perhaps as comments in the .c file.
>>
>
> Or move them to contrib/examples, as was done with git-resolve.sh.
>
> Santi

That probably is a sensible thing to do.  I'll amend the patch.

Re: [PATCH] Make gc a builtin.

[Andy Parkins]

On Wednesday 2007 March 14 07:44, Theodore Tso wrote:

> I agree with Junio; I think the scripts are much more readable and
> easier to understand; In fact, it would be nice if the script were
> preserved somewhere, perhaps as comments in the .c file.

If only there were some tool that would keep collections of files as a 
snapshotted whole and allow us to browse the history of those snapshots in 
some sort of connected graph, with each snapshot being given some sort of 
unique ID.  Then we could simply refer to that unique ID when we wanted to 
tell someone about a particular historical instance.

:-)



Andy
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: [PATCH] Make gc a builtin.

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> On Wednesday 2007 March 14 07:44, Theodore Tso wrote:
>
>> I agree with Junio; I think the scripts are much more readable and
>> easier to understand; In fact, it would be nice if the script were
>> preserved somewhere, perhaps as comments in the .c file.
>
> If only there were some tool that would keep collections of files as a 
> snapshotted whole and allow us to browse the history of those snapshots in 
> some sort of connected graph, with each snapshot being given some sort of 
> unique ID.  Then we could simply refer to that unique ID when we wanted to 
> tell someone about a particular historical instance.
>
> :-)

There is a difference between having a readily greppable and
lessable copy handy to study at your own initiative, and being
able to retrieve to review only after being told.

You could argue that we can all do that with git-grep and
git-less ;-).

Re: [PATCH] Make gc a builtin.

[Andy Parkins]

On Wednesday 2007 March 14 11:12, Junio C Hamano wrote:

> There is a difference between having a readily greppable and
> lessable copy handy to study at your own initiative, and being
> able to retrieve to review only after being told.

Well I was only joking really.

> You could argue that we can all do that with git-grep and
> git-less ;-).

Definitely.  git is so good at this sort of stuff that encouraging the 
retention of commented out code is just filling up source files with junk.

In the old days, before version control, I would often have files with

#if 0
// This is how I used to do it
// ...
#endif

These days I comment it out, then after a few successful commits it gets 
removed from the source file.  Git makes my code cleaner and clearer as it's 
not filled with obsolete junk.  I am always secure in the knowledge that I 
can go back and look if I want.  The same is true, I think, for shell script 
replaced with C code.

What will you do if in the future the C gets a feature that wasn't in the 
shell code - should the shell code be updated?  If you don't then the comment 
is a lie, if you do then it's a maintenance nightmare.

Chuck it and be happy it's chucked.  A rule for life.  I should write fortune 
cookies.



Andy
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: [PATCH] Make gc a builtin.

[Johannes Schindelin]

Hi,

On Wed, 14 Mar 2007, Junio C Hamano wrote:

> Andy Parkins <andyparkins@gmail.com> writes:
> 
> > On Wednesday 2007 March 14 07:44, Theodore Tso wrote:
> >
> >> I agree with Junio; I think the scripts are much more readable and
> >> easier to understand; In fact, it would be nice if the script were
> >> preserved somewhere, perhaps as comments in the .c file.
> >
> > If only there were some tool that would keep collections of files as a 
> > snapshotted whole and allow us to browse the history of those snapshots in 
> > some sort of connected graph, with each snapshot being given some sort of 
> > unique ID.  Then we could simply refer to that unique ID when we wanted to 
> > tell someone about a particular historical instance.
> >
> > :-)
> 
> There is a difference between having a readily greppable and
> lessable copy handy to study at your own initiative, and being
> able to retrieve to review only after being told.
> 
> You could argue that we can all do that with git-grep and
> git-less ;-).

Not to forget git-checkout.

But I like the idea of contrib/examples/. Why not put more stuff there, 
instead of clinging onto scripts for core-git? The purpose of 
contrib/examples/ is to provide easy samples, and the purpose of core-git 
is _not_ to provide easy examples, but a consistent and portable set of 
programs.

Als, when reading Git's scripts, I often think

- wow, what a different style from my one, and
- would locking not be a nice thing?

But I guess that now that the King Penguin spoke, I no longer have to 
argue for more builtins, even if they are trivial. (Who knows, maybe we 
can ship _one_ program, which is then hard linked to git-*, soon?)

Ciao,
Dscho