Invert CVS keyword $Id$ warnings after git migration

Patch is rather trivial and works as fine. Doesn't need more reviews imo.

Applies nicely, but I didn't get a notice of Commits to the git repository do not require the CVS keyword $Id $ in each file.

I'd like to see this added to a release asap though so that people aren't adding $Id's. It's encouraging people to do the wrong thing.

Yeah I didn't get a notice about it until I selected 'minor (most)' option for displaying reviews.

I have a feeling we need to adjust the tests to account for this change.

I don't run the test framework, but I guess it must be something like this (contains original patch + inverted tests).

Patch applies nicely. I will try again to test the tests, but i'm getting fatal errors with other module tests that are being loaded first.

Dave, I'm not really sure if it's a big deal to remove the old ID's in normal mode. Minor's probably fine, but I'll let others decide that. Thanks for letting me know where it was being displayed.

Thanks for doing this!

Ok, finally got through to running the test and there were errors. I think it should be

   function testIdCommentLine() {
-    $this->assertCoderReviewPass('// $Id$' . "\n". $this->file_line);
-    $this->assertCoderReviewPass('// $Id$' . "\n" . $this->file_line);
-    $this->assertCoderReviewFail('/* $Id$ */' . "\n" . $this->file_line);
+    $this->assertCoderReviewFail('// $Id$' . "\n" . $this->file_line);
   }

This line I don't think is important. I think the goal was to check that someone didn't consolidate things for CVS's sake:

-    $this->assertCoderReviewFail('/* $Id$ */' . "\n" . $this->file_line);

It doesn't matter now and don't think we need to make any effort to search to remove it.

I'm not all that good at writing tests, but wanted to throw that out there to help get this into a release.

I was using coder to upgrade by d6 module to d7 (coder_upgrade didn't work, so ran review).
I got this (see attached), which said

* severity: normalLine -1: Include the CVS keyword $Id$ in each file. This should be in the format // $Id$ or // $Id$ (Drupal Docs)

And the link to Drupal docs links to: http://drupal.org/coding-standards, which doesn't say to have $Id$.

The patch in #5 has:

-    $this->assertCoderReviewFail('/* $Id$ */' . "\n" . $this->file_line);
+    $this->assertCoderReviewFail('// $Id$' . "\n" . $this->file_line);
+    $this->assertCoderReviewPass('/* $Id$ */' . "\n" . $this->file_line);

and I think it should just be

+    $this->assertCoderReviewFail('// $Id$' . "\n" . $this->file_line);

Leaving in the

$this->assertCoderReviewFail('/* $Id$ */' . "\n" . $this->file_line);

in there. it should still fail if it has ID with c style comments.

I'm not sure how to use git to get the 7.x dev, apply this patch, make my change and make a new patch... I'll think about it another time.

A variation of this patch was committed to both 6.x-2.x and 7.x versions.