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

Stefan Beller wrote:

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