[v2] connected: use buffered I/O to talk to rev-list

Commit Message

René Scharfe Aug. 12, 2020, 4:52 p.m. UTC

Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,
2016-06-08), significantly reduce the number of system calls and
simplify the code for sending object IDs to rev-list by using stdio's
buffering.

Take care to handle errors immediately to get the correct error code,
and to flush the buffer explicitly before closing the stream in order to
catch any write errors for these last bytes.

Helped-by: Chris Torek <[e-mail]>
Helped-by: Johannes Sixt <[e-mail]>
---
Changes since v1:
- Handle fprintf() errors immediately.
- Call ferror() and fflush() explicitly before calling fclose().
- Report write errors other than EPIPE and EINVAL, like the original
  code does.

 connected.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

--
2.28.0

Comments

Jeff King Aug. 13, 2020, 9:16 a.m. UTC | #1

On Wed, Aug 12, 2020 at 06:52:49PM +0200, René Scharfe wrote:

> Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,
> 2016-06-08), significantly reduce the number of system calls and
> simplify the code for sending object IDs to rev-list by using stdio's
> buffering.
> 
> Take care to handle errors immediately to get the correct error code,
> and to flush the buffer explicitly before closing the stream in order to
> catch any write errors for these last bytes.

FWIW, the error handling in this patch and the other stdio conversions
you sent all look good to me. Thanks (and to Chris and Johannes for
great review on v1).

-Peff

Patch

diff --git a/connected.c b/connected.c
index 21c1ebe9fbf..b18299fdf0e 100644
--- a/connected.c
+++ b/connected.c
@@ -22,14 +22,13 @@  int check_connected(oid_iterate_fn fn, void *cb_data,
 		    struct check_connected_options *opt)
 {
 	struct child_process rev_list = CHILD_PROCESS_INIT;
+	FILE *rev_list_in;
 	struct check_connected_options defaults = CHECK_CONNECTED_INIT;
-	char commit[GIT_MAX_HEXSZ + 1];
 	struct object_id oid;
 	int err = 0;
 	struct packed_git *new_pack = NULL;
 	struct transport *transport;
 	size_t base_len;
-	const unsigned hexsz = the_hash_algo->hexsz;

 	if (!opt)
 		opt = &defaults;
@@ -122,7 +121,8 @@  int check_connected(oid_iterate_fn fn, void *cb_data,

 	sigchain_push(SIGPIPE, SIG_IGN);

-	commit[hexsz] = '\n';
+	rev_list_in = xfdopen(rev_list.in, "w");
+
 	do {
 		/*
 		 * If index-pack already checked that:
@@ -135,16 +135,17 @@  int check_connected(oid_iterate_fn fn, void *cb_data,
 		if (new_pack && find_pack_entry_one(oid.hash, new_pack))
 			continue;

-		memcpy(commit, oid_to_hex(&oid), hexsz);
-		if (write_in_full(rev_list.in, commit, hexsz + 1) < 0) {
-			if (errno != EPIPE && errno != EINVAL)
-				error_errno(_("failed write to rev-list"));
-			err = -1;
+		if (fprintf(rev_list_in, "%s\n", oid_to_hex(&oid)) < 0)
 			break;
-		}
 	} while (!fn(cb_data, &oid));

-	if (close(rev_list.in))
+	if (ferror(rev_list_in) || fflush(rev_list_in)) {
+		if (errno != EPIPE && errno != EINVAL)
+			error_errno(_("failed write to rev-list"));
+		err = -1;
+	}
+
+	if (fclose(rev_list_in))
 		err = error_errno(_("failed to close rev-list's stdin"));

 	sigchain_pop(SIGPIPE);