git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [PATCH] connected: use buffered I/O to talk to rev-list
@ 2020-08-02 14:38 René Scharfe
  2020-08-02 16:08 ` Chris Torek
  2020-08-12 16:52 ` [PATCH v2] " René Scharfe
  0 siblings, 2 replies; 5+ messages in thread
From: René Scharfe @ 2020-08-02 14:38 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

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 and handling errors after the loop.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 connected.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/connected.c b/connected.c
index 937b4bae387..05c2916f38d 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,10 @@ 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;
-			break;
-		}
+		fprintf(rev_list_in, "%s\n", oid_to_hex(&oid));
 	} while (!fn(cb_data, &oid));

-	if (close(rev_list.in))
+	if (fclose(rev_list_in))
 		err = error_errno(_("failed to close rev-list's stdin"));

 	sigchain_pop(SIGPIPE);
--
2.28.0

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] connected: use buffered I/O to talk to rev-list
  2020-08-02 14:38 [PATCH] connected: use buffered I/O to talk to rev-list René Scharfe
@ 2020-08-02 16:08 ` Chris Torek
  2020-08-03 18:17   ` Johannes Sixt
  2020-08-12 16:52 ` [PATCH v2] " René Scharfe
  1 sibling, 1 reply; 5+ messages in thread
From: Chris Torek @ 2020-08-02 16:08 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git Mailing List, Junio C Hamano

On Sun, Aug 2, 2020 at 7:39 AM René Scharfe <l.s.r@web.de> wrote:
> @@ -135,16 +135,10 @@ 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;
> -                       break;
> -               }
> +               fprintf(rev_list_in, "%s\n", oid_to_hex(&oid));
>         } while (!fn(cb_data, &oid));
>
> -       if (close(rev_list.in))
> +       if (fclose(rev_list_in))
>                 err = error_errno(_("failed to close rev-list's stdin"));
>
>         sigchain_pop(SIGPIPE);
> --
> 2.28.0

The same ferror()-before-fclose() remarks apply here too,
but this time the explicit errno checking (EPIPE) cannot
be done -- it's too late, errno is probably overwritten.  I'm
not sure how valuable the explicit errno tests are in the first
place so I will leave that to others, but if we want to keep
the explicit tests, use:

    if (fprintf(...) < 0)

to check each fprintf(), and add a final fflush() call (with
another check) before the fclose().

Chris

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] connected: use buffered I/O to talk to rev-list
  2020-08-02 16:08 ` Chris Torek
@ 2020-08-03 18:17   ` Johannes Sixt
  0 siblings, 0 replies; 5+ messages in thread
From: Johannes Sixt @ 2020-08-03 18:17 UTC (permalink / raw)
  To: Chris Torek, René Scharfe; +Cc: Git Mailing List, Junio C Hamano

Am 02.08.20 um 18:08 schrieb Chris Torek:
> On Sun, Aug 2, 2020 at 7:39 AM René Scharfe <l.s.r@web.de> wrote:
>> @@ -135,16 +135,10 @@ 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;
>> -                       break;
>> -               }
>> +               fprintf(rev_list_in, "%s\n", oid_to_hex(&oid));
>>         } while (!fn(cb_data, &oid));
>>
>> -       if (close(rev_list.in))
>> +       if (fclose(rev_list_in))
>>                 err = error_errno(_("failed to close rev-list's stdin"));
>>
>>         sigchain_pop(SIGPIPE);
> 
> The same ferror()-before-fclose() remarks apply here too,
> but this time the explicit errno checking (EPIPE) cannot
> be done -- it's too late, errno is probably overwritten.

The EPIPE check should really remain in the loop above. (Same comment on
EPIPE on Windows applies here.)

-- Hannes

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2] connected: use buffered I/O to talk to rev-list
  2020-08-02 14:38 [PATCH] connected: use buffered I/O to talk to rev-list René Scharfe
  2020-08-02 16:08 ` Chris Torek
@ 2020-08-12 16:52 ` René Scharfe
  2020-08-13  9:16   ` Jeff King
  1 sibling, 1 reply; 5+ messages in thread
From: René Scharfe @ 2020-08-12 16:52 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Chris Torek, Johannes Sixt

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 <chris.torek@gmail.com>
Helped-by: Johannes Sixt <j6t@kdbg.org>
---
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(-)

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);
--
2.28.0

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] connected: use buffered I/O to talk to rev-list
  2020-08-12 16:52 ` [PATCH v2] " René Scharfe
@ 2020-08-13  9:16   ` Jeff King
  0 siblings, 0 replies; 5+ messages in thread
From: Jeff King @ 2020-08-13  9:16 UTC (permalink / raw)
  To: René Scharfe
  Cc: Git Mailing List, Junio C Hamano, Chris Torek, Johannes Sixt

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-08-13  9:16 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-02 14:38 [PATCH] connected: use buffered I/O to talk to rev-list René Scharfe
2020-08-02 16:08 ` Chris Torek
2020-08-03 18:17   ` Johannes Sixt
2020-08-12 16:52 ` [PATCH v2] " René Scharfe
2020-08-13  9:16   ` Jeff King

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ http://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git