Re: [PATCH] repack: rewrite the shell script in C.
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Aug 21, 2013, 08:49 UTC
- Message-ID
- <vpqhaeje8e0.fsf@anie.imag.fr>
- In-Reply-To
- <1377038334-15799-1-git-send-email-stefanbeller@googlemail.com>
Stefan Beller <stefanbeller@googlemail.com> writes:
> All tests are constantly positive now.
Cool!
> +/* > + * Fills the filename list with all the files found in the pack directory
Detail: "filename list" could be "fname_list" to match the actual argument below.
Show 12 quoted lines
> + * ending with .pack, without that extension.
> + */
> +void get_pack_filenames(char *packdir, struct string_list *fname_list)
> +{
> + DIR *dir;
> + struct dirent *e;
> + char *path, *suffix, *fname;
> +
> + path = mkpath("%s/pack", get_object_directory());
> + suffix = ".pack";
> +
> + dir = opendir(path);I think you should test and complain if dir is NULL ("cannot open pack directory: ...")
Show 8 quoted lines
> +void remove_pack(char *path, char* sha1)
> +{
> + char *exts[] = {".pack", ".idx", ".keep"};
> + int ext = 0;
> + for (ext = 0; ext < 3; ext++) {
> + char *fname;
> + fname = mkpath("%s/%s%s", path, sha1, exts[ext]);
> + unlink(fname);Here also, the return value from unlink is not checked. Probably not serious (mistakenly deleting a pack file would be very serious, but keeping it around by mistake shouldn't harm), but a warning message may be welcome.
These kinds of warnings are never shown in normal usage, but may be welcome when something goes really wrong with the repo, as a diagnosis tool for the user. The shell version had these warnings implicitly since "rm" displays the message on stderr when it fails.
> + struct child_process cmd; > + struct argv_array cmd_args = ARGV_ARRAY_INIT;
Since the command is going to be "pack-objects", you may call the variables pack_cmd and pack_cmd_args or so.
Show 17 quoted lines
> + 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); > + > + memset(&cmd, 0, sizeof(cmd)); > + cmd.argv = cmd_args.argv; > + cmd.git_cmd = 1; > + cmd.out = -1; > + cmd.no_stdin = 1; > + > + if (start_command(&cmd)) > + return 1;
A warning message would be welcome in addition to returning 1.
> + if (!count_packs && !quiet)
> + printf("Nothing new to pack.\n");
> +
> + int failed = 0;Don't declare variables inside code, it's not C90.
Show 12 quoted lines
> + 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))
> + unlink(fname_old);Unchecked returned value.
Show 10 quoted lines
> + if (rename(fname, fname_old)) {
> + failed = 1;
> + break;
> + }
> + string_list_append_nodup(&rollback, fname);
> + free(fname);
> + }
> + if (failed)
> + break;
> + }I tend to dislike these "set a variable and break twice" to exit nested loops. Using an auxiliary function, you could just do
int f()
{
for_each {
for () {
...
if ()
return 1;
...
}
}
return 0;
}(Matter of taste, though. Some people may disagree)
A good side effect would be to move some code out of cmd_repack, which is rather long.
Show 10 quoted lines
> + /* 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;
> + struct stat statbuffer;
> + fname = mkpathdup("%s/pack-%s%s", packdir, item->string, exts[ext]);
> + 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;
> + chmod(fname_old, statbuffer.st_mode);Unchecked return value.
Show 10 quoted lines
> + /* Remove the "old-" files */
> + for_each_string_list_item(item, &names) {
> + char *fname;
> + fname = mkpath("%s/old-pack-%s.idx", packdir, item->string);
> + if (remove_path(fname))
> + die_errno(_("removing '%s' failed"), fname);
> +
> + fname = mkpath("%s/old-pack-%s.pack", packdir, item->string);
> + if (remove_path(fname))
> + die_errno(_("removing '%s' failed"), fname);Does this have to be a fatal error? If I read correctly, it wasn't fatal in the shell version.
Any reason why you duplicate the code for .idx and .pack here, while you iterate over an ext array in other places of the code?
Show 21 quoted lines
> + 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);
> + }
> + 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 = cmd_args.argv;
> + cmd.git_cmd = 1;
> + run_command(&cmd);
> + }It's tempting to call prune_packed_objects() directly here, but it's implemented in builtin/ so it would require a refactoring patch to be moved to libgit.a before I guess.
Show 11 quoted lines
> + 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 = cmd_args.argv;
> + cmd.git_cmd = 1;
> + run_command(&cmd);
> + }
> + return 0;
> +}Any reason to fork a new process instead of just calling update_server_info() directly?
Not that efficiency matters here, but the code would be a bit simpler.
-- Matthieu Moy http://www-verimag.imag.fr/~moy/