Re: "git am" crash (builtin/apply.c:2108) + small repro

[Alexey Spiridonov <snarkmaster@gmail.com>]

Thanks for looking into this, guys!

I seem to run into this with some regularity, but my setting is
apply.whitespace=strip rather than 'fix'.

Is there an obvious workaround?

Here are my remaining settings, sanitized for file paths and URLs:

svn.rmdir=true
push.default=upstream
color.ui=auto
color.diff=auto
color.status=auto
color.branch=auto
color.interactive=auto
color.branch.current=white blue bold
color.branch.local=blue
color.branch.remote=green
color.diff.plain=white
color.diff.meta=yellow bold
color.diff.frag=magenta bold
color.diff.old=red
color.diff.new=green
color.diff.whitespace=red blink
color.status.added=yellow
color.status.changed=green
color.status.untracked=cyan
svn.followparent=true
log.date=relative
blame.date=short
diff.renames=copies
diff.copies=true
diff.mnemonicprefix=true
apply.whitespace=strip
merge.tool=emerge
status.relativepaths=true
web.browser=lynx
rebase.stat=true
core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true
rerere.enabled=true
branch.autosetuprebase=always

-a

On Fri, Oct 12, 2012 at 3:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I've never been a fan of update_pre_post_images().  I think I know
>> what is going on (--whitespace=fix).  Will post a fix sometime
>> later.
>
> It bisects down to 5166714 (apply: Allow blank context lines to
> match beyond EOF, 2010-03-06).
>
> I do not like this patch at all (I _think_ it should somehow check
> the corresponding postimage is sane when the "ah, we ran out of
> preimage because the caller fixed it to reduce trailing blank lines"
> case), but at least this should get the ball rolling.
>
> Björn, both of the changes involved are yours, so I am thinking that
> you may be a good person to give a sanity check on this.  No need to
> hurry, as this is not a recent regression and we are in a pre-release
> freeze.
>
> Thanks.
>
> -- >8 --
> Subject: apply.c:update_pre_post_images(): the preimage can be truncated
>
> 5166714 (apply: Allow blank context lines to match beyond EOF,
> 2010-03-06) and then later 0c3ef98 (apply: Allow blank *trailing*
> context lines to match beyond EOF, 2010-04-08) taught "git apply"
> to trim new blank lines at the end in the patch text when matching
> the contents being patched and the preimage recorded in the patch,
> under --whitespace=fix mode.
>
> When a preimage is modified to match the current contents in
> preparation for such a "fixed" patch application, the context lines
> in the postimage must be updated to match (otherwise, it would
> reintroduce whitespace breakages), and update_pre_post_images()
> function is responsible for doing this.  However, this function was
> not updated to take into account a case where the removal of
> trailing blank lines reduces the number of lines in the preimage,
> and triggered an assertion error.
>
> The logic to fix the postimage by copying the corrected context
> lines from the preimage was not prepared to handle this case,
> either, but it was protected by the assert() and only got exposed
> when the assertion is corrected.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/apply.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git c/builtin/apply.c w/builtin/apply.c
> index 156b3ce..6c11e8b 100644
> --- c/builtin/apply.c
> +++ w/builtin/apply.c
> @@ -2095,7 +2095,7 @@ static void update_pre_post_images(struct image *preimage,
>                                    char *buf,
>                                    size_t len, size_t postlen)
>  {
> -       int i, ctx;
> +       int i, ctx, reduced;
>         char *new, *old, *fixed;
>         struct image fixed_preimage;
>
> @@ -2105,8 +2105,10 @@ static void update_pre_post_images(struct image *preimage,
>          * free "oldlines".
>          */
>         prepare_image(&fixed_preimage, buf, len, 1);
> -       assert(fixed_preimage.nr == preimage->nr);
> -       for (i = 0; i < preimage->nr; i++)
> +       assert(postlen
> +              ? fixed_preimage.nr == preimage->nr
> +              : fixed_preimage.nr <= preimage->nr);
> +       for (i = 0; i < fixed_preimage.nr; i++)
>                 fixed_preimage.line[i].flag = preimage->line[i].flag;
>         free(preimage->line_allocated);
>         *preimage = fixed_preimage;
> @@ -2126,7 +2128,8 @@ static void update_pre_post_images(struct image *preimage,
>         else
>                 new = old;
>         fixed = preimage->buf;
> -       for (i = ctx = 0; i < postimage->nr; i++) {
> +
> +       for (i = reduced = ctx = 0; i < postimage->nr; i++) {
>                 size_t len = postimage->line[i].len;
>                 if (!(postimage->line[i].flag & LINE_COMMON)) {
>                         /* an added line -- no counterparts in preimage */
> @@ -2145,8 +2148,15 @@ static void update_pre_post_images(struct image *preimage,
>                         fixed += preimage->line[ctx].len;
>                         ctx++;
>                 }
> -               if (preimage->nr <= ctx)
> -                       die(_("oops"));
> +
> +               /*
> +                * preimage is expected to run out, if the caller
> +                * fixed addition of trailing blank lines.
> +                */
> +               if (preimage->nr <= ctx) {
> +                       reduced++;
> +                       continue;
> +               }
>
>                 /* and copy it in, while fixing the line length */
>                 len = preimage->line[ctx].len;
> @@ -2159,6 +2169,7 @@ static void update_pre_post_images(struct image *preimage,
>
>         /* Fix the length of the whole thing */
>         postimage->len = new - postimage->buf;
> +       postimage->nr -= reduced;
>  }
>
>  static int match_fragment(struct image *img,