This patch is meant to be squashed into bb4335a21441a0
(repack: rewrite the shell script in C), I'll do so when rerolling
the series. For reviewing I'll just send this patch.

* Remove comments, which likely get out of date (authorship is kept in
  git anyway)
* rename get_pack_filenames to get_non_kept_pack_filenames
* catch return value of unlink and fail as the shell version did
* beauty fixes to remove_temporary_files as Junio proposed
* install signal handling after static variables packdir, packtmp are set
* remove adding the empty string to the buffer.
* fix the rollback mechanism (wrong variable name)

Signed-off-by: Stefan Beller <stefanbel...@googlemail.com>
---
 builtin/repack.c | 78 ++++++++++++++++++++++++++------------------------------
 1 file changed, 36 insertions(+), 42 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 1f13e0d..e0d1f17 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -1,8 +1,3 @@
-/*
- * The shell version was written by Linus Torvalds (2005) and many others.
- * This is a translation into C by Stefan Beller (2013)
- */
-
 #include "builtin.h"
 #include "cache.h"
 #include "dir.h"
@@ -13,9 +8,8 @@
 #include "string-list.h"
 #include "argv-array.h"
 
-/* enabled by default since 22c79eab (2008-06-25) */
 static int delta_base_offset = 1;
-char *packdir;
+static char *packdir, *packtmp;
 
 static const char *const git_repack_usage[] = {
        N_("git repack [options]"),
@@ -41,18 +35,16 @@ static void remove_temporary_files(void)
        DIR *dir;
        struct dirent *e;
 
-       /* .git/objects/pack */
-       strbuf_addstr(&buf, get_object_directory());
-       strbuf_addstr(&buf, "/pack");
-       dir = opendir(buf.buf);
-       if (!dir) {
-               strbuf_release(&buf);
+       dir = opendir(packdir);
+       if (!dir)
                return;
-       }
 
-       /* .git/objects/pack/.tmp-$$-pack-* */
+       strbuf_addstr(&buf, packdir);
+
+       /* dirlen holds the length of the path before the file name */
        dirlen = buf.len + 1;
-       strbuf_addf(&buf, "/.tmp-%d-pack-", (int)getpid());
+       strbuf_addf(&buf, "%s", packtmp);
+       /* prefixlen holds the length of the prefix */
        prefixlen = buf.len - dirlen;
 
        while ((e = readdir(dir))) {
@@ -73,11 +65,16 @@ static void remove_pack_on_signal(int signo)
        raise(signo);
 }
 
-static void get_pack_filenames(struct string_list *fname_list)
+/*
+ * Adds all packs hex strings to the fname list, which do not
+ * have a corresponding .keep file.
+ */
+static void get_non_kept_pack_filenames(struct string_list *fname_list)
 {
        DIR *dir;
        struct dirent *e;
        char *fname;
+       size_t len;
 
        if (!(dir = opendir(packdir)))
                return;
@@ -86,7 +83,7 @@ static void get_pack_filenames(struct string_list *fname_list)
                if (suffixcmp(e->d_name, ".pack"))
                        continue;
 
-               size_t len = strlen(e->d_name) - strlen(".pack");
+               len = strlen(e->d_name) - strlen(".pack");
                fname = xmemdupz(e->d_name, len);
 
                if (!file_exists(mkpath("%s/%s.keep", packdir, fname)))
@@ -95,14 +92,14 @@ static void get_pack_filenames(struct string_list 
*fname_list)
        closedir(dir);
 }
 
-static void remove_redundant_pack(const char *path, const char *sha1)
+static void remove_redundant_pack(const char *path_prefix, const char *hex)
 {
        const char *exts[] = {".pack", ".idx", ".keep"};
        int i;
        struct strbuf buf = STRBUF_INIT;
        size_t plen;
 
-       strbuf_addf(&buf, "%s/%s", path, sha1);
+       strbuf_addf(&buf, "%s/%s", path_prefix, hex);
        plen = buf.len;
 
        for (i = 0; i < ARRAY_SIZE(exts); i++) {
@@ -115,15 +112,14 @@ static void remove_redundant_pack(const char *path, const 
char *sha1)
 int cmd_repack(int argc, const char **argv, const char *prefix)
 {
        const char *exts[2] = {".idx", ".pack"};
-       char *packtmp;
        struct child_process cmd;
        struct string_list_item *item;
        struct argv_array cmd_args = ARGV_ARRAY_INIT;
        struct string_list names = STRING_LIST_INIT_DUP;
-       struct string_list rollback = STRING_LIST_INIT_DUP;
+       struct string_list rollback = STRING_LIST_INIT_NODUP;
        struct string_list existing_packs = STRING_LIST_INIT_DUP;
        struct strbuf line = STRBUF_INIT;
-       int count_packs, ext, ret;
+       int nr_packs, ext, ret, failed;
        FILE *out;
 
        /* variables to be filled by option parsing */
@@ -173,11 +169,11 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
        argc = parse_options(argc, argv, prefix, builtin_repack_options,
                                git_repack_usage, 0);
 
-       sigchain_push_common(remove_pack_on_signal);
-
        packdir = mkpathdup("%s/pack", get_object_directory());
        packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());
 
+       sigchain_push_common(remove_pack_on_signal);
+
        argv_array_push(&cmd_args, "pack-objects");
        argv_array_push(&cmd_args, "--keep-true-parents");
        argv_array_push(&cmd_args, "--honor-pack-keep");
@@ -201,7 +197,7 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
                argv_array_push(&cmd_args, "--unpacked");
                argv_array_push(&cmd_args, "--incremental");
        } else {
-               get_pack_filenames(&existing_packs);
+               get_non_kept_pack_filenames(&existing_packs);
 
                if (existing_packs.nr && delete_redundant) {
                        if (unpack_unreachable)
@@ -233,14 +229,13 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
        if (ret)
                return ret;
 
-       count_packs = 0;
+       nr_packs = 0;
        out = xfdopen(cmd.out, "r");
        while (strbuf_getline(&line, out, '\n') != EOF) {
                if (line.len != 40)
                        die("repack: Expecting 40 character sha1 lines only 
from pack-objects.");
-               strbuf_addstr(&line, "");
                string_list_append(&names, line.buf);
-               count_packs++;
+               nr_packs++;
        }
        fclose(out);
        ret = finish_command(&cmd);
@@ -248,10 +243,10 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
                return ret;
        argv_array_clear(&cmd_args);
 
-       if (!count_packs && !quiet)
+       if (!nr_packs && !quiet)
                printf("Nothing new to pack.\n");
 
-       int failed = 0;
+       failed = 0;
        for_each_string_list_item(item, &names) {
                for (ext = 0; ext < 2; ext++) {
                        char *fname, *fname_old;
@@ -262,24 +257,23 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
                                continue;
                        }
 
-                       fname_old = mkpathdup("%s/old-%s%s", packdir,
+                       fname_old = mkpath("%s/old-%s%s", packdir,
                                                item->string, exts[ext]);
                        if (file_exists(fname_old))
-                               unlink(fname_old);
+                               if (unlink(fname_old))
+                                       failed = 1;
 
-                       if (rename(fname, fname_old)) {
+                       if (!failed && rename(fname, fname_old)) {
                                failed = 1;
                                break;
                        }
-                       string_list_append_nodup(&rollback, fname);
-                       free(fname);
-                       free(fname_old);
+                       string_list_append(&rollback, fname);
                }
                if (failed)
                        break;
        }
        if (failed) {
-               struct string_list rollback_failure;
+               struct string_list rollback_failure = STRING_LIST_INIT_DUP;
                for_each_string_list_item(item, &rollback) {
                        char *fname, *fname_old;
                        fname = mkpathdup("%s/%s", packdir, item->string);
@@ -289,7 +283,7 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
                        free(fname);
                }
 
-               if (rollback.nr) {
+               if (rollback_failure.nr) {
                        int i;
                        fprintf(stderr,
                                "WARNING: Some packs in use have been renamed 
by\n"
@@ -299,10 +293,10 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
                                "WARNING: attempt to rename them back to 
their\n"
                                "WARNING: original names also failed.\n"
                                "WARNING: Please rename them in %s 
manually:\n", packdir);
-                       for (i = 0; i < rollback.nr; i++)
+                       for (i = 0; i < rollback_failure.nr; i++)
                                fprintf(stderr, "WARNING:   old-%s -> %s\n",
-                                       rollback.items[i].string,
-                                       rollback.items[i].string);
+                                       rollback_failure.items[i].string,
+                                       rollback_failure.items[i].string);
                }
                exit(1);
        }
-- 
1.8.4.rc3.1.gc1ebd90

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to