chore(git-node): avoid dealing with patch files for landing #486
Conversation
Codecov Report
@@ Coverage Diff @@
## master #486 +/- ##
=======================================
Coverage 83.13% 83.13%
=======================================
Files 34 34
Lines 1696 1696
=======================================
Hits 1410 1410
Misses 286 286 Continue to review full report at Codecov.
|
|
Thank you! I always struggled to understand why we used patches instead of fetching the PR commits.
I think most of our landing process is untested and doesn't have the testing infrastructure it needs in place, so relying on manual tests is the way to go for now. |
lib/landing_session.js
Outdated
| 'fetch', `https://github.com/${owner}/${repo}.git`, | ||
| `refs/pull/${prid}/merge`]); | ||
| const [base, head] = (await runAsync('git', [ | ||
| 'rev-parse', 'FETCH_HEAD^1', 'FETCH_HEAD^2'], { captureStdout: true })) |
There was a problem hiding this comment.
^1, ^2, etc. in git means "prevous commit", "two commits before this commit", etc. Does this have some special meaning in this case due to us fetching the /merge ref (like ^1 gets the head of the merge commit, ^2 gets the base of the merge commit)? If that's the case it might be worth adding a comment explaining what's happening.
There was a problem hiding this comment.
like ^1 gets the head of the merge commit, ^2 gets the base of the merge commit
Yes, except it’s the other way around :) I’ve added a comment.
|
This will conflict with #473 |
Since I’ll not be involved in Node.js as much in the future, just consider this something that bugged me for quite a while and that I wanted to provide a fix for, but that I’ll probably not be involved in merging long-term. If you want to make changes, feel free to also just push to this branch :) |
|
Also, I'm fine with landing this first or helping merge if my PR lands first since my PR is much simpler anyway and I always find git-am very confusing when there are conflicts (like what do I change to fix them? 😄 ). |
|
Let's land the autorebase first as it should allow the commit-queue to land some PRs it can't today. I'm happy to rebase this and fix the conflicts after we land the autorebase. |
|
@addaleax I rebased and fixed the conflicts locally, let me know if I can force-push. |
|
@mmarchini You can generally do whatever you think is best on any of my branches :) |
|
Force pushed to fix conflicts :) |
|
This can be merged, right? :) |
|
Yeah, I just wanted to make a release first (but I don't have npm publish rights, so I'm waiting for someone else to do it). |
|
Will try to rebase and land this weekend :) |
Working with patch files is, on the one hand, unclean because it requires manual parsing to figure out e.g. the commit message subjects, and on the other hand, poses actual problems when dealing with binary files. Clean the code up by fetching the original commits themselves and applying those commits directly. This also resolves some TODO comments, e.g. we can now check whether the commits that are to be applied match the ones reported through the GitHub API.
|
Resolved another (trivial) conflict :) |
Working with patch files is, on the one hand, unclean because it
requires manual parsing to figure out e.g. the commit message subjects,
and on the other hand, poses actual problems when dealing with binary
files.
Clean the code up by fetching the original commits themselves and
applying those commits directly.
This also resolves some TODO comments, e.g. we can now check whether
the commits that are to be applied match the ones reported through
the GitHub API.
I’m not sure what kind of testing this patch needs? I’ve tried it out manually on one PR so far, but there doesn’t appear to be anything automated in place here.