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

[Stefan Beller <sbeller@google.com>]

On Thu, Mar 23, 2017 at 3:50 PM, Jonathan Nieder <jrnieder@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 <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).

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