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

[Jonathan Nieder]

Stefan Beller wrote:

> Instead of implementing line reading yet again, make use of our beautiful
> library functions.
>
> Signed-off-by: Stefan Beller <sbel...@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);