Re: [PATCH 21/49] builtin/repack.c: factor our "generated_pack_install"

Patrick Steinhardt
2025-09-29T23:21:52Z

On Sun, Sep 28, 2025 at 06:08:31PM -0400, Taylor Blau wrote:

Is the "factor our" in the commit subject supposed to read "factor out"?

> diff --git a/builtin/repack.c b/builtin/repack.c
> index a4d80b6b04..8c3a5f4f80 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -1434,35 +1466,9 @@ int cmd_repack(int argc,
> /*
> * Ok we have prepared all new packfiles.
> */
> - for_each_string_list_item(item, &names) {
> - struct generated_pack *pack = item->util;
> -
> - for (ext = 0; ext < ARRAY_SIZE(exts); ext++) {
> - char *fname;
> -
> - fname = mkpathdup("%s/pack-%s%s",
> - packdir, item->string, exts[ext].name);
> -
> - if (pack->tempfiles[ext]) {
> - const char *fname_old = get_tempfile_path(pack->tempfiles[ext]);
> - struct stat statbuffer;
> -
> - if (!stat(fname_old, &statbuffer)) {
> - statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
> - chmod(fname_old, statbuffer.st_mode);
> - }
> -
> - if (rename_tempfile(&pack->tempfiles[ext], fname))
> - die_errno(_("renaming pack to '%s' failed"), fname);
> - } else if (!exts[ext].optional)
> - die(_("pack-objects did not write a '%s' file for pack %s-%s"),
> - exts[ext].name, packtmp, item->string);
> - else if (unlink(fname) < 0 && errno != ENOENT)
> - die_errno(_("could not unlink: %s"), fname);
> -
> - free(fname);
> - }
> - }
> + for_each_string_list_item(item, &names)
> + generated_pack_install((struct generated_pack *)item->util,

This cast should be unnecessary, right? `item->util` is a void pointer,
so C should do the cast implicitly.

Patrick