Re: [PATCH v3 08/18] rerere: explain the rerere I/O abstraction

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

Junio C Hamano <gitster@pobox.com> writes:

> +/*
> + * Write a conflict marker to io->output (if defined).
> + */
>  static void rerere_io_putconflict(int ch, int size, struct rerere_io *io)
>  {
>  	char buf[64];

This is unrelated to the topic of this step, but this function seems
to be very poorly put together with duct tape.

> static void rerere_io_putconflict(int ch, int size, struct rerere_io *io)
> {
> 	char buf[64];
> 
> 	while (size) {
> 		if (size < sizeof(buf) - 2) {
> 			memset(buf, ch, size);
> 			buf[size] = '\n';
> 			buf[size + 1] = '\0';
> 			size = 0;

The if() guarding this block has an off-by-one error in the benign
direction.  When size is 62, the marker with terminating LF and NUL
should still fit within the buf[], so it should use "<=", not "<",
to compare.

> 		} else {
> 			int sz = sizeof(buf) - 1;
> 			if (size <= sz)
> 				sz -= (sz - size) + 1;
> 			memset(buf, ch, sz);
> 			buf[sz] = '\0';
> 			size -= sz;

This is an even more awkward construct.  sz is what we have to work
with the substring that we cannot emit with a single call (because
buf[] is too small), so naturally I would expect it to be more like

	int to_emit = size;
        if (sz <= size)
        	to_emit = sz;
	memset(buf, ch, to_emit);
        buf[to_emit] = '\0';
	size -= to_emit;

which makes the "if (size <= sz)" in the code look very suspicious.

But rewriting it to the "more natural" version would make it buggy.
At a right boundary condition, i.e. size == 63, we may find that the
conflict marker is too long with LF and NUL to fit in buf[] and come
here, and then fill the full conflict marker with NUL just fine in
buf[], decrementing size to 0, emit that 63-byte long marker.  The
next iteration will exit the loop without emitting the LF.

The unnatural is what is saving us from such a bug.

	if (size <= sz)
		sz -= (sz - size) + 1;

It ensures that subtraction of sz (i.e. "to_emit") from size before
the next iteration will never brings size down to zero by reducing
sz by one too much, forcing another iteration, which will then have
size smaller than "sizeof(buf) - 2" and have us emit the LF.

Not buggy, but ugly.

> 		}
> 		rerere_io_putstr(buf, io);
> 	}
> }