On Thu, Mar 23, 2017 at 3:50 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Stefan Beller wrote:
>
>> Instead of implementing line reading yet again, make use of our beautiful
>> library functions.
>>
>> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
>> ---
>> 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