James BowesThe 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
James Bowes[PATCH 1/2] run-command: Make run_command_va_opt public and add run_command_va
Signed-off-by: James Bowes <jbowes@dangerouslyinc.com> --- run-command.c | 7 ++++++- run-command.h | 2 ++ 2 files changed, 8 insertions(+), 1 deletions(-)
run-command.c, run-command.h
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
James Bowes[PATCH 2/2] Make gc a builtin.
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
Makefile, builtin-gc.c, builtin.h, git-gc.sh, git.c
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
Johannes SchindelinRe: [PATCH 2/2] Make gc a builtin.
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
Junio C HamanoRe: [PATCH 2/2] Make gc a builtin.
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;
James Bowes[PATCH] Make gc a builtin.
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 failurefor 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
Makefile, builtin-gc.c, builtin.h, git-gc.sh, git.c
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
Shawn O. PearceRe: [PATCH] Make gc a builtin.
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.
Johannes SchindelinRe: [PATCH 2/2] Make gc a builtin.
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
Theodore TsoRe: [PATCH 0/2] Make gc a builtin.
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
Johannes SchindelinRe: [PATCH 0/2] Make gc a builtin.
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
Theodore TsoRe: [PATCH 0/2] Make gc a builtin.
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
Linus TorvaldsRe: [PATCH 0/2] Make gc a builtin.
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
Jakub NarebskiRe: [PATCH 0/2] Make gc a builtin.
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
Linus TorvaldsRe: [PATCH 0/2] Make gc a builtin.
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
Shawn O. PearceRe: [PATCH 0/2] Make gc a builtin.
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.