On Thu, Mar 23, 2017 at 3:50 PM, Jonathan Nieder <jrnie...@gmail.com> wrote: > 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).
Thanks for careful reading! > > 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)? The code below makes sense to me. > > Another consequence of switching to streaming is that we may close > before the child finishes. We could have had closing before the child finished before as well: * the first read happens with strbuf_read(&buf, cp.out, 1024); * it contains a line indicating a modified file * The condition breaks out of while: if (ignore_untracked || (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)) break; * after the while loop we have the close(); * but we only had one read, which is not the complete output of the child. > Do we have to worry about handling SIGPIPE > in the child? I haven't checked how this handles that --- a test > might be useful. This patch doesn't make it worse in that respect. Thanks, Stefan