Hacker News Reader

Comment by not2b | Hacker News Reader

In many cases this is the right thing for a maintainer to do: a contributor produces a PR and a proposed patch, but often that patch doesn't solve the whole problem, or clashes with the coding style, or isn't very efficient, so the maintainer does their own fix, because that is faster than getting the contributor to produce a modified version.

View on HN

Yes, I do this for my own OSS projects. The standard approach of giving feedback and waiting for the user to fix something is fine, but for a small change it's easier to just expedite the process and do it myself.

But if my version of the code has substantial changes (ie changes beyond just whitespace, small tweaks to the code, changing the commit message), I push it to a branch and ask the PR author to review and approve it first. Only after they approve it do I merge it into master and close the PR.

I also retain the GIT_AUTHOR of the original PR so that they still get credit; my user is only the GIT_COMMITTER. And I add a "Closes #" ref to the GH PR in the commit message so that it can be tracked later. git also has a de-facto standard of having multiple authors for a commit via `Co-authored-by:` lines in the commit message. This is useful for when my contribution is large enough to be equivalent to the PR author's.

Note that this doesn't work for workflows that require signed commits. If you have such a workflow, you have to go back to giving feedback and waiting for the PR author to make changes.

> Note that this doesn't work for workflows that require signed commits. If you have such a workflow, you have to go back to giving feedback and waiting for the PR author to make changes.

While everyone else who uses the project suffers with the bug that was being fixed as they wait for the person who contributed the patch to go through some hazing process involving code formatting that they (hopefully: I realize some people are in it mostly for the GitHub gamification credit of being a "contributor" on their landing page and thereby will do absolutely anything to get exactly and precisely the author credit on the commit) didn't sign up for. No: please for the love of everyone you are responsible for just commit the fix and thank the person later.

Well, I wasn't talking about GH OSS repos specifically in that point. In any case, I imagine that any repo that requires signed commits is also corporate-enough that it'll have a CLA requirement, so if the PR author is unresponsive the maintainer could take the code and commit it as themselves anyway.

Thank you for all of these tips!

I've lately been feeling bad, and thinking I must look like an ungrateful asshat, about closing lower quality PRs (IMHO) with valid bugfixes but which introduces some new, possibly subtle, bug instead. Or having to close abandoned PRs because the submitter gave up before that last polishing to match the standard of my own repo. :(

Now I feel better knowing that I can do that final polish myself, while keeping the submitters original contrib!

The polite thing is to fork the contributor's PR branch back into the project repo, make changes preserving history, and then merge or squash merge the result.

It is a bit awkward though.

Some projects by nature attract high quality PRs. Others, like a game I built, have the unfortunate curse of attracting PRs with such low quality that it's kinda heart-breaking to shut them down.

It's one thing to read a good feature request in a Github issue and build it yourself. It's a whole other thing to modify a low quality PR in a polite way, see what they were trying to do, clean it up, refactor it. It can easily be 5x the work of just doing it from scratch as the project maintainer.

This experience, especially after your hundredth time, can jade you in a way and make you seem rude when you decide it's not worth the courtesy nor charity.

Yap and that's when you start feeling that OSS burnout creeping up on you.

Yeah, I get why this happens. And to be clear, I didn't dig in to the all the PRs and compare them vs the maintainer's commits, so I have no idea of the difference in code quality between the two.

I'm sure it's frustrating when maintaining a fairly popular OSS tool to receive a PR that's 95% of the way there. Having to go back and forth to coach someone on getting that last 5% (or the contributor just dropping the PR then ghosting) vs just doing it yourself, I totally get it.

However from the contributor's point of view, when GH has support for co-authored commits, it comes of as a bit of jerky move when you take the time to submit a PR to not at least get credit via a co-author commit message.

The commit log could still credit the bug discoverer even if that person's proposed fix isn't used.

GitHub's "support" for this is the difference between a commit credit and an author credit, which is a mechanism in git that has particular meaning with respect to cherry-picks and rebases. It should be considered awkward to attach someone else as "author" on a commit they might only sort of recognize.

Maybe instead of "taking the time to submit a PR" you should first submit an issue and only work on substantial code changes you are going to become emotionally invested in after you've negotiated the correct path forward with the maintainer? Open source used to be about communication and collaboration, not cowboy coding.

No, not cool. If you modify a submitted commit, set the Author field to the original author and add a Signed-off-by field with your email. Then you BOTH appear.

On GitHub you can push commits to the PR branch. I use that to fix up rough edges myself and then merge the PR.

Oh wow never realised that was possible! Thank you!

That will save so much time and energy having to deal with back-and-forths or abandoned PRs

it can be, yes. there's chicken and egg issue of trying to help other people be more proficient in making those mods, learning about larger parts of the system, etc.

there's also the issue of being told "no, that's not a bug" or "no, WONTFIX", then... hours or days later... producing the same code patch as your own. Definitely a jerk move.

Reply on news.ycombinator.com