On Fri, Mar 22, 2013 at 11:08:59AM -0700, Junio C Hamano wrote:

> This may be sufficient.  In the olden days, we relied on that all
> whitespace fixing rules made the result shorter and took advantage
> of it in update-pre-post-images to rewrite the images in place.  The
> oddball tab-in-indent (aka Python), however, can grow the result by
> expanding tabs (deemed "incorrect") in the input into runs of spaces
> (deemed "kosher").
> Fortunately, we already support its more generalized form "match
> while ignoring whitespace differences" that can lengthen the result;
> as long as we correctly count the number of bytes needed to hold
> rewritten postimage, the existing logic in update_pre_post_images
> should be able to do the rest for us.

Yeah, your patch looks right to me. I do wonder if the optimization

> @@ -2399,8 +2401,10 @@ static int match_fragment(struct image *img,
>        * hunk match.  Update the context lines in the postimage.
>        */
>       fixed_buf = strbuf_detach(&fixed, &fixed_len);
> +     if (postlen < postimage->len)
> +             postlen = 0;
>       update_pre_post_images(preimage, postimage,
> -                            fixed_buf, fixed_len, 0);
> +                            fixed_buf, fixed_len, postlen);

should simply go into update_pre_post_images (i.e., let it decide on
whether to do it inline or with a new allocation, rather than making
postlen==0 special). That would let the ignore-whitespace code path use
the optimization, too (when it's possible).

By the way, I notice that when update_pre_post_images does allocate, the
old value of postimage->buf is lost. It looks like that is not leaked,
because it was pointing to a strbuf (newlines in apply_one_fragment)
that we are going to release anyway afterwards. But that means nobody is
freeing postimage->buf, which means that our newly malloc'd version is
getting leaked.

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to