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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 24, 2015, 20:42 UTC
Message-ID: <xmqqio99l22p.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1437171880-21590-9-git-send-email-gitster@pobox.com>

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

Show 6 quoted lines

> +/*
> + * 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.

Show 10 quoted lines

> 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.

Show 7 quoted lines

> 		} 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);
> 	}
> }