Ask HN: Do you rewrite pull requests?

What about sending a patch to them?

Clean and extend it under their name.

If the code which is eventually merged is mostly the contributor's code, and you just had to do some small tweaks to get it up to the project's standard, then commit and merge it under their name.

If the changes are so major that the code cannot reasonably be considered theirs any more, then it would be dishonest to merge it under their name, and would not be doing them or anyone else a favor. In that case, commit and merge under your own name, but include a note in the commit message thanking them for their contribution.

When I was writing a book for O'Reilly about the GitHub API, I interviewed a bunch of people from GitHub. One of them told me a surprising story: he said that often they ask people to write the PR BEFORE writing any code at all. The PR should be created with zero commits. That way they can discuss the ideas, implementation plan, impact, without even thinking about the code.

After hearing this, I believe that a PR should be changed dozens of times even if it is a one line change of code. PRs can reflect a million changing things, and I often update a PR months after the code is done because new information has come in that makes the code change need more context.

More stories like this could help your cause, no?

For contributions by new authors I usually don't make them do a respin for something minor, for instance if I think a more explanatory comment would be helpful -- I just fix those up when I commit. It saves the author doing multiple rounds dealing with a process they may be unfamiliar with, and it's usually less work on my end. For an established submitter the balance is more to ask them to respin, because they ought to be learning the project rules and process; even there I often fix up nits like typos in comment when committing rather than requesting a respin.

I generally err on the side of retaining the authorship when I do this kind of thing (our project follows the kernel's de-facto standard of retaining the original git author but adding a note of extra changes made in square brackets after the author's signed-off-by and before mine as the committer, which is how we record who's responsible for what parts of a change like this). If you've literally completely written a fresh change to solve the same problem a different way then that should be your authorship with some kind of reported-by or other acknowledgement to the PR author, though.

Have you considered adding the original author as a co-author on the rewritten commit(s)? That could still add them to the contributor list

This applies to open-source projects with volunteer contributors, most of whom are undergrads.

If I know they won't be frustrated by a review process, I work with them to improve it. When we get to a point where I think they'll be frustrated if we continue (or if it's just actually good now), and also the code is working, I merge it. I then will not touch it for at least a week or two. At that point, the code is fair game for me to refactor.

(edit - I think it's pretty important that the original merge is only their code, so that they have a sense of ownership & accomplishment, especially when they're college students.)

Assuming I have a good relationship with the author, if I do significant refactors to their code, I'll send them the commit in which I did so and explain to them why I did what they did so they can learn for next time.

This approach has never really backfired on me, except that maybe sometimes I forget/become apathetic about doing the second part, so I have some not-so-great code in part 2. But, it's never (to me) been worth the frustration of a volunteer contributor; people contributing to the project is more important than anything else, because otherwise the project dies. One of my projects does have one mostly-abandoned large component that no one wants to touch because someone came in, wrote a ton of mediocre code, we didn't really want to do a huge code review, and then that person left anyway because they got busy irl and/or thought "good enough" and/or got frustrated with what little code review we did do, but we openly say "this is not actively supported use it at your own risk" and people seem happy enough about the situation. Hopefully someday someone takes it over again.

The `Co-authored-by` trailer is your friend here. Even if you completely rewrite their code, give them coauthor credit in the commits and they’ll get the satisfaction of seeing their face in the commit history without the hassle of actually addressing all your feedback. https://docs.github.com/en/pull-requests/committing-changes-...

There's a git commit key which keeps the original author. You can just use this key always. Git amend = git commit --amend, preserve author, but update time.

The most frustrating thing in a PR is getting comments, addressing those comments, and the getting more comments (on something unrelated) by the same person. Give all your feedback at once. Otherwise, the review process will go on forever.

It's not even about "badly written". Sometimes it's just really hard to get a patch into the shape the maintainer wants (tests, architecture, general feel, how it slots into a longer term vision that may not be clearly defined, but the maintainer knows it when they see it) and takes a lot of back and forth. I don't begrudge that, after all, it's their project.

But it would be nice if there's a way to say "look, it is what it is, and while I'm willing to make an effort, I'm not living and breathing this codebase, and I'm not planning to become a major contributor going forward, much as I like the project. Playing 20 questions is exhausting on both ends and if you'd rather just take it and completely recast it how you want, I honestly don't mind and won't take offense" without sounding like "here's some random code, IDK if it works really, good luck, lol".

I don't even care about the authorship, I'd rather have the fix than get credit for a moribund PR. Half the time I contribute pseudonymously anyway.

I wonder if there is a case to be made for a patch branch that gets all PRs. Then the maintainer can do any further tweaks needed before merging down to mainline.

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.

If the PR is technically correct, just violates coding style or smells a bit like of hacky trash, I merge it anyways. As long as it's not breaking the build, functionality, or bisectability, just go with it. You can always chase it with a cleanup commit of your own.

It's up to the PR's author to decide if they care enough to do better next time. By accepting their work and landing the change you've demonstrated an appreciation for their effort with open arms, and I feel this makes it more likely they will contribute in the future and care about doing better.

Maybe make a comment to that effect before merging as-is, creating the opportunity to clean it up themselves first.

If it's coming from a regular contributor I'm far less charitable, this approach I mostly reserve for first-time/infrequent contributors.

   Original-idea-from: a b <a.b@c.com>

Here's a relevant quote from Google's code review guidelines on speed https://google.github.io/eng-practices/review/reviewer/speed...

>When code reviews are slow, several things happen: ...

>Developers start to protest the code review process. If a reviewer only responds every few days, but requests major changes to the CL each time, that can be frustrating and difficult for developers. Often, this is expressed as complaints about how “strict” the reviewer is being. If the reviewer requests the same substantial changes (changes which really do improve code health), but responds quickly every time the developer makes an update, the complaints tend to disappear. Most complaints about the code review process are actually resolved by making the process faster.

I do this a lot on PRs, especially for small fixes. I’ll make sure to put a comment explaining my changes and link to the relevant doc about why the change was made.

It doesn’t make sense to ask contributors (especially new ones) to spend time going back and forth on such small changes. It’s better use of both our times if I take a few minutes to make the small changes and merge in.

For more complex PRs, my guiding principle is:

- Make those changes that might require a lot of back and forth and harder to explain or might be subjective.

- Request changes that are objective.

Really depends. If its a simple fix for something I will usually merge and clean up in subsequent PR's. The contributor is helping me out, I want to make it as easy for them as possible, which makes it more likely they will continue to help. As the difficulty and complexity of the PR increases I will proportionately increase the standards for the PR. Obviously there are other factors that would change this approach but in general, I try to make contributing easy as possible.

My typical workflow - in private business repos, but for the same reasons specified in OP - is as follows:

1. Fork the PR branch. 2. Make the changes I want to see upstream. 3. Create a PR into the author's branch. 4. Ask the author to review my changes. 5. When my PR is complete, finally review and merge The original PR.

This gives me the opportunity to explain my changes in commit messages, and gives the author an opportunity to push back or argue actual functionality changes I made if it's not all coding style. Now the author has license in accepting my changes. It avoids the cumbersome comment/resolve feedback loop, shows the author that their contribution is important and I'm willing to put time into making it right, and ultimately gives them credit when their PR is merged.

Depending on the details, I'll rewrite as you describe, or merge, then fix with my own commit.

It's frustrating being on the other end of this (PR submitter), since for the repos I've contributed to, the maintainers usually want you to fix it until it's exactly how they like. Then I end up giving up and using a custom fork, or integrating their code into my program directly.

The solution is simple, make it easy for people to contribute in way you deem appropriate. In the case of PR you can simplify the process quite a bit:

https://docs.github.com/en/communities/using-templates-to-en...

On Github at least the default setting when sending a PR to a repo lets maintainers submit their own commits to your branch, so if I need to do some cleanup I'll often default to that so the user can get credit for both the PR and the commit. If for some reason that isn't possible, it's still relatively easy to fetch the branch locally, add another commit, and then merge that (either manually or through a separate PR), at which point you can also add a comment with a link to the PR the user created if you want to make sure to give them credit as much as possible when closing the PR. If for some reason you absolutely need to have a single commit (e.g. the commit the user submitted would fail some CI check and not be mergeable even with a follow-up commit to fix it), then it's also usually possible to amend their commit with the changes you need locally and then merge that (unless they happend to be GPG signing their commits or something). I consider this a last resort since it feels a bit weird to attribute code to someone without them actually having written it themself, so if I need to do this I'll generally also add another paragraph the commit message explaining what I added myself (there's probably some built-in way with git to add a co-author or something that someone can fill me in on, but I don't know it right now).

The other thing I try to make sure to do is thank each contributor explicitly in the release notes when their commits get merged. A simple "Special thanks to @foo for the fix for bug X, @bar for the implementation of feature Y, and @baz for improving the documentation of Z!" is a small amount of effort that I think can go a long way in making contributors feel welcome and appreciated.

There is no way to win, for me, because it burns me out. I am/was a semi-perfectionist maintainer.

If I subject others to my standards, it's annoying for them.

If I hold my nose and merge it anyway, I feel the pressure to clean it up before the release. Which burns me out, so I avoid doing this.

I try to help contributors to make it so their commits can be merged without any changes. For some this can be annoying in the beginning, but the goal is that they learn to write quality patches and that I will have less work getting their work merged. I hope that this is a win-win. There are times where I do some changes (rebasing, small fixes, rewording of commit messages), but I will try to ensure their name is in the Author: field in the commit messages.

As mentioned by sph: some projects have maintainers that have much stricter standards for outside contributors that for themselves. Speaking as a maintainer of a project that accepts contributions through PR requests, I am afraid I do indeed have such a double standard, although I try to avoid it as much as possible. I had to set some rules for myself:

- Contributors must fix outright bugs in their commits.

- Style issues are avoided by having a coding style enforced by a code formatting tool. This saves everyones time.

- If the commit is bug free (passes test suite and code review) and is a net improvement, accept it, regardless of my own plans for the project.

- If there is more that can be done to make it ideal, this can be done in a future pull request, or perhaps I will add those changes later myself.

I also try to hold myself to the same standards as I hold contributors to, and now often create a development branch myself for features so they get the same CI testing as pull requests, although I still sometimes bypass this for quick fixes.

The first person who ever submitted a PR to one of my projects also reformatted the files he touched according to whatever formatter his IDE was using, which was different than mine. I was conflicted about whether to accept his change and inferior formatting or whether telling him to adopt my formatting would be obnoxious. I settled on merging his change and then instantly reformatting it to the way I like it - keeping the meaningful part of his change.