Re: [PATCH 2/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline_fd

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 23, 2017, 22:50 UTC
Message-ID: <20170323225019.GD20794@aiede.mtv.corp.google.com>
In-Reply-To: <20170323223338.32274-3-sbeller@google.com>

Stefan Beller wrote:

Show 7 quoted lines

> Instead of implementing line reading yet again, make use of our beautiful
> library functions.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)

This changes buffering behavior in two ways:

- by using strbuf_getwholeline_fd instead of strbuf_read, we avoid
  having to allocate memory for the entire child process output at
  once.  That is, we limit maximum memory usage (good).

- by using strbuf_getwholeline_fd instead of strbuf_read, we xread
  one byte at a time instead of larger chunks.  That means more
  overhead due to context switches (bad).

Some callers of getwholeline_fd need the one-byte-at-a-time thing to avoid waiting too long for input, and in some cases the alternative is deadlock. We know this caller doesn't fall into that category because it was doing fine slurping the entire file at once. As the getwholeline_fd API doc comment explains:

 * It reads one character at a time, so it is very slow.  Do not
 * use it unless you need the correct position in the file
 * descriptor.

Can this caller use xfdopen and strbuf_getwholeline instead to get back the benefit of buffering (i.e., something like the below)?

Another consequence of switching to streaming is that we may close before the child finishes. Do we have to worry about handling SIGPIPE in the child? I haven't checked how this handles that --- a test might be useful.

Thanks and hope that helps, Jonathan

diff --git i/submodule.c w/submodule.c
index c1b7b78260..184d5739fc 100644
--- i/submodule.c
+++ w/submodule.c
@@ -1043,6 +1043,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT;
+	FILE *fp;
 	unsigned dirty_submodule = 0;
 	const char *git_dir;
 
@@ -1070,7 +1071,8 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	if (start_command(&cp))
 		die("Could not run 'git status --porcelain' in submodule %s", path);
 
-	while (strbuf_getwholeline_fd(&buf, cp.out, '\n') != EOF) {
+	fp = xfdopen(cp.out, "r");
+	while (strbuf_getwholeline(&buf, fp, '\n') != EOF) {
 		if ((buf.buf[0] == '?') && (buf.buf[1] == '?')) {
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
 			if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
@@ -1082,7 +1084,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 				break;
 		}
 	}
-	close(cp.out);
+	fclose(fp);
 
 	if (finish_command(&cp))
 		die("'git status --porcelain' failed in submodule %s", path);