# [PATCH] Make gc a builtin. 10 messages from 2007-03-14 to 2007-03-14. Participants: James Bowes, Shawn O. Pearce, Junio C Hamano, Theodore Tso, Santi Béjar, Andy Parkins, Johannes Schindelin. Thread: https://gitlist.dev/t/7242 ## James Bowes, 2007-03-14 01:58 Subject: [PATCH] Make gc a builtin. Message-ID: <11738375021267-git-send-email-jbowes@dangerouslyinc.com> URL: https://gitlist.dev/e/11738375021267-git-send-email-jbowes%40dangerouslyinc.com ``` Signed-off-by: James Bowes --- On 3/13/07, Johannes Schindelin 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 ``` ## Shawn O. Pearce, 2007-03-14 06:07 Subject: Re: [PATCH] Make gc a builtin. Message-ID: <20070314060727.GC20978@spearce.org> URL: https://gitlist.dev/e/20070314060727.GC20978%40spearce.org In-Reply-To: <11738375021267-git-send-email-jbowes@dangerouslyinc.com> ``` James Bowes wrote: > Signed-off-by: James Bowes 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. ``` ## Junio C Hamano, 2007-03-14 07:19 Subject: Re: [PATCH] Make gc a builtin. Message-ID: <7vodmwfg2c.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vodmwfg2c.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20070314060727.GC20978@spearce.org> ``` "Shawn O. Pearce" writes: > James Bowes wrote: >> Signed-off-by: James Bowes > > 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. ``` ## Theodore Tso, 2007-03-14 07:44 Subject: Re: [PATCH] Make gc a builtin. Message-ID: <20070314074440.GC12710@thunk.org> URL: https://gitlist.dev/e/20070314074440.GC12710%40thunk.org In-Reply-To: <7vodmwfg2c.fsf@assigned-by-dhcp.cox.net> ``` 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 ``` ## Santi Béjar, 2007-03-14 07:55 Subject: Re: [PATCH] Make gc a builtin. Message-ID: <8aa486160703140055s2b03c2aeu1c85dedac59b664b@mail.gmail.com> URL: https://gitlist.dev/e/8aa486160703140055s2b03c2aeu1c85dedac59b664b%40mail.gmail.com In-Reply-To: <20070314074440.GC12710@thunk.org> ``` On 3/14/07, Theodore Tso 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 ``` ## Junio C Hamano, 2007-03-14 09:29 Subject: Re: [PATCH] Make gc a builtin. Message-ID: <7vodmwcgvc.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vodmwcgvc.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <8aa486160703140055s2b03c2aeu1c85dedac59b664b@mail.gmail.com> ``` "Santi Béjar" writes: > On 3/14/07, Theodore Tso 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. ``` ## Andy Parkins, 2007-03-14 10:45 Subject: Re: [PATCH] Make gc a builtin. Message-ID: <200703141045.58739.andyparkins@gmail.com> URL: https://gitlist.dev/e/200703141045.58739.andyparkins%40gmail.com In-Reply-To: <20070314074440.GC12710@thunk.org> ``` 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 ``` ## Junio C Hamano, 2007-03-14 11:12 Subject: Re: [PATCH] Make gc a builtin. Message-ID: <7vps7caxjb.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vps7caxjb.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <200703141045.58739.andyparkins@gmail.com> ``` Andy Parkins 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 ;-). ``` ## Andy Parkins, 2007-03-14 11:48 Subject: Re: [PATCH] Make gc a builtin. Message-ID: <200703141148.12364.andyparkins@gmail.com> URL: https://gitlist.dev/e/200703141148.12364.andyparkins%40gmail.com In-Reply-To: <7vps7caxjb.fsf@assigned-by-dhcp.cox.net> ``` 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 ``` ## Johannes Schindelin, 2007-03-14 12:19 Subject: Re: [PATCH] Make gc a builtin. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0703141314140.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <7vps7caxjb.fsf@assigned-by-dhcp.cox.net> ``` Hi, On Wed, 14 Mar 2007, Junio C Hamano wrote: > Andy Parkins 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 ```