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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to