[PATCH 1/2] Do not output whitespace on blank lines

[PATCH 1/2] Do not output whitespace on blank lines

[Dave Nicolson]

---
 diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index d3734d3..459b36a 100644
--- a/diff.c
+++ b/diff.c
@@ -471,7 +471,7 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
 		has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
 		if (has_trailing_carriage_return)
 			len--;
-		nofirst = 0;
+		nofirst = len == 0 && (char)first == ' ' ? 1 : 0;
 	}
 
 	if (len || !nofirst) {

--
https://github.com/git/git/pull/245

[PATCH 2/2] Fix tests

[Dave Nicolson]

---
 t/lib-diff-alternative.sh        | 4 ++--
 t/t4029-diff-trailing-space.sh   | 2 +-
 t/t4034-diff-words.sh            | 2 +-
 t/t4051-diff-function-context.sh | 6 +++---
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/lib-diff-alternative.sh b/t/lib-diff-alternative.sh
index 8b4dbf2..4a5d9d0 100644
--- a/t/lib-diff-alternative.sh
+++ b/t/lib-diff-alternative.sh
@@ -66,7 +66,7 @@ index 6faa5a3..e3af329 100644
 +++ b/file2
 @@ -1,26 +1,25 @@
  #include <stdio.h>
- 
+
 +int fib(int n)
 +{
 +    if(n > 2)
@@ -86,7 +86,7 @@ index 6faa5a3..e3af329 100644
          printf("%d\n", foo);
      }
  }
- 
+
 -int fact(int n)
 -{
 -    if(n > 1)
diff --git a/t/t4029-diff-trailing-space.sh b/t/t4029-diff-trailing-space.sh
index 3ccc237..751469c 100755
--- a/t/t4029-diff-trailing-space.sh
+++ b/t/t4029-diff-trailing-space.sh
@@ -27,7 +27,7 @@ test_expect_success \
      git config --bool diff.suppressBlankEmpty true &&
      git diff f > actual &&
      test_cmp exp actual &&
-     perl -i.bak -p -e "s/^\$/ /" exp &&
+     perl -i.bak -p -e "s/^\$//" exp &&
      git config --bool diff.suppressBlankEmpty false &&
      git diff f > actual &&
      test_cmp exp actual &&
diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index f2f55fc..7458e61 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -106,7 +106,7 @@ test_expect_success '--word-diff=porcelain' '
 		-h(4)
 		+h(4),hh[44]
 		~
-		 # significant space
+		# significant space
 		~
 		 a = b + c
 		~
diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 001d678..f786139 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -57,7 +57,7 @@ diff --git a/hello.c b/hello.c
  {
 -	/* Classic. */
  	printf("Hello world.\n");
- 
+
  	/* Success! */
  	return 0;
  }
@@ -73,12 +73,12 @@ diff --git a/hello.c b/hello.c
 --- a/hello.c
 +++ b/hello.c
 @@ -9,9 +9,8 @@ static int a(void)
- 
+
  static int hello_world(void)
  {
 -	/* Classic. */
  	printf("Hello world.\n");
- 
+
  	/* Success! */
  	return 0;
  }

--
https://github.com/git/git/pull/245

Re: [PATCH 1/2] Do not output whitespace on blank lines

[Thomas Gummerer]

On 05/29, Dave Nicolson wrote:
> ---

The commit message should describe why you think this is a good
change.  Without the commit message I don't see how this is a
improvement.  It is also missing your Sign-off (see
Documentation/SubmittingPatches section 5).

The test changes you sent in Patch 2/2 should be in this commit,
otherwise the git bisect is broken when it hits this patch, which is
something we want to avoid.

>  diff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/diff.c b/diff.c
> index d3734d3..459b36a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -471,7 +471,7 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
>  		has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
>  		if (has_trailing_carriage_return)
>  			len--;
> -		nofirst = 0;
> +		nofirst = len == 0 && (char)first == ' ' ? 1 : 0;
>  	}
>  
>  	if (len || !nofirst) {
> 
> --
> https://github.com/git/git/pull/245
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Thomas

Re: [PATCH 1/2] Do not output whitespace on blank lines

[René Scharfe]

Am 29.05.2016 um 17:36 schrieb Dave Nicolson:
> ---

git diff marks context lines (in unified diff format) with a preceding 
space character.  Your intent is to remove that marker for empty context 
lines, right?  Why?  How much smaller do diffs get by that (assuming 
output size reduction is one of the reasons)?

How compatible is it with patch, git apply and other programs that 
handle diffs?  Interestingly in that context, POSIX [*] allows it, 
saying: "It is implementation-defined whether an empty unaffected line 
is written as an empty line or a line containing a single <space> 
character."

Also: missing sign-off (see Documentation/SubmittingPatches).

>   diff.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/diff.c b/diff.c
> index d3734d3..459b36a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -471,7 +471,7 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
>   		has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
>   		if (has_trailing_carriage_return)
>   			len--;
> -		nofirst = 0;
> +		nofirst = len == 0 && (char)first == ' ' ? 1 : 0;

Why the cast from int to char?  And you also don't need the "? 1 : 0" part.

>   	}
>
>   	if (len || !nofirst) {
>
> --
> https://github.com/git/git/pull/245

Your second patch adjusts tests.  It would be better to combine the two 
in order to keep tests working with each commit.

René


[*] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html

Re: [PATCH 1/2] Do not output whitespace on blank lines

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 29.05.2016 um 17:36 schrieb Dave Nicolson:
>> ---
>
> git diff marks context lines (in unified diff format) with a preceding
> space character.  Your intent is to remove that marker for empty
> context lines, right?  Why?  How much smaller do diffs get by that
> (assuming output size reduction is one of the reasons)?
>
> How compatible is it with patch, git apply and other programs that
> handle diffs?  Interestingly in that context, POSIX [*] allows it,
> saying: "It is implementation-defined whether an empty unaffected line
> is written as an empty line or a line containing a single <space>
> character."

Indeed POSIX allows it, and I am aware of a push to GNU diff some
time ago that making this possible (I do not offhand know if it was
a move to make it default, though), which is why "git apply" does
tolerate an empty line in the common context that is not a single SP
on a line since b507b465 (git-apply: prepare for upcoming GNU diff
-u format change., 2006-10-19).

But that does not mean we should flip the default without any
justification like this patch does.

FWIW, GNU diffutils 3.3 seems _not_ to default to this behaviour,
and require you to say --suppress-blank-empty when asking for this
form of output.  The rationale for this option in their
documentation is:

    Some text editors and email agents routinely delete trailing
    blanks, so it can be a problem to deal with diff output files
    that contain them. You can avoid this problem with the
    --suppress-blank-empty option. It causes diff to omit trailing
    blanks at the end of output lines in normal, context, and
    unified format, unless the trailing blanks were already present
    in the input.

If you think about this, the above does not make _any_ sense as a
justification for that feature.  "Some editors tend to remove SPs
that you do want if they appear at the end of line, which is a
problem.  To _AVOID_ this problem, we give an option not to produce
the SP that may be eaten by such editors in the first place".

Seriously?  The reason why you do want them in the first place is
because eating them silently would cause trouble on the receiving
end, so it is very understandable to prepare the side that _uses_
diff output to accept such an output that is missing the SPs at the
end of the line.  That justification does not make sense for the
side that _generates_ output at all.

In any case, I am not strongly opposed to add --suppress-blank-empty
option to "git diff" family, even though I do not see much point in
it.  I'll not accept a change to make that the default, until "diff"
implementations everybody else uses uses such default and POSIX
starts saying "a single SP on a line to represent an empty shared
context is deprecated".