Am 21.08.2013 00:36, schrieb Stefan Beller:
I think I got all the suggestions except the
use of git_path/mkpathdup.
I replaced mkpathdup by mkpath where possible,
but it's still not perfect.
I'll wait for the dokumentation patch of Jonathan,
before changing all these occurences forth and back
again.
I trust Jonathan's judgement of how to use git_path, mkpath, and mkpathdup
more than my own. So, please take my earlier comments in this regard with
an appropriately large grain of salt.
Below there is just the diff against RFC PATCHv4,
however I'll send the whole patch as well.
Thanks, that is VERY helpful!
I'll comment here and have a look at the full patch later.
...
int cmd_repack(int argc, const char **argv, const char *prefix) {
You should move the opening brace to the next line, which would then not
be empty anymore.
...
@@ -217,34 +217,34 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {
argv_array_push(&cmd_args, packtmp);
memset(&cmd, 0, sizeof(cmd));
- cmd.argv = argv_array_detach(&cmd_args, NULL);
+ cmd.argv = cmd_args.argv;
cmd.git_cmd = 1;
cmd.out = -1;
cmd.no_stdin = 1;
- if (run_command(&cmd))
+ if (start_command(&cmd))
return 1;
You should have an int ret here and use it like
ret = start_command(&cmd);
if (ret)
return ret;
to retain any exit codes from the sub-process. I know, the script didn't
preserve it:
names=$(git pack-objects ...) || exit 1
but that was not idiomatic as it should have been written as
names=$(git pack-objects ...) || exit
to forward the failure exit code.
- struct string_list names = STRING_LIST_INIT_DUP;
- struct string_list rollback = STRING_LIST_INIT_DUP;
-
- char line[1024];
- int counter = 0;
- FILE *out = xfdopen(cmd.out, "r");
Nice! I missed these decl-after-stmt in my earlier review.
+ count_packs = 0;
+ out = xfdopen(cmd.out, "r");
while (fgets(line, sizeof(line), out)) {
/* a line consists of 40 hex chars + '\n' */
- assert(strlen(line) == 41);
+ if (strlen(line) != 41)
+ die("repack: Expecting 40 character sha1 lines only from pack-objects.");
I agree with Jonathan that you should use strbuf_getline() here.
line[40] = '\0';
string_list_append(&names, line);
- counter++;
+ count_packs++;
}
- if (!counter)
- printf("Nothing new to pack.\n");
+ if (finish_command(&cmd))
+ return 1;
Same as above here:
ret = finish_command(&cmd);
if (ret)
return ret;
I would prefer to see
argv_array_clear(&cmd_args);
here, i.e., at the end of the current use rather than later at the
beginning of the next use. (Ditto for the other uses of cmd_args.)
fclose(out);
This should happen before finish_command(). It doesn't matter if there are
no errors, but if things go awry, closing the channel before
finish_command() avoids deadlocks.
+ if (!count_packs && !quiet)
+ printf("Nothing new to pack.\n");
+
...
@@ -301,33 +299,33 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {
for_each_string_list_item(item, &names) {
for (ext = 0; ext < 2; ext++) {
char *fname, *fname_old;
+ struct stat statbuffer;
fname = mkpathdup("%s/pack-%s%s", packdir, item->string, exts[ext]);
- fname_old = mkpathdup("%s-%s%s", packtmp, item->string, exts[ext]);
- stat(fname_old, &statbuffer);
- statbuffer.st_mode &= ~S_IWUSR | ~S_IWGRP | ~S_IWOTH;
- chmod(fname_old, statbuffer.st_mode);
+ fname_old = mkpath("%s-%s%s", packtmp, item->string, exts[ext]);
+ if (!stat(fname_old, &statbuffer)) {
+ statbuffer.st_mode &= ~S_IWUSR | ~S_IWGRP | ~S_IWOTH;
This is still wrong: it should be one of
... &= ~S_IWUSR & ~S_IWGRP & ~S_IWOTH;
... &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
+ chmod(fname_old, statbuffer.st_mode);
+ }
if (rename(fname_old, fname))
- die("Could not rename packfile: %s -> %s", fname_old, fname);
+ die_errno(_("renaming '%s' failed"), fname_old);
free(fname);
- free(fname_old);
}
}
...
Everything else looks OK. But as I said, mkpath() may have to be reverted
to mkpathdup() as per Jonathans comments.
-- Hannes
--
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