[2/3] pack-write: split up finish_tmp_packfile() function

Commit Message

Ævar Arnfjörð Bjarmason Sept. 7, 2021, 7:42 p.m. UTC

Split up the finish_tmp_packfile() function and use the split-up
version in pack-objects.c. This change should not change any
functionality, but sets up code flow for a bug fix where we'll be able
to move the *.idx in-place after the *.bitmap is written.

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
---
 builtin/pack-objects.c |  9 ++++++---
 pack-write.c           | 39 +++++++++++++++++++++++++++++++--------
 pack.h                 |  9 +++++++++
 3 files changed, 46 insertions(+), 11 deletions(-)

Comments

Taylor Blau Sept. 7, 2021, 10:28 p.m. UTC | #1

On Tue, Sep 07, 2021 at 09:42:37PM +0200, Ævar Arnfjörð Bjarmason wrote:
> Split up the finish_tmp_packfile() function and use the split-up
> version in pack-objects.c. This change should not change any
> functionality, but sets up code flow for a bug fix where we'll be able
> to move the *.idx in-place after the *.bitmap is written.

Seems like a logical step to make, and all makes sense to me. One
thought for future clean-up...

> diff --git a/pack-write.c b/pack-write.c
> index 57b9fc11423..ba5c4767dc8 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -468,16 +468,33 @@ void finish_tmp_packfile(const struct strbuf *tmp_basename,
>  			 uint32_t nr_written,
>  			 struct pack_idx_option *pack_idx_opts,
>  			 unsigned char hash[])
> +{
> +	char *idx_tmp_name = NULL;
> +
> +	stage_tmp_packfile(tmp_basename, pack_tmp_name, written_list,
> +			   nr_written, pack_idx_opts, hash, &idx_tmp_name);
> +	rename_tmp_packfile_idx(tmp_basename, hash, &idx_tmp_name);
> +
> +	free(idx_tmp_name);
> +}
> +

I was surprised that you did not replace the caller in write_pack_file
with this (and instead open-coded the implementation). But that makes
sense, since the very next commit is going to move the bitmap into place
between staging and renaming the .idx.

The only other caller of finish_tmp_packfile is in the bulk-checkin
code. So I might suggest that we get rid of this function altogether and
instead call stage_tmp_packfile() and rename_tmp_packfile_idx() directly
in bulk-checkin.c:finish_bulk_checkin().

That would allow us to slim down the header, but more importantly would
make it clear that something like finish_tmp_packfile() shouldn't be
used blindly in case there is other work to be down between staging and
remaing into place.

> -	idx_tmp_name = write_idx_file(NULL, written_list, nr_written,
> -				      pack_idx_opts, hash);
> -	if (adjust_shared_perm(idx_tmp_name))
> +	*idx_tmp_name = (char *)write_idx_file(NULL, written_list, nr_written,
> +					       pack_idx_opts, hash);

Yuck (and unavoidable!)

Thanks,
Taylor

Patch

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 717003563db..9fa4c6a9be8 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1217,6 +1217,7 @@  static void write_pack_file(void)
 		if (!pack_to_stdout) {
 			struct stat st;
 			struct strbuf tmp_basename = STRBUF_INIT;
+			char *idx_tmp_name = NULL;
 
 			/*
 			 * Packs are runtime accessed in their mtime
@@ -1245,9 +1246,10 @@  static void write_pack_file(void)
 					&to_pack, written_list, nr_written);
 			}
 
-			finish_tmp_packfile(&tmp_basename, pack_tmp_name,
-					    written_list, nr_written,
-					    &pack_idx_opts, hash);
+			stage_tmp_packfile(&tmp_basename, pack_tmp_name,
+					   written_list, nr_written,
+					   &pack_idx_opts, hash, &idx_tmp_name);
+			rename_tmp_packfile_idx(&tmp_basename, hash, &idx_tmp_name);
 
 			if (write_bitmap_index) {
 				struct strbuf sb = STRBUF_INIT;
@@ -1267,6 +1269,7 @@  static void write_pack_file(void)
 				strbuf_release(&sb);
 			}
 
+			free(idx_tmp_name);
 			strbuf_release(&tmp_basename);
 			free(pack_tmp_name);
 			puts(hash_to_hex(hash));
diff --git a/pack-write.c b/pack-write.c
index 57b9fc11423..ba5c4767dc8 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -468,16 +468,33 @@  void finish_tmp_packfile(const struct strbuf *tmp_basename,
 			 uint32_t nr_written,
 			 struct pack_idx_option *pack_idx_opts,
 			 unsigned char hash[])
+{
+	char *idx_tmp_name = NULL;
+
+	stage_tmp_packfile(tmp_basename, pack_tmp_name, written_list,
+			   nr_written, pack_idx_opts, hash, &idx_tmp_name);
+	rename_tmp_packfile_idx(tmp_basename, hash, &idx_tmp_name);
+
+	free(idx_tmp_name);
+}
+
+void stage_tmp_packfile(const struct strbuf *tmp_basename,
+			const char *pack_tmp_name,
+			struct pack_idx_entry **written_list,
+			uint32_t nr_written,
+			struct pack_idx_option *pack_idx_opts,
+			unsigned char hash[],
+			char **idx_tmp_name)
 {
 	struct strbuf sb = STRBUF_INIT;
-	const char *idx_tmp_name, *rev_tmp_name = NULL;
+	const char *rev_tmp_name = NULL;
 
 	if (adjust_shared_perm(pack_tmp_name))
 		die_errno("unable to make temporary pack file readable");
 
-	idx_tmp_name = write_idx_file(NULL, written_list, nr_written,
-				      pack_idx_opts, hash);
-	if (adjust_shared_perm(idx_tmp_name))
+	*idx_tmp_name = (char *)write_idx_file(NULL, written_list, nr_written,
+					       pack_idx_opts, hash);
+	if (adjust_shared_perm(*idx_tmp_name))
 		die_errno("unable to make temporary index file readable");
 
 	rev_tmp_name = write_rev_file(NULL, written_list, nr_written, hash,
@@ -496,12 +513,18 @@  void finish_tmp_packfile(const struct strbuf *tmp_basename,
 		strbuf_reset(&sb);
 	}
 
+	strbuf_release(&sb);
+}
+
+void rename_tmp_packfile_idx(const struct strbuf *tmp_basename,
+			     unsigned char hash[], char **idx_tmp_name)
+{
+	struct strbuf sb = STRBUF_INIT;
+
 	strbuf_addf(&sb, "%s%s.idx", tmp_basename->buf, hash_to_hex(hash));
-	if (rename(idx_tmp_name, sb.buf))
+	if (rename(*idx_tmp_name, sb.buf))
 		die_errno("unable to rename temporary index file");
-	strbuf_reset(&sb);
-
-	free((void *)idx_tmp_name);
+	strbuf_release(&sb);
 }
 
 void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)
diff --git a/pack.h b/pack.h
index ae0c9e04cd9..6c1508c3de7 100644
--- a/pack.h
+++ b/pack.h
@@ -110,6 +110,15 @@  int encode_in_pack_object_header(unsigned char *hdr, int hdr_len,
 int read_pack_header(int fd, struct pack_header *);
 
 struct hashfile *create_tmp_packfile(char **pack_tmp_name);
+void stage_tmp_packfile(const struct strbuf *tmp_basename,
+			const char *pack_tmp_name,
+			struct pack_idx_entry **written_list,
+			uint32_t nr_written,
+			struct pack_idx_option *pack_idx_opts,
+			unsigned char hash[],
+			char **idx_tmp_name);
+void rename_tmp_packfile_idx(const struct strbuf *tmp_basename,
+			     unsigned char hash[], char **idx_tmp_name);
 void finish_tmp_packfile(const struct strbuf *name_buffer,
 			 const char *pack_tmp_name,
 			 struct pack_idx_entry **written_list,