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

Commit Message

Taylor Blau Sept. 9, 2021, 3:25 a.m. UTC

From: Ævar Arnfjörð Bjarmason <[e-mail]>

Refactor the renaming in final() into a helper function, this is
similar in spirit to a preceding refactoring of finish_tmp_packfile()
in pack-write.c.

Before e37d0b8730b (builtin/index-pack.c: write reverse indexes,
2021-01-25) it probably wasn't worth it to have this sort of helper,
due to the differing "else if" case for "pack" files v.s. "idx" files.

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, since we can easily see at a glance that the logic for
writing these three types of files is exactly the same, aside from the
obviously differing cases of "*final_xyz_name" being NULL, and
"else_chmod_if" being different.

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
Signed-off-by: Taylor Blau <[e-mail]>
---
 builtin/index-pack.c | 48 +++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

Comments

Junio C Hamano Sept. 9, 2021, 7:45 p.m. UTC | #1

Taylor Blau <[e-mail]> 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.

> +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.

Taylor Blau Sept. 9, 2021, 9:11 p.m. UTC | #2

On Thu, Sep 09, 2021 at 12:45:04PM -0700, Junio C Hamano wrote:
> Taylor Blau <[e-mail]> 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.
>
> [...]
>
> make-read-only-if-same or somesuch?

Both of these suggestions (dropping the "xyz"s and renaming the last
parameter to "make_read_only_if_same") make sense. Will apply.

Thanks,
Taylor

Patch

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 6cc4890217..cd4e85f5bb 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1477,6 +1477,22 @@  static void write_special_file(const char *suffix, const char *msg,
 	strbuf_release(&name_buf);
 }
 
+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);
+	}
+}
+
 static void final(const char *final_pack_name, const char *curr_pack_name,
 		  const char *final_index_name, const char *curr_index_name,
 		  const char *final_rev_index_name, const char *curr_rev_index_name,
@@ -1505,31 +1521,13 @@  static void final(const char *final_pack_name, const char *curr_pack_name,
 		write_special_file("promisor", promisor_msg, final_pack_name,
 				   hash, NULL);
 
-	if (final_pack_name != curr_pack_name) {
-		if (!final_pack_name)
-			final_pack_name = odb_pack_name(&pack_name, hash, "pack");
-		if (finalize_object_file(curr_pack_name, final_pack_name))
-			die(_("cannot store pack file"));
-	} else if (from_stdin)
-		chmod(final_pack_name, 0444);
-
-	if (final_index_name != curr_index_name) {
-		if (!final_index_name)
-			final_index_name = odb_pack_name(&index_name, hash, "idx");
-		if (finalize_object_file(curr_index_name, final_index_name))
-			die(_("cannot store index file"));
-	} else
-		chmod(final_index_name, 0444);
-
-	if (curr_rev_index_name) {
-		if (final_rev_index_name != curr_rev_index_name) {
-			if (!final_rev_index_name)
-				final_rev_index_name = odb_pack_name(&rev_index_name, hash, "rev");
-			if (finalize_object_file(curr_rev_index_name, final_rev_index_name))
-				die(_("cannot store reverse index file"));
-		} else
-			chmod(final_rev_index_name, 0444);
-	}
+	rename_tmp_packfile(&final_pack_name, curr_pack_name, &pack_name,
+			    hash, "pack", from_stdin);
+	rename_tmp_packfile(&final_index_name, curr_index_name, &index_name,
+			    hash, "idx", 1);
+	if (curr_rev_index_name)
+		rename_tmp_packfile(&final_rev_index_name, curr_rev_index_name,
+				    &rev_index_name, hash, "rev", 1);
 
 	if (do_fsck_object) {
 		struct packed_git *p;