4 ms·

I will type up all the review comments first, then... If I expect or hope the contributor to become a long-term collaborator, I will submit the comments and wa

I will type up all the review comments first, then... If I expect or hope the contributor to become a long-term collaborator, I will submit the comments and wait for them to fix it. I'd rather they learn If I don't expect them to stay around, I'll make the changes, force-push to their branch keeping their authorship, submit the comments, and let them know what I did and link to the diff caused by just the force push. I'll then ask them if it's OK and let the PR sit for a few days or until they respond, then I'll merge it. --- The more of your expectations you can automate or at least document (e.g. in CONTRIBUTING.md) the easier it is.

newaccount74 4y ago
Why do you force push to their branch, instead of committing your fixes as separate commits later on?
- shepmaster 4y ago
  I strive to have every commit be meaningful/buildable/testable/etc. To that end, I use a lot of `git rebase -x` to massage the pull request into a reasonable sequence of commits that “tell a story”. Having a bunch of “fixing foo” commits is ugly to me and reduces my ability to bisect problems later on. Sometimes I will actually create the fixup commits and push them separately to communicate with the submitter, but then squash them into the appropriate places in history before merging. The only technical issue is when someone has signed their commits, as I obviously cannot re-sign them :-). In that case, I call it out to them and let them re-sign if they want before I merge.
saurik 4y ago
Every day you delay pushing a fix for whatever the pull request was accomplishing is a day you are punishing all the other users of your project, as well as robbing them of the benefits of the work done by the person who submitted the request. If I find an issue in your code and I submit a pull request or (more likely) file a detailed issue with associated patch, and I watch you try to block back onto me to edit it to your liking when I know it would take you less effort to just do that final work yourself, you are less likely to get me to care in the future to do the work to isolate an issue and potentially even likely to lose me as a downstream user of your code. I also definitely wouldn't want you force pushing a change with my name on it: if you are editing my code it isn't my code anymore... just commit the code as you. I am not playing some weird game where "credit" on a commit matters: I'm trying to get work done (as I'd hope we all are), and feel a need to be helpful to others instead of selfishly hoarding patches for myself. And if someone is playing such a game, it is probably better to discourage them of it rather than putting up with it.

newaccount74 4y ago

Why do you force push to their branch, instead of committing your fixes as separate commits later on?

shepmaster 4y ago
I strive to have every commit be meaningful/buildable/testable/etc. To that end, I use a lot of `git rebase -x` to massage the pull request into a reasonable sequence of commits that “tell a story”. Having a bunch of “fixing foo” commits is ugly to me and reduces my ability to bisect problems later on. Sometimes I will actually create the fixup commits and push them separately to communicate with the submitter, but then squash them into the appropriate places in history before merging. The only technical issue is when someone has signed their commits, as I obviously cannot re-sign them :-). In that case, I call it out to them and let them re-sign if they want before I merge.

saurik 4y ago

Every day you delay pushing a fix for whatever the pull request was accomplishing is a day you are punishing all the other users of your project, as well as robbing them of the benefits of the work done by the person who submitted the request. If I find an issue in your code and I submit a pull request or (more likely) file a detailed issue with associated patch, and I watch you try to block back onto me to edit it to your liking when I know it would take you less effort to just do that final work yourself, you are less likely to get me to care in the future to do the work to isolate an issue and potentially even likely to lose me as a downstream user of your code. I also definitely wouldn't want you force pushing a change with my name on it: if you are editing my code it isn't my code anymore... just commit the code as you. I am not playing some weird game where "credit" on a commit matters: I'm trying to get work done (as I'd hope we all are), and feel a need to be helpful to others instead of selfishly hoarding patches for myself. And if someone is playing such a game, it is probably better to discourage them of it rather than putting up with it.