From: Phillip Wood Date: Wed, 21 Nov 2018 15:49:56 GMT Subject: Re: [PATCH v1 9/9] diff --color-moved-ws: handle blank lines Message-ID: In-Reply-To: On 20/11/2018 18:05, Stefan Beller wrote: > On Fri, Nov 16, 2018 at 3:04 AM Phillip Wood wrote: >> >> From: Phillip Wood >> >> When using --color-moved-ws=allow-indentation-change allow lines with >> the same indentation change to be grouped across blank lines. For now >> this only works if the blank lines have been moved as well, not for >> blocks that have just had their indentation changed. >> >> This completes the changes to the implementation of >> --color-moved=allow-indentation-change. Running >> >> git diff --color-moved=allow-indentation-change v2.18.0 v2.19.0 >> >> now takes 5.0s. This is a saving of 41% from 8.5s for the optimized >> version of the previous implementation and 66% from the original which >> took 14.6s. >> >> Signed-off-by: Phillip Wood >> --- >> >> Notes: >> Changes since rfc: >> - Split these changes into a separate commit. >> - Detect blank lines when processing the indentation rather than >> parsing each line twice. >> - Tweaked the test to make it harder as suggested by Stefan. >> - Added timing data to the commit message. >> >> diff.c | 34 ++++++++++++++++++++++++++++--- >> t/t4015-diff-whitespace.sh | 41 ++++++++++++++++++++++++++++++++++---- >> 2 files changed, 68 insertions(+), 7 deletions(-) >> >> diff --git a/diff.c b/diff.c >> index 89559293e7..072b5bced6 100644 >> --- a/diff.c >> +++ b/diff.c >> @@ -792,9 +792,11 @@ static void moved_block_clear(struct moved_block *b) >> memset(b, 0, sizeof(*b)); >> } >> >> +#define INDENT_BLANKLINE INT_MIN > > Answering my question from the previous patch: > This is why we need to keep the indents signed. > > This patch looks quite nice to read along. > > The whole series looks good to me. Thanks > Do we need to update the docs in any way? I'm not sure, at the moment it does not make any promises about the exact behavior of --color-moved-ws=allow-indentation-change, we could change it to be more explicit but I'm not sure it's worth it. Thanks for looking over these patches, I'll post a reroll soon based on your comments. Phillip > Thanks, > Stefan >