Re: [PATCH 6/9] index-pack: refactor renaming in final()

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 9, 2021, 19:45 UTC
Message-ID: <xmqq35qdzg73.fsf@gitster.g>
In-Reply-To: <3c9b515907ecb632faf73ce8db83efed1493d1f1.1631157880.git.me@ttaylorr.com>

Taylor Blau <me@ttaylorr.com> writes:

> But since we've got "rev" as well now, let's do the renaming via a
> helper, this is both a net decrease in lines, and improves the
> readability,...

xyz_ may be cute, but is distracting. I do not think it loses any information if we used final_name, current_name, etc.; it may make the result even easier to read.

Show 15 quoted lines

> +static void rename_tmp_packfile(const char **final_xyz_name,
> +				const char *curr_xyz_name,
> +				struct strbuf *xyz_name, unsigned char *hash,
> +				const char *ext, int else_chmod_if)
> +{
> +	if (*final_xyz_name != curr_xyz_name) {
> +		if (!*final_xyz_name)
> +			*final_xyz_name = odb_pack_name(xyz_name, hash, ext);
> +		if (finalize_object_file(curr_xyz_name, *final_xyz_name))
> +			die(_("unable to rename temporary '*.%s' file to '%s"),
> +			    ext, *final_xyz_name);
> +	} else if (else_chmod_if) {
> +		chmod(*final_xyz_name, 0444);
> +	}
> +}

"else_chmod_if" is unclear. The caller must be aware of what 'else' refers to and anybody adding a new caller is forced to go look at the body of this helper. I think chmod (or "make_read_only") happens only when the current one already has the final name, so perhaps that is what the name should highlight?

make-read-only-if-same or somesuch?

Thanks.