{"thread":{"id":"49273","subject":"[PATCH] diff: fix --color-moved-ws=allow-indentation-change","startedAt":"2018-09-04T13:53:14Z","lastAt":"2018-09-11T19:25:03Z","messageCount":8,"participants":["Phillip Wood","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"357293","messageId":"20180904135258.31300-1-phillip.wood@talktalk.net","threadId":"49273","inReplyTo":null,"subject":"[PATCH] diff: fix --color-moved-ws=allow-indentation-change","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-09-04T13:52:58Z","receivedAt":"2018-09-04T13:53:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nIf there is more than one potential moved block and the longest block\nis not the first element of the array of potential blocks then the\nblock is cut short. With --color-moved=blocks this can leave moved\nlines unpainted if the shortened block does not meet the block length\nrequirement. With --color-moved=zebra then in addition to the\nunpainted lines the moved color can change in the middle of a single\nblock.\n\nFix this by freeing the whitespace delta of the match we're discarding\nrather than the one we're keeping.\n\nSigned-off-by: Phillip Wood \n---\n\nWhile I was working on this I spotted a couple of other issues I don't\nhave time to fix myself at the moment, so I thought I mention them in\ncase someone else wants to pick them up\n\n1) I think there is a potential memory leak at the end of\n mark_color_as_moved(). If pmb_nr > 0 then the whitespace deltas\n need freeing before freeing pmb itself.\n\n2) The documentation could be improved to explain that\n allow-indentation-change does not work with indentation that\n contains a mix of tabs and spaces and the motivation for that\n (python?) [I've got some code to add an option that supports that\n which I'll post when I've written some tests after 2.19 is\n released]\n\n diff.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 145cfbae5..4e8f725bb 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -968,8 +968,13 @@ static void pmb_advance_or_null_multi_match(struct diff_options *o,\n \t\t\t/* Carry the white space delta forward */\n \t\t\tpmb[i]->next_line->wsd = pmb[i]->wsd;\n \t\t\tpmb[i] = pmb[i]->next_line;\n-\t\t} else\n+\t\t} else {\n+\t\t\tif (pmb[i]->wsd) {\n+\t\t\t\tfree(pmb[i]->wsd->string);\n+\t\t\t\tFREE_AND_NULL(pmb[i]->wsd);\n+\t\t\t}\n \t\t\tpmb[i] = NULL;\n+\t\t}\n \t}\n }\n \n@@ -990,10 +995,6 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n \n \t\tif (lp < pmb_nr && rp > -1 && lp < rp) {\n \t\t\tpmb[lp] = pmb[rp];\n-\t\t\tif (pmb[rp]->wsd) {\n-\t\t\t\tfree(pmb[rp]->wsd->string);\n-\t\t\t\tFREE_AND_NULL(pmb[rp]->wsd);\n-\t\t\t}\n \t\t\tpmb[rp] = NULL;\n \t\t\trp--;\n \t\t\tlp++;\n-- \n2.18.0\n\n"},{"id":"357322","messageId":"CAGZ79kaBBzG6-QKruCeybN_do735h9tAXHZ7Rjx_YXeh85ax6A@mail.gmail.com","threadId":"49273","inReplyTo":"20180904135258.31300-1-phillip.wood@talktalk.net","subject":"Re: [PATCH] diff: fix --color-moved-ws=allow-indentation-change","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-04T18:08:25Z","receivedAt":"2018-09-04T18:08:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 4, 2018 at 6:53 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> If there is more than one potential moved block and the longest block\n> is not the first element of the array of potential blocks then the\n> block is cut short. With --color-moved=blocks this can leave moved\n> lines unpainted if the shortened block does not meet the block length\n> requirement. With --color-moved=zebra then in addition to the\n> unpainted lines the moved color can change in the middle of a single\n> block.\n>\n> Fix this by freeing the whitespace delta of the match we're discarding\n> rather than the one we're keeping.\n>\n> Signed-off-by: Phillip Wood \n> ---\n>\n> While I was working on this I spotted a couple of other issues I don't\n> have time to fix myself at the moment, so I thought I mention them in\n> case someone else wants to pick them up\n>\n> 1) I think there is a potential memory leak at the end of\n> mark_color_as_moved(). If pmb_nr > 0 then the whitespace deltas\n> need freeing before freeing pmb itself.\n>\n> 2) The documentation could be improved to explain that\n> allow-indentation-change does not work with indentation that\n> contains a mix of tabs and spaces and the motivation for that\n> (python?) [I've got some code to add an option that supports that\n> which I'll post when I've written some tests after 2.19 is\n> released]\n>\n> diff.c | 11 ++++++-----\n> 1 file changed, 6 insertions(+), 5 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 145cfbae5..4e8f725bb 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -968,8 +968,13 @@ static void pmb_advance_or_null_multi_match(struct diff_options *o,\n> /* Carry the white space delta forward */\n> pmb[i]->next_line->wsd = pmb[i]->wsd;\n> pmb[i] = pmb[i]->next_line;\n> - } else\n> + } else {\n> + if (pmb[i]->wsd) {\n> + free(pmb[i]->wsd->string);\n> + FREE_AND_NULL(pmb[i]->wsd);\n> + }\n> pmb[i] = NULL;\n> + }\n\nI agree on this hunk, as it will fix the mem leak in the case of\nallow-indentation-change, wondering if we need the same in\npmb_advance_or_null as well (and anywhere where there is a\n'pmb[i] = NULL' assignment outside the swapping below.).\n\n\n> }\n> }\n>\n> @@ -990,10 +995,6 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n>\n> if (lp < pmb_nr && rp > -1 && lp < rp) {\n> pmb[lp] = pmb[rp];\n> - if (pmb[rp]->wsd) {\n> - free(pmb[rp]->wsd->string);\n> - FREE_AND_NULL(pmb[rp]->wsd);\n> - }\n\nEh, this makes sense, though I had to think about it for a\nwhile as I was confused. By the first line in the condition we\nalso keep around the ->wsd pointer as is.\n\nThanks!\nStefan\n"},{"id":"357324","messageId":"b78b467c-6cae-2e2e-533c-48a4552539f5@talktalk.net","threadId":"49273","inReplyTo":"CAGZ79kaBBzG6-QKruCeybN_do735h9tAXHZ7Rjx_YXeh85ax6A@mail.gmail.com","subject":"Re: [PATCH] diff: fix --color-moved-ws=allow-indentation-change","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-09-04T18:51:01Z","receivedAt":"2018-09-04T18:51:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stefan\n\nOn 04/09/2018 19:08, Stefan Beller wrote:\n> On Tue, Sep 4, 2018 at 6:53 AM Phillip Wood wrote:\n>>\n>> From: Phillip Wood \n>>\n>> If there is more than one potential moved block and the longest block\n>> is not the first element of the array of potential blocks then the\n>> block is cut short. With --color-moved=blocks this can leave moved\n>> lines unpainted if the shortened block does not meet the block length\n>> requirement. With --color-moved=zebra then in addition to the\n>> unpainted lines the moved color can change in the middle of a single\n>> block.\n>>\n>> Fix this by freeing the whitespace delta of the match we're discarding\n>> rather than the one we're keeping.\n>>\n>> Signed-off-by: Phillip Wood \n>> ---\n>>\n>> While I was working on this I spotted a couple of other issues I don't\n>> have time to fix myself at the moment, so I thought I mention them in\n>> case someone else wants to pick them up\n>>\n>> 1) I think there is a potential memory leak at the end of\n>> mark_color_as_moved(). If pmb_nr > 0 then the whitespace deltas\n>> need freeing before freeing pmb itself.\n>>\n>> 2) The documentation could be improved to explain that\n>> allow-indentation-change does not work with indentation that\n>> contains a mix of tabs and spaces and the motivation for that\n>> (python?) [I've got some code to add an option that supports that\n>> which I'll post when I've written some tests after 2.19 is\n>> released]\n>>\n>> diff.c | 11 ++++++-----\n>> 1 file changed, 6 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/diff.c b/diff.c\n>> index 145cfbae5..4e8f725bb 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -968,8 +968,13 @@ static void pmb_advance_or_null_multi_match(struct diff_options *o,\n>> /* Carry the white space delta forward */\n>> pmb[i]->next_line->wsd = pmb[i]->wsd;\n>> pmb[i] = pmb[i]->next_line;\n>> - } else\n>> + } else {\n>> + if (pmb[i]->wsd) {\n>> + free(pmb[i]->wsd->string);\n>> + FREE_AND_NULL(pmb[i]->wsd);\n>> + }\n>> pmb[i] = NULL;\n>> + }\n> \n> I agree on this hunk, as it will fix the mem leak in the case of\n> allow-indentation-change, wondering if we need the same in\n> pmb_advance_or_null as well (and anywhere where there is a\n> 'pmb[i] = NULL' assignment outside the swapping below.).\n\nI don't think we don't call pmb_advance_or_null() if we're using \npmb[i]->wsd. I'm not sure if there are other sites that set 'pmb[i] = \nNULL' when pmb[i]->wsd has been allocated.\n\n> \n> \n>> }\n>> }\n>>\n>> @@ -990,10 +995,6 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n>>\n>> if (lp < pmb_nr && rp > -1 && lp < rp) {\n>> pmb[lp] = pmb[rp];\n>> - if (pmb[rp]->wsd) {\n>> - free(pmb[rp]->wsd->string);\n>> - FREE_AND_NULL(pmb[rp]->wsd);\n>> - }\n> \n> Eh, this makes sense, though I had to think about it for a\n> while as I was confused. By the first line in the condition we\n> also keep around the ->wsd pointer as is.\n\nYes, it took me ages to work out that this is what was breaking the \nhighlighting.\n\nBest Wishes\n\nPhillip\n\n> \n> Thanks!\n> Stefan\n> \n"},{"id":"357857","messageId":"5cff63d6-d9ec-d28f-d34a-e610ac19dbcb@talktalk.net","threadId":"49273","inReplyTo":"b78b467c-6cae-2e2e-533c-48a4552539f5@talktalk.net","subject":"Re: [PATCH] diff: fix --color-moved-ws=allow-indentation-change","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-09-11T10:05:44Z","receivedAt":"2018-09-11T10:05:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 04/09/2018 19:51, Phillip Wood wrote:\n> Hi Stefan\n> \n> On 04/09/2018 19:08, Stefan Beller wrote:\n>> On Tue, Sep 4, 2018 at 6:53 AM Phillip Wood\n>> wrote:\n>>>\n>>> From: Phillip Wood \n>>>\n>>> If there is more than one potential moved block and the longest block\n>>> is not the first element of the array of potential blocks then the\n>>> block is cut short. With --color-moved=blocks this can leave moved\n>>> lines unpainted if the shortened block does not meet the block length\n>>> requirement. With --color-moved=zebra then in addition to the\n>>> unpainted lines the moved color can change in the middle of a single\n>>> block.\n>>>\n>>> Fix this by freeing the whitespace delta of the match we're discarding\n>>> rather than the one we're keeping.\n>>>\n>>> Signed-off-by: Phillip Wood \n>>> ---\n>>>\n>>> While I was working on this I spotted a couple of other issues I don't\n>>> have time to fix myself at the moment, so I thought I mention them in\n>>> case someone else wants to pick them up\n>>>\n>>> 1) I think there is a potential memory leak at the end of\n>>>     mark_color_as_moved(). If pmb_nr > 0 then the whitespace deltas\n>>>     need freeing before freeing pmb itself.\n>>>\n>>> 2) The documentation could be improved to explain that\n>>>     allow-indentation-change does not work with indentation that\n>>>     contains a mix of tabs and spaces and the motivation for that\n>>>     (python?) [I've got some code to add an option that supports that\n>>>     which I'll post when I've written some tests after 2.19 is\n>>>     released]\n>>>\n>>>   diff.c | 11 ++++++-----\n>>>   1 file changed, 6 insertions(+), 5 deletions(-)\n>>>\n>>> diff --git a/diff.c b/diff.c\n>>> index 145cfbae5..4e8f725bb 100644\n>>> --- a/diff.c\n>>> +++ b/diff.c\n>>> @@ -968,8 +968,13 @@ static void\n>>> pmb_advance_or_null_multi_match(struct diff_options *o,\n>>>                          /* Carry the white space delta forward */\n>>>                          pmb[i]->next_line->wsd = pmb[i]->wsd;\n>>>                          pmb[i] = pmb[i]->next_line;\n>>> -               } else\n>>> +               } else {\n>>> +                       if (pmb[i]->wsd) {\n>>> +                               free(pmb[i]->wsd->string);\n>>> +                               FREE_AND_NULL(pmb[i]->wsd);\n>>> +                       }\n>>>                          pmb[i] = NULL;\n>>> +               }\n>>\n>> I agree on this hunk, as it will fix the mem leak in the case of\n>> allow-indentation-change, wondering if we need the same in\n>> pmb_advance_or_null as well (and anywhere where there is a\n>> 'pmb[i] = NULL' assignment outside the swapping below.).\n> \n> I don't think we don't call pmb_advance_or_null() if we're using\n> pmb[i]->wsd. I'm not sure if there are other sites that set 'pmb[i] =\n> NULL' when pmb[i]->wsd has been allocated.\n\nOops there's an extra don't there. Anyway I've had a proper look through\nthe code and pmb_advance_or_null() is the only other place where pmb[i]\nis set to NULL and that code path isn't used when pmb[i]->wsd has been\nallocated. So this should be sufficient.\n\nBest Wishes\n\nPhillip\n\n>>\n>>\n>>>          }\n>>>   }\n>>>\n>>> @@ -990,10 +995,6 @@ static int shrink_potential_moved_blocks(struct\n>>> moved_entry **pmb,\n>>>\n>>>                  if (lp < pmb_nr && rp > -1 && lp < rp) {\n>>>                          pmb[lp] = pmb[rp];\n>>> -                       if (pmb[rp]->wsd) {\n>>> -                               free(pmb[rp]->wsd->string);\n>>> -                               FREE_AND_NULL(pmb[rp]->wsd);\n>>> -                       }\n>>\n>> Eh, this makes sense, though I had to think about it for a\n>> while as I was confused. By the first line in the condition we\n>> also keep around the ->wsd pointer as is.\n> \n> Yes, it took me ages to work out that this is what was breaking the\n> highlighting.\n> \n> Best Wishes\n> \n> Phillip\n> \n>>\n>> Thanks!\n>> Stefan\n>>\n\n"},{"id":"357884","messageId":"CAGZ79kb53jNAJagQ+nG0hoJiozKkR6Aw=oRLZj5xvjFY5Hx=Sg@mail.gmail.com","threadId":"49273","inReplyTo":"5cff63d6-d9ec-d28f-d34a-e610ac19dbcb@talktalk.net","subject":"Re: [PATCH] diff: fix --color-moved-ws=allow-indentation-change","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-11T17:48:05Z","receivedAt":"2018-09-11T17:48:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> [...] So this should be sufficient.\n\nYup.\nThanks for keeping track of this patch, as I lost track of it.\n\nthanks,\nStefan\n"},{"id":"357886","messageId":"xmqqftyfq4qw.fsf@gitster-ct.c.googlers.com","threadId":"49273","inReplyTo":"CAGZ79kb53jNAJagQ+nG0hoJiozKkR6Aw=oRLZj5xvjFY5Hx=Sg@mail.gmail.com","subject":"Re: [PATCH] diff: fix --color-moved-ws=allow-indentation-change","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-11T18:16:23Z","receivedAt":"2018-09-11T18:16:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> [...] So this should be sufficient.\n>\n> Yup.\n> Thanks for keeping track of this patch, as I lost track of it.\n>\n> thanks,\n> Stefan\n\nSo does the above exchange mean that\n<20180904135258.31300-1-phillip.wood@talktalk.net> is ready to go\nwith your Acked-by?\n"},{"id":"357890","messageId":"CAGZ79kacqst9PVCzit_XpKYTHDC7VaKG_7OGoZyd3CtUxw74Cg@mail.gmail.com","threadId":"49273","inReplyTo":"xmqqftyfq4qw.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] diff: fix --color-moved-ws=allow-indentation-change","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-11T18:40:12Z","receivedAt":"2018-09-11T18:40:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 11, 2018 at 11:16 AM Junio C Hamano wrote:\n>\n> Stefan Beller writes:\n>\n> >> [...] So this should be sufficient.\n> >\n> > Yup.\n> > Thanks for keeping track of this patch, as I lost track of it.\n> >\n> > thanks,\n> > Stefan\n>\n> So does the above exchange mean that\n> <20180904135258.31300-1-phillip.wood@talktalk.net> is ready to go\n> with your Acked-by?\n\nAcked-by: Stefan Beller writes:\n\n> On Tue, Sep 11, 2018 at 11:16 AM Junio C Hamano wrote:\n>>\n>> Stefan Beller writes:\n>>\n>> >> [...] So this should be sufficient.\n>> >\n>> > Yup.\n>> > Thanks for keeping track of this patch, as I lost track of it.\n>> >\n>> > thanks,\n>> > Stefan\n>>\n>> So does the above exchange mean that\n>> <20180904135258.31300-1-phillip.wood@talktalk.net> is ready to go\n>> with your Acked-by?\n>\n> Acked-by: Stefan Beller I thought this was implicit, would you be interested in a more formal\n> communication for this, c.f.\n> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n513\n> sections 12 and 13.\n\nIt wasn't like I was explicitly asking for an ack because that\nsection tells us to do so in this case. Rather, I didn't follow the\nstate of this patch, for which the original author of the recent\nfeature ended up saying \"I lost track of it\".\n\nIf a reviewer and/or an acker anticipates that I, who by definition\nneeds to know the final state of more topics than any single\nindividual contributor does, may not remember the state of each\nindividual topic, and tries to be a bit more explicit, our\ncommunication may sometimes end up being extra redundant, but some\nother times, doing so would reduce the need for one extra roundtrip.\n\nIf I need a clarification, I'll ask anyway, just like I did in this\ncase, so I do not think it is all that necessary to be rigidly\n\"formal\", especially among list regulars. Just a simple common\nsense courtesy to try reducing each other's workload and memory\nburden is probably sufficient.\n\nThanks.\n"}]}