Re: [PATCH 1/2] repack: rewrite the shell script in C

Rene, thank you very much for the review!

On 09/15/2013 01:42 PM, René Scharfe wrote:

quoted

+static void remove_temporary_files(void)
+{
+    struct strbuf buf = STRBUF_INIT;
+    size_t dirlen, prefixlen;
+    DIR *dir;
+    struct dirent *e;
+
+    dir = opendir(packdir);
+    if (!dir)
+        return;
+
+    strbuf_addstr(&buf, packdir);

Let's say packdir is ".git/objects/pack" (no trailing slash).  Then buf
is ".git/objects/pack" now as well.

quoted

+
+    /* Point at the slash at the end of ".../objects/pack/" */
+    dirlen = buf.len + 1;
+    strbuf_addf(&buf, "%s", packtmp);

packtmp starts with packdir, so by adding it here we have it twice.  buf
is now ".git/objects/pack.git/objects/pack/.tmp-123-pack" (if our pid is
123), no?

Perhaps don't add the packdir to the strbuf in the first place and
calculate dirlen as strlen(packdir) + 1?

Besided, strbuf_addstr(x, y) is better for adding a string than
strbuf_addf(x, "%s", y).

Oops, thanks for catching that. I'll be sending a fixup commit.

quoted

+    /* Point at the dash at the end of ".../.tmp-%d-pack-" */
+    prefixlen = buf.len - dirlen;

This is the length of "git/objects/pack/.tmp-123-pack" now.

Also fixed.

quoted

+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;
+
+    while ((e = readdir(dir)) != NULL) {
+        if (suffixcmp(e->d_name, ".pack"))
+            continue;
+
+        len = strlen(e->d_name) - strlen(".pack");
+        fname = xmemdupz(e->d_name, len);
+
+        if (!file_exists(mkpath("%s/%s.keep", packdir, fname)))
+            string_list_append_nodup(fname_list, fname);

This leaks fname for packs with .keep files.

fixed.

quoted

+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_prefix, hex);
+    plen = buf.len;
+
+    for (i = 0; i < ARRAY_SIZE(exts); i++) {
+        strbuf_setlen(&buf, plen);
+        strbuf_addstr(&buf, exts[i]);
+        unlink(buf.buf);
+    }
+}

hex must also include the "pack-" prefix and path_prefix must be a
directory name.  Perhaps call them base_name and dir_name or similar to
make that clearer?

buf is leaked.

buf will be freed.

the parameter hex contains the "pack-" already.
The remove_redundant_pack function is called in a loop iterating over
elements of existing_packs, which is filled in get_non_kept_pack_filenames,
which doesn't fill in the hex, but filenames without extension.

I'll rename the variables to base_name and dir_name as you suggested.

quoted

+    failed = 0;
+    for_each_string_list_item(item, &names) {
+        for (ext = 0; ext < 2; ext++) {
+            char *fname, *fname_old;
+            fname = mkpathdup("%s/%s%s", packdir,
+                        item->string, exts[ext]);
+            if (!file_exists(fname)) {
+                free(fname);
+                continue;
+            }
+
+            fname_old = mkpath("%s/old-%s%s", packdir,
+                        item->string, exts[ext]);
+            if (file_exists(fname_old))
+                if (unlink(fname_old))
+                    failed = 1;
+
+            if (!failed && rename(fname, fname_old)) {
+                failed = 1;
+                break;

This leaks fname.

will fix.

quoted

+            if (rename(fname_old, fname))
+                die_errno(_("renaming '%s' failed"), fname_old);

The Shell script exits silently in that case.  How about 	improving error
reporting in a separate patch?

Moved out of the main patch.

quoted

+            if (remove_path(fname))
+                warning(_("removing '%s' failed"), fname);

Similar case here: The Shell script continues silently.

here as well.

If you take care to clear the argv_arrays then perhaps do the same for
the string_lists?  The OS cleans up after us anyway, but calming
Valgrind or similar leak checkers is a good practice nevertheless.

I'll do it.


So here are the changes, I'll resend the series as well.

--8<--
From 63c94df8c74c6643d6291c324661a939b9945619 Mon Sep 17 00:00:00 2001
From: Stefan Beller <redacted>
Date: Sun, 15 Sep 2013 17:05:49 +0200
Subject: [PATCH 1/2] Suggestions by Rene

---
 builtin/repack.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index a0314be..d345d16 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c

@@ -39,12 +39,10 @@ static void remove_temporary_files(void)
 	if (!dir)
 		return;
 
-	strbuf_addstr(&buf, packdir);
-
 	/* Point at the slash at the end of ".../objects/pack/" */
-	dirlen = buf.len + 1;
-	strbuf_addf(&buf, "%s", packtmp);
-	/* Point at the dash at the end of ".../.tmp-%d-pack-" */
+	dirlen = strlen(packdir) + 1;
+	strbuf_addstr(&buf, packtmp);
+	/* Hold the length of  ".tmp-%d-pack-" */
 	prefixlen = buf.len - dirlen;
 
 	while ((e = readdir(dir))) {

@@ -88,6 +86,8 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list)
 
 		if (!file_exists(mkpath("%s/%s.keep", packdir, fname)))
 			string_list_append_nodup(fname_list, fname);
+		else
+			free(fname);
 	}
 	closedir(dir);
 }

@@ -107,6 +107,7 @@ static void remove_redundant_pack(const char *path_prefix, const char *hex)
 		strbuf_addstr(&buf, exts[i]);
 		unlink(buf.buf);
 	}
+	strbuf_release(&buf);
 }
 
 #define ALL_INTO_ONE 1

@@ -273,10 +274,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 					failed = 1;
 
 			if (!failed && rename(fname, fname_old)) {
+				free(fname);
 				failed = 1;
 				break;
+			} else {
+				string_list_append(&rollback, fname);
 			}
-			string_list_append(&rollback, fname);
 		}
 		if (failed)
 			break;

@@ -324,7 +327,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				chmod(fname_old, statbuffer.st_mode);
 			}
 			if (rename(fname_old, fname))
-				die_errno(_("renaming '%s' failed"), fname_old);
+				exit(errno);
 			free(fname);
 			free(fname_old);
 		}

@@ -338,8 +341,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 					packdir,
 					item->string,
 					exts[ext]);
-			if (remove_path(fname))
-				warning(_("removing '%s' failed"), fname);
+			remove_path(fname);
 		}
 	}
 

@@ -376,5 +378,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		argv_array_clear(&cmd_args);
 	}
 	remove_temporary_files();
+	string_list_clear(&names, 0);
+	string_list_clear(&rollback, 0);
+	string_list_clear(&existing_packs, 0);
+	strbuf_release(&line);
+
 	return 0;
 }

-- 
1.8.4.273.ga194ead