{"thread":{"id":"34802","subject":"[PATCH 2/2] repack: retain the return value of pack-objects","startedAt":"2013-08-29T20:39:32Z","lastAt":"2013-09-17T20:15:01Z","messageCount":9,"participants":["Stefan Beller","Junio C Hamano","Felipe Contreras","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"226258","messageId":"1377808774-12505-1-git-send-email-stefanbeller@googlemail.com","threadId":"34802","inReplyTo":null,"subject":"[PATCHv7 0/2] Rewriting repack in C","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-29T20:39:32Z","receivedAt":"2013-08-29T20:39:32Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi,\nI am sorry to have been delaying this patch series for a few days,\nas I was busy. I'll continue to be busy for the next 2 weeks, so \nmy replies (if needed), will take some time.\n\nThe first patch rewrites the repack shell script in C and tries to\nresemble the functionality as close to the original as possible.\nThe second patch introduces some functional changes intentionally.\n\nStefan\n\nHere is a diff since the last time sending this patch series:\n---\n builtin/repack.c | 29 +++++++++++++++++++----------\n 1 file changed, 19 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 0ace2a3..0cc823d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -41,10 +41,10 @@ static void remove_temporary_files(void)\n \n \tstrbuf_addstr(&buf, packdir);\n \n-\t/* dirlen holds the length of the path before the file name */\n+\t/* Point at the slash at the end of \".../objects/pack/\" */\n \tdirlen = buf.len + 1;\n \tstrbuf_addf(&buf, \"%s\", packtmp);\n-\t/* prefixlen holds the length of the prefix */\n+\t/* Point at the dash at the end of \".../.tmp-%d-pack-\" */\n \tprefixlen = buf.len - dirlen;\n \n \twhile ((e = readdir(dir))) {\n@@ -109,9 +109,12 @@ static void remove_redundant_pack(const char *path_prefix, const char *hex)\n \t}\n }\n \n+#define ALL_INTO_ONE 1\n+#define LOOSE_UNREACHABLE 2\n+\n int cmd_repack(int argc, const char **argv, const char *prefix)\n {\n-\tconst char *exts[2] = {\".idx\", \".pack\"};\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@@ -124,7 +127,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \t/* variables to be filled by option parsing */\n \tint pack_everything = 0;\n-\tint pack_everything_but_loose = 0;\n \tint delete_redundant = 0;\n \tchar *unpack_unreachable = NULL;\n \tint window = 0, window_memory = 0;\n@@ -136,10 +138,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tint local = 0;\n \n \tstruct option builtin_repack_options[] = {\n-\t\tOPT_BOOL('a', NULL, &pack_everything,\n-\t\t\t\tN_(\"pack everything in a single pack\")),\n-\t\tOPT_BOOL('A', NULL, &pack_everything_but_loose,\n-\t\t\t\tN_(\"same as -a, and turn unreachable objects loose\")),\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\"), LOOSE_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@@ -193,7 +195,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 && !pack_everything_but_loose) {\n+\tif (!pack_everything) {\n \t\targv_array_push(&cmd_args, \"--unpacked\");\n \t\targv_array_push(&cmd_args, \"--incremental\");\n \t} else {\n@@ -204,7 +206,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\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_but_loose)\n+\t\t\telse if (pack_everything & LOOSE_UNREACHABLE)\n \t\t\t\targv_array_push(&cmd_args,\n \t\t\t\t\t\t\"--unpack-unreachable\");\n \t\t}\n@@ -246,6 +248,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\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@@ -366,5 +374,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\trun_command(&cmd);\n \t\targv_array_clear(&cmd_args);\n \t}\n+\tremove_temporary_files();\n \treturn 0;\n }\n-- \n1.8.4\n\nStefan Beller (2):\n  repack: rewrite the shell script in C\n  repack: retain the return value of pack-objects\n\n Makefile                                        |   2 +-\n builtin.h                                       |   1 +\n builtin/repack.c                                | 379 ++++++++++++++++++++++++\n git-repack.sh => contrib/examples/git-repack.sh |   0\n git.c                                           |   1 +\n 5 files changed, 382 insertions(+), 1 deletion(-)\n create mode 100644 builtin/repack.c\n rename git-repack.sh => contrib/examples/git-repack.sh (100%)\n\n-- \n1.8.4\n"},{"id":"226259","messageId":"1377808774-12505-2-git-send-email-stefanbeller@googlemail.com","threadId":"34802","inReplyTo":"1377808774-12505-1-git-send-email-stefanbeller@googlemail.com","subject":"[PATCH 1/2] repack: rewrite the shell script in C","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-29T20:39:33Z","receivedAt":"2013-08-29T20:39:33Z","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>\n---\n Makefile                                        |   2 +-\n builtin.h                                       |   1 +\n builtin/repack.c                                | 379 ++++++++++++++++++++++++\n git-repack.sh => contrib/examples/git-repack.sh |   0\n git.c                                           |   1 +\n 5 files changed, 382 insertions(+), 1 deletion(-)\n create mode 100644 builtin/repack.c\n rename git-repack.sh => contrib/examples/git-repack.sh (100%)\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..0cc823d\n--- /dev/null\n+++ b/builtin/repack.c\n@@ -0,0 +1,379 @@\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+\tstrbuf_addstr(&buf, packdir);\n+\n+\t/* Point at the slash at the end of \".../objects/pack/\" */\n+\tdirlen = buf.len + 1;\n+\tstrbuf_addf(&buf, \"%s\", packtmp);\n+\t/* Point at the dash at the end 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}\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+}\n+\n+#define ALL_INTO_ONE 1\n+#define LOOSE_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\"), LOOSE_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 & LOOSE_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\tfailed = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tstring_list_append(&rollback, fname);\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\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\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\tif (remove_path(fname))\n+\t\t\t\twarning(_(\"removing '%s' failed\"), 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+\treturn 0;\n+}\ndiff --git a/git-repack.sh b/contrib/examples/git-repack.sh\nsimilarity index 100%\nrename from git-repack.sh\nrename to contrib/examples/git-repack.sh\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\n"},{"id":"226257","messageId":"1377808774-12505-3-git-send-email-stefanbeller@googlemail.com","threadId":"34802","inReplyTo":"1377808774-12505-1-git-send-email-stefanbeller@googlemail.com","subject":"[PATCH 2/2] repack: retain the return value of pack-objects","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-29T20:39:34Z","receivedAt":"2013-08-29T20:39:34Z","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 0cc823d..2fb3b91 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -229,7 +229,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@@ -242,7 +242,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\n"},{"id":"226260","messageId":"xmqqob8g2p8t.fsf@gitster.dls.corp.google.com","threadId":"34802","inReplyTo":"1377808774-12505-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCHv7 0/2] Rewriting repack in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-29T20:53:06Z","receivedAt":"2013-08-29T20:53:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <stefanbeller@googlemail.com> writes:\n\n> Here is a diff since the last time sending this patch series:\n\nThis is very readable.\n\nThere may be people who misread \"LOOSE\" as \"LOSE\"; the option -A is \nabout making the unreachable ones loose so that they can be expired,\nso let's rename it LOOSEN_UNREACHABLE to avoid confusion.\n\nThanks.\n\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 0ace2a3..0cc823d 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -41,10 +41,10 @@ static void remove_temporary_files(void)\n>  \n>  \tstrbuf_addstr(&buf, packdir);\n>  \n> -\t/* dirlen holds the length of the path before the file name */\n> +\t/* Point at the slash at the end of \".../objects/pack/\" */\n>  \tdirlen = buf.len + 1;\n>  \tstrbuf_addf(&buf, \"%s\", packtmp);\n> -\t/* prefixlen holds the length of the prefix */\n> +\t/* Point at the dash at the end of \".../.tmp-%d-pack-\" */\n>  \tprefixlen = buf.len - dirlen;\n>  \n>  \twhile ((e = readdir(dir))) {\n> @@ -109,9 +109,12 @@ static void remove_redundant_pack(const char *path_prefix, const char *hex)\n>  \t}\n>  }\n>  \n> +#define ALL_INTO_ONE 1\n> +#define LOOSE_UNREACHABLE 2\n> +\n>  int cmd_repack(int argc, const char **argv, const char *prefix)\n>  {\n> -\tconst char *exts[2] = {\".idx\", \".pack\"};\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> @@ -124,7 +127,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \n>  \t/* variables to be filled by option parsing */\n>  \tint pack_everything = 0;\n> -\tint pack_everything_but_loose = 0;\n>  \tint delete_redundant = 0;\n>  \tchar *unpack_unreachable = NULL;\n>  \tint window = 0, window_memory = 0;\n> @@ -136,10 +138,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \tint local = 0;\n>  \n>  \tstruct option builtin_repack_options[] = {\n> -\t\tOPT_BOOL('a', NULL, &pack_everything,\n> -\t\t\t\tN_(\"pack everything in a single pack\")),\n> -\t\tOPT_BOOL('A', NULL, &pack_everything_but_loose,\n> -\t\t\t\tN_(\"same as -a, and turn unreachable objects loose\")),\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\"), LOOSE_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> @@ -193,7 +195,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 && !pack_everything_but_loose) {\n> +\tif (!pack_everything) {\n>  \t\targv_array_push(&cmd_args, \"--unpacked\");\n>  \t\targv_array_push(&cmd_args, \"--incremental\");\n>  \t} else {\n> @@ -204,7 +206,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\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_but_loose)\n> +\t\t\telse if (pack_everything & LOOSE_UNREACHABLE)\n>  \t\t\t\targv_array_push(&cmd_args,\n>  \t\t\t\t\t\t\"--unpack-unreachable\");\n>  \t\t}\n> @@ -246,6 +248,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\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> @@ -366,5 +374,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \t\trun_command(&cmd);\n>  \t\targv_array_clear(&cmd_args);\n>  \t}\n> +\tremove_temporary_files();\n>  \treturn 0;\n>  }\n> -- \n> 1.8.4\n>\n> Stefan Beller (2):\n>   repack: rewrite the shell script in C\n>   repack: retain the return value of pack-objects\n>\n>  Makefile                                        |   2 +-\n>  builtin.h                                       |   1 +\n>  builtin/repack.c                                | 379 ++++++++++++++++++++++++\n>  git-repack.sh => contrib/examples/git-repack.sh |   0\n>  git.c                                           |   1 +\n>  5 files changed, 382 insertions(+), 1 deletion(-)\n>  create mode 100644 builtin/repack.c\n>  rename git-repack.sh => contrib/examples/git-repack.sh (100%)\n"},{"id":"226262","messageId":"CAMP44s19gcJAYJ_MbvdoU99EeeHPDj9t+34jBs=MC33VC9nTNw@mail.gmail.com","threadId":"34802","inReplyTo":"1377808774-12505-2-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH 1/2] repack: rewrite the shell script in C","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-29T21:04:54Z","receivedAt":"2013-08-29T21:04:54Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Aug 29, 2013 at 3:39 PM, Stefan Beller\n<stefanbeller@googlemail.com> wrote:\n> The motivation of this patch is to get closer to a goal of being\n> able to have a core subset of git functionality built in to git.\n> That 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\nI think this is great, I'm looking forward to improve the situation in\nthis regard.\n\nDo you have in mind any other command that should also be replaced this way?\n\n> This patch is meant to be mostly a literal translation of the\n> git-repack script; the intent is that later patches would start using\n> more library facilities, but this patch is meant to be as close to a\n> no-op as possible so it doesn't do that kind of thing.\n\nI'm not sure if this has been tackled already, or you could take a\nlook into that, but:\n\nhttp://article.gmane.org/gmane.comp.version-control.git/147190\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"226329","messageId":"522046FF.3030700@googlemail.com","threadId":"34802","inReplyTo":"xmqqob8g2p8t.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv7 0/2] Rewriting repack in C","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-30T07:17:19Z","receivedAt":"2013-08-30T07:17:19Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 08/29/2013 10:53 PM, Junio C Hamano wrote:\n> Stefan Beller <stefanbeller@googlemail.com> writes:\n> \n>> Here is a diff since the last time sending this patch series:\n> \n> This is very readable.\n> \n> There may be people who misread \"LOOSE\" as \"LOSE\"; the option -A is \n> about making the unreachable ones loose so that they can be expired,\n> so let's rename it LOOSEN_UNREACHABLE to avoid confusion.\n> \n> Thanks.\n> \n\nThis would be very appreciated. I am no native speaker, so please\ncorrect any variable naming, which could potentially be missleading.\n\nI've seen you've already put a squashing proposal on top of\norigin/sb/repack-in-c, it looks great, so feel free to squash it\ninto the first commit.\n\nThanks,\nStefan\n\n\n"},{"id":"227691","messageId":"52359D10.1060901@web.de","threadId":"34802","inReplyTo":"1377808774-12505-2-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH 1/2] repack: rewrite the shell script in C","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2013-09-15T11:42:08Z","receivedAt":"2013-09-15T11:42:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 29.08.2013 22:39, schrieb Stefan Beller:\n> The motivation of this patch is to get closer to a goal of being\n> able to have a core subset of git functionality built in to git.\n> That 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>\n> This patch is meant to be mostly a literal translation of the\n> git-repack script; the intent is that later patches would start using\n> more library facilities, but this patch is meant to be as close to a\n> no-op as possible so it doesn't do that kind of thing.\n>\n> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>\n> ---\n>  Makefile                                        |   2 +-\n>  builtin.h                                       |   1 +\n>  builtin/repack.c                                | 379 ++++++++++++++++++++++++\n>  git-repack.sh => contrib/examples/git-repack.sh |   0\n>  git.c                                           |   1 +\n>  5 files changed, 382 insertions(+), 1 deletion(-)\n>  create mode 100644 builtin/repack.c\n>  rename git-repack.sh => contrib/examples/git-repack.sh (100%)\n>\n> diff --git a/Makefile b/Makefile\n> index 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\n> diff --git a/builtin.h b/builtin.h\n> index 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);\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> new file mode 100644\n> index 0000000..0cc823d\n> --- /dev/null\n> +++ b/builtin/repack.c\n> @@ -0,0 +1,379 @@\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> +\tstrbuf_addstr(&buf, packdir);\n\nLet's say packdir is \".git/objects/pack\" (no trailing slash).  Then buf \nis \".git/objects/pack\" now as well.\n\n> +\n> +\t/* Point at the slash at the end of \".../objects/pack/\" */\n> +\tdirlen = buf.len + 1;\n> +\tstrbuf_addf(&buf, \"%s\", packtmp);\n\npacktmp starts with packdir, so by adding it here we have it twice.  buf \nis now \".git/objects/pack.git/objects/pack/.tmp-123-pack\" (if our pid is \n123), no?\n\nPerhaps don't add the packdir to the strbuf in the first place and \ncalculate dirlen as strlen(packdir) + 1?\n\nBesided, strbuf_addstr(x, y) is better for adding a string than \nstrbuf_addf(x, \"%s\", y).\n\n> +\t/* Point at the dash at the end of \".../.tmp-%d-pack-\" */\n> +\tprefixlen = buf.len - dirlen;\n\nThis is the length of \"git/objects/pack/.tmp-123-pack\" now.\n\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\nThis leaks fname for packs with .keep files.\n\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> +}\n\nhex must also include the \"pack-\" prefix and path_prefix must be a \ndirectory name.  Perhaps call them base_name and dir_name or similar to \nmake that clearer?\n\nbuf is leaked.\n\n> +\n> +#define ALL_INTO_ONE 1\n> +#define LOOSE_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\"), LOOSE_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 & LOOSE_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\tfailed = 1;\n> +\t\t\t\tbreak;\n\nThis leaks fname.\n\n> +\t\t\t}\n> +\t\t\tstring_list_append(&rollback, fname);\n\nMental note: This is effectively string_list_append_nodup because \nrollback is initialized with STRING_LIST_INIT_NODUP, so there's no leak \nhere.\n\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\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n\nThe Shell script exits silently in that case.  How about improving error \nreporting in a separate patch?\n\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\tif (remove_path(fname))\n> +\t\t\t\twarning(_(\"removing '%s' failed\"), fname);\n\nSimilar case here: The Shell script continues silently.\n\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\nIf you take care to clear the argv_arrays then perhaps do the same for \nthe string_lists?  The OS cleans up after us anyway, but calming \nValgrind or similar leak checkers is a good practice nevertheless.\n\n> +\treturn 0;\n> +}\n> diff --git a/git-repack.sh b/contrib/examples/git-repack.sh\n> similarity index 100%\n> rename from git-repack.sh\n> rename to contrib/examples/git-repack.sh\n> diff --git a/git.c b/git.c\n> index 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> -- 1.8.4\n>\n"},{"id":"227695","messageId":"5235D2D0.9030203@googlemail.com","threadId":"34802","inReplyTo":"52359D10.1060901@web.de","subject":"Re: [PATCH 1/2] repack: rewrite the shell script in C","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-09-15T15:31:28Z","receivedAt":"2013-09-15T15:31:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Rene, thank you very much for the review!\n\nOn 09/15/2013 01:42 PM, René Scharfe wrote:\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>> +\n>> +    strbuf_addstr(&buf, packdir);\n> \n> Let's say packdir is \".git/objects/pack\" (no trailing slash).  Then buf\n> is \".git/objects/pack\" now as well.\n> \n>> +\n>> +    /* Point at the slash at the end of \".../objects/pack/\" */\n>> +    dirlen = buf.len + 1;\n>> +    strbuf_addf(&buf, \"%s\", packtmp);\n> \n> packtmp starts with packdir, so by adding it here we have it twice.  buf\n> is now \".git/objects/pack.git/objects/pack/.tmp-123-pack\" (if our pid is\n> 123), no?\n> \n> Perhaps don't add the packdir to the strbuf in the first place and\n> calculate dirlen as strlen(packdir) + 1?\n> \n> Besided, strbuf_addstr(x, y) is better for adding a string than\n> strbuf_addf(x, \"%s\", y).\n\nOops, thanks for catching that. I'll be sending a fixup commit.\n\n> \n>> +    /* Point at the dash at the end of \".../.tmp-%d-pack-\" */\n>> +    prefixlen = buf.len - dirlen;\n> \n> This is the length of \"git/objects/pack/.tmp-123-pack\" now.\n\nAlso fixed.\n\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>> +    size_t len;\n>> +\n>> +    if (!(dir = opendir(packdir)))\n>> +        return;\n>> +\n>> +    while ((e = readdir(dir)) != NULL) {\n>> +        if (suffixcmp(e->d_name, \".pack\"))\n>> +            continue;\n>> +\n>> +        len = strlen(e->d_name) - strlen(\".pack\");\n>> +        fname = xmemdupz(e->d_name, len);\n>> +\n>> +        if (!file_exists(mkpath(\"%s/%s.keep\", packdir, fname)))\n>> +            string_list_append_nodup(fname_list, fname);\n> \n> This leaks fname for packs with .keep files.\n> \n\nfixed.\n\n>> +static void remove_redundant_pack(const char *path_prefix, const char\n>> *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>> +    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>> +}\n> \n> hex must also include the \"pack-\" prefix and path_prefix must be a\n> directory name.  Perhaps call them base_name and dir_name or similar to\n> make that clearer?\n> \n> buf is leaked.\n\n\nbuf will be freed.\n\nthe parameter hex contains the \"pack-\" already.\nThe remove_redundant_pack function is called in a loop iterating over\nelements of existing_packs, which is filled in get_non_kept_pack_filenames,\nwhich doesn't fill in the hex, but filenames without extension.\n\nI'll rename the variables to base_name and dir_name as you suggested.\n\n\n>> +    failed = 0;\n>> +    for_each_string_list_item(item, &names) {\n>> +        for (ext = 0; ext < 2; ext++) {\n>> +            char *fname, *fname_old;\n>> +            fname = mkpathdup(\"%s/%s%s\", packdir,\n>> +                        item->string, exts[ext]);\n>> +            if (!file_exists(fname)) {\n>> +                free(fname);\n>> +                continue;\n>> +            }\n>> +\n>> +            fname_old = mkpath(\"%s/old-%s%s\", packdir,\n>> +                        item->string, exts[ext]);\n>> +            if (file_exists(fname_old))\n>> +                if (unlink(fname_old))\n>> +                    failed = 1;\n>> +\n>> +            if (!failed && rename(fname, fname_old)) {\n>> +                failed = 1;\n>> +                break;\n> \n> This leaks fname.\n\nwill fix.\n\n>> +            if (rename(fname_old, fname))\n>> +                die_errno(_(\"renaming '%s' failed\"), fname_old);\n> \n> The Shell script exits silently in that case.  How about \timproving error\n> reporting in a separate patch?\n\nMoved out of the main patch.\n\n\n>> +            if (remove_path(fname))\n>> +                warning(_(\"removing '%s' failed\"), fname);\n> \n> Similar case here: The Shell script continues silently.\n> \n\nhere as well.\n\n> \n> If you take care to clear the argv_arrays then perhaps do the same for\n> the string_lists?  The OS cleans up after us anyway, but calming\n> Valgrind or similar leak checkers is a good practice nevertheless.\n> \n\nI'll do it.\n\n\nSo here are the changes, I'll resend the series as well.\n\n--8<--\nFrom 63c94df8c74c6643d6291c324661a939b9945619 Mon Sep 17 00:00:00 2001\nFrom: Stefan Beller <stefanbeller@googlemail.com>\nDate: Sun, 15 Sep 2013 17:05:49 +0200\nSubject: [PATCH 1/2] Suggestions by Rene\n\n---\n builtin/repack.c | 25 ++++++++++++++++---------\n 1 file changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a0314be..d345d16 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -39,12 +39,10 @@ static void remove_temporary_files(void)\n \tif (!dir)\n \t\treturn;\n \n-\tstrbuf_addstr(&buf, packdir);\n-\n \t/* Point at the slash at the end of \".../objects/pack/\" */\n-\tdirlen = buf.len + 1;\n-\tstrbuf_addf(&buf, \"%s\", packtmp);\n-\t/* Point at the dash at the end of \".../.tmp-%d-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@@ -88,6 +86,8 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list)\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@@ -107,6 +107,7 @@ static void remove_redundant_pack(const char *path_prefix, const char *hex)\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@@ -273,10 +274,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\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\tstring_list_append(&rollback, fname);\n \t\t}\n \t\tif (failed)\n \t\t\tbreak;\n@@ -324,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\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n+\t\t\t\texit(errno);\n \t\t\tfree(fname);\n \t\t\tfree(fname_old);\n \t\t}\n@@ -338,8 +341,7 @@ 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\tif (remove_path(fname))\n-\t\t\t\twarning(_(\"removing '%s' failed\"), fname);\n+\t\t\tremove_path(fname);\n \t\t}\n \t}\n \n@@ -376,5 +378,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\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 }\n-- \n1.8.4.273.ga194ead\n"},{"id":"227787","messageId":"5238B845.5080603@googlemail.com","threadId":"34802","inReplyTo":"5235D2D0.9030203@googlemail.com","subject":"Re: [PATCH 1/2] repack: rewrite the shell script in C","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-09-17T20:15:01Z","receivedAt":"2013-09-17T20:15:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 09/15/2013 05:31 PM, Stefan Beller wrote:\n> Rene, thank you very much for the review!\n\n> \n> the parameter hex contains the \"pack-\" already.\n> The remove_redundant_pack function is called in a loop iterating over\n> elements of existing_packs, which is filled in get_non_kept_pack_filenames,\n> which doesn't fill in the hex, but filenames without extension.\n> \n> I'll rename the variables to base_name and dir_name as you suggested.\n> \n> \n\nI did write a lengthy paragraph, but forgot to actually to it.\n---8<---\n\nFrom 92af79a5a89929d8834ff6d585c0455c867a774f Mon Sep 17 00:00:00 2001\nFrom: Stefan Beller <stefanbeller@googlemail.com>\nDate: Tue, 17 Sep 2013 22:04:14 +0200\nSubject: [PATCH 1/2] Fixup missing by Rene.\n\nSigned-off-by: Stefan Beller <stefanbeller@googlemail.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 f7e91bf..e5f90c6 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -92,14 +92,14 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list)\n \tclosedir(dir);\n }\n \n-static void remove_redundant_pack(const char *path_prefix, const char *hex)\n+static void remove_redundant_pack(const char *dir_name, const char *base_name)\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+\tstrbuf_addf(&buf, \"%s/%s\", dir_name, base_name);\n \tplen = buf.len;\n \n \tfor (i = 0; i < ARRAY_SIZE(exts); i++) {\n-- \n1.8.4.273.ga194ead\n\n\n"}]}