On Mon, Aug 09, 2021 at 10:26:51PM -0700, Carlo Arenas wrote:
> On Mon, Aug 9, 2021 at 9:24 PM Jeff King <peff@xxxxxxxx> wrote:
> > diff --git a/apply.c b/apply.c
> > index 44bc31d6eb..4ed4b27169 100644
> > --- a/apply.c
> > +++ b/apply.c
> > @@ -1917,6 +1917,7 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,
> >
> > state->linenr++;
> > buffer += llen;
> > + size -= llen;
> > while (1) {
>
> Ironically, I was looking at this code because of your previous
> patch[1] that you suggested was ugly
> and because I was going to suggest moving from a for to a while loop
> to avoid the overly long line.
>
> It is interesting to note though, that having a for (and obviously
> removing the last 2 lines from the loop) with a comma separated
> increment instead would
> have made this issue IMHO more obvious, and also why I decided against
> that; would it be a good idea to fix that as well?
>
> for (; ; buffer += llen, size -= llen) {
That wouldn't have fixed this issue, since it was actually before the
start of the loop that where the problem was. It might have made it more
obvious if the two parts were next to each other, but I'm not so sure.
There are lots of reasons why the stuff before a loop may not be
symmetric with the loop increment.
We could also initialize it like this:
for (buffer += llen, size -= llen; ; buffer += llen, size -= llen)
but IMHO that is harder to read. The increment at the start is not part
of the loop initialization. It is really a "finishing" of the earlier
parsing that occurred (in fact, if I were writing this from scratch, I'd
probably have put extra whitespace before the start of the loop).
I did consider moving that other case to a while-loop, but then you have
to manually catch all of the loop continuations (which is verbose and
error-prone). So I dunno. There is no free lunch. ;)
-Peff
