Re: [PATCH] sub-process: use gentle handshake to avoid die() on startup failure

[Junio C Hamano <gitster@pobox.com>]

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/sub-process.c b/sub-process.c
> index 83bf0a0e82..22c68bd10d 100644
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -132,18 +132,19 @@ static int handshake_version(struct child_process *process,
>  	if (packet_flush_gently(process->in))
>  		return error("Could not write flush packet");
>  
> -	if (!(line = packet_read_line(process->out, NULL)) ||
> +	if (packet_read_line_gently(process->out, NULL, &line) <= 0 ||
>  	    !skip_prefix(line, welcome_prefix, &p) ||
>  	    strcmp(p, "-server"))
>  		return error("Unexpected line '%s', expected %s-server",
>  			     line ? line : "<flush packet>", welcome_prefix);

If `packet_read_line_gently()` returns `< 0` (due to an EOF or read
error), `line` will be `NULL`.  The error message printed will be:

    `Unexpected line '<flush packet>', expected filter-server`

This is misleading when the remote process didn't send a flush
packet; it hung up or crashed.



> -	if (!(line = packet_read_line(process->out, NULL)) ||
> +	if (packet_read_line_gently(process->out, NULL, &line) <= 0 ||
>  	    !skip_prefix(line, "version=", &p) ||
>  	    strtol_i(p, 10, chosen_version))
>  		return error("Unexpected line '%s', expected version",
>  			     line ? line : "<flush packet>");

Ditto.

> -	if ((line = packet_read_line(process->out, NULL)))
> -		return error("Unexpected line '%s', expected flush", line);
> +	if (packet_read_line_gently(process->out, NULL, &line) < 0 || line)
> +		return error("Unexpected line '%s', expected flush",
> +			     line ? line : "<read error>");

We catch error return (< 0) or a line with payload (!!line) and
report an error here, because we want to see <flush> here.  OK.


> @@ -171,7 +172,7 @@ static int handshake_capabilities(struct child_process *process,
>  	if (packet_flush_gently(process->in))
>  		return error("Could not write flush packet");
>  
> -	while ((line = packet_read_line(process->out, NULL))) {
> +	while (packet_read_line_gently(process->out, NULL, &line) > 0) {
>  		const char *p;
>  		if (!skip_prefix(line, "capability=", &p))
>  			continue;

While this correctly stops the loop if packet_read_line_gently()
returns a non-positive value, doesn't it introduce a subtle bug?

`packet_read_line_gently()` returns:

  - `> 0` for a normal line (which keeps the loop running).

  - `0` for a flush packet (which we expect as the normal terminator
    of the capabilities list, stopping the loop).

  - `< 0` for an EOF or read error (which also stops the loop).

In the original code, an EOF or read error would have caused
`packet_read_line()` to call `die()`, aborting the process.

With the new code, if the child process dies or closes its pipe
during the capabilities handshake, the loop will terminate, and the
function will return `0` (success). The parent process will proceed
as if the capabilities were successfully negotiated.  Any further
communication with the child process would fail so the damage may
not be huge, but somebody must check if the loop terminated because
of a flush packet, or an error.

	while (1) {
	        const char *p;
        	int len = packet_read_line_gently(process->out, NULL, &line);

		if (len < 0)
			return error(_("subprocess `%s` failed to give capabilities"),
				process->args.v[0]);
		if (!skip_prefix(line, "capability=", &p))
			continue;
		...

or something, perhaps?