{"thread":{"id":"34935","subject":"[PATCH 2/3] repack: retain the return value of pack-objects","startedAt":"2013-09-15T15:33:20Z","lastAt":"2013-09-17T20:25:14Z","messageCount":8,"participants":["Stefan Beller","Ramkumar Ramachandra","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"227697","messageId":"1379259202-5042-1-git-send-email-stefanbeller@googlemail.com","threadId":"34935","inReplyTo":null,"subject":"[PATCH 1/3] repack: rewrite the shell script in C","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-09-15T15:33:20Z","receivedAt":"2013-09-15T15:33:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The motivation of this patch is to get closer to a goal of being\nable to have a core subset of git functionality built in to git.\nThat would mean\n\n * people on Windows could get a copy of at least the core parts\n   of Git without having to install a Unix-style shell\n\n * people using git in on servers with chrooted environments\n   do not need to worry about standard tools lacking for shell\n   scripts.\n\nThis patch is meant to be mostly a literal translation of the\ngit-repack script; the intent is that later patches would start using\nmore library facilities, but this patch is meant to be as close to a\nno-op as possible so it doesn't do that kind of thing.\n\nSigned-off-by: Stefan Beller <stefanbeller@googlemail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Makefile                       |   2 +-\n builtin.h                      |   1 +\n builtin/repack.c               | 387 +++++++++++++++++++++++++++++++++++++++++\n contrib/examples/git-repack.sh | 194 +++++++++++++++++++++\n git-repack.sh                  | 194 ---------------------\n git.c                          |   1 +\n 6 files changed, 584 insertions(+), 195 deletions(-)\n create mode 100644 builtin/repack.c\n create mode 100755 contrib/examples/git-repack.sh\n delete mode 100755 git-repack.sh\n\ndiff --git a/Makefile b/Makefile\nindex 3588ca1..69e5267 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -464,7 +464,6 @@ SCRIPT_SH += git-pull.sh\n SCRIPT_SH += git-quiltimport.sh\n SCRIPT_SH += git-rebase.sh\n SCRIPT_SH += git-remote-testgit.sh\n-SCRIPT_SH += git-repack.sh\n SCRIPT_SH += git-request-pull.sh\n SCRIPT_SH += git-stash.sh\n SCRIPT_SH += git-submodule.sh\n@@ -971,6 +970,7 @@ BUILTIN_OBJS += builtin/reflog.o\n BUILTIN_OBJS += builtin/remote.o\n BUILTIN_OBJS += builtin/remote-ext.o\n BUILTIN_OBJS += builtin/remote-fd.o\n+BUILTIN_OBJS += builtin/repack.o\n BUILTIN_OBJS += builtin/replace.o\n BUILTIN_OBJS += builtin/rerere.o\n BUILTIN_OBJS += builtin/reset.o\ndiff --git a/builtin.h b/builtin.h\nindex 8afa2de..b56cf07 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -102,6 +102,7 @@ extern int cmd_reflog(int argc, const char **argv, const char *prefix);\n extern int cmd_remote(int argc, const char **argv, const char *prefix);\n extern int cmd_remote_ext(int argc, const char **argv, const char *prefix);\n extern int cmd_remote_fd(int argc, const char **argv, const char *prefix);\n+extern int cmd_repack(int argc, const char **argv, const char *prefix);\n extern int cmd_repo_config(int argc, const char **argv, const char *prefix);\n extern int cmd_rerere(int argc, const char **argv, const char *prefix);\n extern int cmd_reset(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/repack.c b/builtin/repack.c\nnew file mode 100644\nindex 0000000..a15bd9b\n--- /dev/null\n+++ b/builtin/repack.c\n@@ -0,0 +1,387 @@\n+#include \"builtin.h\"\n+#include \"cache.h\"\n+#include \"dir.h\"\n+#include \"parse-options.h\"\n+#include \"run-command.h\"\n+#include \"sigchain.h\"\n+#include \"strbuf.h\"\n+#include \"string-list.h\"\n+#include \"argv-array.h\"\n+\n+static int delta_base_offset = 1;\n+static char *packdir, *packtmp;\n+\n+static const char *const git_repack_usage[] = {\n+\tN_(\"git repack [options]\"),\n+\tNULL\n+};\n+\n+static int repack_config(const char *var, const char *value, void *cb)\n+{\n+\tif (!strcmp(var, \"repack.usedeltabaseoffset\")) {\n+\t\tdelta_base_offset = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\treturn git_default_config(var, value, cb);\n+}\n+\n+/*\n+ * Remove temporary $GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-* files.\n+ */\n+static void remove_temporary_files(void)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tsize_t dirlen, prefixlen;\n+\tDIR *dir;\n+\tstruct dirent *e;\n+\n+\tdir = opendir(packdir);\n+\tif (!dir)\n+\t\treturn;\n+\n+\t/* Point at the slash at the end of \".../objects/pack/\" */\n+\tdirlen = strlen(packdir) + 1;\n+\tstrbuf_addstr(&buf, packtmp);\n+\t/* Hold the length of  \".tmp-%d-pack-\" */\n+\tprefixlen = buf.len - dirlen;\n+\n+\twhile ((e = readdir(dir))) {\n+\t\tif (strncmp(e->d_name, buf.buf + dirlen, prefixlen))\n+\t\t\tcontinue;\n+\t\tstrbuf_setlen(&buf, dirlen);\n+\t\tstrbuf_addstr(&buf, e->d_name);\n+\t\tunlink(buf.buf);\n+\t}\n+\tclosedir(dir);\n+\tstrbuf_release(&buf);\n+}\n+\n+static void remove_pack_on_signal(int signo)\n+{\n+\tremove_temporary_files();\n+\tsigchain_pop(signo);\n+\traise(signo);\n+}\n+\n+/*\n+ * Adds all packs hex strings to the fname list, which do not\n+ * have a corresponding .keep file.\n+ */\n+static void get_non_kept_pack_filenames(struct string_list *fname_list)\n+{\n+\tDIR *dir;\n+\tstruct dirent *e;\n+\tchar *fname;\n+\tsize_t len;\n+\n+\tif (!(dir = opendir(packdir)))\n+\t\treturn;\n+\n+\twhile ((e = readdir(dir)) != NULL) {\n+\t\tif (suffixcmp(e->d_name, \".pack\"))\n+\t\t\tcontinue;\n+\n+\t\tlen = strlen(e->d_name) - strlen(\".pack\");\n+\t\tfname = xmemdupz(e->d_name, len);\n+\n+\t\tif (!file_exists(mkpath(\"%s/%s.keep\", packdir, fname)))\n+\t\t\tstring_list_append_nodup(fname_list, fname);\n+\t\telse\n+\t\t\tfree(fname);\n+\t}\n+\tclosedir(dir);\n+}\n+\n+static void remove_redundant_pack(const char *path_prefix, const char *hex)\n+{\n+\tconst char *exts[] = {\".pack\", \".idx\", \".keep\"};\n+\tint i;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tsize_t plen;\n+\n+\tstrbuf_addf(&buf, \"%s/%s\", path_prefix, hex);\n+\tplen = buf.len;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(exts); i++) {\n+\t\tstrbuf_setlen(&buf, plen);\n+\t\tstrbuf_addstr(&buf, exts[i]);\n+\t\tunlink(buf.buf);\n+\t}\n+\tstrbuf_release(&buf);\n+}\n+\n+#define ALL_INTO_ONE 1\n+#define LOOSEN_UNREACHABLE 2\n+\n+int cmd_repack(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *exts[2] = {\".pack\", \".idx\"};\n+\tstruct child_process cmd;\n+\tstruct string_list_item *item;\n+\tstruct argv_array cmd_args = ARGV_ARRAY_INIT;\n+\tstruct string_list names = STRING_LIST_INIT_DUP;\n+\tstruct string_list rollback = STRING_LIST_INIT_NODUP;\n+\tstruct string_list existing_packs = STRING_LIST_INIT_DUP;\n+\tstruct strbuf line = STRBUF_INIT;\n+\tint nr_packs, ext, ret, failed;\n+\tFILE *out;\n+\n+\t/* variables to be filled by option parsing */\n+\tint pack_everything = 0;\n+\tint delete_redundant = 0;\n+\tchar *unpack_unreachable = NULL;\n+\tint window = 0, window_memory = 0;\n+\tint depth = 0;\n+\tint max_pack_size = 0;\n+\tint no_reuse_delta = 0, no_reuse_object = 0;\n+\tint no_update_server_info = 0;\n+\tint quiet = 0;\n+\tint local = 0;\n+\n+\tstruct option builtin_repack_options[] = {\n+\t\tOPT_BIT('a', NULL, &pack_everything,\n+\t\t\t\tN_(\"pack everything in a single pack\"), ALL_INTO_ONE),\n+\t\tOPT_BIT('A', NULL, &pack_everything,\n+\t\t\t\tN_(\"same as -a, and turn unreachable objects loose\"),\n+\t\t\t\t   LOOSEN_UNREACHABLE),\n+\t\tOPT_BOOL('d', NULL, &delete_redundant,\n+\t\t\t\tN_(\"remove redundant packs, and run git-prune-packed\")),\n+\t\tOPT_BOOL('f', NULL, &no_reuse_delta,\n+\t\t\t\tN_(\"pass --no-reuse-delta to git-pack-objects\")),\n+\t\tOPT_BOOL('F', NULL, &no_reuse_object,\n+\t\t\t\tN_(\"pass --no-reuse-object to git-pack-objects\")),\n+\t\tOPT_BOOL('n', NULL, &no_update_server_info,\n+\t\t\t\tN_(\"do not run git-update-server-info\")),\n+\t\tOPT__QUIET(&quiet, N_(\"be quiet\")),\n+\t\tOPT_BOOL('l', \"local\", &local,\n+\t\t\t\tN_(\"pass --local to git-pack-objects\")),\n+\t\tOPT_STRING(0, \"unpack-unreachable\", &unpack_unreachable, N_(\"approxidate\"),\n+\t\t\t\tN_(\"with -A, do not loosen objects older than this\")),\n+\t\tOPT_INTEGER(0, \"window\", &window,\n+\t\t\t\tN_(\"size of the window used for delta compression\")),\n+\t\tOPT_INTEGER(0, \"window-memory\", &window_memory,\n+\t\t\t\tN_(\"same as the above, but limit memory size instead of entries count\")),\n+\t\tOPT_INTEGER(0, \"depth\", &depth,\n+\t\t\t\tN_(\"limits the maximum delta depth\")),\n+\t\tOPT_INTEGER(0, \"max-pack-size\", &max_pack_size,\n+\t\t\t\tN_(\"maximum size of each packfile\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tgit_config(repack_config, NULL);\n+\n+\targc = parse_options(argc, argv, prefix, builtin_repack_options,\n+\t\t\t\tgit_repack_usage, 0);\n+\n+\tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n+\tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n+\n+\tsigchain_push_common(remove_pack_on_signal);\n+\n+\targv_array_push(&cmd_args, \"pack-objects\");\n+\targv_array_push(&cmd_args, \"--keep-true-parents\");\n+\targv_array_push(&cmd_args, \"--honor-pack-keep\");\n+\targv_array_push(&cmd_args, \"--non-empty\");\n+\targv_array_push(&cmd_args, \"--all\");\n+\targv_array_push(&cmd_args, \"--reflog\");\n+\tif (window)\n+\t\targv_array_pushf(&cmd_args, \"--window=%u\", window);\n+\tif (window_memory)\n+\t\targv_array_pushf(&cmd_args, \"--window-memory=%u\", window_memory);\n+\tif (depth)\n+\t\targv_array_pushf(&cmd_args, \"--depth=%u\", depth);\n+\tif (max_pack_size)\n+\t\targv_array_pushf(&cmd_args, \"--max_pack_size=%u\", max_pack_size);\n+\tif (no_reuse_delta)\n+\t\targv_array_pushf(&cmd_args, \"--no-reuse-delta\");\n+\tif (no_reuse_object)\n+\t\targv_array_pushf(&cmd_args, \"--no-reuse-object\");\n+\n+\tif (!pack_everything) {\n+\t\targv_array_push(&cmd_args, \"--unpacked\");\n+\t\targv_array_push(&cmd_args, \"--incremental\");\n+\t} else {\n+\t\tget_non_kept_pack_filenames(&existing_packs);\n+\n+\t\tif (existing_packs.nr && delete_redundant) {\n+\t\t\tif (unpack_unreachable)\n+\t\t\t\targv_array_pushf(&cmd_args,\n+\t\t\t\t\t\t\"--unpack-unreachable=%s\",\n+\t\t\t\t\t\tunpack_unreachable);\n+\t\t\telse if (pack_everything & LOOSEN_UNREACHABLE)\n+\t\t\t\targv_array_push(&cmd_args,\n+\t\t\t\t\t\t\"--unpack-unreachable\");\n+\t\t}\n+\t}\n+\n+\tif (local)\n+\t\targv_array_push(&cmd_args,  \"--local\");\n+\tif (quiet)\n+\t\targv_array_push(&cmd_args,  \"--quiet\");\n+\tif (delta_base_offset)\n+\t\targv_array_push(&cmd_args,  \"--delta-base-offset\");\n+\n+\targv_array_push(&cmd_args, packtmp);\n+\n+\tmemset(&cmd, 0, sizeof(cmd));\n+\tcmd.argv = cmd_args.argv;\n+\tcmd.git_cmd = 1;\n+\tcmd.out = -1;\n+\tcmd.no_stdin = 1;\n+\n+\tret = start_command(&cmd);\n+\tif (ret)\n+\t\treturn 1;\n+\n+\tnr_packs = 0;\n+\tout = xfdopen(cmd.out, \"r\");\n+\twhile (strbuf_getline(&line, out, '\\n') != EOF) {\n+\t\tif (line.len != 40)\n+\t\t\tdie(\"repack: Expecting 40 character sha1 lines only from pack-objects.\");\n+\t\tstring_list_append(&names, line.buf);\n+\t\tnr_packs++;\n+\t}\n+\tfclose(out);\n+\tret = finish_command(&cmd);\n+\tif (ret)\n+\t\treturn 1;\n+\targv_array_clear(&cmd_args);\n+\n+\tif (!nr_packs && !quiet)\n+\t\tprintf(\"Nothing new to pack.\\n\");\n+\n+\t/*\n+\t * Ok we have prepared all new packfiles.\n+\t * First see if there are packs of the same name and if so\n+\t * if we can move them out of the way (this can happen if we\n+\t * repacked immediately after packing fully.\n+\t */\n+\tfailed = 0;\n+\tfor_each_string_list_item(item, &names) {\n+\t\tfor (ext = 0; ext < 2; ext++) {\n+\t\t\tchar *fname, *fname_old;\n+\t\t\tfname = mkpathdup(\"%s/%s%s\", packdir,\n+\t\t\t\t\t\titem->string, exts[ext]);\n+\t\t\tif (!file_exists(fname)) {\n+\t\t\t\tfree(fname);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\n+\t\t\tfname_old = mkpath(\"%s/old-%s%s\", packdir,\n+\t\t\t\t\t\titem->string, exts[ext]);\n+\t\t\tif (file_exists(fname_old))\n+\t\t\t\tif (unlink(fname_old))\n+\t\t\t\t\tfailed = 1;\n+\n+\t\t\tif (!failed && rename(fname, fname_old)) {\n+\t\t\t\tfree(fname);\n+\t\t\t\tfailed = 1;\n+\t\t\t\tbreak;\n+\t\t\t} else {\n+\t\t\t\tstring_list_append(&rollback, fname);\n+\t\t\t}\n+\t\t}\n+\t\tif (failed)\n+\t\t\tbreak;\n+\t}\n+\tif (failed) {\n+\t\tstruct string_list rollback_failure = STRING_LIST_INIT_DUP;\n+\t\tfor_each_string_list_item(item, &rollback) {\n+\t\t\tchar *fname, *fname_old;\n+\t\t\tfname = mkpathdup(\"%s/%s\", packdir, item->string);\n+\t\t\tfname_old = mkpath(\"%s/old-%s\", packdir, item->string);\n+\t\t\tif (rename(fname_old, fname))\n+\t\t\t\tstring_list_append(&rollback_failure, fname);\n+\t\t\tfree(fname);\n+\t\t}\n+\n+\t\tif (rollback_failure.nr) {\n+\t\t\tint i;\n+\t\t\tfprintf(stderr,\n+\t\t\t\t\"WARNING: Some packs in use have been renamed by\\n\"\n+\t\t\t\t\"WARNING: prefixing old- to their name, in order to\\n\"\n+\t\t\t\t\"WARNING: replace them with the new version of the\\n\"\n+\t\t\t\t\"WARNING: file.  But the operation failed, and the\\n\"\n+\t\t\t\t\"WARNING: attempt to rename them back to their\\n\"\n+\t\t\t\t\"WARNING: original names also failed.\\n\"\n+\t\t\t\t\"WARNING: Please rename them in %s manually:\\n\", packdir);\n+\t\t\tfor (i = 0; i < rollback_failure.nr; i++)\n+\t\t\t\tfprintf(stderr, \"WARNING:   old-%s -> %s\\n\",\n+\t\t\t\t\trollback_failure.items[i].string,\n+\t\t\t\t\trollback_failure.items[i].string);\n+\t\t}\n+\t\texit(1);\n+\t}\n+\n+\t/* Now the ones with the same name are out of the way... */\n+\tfor_each_string_list_item(item, &names) {\n+\t\tfor (ext = 0; ext < 2; ext++) {\n+\t\t\tchar *fname, *fname_old;\n+\t\t\tstruct stat statbuffer;\n+\t\t\tfname = mkpathdup(\"%s/pack-%s%s\",\n+\t\t\t\t\tpackdir, item->string, exts[ext]);\n+\t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n+\t\t\t\t\tpacktmp, item->string, exts[ext]);\n+\t\t\tif (!stat(fname_old, &statbuffer)) {\n+\t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n+\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n+\t\t\t}\n+\t\t\tif (rename(fname_old, fname))\n+\t\t\t\texit(errno);\n+\t\t\tfree(fname);\n+\t\t\tfree(fname_old);\n+\t\t}\n+\t}\n+\n+\t/* Remove the \"old-\" files */\n+\tfor_each_string_list_item(item, &names) {\n+\t\tfor (ext = 0; ext < 2; ext++) {\n+\t\t\tchar *fname;\n+\t\t\tfname = mkpath(\"%s/old-pack-%s%s\",\n+\t\t\t\t\tpackdir,\n+\t\t\t\t\titem->string,\n+\t\t\t\t\texts[ext]);\n+\t\t\tremove_path(fname);\n+\t\t}\n+\t}\n+\n+\t/* End of pack replacement. */\n+\n+\tif (delete_redundant) {\n+\t\tsort_string_list(&names);\n+\t\tfor_each_string_list_item(item, &existing_packs) {\n+\t\t\tchar *sha1;\n+\t\t\tsize_t len = strlen(item->string);\n+\t\t\tif (len < 40)\n+\t\t\t\tcontinue;\n+\t\t\tsha1 = item->string + len - 40;\n+\t\t\tif (!string_list_has_string(&names, sha1))\n+\t\t\t\tremove_redundant_pack(packdir, item->string);\n+\t\t}\n+\t\targv_array_push(&cmd_args, \"prune-packed\");\n+\t\tif (quiet)\n+\t\t\targv_array_push(&cmd_args, \"--quiet\");\n+\n+\t\tmemset(&cmd, 0, sizeof(cmd));\n+\t\tcmd.argv = cmd_args.argv;\n+\t\tcmd.git_cmd = 1;\n+\t\trun_command(&cmd);\n+\t\targv_array_clear(&cmd_args);\n+\t}\n+\n+\tif (!no_update_server_info) {\n+\t\targv_array_push(&cmd_args, \"update-server-info\");\n+\t\tmemset(&cmd, 0, sizeof(cmd));\n+\t\tcmd.argv = cmd_args.argv;\n+\t\tcmd.git_cmd = 1;\n+\t\trun_command(&cmd);\n+\t\targv_array_clear(&cmd_args);\n+\t}\n+\tremove_temporary_files();\n+\tstring_list_clear(&names, 0);\n+\tstring_list_clear(&rollback, 0);\n+\tstring_list_clear(&existing_packs, 0);\n+\tstrbuf_release(&line);\n+\n+\treturn 0;\n+}\ndiff --git a/contrib/examples/git-repack.sh b/contrib/examples/git-repack.sh\nnew file mode 100755\nindex 0000000..7579331\n--- /dev/null\n+++ b/contrib/examples/git-repack.sh\n@@ -0,0 +1,194 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2005 Linus Torvalds\n+#\n+\n+OPTIONS_KEEPDASHDASH=\n+OPTIONS_SPEC=\"\\\n+git repack [options]\n+--\n+a               pack everything in a single pack\n+A               same as -a, and turn unreachable objects loose\n+d               remove redundant packs, and run git-prune-packed\n+f               pass --no-reuse-delta to git-pack-objects\n+F               pass --no-reuse-object to git-pack-objects\n+n               do not run git-update-server-info\n+q,quiet         be quiet\n+l               pass --local to git-pack-objects\n+unpack-unreachable=  with -A, do not loosen objects older than this\n+ Packing constraints\n+window=         size of the window used for delta compression\n+window-memory=  same as the above, but limit memory size instead of entries count\n+depth=          limits the maximum delta depth\n+max-pack-size=  maximum size of each packfile\n+\"\n+SUBDIRECTORY_OK='Yes'\n+. git-sh-setup\n+\n+no_update_info= all_into_one= remove_redundant= unpack_unreachable=\n+local= no_reuse= extra=\n+while test $# != 0\n+do\n+\tcase \"$1\" in\n+\t-n)\tno_update_info=t ;;\n+\t-a)\tall_into_one=t ;;\n+\t-A)\tall_into_one=t\n+\t\tunpack_unreachable=--unpack-unreachable ;;\n+\t--unpack-unreachable)\n+\t\tunpack_unreachable=\"--unpack-unreachable=$2\"; shift ;;\n+\t-d)\tremove_redundant=t ;;\n+\t-q)\tGIT_QUIET=t ;;\n+\t-f)\tno_reuse=--no-reuse-delta ;;\n+\t-F)\tno_reuse=--no-reuse-object ;;\n+\t-l)\tlocal=--local ;;\n+\t--max-pack-size|--window|--window-memory|--depth)\n+\t\textra=\"$extra $1=$2\"; shift ;;\n+\t--) shift; break;;\n+\t*)\tusage ;;\n+\tesac\n+\tshift\n+done\n+\n+case \"`git config --bool repack.usedeltabaseoffset || echo true`\" in\n+true)\n+\textra=\"$extra --delta-base-offset\" ;;\n+esac\n+\n+PACKDIR=\"$GIT_OBJECT_DIRECTORY/pack\"\n+PACKTMP=\"$PACKDIR/.tmp-$$-pack\"\n+rm -f \"$PACKTMP\"-*\n+trap 'rm -f \"$PACKTMP\"-*' 0 1 2 3 15\n+\n+# There will be more repacking strategies to come...\n+case \",$all_into_one,\" in\n+,,)\n+\targs='--unpacked --incremental'\n+\t;;\n+,t,)\n+\targs= existing=\n+\tif [ -d \"$PACKDIR\" ]; then\n+\t\tfor e in `cd \"$PACKDIR\" && find . -type f -name '*.pack' \\\n+\t\t\t| sed -e 's/^\\.\\///' -e 's/\\.pack$//'`\n+\t\tdo\n+\t\t\tif [ -e \"$PACKDIR/$e.keep\" ]; then\n+\t\t\t\t: keep\n+\t\t\telse\n+\t\t\t\texisting=\"$existing $e\"\n+\t\t\tfi\n+\t\tdone\n+\t\tif test -n \"$existing\" -a -n \"$unpack_unreachable\" -a \\\n+\t\t\t-n \"$remove_redundant\"\n+\t\tthen\n+\t\t\t# This may have arbitrary user arguments, so we\n+\t\t\t# have to protect it against whitespace splitting\n+\t\t\t# when it gets run as \"pack-objects $args\" later.\n+\t\t\t# Fortunately, we know it's an approxidate, so we\n+\t\t\t# can just use dots instead.\n+\t\t\targs=\"$args $(echo \"$unpack_unreachable\" | tr ' ' .)\"\n+\t\tfi\n+\tfi\n+\t;;\n+esac\n+\n+mkdir -p \"$PACKDIR\" || exit\n+\n+args=\"$args $local ${GIT_QUIET:+-q} $no_reuse$extra\"\n+names=$(git pack-objects --keep-true-parents --honor-pack-keep --non-empty --all --reflog $args </dev/null \"$PACKTMP\") ||\n+\texit 1\n+if [ -z \"$names\" ]; then\n+\tsay Nothing new to pack.\n+fi\n+\n+# Ok we have prepared all new packfiles.\n+\n+# First see if there are packs of the same name and if so\n+# if we can move them out of the way (this can happen if we\n+# repacked immediately after packing fully.\n+rollback=\n+failed=\n+for name in $names\n+do\n+\tfor sfx in pack idx\n+\tdo\n+\t\tfile=pack-$name.$sfx\n+\t\ttest -f \"$PACKDIR/$file\" || continue\n+\t\trm -f \"$PACKDIR/old-$file\" &&\n+\t\tmv \"$PACKDIR/$file\" \"$PACKDIR/old-$file\" || {\n+\t\t\tfailed=t\n+\t\t\tbreak\n+\t\t}\n+\t\trollback=\"$rollback $file\"\n+\tdone\n+\ttest -z \"$failed\" || break\n+done\n+\n+# If renaming failed for any of them, roll the ones we have\n+# already renamed back to their original names.\n+if test -n \"$failed\"\n+then\n+\trollback_failure=\n+\tfor file in $rollback\n+\tdo\n+\t\tmv \"$PACKDIR/old-$file\" \"$PACKDIR/$file\" ||\n+\t\trollback_failure=\"$rollback_failure $file\"\n+\tdone\n+\tif test -n \"$rollback_failure\"\n+\tthen\n+\t\techo >&2 \"WARNING: Some packs in use have been renamed by\"\n+\t\techo >&2 \"WARNING: prefixing old- to their name, in order to\"\n+\t\techo >&2 \"WARNING: replace them with the new version of the\"\n+\t\techo >&2 \"WARNING: file.  But the operation failed, and\"\n+\t\techo >&2 \"WARNING: attempt to rename them back to their\"\n+\t\techo >&2 \"WARNING: original names also failed.\"\n+\t\techo >&2 \"WARNING: Please rename them in $PACKDIR manually:\"\n+\t\tfor file in $rollback_failure\n+\t\tdo\n+\t\t\techo >&2 \"WARNING:   old-$file -> $file\"\n+\t\tdone\n+\tfi\n+\texit 1\n+fi\n+\n+# Now the ones with the same name are out of the way...\n+fullbases=\n+for name in $names\n+do\n+\tfullbases=\"$fullbases pack-$name\"\n+\tchmod a-w \"$PACKTMP-$name.pack\"\n+\tchmod a-w \"$PACKTMP-$name.idx\"\n+\tmv -f \"$PACKTMP-$name.pack\" \"$PACKDIR/pack-$name.pack\" &&\n+\tmv -f \"$PACKTMP-$name.idx\"  \"$PACKDIR/pack-$name.idx\" ||\n+\texit\n+done\n+\n+# Remove the \"old-\" files\n+for name in $names\n+do\n+\trm -f \"$PACKDIR/old-pack-$name.idx\"\n+\trm -f \"$PACKDIR/old-pack-$name.pack\"\n+done\n+\n+# End of pack replacement.\n+\n+if test \"$remove_redundant\" = t\n+then\n+\t# We know $existing are all redundant.\n+\tif [ -n \"$existing\" ]\n+\tthen\n+\t\t( cd \"$PACKDIR\" &&\n+\t\t  for e in $existing\n+\t\t  do\n+\t\t\tcase \" $fullbases \" in\n+\t\t\t*\" $e \"*) ;;\n+\t\t\t*)\trm -f \"$e.pack\" \"$e.idx\" \"$e.keep\" ;;\n+\t\t\tesac\n+\t\t  done\n+\t\t)\n+\tfi\n+\tgit prune-packed ${GIT_QUIET:+-q}\n+fi\n+\n+case \"$no_update_info\" in\n+t) : ;;\n+*) git update-server-info ;;\n+esac\ndiff --git a/git-repack.sh b/git-repack.sh\ndeleted file mode 100755\nindex 7579331..0000000\n--- a/git-repack.sh\n+++ /dev/null\n@@ -1,194 +0,0 @@\n-#!/bin/sh\n-#\n-# Copyright (c) 2005 Linus Torvalds\n-#\n-\n-OPTIONS_KEEPDASHDASH=\n-OPTIONS_SPEC=\"\\\n-git repack [options]\n---\n-a               pack everything in a single pack\n-A               same as -a, and turn unreachable objects loose\n-d               remove redundant packs, and run git-prune-packed\n-f               pass --no-reuse-delta to git-pack-objects\n-F               pass --no-reuse-object to git-pack-objects\n-n               do not run git-update-server-info\n-q,quiet         be quiet\n-l               pass --local to git-pack-objects\n-unpack-unreachable=  with -A, do not loosen objects older than this\n- Packing constraints\n-window=         size of the window used for delta compression\n-window-memory=  same as the above, but limit memory size instead of entries count\n-depth=          limits the maximum delta depth\n-max-pack-size=  maximum size of each packfile\n-\"\n-SUBDIRECTORY_OK='Yes'\n-. git-sh-setup\n-\n-no_update_info= all_into_one= remove_redundant= unpack_unreachable=\n-local= no_reuse= extra=\n-while test $# != 0\n-do\n-\tcase \"$1\" in\n-\t-n)\tno_update_info=t ;;\n-\t-a)\tall_into_one=t ;;\n-\t-A)\tall_into_one=t\n-\t\tunpack_unreachable=--unpack-unreachable ;;\n-\t--unpack-unreachable)\n-\t\tunpack_unreachable=\"--unpack-unreachable=$2\"; shift ;;\n-\t-d)\tremove_redundant=t ;;\n-\t-q)\tGIT_QUIET=t ;;\n-\t-f)\tno_reuse=--no-reuse-delta ;;\n-\t-F)\tno_reuse=--no-reuse-object ;;\n-\t-l)\tlocal=--local ;;\n-\t--max-pack-size|--window|--window-memory|--depth)\n-\t\textra=\"$extra $1=$2\"; shift ;;\n-\t--) shift; break;;\n-\t*)\tusage ;;\n-\tesac\n-\tshift\n-done\n-\n-case \"`git config --bool repack.usedeltabaseoffset || echo true`\" in\n-true)\n-\textra=\"$extra --delta-base-offset\" ;;\n-esac\n-\n-PACKDIR=\"$GIT_OBJECT_DIRECTORY/pack\"\n-PACKTMP=\"$PACKDIR/.tmp-$$-pack\"\n-rm -f \"$PACKTMP\"-*\n-trap 'rm -f \"$PACKTMP\"-*' 0 1 2 3 15\n-\n-# There will be more repacking strategies to come...\n-case \",$all_into_one,\" in\n-,,)\n-\targs='--unpacked --incremental'\n-\t;;\n-,t,)\n-\targs= existing=\n-\tif [ -d \"$PACKDIR\" ]; then\n-\t\tfor e in `cd \"$PACKDIR\" && find . -type f -name '*.pack' \\\n-\t\t\t| sed -e 's/^\\.\\///' -e 's/\\.pack$//'`\n-\t\tdo\n-\t\t\tif [ -e \"$PACKDIR/$e.keep\" ]; then\n-\t\t\t\t: keep\n-\t\t\telse\n-\t\t\t\texisting=\"$existing $e\"\n-\t\t\tfi\n-\t\tdone\n-\t\tif test -n \"$existing\" -a -n \"$unpack_unreachable\" -a \\\n-\t\t\t-n \"$remove_redundant\"\n-\t\tthen\n-\t\t\t# This may have arbitrary user arguments, so we\n-\t\t\t# have to protect it against whitespace splitting\n-\t\t\t# when it gets run as \"pack-objects $args\" later.\n-\t\t\t# Fortunately, we know it's an approxidate, so we\n-\t\t\t# can just use dots instead.\n-\t\t\targs=\"$args $(echo \"$unpack_unreachable\" | tr ' ' .)\"\n-\t\tfi\n-\tfi\n-\t;;\n-esac\n-\n-mkdir -p \"$PACKDIR\" || exit\n-\n-args=\"$args $local ${GIT_QUIET:+-q} $no_reuse$extra\"\n-names=$(git pack-objects --keep-true-parents --honor-pack-keep --non-empty --all --reflog $args </dev/null \"$PACKTMP\") ||\n-\texit 1\n-if [ -z \"$names\" ]; then\n-\tsay Nothing new to pack.\n-fi\n-\n-# Ok we have prepared all new packfiles.\n-\n-# First see if there are packs of the same name and if so\n-# if we can move them out of the way (this can happen if we\n-# repacked immediately after packing fully.\n-rollback=\n-failed=\n-for name in $names\n-do\n-\tfor sfx in pack idx\n-\tdo\n-\t\tfile=pack-$name.$sfx\n-\t\ttest -f \"$PACKDIR/$file\" || continue\n-\t\trm -f \"$PACKDIR/old-$file\" &&\n-\t\tmv \"$PACKDIR/$file\" \"$PACKDIR/old-$file\" || {\n-\t\t\tfailed=t\n-\t\t\tbreak\n-\t\t}\n-\t\trollback=\"$rollback $file\"\n-\tdone\n-\ttest -z \"$failed\" || break\n-done\n-\n-# If renaming failed for any of them, roll the ones we have\n-# already renamed back to their original names.\n-if test -n \"$failed\"\n-then\n-\trollback_failure=\n-\tfor file in $rollback\n-\tdo\n-\t\tmv \"$PACKDIR/old-$file\" \"$PACKDIR/$file\" ||\n-\t\trollback_failure=\"$rollback_failure $file\"\n-\tdone\n-\tif test -n \"$rollback_failure\"\n-\tthen\n-\t\techo >&2 \"WARNING: Some packs in use have been renamed by\"\n-\t\techo >&2 \"WARNING: prefixing old- to their name, in order to\"\n-\t\techo >&2 \"WARNING: replace them with the new version of the\"\n-\t\techo >&2 \"WARNING: file.  But the operation failed, and\"\n-\t\techo >&2 \"WARNING: attempt to rename them back to their\"\n-\t\techo >&2 \"WARNING: original names also failed.\"\n-\t\techo >&2 \"WARNING: Please rename them in $PACKDIR manually:\"\n-\t\tfor file in $rollback_failure\n-\t\tdo\n-\t\t\techo >&2 \"WARNING:   old-$file -> $file\"\n-\t\tdone\n-\tfi\n-\texit 1\n-fi\n-\n-# Now the ones with the same name are out of the way...\n-fullbases=\n-for name in $names\n-do\n-\tfullbases=\"$fullbases pack-$name\"\n-\tchmod a-w \"$PACKTMP-$name.pack\"\n-\tchmod a-w \"$PACKTMP-$name.idx\"\n-\tmv -f \"$PACKTMP-$name.pack\" \"$PACKDIR/pack-$name.pack\" &&\n-\tmv -f \"$PACKTMP-$name.idx\"  \"$PACKDIR/pack-$name.idx\" ||\n-\texit\n-done\n-\n-# Remove the \"old-\" files\n-for name in $names\n-do\n-\trm -f \"$PACKDIR/old-pack-$name.idx\"\n-\trm -f \"$PACKDIR/old-pack-$name.pack\"\n-done\n-\n-# End of pack replacement.\n-\n-if test \"$remove_redundant\" = t\n-then\n-\t# We know $existing are all redundant.\n-\tif [ -n \"$existing\" ]\n-\tthen\n-\t\t( cd \"$PACKDIR\" &&\n-\t\t  for e in $existing\n-\t\t  do\n-\t\t\tcase \" $fullbases \" in\n-\t\t\t*\" $e \"*) ;;\n-\t\t\t*)\trm -f \"$e.pack\" \"$e.idx\" \"$e.keep\" ;;\n-\t\t\tesac\n-\t\t  done\n-\t\t)\n-\tfi\n-\tgit prune-packed ${GIT_QUIET:+-q}\n-fi\n-\n-case \"$no_update_info\" in\n-t) : ;;\n-*) git update-server-info ;;\n-esac\ndiff --git a/git.c b/git.c\nindex 2025f77..03510be 100644\n--- a/git.c\n+++ b/git.c\n@@ -396,6 +396,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"remote\", cmd_remote, RUN_SETUP },\n \t\t{ \"remote-ext\", cmd_remote_ext },\n \t\t{ \"remote-fd\", cmd_remote_fd },\n+\t\t{ \"repack\", cmd_repack, RUN_SETUP },\n \t\t{ \"replace\", cmd_replace, RUN_SETUP },\n \t\t{ \"repo-config\", cmd_repo_config, RUN_SETUP_GENTLY },\n \t\t{ \"rerere\", cmd_rerere, RUN_SETUP },\n-- \n1.8.4.273.ga194ead\n"},{"id":"227696","messageId":"1379259202-5042-2-git-send-email-stefanbeller@googlemail.com","threadId":"34935","inReplyTo":"1379259202-5042-1-git-send-email-stefanbeller@googlemail.com","subject":"[PATCH 2/3] repack: retain the return value of pack-objects","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-09-15T15:33:21Z","receivedAt":"2013-09-15T15:33:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"During the review process of the previous commit (repack: rewrite the\nshell script in C), Johannes Sixt proposed to retain any exit codes from\nthe sub-process, which makes it probably more obvious in case of failure.\n\nAs the commit before should behave as close to the original shell\nscript, the proposed change is put in this extra commit.\nThe infrastructure however was already setup in the previous commit.\n(Having a local 'ret' variable)\n\nSigned-off-by: Stefan Beller <stefanbeller@googlemail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/repack.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a15bd9b..d345d16 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -231,7 +231,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tret = start_command(&cmd);\n \tif (ret)\n-\t\treturn 1;\n+\t\treturn ret;\n \n \tnr_packs = 0;\n \tout = xfdopen(cmd.out, \"r\");\n@@ -244,7 +244,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tfclose(out);\n \tret = finish_command(&cmd);\n \tif (ret)\n-\t\treturn 1;\n+\t\treturn ret;\n \targv_array_clear(&cmd_args);\n \n \tif (!nr_packs && !quiet)\n-- \n1.8.4.273.ga194ead\n"},{"id":"227698","messageId":"1379259202-5042-3-git-send-email-stefanbeller@googlemail.com","threadId":"34935","inReplyTo":"1379259202-5042-1-git-send-email-stefanbeller@googlemail.com","subject":"[PATCH 3/3] repack: improve warnings about failure of renaming and removing files","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-09-15T15:33:22Z","receivedAt":"2013-09-15T15:33:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>\n---\n builtin/repack.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex d345d16..f7e91bf 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -327,7 +327,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tchmod(fname_old, statbuffer.st_mode);\n \t\t\t}\n \t\t\tif (rename(fname_old, fname))\n-\t\t\t\texit(errno);\n+\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n \t\t\tfree(fname);\n \t\t\tfree(fname_old);\n \t\t}\n@@ -341,7 +341,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tpackdir,\n \t\t\t\t\titem->string,\n \t\t\t\t\texts[ext]);\n-\t\t\tremove_path(fname);\n+\t\t\tif (remove_path(fname))\n+\t\t\t\twarning(_(\"removing '%s' failed\"), fname);\n \t\t}\n \t}\n \n-- \n1.8.4.273.ga194ead\n"},{"id":"227702","messageId":"CALkWK0nG4+pdSEEUhHrN=+kXEetJ4sJxVOc0VWSweRuJUL-jsA@mail.gmail.com","threadId":"34935","inReplyTo":"1379259202-5042-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH 1/3] repack: rewrite the shell script in C","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-09-15T17:54:34Z","receivedAt":"2013-09-15T17:54:34Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Stefan Beller wrote:\n>  Makefile                       |   2 +-\n>  builtin.h                      |   1 +\n>  builtin/repack.c               | 387 +++++++++++++++++++++++++++++++++++++++++\n>  contrib/examples/git-repack.sh | 194 +++++++++++++++++++++\n>  git-repack.sh                  | 194 ---------------------\n>  git.c                          |   1 +\n>  6 files changed, 584 insertions(+), 195 deletions(-)\n>  create mode 100644 builtin/repack.c\n>  create mode 100755 contrib/examples/git-repack.sh\n>  delete mode 100755 git-repack.sh\n\nLooks like repack.c is significantly larger than git-repack.sh. I look\nforward to reading the code.\n\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> new file mode 100644\n> index 0000000..a15bd9b\n> --- /dev/null\n> +++ b/builtin/repack.c\n> @@ -0,0 +1,387 @@\n> +#include \"builtin.h\"\n> +#include \"cache.h\"\n> +#include \"dir.h\"\n> +#include \"parse-options.h\"\n> +#include \"run-command.h\"\n> +#include \"sigchain.h\"\n> +#include \"strbuf.h\"\n> +#include \"string-list.h\"\n> +#include \"argv-array.h\"\n> +\n> +static int delta_base_offset = 1;\n> +static char *packdir, *packtmp;\n> +\n> +static const char *const git_repack_usage[] = {\n> +       N_(\"git repack [options]\"),\n> +       NULL\n> +};\n> +\n> +static int repack_config(const char *var, const char *value, void *cb)\n> +{\n> +       if (!strcmp(var, \"repack.usedeltabaseoffset\")) {\n> +               delta_base_offset = git_config_bool(var, value);\n> +               return 0;\n> +       }\n> +       return git_default_config(var, value, cb);\n> +}\n\nConfiguration option: one bool. I wonder what other configuration\noptions the future will bring in.\n\n> +/*\n> + * Remove temporary $GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-* files.\n> + */\n\nIs $GIT_OBJECT_DIRECTORY a standard variable, or should it be\n$GIT_DIR/objects? A quick grep tells me that there are a few\nreferences to it, but I'm yet to be convinced that it can be something\nother than $GIT_DIR/objects.\n\n> +static void remove_temporary_files(void)\n> +{\n> +       struct strbuf buf = STRBUF_INIT;\n> +       size_t dirlen, prefixlen;\n> +       DIR *dir;\n> +       struct dirent *e;\n> +\n> +       dir = opendir(packdir);\n> +       if (!dir)\n> +               return;\n\nWait a minute: where did we initalize packdir? Ah, it's static, so it\nmust have been initalized before the function was called (in some sort\nof setup function), right? Why don't you return an int from the\nfunction so it's possible to differentiate between success and\nfailure?\n\n> +       /* Point at the slash at the end of \".../objects/pack/\" */\n\nIs packdir a relative or absolute path? The ... isn't helping.\n\n> +       dirlen = strlen(packdir) + 1;\n> +       strbuf_addstr(&buf, packtmp);\n\nMysterious initalization of packtmp.\n\n> +       /* Hold the length of  \".tmp-%d-pack-\" */\n> +       prefixlen = buf.len - dirlen;\n\nOkay, so that's what packtmp contains: a reading of repack.sh told me\nthat you didn't even change the variable names.\n\n> +       while ((e = readdir(dir))) {\n> +               if (strncmp(e->d_name, buf.buf + dirlen, prefixlen))\n> +                       continue;\n\nSkip the dentry that points to the .tmp-* thing.\n\n> +               strbuf_setlen(&buf, dirlen);\n> +               strbuf_addstr(&buf, e->d_name);\n> +               unlink(buf.buf);\n> +       }\n> +       closedir(dir);\n> +       strbuf_release(&buf);\n> +}\n\nOkay.\n\n> +static void remove_pack_on_signal(int signo)\n> +{\n> +       remove_temporary_files();\n> +       sigchain_pop(signo);\n> +       raise(signo);\n> +}\n\nOkay, although I'd have named the variable \"signal\", not \"signo\".\n\n> +/*\n> + * Adds all packs hex strings to the fname list, which do not\n> + * have a corresponding .keep file.\n> + */\n\nThe packs which don't have a corresponding .keep file must be removed/\nrepacked. Okay.\n\n> +static void get_non_kept_pack_filenames(struct string_list *fname_list)\n> +{\n> +       DIR *dir;\n> +       struct dirent *e;\n> +       char *fname;\n\nFilename, I assume.\n\n> +       size_t len;\n> +\n> +       if (!(dir = opendir(packdir)))\n> +               return;\n\nAn int return, please.\n\n> +       while ((e = readdir(dir)) != NULL) {\n> +               if (suffixcmp(e->d_name, \".pack\"))\n> +                       continue;\n\nIf we didn't find a .pack file, skip over?\n\n> +               len = strlen(e->d_name) - strlen(\".pack\");\n> +               fname = xmemdupz(e->d_name, len);\n\nYou can probably use the pathbufs to save some memory here, but I\nwouldn't worry about it at this stage.\n\n> +               if (!file_exists(mkpath(\"%s/%s.keep\", packdir, fname)))\n> +                       string_list_append_nodup(fname_list, fname);\n> +               else\n> +                       free(fname);\n> +       }\n> +       closedir(dir);\n> +}\n\nOkay.\n\n> +static void remove_redundant_pack(const char *path_prefix, const char *hex)\n> +{\n> +       const char *exts[] = {\".pack\", \".idx\", \".keep\"};\n> +       int i;\n> +       struct strbuf buf = STRBUF_INIT;\n> +       size_t plen;\n> +\n> +       strbuf_addf(&buf, \"%s/%s\", path_prefix, hex);\n\nhex is the sha1-hex written out to the full 40 characters?\n\n> +       plen = buf.len;\n> +\n> +       for (i = 0; i < ARRAY_SIZE(exts); i++) {\n> +               strbuf_setlen(&buf, plen);\n> +               strbuf_addstr(&buf, exts[i]);\n> +               unlink(buf.buf);\n> +       }\n> +       strbuf_release(&buf);\n> +}\n\nSo you find the redundant sha1-hexes and unlink the corresponding\n.pack, .idx and .keep files. Okay.\n\n> +#define ALL_INTO_ONE 1\n> +#define LOOSEN_UNREACHABLE 2\n\nWait, isn't all_into_one supposed to be configurable? What is\nloosen_unreachable?\n\n> +int cmd_repack(int argc, const char **argv, const char *prefix)\n> +{\n> [...]\n\nSo argument parsing and spawning pack-objects appropriately is what\nblew up the size of this file. I'll review this monolith in another\nsitting.\n\nThanks for the enjoyable read.\n\nRam\n"},{"id":"227759","messageId":"xmqqwqmfz9fc.fsf@gitster.dls.corp.google.com","threadId":"34935","inReplyTo":"CALkWK0nG4+pdSEEUhHrN=+kXEetJ4sJxVOc0VWSweRuJUL-jsA@mail.gmail.com","subject":"Re: [PATCH 1/3] repack: rewrite the shell script in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-17T16:42:47Z","receivedAt":"2013-09-17T16:42:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Is $GIT_OBJECT_DIRECTORY a standard variable, or should it be\n> $GIT_DIR/objects?\n\n\"man git\" ;-)  It has been there since early May 2005\n"},{"id":"227771","messageId":"xmqqy56vxqhi.fsf@gitster.dls.corp.google.com","threadId":"34935","inReplyTo":"1379259202-5042-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH 1/3] repack: rewrite the shell script in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-17T18:17:13Z","receivedAt":"2013-09-17T18:17:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <stefanbeller@googlemail.com> writes:\n\n> +\tstruct option builtin_repack_options[] = {\n> +\t\tOPT_BIT('a', NULL, &pack_everything,\n> +\t\t\t\tN_(\"pack everything in a single pack\"), ALL_INTO_ONE),\n> +\t\tOPT_BIT('A', NULL, &pack_everything,\n> +\t\t\t\tN_(\"same as -a, and turn unreachable objects loose\"),\n> +\t\t\t\t   LOOSEN_UNREACHABLE),\n\nMicronit.\n\nWith the current version of the code in cmd_repack() that uses the\npack_everything variable this may not make a difference, but I think\nthis should logically be \"LOOSEN_UNREACHABLE | ALL_INTO_ONE\" instead,\nand the code should check (pack_evertying & ALL_INTO_ONE) instead of\nchecking \"!pack_everything\".  You may want to add to this flag variable\na new bit that does _not_ cause it to pack everything into one.\n"},{"id":"227784","messageId":"5238B7E4.6060506@googlemail.com","threadId":"34935","inReplyTo":"xmqqy56vxqhi.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] repack: rewrite the shell script in C","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-09-17T20:13:24Z","receivedAt":"2013-09-17T20:13:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 09/17/2013 08:17 PM, Junio C Hamano wrote:\n> Stefan Beller <stefanbeller@googlemail.com> writes:\n> \n>> +\tstruct option builtin_repack_options[] = {\n>> +\t\tOPT_BIT('a', NULL, &pack_everything,\n>> +\t\t\t\tN_(\"pack everything in a single pack\"), ALL_INTO_ONE),\n>> +\t\tOPT_BIT('A', NULL, &pack_everything,\n>> +\t\t\t\tN_(\"same as -a, and turn unreachable objects loose\"),\n>> +\t\t\t\t   LOOSEN_UNREACHABLE),\n> \n> Micronit.\n> \n> With the current version of the code in cmd_repack() that uses the\n> pack_everything variable this may not make a difference, but I think\n> this should logically be \"LOOSEN_UNREACHABLE | ALL_INTO_ONE\" instead,\n> and the code should check (pack_evertying & ALL_INTO_ONE) instead of\n> checking \"!pack_everything\".  You may want to add to this flag variable\n> a new bit that does _not_ cause it to pack everything into one.\n> \n\nI do understand the \"LOOSEN_UNREACHABLE | ALL_INTO_ONE\" here, as that\nis the logical thing we are doing. Combined with your second idea this \nwould result in\n---8<---\n\nFrom 4bbbfb312bf23efa7e702e200fbc2d4479e3477e Mon Sep 17 00:00:00 2001\nFrom: Stefan Beller <stefanbeller@googlemail.com>\nDate: Tue, 17 Sep 2013 22:04:35 +0200\nSubject: [PATCH 2/2] Suggestions by Junio\n\nSigned-off-by: Stefan Beller <stefanbeller@googlemail.com>\n---\n builtin/repack.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex e5f90c6..a0ff5c7 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -143,7 +143,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"pack everything in a single pack\"), ALL_INTO_ONE),\n \t\tOPT_BIT('A', NULL, &pack_everything,\n \t\t\t\tN_(\"same as -a, and turn unreachable objects loose\"),\n-\t\t\t\t   LOOSEN_UNREACHABLE),\n+\t\t\t\t   LOOSEN_UNREACHABLE | ALL_INTO_ONE),\n \t\tOPT_BOOL('d', NULL, &delete_redundant,\n \t\t\t\tN_(\"remove redundant packs, and run git-prune-packed\")),\n \t\tOPT_BOOL('f', NULL, &no_reuse_delta,\n@@ -197,10 +197,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (no_reuse_object)\n \t\targv_array_pushf(&cmd_args, \"--no-reuse-object\");\n \n-\tif (!pack_everything) {\n-\t\targv_array_push(&cmd_args, \"--unpacked\");\n-\t\targv_array_push(&cmd_args, \"--incremental\");\n-\t} else {\n+\tif (pack_everything & ALL_INTO_ONE) {\n \t\tget_non_kept_pack_filenames(&existing_packs);\n \n \t\tif (existing_packs.nr && delete_redundant) {\n@@ -212,6 +209,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\targv_array_push(&cmd_args,\n \t\t\t\t\t\t\"--unpack-unreachable\");\n \t\t}\n+\t} else {\n+\t\targv_array_push(&cmd_args, \"--unpacked\");\n+\t\targv_array_push(&cmd_args, \"--incremental\");\n \t}\n \n \tif (local)\n-- \n1.8.4.273.ga194ead\n\n\nHowever I assume you mean to even ease up the conditions now, because now\nboth -a as well as -A set ALL_INTO_ONE we could apply the following \non top of the previous.\n---8<---\n\nFrom 80199368ab6c7ab72f81a5c531f79073a99d2498 Mon Sep 17 00:00:00 2001\nFrom: Stefan Beller <stefanbeller@googlemail.com>\nDate: Tue, 17 Sep 2013 22:11:08 +0200\nSubject: [PATCH] Further improvements by reducing nested ifs\n\nThis may pass --unpacked and --unpack-unreachable to pack-objects in one\ncommand, which is redundant. On the other hand we may gain simplicity in\nrepack.\n\nSigned-off-by: Stefan Beller <stefanbeller@googlemail.com>\n---\n builtin/repack.c | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a0ff5c7..3e56614 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -197,23 +197,23 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (no_reuse_object)\n \t\targv_array_pushf(&cmd_args, \"--no-reuse-object\");\n \n-\tif (pack_everything & ALL_INTO_ONE) {\n+\tif (pack_everything & ALL_INTO_ONE)\n \t\tget_non_kept_pack_filenames(&existing_packs);\n-\n-\t\tif (existing_packs.nr && delete_redundant) {\n-\t\t\tif (unpack_unreachable)\n-\t\t\t\targv_array_pushf(&cmd_args,\n-\t\t\t\t\t\t\"--unpack-unreachable=%s\",\n-\t\t\t\t\t\tunpack_unreachable);\n-\t\t\telse if (pack_everything & LOOSEN_UNREACHABLE)\n-\t\t\t\targv_array_push(&cmd_args,\n-\t\t\t\t\t\t\"--unpack-unreachable\");\n-\t\t}\n-\t} else {\n+\telse {\n \t\targv_array_push(&cmd_args, \"--unpacked\");\n \t\targv_array_push(&cmd_args, \"--incremental\");\n \t}\n \n+\tif (existing_packs.nr && delete_redundant) {\n+\t\tif (unpack_unreachable)\n+\t\t\targv_array_pushf(&cmd_args,\n+\t\t\t\t\t\"--unpack-unreachable=%s\",\n+\t\t\t\t\tunpack_unreachable);\n+\t\telse if (pack_everything & LOOSEN_UNREACHABLE)\n+\t\t\targv_array_push(&cmd_args,\n+\t\t\t\t\t\"--unpack-unreachable\");\n+\t}\n+\n \tif (local)\n \t\targv_array_push(&cmd_args,  \"--local\");\n \tif (quiet)\n-- \n1.8.4.273.ga194ead\n\n\n\n"},{"id":"227790","messageId":"xmqqa9jbxkk5.fsf@gitster.dls.corp.google.com","threadId":"34935","inReplyTo":"5238B7E4.6060506@googlemail.com","subject":"Re: [PATCH 1/3] repack: rewrite the shell script in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-17T20:25:14Z","receivedAt":"2013-09-17T20:25:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <stefanbeller@googlemail.com> writes:\n\n> On 09/17/2013 08:17 PM, Junio C Hamano wrote:\n>> Stefan Beller <stefanbeller@googlemail.com> writes:\n>> \n>>> +\tstruct option builtin_repack_options[] = {\n>>> +\t\tOPT_BIT('a', NULL, &pack_everything,\n>>> +\t\t\t\tN_(\"pack everything in a single pack\"), ALL_INTO_ONE),\n>>> +\t\tOPT_BIT('A', NULL, &pack_everything,\n>>> +\t\t\t\tN_(\"same as -a, and turn unreachable objects loose\"),\n>>> +\t\t\t\t   LOOSEN_UNREACHABLE),\n>> \n>> Micronit.\n>> \n>> With the current version of the code in cmd_repack() that uses the\n>> pack_everything variable this may not make a difference, but I think\n>> this should logically be \"LOOSEN_UNREACHABLE | ALL_INTO_ONE\" instead,\n>> and the code should check (pack_evertying & ALL_INTO_ONE) instead of\n>> checking \"!pack_everything\".  You may want to add to this flag variable\n>> a new bit that does _not_ cause it to pack everything into one.\n>> \n>\n> I do understand the \"LOOSEN_UNREACHABLE | ALL_INTO_ONE\" here, as that\n> is the logical thing we are doing. Combined with your second idea this \n> would result in\n> ---8<---\n>\n> From 4bbbfb312bf23efa7e702e200fbc2d4479e3477e Mon Sep 17 00:00:00 2001\n> From: Stefan Beller <stefanbeller@googlemail.com>\n> Date: Tue, 17 Sep 2013 22:04:35 +0200\n> Subject: [PATCH 2/2] Suggestions by Junio\n>\n> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>\n> ---\n>  builtin/repack.c | 10 +++++-----\n>  1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index e5f90c6..a0ff5c7 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -143,7 +143,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \t\t\t\tN_(\"pack everything in a single pack\"), ALL_INTO_ONE),\n>  \t\tOPT_BIT('A', NULL, &pack_everything,\n>  \t\t\t\tN_(\"same as -a, and turn unreachable objects loose\"),\n> -\t\t\t\t   LOOSEN_UNREACHABLE),\n> +\t\t\t\t   LOOSEN_UNREACHABLE | ALL_INTO_ONE),\n>  \t\tOPT_BOOL('d', NULL, &delete_redundant,\n>  \t\t\t\tN_(\"remove redundant packs, and run git-prune-packed\")),\n>  \t\tOPT_BOOL('f', NULL, &no_reuse_delta,\n> @@ -197,10 +197,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \tif (no_reuse_object)\n>  \t\targv_array_pushf(&cmd_args, \"--no-reuse-object\");\n>  \n> -\tif (!pack_everything) {\n> -\t\targv_array_push(&cmd_args, \"--unpacked\");\n> -\t\targv_array_push(&cmd_args, \"--incremental\");\n> -\t} else {\n> +\tif (pack_everything & ALL_INTO_ONE) {\n>  \t\tget_non_kept_pack_filenames(&existing_packs);\n>  \n>  \t\tif (existing_packs.nr && delete_redundant) {\n> @@ -212,6 +209,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \t\t\t\targv_array_push(&cmd_args,\n>  \t\t\t\t\t\t\"--unpack-unreachable\");\n>  \t\t}\n> +\t} else {\n> +\t\targv_array_push(&cmd_args, \"--unpacked\");\n> +\t\targv_array_push(&cmd_args, \"--incremental\");\n>  \t}\n>  \n>  \tif (local)\n> -- \n> 1.8.4.273.ga194ead\n\nYes.\n\nAbove was what I would have expected from a straight *.sh to *.c\nconversion.\n\nBut I didn't think about the change in the patch below.\n\n> However I assume you mean to even ease up the conditions now, because now\n> both -a as well as -A set ALL_INTO_ONE we could apply the following \n> on top of the previous.\n> ---8<---\n>\n> From 80199368ab6c7ab72f81a5c531f79073a99d2498 Mon Sep 17 00:00:00 2001\n> From: Stefan Beller <stefanbeller@googlemail.com>\n> Date: Tue, 17 Sep 2013 22:11:08 +0200\n> Subject: [PATCH] Further improvements by reducing nested ifs\n>\n> This may pass --unpacked and --unpack-unreachable to pack-objects in one\n> command, which is redundant. On the other hand we may gain simplicity in\n> repack.\n>\n> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>\n> ---\n>  builtin/repack.c | 24 ++++++++++++------------\n>  1 file changed, 12 insertions(+), 12 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index a0ff5c7..3e56614 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -197,23 +197,23 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \tif (no_reuse_object)\n>  \t\targv_array_pushf(&cmd_args, \"--no-reuse-object\");\n>  \n> -\tif (pack_everything & ALL_INTO_ONE) {\n> +\tif (pack_everything & ALL_INTO_ONE)\n>  \t\tget_non_kept_pack_filenames(&existing_packs);\n> -\n> -\t\tif (existing_packs.nr && delete_redundant) {\n> -\t\t\tif (unpack_unreachable)\n> -\t\t\t\targv_array_pushf(&cmd_args,\n> -\t\t\t\t\t\t\"--unpack-unreachable=%s\",\n> -\t\t\t\t\t\tunpack_unreachable);\n> -\t\t\telse if (pack_everything & LOOSEN_UNREACHABLE)\n> -\t\t\t\targv_array_push(&cmd_args,\n> -\t\t\t\t\t\t\"--unpack-unreachable\");\n> -\t\t}\n> -\t} else {\n> +\telse {\n>  \t\targv_array_push(&cmd_args, \"--unpacked\");\n>  \t\targv_array_push(&cmd_args, \"--incremental\");\n>  \t}\n>  \n> +\tif (existing_packs.nr && delete_redundant) {\n> +\t\tif (unpack_unreachable)\n> +\t\t\targv_array_pushf(&cmd_args,\n> +\t\t\t\t\t\"--unpack-unreachable=%s\",\n> +\t\t\t\t\tunpack_unreachable);\n> +\t\telse if (pack_everything & LOOSEN_UNREACHABLE)\n> +\t\t\targv_array_push(&cmd_args,\n> +\t\t\t\t\t\"--unpack-unreachable\");\n> +\t}\n> +\n>  \tif (local)\n>  \t\targv_array_push(&cmd_args,  \"--local\");\n>  \tif (quiet)\n"}]}