From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 29 Sep 2025 23:21:46 GMT
Subject: Re: [PATCH 21/49] builtin/repack.c: factor our "generated_pack_install"
Message-ID: <aNsUiho56-S4MQZg@pks.im>
In-Reply-To: <96d27261e75fd2ae46d9633b3d4eff243dbcc187.1759097191.git.me@ttaylorr.com>

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