[PATCH 1/2] fetch-pack: rename helper to create_promisor_file()

As we are going to refactor the code that actually writes
the promisor file into a separate function in a following
commit, let's rename the current write_promisor_file()
function to create_promisor_file().

Signed-off-by: Christian Couder <redacted>
---
 fetch-pack.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 876f90c759..c5fa4992a6 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c

@@ -772,8 +772,8 @@ static int sideband_demux(int in, int out, void *data)
 	return ret;
 }
 
-static void write_promisor_file(const char *keep_name,
-				struct ref **sought, int nr_sought)
+static void create_promisor_file(const char *keep_name,
+				 struct ref **sought, int nr_sought)
 {
 	struct strbuf promisor_name = STRBUF_INIT;
 	int suffix_stripped;

@@ -875,7 +875,7 @@ static int get_pack(struct fetch_pack_args *args,
 
 		if (args->from_promisor)
 			/*
-			 * write_promisor_file() may be called afterwards but
+			 * create_promisor_file() may be called afterwards but
 			 * we still need index-pack to know that this is a
 			 * promisor pack. For example, if transfer.fsckobjects
 			 * is true, index-pack needs to know that .gitmodules

@@ -943,7 +943,7 @@ static int get_pack(struct fetch_pack_args *args,
 	 * obtained .keep filename if necessary
 	 */
 	if (do_keep && pack_lockfiles && pack_lockfiles->nr && args->from_promisor)
-		write_promisor_file(pack_lockfiles->items[0].string, sought, nr_sought);
+		create_promisor_file(pack_lockfiles->items[0].string, sought, nr_sought);
 
 	return 0;
 }

-- 
2.30.0.83.g36fd80b35a

Let's replace the 2 different pieces of code that write a
promisor file in 'builtin/repack.c' and 'fetch-pack.c'
with a new function called 'write_promisor_file()' in
'pack-write.c' and 'pack.h'.

This might also help us in the future, if we want to put
back the ref names and associated hashes that were in
the promisor files we are repacking in 'builtin/repack.c'
as suggested by a NEEDSWORK comment just above the code
we are refactoring.

Signed-off-by: Christian Couder <redacted>
---
 builtin/repack.c |  8 +++-----
 fetch-pack.c     |  8 +-------
 pack-write.c     | 12 ++++++++++++
 pack.h           |  4 ++++
 4 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 279be11a16..2158b48f4c 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c

@@ -14,6 +14,7 @@
 #include "object-store.h"
 #include "promisor-remote.h"
 #include "shallow.h"
+#include "pack.h"
 
 static int delta_base_offset = 1;
 static int pack_kept_objects = -1;

@@ -263,7 +264,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
 	while (strbuf_getline_lf(&line, out) != EOF) {
 		struct string_list_item *item;
 		char *promisor_name;
-		int fd;
+
 		if (line.len != the_hash_algo->hexsz)
 			die(_("repack: Expecting full hex object ID lines only from pack-objects."));
 		item = string_list_append(names, line.buf);

@@ -281,10 +282,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
 		 */
 		promisor_name = mkpathdup("%s-%s.promisor", packtmp,
 					  line.buf);
-		fd = open(promisor_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
-		if (fd < 0)
-			die_errno(_("unable to create '%s'"), promisor_name);
-		close(fd);
+		write_promisor_file(promisor_name, NULL, 0);
 
 		item->util = (void *)(uintptr_t)populate_pack_exts(item->string);
 

diff --git a/fetch-pack.c b/fetch-pack.c
index c5fa4992a6..1eaedcb5dc 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c

@@ -777,8 +777,6 @@ static void create_promisor_file(const char *keep_name,
 {
 	struct strbuf promisor_name = STRBUF_INIT;
 	int suffix_stripped;
-	FILE *output;
-	int i;
 
 	strbuf_addstr(&promisor_name, keep_name);
 	suffix_stripped = strbuf_strip_suffix(&promisor_name, ".keep");

@@ -787,11 +785,7 @@ static void create_promisor_file(const char *keep_name,
 		    keep_name);
 	strbuf_addstr(&promisor_name, ".promisor");
 
-	output = xfopen(promisor_name.buf, "w");
-	for (i = 0; i < nr_sought; i++)
-		fprintf(output, "%s %s\n", oid_to_hex(&sought[i]->old_oid),
-			sought[i]->name);
-	fclose(output);
+	write_promisor_file(promisor_name.buf, sought, nr_sought);
 
 	strbuf_release(&promisor_name);
 }

diff --git a/pack-write.c b/pack-write.c
index 3513665e1e..db3ff9980f 100644
--- a/pack-write.c
+++ b/pack-write.c

@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "pack.h"
 #include "csum-file.h"
+#include "remote.h"
 
 void reset_pack_idx_option(struct pack_idx_option *opts)
 {

@@ -367,3 +368,14 @@ void finish_tmp_packfile(struct strbuf *name_buffer,
 
 	free((void *)idx_tmp_name);
 }
+
+void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)
+{
+	int i;
+	FILE *output = xfopen(promisor_name, "w");
+
+	for (i = 0; i < nr_sought; i++)
+		fprintf(output, "%s %s\n", oid_to_hex(&sought[i]->old_oid),
+			sought[i]->name);
+	fclose(output);
+}

diff --git a/pack.h b/pack.h
index 9fc0945ac9..9ae640f417 100644
--- a/pack.h
+++ b/pack.h

@@ -87,6 +87,10 @@ off_t write_pack_header(struct hashfile *f, uint32_t);
 void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);
 char *index_pack_lockfile(int fd);
 
+struct ref;
+
+void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought);
+
 /*
  * The "hdr" output buffer should be at least this big, which will handle sizes
  * up to 2^67.

-- 
2.30.0.83.g36fd80b35a

On Tue, Jan 12, 2021 at 09:21:59AM +0100, Christian Couder wrote:

Let's replace the 2 different pieces of code that write a
promisor file in 'builtin/repack.c' and 'fetch-pack.c'
with a new function called 'write_promisor_file()' in
'pack-write.c' and 'pack.h'.

This is just great. Even though I'm sure that I have read that code in
builtin/repack.c a hundred times before, I for some reason always have
to remind myself what it is doing.

So, I'm very happy to see that it is being cleaned up behind a function
whose name describes its behavior clearly. And, if that function can be
used to remove some duplication in a sensible fashion along the way,
then all the better.

  Reviewed-by: Taylor Blau [off-list ref]

Thanks,
Taylor

On Tue, Jan 12, 2021 at 09:21:59AM +0100, Christian Couder wrote:

Let's replace the 2 different pieces of code that write a
promisor file in 'builtin/repack.c' and 'fetch-pack.c'
with a new function called 'write_promisor_file()' in
'pack-write.c' and 'pack.h'.

This might also help us in the future, if we want to put
back the ref names and associated hashes that were in
the promisor files we are repacking in 'builtin/repack.c'
as suggested by a NEEDSWORK comment just above the code
we are refactoring.

I think the interface for the callers is much nicer. Not a new problem,
but one thing I did notice in the implementation:

+void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)
+{
+	int i;
+	FILE *output = xfopen(promisor_name, "w");
+
+	for (i = 0; i < nr_sought; i++)
+		fprintf(output, "%s %s\n", oid_to_hex(&sought[i]->old_oid),
+			sought[i]->name);
+	fclose(output);
+}

We check errors on open via xfopen(), which is good. But we would not
notice any problems writing via fprintf or fclose. Is it worth doing
something like:

  err = ferror(output);
  err |= fclose(output);
  return err ? -1 : 0;

?

(As an aside, this ferror/fclose dance is awkward enough and has caused
us enough questions in the past that I wonder if it is worth
encapsulating into a wrapper).

The existing callers behave the same way (checking open, but not the
writes), so definitely not a regression. But the helper function may
provide an opportunity to make things more robust without adding a lot
of duplicated code.

-Peff

On Wed, Jan 13, 2021 at 08:25:40AM -0500, Jeff King wrote:

quoted

+void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)
+{
+	int i;
+	FILE *output = xfopen(promisor_name, "w");
+
+	for (i = 0; i < nr_sought; i++)
+		fprintf(output, "%s %s\n", oid_to_hex(&sought[i]->old_oid),
+			sought[i]->name);
+	fclose(output);
+}

We check errors on open via xfopen(), which is good. But we would not
notice any problems writing via fprintf or fclose. Is it worth doing
something like:

  err = ferror(output);
  err |= fclose(output);
  return err ? -1 : 0;

?

I agree below that *not* doing this isn't a regression against the
current code, since it doesn't check either, but this could be done
relatively easily. It is appropriate for both callers of
write_promisor_file() to immediately die() if they get an error, so I
think that this is potentially worth doing.

(As an aside, this ferror/fclose dance is awkward enough and has caused
us enough questions in the past that I wonder if it is worth
encapsulating into a wrapper).

From a quick grep through uses of ferror, there are a reasonable handful
of spots that I think could be improved if there was a ferror+fclose
helper, perhaps: xfclose().

The existing callers behave the same way (checking open, but not the
writes), so definitely not a regression. But the helper function may
provide an opportunity to make things more robust without adding a lot
of duplicated code.

Yep.

-Peff

Thanks,
Taylor