Re: Consequences of CRLF in index?

From: Stefan Beller <sbeller@google.com>
Date: Oct 25, 2017, 16:44 UTC
Message-ID: <CAGZ79kbgjgOhQuK0-zF7oTvuM_gWKF25+uAf-hsHK2Xg4yshhA@mail.gmail.com>
In-Reply-To: <xmqqshe7j0af.fsf@gitster.mtv.corp.google.com>

On Tue, Oct 24, 2017 at 9:53 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

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

Show 17 quoted lines

>  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?

Show 6 quoted lines

>  #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@google.com/ as that started this discussion already.

Show 17 quoted lines

>  #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.

Show 36 quoted lines

>          * 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])
>
>