Add docs on updating PRs using git #10997
Conversation
|
👍 This exactly what I need. Just a question: Often people review certain lines of code. How are those treated? Will the review then point to a nonexisting line after ammending? |
| Often, you'll want to update your contribution based on the feedback in the | ||
| pull request. While it's possible to simply push additional commits to your | ||
| feature branch, this will clutter the history. Not every step in the | ||
| development of a feature is worth to be recorded. In contrast, a clean |
There was a problem hiding this comment.
perhaps "not every step...is worth recording"
| pull request. While it's possible to simply push additional commits to your | ||
| feature branch, this will clutter the history. Not every step in the | ||
| development of a feature is worth to be recorded. In contrast, a clean | ||
| history with one or a few thematically separted commits helps to keep it |
|
|
||
| Both techniques rewrite the history of your feature branch. While generally | ||
| one has to be careful with rewriting the history, this is intended and | ||
| perfectly fine for updates to your feature branch. |
There was a problem hiding this comment.
perhaps: "perfectly fine for commits that are currently on your feature branch only".
a point that should be clear is that commits already in master shouldn't be touched.
| On your feature branch:: | ||
|
|
||
| # make your updates | ||
| git commit --all --amend --no-edit |
There was a problem hiding this comment.
Non-blocker: I'm a little nervous about recommending --all. I'd prefer to instruct people unfamiliar with git to add each of the files rather that possibly adding a bunch of unrelated files.
There was a problem hiding this comment.
Yes definitely agree. For better or worse I have a bunch of test files in my local dev repository and if I used -all I’d be in a lot of trouble.
|
|
||
| git reset --soft HEAD~3 | ||
| git commit | ||
| git push --force |
There was a problem hiding this comment.
does this achieve the same thing as:
git checkout feature-branch
git rebase -i HEAD~8 # opens text editor, check your gitconfig
# use the fixup option on all but the first line
# save and exit text editor (for vim that's ":wq")
git push origin feature-branch --force
There was a problem hiding this comment.
Yeah it does. But I think it requires one fewer editor opens. I somewhat prefer the rebase option.
There was a problem hiding this comment.
I think so, but you're never quite sure with git. I took this from https://stackoverflow.com/a/5201642/2726279
There was a problem hiding this comment.
In my experience rebasing is more difficult to understand than soft reset. That's why I chose this approach. I still feel like I cannot explain rebasing reasonably simple.
Any help with rebase and the "Rewriting commit history" section would be welcome.
|
@ImportanceOfBeingErnest when an individually reviewed line changes, the GitHub UI collapses the comment. You can click on a button labeled "show outdated comment" to see the history. Your PR from the pie docs has this: #10953 (review) |
|
You can see the outdated comment and the few lines of code segment. However, naturally the "View Changes" button leads nowhere as there is simply no commit associated anymore. This is inherent. We wanted to throw away the preliminary commits. |
f8dfcef to
75215c4
Compare
|
Ok, so this is in anyway desireable to do. There is a section down below at that page called "Rewriting commit history", should that not somehow unified with the new changes? A further note. I think the problems I had with previous PRs trying to squash was that there were other commits from other people in between those I wanted to merge. So when I did the commits shown in the editor were not just my last commits from my branch, but simply the last 4 commits to the matplotlib. E.g. So it wasn't clear what to do then. The explanation in the "Rewriting commit history" has the drawback that it isn't clear which of the mentionned commits would be mine. Also there is the word "last", for which I wasn't sure if it is meant temporarily (newest) or spacially (bottom-most?). Also Concerning ammend, is this still an option if you already have several commits (due to forgetting to ammend previously or due to not knowing about the existance of this option)? |
|
@ImportanceOfBeingEarnest that’s almost certainly because you did a merge commit before you attempted rebasing. I’ve done that by accident a few times and if that happens you better hope you have a backup. That eventuality would ideally be covered here as well. I agree that this should be homogenized with the rewriting history. Btw I’m not sure we get to edit any of this. I think it gets pulled from the gitwash repository. |
|
If you've accidentially merged, you can still reset --hard to your branch before the merge commit. The "Rewriting commit history" section is also part of development_workflow.rst. It's editable in principle, but I don't feel comfortable with the topic. @ImportanceOfBeingErnest Concerning amend when you already have multiple commits. You an change single ones but the number of commits will stay. How to amend the last one is explained above. To amend an earlier commit you have to edit and commit-amend as part of a (interactive) rebase. |
|
That But again, do we actually get to edit this, or is it just copied from https://github.com/matthew-brett/gitwash/tree/master/gitwash ? I think the changes need to be proposed there, but I'm fuzzy on whether that is really the case. |
|
This should be opened up against upstream gitwash or your changes will at somepoint be blown away and you will be sad. |
|
@timhoffm I'm closing this as this is the wrong repository for this. But I think the actual work is great! |
PR Summary
This PR adds documentation on how to update PRs using git by amending or squashing.
This is in the context of #8503.