Newline test

Thanks for the report, fixed in this commit: http://drupalcode.org/sandbox/klausi/1320008.git/commit/50a52c5

Just leave it as "fixed", maybe someone else also looks for this issue right now. Fixed issues will be closed automatically after 2 weeks with no activity.

No, no, this is all wrong. The script now complains if there are NO blank lines at the end of the file - exactly the OPPOSITE of what it should be doing. This is leading to dozens of sandbox projects being forced to add blank lines, and making it so contributions HAVE to violate our coding standards in order to be approved.

There are two coding standards that are involved here:
1) "Lines should have no trailing whitespace at the end."
2) "All text files should end in a single newline (\n)."

The first says there should be no blank lines following the last line of code. Specifically, this forbids additional lines containing nothing but line terminators. Open up any Drupal core file in any text editor and you will see this is a hard and fast rule. This is not just my interpretation - examine core yourself to verify that this is what was intended by the standard.

The second says that the last character in any file should be a single \n character. Again, this rule is followed exactly by Drupal core, and again the intention of the standard is clear from how it is applied in Drupal core.

Commit http://drupalcode.org/sandbox/klausi/1320008.git/commit/1e3f086 introduced this code:

for FILE in $TEXT_FILES; do
  BAD_LINES=`tail -n1 $FILE | grep -v "^$"`
  if [ $? = 0 ]; then
    BAD_FILES=("${BAD_FILES[@]}" "$FILE")
  fi
done

This is just wrong. This code checks only the last line of the file to see that it is empty. More exactly, it checks to see that there are no characters on the last line except, possibly, line terminators ($ doesn't require a line terminator, it will also match EOF). This violates the first coding standard that says there should be no trailing whitespace, and this check fails on every single Drupal core file.

Moreover, this code does NOT check to ensure that the last character of the file is \n, nor does it check for *multiple* lines of trailing whitespace.

I'm setting the priority to critical because this script is being used to evaluate new project applications, and the applicants are being instructed to do the wrong thing on the basis of this script. By teaching applicants the wrong standards, this script is currently doing harm!

I'm sorry, I don't understand the problem here. All files should end in a single newline ('\n') which is equal to an empty line at the end. Yes, my script does currently not check for multiple new lines at the end, but it bails out if there is no empty line at all.

Consider the following file with no new line at the end:

line1\n
line2\n
line3

My script takes the last line and checks if it is empty. No, it is not in this case, so it complains.

The correct file would look like:

line1\n
line2\n
line3\n

Last line is empty now, script does not complain.

This pattern is followed by Drupal core files, and I cannot find an error in my script. What am I missing?

You say that the correct file would look like:

line1\n
line2\n
line3\n

and that's true. However, the script will complain about this! You say that the "Last line is empty now" in this case, but it isn't - the last line contains line3\n, and that's why your script complains, because in this correct file the last line is not empty like the script expects. The script requires the file look like:

line1\n
line2\n
line3\n
\n

and that's wrong.

Instead of hypothetical examples, let's look at a real project. I posted a specific example in http://drupal.org/node/1341360#comment-5268804, where PAReview.sh complained when there was only one \n at the end of the file (as required by coding standards), and the applicant was requested to add a second \n to the end of each file. The repository currently contains files with only one \n (as required), as is evident by the hexdump output that I posted

Also, using Drupal core as an example, look at any file - index.php for example - and you will see that tail -n1 index.php | grep -v "^$" (which is the test you make in your script) shows that index.php fails your test. The test is wrong. I wrote this up in detail in http://drupal.org/node/1344988.

There are now over 50 project applications where, on the basis of the output of PAReview.sh, the applicant has been mistakenly instructed to add additional \n characters. I think this qualifies as critical, especially because trailing whitespace in module files is known to cause problems in Drupal (PHP header already sent errors).

Ah, now I see. I use Eclipse as editor and it displays '\n' as new line + carriage return, so I thought that the blank line is mandatory. All core files appear to me that way, however, if I open them in vim or gedit there is no additional blank line at the end.

Thank you very much for pointing this out in so much detail! Fixed in http://drupalcode.org/sandbox/klausi/1320008.git/commit/c0a8dd0

Now I need to fix up my Eclipse configuration which is severely broken. Sorry for all the inconvenience, I will clean up the mess in the project application issues.

Leaving this open until I can come up with a '\n' test for pareview.sh that actually works.

Note for Eclipse users on linux: The blank line that is displayed instead of '\n' is saved as '\n' only. So whenever you edit Drupal text files make sure that the blank line is there, it will be saved correctly. You can check that by opening the file in a different text editor, which will not show the blank line at the end.

Added a new rule to pareview.sh checking the last character to be \n, see http://drupalcode.org/sandbox/klausi/1320008.git/commit/89e3841

Sorry to spoil the party!

Looks like pareview_sh is not picking up two newlines at the end of the file.

line1\n
line2

results in fail - correct.

line1\n
line2\n

results in pass - correct.

line1\n
line2\n\n

results in pass - which is incorrect.

The new line check has been completely removed from pareview.sh, this is now handled by drupalcs.