Re: [PATCHv15 2/5] run_processes_parallel: add LF when caller is sloppy

[Jonathan Nieder <jrnieder@gmail.com>]

Stefan Beller wrote:
> On Wed, Feb 24, 2016 at 1:19 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> > Stefan Beller wrote:

>>> When the callers of parallel processing machine are sloppy with their
>>> messages, make sure the output is terminated with LF after one child
>>> process is handled.
>>
>> Why not always add \n here?
>
> So you propose to always add a \n if the output length was > 0 ?

Ah, now I see where I was confused.

I was seeing an analogy to functions like ref_transaction_begin(),
which use a 'struct strbuf *err' argument to store the argument to
die() that describes why they failed.  They get used like this:

	struct strbuf err = STRBUF_INIT;
	if (ref_transaction_begin(..., &err))
		die(err.buf);

and die() appends a \n at the end.  They typically are implemented
like this:

	if (open(...)) {
		strbuf_addf(&err,
			    "cannot open '%s': %s", ..., strerror(errno));
		return -1;
	}

When the function doesn't fail, err doesn't need to be inspected at all.

get_next_task_fn et al looked similar to that pattern, but they are
doing something different.  The strbuf passed in is the same buffer
that is used to collect the child process's output.  Writing to that
strbuf is not a way to provide an error message for die() --- instead,
it is a way to provide additional output that should be combined with
the child process's output.

Renaming the parameter to something like 'struct strbuf *out' would
have avoided this clash of conventions and got me un-confused.  That
would make it clearer that the callback function should do

	strbuf_addf(out, "warning: foo\n");

including both its own 'warning: ' prefix and its own newline.  It is
providing output meant to be passed as-is to the terminal.

That is a convenient API since if you want to write multiple messages,
you can do

	if (foo)
		strbuf_addf(out, "warning: foo\n");

	... do some other things ...

	if (bar)
		strbuf_addf(out, "warning: bar\n");

The newlines avoid the messages running together.

The functions default_start_failure and default_task_finished are
buggy under that API, since they do not include newlines in their
output.

Once they're fixed, there wouldn't be any need to add \n, unless we
are worried about a child process that writes output that doesn't end
with a newline.  It can be convenient for child processes to do things
like

	printf '%s\t' "some information"

so I am not convinced this patch is helping.

Does that make sense?

Sorry for the confusion,
Jonathan