From: Phillip Wood Date: Tue, 11 Sep 2018 10:05:44 GMT Subject: Re: [PATCH] diff: fix --color-moved-ws=allow-indentation-change Message-ID: <5cff63d6-d9ec-d28f-d34a-e610ac19dbcb@talktalk.net> In-Reply-To: On 04/09/2018 19:51, Phillip Wood wrote: > Hi Stefan > > On 04/09/2018 19:08, Stefan Beller wrote: >> On Tue, Sep 4, 2018 at 6:53 AM Phillip Wood >> wrote: >>> >>> From: Phillip Wood >>> >>> If there is more than one potential moved block and the longest block >>> is not the first element of the array of potential blocks then the >>> block is cut short. With --color-moved=blocks this can leave moved >>> lines unpainted if the shortened block does not meet the block length >>> requirement. With --color-moved=zebra then in addition to the >>> unpainted lines the moved color can change in the middle of a single >>> block. >>> >>> Fix this by freeing the whitespace delta of the match we're discarding >>> rather than the one we're keeping. >>> >>> Signed-off-by: Phillip Wood >>> --- >>> >>> While I was working on this I spotted a couple of other issues I don't >>> have time to fix myself at the moment, so I thought I mention them in >>> case someone else wants to pick them up >>> >>> 1) I think there is a potential memory leak at the end of >>>     mark_color_as_moved(). If pmb_nr > 0 then the whitespace deltas >>>     need freeing before freeing pmb itself. >>> >>> 2) The documentation could be improved to explain that >>>     allow-indentation-change does not work with indentation that >>>     contains a mix of tabs and spaces and the motivation for that >>>     (python?) [I've got some code to add an option that supports that >>>     which I'll post when I've written some tests after 2.19 is >>>     released] >>> >>>   diff.c | 11 ++++++----- >>>   1 file changed, 6 insertions(+), 5 deletions(-) >>> >>> diff --git a/diff.c b/diff.c >>> index 145cfbae5..4e8f725bb 100644 >>> --- a/diff.c >>> +++ b/diff.c >>> @@ -968,8 +968,13 @@ static void >>> pmb_advance_or_null_multi_match(struct diff_options *o, >>>                          /* Carry the white space delta forward */ >>>                          pmb[i]->next_line->wsd = pmb[i]->wsd; >>>                          pmb[i] = pmb[i]->next_line; >>> -               } else >>> +               } else { >>> +                       if (pmb[i]->wsd) { >>> +                               free(pmb[i]->wsd->string); >>> +                               FREE_AND_NULL(pmb[i]->wsd); >>> +                       } >>>                          pmb[i] = NULL; >>> +               } >> >> I agree on this hunk, as it will fix the mem leak in the case of >> allow-indentation-change, wondering if we need the same in >> pmb_advance_or_null as well (and anywhere where there is a >> 'pmb[i] = NULL' assignment outside the swapping below.). > > I don't think we don't call pmb_advance_or_null() if we're using > pmb[i]->wsd. I'm not sure if there are other sites that set 'pmb[i] = > NULL' when pmb[i]->wsd has been allocated. Oops there's an extra don't there. Anyway I've had a proper look through the code and pmb_advance_or_null() is the only other place where pmb[i] is set to NULL and that code path isn't used when pmb[i]->wsd has been allocated. So this should be sufficient. Best Wishes Phillip >> >> >>>          } >>>   } >>> >>> @@ -990,10 +995,6 @@ static int shrink_potential_moved_blocks(struct >>> moved_entry **pmb, >>> >>>                  if (lp < pmb_nr && rp > -1 && lp < rp) { >>>                          pmb[lp] = pmb[rp]; >>> -                       if (pmb[rp]->wsd) { >>> -                               free(pmb[rp]->wsd->string); >>> -                               FREE_AND_NULL(pmb[rp]->wsd); >>> -                       } >> >> Eh, this makes sense, though I had to think about it for a >> while as I was confused. By the first line in the condition we >> also keep around the ->wsd pointer as is. > > Yes, it took me ages to work out that this is what was breaking the > highlighting. > > Best Wishes > > Phillip > >> >> Thanks! >> Stefan >>