On Tue, Oct 24, 2017 at 9:53 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Here is a lunch-time hack to add that option. As you can see from > the lack of docs, tests and a proper log message, I haven't played > with it long enough to know how buggy it is, though. I am not all > that interested in polishing it to completion myself and prefer to > leave it as #leftoverbits ;-) Ok, nevertheless a review pointing out a couple things would be useful for those who pick it up later, I assume. > Also I didn't bother teaching this to Stefan's color-moved thing, as > the line comparator it uses will hopefully be unified with the one I > am touching in xdiff/ with this patch. which will be rerolled shortly fixing just the parameter names as Eric mentioned. > diff.c | 5 ++++- > merge-recursive.c | 2 ++ > xdiff/xdiff.h | 3 ++- > xdiff/xutils.c | 34 ++++++++++++++++++++++++++++++++-- > 4 files changed, 40 insertions(+), 4 deletions(-) > > diff --git a/diff.c b/diff.c > index 6fd288420b..eeca0fd3b8 100644 > --- a/diff.c > +++ b/diff.c > @@ -4202,7 +4202,8 @@ void diff_setup_done(struct diff_options *options) > > if (DIFF_XDL_TST(options, IGNORE_WHITESPACE) || > DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) || > - DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL)) > + DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL) || > + DIFF_XDL_TST(options, IGNORE_CR_AT_EOL)) This highlights another part of the flag macros, that could be made nicer. All these flags combined are XDF_WHITESPACE_FLAGS, so this if could be written without the macros as if (options->xdl_ops & XDF_WHITESPACE_FLAGS) which we might want to hide in a macro DIFF_XDL_MASK_TST(options, mask) or such? > #define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4) > -#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL) > +#define XDF_IGNORE_CR_AT_EOL (1 << 5) > +#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL | XDF_IGNORE_CR_AT_EOL) > > #define XDF_PATIENCE_DIFF (1 << 5) (1<<5) is taken twice now. Currently there is only one unused free bit (but that was used once upon a time); so we have to think how we revamp the flag system to support more than 32 bits. See also the answers to https://public-inbox.org/git/20171024000931.14814-1-sbeller@xxxxxxxxxx/ as that started this discussion already. > #define XDF_HISTOGRAM_DIFF (1 << 6) > diff --git a/xdiff/xutils.c b/xdiff/xutils.c > index 04d7b32e4e..8720e272b9 100644 > --- a/xdiff/xutils.c > +++ b/xdiff/xutils.c > @@ -156,6 +156,21 @@ int xdl_blankline(const char *line, long size, long flags) > return (i == size); > } > > +/* > + * Have we eaten everything on the line, except for an optional > + * CR at the very end? > + */ > +static int ends_with_optional_cr(const char *l, long s, long i) > +{ > + if (s && l[s-1] == '\n') > + s--; so first we cut off the '\n', > + if (s == i) > + return 1; then we either have an ending without > + if (s == i + 1 && l[i] == '\r') > + return 1; or with a '\r' before. That seems correct after some thought; I might offer another easier to understand (for me) solution, which is { /* cut of ending LF */ if (s && l[s-1] == '\n') s--; /* do we only need to cut LF? */ if (i == s) return 1; /* cut of ending CR */ if (s && l[s-1] == '\r') s--; /* was this everything to cut? */ return i == s } Though this seems even more complicated after having it written down. > * Each flavor of ignoring needs different logic to skip whitespaces > * while we have both sides to compare. > @@ -204,6 +220,14 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags) > i1++; > i2++; > } > + } else if (flags & XDF_IGNORE_CR_AT_EOL) { > + /* Find the first difference and see how the line ends */ > + while (i1 < s1 && i2 < s2 && l1[i1] == l2[i2]) { > + i1++; > + i2++; > + } > + return (ends_with_optional_cr(l1, s1, i1) && > + ends_with_optional_cr(l2, s2, i2)); > } > > /* > @@ -230,9 +254,15 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data, > char const *top, long flags) { > unsigned long ha = 5381; > char const *ptr = *data; > + int cr_at_eol_only = (flags & XDF_WHITESPACE_FLAGS) == XDF_IGNORE_CR_AT_EOL; > > for (; ptr < top && *ptr != '\n'; ptr++) { > - if (XDL_ISSPACE(*ptr)) { > + if (cr_at_eol_only) { > + if (*ptr == '\r' && > + (top <= ptr + 1 || ptr[1] == '\n')) > + continue; > + } > + else if (XDL_ISSPACE(*ptr)) { > const char *ptr2 = ptr; > int at_eol; > while (ptr + 1 < top && XDL_ISSPACE(ptr[1]) > >
