Add a presubmit check for accidental checkins of files under a SVN modified d...

Add a presubmit check for accidental checkins of files under a SVN modified directory.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=23271

[Lei Zhang, 2009-07-14 08:04:36]

Don't let svn's recursion behavior cause anymore accidents.

[M-A Ruel, 2009-07-16 00:14:20]

http://codereview.chromium.org/155489/diff/6/1001
File presubmit_canned_checks.py (right):

http://codereview.chromium.org/155489/diff/6/1001#newcode98
Line 98: current_cl_files = input_api.GetModifiedFiles(all=False)
if input_api.change.scm != 'svn':
  return []
Well, that needs to be added. :)

http://codereview.chromium.org/155489/diff/6/1001#newcode99
Line 99: modified_files = input_api.GetModifiedFiles(all=True)
input_api.change.GetModifiedFiles(all=True)
I don't know, I feel like the function is redundant. Maybe it should be named
GetAllModifiedFiles() since I see no context to use it with all=False.

http://codereview.chromium.org/155489/diff/6/1002
File presubmit_support.py (right):

http://codereview.chromium.org/155489/diff/6/1002#newcode355
Line 355: def GetModifiedFiles(self, all=False):
I disagree, this definitely is SCM aware. I'm working on removing SCM-specific
commands from InputApi towards Change. So I'd rather add this function to
SvnChange and GitChange. While at it, I'd add a self.scm. I did that in
SvnAffectedFile but that was an error, it was before the SvnChange class
existed.

http://codereview.chromium.org/155489/diff/6/1003
File tests/presubmit_unittest.py (right):

http://codereview.chromium.org/155489/diff/6/1003#newcode1016
Line 1016: 'CheckSVNModifiedDirectories',
I'd rather have a real (simple) unit test.

[M-A Ruel, 2009-08-03 14:42:06]

ping

[Lei Zhang, 2009-08-03 19:08:33]

I put this on the back burner while you were on vacation. I'll definitely send
more patch sets this week.

[Lei Zhang, 2009-08-07 01:53:03]

I still need to write the unit test, but here's one iteration to address the scm
comments.

http://codereview.chromium.org/155489/diff/6/1001
File presubmit_canned_checks.py (right):

http://codereview.chromium.org/155489/diff/6/1001#newcode98
Line 98: current_cl_files = input_api.GetModifiedFiles(all=False)
On 2009/07/16 00:14:20, Marc-Antoine Ruel wrote:
> if input_api.change.scm != 'svn':
>   return []
> Well, that needs to be added. :)

Done.

http://codereview.chromium.org/155489/diff/6/1001#newcode99
Line 99: modified_files = input_api.GetModifiedFiles(all=True)
On 2009/07/16 00:14:20, Marc-Antoine Ruel wrote:
> input_api.change.GetModifiedFiles(all=True)
> I don't know, I feel like the function is redundant. Maybe it should be named
> GetAllModifiedFiles() since I see no context to use it with all=False.

Umm, I used all=False in line 98. I separated GetModifiedFiles() into
GetModifiedFiles() and GetAllModifiedFiles().

http://codereview.chromium.org/155489/diff/6/1002
File presubmit_support.py (right):

http://codereview.chromium.org/155489/diff/6/1002#newcode355
Line 355: def GetModifiedFiles(self, all=False):
On 2009/07/16 00:14:20, Marc-Antoine Ruel wrote:
> I disagree, this definitely is SCM aware. I'm working on removing SCM-specific
> commands from InputApi towards Change. So I'd rather add this function to
> SvnChange and GitChange. While at it, I'd add a self.scm. I did that in
> SvnAffectedFile but that was an error, it was before the SvnChange class
> existed.

Ok, done. I removed the scm member from AffectedFile and its subclasses.

http://codereview.chromium.org/155489/diff/6/1003
File tests/presubmit_unittest.py (right):

http://codereview.chromium.org/155489/diff/6/1003#newcode1016
Line 1016: 'CheckSVNModifiedDirectories',
On 2009/07/16 00:14:20, Marc-Antoine Ruel wrote:
> I'd rather have a real (simple) unit test.

Ok, I just want to hammer out all the other details first.

[M-A Ruel, 2009-08-11 15:03:54]

lgtm

ping me next time when I forget.

http://codereview.chromium.org/155489/diff/4001/5001
File presubmit_canned_checks.py (right):

http://codereview.chromium.org/155489/diff/4001/5001#newcode118
Line 118: error_type = output_api.PresubmitError
I'd say a warning?

[Lei Zhang, 2009-08-12 22:40:11]

Ok, I added a unit test.

http://codereview.chromium.org/155489/diff/4001/5001
File presubmit_canned_checks.py (right):

http://codereview.chromium.org/155489/diff/4001/5001#newcode118
Line 118: error_type = output_api.PresubmitError
On 2009/08/11 15:03:54, Marc-Antoine Ruel wrote:
> I'd say a warning?

Sure.

[M-A Ruel, 2009-08-13 00:19:27]

lgtm!