Add a presubmit check to check that a certain set of Android specific files only have deletions.

Add a presubmit check to check that a certain set of Android specific files only have deletions.

We want certain Android specific files, like findbugs_known_bugs.txt, to only have deletions (not even modifications to existing lines). The presubmit check added in this CL checks that a certain list of files only have deletions.


BUG=165608
NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177241

[Siva Chandra, 2012-12-27 23:54:14]

This CL adds a presubmit check which checks that a listed set of files only have
deletions. They cannot have additions or modifications to existing lines.

The marked bug actually requests to check that a particular file doesn't "grow".
This CL adds that particular file to the listed files which will be checked by
this new presubmit check.

[Siva Chandra, 2013-01-09 00:25:32]

PING

[cmp, 2013-01-09 00:27:04]

Would like Isaac to review this, I'm here if there are further questions,
though.

[Isaac (away), 2013-01-09 03:55:03]

This looks good to me, but lets move to build/android/PRESUBMIT.py (which was
added a couple minutes ago for a different patch).

+M-A for a second look.

[Isaac (away), 2013-01-09 05:40:10]

https://chromiumcodereview.appspot.com/11583028/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/11583028/diff/1/PRESUBMIT.py#newcode197
PRESUBMIT.py:197: 'build/android/findbugs_filter/findbugs_known_bugs.txt'
if you keep this, you need a trailing comma on 197 or _DELETIONS_ONLY_FILES will
be a string, not a tuple.

https://chromiumcodereview.appspot.com/11583028/diff/1/PRESUBMIT.py#newcode641
PRESUBMIT.py:641: if f.LocalPath() in _DELETIONS_ONLY_FILES:
how about just:

if f.LocalPath() = 'build/android/...'

I don't think we're going to need this function for other files.

[Siva Chandra, 2013-01-11 01:24:04]

https://codereview.chromium.org/11583028/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11583028/diff/1/PRESUBMIT.py#newcode197
PRESUBMIT.py:197: 'build/android/findbugs_filter/findbugs_known_bugs.txt'
On 2013/01/09 05:40:10, Isaac wrote:
> if you keep this, you need a trailing comma on 197 or _DELETIONS_ONLY_FILES
will
> be a string, not a tuple.

Done.

https://codereview.chromium.org/11583028/diff/1/PRESUBMIT.py#newcode641
PRESUBMIT.py:641: if f.LocalPath() in _DELETIONS_ONLY_FILES:
On 2013/01/09 05:40:10, Isaac wrote:
> how about just:
> 
> if f.LocalPath() = 'build/android/...'
> 
> I don't think we're going to need this function for other files.

Unless you feel generality is bad here, I would prefer to keep it general. In an
other comment, you suggested that I move these changes to build/android. I have
same concern about generality for that as well.

[Isaac (away), 2013-01-12 02:42:30]

Looks good to me, but I don't have OWNERS here.  Let's wait for M-A's review.

https://codereview.chromium.org/11583028/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11583028/diff/1/PRESUBMIT.py#newcode637
PRESUBMIT.py:637: """Check that a certain listed files only have deletions.
nit: end triple quote on line 637 (delete line 638).

https://codereview.chromium.org/11583028/diff/1/PRESUBMIT.py#newcode641
PRESUBMIT.py:641: if f.LocalPath() in _DELETIONS_ONLY_FILES:
On 2013/01/11 01:24:04, Siva Chandra wrote:
> On 2013/01/09 05:40:10, Isaac wrote:
> > how about just:
> > 
> > if f.LocalPath() = 'build/android/...'
> > 
> > I don't think we're going to need this function for other files.
> 
> Unless you feel generality is bad here, I would prefer to keep it general. In
an
> other comment, you suggested that I move these changes to build/android. I
have
> same concern about generality for that as well.

SGTM.  It's fine to keep the DELETION_ONLY_FILES as a list, but the style in
this file is to define the list in the narrowest possible scope.  i.e. if the
list is only used in this function, define the list inside the function.

[M-A Ruel, 2013-01-12 02:53:04]

On 2013/01/11 01:24:04, Siva Chandra wrote:
> Unless you feel generality is bad here, I would prefer to keep it general. In
an
> other comment, you suggested that I move these changes to build/android. I
have
> same concern about generality for that as well.

I think this is a one-off and belongs
src/build/android/findbugs_filter/PRESUBMIT.py. src/PRESUBMIT.py is already
large enough ,no need to add more generality to it unless needed. Please move
accordingly.

Thanks

[Siva Chandra, 2013-01-15 01:26:53]

Moved the file per suggestion. PTAL

[M-A Ruel, 2013-01-15 02:16:23]

lgtm

https://codereview.chromium.org/11583028/diff/13001/build/android/findbugs_fi...
File build/android/findbugs_filter/PRESUBMIT.py (right):

https://codereview.chromium.org/11583028/diff/13001/build/android/findbugs_fi...
build/android/findbugs_filter/PRESUBMIT.py:7:
'build/android/findbugs_filter/findbugs_known_bugs.txt',
I thought the path was corrected to you should use 'findbugs_known_bugs.txt' but
maybe it's a bug in presubmit_support.py.

[Isaac (use chromium), 2013-01-15 02:36:40]

lets put this in the build/android/PRESUBMIT.py file.

[Siva Chandra, 2013-01-16 01:06:46]

PTAL, moved per ilevy's last suggestion.

[Isaac (use chromium), 2013-01-16 01:25:34]

lgtm

[commit-bot: I haz the power, 2013-01-16 01:36:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sivachandra@chromium.org/11583028/18001

[Isaac (away), 2013-01-16 07:16:40]

Sorry for the extra nit: This needs a better title and description before it can
be committed.  This is a general chromium repo so any change you make that is
specific to android needs to be labeled as such.

I've unchecked the CQ box.

[Siva Chandra, 2013-01-16 20:09:56]

PTAL

[commit-bot: I haz the power, 2013-01-16 20:16:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sivachandra@chromium.org/11583028/18001

[commit-bot: I haz the power, 2013-01-16 22:39:24]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-01-16 22:45:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sivachandra@chromium.org/11583028/18001

[commit-bot: I haz the power, 2013-01-16 22:52:23]

Change committed as 177241