[PATCH] diff: fix --color-moved-ws=allow-indentation-change

[PATCH] diff: fix --color-moved-ws=allow-indentation-change

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---

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;
+		}
 	}
 }
 
@@ -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);
-			}
 			pmb[rp] = NULL;
 			rp--;
 			lp++;
-- 
2.18.0

Re: [PATCH] diff: fix --color-moved-ws=allow-indentation-change

[Stefan Beller]

On Tue, Sep 4, 2018 at 6:53 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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 <phillip.wood@dunelm.org.uk>
> ---
>
> 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.).


>         }
>  }
>
> @@ -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.

Thanks!
Stefan

Re: [PATCH] diff: fix --color-moved-ws=allow-indentation-change

[Phillip Wood]

Hi Stefan

On 04/09/2018 19:08, Stefan Beller wrote:
> On Tue, Sep 4, 2018 at 6:53 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
>>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> 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 <phillip.wood@dunelm.org.uk>
>> ---
>>
>> 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.

> 
> 
>>          }
>>   }
>>
>> @@ -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
> 

Re: [PATCH] diff: fix --color-moved-ws=allow-indentation-change

[Phillip Wood]

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
>> <phillip.wood@talktalk.net> wrote:
>>>
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> 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 <phillip.wood@dunelm.org.uk>
>>> ---
>>>
>>> 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
>>

Re: [PATCH] diff: fix --color-moved-ws=allow-indentation-change

[Stefan Beller]

>  [...] So this should be sufficient.

Yup.
Thanks for keeping track of this patch, as I lost track of it.

thanks,
Stefan

Re: [PATCH] diff: fix --color-moved-ws=allow-indentation-change

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>>  [...] So this should be sufficient.
>
> Yup.
> Thanks for keeping track of this patch, as I lost track of it.
>
> thanks,
> Stefan

So does the above exchange mean that
<20180904135258.31300-1-phillip.wood@talktalk.net> is ready to go
with your Acked-by?

Re: [PATCH] diff: fix --color-moved-ws=allow-indentation-change

[Stefan Beller]

On Tue, Sep 11, 2018 at 11:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Beller <sbeller@google.com> writes:
>
> >>  [...] So this should be sufficient.
> >
> > Yup.
> > Thanks for keeping track of this patch, as I lost track of it.
> >
> > thanks,
> > Stefan
>
> So does the above exchange mean that
> <20180904135258.31300-1-phillip.wood@talktalk.net> is ready to go
> with your Acked-by?

Acked-by: Stefan Beller <sbeller@google.com

I thought this was implicit, would you be interested in a more formal
communication for this, c.f.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n513
sections 12 and 13.

thanks.

Re: [PATCH] diff: fix --color-moved-ws=allow-indentation-change

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Sep 11, 2018 at 11:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Stefan Beller <sbeller@google.com> writes:
>>
>> >>  [...] So this should be sufficient.
>> >
>> > Yup.
>> > Thanks for keeping track of this patch, as I lost track of it.
>> >
>> > thanks,
>> > Stefan
>>
>> So does the above exchange mean that
>> <20180904135258.31300-1-phillip.wood@talktalk.net> is ready to go
>> with your Acked-by?
>
> Acked-by: Stefan Beller <sbeller@google.com

Thanks.

> I thought this was implicit, would you be interested in a more formal
> communication for this, c.f.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n513
> sections 12 and 13.

It wasn't like I was explicitly asking for an ack because that
section tells us to do so in this case.  Rather, I didn't follow the
state of this patch, for which the original author of the recent
feature ended up saying "I lost track of it".

If a reviewer and/or an acker anticipates that I, who by definition
needs to know the final state of more topics than any single
individual contributor does, may not remember the state of each
individual topic, and tries to be a bit more explicit, our
communication may sometimes end up being extra redundant, but some
other times, doing so would reduce the need for one extra roundtrip.

If I need a clarification, I'll ask anyway, just like I did in this
case, so I do not think it is all that necessary to be rigidly
"formal", especially among list regulars.  Just a simple common
sense courtesy to try reducing each other's workload and memory
burden is probably sufficient.

Thanks.