Fix changed file detection on travis when the PR commits predate master commits. #3129
Conversation
|
This can be labelled with both |
|
@ammaraskar Would you mind rebasing this? |
…er commits. Travis when merging changes from a pull request onto the target branch does not perform a rebase, instead it does a simple merge which causes the PR commits to retain their commit dates. This means that the commit log can potentially look like: PR merge <-- HEAD normal master commit <- master more commits from normal workflow PR commit 1 another master commit PR commit 2 Performing a git diff from PR commit 2 to master will accidentally include files that should not be there. Closes python/core-workflow#14
ammaraskar
force-pushed
the
travis_fix
branch
from
35e176e to
df65aff
Compare
|
@zware done |
There was a problem hiding this comment.
Sorry for my very late response. This looks good to me and I agree with Ammar's analysis at python/core-workflow#14 (comment). I just left two minor comments.
.travis.yml
Outdated
| files_changed=$(git diff --name-only HEAD $(git merge-base HEAD $TRAVIS_BRANCH)) | ||
| fi | ||
|
|
||
| echo "files_changed: " |
There was a problem hiding this comment.
Can you add a comment saying that this is for debugging purposes and rename "files_changed:" to "Files changed:" to follow the current style?
.travis.yml
Outdated
| # Pull requests are slightly complicated because merging the PR commit without | ||
| # rebasing causes it to retain its old commit date. Meaning in history if any | ||
| # commits have been made on master that post-date it, they will be accidentally | ||
| # included in the diff if we use the simple TRAVIS_COMMIT_RANGE method |
There was a problem hiding this comment.
Can we just say "if we use TRAVIS_COMMIT_RANGE" or "if we use the TRAVIS_COMMIT_RANGE environment variable" to make it clearer? Perhaps it's just me but I got confused by the "method" part of that sentence :)
(Also, style nit: Can you finish the sentence with a full stop?)
|
Fixed up the comment style. |
|
everything looks good to me too, also tested the regex, this should be good to go. |
|
Thanks @ammaraskar for the PR, and @berkerpeksag for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6. |
|
Thanks! |
Travis when merging changes from a pull request onto the target branch does not perform a rebase, instead it does a simple merge which causes the PR commits to retain their commit dates. This means that the commit log can potentially look like: PR merge <-- HEAD normal master commit <- master more commits from normal workflow PR commit 1 another master commit PR commit 2 Performing a git diff from PR commit 2 to master will accidentally include files that should not be there. Closes python/core-workflowGH-14 (cherry picked from commit b2ec361)
|
GH-5365 is a backport of this pull request to the 3.6 branch. |
Travis when merging changes from a pull request onto the target branch does not perform a rebase, instead it does a simple merge which causes the PR commits to retain their commit dates. This means that the commit log can potentially look like: PR merge <-- HEAD normal master commit <- master more commits from normal workflow PR commit 1 another master commit PR commit 2 Performing a git diff from PR commit 2 to master will accidentally include files that should not be there. Closes python/core-workflow#14 (cherry picked from commit b2ec361)
See python/core-workflow#14 for my investigation into this.
I got the command for the pull request file detection from http://eng.localytics.com/best-practices-and-common-mistakes-with-travis-ci/ though there's is hardcoded to only work for master
additionally I found the exact same command here: https://stackoverflow.com/a/41159710
and here: https://stackoverflow.com/questions/25071579/list-all-files-changed-in-a-pull-request-in-git-github
I've also made the script print out what files changed so this is easier to debug if it becomes an issue again in the future.