Consequences of CRLF in index?

Consequences of CRLF in index?

[Lars Schneider]

Hi,

I've migrated a large repo (110k+ files) with a lot of history (177k commits) 
and a lot of users (200+) to Git. Unfortunately, all text files in the index
of the repo have CRLF line endings. In general this seems not to be a problem 
as the project is developed exclusively on Windows.

However, I wonder if there are any "hidden consequences" of this setup?
If there are consequences, then I see two options. Either I rebase the repo 
and fix the line endings for all commits or I add a single commit that fixes 
the line endings for all files. Both actions require coordination with the 
users to avoid repo trouble/merge conflicts. The "single fixup commit" options 
would also make diffs into the past look bad. Would a single large commit have
any impact on the performance of standard Git operations?

Thanks,
Lars

Re: Consequences of CRLF in index?

[Jonathan Nieder]

Hi,

Lars Schneider wrote:

> I've migrated a large repo (110k+ files) with a lot of history (177k commits)
> and a lot of users (200+) to Git. Unfortunately, all text files in the index
> of the repo have CRLF line endings. In general this seems not to be a problem
> as the project is developed exclusively on Windows.

Sounds good.

> However, I wonder if there are any "hidden consequences" of this setup?
> If there are consequences, then I see two options. Either I rebase the repo
> and fix the line endings for all commits or I add a single commit that fixes
> the line endings for all files. Both actions require coordination with the
> users to avoid repo trouble/merge conflicts. The "single fixup commit" options
> would also make diffs into the past look bad. Would a single large commit have
> any impact on the performance of standard Git operations?

There are no hidden consequences that I'm aware of.  If you later
decide that you want to become a cross-platform project, then you may
want to switch to LF endings, in which case I suggest the "single
fixup commit" strategy.

In any event, you also probably want to declare what you're doing
using .gitattributes.  By checking in the files as CRLF, you are
declaring that you do *not* want Git to treat them as text files
(i.e., you do not want Git to change the line endings), so something
as simple as

	* -text

should do the trick.  See gitattributes(5) for details.

I'd be interested to hear what happens when diff-ing across a line
ending fixup commit.  Is this an area where Git needs some
improvement?  "git merge" knows an -Xrenormalize option to deal with a
related problem --- it's possible that "git diff" needs to learn a
similar trick.

Thanks and hope that helps,
Jonathan

Re: Consequences of CRLF in index?

[Torsten Bögershausen]

On Tue, Oct 24, 2017 at 11:14:15AM -0700, Jonathan Nieder wrote:
> Hi,
> 
> Lars Schneider wrote:
> 
> > I've migrated a large repo (110k+ files) with a lot of history (177k commits)
> > and a lot of users (200+) to Git. Unfortunately, all text files in the index
> > of the repo have CRLF line endings. In general this seems not to be a problem
> > as the project is developed exclusively on Windows.
> 
> Sounds good.
> 
> > However, I wonder if there are any "hidden consequences" of this setup?
> > If there are consequences, then I see two options. Either I rebase the repo
> > and fix the line endings for all commits or I add a single commit that fixes
> > the line endings for all files. Both actions require coordination with the
> > users to avoid repo trouble/merge conflicts. The "single fixup commit" options
> > would also make diffs into the past look bad. Would a single large commit have
> > any impact on the performance of standard Git operations?
> 
> There are no hidden consequences that I'm aware of.  If you later
> decide that you want to become a cross-platform project, then you may
> want to switch to LF endings, in which case I suggest the "single
> fixup commit" strategy.
> 
> In any event, you also probably want to declare what you're doing
> using .gitattributes.  By checking in the files as CRLF, you are
> declaring that you do *not* want Git to treat them as text files
> (i.e., you do not want Git to change the line endings), so something
> as simple as
> 
> 	* -text
> 
> should do the trick.  See gitattributes(5) for details.

Exactly.
The "hidden consequence" may be the other way around:
If you don't specify .gitattributes, then all people who have core.autocrlf=true
will suffer from a runtime penalty.
core.autocrlf=true means "auto".
At each checkout Git needs to figure out that the file has CRLF in the repo,
so that there is no conversion done.
The same happens on "git add" or "git commit" (and sometimes on "git status").

The penalty may be low, but Dscho once reported that it is measurable & painful
on a "big repo".

The other advantage of "* -text" in .gitattributes is that new files are handled
consistantly accross developers.

Those who have "core.autocrlf=false" would produce commits with CRLF for new
files, and those developpers who have core.autocrlf=true would produce files
with LF in the index and CRLF in the worktree.
This may (most probably will) cause confusion later, when things are pushed and
pulled.

> 
> I'd be interested to hear what happens when diff-ing across a line
> ending fixup commit.  Is this an area where Git needs some
> improvement?  "git merge" knows an -Xrenormalize option to deal with a
> related problem --- it's possible that "git diff" needs to learn a
> similar trick.

That is a tricky thing.
Sometimes you want to see the CLRF - LF as a diff, (represented as "^M"),
and sometimes not.

All in all "* -text" is a good choice for Windows-only projects.

> 
> Thanks and hope that helps,
> Jonathan

Re: Consequences of CRLF in index?

[Johannes Sixt]

Am 24.10.2017 um 19:48 schrieb Lars Schneider:
> I've migrated a large repo (110k+ files) with a lot of history (177k commits)
> and a lot of users (200+) to Git. Unfortunately, all text files in the index
> of the repo have CRLF line endings. In general this seems not to be a problem
> as the project is developed exclusively on Windows.
> 
> However, I wonder if there are any "hidden consequences" of this setup?

I've been working on a project with CRLF in every source file for a 
decade now. It's C++ source, and it isn't even Windows-only: when 
checked out on Linux, there are CRs in the files, with no bad 
consequences so far. GCC is happy with them.

-- Hannes

Re: Consequences of CRLF in index?

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I'd be interested to hear what happens when diff-ing across a line
> ending fixup commit.  Is this an area where Git needs some
> improvement?  "git merge" knows an -Xrenormalize option to deal with a
> related problem --- it's possible that "git diff" needs to learn a
> similar trick.

My knee-jerk reaction is that (1) the end user definitely wants to
see preimage and postimage lines are different in such a commit by
default, one side has and the other side lacks ^M at the end., and
(2) one of the "whitespace ignoring" options (namely, "ignore space
at eol") may suffice, but if not, it should be easy to invent a new
one "ignore CR at eol".

Re: Consequences of CRLF in index?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> I'd be interested to hear what happens when diff-ing across a line
>> ending fixup commit.  Is this an area where Git needs some
>> improvement?  "git merge" knows an -Xrenormalize option to deal with a
>> related problem --- it's possible that "git diff" needs to learn a
>> similar trick.
>
> My knee-jerk reaction is that (1) the end user definitely wants to
> see preimage and postimage lines are different in such a commit by
> default, one side has and the other side lacks ^M at the end., and
> (2) one of the "whitespace ignoring" options (namely, "ignore space
> at eol") may suffice, but if not, it should be easy to invent a new
> one "ignore CR at eol".

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 ;-)

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.

Have fun.

 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))
 		DIFF_OPT_SET(options, DIFF_FROM_CONTENTS);
 	else
 		DIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);
@@ -4659,6 +4660,8 @@ int diff_opt_parse(struct diff_options *options,
 		DIFF_XDL_SET(options, IGNORE_WHITESPACE_CHANGE);
 	else if (!strcmp(arg, "--ignore-space-at-eol"))
 		DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
+	else if (!strcmp(arg, "--ignore-cr-at-eol"))
+		DIFF_XDL_SET(options, IGNORE_CR_AT_EOL);
 	else if (!strcmp(arg, "--ignore-blank-lines"))
 		DIFF_XDL_SET(options, IGNORE_BLANK_LINES);
 	else if (!strcmp(arg, "--indent-heuristic"))
diff --git a/merge-recursive.c b/merge-recursive.c
index 1d3f8f0d22..4a49ec93b1 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2251,6 +2251,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)
 		DIFF_XDL_SET(o, IGNORE_WHITESPACE);
 	else if (!strcmp(s, "ignore-space-at-eol"))
 		DIFF_XDL_SET(o, IGNORE_WHITESPACE_AT_EOL);
+	else if (!strcmp(s, "ignore-cr-at-eol"))
+		DIFF_XDL_SET(o, IGNORE_CR_AT_EOL);
 	else if (!strcmp(s, "renormalize"))
 		o->renormalize = 1;
 	else if (!strcmp(s, "no-renormalize"))
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index b090ad8eac..ff16243da9 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -32,7 +32,8 @@ extern "C" {
 #define XDF_IGNORE_WHITESPACE (1 << 2)
 #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
 #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)
 #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--;
+	if (s == i)
+		return 1;
+	if (s == i + 1 && l[i] == '\r')
+		return 1;
+	return 0;
+}
+
 int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 {
 	int i1, i2;
@@ -170,7 +185,8 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 
 	/*
 	 * -w matches everything that matches with -b, and -b in turn
-	 * matches everything that matches with --ignore-space-at-eol.
+	 * matches everything that matches with --ignore-space-at-eol,
+	 * which in turn matches everything that matches with --ignore-cr-at-eol.
 	 *
 	 * 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])

Re: Consequences of CRLF in index?

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 402 bytes --]

Hi,

On Tue, 24 Oct 2017, Torsten Bögershausen wrote:

> The penalty may be low, but Dscho once reported that it [line endings
> conversion] is measurable & painful on a "big repo".

Yes, I do not remember the details, but it must have been around 5-15%
speed improvement to prevent the autoCRLF stuff from doing its thing.

At work, we always switch it off, for that reason.

Ciao,
Dscho

Re: Consequences of CRLF in index?

[Johannes Schindelin]

Hi Hannes,

On Tue, 24 Oct 2017, Johannes Sixt wrote:

> Am 24.10.2017 um 19:48 schrieb Lars Schneider:
> > I've migrated a large repo (110k+ files) with a lot of history (177k
> > commits)
> > and a lot of users (200+) to Git. Unfortunately, all text files in the index
> > of the repo have CRLF line endings. In general this seems not to be a
> > problem
> > as the project is developed exclusively on Windows.
> > 
> > However, I wonder if there are any "hidden consequences" of this setup?
> 
> I've been working on a project with CRLF in every source file for a decade
> now. It's C++ source, and it isn't even Windows-only: when checked out on
> Linux, there are CRs in the files, with no bad consequences so far. GCC is
> happy with them.

I envy you for the blessing of such a clean C++ source that you do not
have any, say, Unix shell script in it. Try this, and weep:

	$ printf 'echo \\\r\n\t123\r\n' >a1

	$ sh a1

	a1: 2: a1: 123: not found

For the same reason (Unix shell not handling CR/LF gracefull), I went
through that painful work that finally landed as 00ddc9d13ca (Fix build
with core.autocrlf=true, 2017-05-09).

Ciao,
Dscho

Re: Consequences of CRLF in index?

[Stefan Beller]

On Tue, Oct 24, 2017 at 9:53 PM, Junio C Hamano <gitster@pobox.com> 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@google.com/
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])
>
>

Re: Consequences of CRLF in index?

[Lars Schneider]

> On 24 Oct 2017, at 20:14, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 
> Hi,
> 
> Lars Schneider wrote:
> 
>> I've migrated a large repo (110k+ files) with a lot of history (177k commits)
>> and a lot of users (200+) to Git. Unfortunately, all text files in the index
>> of the repo have CRLF line endings. In general this seems not to be a problem
>> as the project is developed exclusively on Windows.
> 
> Sounds good.
> 
>> However, I wonder if there are any "hidden consequences" of this setup?
>> If there are consequences, then I see two options. Either I rebase the repo
>> and fix the line endings for all commits or I add a single commit that fixes
>> the line endings for all files. Both actions require coordination with the
>> users to avoid repo trouble/merge conflicts. The "single fixup commit" options
>> would also make diffs into the past look bad. Would a single large commit have
>> any impact on the performance of standard Git operations?
> 
> There are no hidden consequences that I'm aware of.  If you later
> decide that you want to become a cross-platform project, then you may
> want to switch to LF endings, in which case I suggest the "single
> fixup commit" strategy.
> 
> In any event, you also probably want to declare what you're doing
> using .gitattributes.  By checking in the files as CRLF, you are
> declaring that you do *not* want Git to treat them as text files
> (i.e., you do not want Git to change the line endings), so something
> as simple as
> 
> 	* -text

That's sounds good. Does "-text" have any other implications?
For whatever reason I always thought this is the way to tell
Git that a particular file is binary with the implication that
Git should not attempt to diff it.

Thanks,
Lars

Re: Consequences of CRLF in index?

[Jonathan Nieder]

Hi again,

Lars Schneider wrote:
>> On 24 Oct 2017, at 20:14, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> In any event, you also probably want to declare what you're doing
>> using .gitattributes.  By checking in the files as CRLF, you are
>> declaring that you do *not* want Git to treat them as text files
>> (i.e., you do not want Git to change the line endings), so something
>> as simple as
>>
>> 	* -text
>
> That's sounds good. Does "-text" have any other implications?
> For whatever reason I always thought this is the way to tell
> Git that a particular file is binary with the implication that
> Git should not attempt to diff it.

No other implications.  You're thinking of "-diff".  There is also a
shortcut "binary" which simply means "-text -diff".

Ideas for wording improvements to gitattributes(5) on this subject?

Thanks,
Jonathan

Re: Consequences of CRLF in index?

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

Yes, and I think the codepath that matters most already uses that
form.  Perhaps it is a good idea to do the rewrite without a macro
(XDF_WHITESPACE_FLAGS is already a macro enough).

> (1<<5) is taken twice now.

Good eyes.  I think we use bits #1-#8 now (bit #0 is vacant, so are
#9-#31).

>> 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',

> That seems correct after some thought;

I added the "trim the LF at the end" to the beginning of the
function at the last minute to cheat, and it is debatable if it is
entirely correct on an incomplete line.

The byte at the end of line, i.e. l[s-1], could be either '\n' or
something else, and the latter is an incompete line at the end of
the file.  When we trimmed the LF and decremented s, CR at l[s-1]
is the CR in CRLF, which we do want to ignore.  If we didn't, then
what is CR sitting at l[s-1]?  It is a lone CR at the end of file,
not a part of CRLF.  Do we really want to ignore it?

If we take the name of the option "ignore-cr-at-eol" literally, yes,
it is a CR sitting at the end of a line, which happens to be an
incomplete one, so we do want to ignore.  But if we think about the
reason why we are adding the option (i.e. to help conversion between
CRLF and LF), it is somewhat iffy.  The lone CR at the end of file
cannot be something that came from CRLF<->LF conversion, and ignoring
it would hide possible problems in conversion or the original data.

> I might offer
> another easier to understand (for me) solution,
> ...
> Though this seems even more complicated
> after having it written down.

This happens to me quite often and my solution to it is to remove
the alternative I tried to formulate after convincing myself that it
is not that much of an improvement ;-).

Re: Consequences of CRLF in index?

[Johannes Sixt]

Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:
> I envy you for the blessing of such a clean C++ source that you do not
> have any, say, Unix shell script in it. Try this, and weep:
> 
> 	$ printf 'echo \\\r\n\t123\r\n' >a1
> 
> 	$ sh a1
> 
> 	a1: 2: a1: 123: not found

I was bitten by that, too. For this reason, I ensure that shell scripts 
and Makefiles begin their life on Linux. Fortunately, modern editors on 
Windows, includ^Wand vi, do not force CRLF line breaks, and such files 
can be edited on Windows, too.

Of course, I do not set core.autocrlf anywhere to avoid any changes 
behind my back.

> For the same reason (Unix shell not handling CR/LF gracefull), I went
> through that painful work that finally landed as 00ddc9d13ca (Fix build
> with core.autocrlf=true, 2017-05-09).

That's much appreciated!

-- Hannes

Re: Consequences of CRLF in index?

[Lars Schneider]

> On 26 Oct 2017, at 09:09, Johannes Sixt <j6t@kdbg.org> wrote:
> 
> Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:
>> I envy you for the blessing of such a clean C++ source that you do not
>> have any, say, Unix shell script in it. Try this, and weep:
>> 	$ printf 'echo \\\r\n\t123\r\n' >a1
>> 	$ sh a1
>> 	a1: 2: a1: 123: not found
> 
> I was bitten by that, too. For this reason, I ensure that shell scripts and Makefiles begin their life on Linux. Fortunately, modern editors on Windows, includ^Wand vi, do not force CRLF line breaks, and such files can be edited on Windows, too.

Wouldn't this kind of .gitattributes setup solve the problem?

*     -text
*.sh   text eol=lf

Thanks,
Lars

Re: Consequences of CRLF in index?

[Lars Schneider]

> On 25 Oct 2017, at 19:13, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 
> Hi again,
> 
> Lars Schneider wrote:
>>> On 24 Oct 2017, at 20:14, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 
>>> In any event, you also probably want to declare what you're doing
>>> using .gitattributes.  By checking in the files as CRLF, you are
>>> declaring that you do *not* want Git to treat them as text files
>>> (i.e., you do not want Git to change the line endings), so something
>>> as simple as
>>> 
>>> 	* -text
>> 
>> That's sounds good. Does "-text" have any other implications?
>> For whatever reason I always thought this is the way to tell
>> Git that a particular file is binary with the implication that
>> Git should not attempt to diff it.
> 
> No other implications.  You're thinking of "-diff".  There is also a
> shortcut "binary" which simply means "-text -diff".

Yeah. Well, when I read "-text" then I think "no text" and that makes
me think "is binary".


> Ideas for wording improvements to gitattributes(5) on this subject?

I think the wording in the docs is good. It is just the "text" keyword
that confused me. Maybe this could have been names "eolnorm" and
"-eolnorm" or something. But it is too late for that now I guess :-)

Thanks,
Lars

Re: Consequences of CRLF in index?

[Torsten Bögershausen]

On Wed, Oct 25, 2017 at 10:13:57AM -0700, Jonathan Nieder wrote:
> Hi again,
> 
> Lars Schneider wrote:
> >> On 24 Oct 2017, at 20:14, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 
> >> In any event, you also probably want to declare what you're doing
> >> using .gitattributes.  By checking in the files as CRLF, you are
> >> declaring that you do *not* want Git to treat them as text files
> >> (i.e., you do not want Git to change the line endings), so something
> >> as simple as
> >>
> >> 	* -text
> >
> > That's sounds good. Does "-text" have any other implications?
> > For whatever reason I always thought this is the way to tell
> > Git that a particular file is binary with the implication that
> > Git should not attempt to diff it.
> 
> No other implications.  You're thinking of "-diff".  There is also a
> shortcut "binary" which simply means "-text -diff".

Not 100% the same, as far as I know.
"binary" means: Don't convert line endings, and there is now way to
do a readable diff.
The only thing to tell the user is: The binary blobs are different.

Then we have "text". The "old" version of "text" was "crlf", which
for some people was more intuitive, and less intuitive for others.
"* crlf" is the same as "* text" and means please convert line endings.
And yes, the file is still line oriented.
"* -crlf" means don't touch the line endings, the file is
line-orinted and diff and  merge will work.
"* -text" is the same as "* -crlf"

> 
> Ideas for wording improvements to gitattributes(5) on this subject?

None from me at the moment.

> 
> Thanks,
> Jonathan

Re: Consequences of CRLF in index?

[Torsten Bögershausen]

On Thu, Oct 26, 2017 at 01:01:25PM +0200, Lars Schneider wrote:
> 
> > On 26 Oct 2017, at 09:09, Johannes Sixt <j6t@kdbg.org> wrote:
> > 
> > Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:
> >> I envy you for the blessing of such a clean C++ source that you do not
> >> have any, say, Unix shell script in it. Try this, and weep:
> >> 	$ printf 'echo \\\r\n\t123\r\n' >a1
> >> 	$ sh a1
> >> 	a1: 2: a1: 123: not found
> > 
> > I was bitten by that, too. For this reason, I ensure that shell scripts and Makefiles begin their life on Linux. Fortunately, modern editors on Windows, includ^Wand vi, do not force CRLF line breaks, and such files can be edited on Windows, too.
> 
> Wouldn't this kind of .gitattributes setup solve the problem?
> 
> *     -text
> *.sh   text eol=lf
> 

Yes, exactly. and for the snake-lovers:

*.py   text eol=lf

Re: Consequences of CRLF in index?

[Johannes Sixt]

Am 26.10.2017 um 13:01 schrieb Lars Schneider:
>> On 26 Oct 2017, at 09:09, Johannes Sixt <j6t@kdbg.org> wrote:
>> Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:
>>> I envy you for the blessing of such a clean C++ source that you do not
>>> have any, say, Unix shell script in it. Try this, and weep:
>>> 	$ printf 'echo \\\r\n\t123\r\n' >a1
>>> 	$ sh a1
>>> 	a1: 2: a1: 123: not found
>>
>> I was bitten by that, too. For this reason, I ensure that shell scripts and Makefiles begin their life on Linux. Fortunately, modern editors on Windows, includ^Wand vi, do not force CRLF line breaks, and such files can be edited on Windows, too.
> 
> Wouldn't this kind of .gitattributes setup solve the problem?

But there is no problem. Don't have CRs in shell scripts and be done 
with it.

> 
> *     -text
> *.sh   text eol=lf

Why would that be necessary? I cannot have CRLF in shell scripts etc., 
not even on Windows. (And in addition I do not mind CR in C++ source 
code.) All I want is: Git, please, by all means, do not mess with my 
files, ever. Isn't the simplest way to achieve that to set *nothing* at 
all? Not even core.autocrlf?

-- Hannes

Re: Consequences of CRLF in index?

[Jonathan Nieder]

Johannes Sixt wrote:
> Am 26.10.2017 um 13:01 schrieb Lars Schneider:

>> *     -text
>> *.sh   text eol=lf
>
> Why would that be necessary? I cannot have CRLF in shell scripts etc., not
> even on Windows. (And in addition I do not mind CR in C++ source code.) All
> I want is: Git, please, by all means, do not mess with my files, ever. Isn't
> the simplest way to achieve that to set *nothing* at all? Not even
> core.autocrlf?

That's why Lars suggests "* -text" in .gitattributes.  That way, if
some user of the repository *does* set core.autocrlf, you still get
the "do not mess with my files" semantics you want.

If you control the configuration of all users of the repository, you
don't need that, but it doesn't hurt, either.

With that "* -text", you do not need "*.sh text eol=lf", but maybe
you'd want it in order to get some warnings when someone changes the
line endings by mistake without running tests.  (Better to have a
culture of running tests, you might say.  Fair enough, but as with the
other .gitattributes line, it doesn't hurt.)

Jonathan

Re: Consequences of CRLF in index?

[Johannes Sixt]

Thank you for the clarification, it's much appreciated.

-- Hannes

Re: Consequences of CRLF in index?

[Ross Kabus]

Is "* -text" in any way different than "-text" (without the * asterisk)? All
of my .gitattributes files have "-text" (no * asterisk) and I haven't noticed
any difference but could I be missing something subtle?

~Ross

Re: Consequences of CRLF in index?

[Junio C Hamano]

Ross Kabus <rkabus@aerotech.com> writes:

> Is "* -text" in any way different than "-text" (without the * asterisk)? All
> of my .gitattributes files have "-text" (no * asterisk) and I haven't noticed
> any difference but could I be missing something subtle?
>
> ~Ross

A line in the .gitattibute file is of the form

	<pattern>	<attribute definition>...

i.e. a pattern to match paths, with a list of attribute definitions.
The asterisk they are showing in their description is the <pattern>
part, i.e. "apply the '-text' thing to paths that match '*'", which
is equivalent to saying "set text attribute to false for all paths".

Re* Consequences of CRLF in index?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Stefan Beller <sbeller@google.com> writes:
>
>> (1<<5) is taken twice now.
>
> Good eyes.  I think we use bits #1-#8 now (bit #0 is vacant, so are
> #9-#31).

Let's do this bit-shuffling as a preliminary clean-up.

-- >8 --
Subject: [PATCH] xdiff: reassign xpparm_t.flags bits

We have packed the bits too tightly in such a way that it is not
easy to add a new type of whitespace ignoring option, a new type
of LCS algorithm, or a new type of post-cleanup heuristics.

Reorder bits a bit to give room for these three classes of options
to grow.

While at it, add a comment in front of the bit definitions to
clarify in which structure these defined bits may appear.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 xdiff/xdiff.h | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index b090ad8eac..457cac32d8 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -27,22 +27,24 @@
 extern "C" {
 #endif /* #ifdef __cplusplus */
 
+/* xpparm_t.flags */
+#define XDF_NEED_MINIMAL (1 << 0)
 
-#define XDF_NEED_MINIMAL (1 << 1)
 #define XDF_IGNORE_WHITESPACE (1 << 2)
 #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
 #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_PATIENCE_DIFF (1 << 5)
-#define XDF_HISTOGRAM_DIFF (1 << 6)
+#define XDF_IGNORE_BLANK_LINES (1 << 7)
+
+#define XDF_PATIENCE_DIFF (1 << 14)
+#define XDF_HISTOGRAM_DIFF (1 << 15)
 #define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)
 #define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)
 
-#define XDF_IGNORE_BLANK_LINES (1 << 7)
-
-#define XDF_INDENT_HEURISTIC (1 << 8)
+#define XDF_INDENT_HEURISTIC (1 << 23)
 
+/* xdemitconf_t.flags */
 #define XDL_EMIT_FUNCNAMES (1 << 0)
 #define XDL_EMIT_FUNCCONTEXT (1 << 2)
 
-- 
2.15.0-rc2-266-g8f92d095f4

Re: Consequences of CRLF in index?

[Johannes Schindelin]

Hi Lars,

On Thu, 26 Oct 2017, Lars Schneider wrote:

> > On 26 Oct 2017, at 09:09, Johannes Sixt <j6t@kdbg.org> wrote:
> > 
> > Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:
> >> I envy you for the blessing of such a clean C++ source that you do not
> >> have any, say, Unix shell script in it. Try this, and weep:
> >> 	$ printf 'echo \\\r\n\t123\r\n' >a1
> >> 	$ sh a1
> >> 	a1: 2: a1: 123: not found
> > 
> > I was bitten by that, too. For this reason, I ensure that shell
> > scripts and Makefiles begin their life on Linux. Fortunately, modern
> > editors on Windows, includ^Wand vi, do not force CRLF line breaks, and

                        ^^^^^^^^^^^^^^

This put a well-needed smile on my face. Thanks.

> > such files can be edited on Windows, too.
> 
> Wouldn't this kind of .gitattributes setup solve the problem?
> 
> *     -text
> *.sh   text eol=lf

If you look at the commit I mentioned, you will see examples where it
breaks down: when using Unix shell scripts *without* .sh file extension.
Most notable offender: GIT-VERSION-GEN.

Ciao,
Dscho

Re: Re* Consequences of CRLF in index?

[Stefan Beller]

On Thu, Oct 26, 2017 at 11:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Subject: [PATCH] xdiff: reassign xpparm_t.flags bits
>
> We have packed the bits too tightly in such a way that it is not
> easy to add a new type of whitespace ignoring option, a new type
> of LCS algorithm, or a new type of post-cleanup heuristics.
>
> Reorder bits a bit to give room for these three classes of options
> to grow.
>
> While at it, add a comment in front of the bit definitions to
> clarify in which structure these defined bits may appear.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Reviewed-by: Stefan Beller <sbeller@google.com>

> ---
>  xdiff/xdiff.h | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
> index b090ad8eac..457cac32d8 100644
> --- a/xdiff/xdiff.h
> +++ b/xdiff/xdiff.h
> @@ -27,22 +27,24 @@
>  extern "C" {
>  #endif /* #ifdef __cplusplus */
>
> +/* xpparm_t.flags */
> +#define XDF_NEED_MINIMAL (1 << 0)
>
> -#define XDF_NEED_MINIMAL (1 << 1)

The whitespace flags are also stored in xpparm_t, though
these flags can also come in from other sources (e.g. we
just pass it in manually via the interface).

>  #define XDF_IGNORE_WHITESPACE (1 << 2)
>  #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
>  #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_PATIENCE_DIFF (1 << 5)
> -#define XDF_HISTOGRAM_DIFF (1 << 6)
> +#define XDF_IGNORE_BLANK_LINES (1 << 7)

Is XDF_IGNORE_BLANK_LINES a whitespace option, just
on the lines level instead of the inside one line? I think
it still makes sense to keep it here, slightly separated from
the IGNORE flags.

> +#define XDF_PATIENCE_DIFF (1 << 14)
> +#define XDF_HISTOGRAM_DIFF (1 << 15)
>  #define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)
>  #define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)
>
> -#define XDF_IGNORE_BLANK_LINES (1 << 7)
> -
> -#define XDF_INDENT_HEURISTIC (1 << 8)
> +#define XDF_INDENT_HEURISTIC (1 << 23)
>
> +/* xdemitconf_t.flags */
>  #define XDL_EMIT_FUNCNAMES (1 << 0)
>  #define XDL_EMIT_FUNCCONTEXT (1 << 2)

This looked like it was carefully crafted to avoid accidental bit overlap
with XDF_NEED_MINIMAL; but these are in different flag fields, this
should be fine.

Thanks,
Stefan

[PATCH 0/2] Re* Consequences of CRLF in index?

[Stefan Beller]

> Let's do this bit-shuffling as a preliminary clean-up.

These 2 patches can go on top of that as well.

Thanks,
Stefan

Stefan Beller (2):
  xdiff/xdiff.h: remove unused flags
  xdiff/xdiffi.c: remove unneeded function declarations

 xdiff/xdiff.h  |  8 --------
 xdiff/xdiffi.c | 17 -----------------
 2 files changed, 25 deletions(-)

-- 
2.15.0.rc2.443.gfcc3b81c0a

[PATCH 1/2] xdiff/xdiff.h: remove unused flags

[Stefan Beller]

These flags were there since the beginning (3443546f6e (Use a *real*
built-in diff generator, 2006-03-24), but were never used. Remove them.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 xdiff/xdiff.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 457cac32d8..0a41f5e500 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -48,14 +48,6 @@ extern "C" {
 #define XDL_EMIT_FUNCNAMES (1 << 0)
 #define XDL_EMIT_FUNCCONTEXT (1 << 2)
 
-#define XDL_MMB_READONLY (1 << 0)
-
-#define XDL_MMF_ATOMIC (1 << 0)
-
-#define XDL_BDOP_INS 1
-#define XDL_BDOP_CPY 2
-#define XDL_BDOP_INSB 3
-
 /* merge simplification levels */
 #define XDL_MERGE_MINIMAL 0
 #define XDL_MERGE_EAGER 1
-- 
2.15.0.rc2.443.gfcc3b81c0a

[PATCH 2/2] xdiff/xdiffi.c: remove unneeded function declarations

[Stefan Beller]

There is no need to forward-declare these functions, as they are used
after their implementation only.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 xdiff/xdiffi.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 93a65680a1..0a4ea948f9 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -22,34 +22,17 @@
 
 #include "xinclude.h"
 
-
-
 #define XDL_MAX_COST_MIN 256
 #define XDL_HEUR_MIN_COST 256
 #define XDL_LINE_MAX (long)((1UL << (CHAR_BIT * sizeof(long) - 1)) - 1)
 #define XDL_SNAKE_CNT 20
 #define XDL_K_HEUR 4
 
-
-
 typedef struct s_xdpsplit {
 	long i1, i2;
 	int min_lo, min_hi;
 } xdpsplit_t;
 
-
-
-
-static long xdl_split(unsigned long const *ha1, long off1, long lim1,
-		      unsigned long const *ha2, long off2, long lim2,
-		      long *kvdf, long *kvdb, int need_min, xdpsplit_t *spl,
-		      xdalgoenv_t *xenv);
-static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1, long chg2);
-
-
-
-
-
 /*
  * See "An O(ND) Difference Algorithm and its Variations", by Eugene Myers.
  * Basically considers a "box" (off1, off2, lim1, lim2) and scan from both
-- 
2.15.0.rc2.443.gfcc3b81c0a

Re: [PATCH 0/2] Re* Consequences of CRLF in index?

[Stefan Beller]

On Fri, Oct 27, 2017 at 10:07 AM, Stefan Beller <sbeller@google.com> wrote:
>> Let's do this bit-shuffling as a preliminary clean-up.
>
> These 2 patches can go on top of that as well.

Actually these textually do not conflict with your patch,
and they can be picked independently, e.g. they could
go on top of sb/diff-color-moved-use-xdl-recmatch
as a diff cleanup.

(I note this as you regard your patches as a lunch time hack
in the cooking email; I am serious about these patches though.)

Thanks,
Stefan

Re: [PATCH 0/2] Re* Consequences of CRLF in index?

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> (I note this as you regard your patches as a lunch time hack
> in the cooking email; I am serious about these patches though.)

We do not want to touch borrowed code unnecessarily.  Are these
lines and bits hampering further progress we are actively trying to
make right now?

Re: [PATCH 0/2] Re* Consequences of CRLF in index?

[Stefan Beller]

On Mon, Oct 30, 2017 at 7:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> (I note this as you regard your patches as a lunch time hack
>> in the cooking email; I am serious about these patches though.)
>
> We do not want to touch borrowed code unnecessarily.  Are these
> lines and bits hampering further progress we are actively trying to
> make right now?

No they are not, you are correct.

I differ in opinion on 'borrowed code'. The latest release of xdiff
(v0.23) was in Nov 13, 2008 according to http://freecode.com/projects/xdiff-lib
or on March 23, 2006 according to https://directory.fsf.org/wiki/LibXDiff
and given that we incorporated so many changes already to xdiff,
I would argue it is sufficiently different from the original, we'll probably
never import another upstream version (if there will be a release at all).

So the code was rather taken and now we are the bag holders in
maintaining it, so we can make it pretty even only for the sake of
pleasing ourselves (or rather: not confusing ourselves with too
many unused flags).

Thanks,
Stefan

Re: [PATCH 0/2] Re* Consequences of CRLF in index?

[Jeff King]

On Tue, Oct 31, 2017 at 09:41:25AM -0700, Stefan Beller wrote:

> On Mon, Oct 30, 2017 at 7:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Stefan Beller <sbeller@google.com> writes:
> >
> >> (I note this as you regard your patches as a lunch time hack
> >> in the cooking email; I am serious about these patches though.)
> >
> > We do not want to touch borrowed code unnecessarily.  Are these
> > lines and bits hampering further progress we are actively trying to
> > make right now?
> 
> No they are not, you are correct.
> 
> I differ in opinion on 'borrowed code'. The latest release of xdiff
> (v0.23) was in Nov 13, 2008 according to http://freecode.com/projects/xdiff-lib
> or on March 23, 2006 according to https://directory.fsf.org/wiki/LibXDiff
> and given that we incorporated so many changes already to xdiff,
> I would argue it is sufficiently different from the original, we'll probably
> never import another upstream version (if there will be a release at all).
> 
> So the code was rather taken and now we are the bag holders in
> maintaining it, so we can make it pretty even only for the sake of
> pleasing ourselves (or rather: not confusing ourselves with too
> many unused flags).

Yes, I'd agree. For what it's worth both sides of this argument played
out in my head when I saw your patches, and I ended up at the same "we
are the bag holders" place. And there's certainly precedent for touching
that code and cleaning it up to make it easier to work with (just look
at at "git log -p xdiff").

I don't know that I would support a full-scale rewrite (for the same
reason that I wouldn't on the rest of the code base -- avoiding churn).
But deleting unused bits seems like an easy win for readability.

-Peff

[PATCH v2 0/2] Teach "diff" to ignore only CR at EOL

[Junio C Hamano]

This time with doc updates and tests.  The previous one is at 

https://public-inbox.org/git/xmqqshe7j0af.fsf@gitster.mtv.corp.google.com

A change that is supposed to only change the end-of-line convention
between LF <-> CRLF and nothing else can be verified with "diff -b"
or "diff --ignore-space-at-eol" in practice, but these hide changes
in whitespaces other than the carriage return at the end of the lines.

These two patches introduce a new --ignore-cr-at-eol option so that
only CR at eol and no other whitespace changes are ignored while
making a comparison, to allow a stricter validation.

I am not sure how much practical value the new option has.  The
preliminary clean-up patch to shuffle the bis assignment for flags
does have values, though ;-).

Junio C Hamano (2):
  xdiff: reassign xpparm_t.flags bits
  diff: --ignore-cr-at-eol

 Documentation/diff-options.txt     |  3 +++
 Documentation/merge-strategies.txt |  5 +++--
 diff.c                             |  6 +++---
 merge-recursive.c                  |  2 ++
 t/t4015-diff-whitespace.sh         | 28 ++++++++++++++++++++++++++++
 xdiff/xdiff.h                      | 26 ++++++++++++++++----------
 xdiff/xutils.c                     | 38 ++++++++++++++++++++++++++++++++++++--
 7 files changed, 91 insertions(+), 17 deletions(-)

-- 
2.15.0-263-g47cc852023

[PATCH v2 1/2] xdiff: reassign xpparm_t.flags bits

[Junio C Hamano]

We have packed the bits too tightly in such a way that it is not
easy to add a new type of whitespace ignoring option, a new type
of LCS algorithm, or a new type of post-cleanup heuristics.

Reorder bits a bit to give room for these three classes of options
to grow.  Also make use of XDF_WHITESPACE_FLAGS macro where we check
any of these bits are on, instead of using DIFF_XDL_TST() macro on
individual possibilities.  That way, the "is any of the bits on?"
code does not have to change when we add more ways to ignore
whitespaces.

While at it, add a comment in front of the bit definitions to
clarify in which structure these defined bits may appear.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c        |  4 +---
 xdiff/xdiff.h | 24 ++++++++++++++----------
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/diff.c b/diff.c
index 74283d9001..790250fe86 100644
--- a/diff.c
+++ b/diff.c
@@ -3434,9 +3434,7 @@ void diff_setup_done(struct diff_options *options)
 	 * inside contents.
 	 */
 
-	if (DIFF_XDL_TST(options, IGNORE_WHITESPACE) ||
-	    DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) ||
-	    DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL))
+	if ((options->xdl_opts & XDF_WHITESPACE_FLAGS))
 		DIFF_OPT_SET(options, DIFF_FROM_CONTENTS);
 	else
 		DIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index b090ad8eac..cbf5d8e166 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -27,22 +27,26 @@
 extern "C" {
 #endif /* #ifdef __cplusplus */
 
+/* xpparm_t.flags */
+#define XDF_NEED_MINIMAL (1 << 0)
 
-#define XDF_NEED_MINIMAL (1 << 1)
-#define XDF_IGNORE_WHITESPACE (1 << 2)
-#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
-#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_WHITESPACE (1 << 1)
+#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 2)
+#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 3)
+#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | \
+			      XDF_IGNORE_WHITESPACE_CHANGE | \
+			      XDF_IGNORE_WHITESPACE_AT_EOL)
 
-#define XDF_PATIENCE_DIFF (1 << 5)
-#define XDF_HISTOGRAM_DIFF (1 << 6)
+#define XDF_IGNORE_BLANK_LINES (1 << 7)
+
+#define XDF_PATIENCE_DIFF (1 << 14)
+#define XDF_HISTOGRAM_DIFF (1 << 15)
 #define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)
 #define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)
 
-#define XDF_IGNORE_BLANK_LINES (1 << 7)
-
-#define XDF_INDENT_HEURISTIC (1 << 8)
+#define XDF_INDENT_HEURISTIC (1 << 23)
 
+/* xdemitconf_t.flags */
 #define XDL_EMIT_FUNCNAMES (1 << 0)
 #define XDL_EMIT_FUNCCONTEXT (1 << 2)
 
-- 
2.15.0-263-g47cc852023

[PATCH v2 2/2] diff: --ignore-cr-at-eol

[Junio C Hamano]

A new option --ignore-cr-at-eol tells the diff machinery to treat a
carriage-return at the end of a (complete) line as if it does not
exist.

This would make it easier to review a change whose only effect is to
turn line endings from CRLF to LF or the other way around.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/diff-options.txt     |  3 +++
 Documentation/merge-strategies.txt |  5 +++--
 diff.c                             |  2 ++
 merge-recursive.c                  |  2 ++
 t/t4015-diff-whitespace.sh         | 28 ++++++++++++++++++++++++++++
 xdiff/xdiff.h                      |  4 +++-
 xdiff/xutils.c                     | 38 ++++++++++++++++++++++++++++++++++++--
 7 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 89cc0f48de..aa2c0ff74d 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -519,6 +519,9 @@ endif::git-format-patch[]
 --text::
 	Treat all files as text.
 
+--ignore-cr-at-eol::
+	Ignore carrige-return at the end of line when doing a comparison.
+
 --ignore-space-at-eol::
 	Ignore changes in whitespace at EOL.
 
diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 2eb92b9327..030744910e 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -57,11 +57,12 @@ diff-algorithm=[patience|minimal|histogram|myers];;
 ignore-space-change;;
 ignore-all-space;;
 ignore-space-at-eol;;
+ignore-cr-at-eol;;
 	Treats lines with the indicated type of whitespace change as
 	unchanged for the sake of a three-way merge.  Whitespace
 	changes mixed with other changes to a line are not ignored.
-	See also linkgit:git-diff[1] `-b`, `-w`, and
-	`--ignore-space-at-eol`.
+	See also linkgit:git-diff[1] `-b`, `-w`,
+	`--ignore-space-at-eol`, and `--ignore-cr-at-eol`.
 +
 * If 'their' version only introduces whitespace changes to a line,
   'our' version is used;
diff --git a/diff.c b/diff.c
index 790250fe86..dd14e4190c 100644
--- a/diff.c
+++ b/diff.c
@@ -3888,6 +3888,8 @@ int diff_opt_parse(struct diff_options *options,
 		DIFF_XDL_SET(options, IGNORE_WHITESPACE_CHANGE);
 	else if (!strcmp(arg, "--ignore-space-at-eol"))
 		DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
+	else if (!strcmp(arg, "--ignore-cr-at-eol"))
+		DIFF_XDL_SET(options, IGNORE_CR_AT_EOL);
 	else if (!strcmp(arg, "--ignore-blank-lines"))
 		DIFF_XDL_SET(options, IGNORE_BLANK_LINES);
 	else if (!strcmp(arg, "--indent-heuristic"))
diff --git a/merge-recursive.c b/merge-recursive.c
index 7a7d55aabe..006b94baf2 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2214,6 +2214,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)
 		DIFF_XDL_SET(o, IGNORE_WHITESPACE);
 	else if (!strcmp(s, "ignore-space-at-eol"))
 		DIFF_XDL_SET(o, IGNORE_WHITESPACE_AT_EOL);
+	else if (!strcmp(s, "ignore-cr-at-eol"))
+		DIFF_XDL_SET(o, IGNORE_CR_AT_EOL);
 	else if (!strcmp(s, "renormalize"))
 		o->renormalize = 1;
 	else if (!strcmp(s, "no-renormalize"))
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 289806d0c7..32dd54c21d 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -106,6 +106,8 @@ test_expect_success 'another test, without options' '
 	git diff -w -b --ignore-space-at-eol >out &&
 	test_cmp expect out &&
 
+	git diff -w --ignore-cr-at-eol >out &&
+	test_cmp expect out &&
 
 	tr "Q_" "\015 " <<-\EOF >expect &&
 	diff --git a/x b/x
@@ -128,6 +130,9 @@ test_expect_success 'another test, without options' '
 	git diff -b --ignore-space-at-eol >out &&
 	test_cmp expect out &&
 
+	git diff -b --ignore-cr-at-eol >out &&
+	test_cmp expect out &&
+
 	tr "Q_" "\015 " <<-\EOF >expect &&
 	diff --git a/x b/x
 	index d99af23..22d9f73 100644
@@ -145,6 +150,29 @@ test_expect_success 'another test, without options' '
 	 CR at end
 	EOF
 	git diff --ignore-space-at-eol >out &&
+	test_cmp expect out &&
+
+	git diff --ignore-space-at-eol --ignore-cr-at-eol >out &&
+	test_cmp expect out &&
+
+	tr "Q_" "\015 " <<-\EOF >expect &&
+	diff --git a/x b/x
+	index_d99af23..22d9f73 100644
+	--- a/x
+	+++ b/x
+	@@ -1,6 +1,6 @@
+	-whitespace at beginning
+	-whitespace change
+	-whitespace in the middle
+	-whitespace at end
+	+_	whitespace at beginning
+	+whitespace_	_change
+	+white space in the middle
+	+whitespace at end__
+	 unchanged line
+	 CR at end
+	EOF
+	git diff --ignore-cr-at-eol >out &&
 	test_cmp expect out
 '
 
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index cbf5d8e166..51f8926215 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -33,9 +33,11 @@ extern "C" {
 #define XDF_IGNORE_WHITESPACE (1 << 1)
 #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 2)
 #define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 3)
+#define XDF_IGNORE_CR_AT_EOL (1 << 4)
 #define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | \
 			      XDF_IGNORE_WHITESPACE_CHANGE | \
-			      XDF_IGNORE_WHITESPACE_AT_EOL)
+			      XDF_IGNORE_WHITESPACE_AT_EOL | \
+			      XDF_IGNORE_CR_AT_EOL)
 
 #define XDF_IGNORE_BLANK_LINES (1 << 7)
 
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 04d7b32e4e..b2cbcc818f 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -156,6 +156,24 @@ 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)
+{
+	int complete = s && l[s-1] == '\n';
+
+	if (complete)
+		s--;
+	if (s == i)
+		return 1;
+	/* do not ignore CR at the end of an incomplete line */
+	if (complete && s == i + 1 && l[i] == '\r')
+		return 1;
+	return 0;
+}
+
 int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 {
 	int i1, i2;
@@ -170,7 +188,8 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 
 	/*
 	 * -w matches everything that matches with -b, and -b in turn
-	 * matches everything that matches with --ignore-space-at-eol.
+	 * matches everything that matches with --ignore-space-at-eol,
+	 * which in turn matches everything that matches with --ignore-cr-at-eol.
 	 *
 	 * Each flavor of ignoring needs different logic to skip whitespaces
 	 * while we have both sides to compare.
@@ -204,6 +223,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 +257,16 @@ 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) {
+			/* do not ignore CR at the end of an incomplete line */
+			if (*ptr == '\r' &&
+			    (ptr + 1 < top && ptr[1] == '\n'))
+				continue;
+		}
+		else if (XDL_ISSPACE(*ptr)) {
 			const char *ptr2 = ptr;
 			int at_eol;
 			while (ptr + 1 < top && XDL_ISSPACE(ptr[1])
-- 
2.15.0-263-g47cc852023

Re: [PATCH v2 0/2] Teach "diff" to ignore only CR at EOL

[Johannes Schindelin]

Hi Junio,

On Tue, 7 Nov 2017, Junio C Hamano wrote:

> This time with doc updates and tests.  The previous one is at 
> 
> https://public-inbox.org/git/xmqqshe7j0af.fsf@gitster.mtv.corp.google.com
> 
> A change that is supposed to only change the end-of-line convention
> between LF <-> CRLF and nothing else can be verified with "diff -b"
> or "diff --ignore-space-at-eol" in practice, but these hide changes
> in whitespaces other than the carriage return at the end of the lines.

Good. I was wishing for such a feature in the past.

However, the short and sweet `-b` or `-w` switches are really, really
nice. `--ignore-cr-at-eol` is just very cumbersome to type out. So I think
you will want to add this patch to your patch series:

-- snipsnap --
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 0e16f017a41..b7a45e8df29 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1400,7 +1400,7 @@ __git_diff_common_options="--stat --numstat --shortstat --summary
 			--patch-with-stat --name-only --name-status --color
 			--no-color --color-words --no-renames --check
 			--full-index --binary --abbrev --diff-filter=
-			--find-copies-harder
+			--find-copies-harder --ignore-cr-at-eol
 			--text --ignore-space-at-eol --ignore-space-change
 			--ignore-all-space --ignore-blank-lines --exit-code
 			--quiet --ext-diff --no-ext-diff

Re: [PATCH v2 1/2] xdiff: reassign xpparm_t.flags bits

[Johannes Schindelin]

Hi Junio,

On Tue, 7 Nov 2017, Junio C Hamano wrote:

> We have packed the bits too tightly in such a way that it is not
> easy to add a new type of whitespace ignoring option, a new type
> of LCS algorithm, or a new type of post-cleanup heuristics.
> 
> Reorder bits a bit to give room for these three classes of options
> to grow.  Also make use of XDF_WHITESPACE_FLAGS macro where we check
> any of these bits are on, instead of using DIFF_XDL_TST() macro on
> individual possibilities.  That way, the "is any of the bits on?"
> code does not have to change when we add more ways to ignore
> whitespaces.
> 
> While at it, add a comment in front of the bit definitions to
> clarify in which structure these defined bits may appear.

Makes sense.

> diff --git a/diff.c b/diff.c
> index 74283d9001..790250fe86 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3434,9 +3434,7 @@ void diff_setup_done(struct diff_options *options)
>  	 * inside contents.
>  	 */
>  
> -	if (DIFF_XDL_TST(options, IGNORE_WHITESPACE) ||
> -	    DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) ||
> -	    DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL))
> +	if ((options->xdl_opts & XDF_WHITESPACE_FLAGS))

Not only shorter now, but a lot clearer, too: nobody needs to wonder now
whether one whitespace flag was excluded on purpose.

>  		DIFF_OPT_SET(options, DIFF_FROM_CONTENTS);
>  	else
>  		DIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);
> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
> index b090ad8eac..cbf5d8e166 100644
> --- a/xdiff/xdiff.h
> +++ b/xdiff/xdiff.h
> @@ -27,22 +27,26 @@
>  extern "C" {
>  #endif /* #ifdef __cplusplus */
>  
> +/* xpparm_t.flags */
> +#define XDF_NEED_MINIMAL (1 << 0)
>  
> -#define XDF_NEED_MINIMAL (1 << 1)

This change makes me wonder whether the least significant bit was omitted
on purpose originally. You probably looked at that? May be worth
mentioning in the commit message.

> -#define XDF_IGNORE_WHITESPACE (1 << 2)
> -#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
> -#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_WHITESPACE (1 << 1)
> +#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 2)
> +#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 3)
> +#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | \
> +			      XDF_IGNORE_WHITESPACE_CHANGE | \
> +			      XDF_IGNORE_WHITESPACE_AT_EOL)
>  
> -#define XDF_PATIENCE_DIFF (1 << 5)
> -#define XDF_HISTOGRAM_DIFF (1 << 6)
> +#define XDF_IGNORE_BLANK_LINES (1 << 7)
> +
> +#define XDF_PATIENCE_DIFF (1 << 14)
> +#define XDF_HISTOGRAM_DIFF (1 << 15)
>  #define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)
>  #define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)
>  
> -#define XDF_IGNORE_BLANK_LINES (1 << 7)
> -
> -#define XDF_INDENT_HEURISTIC (1 << 8)
> +#define XDF_INDENT_HEURISTIC (1 << 23)
>  
> +/* xdemitconf_t.flags */
>  #define XDL_EMIT_FUNCNAMES (1 << 0)
>  #define XDL_EMIT_FUNCCONTEXT (1 << 2)

It is a pity that this diff is not easier to review, but it shows how much
it was in need of cleaning up. Looks much nicer now.

I wonder, however, what your guiding principle was in determining the
gaps? I would have expected consecutive bits, except for the one gap to
make room for the upcoming flag, of course.

Future patch series could always start by making room for new flags, as
needed, after all.

Ciao,
Dscho

Re: [PATCH v2 2/2] diff: --ignore-cr-at-eol

[Johannes Schindelin]

Hi Junio,

On Tue, 7 Nov 2017, Junio C Hamano wrote:

> A new option --ignore-cr-at-eol tells the diff machinery to treat a
> carriage-return at the end of a (complete) line as if it does not
> exist.
> 
> This would make it easier to review a change whose only effect is to
> turn line endings from CRLF to LF or the other way around.

If the goal is to make CR/LF -> LF conversions easier to review (or for
that matter, LF -> CR/LF), then this option may not be *completely*
satisfactory, as it would hide mixed changes (i.e. where some lines are
converted from CR/LF to LF and others are converted in the other direction
*in the same patch*).

I wonder whether it would make this patch series even more useful if you
would instead introduce --hide-crlf-to-lf and --hide-lf-to-crlf options
(not even necessarily mutually exclusive, in case the reviewer really
wants to ignore all line ending conversions).

> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 89cc0f48de..aa2c0ff74d 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -519,6 +519,9 @@ endif::git-format-patch[]
>  --text::
>  	Treat all files as text.
>  
> +--ignore-cr-at-eol::
> +	Ignore carrige-return at the end of line when doing a comparison.

I am not a native speaker, either, yet I have the impression that "do a
comparison" may be more colloquial than not. Also, it is a carriage-return
(as in Sinatra's famous song about Love and Marriage) not a carrige-return.

How about "Hide changed line endings"?

> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> index 04d7b32e4e..b2cbcc818f 100644
> --- a/xdiff/xutils.c
> +++ b/xdiff/xutils.c
> @@ -156,6 +156,24 @@ 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)
> +{
> +	int complete = s && l[s-1] == '\n';
> +
> +	if (complete)
> +		s--;
> +	if (s == i)
> +		return 1;

What is the role of `s`, what of `i`? Maybe `length` and `current_offset`?

> +	/* do not ignore CR at the end of an incomplete line */
> +	if (complete && s == i + 1 && l[i] == '\r')
> +		return 1;

This made me scratch my head: too many negations. The comment may better
read "ignore CR only at the end of a complete line".

And now I understand even less why `1` is returned if `s == i`? Is this
not an empty line (complete or incomplete) *without* a CR?

> @@ -204,6 +223,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));

There are extra parentheses around the `return` expression.

To accommodate the tentative --hide-crlf-to-lf and --hide-lf-to-crlf
options that I suggested earlier, this would simply become something like
this:

	} else if (flags & (XDF_IGNORE_CRLF_TO_LF | XDF_IGNORE_LF_TO_CRLF)) {
		/* Early exit: length must be equal or differ by 1 */
		if (s1 - i1 != s2 - i2 &&
		    s1 - i1 != s2 + 1 - i2 && s1 + 1 - i1 != s2 - i2)
			return 0;

		/* Early exit: skip incomplete lines */
		if (!s1 || !s2 || l1[s1-1] != '\n' || l2[s2-1] != '\n')
			return 0;

		/* Find the first difference and see how the line ends */
		while (i1 < s1 && i2 < s2 && l1[i1] == l2[i2]) {
			i1++;
			i2++;
		}

		/* Lines must be identical or have the indicated EOL change */
		return ((i1 == s1 && i2 == s2) ||
			((flags & XDF_IGNORE_CRLF_TO_LF) &&
			 i1 + 2 == s1 && l1[i1] == '\r' && i2 + 1 == s2) ||
			((flags & XDF_IGNORE_LF_TO_CRLF) &&
			 i1 + 1 == s1 && i2 + 2 == s2 && l2[i2] == '\r');

Note: I do not even know whether the code in this function has to assume
that the lines can be byte-wise identical or not, I just erred on the side
of caution. I also do not know whether the return value 0 indicates a
mistmatch, I just assumed it did. I really wish that I could have reviewed
the real code, not a patch in a mail program that lacks the context.

Caveat emptor: my proposed code change has been written in the mail
program (if we had a more code-centric review process, you would have a PR
with a suggested alternative already, I am really sorry for the
inconvenience).

Ciao,
Dscho

Re: [PATCH v2 1/2] xdiff: reassign xpparm_t.flags bits

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
>> index b090ad8eac..cbf5d8e166 100644
>> --- a/xdiff/xdiff.h
>> +++ b/xdiff/xdiff.h
>> @@ -27,22 +27,26 @@
>>  extern "C" {
>>  #endif /* #ifdef __cplusplus */
>>  
>> +/* xpparm_t.flags */
>> +#define XDF_NEED_MINIMAL (1 << 0)
>>  
>> -#define XDF_NEED_MINIMAL (1 << 1)
>
> This change makes me wonder whether the least significant bit was omitted
> on purpose originally. You probably looked at that? May be worth
> mentioning in the commit message.
>
>> -#define XDF_IGNORE_WHITESPACE (1 << 2)
>> -#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
>> -#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_WHITESPACE (1 << 1)
>> +#define XDF_IGNORE_WHITESPACE_CHANGE (1 << 2)
>> +#define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 3)
>> +#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE | \
>> +			      XDF_IGNORE_WHITESPACE_CHANGE | \
>> +			      XDF_IGNORE_WHITESPACE_AT_EOL)
>>  
>> -#define XDF_PATIENCE_DIFF (1 << 5)
>> -#define XDF_HISTOGRAM_DIFF (1 << 6)
>> +#define XDF_IGNORE_BLANK_LINES (1 << 7)
>> +
>> +#define XDF_PATIENCE_DIFF (1 << 14)
>> +#define XDF_HISTOGRAM_DIFF (1 << 15)
>>  #define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)
>>  #define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)
>>  
>> -#define XDF_IGNORE_BLANK_LINES (1 << 7)
>> -
>> -#define XDF_INDENT_HEURISTIC (1 << 8)
>> +#define XDF_INDENT_HEURISTIC (1 << 23)
>>  
>> +/* xdemitconf_t.flags */
>>  #define XDL_EMIT_FUNCNAMES (1 << 0)
>>  #define XDL_EMIT_FUNCCONTEXT (1 << 2)
>
> It is a pity that this diff is not easier to review, but it shows how much
> it was in need of cleaning up. Looks much nicer now.
>
> I wonder, however, what your guiding principle was in determining the
> gaps? I would have expected consecutive bits, except for the one gap to
> make room for the upcoming flag, of course.

There wasn't that deep a thought went into it, actually.  

I wanted to give room for each "groupsof bits" to grow, and gave
each group a byte.  Whitespace bits sits in the first byte (but
ignore-blank-lines work quite different so it grabs the bit from the
other end), fundamental choices of LCS algos is another group that
sits in the second byte, then hunk shifting and other heuristics as
another group.  I guess that the "need minimal" thing could have
been thrown into the last group, but somehow I didn't think it would
grow that much, so I left it at the lowest place.

Re: [PATCH v2 0/2] Teach "diff" to ignore only CR at EOL

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Good. I was wishing for such a feature in the past.
>
> However, the short and sweet `-b` or `-w` switches are really, really
> nice. `--ignore-cr-at-eol` is just very cumbersome to type out. So I think
> you will want to add this patch to your patch series:

Yeah, I should probably have added that to 2/2 from the beginning.

> -- snipsnap --
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 0e16f017a41..b7a45e8df29 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1400,7 +1400,7 @@ __git_diff_common_options="--stat --numstat --shortstat --summary
>  			--patch-with-stat --name-only --name-status --color
>  			--no-color --color-words --no-renames --check
>  			--full-index --binary --abbrev --diff-filter=
> -			--find-copies-harder
> +			--find-copies-harder --ignore-cr-at-eol
>  			--text --ignore-space-at-eol --ignore-space-change
>  			--ignore-all-space --ignore-blank-lines --exit-code
>  			--quiet --ext-diff --no-ext-diff

Re: [PATCH v2 0/2] Teach "diff" to ignore only CR at EOL

[Stefan Beller]

On Tue, Nov 7, 2017 at 7:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> Good. I was wishing for such a feature in the past.
>>
>> However, the short and sweet `-b` or `-w` switches are really, really
>> nice. `--ignore-cr-at-eol` is just very cumbersome to type out. So I think
>> you will want to add this patch to your patch series:
>
> Yeah, I should probably have added that to 2/2 from the beginning.
>
>> -- snipsnap --
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 0e16f017a41..b7a45e8df29 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -1400,7 +1400,7 @@ __git_diff_common_options="--stat --numstat --shortstat --summary
>>                       --patch-with-stat --name-only --name-status --color
>>                       --no-color --color-words --no-renames --check
>>                       --full-index --binary --abbrev --diff-filter=
>> -                     --find-copies-harder
>> +                     --find-copies-harder --ignore-cr-at-eol
>>                       --text --ignore-space-at-eol --ignore-space-change
>>                       --ignore-all-space --ignore-blank-lines --exit-code
>>                       --quiet --ext-diff --no-ext-diff

That series, with this squash, looks good to me.

Re: [PATCH v2 2/2] diff: --ignore-cr-at-eol

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Tue, 7 Nov 2017, Junio C Hamano wrote:
>
>> A new option --ignore-cr-at-eol tells the diff machinery to treat a
>> carriage-return at the end of a (complete) line as if it does not
>> exist.
>> 
>> This would make it easier to review a change whose only effect is to
>> turn line endings from CRLF to LF or the other way around.
>
> If the goal is to make CR/LF -> LF conversions easier to review (or for
> that matter, LF -> CR/LF), then this option may not be *completely*
> satisfactory, as it would hide mixed changes (i.e. where some lines are
> converted from CR/LF to LF and others are converted in the other direction
> *in the same patch*).

You are 100% right.

This feature is not about helping to review a patch that wanted to
do CRLF-to-LF (or the other way around) conversion at all.  Just
like the --ignore-space-at-eol is not a feature to make sure that
the only thing you did was to remove trailing whitespaces---it will
also ignore lines you added trailing whitespaces as irrelevant and
uninteresting.

In general, selling these "--ignore-*" whitespace options as a tool
for such a verification is incorrect.

These "--ignore-*" whitespace options are to help reviewing _other_
changes without getting distracted by the class of changes these
options represent.  I guess I may have to update the log message (I
do not think I wrote anything like that in the documentation update).

Thanks for pointing it out.

Re: [PATCH v2 2/2] diff: --ignore-cr-at-eol

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> These "--ignore-*" whitespace options are to help reviewing _other_
> changes without getting distracted by the class of changes these
> options represent.  I guess I may have to update the log message (I
> do not think I wrote anything like that in the documentation update).

Here is what I just did with "commit --amend" (there is no change
in the contents of the patch).

    diff: --ignore-cr-at-eol
    
    A new option --ignore-cr-at-eol tells the diff machinery to treat a
    carriage-return at the end of a (complete) line as if it does not
    exist.
    
    Just like other "--ignore-*" options to ignore various kinds of
    whitespace differences, this will help reviewing the real changes
    you made without getting distracted by spurious CRLF<->LF conversion
    made by your editor program.
    
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH v2 2/2] diff: --ignore-cr-at-eol

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

I notice that I left a few things unanswered even after giving
answers to the most important part (i.e. "what is this for was
sold incorrectly").  Here are the leftover bits.

>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
>> index 89cc0f48de..aa2c0ff74d 100644
>> --- a/Documentation/diff-options.txt
>> +++ b/Documentation/diff-options.txt
>> @@ -519,6 +519,9 @@ endif::git-format-patch[]
>>  --text::
>>  	Treat all files as text.
>>  
>> +--ignore-cr-at-eol::
>> +	Ignore carrige-return at the end of line when doing a comparison.
>
> I am not a native speaker, either, yet I have the impression that "do a
> comparison" may be more colloquial than not. Also, it is a carriage-return
> (as in Sinatra's famous song about Love and Marriage) not a carrige-return.
>
> How about "Hide changed line endings"?

That felt like a good suggestion when I saw your reaction,
especially with only the parts visible in the patch and its context,
but after reviewing descriptions of other --ignore-* options, I no
longer think so.  The existing description of "--ignore-*" matches
manpages of GNU diff and they do not say "hide", either.

>> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
>> index 04d7b32e4e..b2cbcc818f 100644
>> --- a/xdiff/xutils.c
>> +++ b/xdiff/xutils.c
>> @@ -156,6 +156,24 @@ 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)
>> +{
>> +	int complete = s && l[s-1] == '\n';
>> +
>> +	if (complete)
>> +		s--;
>> +	if (s == i)
>> +		return 1;
>
> What is the role of `s`, what of `i`? Maybe `length` and `current_offset`?

I'd agree with that sentiment if this file were not borrowed code
but our own, but after looking at xdiff/ code, I think the names of
these variables follow the convention used there better, which
consistently names the variable for lines they deal with l1, l2, etc.,
their sizes s1, s2, etc., and the indices into the line i1, i2, etc.

>> +	/* do not ignore CR at the end of an incomplete line */
>> +	if (complete && s == i + 1 && l[i] == '\r')
>> +		return 1;
>
> This made me scratch my head: too many negations. The comment may better
> read "ignore CR only at the end of a complete line".

Perhaps.  "incomplete line" is a term with a specific definition
(and I think by "complete line" you mean a line that is not an
incomplete line), so I do not see the above comment as having too
many negations, though.  If you feel strongly about it, you could
"fix" it with a follow-up patch.

>> @@ -204,6 +223,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));
>
> There are extra parentheses around the `return` expression.

Yes, everybody knows that return is not a function that needs a
parentheses around its parameter.  I would drop them if this
expression were not split into two lines, but because the expression
is split at &&, I think it reads better with the extra parens.  So
I'll leave them as-is.

Thanks.