[PATCH] git diff ignore-space options should ignore missing EOL at EOF differences

[PATCH] git diff ignore-space options should ignore missing EOL at EOF differences

[demerphq]

Fix bug where the diff library complains about missing EOL at EOF
mismatches even when run under the various ignore whitespace options.

Unfortunately the routine that needs the whitespace flags doesn't get
passed them directly, and is part of the API for the diff library, so
can't be modified.

Therefore it seems the best solution is to modify the xdemitcb_t struct
so that the flags are available inside the emit routine and then copy
them over at a suitable point in the call tree to ensure that they are
always the same.

This also adds a new flag to control this behavior independently of the
other whitespace options, and a new switch '--ignore-space-at-eof' to
git diff to expose the flag, as well as some slight clarifying language
to document the implications of the different whitespace options.

Also includes new tests to exercise each of the possible whitespace
options and verify that the EOF mismatch detection works as expected,
and is correctly suppressed by the relevant options.

Signed-off-by: Yves Orton <demerphq@gmail.com>
---
 Documentation/diff-options.txt         |    9 ++-
 contrib/completion/git-completion.bash |    1 +
 diff.c                                 |    2 +
 t/t4015-diff-whitespace.sh             |   79 ++++++++++++++++++++++++++++++++
 xdiff/xdiff.h                          |   10 ++++-
 xdiff/xdiffi.c                         |    3 +
 xdiff/xutils.c                         |    5 ++-
 7 files changed, 104 insertions(+), 5 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 813a7b1..6bc40e5 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -207,14 +207,17 @@ override configuration settings.
 --text::
 	Treat all files as text.

+--ignore-space-at-eof::
+	Ignore changes in whitespace at EOF.
+
 --ignore-space-at-eol::
-	Ignore changes in whitespace at EOL.
+	Ignore changes in whitespace at EOL (including EOF).

 -b::
 --ignore-space-change::
 	Ignore changes in amount of whitespace.  This ignores whitespace
-	at line end, and considers all other sequences of one or
-	more whitespace characters to be equivalent.
+	at line and file end, and considers all other sequences of one
+	or more whitespace characters to be equivalent.

 -w::
 --ignore-all-space::
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 412d2c0..5c6b942 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -801,6 +801,7 @@ __git_diff_common_options="--stat --numstat
--shortstat --summary
 			--full-index --binary --abbrev --diff-filter=
 			--find-copies-harder
 			--text --ignore-space-at-eol --ignore-space-change
+			--ignore-space-at-eof
 			--ignore-all-space --exit-code --quiet --ext-diff
 			--no-ext-diff
 			--no-prefix --src-prefix= --dst-prefix=
diff --git a/diff.c b/diff.c
index 006aa01..51a0c90 100644
--- a/diff.c
+++ b/diff.c
@@ -2572,6 +2572,8 @@ int diff_opt_parse(struct diff_options *options,
const char **av, int ac)
 		options->xdl_opts |= XDF_IGNORE_WHITESPACE_CHANGE;
 	else if (!strcmp(arg, "--ignore-space-at-eol"))
 		options->xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
+	else if (!strcmp(arg, "--ignore-space-at-eof"))
+		options->xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOF;
 	else if (!strcmp(arg, "--patience"))
 		options->xdl_opts |= XDF_PATIENCE_DIFF;

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 6d13da3..1ba851e 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -51,6 +51,85 @@ test_expect_success "Ray's example with -w"
'test_cmp expect out'
 git diff -b > out
 test_expect_success "Ray's example with -b" 'test_cmp expect out'

+# newline at eof tests (original has no newline)
+IGNORE_SPACE_AT_EOF_OPTS="-w -b --ignore-all-space
--ignore-space-change --ignore-space-at-eol --ignore-space-at-eof"
+tr -d "\n" << EOF > x
+this file has no newline at end of file
+EOF
+
+git update-index x
+
+cat << EOF > x
+this file does have a newline at the end of file
+EOF
+
+cat << EOF > expect
+diff --git a/x b/x
+index 7668968..95ce259 100644
+--- a/x
++++ b/x
+@@ -1 +1 @@
+-this file has no newline at end of file
++this file does have a newline at the end of file
+EOF
+for opt in $IGNORE_SPACE_AT_EOF_OPTS; do
+    git diff $opt > out
+    test_expect_success "diff $opt must not warn about trailing
whitespace (add newline case)" 'test_cmp expect out'
+done
+
+cat << EOF > expect
+diff --git a/x b/x
+index 7668968..95ce259 100644
+--- a/x
++++ b/x
+@@ -1 +1 @@
+-this file has no newline at end of file
+\ No newline at end of file
++this file does have a newline at the end of file
+EOF
+git diff > out
+test_expect_success 'standard diff must warn about trailing
whitespace (add newline case)' 'test_cmp expect out'
+
+# newline at eof tests (replacement has no newline)
+cat << EOF > x
+this file does have a newline at the end of file
+EOF
+
+git update-index x
+
+tr -d "\n" << EOF > x
+this file has no newline at end of file
+EOF
+
+cat << EOF > expect
+diff --git a/x b/x
+index 95ce259..7668968 100644
+--- a/x
++++ b/x
+@@ -1 +1 @@
+-this file does have a newline at the end of file
++this file has no newline at end of file
+EOF
+for opt in $IGNORE_SPACE_AT_EOF_OPTS; do
+    git diff $opt > out
+    test_expect_success "diff $opt must not warn about trailing
whitespace (remove newline case)" 'test_cmp expect out'
+done
+
+cat << EOF > expect
+diff --git a/x b/x
+index 95ce259..7668968 100644
+--- a/x
++++ b/x
+@@ -1 +1 @@
+-this file does have a newline at the end of file
++this file has no newline at end of file
+\ No newline at end of file
+EOF
+git diff > out
+test_expect_success 'plain diff must warn about trailing whitespace
(remove newline case)' 'test_cmp expect out'
+
+# end newline at eof tests
+
 tr 'Q' '\015' << EOF > x
 whitespace at beginning
 whitespace change
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 4da052a..00367af 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -33,7 +33,14 @@ extern "C" {
 #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
 #define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)
 #define XDF_PATIENCE_DIFF (1 << 5)
-#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE |
XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)
+#define XDF_IGNORE_WHITESPACE_AT_EOF (1 << 6)
+/*
+ * note this is deliberately a different define from XDF_WHITESPACE_FLAGS as
+ * there could be a new whitespace related flag which would not be part of
+ * the XDF_IGNORE_WHITESPACE_AT_EOF_ANY flags.
+ */
+#define XDF_IGNORE_WHITESPACE_AT_EOF_ANY
(XDF_IGNORE_WHITESPACE_AT_EOL | XDF_IGNORE_WHITESPACE_CHANGE |
XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_AT_EOF)
+#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE |
XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL |
XDF_IGNORE_WHITESPACE_AT_EOF)

 #define XDL_PATCH_NORMAL '-'
 #define XDL_PATCH_REVERSE '+'
@@ -79,6 +86,7 @@ typedef struct s_xpparam {
 typedef struct s_xdemitcb {
 	void *priv;
 	int (*outf)(void *, mmbuffer_t *, int);
+	long flags; /* whitespace flags */
 } xdemitcb_t;

 typedef long (*find_func_t)(const char *line, long line_len, char
*buffer, long buffer_size, void *priv);
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 3e97462..93dd284 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -556,6 +556,9 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2,
xpparam_t const *xpp,
 		return -1;
 	}
 	if (xscr) {
+		/* copy the whitespace flags over so they can be made available
+		 * to emit api code that only gets ecb */
+		ecb->flags = xpp->flags;
 		if (ef(&xe, xscr, ecb, xecfg) < 0) {

 			xdl_free_script(xscr);
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 04ad468..88591b7 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -52,7 +52,10 @@ int xdl_emit_diffrec(char const *rec, long size,
char const *pre, long psize,
 	mb[1].ptr = (char *) rec;
 	mb[1].size = size;
 	if (size > 0 && rec[size - 1] != '\n') {
-		mb[2].ptr = (char *) "\n\\ No newline at end of file\n";
+		if (ecb->flags & XDF_IGNORE_WHITESPACE_AT_EOF_ANY)
+			mb[2].ptr = (char *) "\n";
+		else
+			mb[2].ptr = (char *) "\n\\ No newline at end of file\n";
 		mb[2].size = strlen(mb[2].ptr);
 		i++;
 	}
-- 
1.6.0.3

Re: [PATCH] git diff ignore-space options should ignore missing EOL at EOF differences

[Johannes Schindelin]

Hi,

On Sun, 15 Feb 2009, demerphq wrote:

>  t/t4015-diff-whitespace.sh             |   79 ++++++++++++++++++++++++++++++++

Phew, you certainly want to make sure that it works...

> @@ -33,7 +33,14 @@ extern "C" {
>  #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
>  #define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)
>  #define XDF_PATIENCE_DIFF (1 << 5)
> -#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE |
> XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)
> +#define XDF_IGNORE_WHITESPACE_AT_EOF (1 << 6)
> +/*
> + * note this is deliberately a different define from XDF_WHITESPACE_FLAGS as
> + * there could be a new whitespace related flag which would not be part of
> + * the XDF_IGNORE_WHITESPACE_AT_EOF_ANY flags.
> + */
> +#define XDF_IGNORE_WHITESPACE_AT_EOF_ANY
> (XDF_IGNORE_WHITESPACE_AT_EOL | XDF_IGNORE_WHITESPACE_CHANGE |
> XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_AT_EOF)
> +#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE |
> XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL |
> XDF_IGNORE_WHITESPACE_AT_EOF)

As I told you on IRC, I do not follow that reasoning.  Rather, I would add 
the exceptions to xemit.c, when -- and if(!) -- they are needed.

Ciao,
Dscho

Re: [PATCH] git diff ignore-space options should ignore missing EOL at EOF differences

[demerphq]

2009/2/15 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Hi,
>
> On Sun, 15 Feb 2009, demerphq wrote:
>
>>  t/t4015-diff-whitespace.sh             |   79 ++++++++++++++++++++++++++++++++
>
> Phew, you certainly want to make sure that it works...

Yeah, Exhaustive testing is good. (When it doesn't take hours and
hours to run :-)

>
>> @@ -33,7 +33,14 @@ extern "C" {
>>  #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
>>  #define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)
>>  #define XDF_PATIENCE_DIFF (1 << 5)
>> -#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE |
>> XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)
>> +#define XDF_IGNORE_WHITESPACE_AT_EOF (1 << 6)
>> +/*
>> + * note this is deliberately a different define from XDF_WHITESPACE_FLAGS as
>> + * there could be a new whitespace related flag which would not be part of
>> + * the XDF_IGNORE_WHITESPACE_AT_EOF_ANY flags.
>> + */
>> +#define XDF_IGNORE_WHITESPACE_AT_EOF_ANY
>> (XDF_IGNORE_WHITESPACE_AT_EOL | XDF_IGNORE_WHITESPACE_CHANGE |
>> XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_AT_EOF)
>> +#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE |
>> XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL |
>> XDF_IGNORE_WHITESPACE_AT_EOF)
>
> As I told you on IRC, I do not follow that reasoning.  Rather, I would add
> the exceptions to xemit.c, when -- and if(!) -- they are needed.

Yeah I know you said that, and I *think* I followed all your advice
(much appreciated by the way) except for that point as I've been
nailed by inappropriate addition of flags to masks before, and well,
you know, once bitten twice shy, and patchers perogative and all that
eh? :-)

For instance what happens if someone adds XDF_IGNORE_WHITESPACE_AT_SOL
(start of line) or XDF_IGNORE_SPACES_WHERE_TABS_EXPECTED in the
future, and then adds it to XDF_IGNORE_WHITESPACE_FLAGS? And
personally such options seem quite reasonable to me. It just happens
to be coincidence that all of the currently existing flags also impact
this particular behaviour, IMO it wouldnt have been so strange to find
one that didn't.

And thanks again for your handholding on this patch. I hope the
pasting of it inline was correct. I'm not sure where I should have
said that it was a patch against the master branch without it also
appearing in the commit body. Should I haved attached the format-patch
file as well?

Also, on a related note I personally would have reorganized the flags
so that the ones relating to whitespace control are in a different bit
range than the ones that have to do with other things. The precedent
in the file appeared to not follow this approach as the patience flag
was "at the end", so i didnt modify this, and just stuck the new flag
also at the end. I think it would be better to do a lowbit moving up
and highbit moving down approach or something like that as otherwise
when new flags get added over time the different types of flags are
interspersed and it becomes a real mess to maintain and understand. In
fact, I dont really "get" the whitespace flags as currently
implemented. Why does "ignore all whitespace" get its own bit?
Shouldn't it just be a mask of all the other whitespace bits?

cheers,
Yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Re: [PATCH] git diff ignore-space options should ignore missing EOL at EOF differences

[Johannes Schindelin]

Hi,

On Sun, 15 Feb 2009, demerphq wrote:

> 2009/2/15 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> > Hi,
> >
> > On Sun, 15 Feb 2009, demerphq wrote:
> >
> >>  t/t4015-diff-whitespace.sh             |   79 ++++++++++++++++++++++++++++++++
> >
> > Phew, you certainly want to make sure that it works...
> 
> Yeah, Exhaustive testing is good. (When it doesn't take hours and
> hours to run :-)

You read my mind.

> >> @@ -33,7 +33,14 @@ extern "C" {
> >>  #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
> >>  #define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)
> >>  #define XDF_PATIENCE_DIFF (1 << 5)
> >> -#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE |
> >> XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)
> >> +#define XDF_IGNORE_WHITESPACE_AT_EOF (1 << 6)
> >> +/*
> >> + * note this is deliberately a different define from XDF_WHITESPACE_FLAGS as
> >> + * there could be a new whitespace related flag which would not be part of
> >> + * the XDF_IGNORE_WHITESPACE_AT_EOF_ANY flags.
> >> + */
> >> +#define XDF_IGNORE_WHITESPACE_AT_EOF_ANY
> >> (XDF_IGNORE_WHITESPACE_AT_EOL | XDF_IGNORE_WHITESPACE_CHANGE |
> >> XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_AT_EOF)
> >> +#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE |
> >> XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL |
> >> XDF_IGNORE_WHITESPACE_AT_EOF)
> >
> > As I told you on IRC, I do not follow that reasoning.  Rather, I would add
> > the exceptions to xemit.c, when -- and if(!) -- they are needed.
> 
> Yeah I know you said that, and I *think* I followed all your advice
> (much appreciated by the way) except for that point as I've been
> nailed by inappropriate addition of flags to masks before, and well,
> you know, once bitten twice shy, and patchers perogative and all that
> eh? :-)

I understand that, but IMHO it is overengineered.  I am not really 
convinced that ignore-whitespace-at-sol makes sense, either...

Ciao,
Dscho

Re: [PATCH] git diff ignore-space options should ignore missing EOL at EOF differences

[demerphq]

2009/2/15 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Hi,
>
> On Sun, 15 Feb 2009, demerphq wrote:
>
>> 2009/2/15 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>> > Hi,
>> >
>> > On Sun, 15 Feb 2009, demerphq wrote:
>> >
>> >>  t/t4015-diff-whitespace.sh             |   79 ++++++++++++++++++++++++++++++++
>> >
>> > Phew, you certainly want to make sure that it works...
>>
>> Yeah, Exhaustive testing is good. (When it doesn't take hours and
>> hours to run :-)
>
> You read my mind.
>
>> >> @@ -33,7 +33,14 @@ extern "C" {
>> >>  #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)
>> >>  #define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)
>> >>  #define XDF_PATIENCE_DIFF (1 << 5)
>> >> -#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE |
>> >> XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL)
>> >> +#define XDF_IGNORE_WHITESPACE_AT_EOF (1 << 6)
>> >> +/*
>> >> + * note this is deliberately a different define from XDF_WHITESPACE_FLAGS as
>> >> + * there could be a new whitespace related flag which would not be part of
>> >> + * the XDF_IGNORE_WHITESPACE_AT_EOF_ANY flags.
>> >> + */
>> >> +#define XDF_IGNORE_WHITESPACE_AT_EOF_ANY
>> >> (XDF_IGNORE_WHITESPACE_AT_EOL | XDF_IGNORE_WHITESPACE_CHANGE |
>> >> XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_AT_EOF)
>> >> +#define XDF_WHITESPACE_FLAGS (XDF_IGNORE_WHITESPACE |
>> >> XDF_IGNORE_WHITESPACE_CHANGE | XDF_IGNORE_WHITESPACE_AT_EOL |
>> >> XDF_IGNORE_WHITESPACE_AT_EOF)
>> >
>> > As I told you on IRC, I do not follow that reasoning.  Rather, I would add
>> > the exceptions to xemit.c, when -- and if(!) -- they are needed.
>>
>> Yeah I know you said that, and I *think* I followed all your advice
>> (much appreciated by the way) except for that point as I've been
>> nailed by inappropriate addition of flags to masks before, and well,
>> you know, once bitten twice shy, and patchers perogative and all that
>> eh? :-)
>
> I understand that, but IMHO it is overengineered.  I am not really
> convinced that ignore-whitespace-at-sol makes sense, either...

Well, if there is a consensus that it is overengineered to add a new
define that will prevent hard to detect future bugs, then ill change
the code. Although id feel more comfortable with hearing this from
Junio himself. But before I put together a new patch is there any
other feedback?

Yves

-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Re: [PATCH] git diff ignore-space options should ignore missing EOL at EOF differences

[Junio C Hamano]

demerphq <demerphq@gmail.com> writes:

> Fix bug where the diff library complains about missing EOL at EOF
> mismatches even when run under the various ignore whitespace options.

Hmm.  Perhaps I am not reading the problem description correctly, but this
is not certainly you are talking about, right?

    $ printf "abcde " >one
    $ git add one
    $ printf "abcde  " >one
    $ git diff -w one
    diff --git a/one b/one
    index c1401e2..9f6f8e6 100644
    $ echo "abcde  " >one
    $ git diff -w one
    index c1401e2..21ee909 100644
    $ exit