# [PATCH 2/2] Fix tests 5 messages from 2016-05-29 to 2016-05-30. Participants: Dave Nicolson, Thomas Gummerer, René Scharfe, Junio C Hamano. Thread: https://gitlist.dev/t/42478 ## Dave Nicolson, 2016-05-29 15:36 Subject: [PATCH 2/2] Fix tests Message-ID: <01020154fd28aaa8-7c49c7ef-eed2-4092-80c5-6f43986f174a-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020154fd28aaa8-7c49c7ef-eed2-4092-80c5-6f43986f174a-000000%40eu-west-1.amazonses.com In-Reply-To: <01020154fd28aa12-f536bf3e-58df-4d1f-b903-929b429954d3-000000@eu-west-1.amazonses.com> ``` --- 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 - + +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 ``` ## Dave Nicolson, 2016-05-29 15:36 Subject: [PATCH 1/2] Do not output whitespace on blank lines Message-ID: <01020154fd28aa12-f536bf3e-58df-4d1f-b903-929b429954d3-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020154fd28aa12-f536bf3e-58df-4d1f-b903-929b429954d3-000000%40eu-west-1.amazonses.com ``` --- 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 ``` ## Thomas Gummerer, 2016-05-29 17:06 Subject: Re: [PATCH 1/2] Do not output whitespace on blank lines Message-ID: <20160529170637.GC2286@hank> URL: https://gitlist.dev/e/20160529170637.GC2286%40hank In-Reply-To: <01020154fd28aa12-f536bf3e-58df-4d1f-b903-929b429954d3-000000@eu-west-1.amazonses.com> ``` 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 ``` ## René Scharfe, 2016-05-29 17:25 Subject: Re: [PATCH 1/2] Do not output whitespace on blank lines Message-ID: <574B25F4.8090409@web.de> URL: https://gitlist.dev/e/574B25F4.8090409%40web.de In-Reply-To: <01020154fd28aa12-f536bf3e-58df-4d1f-b903-929b429954d3-000000@eu-west-1.amazonses.com> ``` 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 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 ``` ## Junio C Hamano, 2016-05-30 00:30 Subject: Re: [PATCH 1/2] Do not output whitespace on blank lines Message-ID: URL: https://gitlist.dev/e/xmqqa8j8pet8.fsf%40gitster.mtv.corp.google.com In-Reply-To: <574B25F4.8090409@web.de> ``` René Scharfe 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 > 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". ```