Re: [PATCH 1/2] repack: rewrite the shell script in C
- From
Stefan Beller <stefanbeller@googlemail.com>
- Date
- Sep 15, 2013, 15:31 UTC
- Message-ID
- <5235D2D0.9030203@googlemail.com>
- In-Reply-To
- <52359D10.1060901@web.de>
Rene, thank you very much for the review!
On 09/15/2013 01:42 PM, René Scharfe wrote:
Show 30 quoted lines
>> +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.
Show 5 quoted lines
> >> + /* 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.
Show 22 quoted lines
>> +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.
Show 23 quoted lines
>> +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.
Show 22 quoted lines
>> + 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.
Show 5 quoted lines
>> + 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.
Show 5 quoted lines
>> + if (remove_path(fname))
>> + warning(_("removing '%s' failed"), fname);
>
> Similar case here: The Shell script continues silently.
> here as well.
Show 5 quoted lines
> > 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