{"thread":{"id":"7201","subject":"[PATCH 0/2] Make gc a builtin.","startedAt":"2007-03-11T22:06:56Z","lastAt":"2007-03-13T01:20:15Z","messageCount":15,"participants":["James Bowes","Johannes Schindelin","Junio C Hamano","Theodore Tso","Shawn O. Pearce","Linus Torvalds","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"36874","messageId":"11736508181273-git-send-email-jbowes@dangerouslyinc.com","threadId":"7201","inReplyTo":null,"subject":"[PATCH 0/2] Make gc a builtin.","fromName":"James Bowes","fromEmail":"jbowes@dangerouslyinc.com","sentAt":"2007-03-11T22:06:56Z","receivedAt":"2007-03-11T22:06:56Z","isPatch":true,"sender":{"key":"jbowes@dangerouslyinc.com","avatar":"https://gravatar.com/avatar/a2fe98c66b2b47a9fa9d2ba92ff949d54c3208b1f8acc2e745b4b84ae3c4483a?d=mp&s=160"},"body":"The following two patches make git-gc a builtin command.\n\nThe first patch modifies run-command.*, making two public functions that take\nva_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\nbuiltin-gc.c code itself.\n\n-James\n"},{"id":"36875","messageId":"11736508191143-git-send-email-jbowes@dangerouslyinc.com","threadId":"7201","inReplyTo":"11736508181273-git-send-email-jbowes@dangerouslyinc.com","subject":"[PATCH 1/2] run-command: Make run_command_va_opt public and add run_command_va","fromName":"James Bowes","fromEmail":"jbowes@dangerouslyinc.com","sentAt":"2007-03-11T22:06:57Z","receivedAt":"2007-03-11T22:06:57Z","isPatch":true,"sender":{"key":"jbowes@dangerouslyinc.com","avatar":"https://gravatar.com/avatar/a2fe98c66b2b47a9fa9d2ba92ff949d54c3208b1f8acc2e745b4b84ae3c4483a?d=mp&s=160"},"body":"Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>\n---\n run-command.c |    7 ++++++-\n run-command.h |    2 ++\n 2 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex cfbad74..34fc100 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -52,7 +52,7 @@ int run_command_v(const char **argv)\n \treturn run_command_v_opt(argv, 0);\n }\n \n-static int run_command_va_opt(int opt, const char *cmd, va_list param)\n+int run_command_va_opt(int opt, const char *cmd, va_list param)\n {\n \tint argc;\n \tconst char *argv[MAX_RUN_COMMAND_ARGS];\n@@ -70,6 +70,11 @@ static int run_command_va_opt(int opt, const char *cmd, va_list param)\n \treturn run_command_v_opt(argv, opt);\n }\n \n+int run_command_va(const char *cmd, va_list param)\n+{\n+\treturn run_command_va_opt(0, cmd, param);\n+}\n+\n int run_command_opt(int opt, const char *cmd, ...)\n {\n \tva_list params;\ndiff --git a/run-command.h b/run-command.h\nindex 59c4476..3934643 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -16,6 +16,8 @@ enum {\n #define RUN_COMMAND_STDOUT_TO_STDERR 4\n int run_command_v_opt(const char **argv, int opt);\n int run_command_v(const char **argv);\n+int run_command_va_opt(int opt, const char *cmd, va_list param);\n+int run_command_va(const char *cmd, va_list param);\n int run_command_opt(int opt, const char *cmd, ...);\n int run_command(const char *cmd, ...);\n \n-- \n1.5.0.2\n"},{"id":"36876","messageId":"1173650820969-git-send-email-jbowes@dangerouslyinc.com","threadId":"7201","inReplyTo":"11736508181273-git-send-email-jbowes@dangerouslyinc.com","subject":"[PATCH 2/2] Make gc a builtin.","fromName":"James Bowes","fromEmail":"jbowes@dangerouslyinc.com","sentAt":"2007-03-11T22:06:58Z","receivedAt":"2007-03-11T22:06:58Z","isPatch":true,"sender":{"key":"jbowes@dangerouslyinc.com","avatar":"https://gravatar.com/avatar/a2fe98c66b2b47a9fa9d2ba92ff949d54c3208b1f8acc2e745b4b84ae3c4483a?d=mp&s=160"},"body":"Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>\n---\n Makefile     |    3 +-\n builtin-gc.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n builtin.h    |    1 +\n git-gc.sh    |   37 --------------------------\n git.c        |    1 +\n 5 files changed, 85 insertions(+), 38 deletions(-)\n create mode 100644 builtin-gc.c\n delete mode 100755 git-gc.sh\n\ndiff --git a/Makefile b/Makefile\nindex f0fc2f8..fb17cfb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -177,7 +177,7 @@ BASIC_LDFLAGS =\n SCRIPT_SH = \\\n \tgit-bisect.sh git-checkout.sh \\\n \tgit-clean.sh git-clone.sh git-commit.sh \\\n-\tgit-fetch.sh git-gc.sh \\\n+\tgit-fetch.sh \\\n \tgit-ls-remote.sh \\\n \tgit-merge-one-file.sh git-parse-remote.sh \\\n \tgit-pull.sh git-rebase.sh \\\n@@ -297,6 +297,7 @@ BUILTIN_OBJS = \\\n \tbuiltin-fmt-merge-msg.o \\\n \tbuiltin-for-each-ref.o \\\n \tbuiltin-fsck.o \\\n+\tbuiltin-gc.o \\\n \tbuiltin-grep.o \\\n \tbuiltin-init-db.o \\\n \tbuiltin-log.o \\\ndiff --git a/builtin-gc.c b/builtin-gc.c\nnew file mode 100644\nindex 0000000..7a9332b\n--- /dev/null\n+++ b/builtin-gc.c\n@@ -0,0 +1,81 @@\n+/*\n+ * git gc builtin command\n+ *\n+ * Cleanup unreachable files and optimize the repository.\n+ *\n+ * Copyright (c) 2007 James Bowes\n+ *\n+ * Based on git-gc.sh, which is\n+ *\n+ * Copyright (c) 2006 Shawn O. Pearce\n+ */\n+\n+#include \"cache.h\"\n+#include \"run-command.h\"\n+\n+static const char builtin_gc_usage[] = \"git-gc [--prune]\";\n+\n+static int pack_refs;\n+\n+static int gc_config(const char *var, const char *value)\n+{\n+\tif (!strcmp(var, \"gc.packrefs\"))\n+\t\tif (strlen(value) == 0 || !strcmp(value, \"notbare\"))\n+\t\t\tpack_refs = !is_bare_repository();\n+\t\telse\n+\t\t\tpack_refs = git_config_bool(var, value);\n+\telse\n+\t\treturn git_default_config(var, value);\n+\treturn 0;\n+}\n+\n+static void run_command_or_die(const char *cmd, ...)\n+{\n+\tint err;\n+\tva_list params;\n+\n+\tva_start(params, cmd);\n+\terr = run_command_va(cmd, params);\n+\tva_end(params);\n+\n+\tswitch (err) {\n+\tcase 0:\n+\t\treturn;\n+\tcase -ERR_RUN_COMMAND_FORK:\n+\t\tdie(\"unable to fork for %s\", cmd);\n+\tcase -ERR_RUN_COMMAND_EXEC:\n+\t\tdie(\"unable to exec %s\", cmd);\n+\tdefault:\n+\t\tdie(\"%s died with strange error\", cmd);\n+\t}\n+}\n+\n+int cmd_gc(int argc, const char **argv, const char *prefix)\n+{\n+\tint i;\n+\tint prune = 0;\n+\n+\tgit_config(gc_config);\n+\n+\tfor (i = 1; i < argc; i++) {\n+\t\tconst char *arg = argv[i];\n+\t\tif (!strcmp(arg, \"--prune\")) {\n+\t\t\tprune = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\t/* perhaps other parameters later... */\n+\t\tbreak;\n+\t}\n+\tif (i != argc)\n+\t\tusage(builtin_gc_usage);\n+\n+\tif (pack_refs)\n+\t\trun_command_or_die(\"git-pack-refs\", \"--prune\", NULL);\n+\trun_command_or_die(\"git-reflog\", \"expire\", \"--all\", NULL);\n+\trun_command_or_die(\"git-repack\", \"-a\", \"-d\", \"-l\", NULL);\n+\tif (prune)\n+\t\trun_command_or_die(\"git-prune\", NULL);\n+\trun_command_or_die(\"git-rerere\", \"gc\", NULL);\n+\n+\treturn 0;\n+}\ndiff --git a/builtin.h b/builtin.h\nindex 1cb64b7..af203e9 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -37,6 +37,7 @@ extern int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix);\n extern int cmd_for_each_ref(int argc, const char **argv, const char *prefix);\n extern int cmd_format_patch(int argc, const char **argv, const char *prefix);\n extern int cmd_fsck(int argc, const char **argv, const char *prefix);\n+extern int cmd_gc(int argc, const char **argv, const char *prefix);\n extern int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix);\n extern int cmd_grep(int argc, const char **argv, const char *prefix);\n extern int cmd_help(int argc, const char **argv, const char *prefix);\ndiff --git a/git-gc.sh b/git-gc.sh\ndeleted file mode 100755\nindex 436d7ca..0000000\n--- a/git-gc.sh\n+++ /dev/null\n@@ -1,37 +0,0 @@\n-#!/bin/sh\n-#\n-# Copyright (c) 2006, Shawn O. Pearce\n-#\n-# Cleanup unreachable files and optimize the repository.\n-\n-USAGE='[--prune]'\n-SUBDIRECTORY_OK=Yes\n-. git-sh-setup\n-\n-no_prune=:\n-while case $# in 0) break ;; esac\n-do\n-\tcase \"$1\" in\n-\t--prune)\n-\t\tno_prune=\n-\t\t;;\n-\t--)\n-\t\tusage\n-\t\t;;\n-\tesac\n-\tshift\n-done\n-\n-case \"$(git config --get gc.packrefs)\" in\n-notbare|\"\")\n-\ttest $(is_bare_repository) = true || pack_refs=true;;\n-*)\n-\tpack_refs=$(git config --bool --get gc.packrefs)\n-esac\n-\n-test \"true\" != \"$pack_refs\" ||\n-git-pack-refs --prune &&\n-git-reflog expire --all &&\n-git-repack -a -d -l &&\n-$no_prune git-prune &&\n-git-rerere gc || exit\ndiff --git a/git.c b/git.c\nindex dde4d07..ed1c65e 100644\n--- a/git.c\n+++ b/git.c\n@@ -249,6 +249,7 @@ static void handle_internal_command(int argc, const char **argv, char **envp)\n \t\t{ \"format-patch\", cmd_format_patch, RUN_SETUP },\n \t\t{ \"fsck\", cmd_fsck, RUN_SETUP },\n \t\t{ \"fsck-objects\", cmd_fsck, RUN_SETUP },\n+\t\t{ \"gc\", cmd_gc, RUN_SETUP },\n \t\t{ \"get-tar-commit-id\", cmd_get_tar_commit_id },\n \t\t{ \"grep\", cmd_grep, RUN_SETUP | USE_PAGER },\n \t\t{ \"help\", cmd_help },\n-- \n1.5.0.2\n"},{"id":"36879","messageId":"Pine.LNX.4.63.0703112332550.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"7201","inReplyTo":"1173650820969-git-send-email-jbowes@dangerouslyinc.com","subject":"Re: [PATCH 2/2] Make gc a builtin.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-03-11T22:48:45Z","receivedAt":"2007-03-11T22:48:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 11 Mar 2007, James Bowes wrote:\n\n> +\tif (pack_refs)\n> +\t\trun_command_or_die(\"git-pack-refs\", \"--prune\", NULL);\n> +\trun_command_or_die(\"git-reflog\", \"expire\", \"--all\", NULL);\n> +\trun_command_or_die(\"git-repack\", \"-a\", \"-d\", \"-l\", NULL);\n> +\tif (prune)\n> +\t\trun_command_or_die(\"git-prune\", NULL);\n> +\trun_command_or_die(\"git-rerere\", \"gc\", NULL);\n\nShawn recently sent a series which discourages the va_list versions of \nrun_command. I think that makes sense. So, using \nrun_command_v_opt(argv_pack_refs, RUN_GIT_CMD) would be better IMHO.\n\nAnd instead of die()ing, I'd rather do something like\n\n\treturn (pack_refs || run_command_v_opt(argv_pack_refs, RUN_GIT_CMD) &&\n\t\trun_command_v_opt(argv_reflog_expire, RUN_GIT_CMD) &&\n\t\trun_command_v_opt(argv_repack, RUN_GIT_CMD) &&\n\t\t(prune || run_command_v_opt(argv_prune, RUN_GIT_CMD) &&\n\t\trun_command_v_opt(argv_rerere, RUN_GIT_CMD);\n\nHmm?\n\nCiao,\nDscho\n"},{"id":"36883","messageId":"7vtzwrtdmx.fsf@assigned-by-dhcp.cox.net","threadId":"7201","inReplyTo":"Pine.LNX.4.63.0703112332550.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 2/2] Make gc a builtin.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-12T02:11:02Z","receivedAt":"2007-03-12T02:11:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> And instead of die()ing, I'd rather do something like\n>\n> \treturn (pack_refs || run_command_v_opt(argv_pack_refs, RUN_GIT_CMD) &&\n> \t\trun_command_v_opt(argv_reflog_expire, RUN_GIT_CMD) &&\n> \t\trun_command_v_opt(argv_repack, RUN_GIT_CMD) &&\n> \t\t(prune || run_command_v_opt(argv_prune, RUN_GIT_CMD) &&\n> \t\trun_command_v_opt(argv_rerere, RUN_GIT_CMD);\n\nGaaaaaaaah.\n\nThat may be valid C, but please do that as a sequence of\nseparate statements.\n\n\tif (we are told to pack-refs)\n        \tif (try to pack refs and find error)\n\t\t\tgoto failure;\n\n\tif (try to reflog expire and find error)\n\t\tgoto failure;\n\n        ...\n\n\treturn Ok;\n\n\tfailure:\n\n\treturn Error;\n"},{"id":"36884","messageId":"3f80363f0703111951x9d88e74x8d7723af97c18c7@mail.gmail.com","threadId":"7201","inReplyTo":"7vtzwrtdmx.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] Make gc a builtin.","fromName":"James Bowes","fromEmail":"jbowes@dangerouslyinc.com","sentAt":"2007-03-12T02:51:18Z","receivedAt":"2007-03-12T02:51:18Z","isPatch":true,"sender":{"key":"jbowes@dangerouslyinc.com","avatar":"https://gravatar.com/avatar/a2fe98c66b2b47a9fa9d2ba92ff949d54c3208b1f8acc2e745b4b84ae3c4483a?d=mp&s=160"},"body":"Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>\n---\n\nThis patch replaces replaces my previous two. It uses\nrun_command_v_opt rather than changing run-command.*, and\n\nif (command_fails)\n    goto failure\n\nfor running the commands.\n\n-James\n\n Makefile     |    3 +-\n builtin-gc.c |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n builtin.h    |    1 +\n git-gc.sh    |   37 ---------------------------\n git.c        |    1 +\n 5 files changed, 82 insertions(+), 38 deletions(-)\n create mode 100644 builtin-gc.c\n delete mode 100755 git-gc.sh\n\ndiff --git a/Makefile b/Makefile\nindex f0fc2f8..fb17cfb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -177,7 +177,7 @@ BASIC_LDFLAGS =\n SCRIPT_SH = \\\n \tgit-bisect.sh git-checkout.sh \\\n \tgit-clean.sh git-clone.sh git-commit.sh \\\n-\tgit-fetch.sh git-gc.sh \\\n+\tgit-fetch.sh \\\n \tgit-ls-remote.sh \\\n \tgit-merge-one-file.sh git-parse-remote.sh \\\n \tgit-pull.sh git-rebase.sh \\\n@@ -297,6 +297,7 @@ BUILTIN_OBJS = \\\n \tbuiltin-fmt-merge-msg.o \\\n \tbuiltin-for-each-ref.o \\\n \tbuiltin-fsck.o \\\n+\tbuiltin-gc.o \\\n \tbuiltin-grep.o \\\n \tbuiltin-init-db.o \\\n \tbuiltin-log.o \\\ndiff --git a/builtin-gc.c b/builtin-gc.c\nnew file mode 100644\nindex 0000000..c507bdf\n--- /dev/null\n+++ b/builtin-gc.c\n@@ -0,0 +1,78 @@\n+/*\n+ * git gc builtin command\n+ *\n+ * Cleanup unreachable files and optimize the repository.\n+ *\n+ * Copyright (c) 2007 James Bowes\n+ *\n+ * Based on git-gc.sh, which is\n+ *\n+ * Copyright (c) 2006 Shawn O. Pearce\n+ */\n+\n+#include \"cache.h\"\n+#include \"run-command.h\"\n+\n+static const char builtin_gc_usage[] = \"git-gc [--prune]\";\n+\n+static int pack_refs;\n+\n+static const char *argv_pack_refs[] = {\"pack-refs\", \"--prune\", NULL};\n+static const char *argv_reflog[] = {\"reflog\", \"expire\", \"--all\", NULL};\n+static const char *argv_repack[] = {\"repack\", \"-a\", \"-d\", \"-l\", NULL};\n+static const char *argv_prune[] = {\"prune\", NULL};\n+static const char *argv_rerere[] = {\"rerere\", \"gc\", NULL};\n+\n+static int gc_config(const char *var, const char *value)\n+{\n+\tif (!strcmp(var, \"gc.packrefs\"))\n+\t\tif (strlen(value) == 0 || !strcmp(value, \"notbare\"))\n+\t\t\tpack_refs = !is_bare_repository();\n+\t\telse\n+\t\t\tpack_refs = git_config_bool(var, value);\n+\telse\n+\t\treturn git_default_config(var, value);\n+\treturn 0;\n+}\n+\n+int cmd_gc(int argc, const char **argv, const char *prefix)\n+{\n+\tint i;\n+\tint prune = 0;\n+\n+\tgit_config(gc_config);\n+\n+\tfor (i = 1; i < argc; i++) {\n+\t\tconst char *arg = argv[i];\n+\t\tif (!strcmp(arg, \"--prune\")) {\n+\t\t\tprune = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\t/* perhaps other parameters later... */\n+\t\tbreak;\n+\t}\n+\tif (i != argc)\n+\t\tusage(builtin_gc_usage);\n+\n+    if (pack_refs)\n+\t    if (run_command_v_opt(argv_pack_refs, RUN_GIT_CMD))\n+            goto failure;\n+\n+    if (run_command_v_opt(argv_reflog, RUN_GIT_CMD))\n+        goto failure;\n+\n+    if (run_command_v_opt(argv_repack, RUN_GIT_CMD))\n+        goto failure;\n+\n+    if (prune)\n+        if (run_command_v_opt(argv_prune, RUN_GIT_CMD))\n+            goto failure;\n+\n+    if (run_command_v_opt(argv_rerere, RUN_GIT_CMD))\n+        goto failure;\n+\n+    return 0;\n+\n+failure:\n+    return -1;\n+}\ndiff --git a/builtin.h b/builtin.h\nindex 1cb64b7..af203e9 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -37,6 +37,7 @@ extern int cmd_fmt_merge_msg(int argc, const char\n**argv, const char *prefix);\n extern int cmd_for_each_ref(int argc, const char **argv, const char *prefix);\n extern int cmd_format_patch(int argc, const char **argv, const char *prefix);\n extern int cmd_fsck(int argc, const char **argv, const char *prefix);\n+extern int cmd_gc(int argc, const char **argv, const char *prefix);\n extern int cmd_get_tar_commit_id(int argc, const char **argv, const\nchar *prefix);\n extern int cmd_grep(int argc, const char **argv, const char *prefix);\n extern int cmd_help(int argc, const char **argv, const char *prefix);\ndiff --git a/git-gc.sh b/git-gc.sh\ndeleted file mode 100755\nindex 436d7ca..0000000\n--- a/git-gc.sh\n+++ /dev/null\n@@ -1,37 +0,0 @@\n-#!/bin/sh\n-#\n-# Copyright (c) 2006, Shawn O. Pearce\n-#\n-# Cleanup unreachable files and optimize the repository.\n-\n-USAGE='[--prune]'\n-SUBDIRECTORY_OK=Yes\n-. git-sh-setup\n-\n-no_prune=:\n-while case $# in 0) break ;; esac\n-do\n-\tcase \"$1\" in\n-\t--prune)\n-\t\tno_prune=\n-\t\t;;\n-\t--)\n-\t\tusage\n-\t\t;;\n-\tesac\n-\tshift\n-done\n-\n-case \"$(git config --get gc.packrefs)\" in\n-notbare|\"\")\n-\ttest $(is_bare_repository) = true || pack_refs=true;;\n-*)\n-\tpack_refs=$(git config --bool --get gc.packrefs)\n-esac\n-\n-test \"true\" != \"$pack_refs\" ||\n-git-pack-refs --prune &&\n-git-reflog expire --all &&\n-git-repack -a -d -l &&\n-$no_prune git-prune &&\n-git-rerere gc || exit\ndiff --git a/git.c b/git.c\nindex dde4d07..ed1c65e 100644\n--- a/git.c\n+++ b/git.c\n@@ -249,6 +249,7 @@ static void handle_internal_command(int argc,\nconst char **argv, char **envp)\n \t\t{ \"format-patch\", cmd_format_patch, RUN_SETUP },\n \t\t{ \"fsck\", cmd_fsck, RUN_SETUP },\n \t\t{ \"fsck-objects\", cmd_fsck, RUN_SETUP },\n+\t\t{ \"gc\", cmd_gc, RUN_SETUP },\n \t\t{ \"get-tar-commit-id\", cmd_get_tar_commit_id },\n \t\t{ \"grep\", cmd_grep, RUN_SETUP | USE_PAGER },\n \t\t{ \"help\", cmd_help },\n-- \n1.5.0.2\n"},{"id":"36887","messageId":"20070312025736.GA28505@thunk.org","threadId":"7201","inReplyTo":"11736508181273-git-send-email-jbowes@dangerouslyinc.com","subject":"Re: [PATCH 0/2] Make gc a builtin.","fromName":"Theodore Tso","fromEmail":"tytso@mit.edu","sentAt":"2007-03-12T02:57:36Z","receivedAt":"2007-03-12T02:57:36Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Sun, Mar 11, 2007 at 06:06:56PM -0400, James Bowes wrote:\n> The following two patches make git-gc a builtin command.\n\nWhat's the advantage in making git-gc a builtin command?  It's not\nlike it's going to help performance a whole lot (especially since\nyou're just forking separate processes to run git-prune,\ngit-pack-refs, et.al.), and as a shell script it's a lot easier to\nexplain to people what git-gc is actually doing, so there is\npedagogical value to keeping it as a shell script.\n\nRegards,\n\n\t\t\t\t\t\t- Ted\n"},{"id":"36886","messageId":"Pine.LNX.4.63.0703120403360.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"7201","inReplyTo":"7vtzwrtdmx.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/2] Make gc a builtin.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-03-12T03:07:43Z","receivedAt":"2007-03-12T03:07:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 11 Mar 2007, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > And instead of die()ing, I'd rather do something like\n> >\n> > \treturn (pack_refs || run_command_v_opt(argv_pack_refs, RUN_GIT_CMD) &&\n> > \t\trun_command_v_opt(argv_reflog_expire, RUN_GIT_CMD) &&\n> > \t\trun_command_v_opt(argv_repack, RUN_GIT_CMD) &&\n> > \t\t(prune || run_command_v_opt(argv_prune, RUN_GIT_CMD) &&\n> > \t\trun_command_v_opt(argv_rerere, RUN_GIT_CMD);\n> \n> Gaaaaaaaah.\n> \n> That may be valid C,\n\nActually, it is not. As usual, I fscked up: run_command_v_opt() is \nsupposed to return 0 on _success_, so all the \"&&\" should be \"||\", and all \nthe \"||\" should be \"&&\".\n\n> \tif (we are told to pack-refs)\n>         \tif (try to pack refs and find error)\n> \t\t\tgoto failure;\n\nI find this not very elegant. Instead, I'd do\n\n\tif (do_pack_refs && run_comand_v_opt(argv_pack_refs, RUN_GIT_CMD))\n\t\treturn error(\"Could not run pack-refs.\");\n\nIt does not only avoid the evil goto, but uses the screen estate for some \nnice error messages so that the user is not left out in the cold when \nsomething is wrong.\n\nCiao,\nDscho\n"},{"id":"36897","messageId":"Pine.LNX.4.63.0703121222350.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"7201","inReplyTo":"20070312025736.GA28505@thunk.org","subject":"Re: [PATCH 0/2] Make gc a builtin.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-03-12T11:23:41Z","receivedAt":"2007-03-12T11:23:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 11 Mar 2007, Theodore Tso wrote:\n\n> On Sun, Mar 11, 2007 at 06:06:56PM -0400, James Bowes wrote:\n> > The following two patches make git-gc a builtin command.\n> \n> What's the advantage in making git-gc a builtin command?\n\nPortability. Plus, James wanted to get involved in Git development, and \nbuilding in gc really was the shortest path into that.\n\nCiao,\nDscho\n"},{"id":"36906","messageId":"20070312133612.GD4372@thunk.org","threadId":"7201","inReplyTo":"Pine.LNX.4.63.0703121222350.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 0/2] Make gc a builtin.","fromName":"Theodore Tso","fromEmail":"tytso@mit.edu","sentAt":"2007-03-12T13:36:12Z","receivedAt":"2007-03-12T13:36:12Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Mon, Mar 12, 2007 at 12:23:41PM +0100, Johannes Schindelin wrote:\n> Hi,\n> \n> On Sun, 11 Mar 2007, Theodore Tso wrote:\n> \n> > On Sun, Mar 11, 2007 at 06:06:56PM -0400, James Bowes wrote:\n> > > The following two patches make git-gc a builtin command.\n> > \n> > What's the advantage in making git-gc a builtin command?\n> \n> Portability. Plus, James wanted to get involved in Git development, and \n> building in gc really was the shortest path into that.\n> \n\nI'm not sure I understand the portability argument?  All of the\nplatforms that git currently supports will handle shell scripts,\nright?  \n\nHeck, git-commit is still a shell script, and that's a rather, ah,\nfundamental command, isn't it? \n\n\t\t\t\t\t\t- Ted\n"},{"id":"36911","messageId":"20070312142936.GD15150@spearce.org","threadId":"7201","inReplyTo":"Pine.LNX.4.63.0703121222350.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 0/2] Make gc a builtin.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-03-12T14:29:36Z","receivedAt":"2007-03-12T14:29:36Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> On Sun, 11 Mar 2007, Theodore Tso wrote:\n> \n> > On Sun, Mar 11, 2007 at 06:06:56PM -0400, James Bowes wrote:\n> > > The following two patches make git-gc a builtin command.\n> > \n> > What's the advantage in making git-gc a builtin command?\n> \n> Portability. Plus, James wanted to get involved in Git development, and \n> building in gc really was the shortest path into that.\n\nActually, git-gc.sh is pretty portable.  To POSIX systems.\nWindows ain't POSIX.  Getting rid of some of those shell scripts\njust makes us more portable, even to Windows.  (Yes, people really\ndo still get forced to use that non-operating system.)\n\nTed talked about git-commit.sh being more important, but Dsco\nclipped it. ;-)\n\nI think git-commit.sh and git-merge.sh should both get ported to\nbuiltins too, as both are somewhat hairy in shell, are quite core\nto the system, and would be faster on Windows if written in C\n(less forking == more speed there).\n\nBut they are so core that any rewrite must be undertaken carefully.\n\n-- \nShawn.\n"},{"id":"36912","messageId":"20070312144312.GE15150@spearce.org","threadId":"7201","inReplyTo":"3f80363f0703111951x9d88e74x8d7723af97c18c7@mail.gmail.com","subject":"Re: [PATCH] Make gc a builtin.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-03-12T14:43:12Z","receivedAt":"2007-03-12T14:43:12Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"A good (second) try.\n\nJames Bowes <jbowes@dangerouslyinc.com> wrote:\n> diff --git a/builtin-gc.c b/builtin-gc.c\n> +\n> +static int pack_refs;\n\nActually I think you want to use:\n\nstatic int pack_refs = -1;\n\nSee below for why...\n\n> +static int gc_config(const char *var, const char *value)\n> +{\n> +\tif (!strcmp(var, \"gc.packrefs\"))\n> +\t\tif (strlen(value) == 0 || !strcmp(value, \"notbare\"))\n> +\t\t\tpack_refs = !is_bare_repository();\n> +\t\telse\n> +\t\t\tpack_refs = git_config_bool(var, value);\n> +\telse\n> +\t\treturn git_default_config(var, value);\n> +\treturn 0;\n> +}\n\nGaaah.  How about some curly braces around the then part of that\nfirst if?\n\nActually, we typically just write this more like:\n\nstatic int gc_config(const char *var, const char *value)\n{\n\tif (!strcmp(var, \"gc.packrefs\")) {\n\t\tif (!strcmp(value, \"notbare\"))\n\t\t\tpack_refs = -1;\n\t\telse\n\t\t\tpack_refs = git_config_bool(var, value);\n\t}\n\treturn git_default_config(var, value);\n}\n\n> +int cmd_gc(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint i;\n> +\tint prune = 0;\n> +\n> +\tgit_config(gc_config);\n\nif (pack_refs < 0)\n\tpack_refs = !is_bare_repository();\n\nThe is_bare_repository function guesses until the configuration\nis done parsing; once the configuration has been parsed it has a\ndefinate answer one way or the other.  So what I'm suggesting you\ndo here is set pack_refs = -1 to mean use the is_bare_repository\nsetting, otherwise it stays what it was set to.\n\n> +    if (pack_refs)\n> +\t    if (run_command_v_opt(argv_pack_refs, RUN_GIT_CMD))\n> +            goto failure;\n....\n> +    if (prune)\n> +        if (run_command_v_opt(argv_prune, RUN_GIT_CMD))\n> +            goto failure;\n\nGaah.  Tabs-vs-spaces, not to mention that these aren't even lining\nup the same way.  I too prefer what Dsco suggested already:\n\n\tif (prune && run_command_v_opt(argv_prune, RUN_GIT_CMD))\n\t\treturn error(\"failed to run %s\", argv_prune[0]);\n\n-- \nShawn.\n"},{"id":"36939","messageId":"Pine.LNX.4.64.0703121202560.9690@woody.linux-foundation.org","threadId":"7201","inReplyTo":"20070312133612.GD4372@thunk.org","subject":"Re: [PATCH 0/2] Make gc a builtin.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-03-12T19:14:04Z","receivedAt":"2007-03-12T19:14:04Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 12 Mar 2007, Theodore Tso wrote:\n> \n> I'm not sure I understand the portability argument?  All of the\n> platforms that git currently supports will handle shell scripts,\n> right?  \n\nGit \"supports\" MinGW, or at least wants to. And yes, you can put bash in \nthere, but we'd be *so* much better off if we had no shell scripting at \nall.\n\nAnother thing I find annoying (even as a UNIX user) is that whenever I do \nany tracing for performance data, shell is absolutely horrid. It's *so* \nmuch nicer to do 'strace' on built-in programs that it's not even funny.\n\nIt's also sad how many performance issues we've had with shell, just \nbecause even something really simple (like a few hundred refs) is just too \nslow for shell scripting.\n\n> Heck, git-commit is still a shell script, and that's a rather, ah,\n> fundamental command, isn't it? \n\nYeah, and that's probably my pet peeve. I'd love to see a built-in \"git \ncommit\" and \"git fetch\". The \"fetch--tool\" thing in next gets rid of some \nof the latter (and apparently the worst performance problems), but it's \nsad how we have a really nice builtin \"push\", but our \"fetch\" is still \nmostly really hairy shell-code (not just \"git-fetch.sh\" itself, but \n\"git-parse-remote.sh\".\n\nA gold star for whoever gets rid of any of of commit/clone/fetch or \nls-remote\n\n(ls-remote isn't that big or hairy, but I mention it because it's a user \nof \"parse-remote\", so making even just ls-remote built-in is probably \ngoing to help with fetch/clone eventually).\n\n\t\tLinus\n"},{"id":"36978","messageId":"et4s76$9u3$1@sea.gmane.org","threadId":"7201","inReplyTo":"Pine.LNX.4.64.0703121202560.9690@woody.linux-foundation.org","subject":"Re: [PATCH 0/2] Make gc a builtin.","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-03-13T00:48:09Z","receivedAt":"2007-03-13T00:48:09Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Linus Torvalds wrote:\n\n> Another thing I find annoying (even as a UNIX user) is that whenever I do \n> any tracing for performance data, shell is absolutely horrid. It's *so* \n> much nicer to do 'strace' on built-in programs that it's not even funny.\n\nIsn't that what GIT_TRACE was made for?\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"36982","messageId":"Pine.LNX.4.64.0703121815411.9690@woody.linux-foundation.org","threadId":"7201","inReplyTo":"et4s76$9u3$1@sea.gmane.org","subject":"Re: [PATCH 0/2] Make gc a builtin.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-03-13T01:20:15Z","receivedAt":"2007-03-13T01:20:15Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 13 Mar 2007, Jakub Narebski wrote:\n\n> Linus Torvalds wrote:\n> \n> > Another thing I find annoying (even as a UNIX user) is that whenever I do \n> > any tracing for performance data, shell is absolutely horrid. It's *so* \n> > much nicer to do 'strace' on built-in programs that it's not even funny.\n> \n> Isn't that what GIT_TRACE was made for?\n\nThat just shows the high-level git commands.\n\nIf you look for performance issues or correctness issues (like when I \ntried to figure out if O_LARGEFILE was set for \"git clone\"), GIT_TRACE \ndoes nothing. You want to do \"strace -f -o trace-file\".\n\nAnd shell scripts look horrible there, and make it much harder to follow \nthings. In fact, it doesn't even need to be shell per se, but fork/exec \nalready makes things harder to see, shell just tends to (a) make it even \nmore so (try stracing though a shell startup, ugh) and (b) cause tons of \nfork/exec cases.\n\nFor example, when we made patch generation a built-in, it suddenly became \n*hugely* easier to follow what was going on in the traces, because it got \nmuch more streamlined. In general I find that \"high performance\" == \"easy \nto trace\".\n\n\t\tLinus\n"}]}