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

[Stefan Beller <stefanbeller@googlemail.com>]

Rene, thank you very much for the review!

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

>> +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.
> 
>> +
>> +    /* 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.

> 
>> +    /* 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.


>> +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.

>> +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.


>> +    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.

>> +            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.


>> +            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 <stefanbeller@googlemail.com>
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