Re: [PATCH] blame.c: prepare_lines should not call xrealloc for every line

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

Admitted. So we have one _proper_ loop termination condition in the middle of the loop, and we have a snake oil condition at the start of the loop that can, according to the standard, _only_ yield true reliably when p == end (any end > p can only result from undefined behavior when p points to an object of size end - p).

The effect of this condition basically is to state "we don't trust memchr to do the right thing when called with 0 as its last argument which can happen at most once". This condition will come about by the _combined_ effect of p = ... _in_ the loop (which is the real iteration advancing p) and p++ hidden in the for statement (which never makes any sense separated from the p = ...).

It turns out that
a) memchr is provided by a compatibility layer in case it is missing or
   defective
b) other code in Git demands that memchr works correctly with a zero
   last argument.  See, for example, attr.c:
                        if (dirlen <= len)
                                break;
                        cp = memchr(path + len + 1, '/', dirlen - len - 1);

This clearly needs to be able to work with dirlen == len + 1.

So the loop gets rewritten in a manner where the for statement _pretends_ to loop linearly through buf, but where the loop _body_ has its own _regular_ termination condition it shields from the original for, and p is advanced _independently_. The advancement of p to beyond the next '\n' is distributed to two different places in the loop, and one place is made to look as if it does something else.

It's snake oil making debugging harder. It masks the real action, and it will route around suspected faulty behavior that is wrong according to the standards and not permitted elsewhere in the codebase. Other than that, it just takes additional performance from every iteration.

Putting belts and suspenders on a bike looks comforting until the suspenders get caught in the wheelspokes.

But it's only half the control.

As it is clear that we won't get to a state where I'd be willing to have "git blame" pointing to me as the author, I suggest that you either put your belts and suspenders on with a separate commit under your own authorship, or that we drop this altogether.

As I stated already: this patch was just provided because the original code offends my sense of aesthetics, so there is no point to it if it offends yours.

I'd still recommend fixing the sizeof(int *) with sizeof(int) confusion.

That would be totally ridiculous since end > p cannot ever happen except by undefined behavior. Pointer inequalities require both pointers to be associated with the same object.

At the expense of seriously impacting my motivation to do any further code cleanup on Git.

Which is a reasonable tradeoff since your fun is more important to Git's well-being than my one.