git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC PATCHv4] repack: rewrite the shell script in C.

From
Stefan Beller <stefanbeller@googlemail.com>
Date
Aug 20, 2013, 15:08 UTC
Message-ID
<52138686.1070304@googlemail.com>
In-Reply-To
<52136F9C.6030308@kdbg.org>
On 08/20/2013 03:31 PM, Johannes Sixt wrote:
> 
> Are the long forms of options your invention?

I tried to keep strong similarity with the shell script for ease of review. In the shellscript the options where put in variables having these names, so for example there was:

	-f)	no_reuse=--no-reuse-delta ;;
	-F)	no_reuse=--no-reuse-object ;;

So I used these variable names as well in here. And as I assumed the variables are meaningful in itself.

In the shell script they may be meaningful, but with the option parser in the C version, I overlooked the possibility for --no-<option> being possible as you noted below.

Maybe we should inverse the logic and have the variables and options called reuse-delta and being enabled by default.

Show 9 quoted lines
> 
>> +        OPT_BOOL('a', "all", &pack_everything,
>> +                N_("pack everything in a single pack")),
>> +        OPT_BOOL('A', "all-but-loose", &pack_everything_but_loose,
>> +                N_("same as -a, and turn unreachable objects loose")),
> 
> --all-but-loose does not express what the help text says. The long form
> of -A is --all --unpack-unreachable, so it is really just a short option
> for convenience. It does not need its own long form.

Ok, I'll keep that in mind, and will only use the varialbe tied to -A to set the -a and --unpack-unreachable variable.

Show 9 quoted lines
> 
>> +        OPT_BOOL('d', "delete-redundant", &delete_redundant,
>> +                N_("remove redundant packs, and run git-prune-packed")),
>> +        OPT_BOOL('f', "no-reuse-delta", &no_reuse_delta,
>> +                N_("pass --no-reuse-delta to git-pack-objects")),
>> +        OPT_BOOL('F', "no-reuse-object", &no_reuse_object,
>> +                N_("pass --no-reuse-object to git-pack-objects")),
> 
> Do we want to allow --no-no-reuse-delta and --no-no-reuse-object?
see above, I'd try not to.
Show 5 quoted lines
> 
>> +        OPT_BOOL('n', NULL, &no_update_server_info,
>> +                N_("do not run git-update-server-info")),
> 
> No long option name?

This is also a negated option, so as above, maybe we could have --update_server_info and --no-update_server_info respectively. Talking about the shortform then: Is it possible to negate the shortform?

Show 54 quoted lines
> 
>> +        OPT__QUIET(&quiet, N_("be quiet")),
>> +        OPT_BOOL('l', "local", &local,
>> +                N_("pass --local to git-pack-objects")),
> 
> Good.
> 
>> +        OPT_STRING(0, "unpack-unreachable", &unpack_unreachable,
>> N_("approxidate"),
>> +                N_("with -A, do not loosen objects older than this
>> Packing constraints")),
> 
> "Packing constraints" is a section heading, not a continuation of the
> previous help text.
> 
>> +        OPT_INTEGER(0, "window", &window,
>> +                N_("size of the window used for delta compression")),
> 
> This help text is suboptimal as the option is a count, not a "size" in
> the narrow sense. But that can be changed later (as it would affect
> other tools as well, I guess).
> 
>> +        OPT_INTEGER(0, "window-memory", &window_memory,
>> +                N_("same as the above, but limit memory size instead
>> of entries count")),
>> +        OPT_INTEGER(0, "depth", &depth,
>> +                N_("limits the maximum delta depth")),
>> +        OPT_INTEGER(0, "max-pack-size", &max_pack_size,
>> +                N_("maximum size of each packfile")),
>> +        OPT_END()
>> +    };
> 
> Good.
> 
>> +
>> +    git_config(repack_config, NULL);
>> +
>> +    argc = parse_options(argc, argv, prefix, builtin_repack_options,
>> +                git_repack_usage, 0);
>> +
>> +    sigchain_push_common(remove_pack_on_signal);
> 
> Good.
> 
>> +    packdir = mkpathdup("%s/pack", get_object_directory());
>> +    packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, getpid());
> 
> Should this not be
> 
>     packdir = xstrdup(git_path("pack"));
>     packtmp = xstrdup(git_path("pack/.tmp-%d-pack", getpid()));
> 
> Perhaps make packdir and packtmp global so that the strings need not be
> duplicated in get_pack_filenames and remove_temporary_files?
ok
Show 5 quoted lines
> 
>> +
>> +    remove_temporary_files();
> 
> Yes, the shell script had this. But is it really necessary?
Well I can drop it if it's not needed.
It actually should implement
	rm -f "$PACKTMP"-*
and then the trap 'rm -f "$PACKTMP"-*' 0 1 2 3 15
as well.
Show 5 quoted lines
> 
>> +
>> +    struct argv_array cmd_args = ARGV_ARRAY_INIT;
> 
> Declaration after statement.
will fix.
Show 30 quoted lines
> 
>> +    argv_array_push(&cmd_args, "pack-objects");
>> +    argv_array_push(&cmd_args, "--keep-true-parents");
>> +    argv_array_push(&cmd_args, "--honor-pack-keep");
>> +    argv_array_push(&cmd_args, "--non-empty");
>> +    argv_array_push(&cmd_args, "--all");
>> +    argv_array_push(&cmd_args, "--reflog");
>> +
>> +    if (window)
>> +        argv_array_pushf(&cmd_args, "--window=%u", window);
>> +
>> +    if (window_memory)
>> +        argv_array_pushf(&cmd_args, "--window-memory=%u",
>> window_memory);
>> +
>> +    if (depth)
>> +        argv_array_pushf(&cmd_args, "--depth=%u", depth);
>> +
>> +    if (max_pack_size)
>> +        argv_array_pushf(&cmd_args, "--max_pack_size=%u",
>> max_pack_size);
>> +
>> +    if (no_reuse_delta)
>> +        argv_array_pushf(&cmd_args, "--no-reuse-delta");
>> +
>> +    if (no_reuse_object)
>> +        argv_array_pushf(&cmd_args, "--no-reuse-object");
> 
> no_reuse_delta and no_reuse_object are mutually exclusive, according to
> the shell script version.
I'll change it then to OPT_BIT and die() when both are set.
Show 18 quoted lines
> 
>> +
>> +    if (pack_everything + pack_everything_but_loose == 0) {
>> +        argv_array_push(&cmd_args, "--unpacked");
>> +        argv_array_push(&cmd_args, "--incremental");
>> +    } else {
>> +        struct string_list fname_list = STRING_LIST_INIT_DUP;
>> +        get_pack_filenames(packdir, &fname_list);
>> +        for_each_string_list_item(item, &fname_list) {
>> +            char *fname;
>> +            fname = mkpathdup("%s/%s.keep", packdir, item->string);
>> +            if (stat(fname, &statbuffer) &&
>> S_ISREG(statbuffer.st_mode)) {
> 
>             if (!stat(fname, &statbuffer) && ...
> 
> But you are using file_exists() later. That should be good enough here
> as well, no?
will do.
Show 34 quoted lines
> 
>> +                /* when the keep file is there, we're ignoring that
>> pack */
>> +            } else {
>> +                string_list_append(&existing_packs, item->string);
>> +            }
>> +            free(fname);
>> +        }
>> +
>> +        if (existing_packs.nr && delete_redundant) {
>> +            if (unpack_unreachable)
>> +                argv_array_pushf(&cmd_args,
>> "--unpack-unreachable=%s", unpack_unreachable);
>> +            else if (pack_everything_but_loose)
>> +                argv_array_push(&cmd_args, "--unpack-unreachable");
>> +        }
>> +    }
>> +
>> +    if (local)
>> +        argv_array_push(&cmd_args,  "--local");
>> +    if (quiet)
>> +        argv_array_push(&cmd_args,  "--quiet");
>> +    if (delta_base_offset)
>> +        argv_array_push(&cmd_args,  "--delta-base-offset");
>> +
>> +    argv_array_push(&cmd_args, packtmp);
> 
> Otherwise, argument setup looks fine.
> 
>> +
>> +    memset(&cmd, 0, sizeof(cmd));
>> +    cmd.argv = argv_array_detach(&cmd_args, NULL);
> 
> Is it necessary to detach the arguments?
Probably not.
Show 10 quoted lines
> 
>> +    cmd.git_cmd = 1;
>> +    cmd.out = -1;
>> +    cmd.no_stdin = 1;
>> +
>> +    if (run_command(&cmd))
>> +        return 1;
> 
> You cannot run_command() and then later read its output! You must split
> it into start_command(), read stdout, finish_command().

Thanks for this hint. Could that explain rare non-deterministic failures in the test suite?

Show 6 quoted lines
> 
>> +
>> +    struct string_list names = STRING_LIST_INIT_DUP;
>> +    struct string_list rollback = STRING_LIST_INIT_DUP;
> 
> Declaration after statement.
will fix
Show 14 quoted lines
> 
>> +
>> +    char line[1024];
>> +    int counter = 0;
>> +    FILE *out = xfdopen(cmd.out, "r");
>> +    while (fgets(line, sizeof(line), out)) {
>> +        /* a line consists of 40 hex chars + '\n' */
>> +        assert(strlen(line) == 41);
> 
> You cannot make assertions about input that you read from an external
> command! You can die() if the expectation is not met. But I think that
> in this case the only necessary expectation is that a line is not empty.
> 
> BTW, don't we have strbuf functions to read from an fd linewise?
I'll check.
Show 9 quoted lines
> 
>> +        line[40] = '\0';
>> +        string_list_append(&names, line);
>> +        counter++;
>> +    }
>> +    if (!counter)
>> +        printf("Nothing new to pack.\n");
> 
> This was 'say Nothing new to pack.'. say obeys --quiet, IIRC.
ok
Show 40 quoted lines
> 
>> +    fclose(out);
>> +
>> +    int failed = 0;
>> +    for_each_string_list_item(item, &names) {
>> +        for (ext = 0; ext < 1; ext++) {
>> +            char *fname, *fname_old;
>> +            fname = mkpathdup("%s/%s%s", packdir, item->string,
>> exts[ext]);
>> +            if (!file_exists(fname)) {
>> +                free(fname);
>> +                continue;
>> +            }
>> +
>> +            fname_old = mkpathdup("%s/old-%s%s", packdir,
>> item->string, exts[ext]);
> 
> If you could use git_path() instead of mkpathdup() in these two cases,
> we would not need to free() the names.
> 
>> +            if (file_exists(fname_old))
>> +                unlink(fname_old);
>> +
>> +            if (rename(fname, fname_old)) {
>> +                failed = 1;
>> +                break;
>> +            }
>> +            free(fname_old);
>> +            string_list_append_nodup(&rollback, fname);
> 
> Ah, we would need to allocate here then.
> 
>> +        }
>> +        if (failed)
>> +            /* set to last element to break for_each loop */
>> +            item = names.items + names.nr;
> 
> A mere
>             break;
> doesn't do it here?
Sure! I'll replace by break.
Show 12 quoted lines
> 
>> +    }
>> +    if (failed) {
>> +        struct string_list rollback_failure;
>> +        for_each_string_list_item(item, &rollback) {
>> +            char *fname, *fname_old;
>> +            fname = mkpathdup("%s/%s", packdir, item->string);
>> +            fname_old = mkpathdup("%s/old-%s", packdir, item->string);
> 
> I think it's possible to attach arbitrary data to each string_list item.
> We could attach the "%s/old-%s" name to the item name, then we wouldn't
> need to re-construct the names here.
handy! I'll try to do that.
Show 41 quoted lines
> 
>> +            if (rename(fname_old, fname))
>> +                string_list_append(&rollback_failure, fname);
>> +            free(fname);
>> +            free(fname_old);
>> +        }
>> +
>> +        if (rollback.nr) {
>> +            int i;
>> +            fprintf(stderr,
>> +                "WARNING: Some packs in use have been renamed by\n"
>> +                "WARNING: prefixing old- to their name, in order to\n"
>> +                "WARNING: replace them with the new version of the\n"
>> +                "WARNING: file.  But the operation failed, and the\n"
>> +                "WARNING: attempt to rename them back to their\n"
>> +                "WARNING: original names also failed.\n"
>> +                "WARNING: Please rename them in $PACKDIR manually:\n");
>> +            for (i = 0; i < rollback.nr; i++)
>> +                fprintf(stderr, "WARNING:   old-%s -> %s\n",
>> +                    rollback.items[i].string,
>> +                    rollback.items[i].string);
>> +        }
>> +        exit(1);
>> +    }
>> +
>> +    /* Now the ones with the same name are out of the way... */
>> +    for_each_string_list_item(item, &names) {
>> +        for (ext = 0; ext < 2; ext++) {
>> +            char *fname, *fname_old;
>> +            fname = mkpathdup("%s/pack-%s%s", packdir, item->string,
>> exts[ext]);
>> +            fname_old = mkpathdup("%s-%s%s", packtmp, item->string,
>> exts[ext]);
> 
> Same here: git_path()?
> 
>> +            stat(fname_old, &statbuffer);
> 
> We ignore errors during chmod in the shell script. But this doesn't give
> you license to ignore stat() errors completely: If stat() fails, then
> don't chmod() below, either.
ok
Show 93 quoted lines
> 
>> +            statbuffer.st_mode &= ~S_IWUSR | ~S_IWGRP | ~S_IWOTH;
> 
>             statbuffer.st_mode &= ~(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);
> 
> Use die_errno() here.
> 
>> +            free(fname);
>> +            free(fname_old);
>> +        }
>> +    }
>> +
>> +    /* Remove the "old-" files */
>> +    for_each_string_list_item(item, &names) {
>> +        char *fname;
>> +        fname = mkpathdup("%s/old-pack-%s.idx", packdir, item->string);
>> +        if (remove_path(fname))
>> +            die("Could not remove file: %s", fname);
> 
> die_errno() makes sense here, too.
> 
>> +        free(fname);
>> +
>> +        fname = mkpathdup("%s/old-pack-%s.pack", packdir, item->string);
>> +        if (remove_path(fname))
>> +            die("Could not remove file: %s", fname);
> 
> and here as well.
> 
>> +        free(fname);
> 
> Again git_path?
> 
>> +    }
>> +
>> +    /* End of pack replacement. */
> 
> Nit: A blank line should follow this comment.
> 
>> +    if (delete_redundant) {
>> +        sort_string_list(&names);
>> +        for_each_string_list_item(item, &existing_packs) {
>> +            char *sha1;
>> +            size_t len = strlen(item->string);
>> +            if (len < 40)
>> +                continue;
>> +            sha1 = item->string + len - 40;
>> +            if (!string_list_has_string(&names, sha1))
>> +                remove_pack(packdir, item->string);
>> +        }
> 
> OK.
> 
>> +        argv_array_clear(&cmd_args);
>> +        argv_array_push(&cmd_args, "prune-packed");
>> +        if (quiet)
>> +            argv_array_push(&cmd_args, "--quiet");
>> +
>> +        memset(&cmd, 0, sizeof(cmd));
>> +        cmd.argv = argv_array_detach(&cmd_args, NULL);
> 
> Again: is it necessary to detach?
> 
>> +        cmd.git_cmd = 1;
>> +        run_command(&cmd);
>> +    }
>> +
>> +    if (!no_update_server_info) {
>> +        argv_array_clear(&cmd_args);
>> +        argv_array_push(&cmd_args, "update-server-info");
>> +
>> +        memset(&cmd, 0, sizeof(cmd));
>> +        cmd.argv = argv_array_detach(&cmd_args, NULL);
> 
> Same here?
> 
>> +        cmd.git_cmd = 1;
>> +        run_command(&cmd);
>> +    }
>> +    return 0;
>> +}
> 
> In my opinion, it is good that you keep a large function that resembles
> the structure of the shell script because it is easier to review. But
> ultimately, it should be factored into smaller functions.
> 
> -- Hannes
> 
Hannes,

thank you very much for the review. I'll follow your suggestions and dive deeper into the API to change your annotated lines.

Thanks, Stefan

Previous: Johannes SixtNext: Johannes Sixt
Message 25 of 72 in “Rewriting git-repack in C”
  1. Rewriting git-repack in CStefan Beller, Aug 13, 2013
  2. repack: rewrite the shell script in C.Stefan Beller, Aug 13, 2013
  3. Matthieu MoyAug 14, 2013
  4. Stefan BellerAug 14, 2013
  5. repack: rewrite the shell script in C.Stefan Beller, Aug 14, 2013
  6. Antoine PelisseAug 14, 2013
  7. Stefan BellerAug 14, 2013
  8. Jeff KingAug 14, 2013
  9. Martin FickAug 14, 2013
  10. Stefan BellerAug 14, 2013
  11. Martin FickAug 14, 2013
  12. Junio C HamanoAug 14, 2013
  13. Martin FickAug 14, 2013
  14. Junio C HamanoAug 15, 2013
  15. [RFC PATCHv2] repack: rewrite the shell script in C.Stefan Beller, Aug 16, 2013
  16. René ScharfeAug 17, 2013
  17. Kyle J. McKayAug 17, 2013
  18. Stefan BellerAug 18, 2013
  19. [RFC PATCHv3] repack: rewrite the shell script in C.Stefan Beller, Aug 18, 2013
  20. Kyle J. McKayAug 18, 2013
  21. René ScharfeAug 18, 2013
  22. [RFC PATCHv4] repack: rewrite the shell script in C.Stefan Beller, Aug 18, 2013
  23. [RFC PATCHv4] repack: rewrite the shell script in C.Stefan Beller, Aug 19, 2013
  24. Johannes SixtAug 20, 2013
  25. Stefan BellerAug 20, 2013
  26. Johannes SixtAug 20, 2013
  27. René ScharfeAug 20, 2013
  28. Stefan BellerAug 20, 2013
  29. repack: rewrite the shell script in C.Stefan Beller, Aug 20, 2013
  30. Jonathan NiederAug 21, 2013
  31. Stefan BellerAug 21, 2013
  32. Stefan BellerAug 21, 2013
  33. 1/2 repack: rewrite the shell script in CStefan Beller, Aug 21, 2013
  34. 2/2 repack: retain the return value of pack-objectsStefan Beller, Aug 21, 2013
  35. Junio C HamanoAug 21, 2013
  36. Matthieu MoyAug 21, 2013
  37. Stefan BellerAug 21, 2013
  38. Junio C HamanoAug 21, 2013
  39. Stefan BellerAug 21, 2013
  40. Johannes SixtAug 22, 2013
  41. Jonathan NiederAug 22, 2013
  42. Matthieu MoyAug 21, 2013
  43. Stefan BellerAug 21, 2013
  44. Matthieu MoyAug 21, 2013
  45. Stefan BellerAug 21, 2013
  46. Matthieu MoyAug 21, 2013
  47. Johannes SixtAug 22, 2013
  48. Johannes SixtAug 22, 2013
  49. repack: rewrite the shell script in C (squashing proposal)Stefan Beller, Aug 22, 2013
  50. Junio C HamanoAug 22, 2013
  51. Jonathan NiederAug 20, 2013
  52. Johannes SixtAug 21, 2013
  53. Stefan BellerAug 20, 2013
  54. Jonathan NiederAug 20, 2013
  55. Dokumenting api-paths.txtStefan Beller, Aug 20, 2013
  56. Jonathan NiederAug 20, 2013
  57. Stefan BellerAug 21, 2013
  58. Junio C HamanoAug 22, 2013
  59. Junio C HamanoAug 14, 2013
  60. Matthieu MoyAug 14, 2013
  61. Stefan BellerAug 15, 2013
  62. Duy NguyenAug 15, 2013
  63. Junio C HamanoAug 14, 2013
  64. Matthieu MoyAug 14, 2013
  65. Martin FickAug 14, 2013
  66. Martin FickAug 15, 2013
  67. Stefan BellerAug 15, 2013
  68. Martin FickAug 15, 2013
  69. Duy NguyenAug 15, 2013
  70. Junio C HamanoAug 14, 2013
  71. Stefan BellerAug 15, 2013
  72. Matthieu MoyAug 14, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.