Re: [PATCH] apply: keep buffer/size pair in sync when parsing binary hunks
- From
Jeff King <peff@peff.net>
- Date
- Aug 10, 2021, 18:53 UTC
- Message-ID
- <YRLLPPANZhhTNUTT@coredump.intra.peff.net>
- In-Reply-To
- <CAPUEsphWN_-BWyfF9mnPhL56RSnmPZfmvh_QwhjoAb3xin8V-w@mail.gmail.com>
On Mon, Aug 09, 2021 at 10:26:51PM -0700, Carlo Arenas wrote:
Show 24 quoted lines
> On Mon, Aug 9, 2021 at 9:24 PM Jeff King <peff@peff.net> 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