Repository: palantir/policy-bot
Pull request #225 (2020-08-27 20:53:48)

Fix rename handling for changed files
Properly expand a rename into a deletion of the old file and an addition
of the new one so predicates trigger if they apply to either side of the
rename operation. Previously, only the new file (the destination) was
included as a changed file.

cc @AdamDeHovitz 


Patch:
diff --git a/pull/github.go b/pull/github.go
index e2c6aea..55f4da4 100644
--- a/pull/github.go
+++ b/pull/github.go
@@ -207,24 +207,34 @@ func (ghc *GitHubContext) ChangedFiles() ([]*File, error) {
 			opt.Page = res.NextPage
 		}
 
-		ghc.files = make([]*File, len(allFiles))
-		for i, f := range allFiles {
-			var status FileStatus
+		ghc.files = make([]*File, 0, len(allFiles))
+		for _, f := range allFiles {
+			status := FileModified
 			switch f.GetStatus() {
 			case "added":
 				status = FileAdded
 			case "deleted":
 				status = FileDeleted
-			case "modified":
-				status = FileModified
+			case "renamed":
+				// Break renames into components: the new file is added and we
+				// generate an extra entry for the old file that is deleted.
+				// Attribute all modifications to the new file to avoid double
+				// counting.
+				status = FileAdded
+				ghc.files = append(ghc.files, &File{
+					Filename:  f.GetPreviousFilename(),
+					Status:    FileDeleted,
+					Additions: 0,
+					Deletions: 0,
+				})
 			}
 
-			ghc.files[i] = &File{
+			ghc.files = append(ghc.files, &File{
 				Filename:  f.GetFilename(),
 				Status:    status,
 				Additions: f.GetAdditions(),
 				Deletions: f.GetDeletions(),
-			}
+			})
 		}
 	}
 	if len(ghc.files) >= MaxPullRequestFiles {


Test patch:
diff --git a/pull/github_test.go b/pull/github_test.go
index b0d8060..aa7c4cb 100644
--- a/pull/github_test.go
+++ b/pull/github_test.go
@@ -39,7 +39,7 @@ func TestChangedFiles(t *testing.T) {
 	files, err := ctx.ChangedFiles()
 	require.NoError(t, err)
 
-	require.Len(t, files, 3, "incorrect number of files")
+	require.Len(t, files, 5, "incorrect number of files")
 	assert.Equal(t, 2, filesRule.Count, "no http request was made")
 
 	assert.Equal(t, "path/foo.txt", files[0].Filename)
@@ -51,11 +51,21 @@ func TestChangedFiles(t *testing.T) {
 	assert.Equal(t, "README.md", files[2].Filename)
 	assert.Equal(t, FileModified, files[2].Status)
 
+	assert.Equal(t, "path/old.txt", files[3].Filename)
+	assert.Equal(t, FileDeleted, files[3].Status)
+	assert.Equal(t, 0, files[3].Additions)
+	assert.Equal(t, 0, files[3].Deletions)
+
+	assert.Equal(t, "path/new.txt", files[4].Filename)
+	assert.Equal(t, FileAdded, files[4].Status)
+	assert.Equal(t, 2, files[4].Additions)
+	assert.Equal(t, 4, files[4].Deletions)
+
 	// verify that the file list is cached
 	files, err = ctx.ChangedFiles()
 	require.NoError(t, err)
 
-	require.Len(t, files, 3, "incorrect number of files")
+	require.Len(t, files, 5, "incorrect number of files")
 	assert.Equal(t, 2, filesRule.Count, "cached files were not used")
 }
 
diff --git a/pull/testdata/responses/pull_files.yml b/pull/testdata/responses/pull_files.yml
index e543121..c9964eb 100644
--- a/pull/testdata/responses/pull_files.yml
+++ b/pull/testdata/responses/pull_files.yml
@@ -33,5 +33,13 @@
         "additions": 103,
         "deletions": 21,
         "changes": 124
+      },
+      {
+        "filename": "path/new.txt",
+        "status": "renamed",
+        "additions": 2,
+        "deletions": 4,
+        "changes": 6,
+        "previous_filename": "path/old.txt"
       }
     ]