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@xxxxxxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html