Re: [PATCH v4 16/21] range-diff --dual-color: fix bogus white-space warning

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Junio C Hamano <gitster@xxxxxxxxx> writes:

> It is pleasing to see that with a surprisingly clean and small
> change like this we can exempt the initial space byte from
> SP-before-HT check and from Indent-with-non-tab at the same time.
>
> Very nice.
>
> One reason why a surprisingly small special case is required is
> perhaps because we are blessed with the original code being clean
> [*1*], and the fact that a line[0] that is not ' ' will not trigger
> any indentation related whitespace errors without this special case,
> I guess.

Having said good things about the patch, I unfortunately realized
that we weren't that lucky.  As we do want to see whitespace errors
on lines with line[0] != ' ' to be flagged.  So "... will not
trigger" in the above is not a blessing, but something that further
needs to be fixed.  The special case should also be made for line[0]
that is '+' and possibly '-' (and I also suspect that the changes in
this patch may mostly be reusable with little tweaks if any).

Imagine we start from this commit that "git show" shows us like so:

	 int main(int ac, char **av)
	 {
	 ________  printf("Hello");
	+________  putchar(',');
	+          putchar(' ');
	           printf("World\n");
	           return 0;
	 }

I've drawn a horizontal-tab as long underscore to clarify in the
above picture.  If you have "core.whitespace=indent-with-non-tab"
"git show" would paint the line that adds " " as violating (as it
types 10 SPs, when it could have been a tab and 2 SPs), but it does
not highlight the line with "World" or "return", which is sensible
as they are pre-existing violations.

Then imagine we did "git commit --amend" and "git show" would give
this instead:

	 int main(int ac, char **av)
	 {
	 ________  printf("Hello");
	+          putchar(',');
	+________  putchar(' ');
	           printf("World\n");
	           return 0;
	 }

That is, relative to the previous attempt, we stopped introducing
new indent-with-non-tab violation to the line that adds " ", but
added a new violation to the line that adds ",".

After such "git commit --amend", what do we want to see in the
output from "git range-diff @{1}..."?

My quick test of the current code does not show any whitespace
breakage for either versions.  I *think* what we want to actually
see is

 - just like WS_IGNORE_FIRST_SPACE logic shifted the column by
   incrementing written (and final condition) for the loop in your
   patch for line[0]==' ', detect the overlong run of SP correctly
   by ignoring the first column that is '+', and complaining that
   the new commit is typing 10 SPs before putchar(',').

 - Earlier I thought that lines with '-' in the outer diff should
   become exempt from whitespace-error highlighting, but I think
   that is a mistake.  If a line in diff-of-diff that begins with
   "-+" has whitespace violation (e.g "-+" followed by 10 SPs), that
   is "the old version of the patch used to introduce whitespace
   violation", which is a useful piece of information when we look
   at the corresponding line in the same diff-of-diff that begins
   with "++".  We could either say "and you cleaned that mistake up
   in your newer patch", or "and you still have that mistake in your
   newer patch" (another possibility is there is no whitespace error
   on a "-+" line, and the corresponding "++" line has one---"you
   got it right in the previous round, but you somehow made it
   worse").

Here is the reproduction of the sample data I based on the above
thought experiment on.

diff --git a/script.sh b/script.sh
new file mode 100755
index 0000000..0090661
--- /dev/null
+++ b/script.sh
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+git init
+git config core.whitespace indent-with-non-tab
+
+tr _ '\011' <<\EOF >hello.c
+int main(int ac, char **av)
+{
+_  printf("Hello");
+          printf("World\n");
+          return 0;
+}
+EOF
+
+git add hello.c && git commit -m initial
+
+tr _ '\011' <<\EOF >hello.c
+int main(int ac, char **av)
+{
+_  printf("Hello");
+          putchar(',');
+_  putchar(' ');
+          printf("World\n");
+          return 0;
+}
+EOF
+
+git commit -a -m second
+
+tr _ '\011' <<\EOF >hello.c
+int main(int ac, char **av)
+{
+_  printf("Hello");
+_  putchar(',');
+          putchar(' ');
+          printf("World\n");
+          return 0;
+}
+EOF
+
+git commit -a --amend -m third
+
+
+git show @{1}
+git show HEAD
+
+git range-diff ..@{1} @{1}..
+
-- 
2.18.0-232-gb7bd9486b0






[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux