Re: [PATCH] Allow combined diff to ignore white-spaces
Antoine Pelisse <apelisse@gmail.com> writes:
Show 47 quoted lines
> Currently, it's not possible to use the space-ignoring options (-b, -w,
> --ignore-space-at-eol) with combined diff. It makes it pretty impossible
> to read a merge between a branch that changed all tabs to spaces, and a
> branch with functional changes.
>
> Pass diff flags to diff engine, so that combined diff behaves as normal
> diff does with spaces.
> Also coalesce lines that are removed from both (or more) parents.
>
> It also means that a conflict-less merge done using a ignore-* strategy
> option will not show any conflict if shown in combined-diff using the
> same option.
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
> OK, I added some tests and coalesce similar lost lines (using the same flags
> we used for diff.
>
> combine-diff.c | 57 ++++++++++++++++++++++++++++++-----
> t/t4038-diff-combined.sh | 75 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 125 insertions(+), 7 deletions(-)
>
> diff --git a/combine-diff.c b/combine-diff.c
> index 35d41cd..0f33983 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -5,6 +5,7 @@
> #include "diffcore.h"
> #include "quote.h"
> #include "xdiff-interface.h"
> +#include "xdiff/xmacros.h"
> #include "log-tree.h"
> #include "refs.h"
> #include "userdiff.h"
> @@ -122,7 +123,47 @@ static char *grab_blob(const unsigned char *sha1, unsigned int mode,
> return blob;
> }
>
> -static void append_lost(struct sline *sline, int n, const char *line, int len)
> +static int match_string_spaces(const char *line1, int len1,
> + const char *line2, int len2,
> + long flags)
> +{
> + if (flags & XDF_WHITESPACE_FLAGS) {
> + for (; len1 > 0 && XDL_ISSPACE(line1[len1 - 1]); len1--);
> + for (; len2 > 0 && XDL_ISSPACE(line2[len2 - 1]); len2--);
> + }This says "any XDF_WHITESPACE_FLAGS causes the trailing blanks ignored", which is correct: --ignore-space-at-eol, -b or -w are all we have. OK.
> + if (!(flags & (XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_CHANGE)))
> + return (len1 == len2 && !memcmp(line1, line2, len1));
If --ignore-space-at-eol is the only one given, or none of the whitespace flags is given, then we can just do memcmp(). OK.
The remainder of the function is only used when either -b or -w is (or both are) in effect.
> + while (len1 > 0 && len2 > 0) {
> + len1--;
> + len2--;Scanning from the tail end of the strings...
> + if (XDL_ISSPACE(line1[len1]) || XDL_ISSPACE(line2[len2])) {
> + if ((flags & XDF_IGNORE_WHITESPACE_CHANGE) &&
> + (!XDL_ISSPACE(line1[len1]) || !XDL_ISSPACE(line2[len2])))
> + return 0;If --ignore-space-change and one side is blank while the other not, then these cannot be the same. If we see blank on both sides, that is fine under --ignore-whitespace, too.
> + for (; len1 > 0 && XDL_ISSPACE(line1[len1]); len1--);
> + for (; len2 > 0 && XDL_ISSPACE(line2[len2]); len2--);
Then, strip the blanks, if any, so that the next comparison is done for the last non-blank char from both sides. OK.
> + }
> + if (line1[len1] != line2[len2])
> + return 0;
And we can say "not same" as soon as we find differences. Rinse and repeat.
> + }
> +
> + if (flags & XDF_IGNORE_WHITESPACE) {
> + // Consume remaining spacesNo C++/C99 comments in our codebase, please.
> + for (; len1 > 0 && XDL_ISSPACE(line1[len1 - 1]); len1--);
> + for (; len2 > 0 && XDL_ISSPACE(line2[len2 - 1]); len2--);
If we are ignoring all whitespaces, we can simply discard them. OK.
I wonder what happens when --ignore-space-change is in effect but not --ignore-all-space? The loop must have exited because one (or both) of len1 and len2 went down to zero. If both are zero, we obviously have a match, and otherwise, one has blanks at the beginning and the other does not. Wait, is that really true? Yes, because if len1 == 0 && len2 != 0, line2[len2] cannot be blank (otherwise len2 would be zero because we have a loop to eat a run of blanks).
OK, sounds good.
> + }
> +
> + // We matched full line1 and line2
Show 5 quoted lines
> + if (!len1 && !len2)
> + return 1;
> +
> + return 0;
> +}
Show 15 quoted lines
> +static void append_lost(struct sline *sline, int n, const char *line, int len, long flags)
> {
> struct lline *lline;
> unsigned long this_mask = (1UL<<n);
> @@ -133,8 +174,8 @@ static void append_lost(struct sline *sline, int n, const char *line, int len)
> if (sline->lost_head) {
> lline = sline->next_lost;
> while (lline) {
> - if (lline->len == len &&
> - !memcmp(lline->line, line, len)) {
> + if (match_string_spaces(lline->line, lline->len,
> + line, len, flags)) {
> lline->parent_map |= this_mask;
> sline->next_lost = lline->next;
> return;Nicely done.
Thanks, will queue with comment style fix-ups.
Show 86 quoted lines
> diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
> index 614425a..ba8a56b 100755
> --- a/t/t4038-diff-combined.sh
> +++ b/t/t4038-diff-combined.sh
> @@ -113,4 +113,79 @@ test_expect_success 'check --cc --raw with forty trees' '
> grep "^::::::::::::::::::::::::::::::::::::::::[^:]" out
> '
>
> +test_expect_success 'setup combined ignore spaces' '
> + git checkout master &&
> + >test &&
> + git add test &&
> + git commit -m initial &&
> +
> + echo "
> + always coalesce
> + eol space coalesce \n\
> + space change coalesce
> + all spa ces coalesce
> + eol spaces \n\
> + space change
> + all spa ces" >test &&
> + git commit -m "change three" -a &&
> +
> + git checkout -b side HEAD^ &&
> + echo "
> + always coalesce
> + eol space coalesce
> + space change coalesce
> + all spaces coalesce
> + eol spaces
> + space change
> + all spaces" >test &&
> + git commit -m indent -a &&
> +
> + test_must_fail git merge master &&
> + echo "
> + eol spaces \n\
> + space change
> + all spa ces" > test &&
> + git commit -m merged -a
> +'
> +
> +test_expect_success 'check combined output (no ignore space)' '
> + git show | test_i18ngrep "^-\s*eol spaces" &&
> + git show | test_i18ngrep "^-\s*eol space coalesce" &&
> + git show | test_i18ngrep "^-\s*space change" &&
> + git show | test_i18ngrep "^-\s*space change coalesce" &&
> + git show | test_i18ngrep "^-\s*all spaces" &&
> + git show | test_i18ngrep "^-\s*all spaces coalesce" &&
> + git show | test_i18ngrep "^--\s*always coalesce"
> +'
> +
> +test_expect_success 'check combined output (ignore space at eol)' '
> + git show --ignore-space-at-eol | test_i18ngrep "^\s*eol spaces" &&
> + git show --ignore-space-at-eol | test_i18ngrep "^--\s*eol space coalesce" &&
> + git show --ignore-space-at-eol | test_i18ngrep "^-\s*space change" &&
> + git show --ignore-space-at-eol | test_i18ngrep "^-\s*space change coalesce" &&
> + git show --ignore-space-at-eol | test_i18ngrep "^-\s*all spaces" &&
> + git show --ignore-space-at-eol | test_i18ngrep "^-\s*all spaces coalesce" &&
> + git show --ignore-space-at-eol | test_i18ngrep "^--\s*always coalesce"
> +'
> +
> +test_expect_success 'check combined output (ignore space change)' '
> + git show -b | test_i18ngrep "^\s*eol spaces" &&
> + git show -b | test_i18ngrep "^--\s*eol space coalesce" &&
> + git show -b | test_i18ngrep "^\s*space change" &&
> + git show -b | test_i18ngrep "^--\s*space change coalesce" &&
> + git show -b | test_i18ngrep "^-\s*all spaces" &&
> + git show -b | test_i18ngrep "^-\s*all spaces coalesce" &&
> + git show -b | test_i18ngrep "^--\s*always coalesce"
> +'
> +
> +test_expect_success 'check combined output (ignore all spaces)' '
> + git show -w | test_i18ngrep "^\s*eol spaces" &&
> + git show -w | test_i18ngrep "^--\s*eol space coalesce" &&
> + git show -w | test_i18ngrep "^\s*space change" &&
> + git show -w | test_i18ngrep "^--\s*space change coalesce" &&
> + git show -w | test_i18ngrep "^\s*all spa ces" &&
> + git show -w | test_i18ngrep "^--\s*all spaces coalesce" &&
> + git show -w | test_i18ngrep "^--\s*always coalesce"
> +'
> +
> test_done
> --
> 1.7.9.5
Antoine Pelisse
