Counterpoint: gorhill and uBlock. IIRC, gorhill (Raymond Hill), creator of the popular uBlock adblock extension, wanted to step down and hand the reins off to a contributor. The contributor then promptly removed all references to gorhill and started charging for the plugin and turning the extension into an affiliate marketing product.
> The PR was bigger than what I felt I could sensibly review and, in honesty, my desire to go through the hours of work I could tell this would take for a project I no longer used was not stellar.
This doesn't look like a particularly big PR to me, judging solely by the amount of code changed and the nature of the changes at first glance.
Are most of your PRs at work tiny, couple lines of code at most? Am I sloppy for not even consider reviewing this for "hours"? Are all code bases I have worked on sloppy because features often require changing more code than this?
When I wrote this, almost all my engineering experience was in OSS database development. That environment has several forces pushing towards very detailed reviews and clean commit histories, like others have hinted at in the thread here.
The PR review is in public and heavily scrutinized by paying customers and passionate community members. APIs cannot be broken, and even with automated tooling it's very easy to accidentally introduce a change that breaks tens of thousands of deployments. And the code itself is really sensitive. If a bug gets in and released, it can be several days of grind to get a patch out, and after that many months of new tickets for the bug from customers that won't move to the latest patches.
Now I work somewhere where the code I write runs in-house. If a bug sneaks in, it's usually a 5-minute redeploy to resolve and the cost is borne primarily by my own team.
So I think the answer to your question is: it really depends on the environment you're writing code in. In some setups the cost of introducing mistakes is very high, so it makes sense to pay a lot at the review stage; in others the correct balance is less strict review and fast fixes/rollbacks instead.
The PR isn't very large, diff-wise, but IMO it isn't well made, which makes it hard to review.
Half of the commits are merges from other fork branches into the contributor's master, and the PR name and description doesn't mirror that in the least.
Then (eyeballing) 90% of the diff is whitespace changes, which would be fine in its own PR ("Formatting changes") because it's easy to eyeball that it's just that, but when you mix it with other changes, it's hard again.
After a brief scan I’d call the full change reviewable enough I could do it in a sitting. Most of it looks reviewable on my phone. But seeing >30 commits, I’d pause. Partly because I’ve become a lot more sensitive to the impact of commit history itself, partly because the quick scan of such a small change set doesn’t seem to line up with so many commits, but mostly because it implies much more context exists than the attention I’d pay if it came pre-squashed.
That kind of implication stops me in my tracks to learn more. I’ve spent literal days tracking down the meaning of single line code changes through multiple dozens of commits, sometimes across repo boundaries (ahem the original author’s suggestion of deprecating in favor of a fork comes to mind).
The size of this particular PR only becomes a factor when any one of those numerous commits can become that rabbit hole. How many humans’ days are going to be spent tracing history through this particular merge? For how many different reasons? I didn’t even look at the changes midway, but how many nuances are buried in there and lost unless this weird bundle of changes is preserved?
So with libraries that are consumed by third parties, it's rarely about the number of lines of code. Since you don't really have a definitive list of all usages of your code, things like the changes to `_money_from_dict` making things nullable mean you have to consider all ways in which that might blow up.
And like "the public API is the public API, the private API everything goes" is easy to say, but it's so easy to just break other projects with these kinds of changes. This makes it very hard to move forward with certain kinds of "bug fixes" in projects.
That being said, it's not that that PR is "hard", but it's hard to say "merging this is A-OK" instantly. Hours? I dunno, but I would definitely add some tests and try to figure out how to write code that breaks with those changes.
If I were managing that project at the time, and had motivation, I'd definitely do a lot of cherrypicking, get all the "obviously won't break anything" changes merged in, to leave the problematic bits in for a focused review. Again, this all might be under an hour, but sometimes you look at a thing and are like "I don't really want to deal with this, I have my own life to deal with."
At a higher level, the biggest problem with these kinds of libraries is having the single person who can merge things in, who can hit the "release" button. There's a lot of projects that interact with Django that have heavy usage, and survive mainly thanks to random people making forks and adding patches that properly implement support for newer Django. At $OLD_JOB we used a fork of django-money (including a lot of patches to fix stuff like "USD could be ordered with JPY", pure bug generators). It was very easy to add patches because, well, we had our usage and our test suite and no external users. It's great, but it's also important to try and get patches upstreamed when possible (and we did for a lot of projects).
According to the article, the author no longer used this project. It was no longer front of mind. There is a massive difference between reviewing a PR when you are actively involved in a project and reviewing one when the project is in your past.
In that case, it’s perfectly reasonable to spend a couple of hours getting back into code you wrote a long time ago. If anything, taking that time is a big win for overall project stability.
It's not too bad, but in the context of reviewing for a project in your spare time, it can be draining. I totally understand why he didn't feel like doing it.
I've committed bigger just with a "minor changes" -message :D
But seriously: that code seems to be touching bits that really should have automated tests attached. If the tests pass, then I would feel more comfortable accepting the PR.
Depends on the bits they don't do. I get contributions from time to time. Yet can look small , but I need to add tests, documentation, and so on. It can easily take an hour for just one method added, or whatever.
If the code is just fire and forget, then fine. If it's part of a bigger system with rigorous standards then 300 lines can take a day or more to "merge" in.
It's certainly not hours of work to review it... or maybe it is since it's financial? Either way, "it was in the script" as my wife and I say about corny movie moments. It made for a good article.
For this PR in particular, seems like a lot of it is formatting changes, so you’re right, may not have been a big deal in practice. But I wouldn’t take the statement so literally. For an open source project, every PR can contain unbounded toil for no pay.
It isn't that big at all, but when you maintain something you really don't want to maintain, you're over anything but the tiniest changes such as updating the copy. Ask me how I know. lol
This is not at all a huge PR. Sometimes I make thousand line changes (or way over that), although in most cases it is just me working on a project. Changing a couple files (like in this PR) should be OK.
Do others share the sentiment that your reply is quite, sad to say that, arrogant?
What are you exactly trying to achieve by comparing the guy's "I'm busy, this is long" with yours or anybody elses? Moreover, what on earth has his job to do with the topic?
Trust goes a long way. Microsoft had given me read/write access to full Windows source code on my first day despite that I was only hired to work on certain parts of the kernel and drivers. I'd never been shown this kind of trust before; other companies I'd worked with always had layers around trust, so, this kind of "you're 100% one of us now" message on my first day had made me extra happy and motivated at Microsoft. I was extra careful protecting source code too. Loved working there until the day I quit.
"he emailed me and said he wanted to maintain the module, so I gave it to him. I don't get any thing from maintaining this module, and I don't even use it anymore, and havn't for years"
This is the risk of open source not figuring out funding.
You can not compare an email asking for rights, to someone who has written a massive amount of work, which you can randomly pick a section and see it is correct.
I've given commit and other "dangerous" access (moderator or admin privileges) to people I don't personally know or barely know for a long time now and I don't think I've ever regretted it.
My criteria is usually just a willingness to improve the situation. I can observe this over time via pull requests, forks and general community participation.
I'm very reluctant to give access to someone asking for it. I firmly believe this is something that should be given and not to be expected.
I gave moderator and admin rights to someone over my Ingress Google+ community I hard worked hard to build, because I didn't play anymore. He promptly removed my admin rights and pursued his agenda with regards to some community Drama. That was an education to me in the human appetite for meaningless petty power plays when all is needed is a bit of good will and cooperation.
I did this with every first committer to https://github.com/zladx/LADX-Disassembly : giving commit rights immediately (so that they can merge their first PR themselves).
I did wonders to foster a community of contributors, and get more patches coming. The CI ensures nothing breaks, and there never was any trust incident.
I wanted contribute a fix for a bug that I ran into all the time to https://curlconverter.com/ . The author gave me commit access while I was still working on my first PR (or shortly after) without me asking. I appreciated the trust and I ended up contributing way more than I originally intended to.
See also [0]. Rutger argues that if you have never trusted someone like this (somewhat) blindly, you may have indeed protected yourself from some misery, but you are also overwhelmingly denying yourself the better parts of life. Because "most people are kind" (which is the original Dutch title of the book btw).
My personal rule is that you get commit access after some number of good PRs, depending on the project. Has worked out quite well:
- instar. I had two guys basically rewrite the entire thing and make it WAY better. I had a good vision for the API but my implementation was pretty bad.
- mutmut. I would never have gotten windows support going without help. (Although I am thinking of abandoning windows anyway soon...)
- iommi. This project is much more complex and has a certain philosophy, but we gave commit access to one developer pretty fast as it was super obvious from the first PR what kind of deep thinking he did.
These days getting commit rights to inject malware into an already popular gem has become a real threat.
In 2016 I think it wasn't yet/wasn't recognized.
I am very sympathetic to the suggestion in OP prior to recognizing that there may be people actually actively trying to abuse your trust to intentionally inject malware.
If the malicious committer is able to publish releases, that malicious commit might have already reached users (and stolen/deleted their data, or whatever it was meant to do) by the time it's detected, and reverting won't help.
I created/maintained a popular project for years[^1], and recently passed ownership to someone else. It's been great seeing issues resolve, PRs merge, etc, after languishing for a while :)
Man I remember having to monkey patch that library to do some caching of the exchange rates. Otherwise it would do dozens of requests for the same information exchange rate.
This happened with clojupyter for me. I just gave everyone who submitted something good commit access, and a handful of people who are way better clojure coders than me made it way better in every way.
If the maintainer of a package trusts everyone on the Internet, then users who trust that maintainer (by installing their package) transitively trusts everyone on the Internet, which they might not have expected if the maintainer didn't declare this position upfront.
Maybe we need a way to declare in the package and repository metadata that the maintainer considers it world-writable and it shouldn't be installed or updated without very carefully reviewing the code of every new version.
[+] [-] faitswulff|2 years ago|reply
This reddit comment covers it pretty well: https://reddit.com/r/ublock/comments/32mos6/_/cte0a3n/?conte...
[+] [-] bbbobbb|2 years ago|reply
> The PR was bigger than what I felt I could sensibly review and, in honesty, my desire to go through the hours of work I could tell this would take for a project I no longer used was not stellar.
The PR: https://github.com/django-money/django-money/pull/2/files?di...
Do others share this sentiment?
This doesn't look like a particularly big PR to me, judging solely by the amount of code changed and the nature of the changes at first glance.
Are most of your PRs at work tiny, couple lines of code at most? Am I sloppy for not even consider reviewing this for "hours"? Are all code bases I have worked on sloppy because features often require changing more code than this?
[+] [-] jakewins|2 years ago|reply
The PR review is in public and heavily scrutinized by paying customers and passionate community members. APIs cannot be broken, and even with automated tooling it's very easy to accidentally introduce a change that breaks tens of thousands of deployments. And the code itself is really sensitive. If a bug gets in and released, it can be several days of grind to get a patch out, and after that many months of new tickets for the bug from customers that won't move to the latest patches.
Now I work somewhere where the code I write runs in-house. If a bug sneaks in, it's usually a 5-minute redeploy to resolve and the cost is borne primarily by my own team.
So I think the answer to your question is: it really depends on the environment you're writing code in. In some setups the cost of introducing mistakes is very high, so it makes sense to pay a lot at the review stage; in others the correct balance is less strict review and fast fixes/rollbacks instead.
[+] [-] akx|2 years ago|reply
Half of the commits are merges from other fork branches into the contributor's master, and the PR name and description doesn't mirror that in the least.
Then (eyeballing) 90% of the diff is whitespace changes, which would be fine in its own PR ("Formatting changes") because it's easy to eyeball that it's just that, but when you mix it with other changes, it's hard again.
[+] [-] eyelidlessness|2 years ago|reply
That kind of implication stops me in my tracks to learn more. I’ve spent literal days tracking down the meaning of single line code changes through multiple dozens of commits, sometimes across repo boundaries (ahem the original author’s suggestion of deprecating in favor of a fork comes to mind).
The size of this particular PR only becomes a factor when any one of those numerous commits can become that rabbit hole. How many humans’ days are going to be spent tracing history through this particular merge? For how many different reasons? I didn’t even look at the changes midway, but how many nuances are buried in there and lost unless this weird bundle of changes is preserved?
[+] [-] rtpg|2 years ago|reply
And like "the public API is the public API, the private API everything goes" is easy to say, but it's so easy to just break other projects with these kinds of changes. This makes it very hard to move forward with certain kinds of "bug fixes" in projects.
That being said, it's not that that PR is "hard", but it's hard to say "merging this is A-OK" instantly. Hours? I dunno, but I would definitely add some tests and try to figure out how to write code that breaks with those changes.
If I were managing that project at the time, and had motivation, I'd definitely do a lot of cherrypicking, get all the "obviously won't break anything" changes merged in, to leave the problematic bits in for a focused review. Again, this all might be under an hour, but sometimes you look at a thing and are like "I don't really want to deal with this, I have my own life to deal with."
At a higher level, the biggest problem with these kinds of libraries is having the single person who can merge things in, who can hit the "release" button. There's a lot of projects that interact with Django that have heavy usage, and survive mainly thanks to random people making forks and adding patches that properly implement support for newer Django. At $OLD_JOB we used a fork of django-money (including a lot of patches to fix stuff like "USD could be ordered with JPY", pure bug generators). It was very easy to add patches because, well, we had our usage and our test suite and no external users. It's great, but it's also important to try and get patches upstreamed when possible (and we did for a lot of projects).
[+] [-] hluska|2 years ago|reply
In that case, it’s perfectly reasonable to spend a couple of hours getting back into code you wrote a long time ago. If anything, taking that time is a big win for overall project stability.
[+] [-] theden|2 years ago|reply
[+] [-] theshrike79|2 years ago|reply
But seriously: that code seems to be touching bits that really should have automated tests attached. If the tests pass, then I would feel more comfortable accepting the PR.
[+] [-] bruce511|2 years ago|reply
If the code is just fire and forget, then fine. If it's part of a bigger system with rigorous standards then 300 lines can take a day or more to "merge" in.
[+] [-] MentallyRetired|2 years ago|reply
[+] [-] bjornasm|2 years ago|reply
Well the author specified that this was their subjective take on it.
[+] [-] femiagbabiaka|2 years ago|reply
[+] [-] unknown|2 years ago|reply
[deleted]
[+] [-] xeromal|2 years ago|reply
[+] [-] paulddraper|2 years ago|reply
Some typo could break everything.
[+] [-] golergka|2 years ago|reply
[+] [-] quickthrower2|2 years ago|reply
[+] [-] edem|2 years ago|reply
[+] [-] kunley|2 years ago|reply
What are you exactly trying to achieve by comparing the guy's "I'm busy, this is long" with yours or anybody elses? Moreover, what on earth has his job to do with the topic?
Bad day...?
[+] [-] sedatk|2 years ago|reply
[+] [-] psacawa|2 years ago|reply
[+] [-] ipaddr|2 years ago|reply
This is the risk of open source not figuring out funding.
[+] [-] aaron695|2 years ago|reply
This commentator puts it well - https://news.ycombinator.com/item?id=36121561
This is simple spammer entropy.
Now with GPT spammers can create entropy very easily, so it's tricker.
[+] [-] CapsAdmin|2 years ago|reply
My criteria is usually just a willingness to improve the situation. I can observe this over time via pull requests, forks and general community participation.
I'm very reluctant to give access to someone asking for it. I firmly believe this is something that should be given and not to be expected.
[+] [-] totetsu|2 years ago|reply
[+] [-] kemenaran|2 years ago|reply
I did wonders to foster a community of contributors, and get more patches coming. The CI ensures nothing breaks, and there never was any trust incident.
[+] [-] kqr|2 years ago|reply
(The notable exception are people who specifically seek power. Somehow they seem to be the least responsible with it.)
[+] [-] verhovsky|2 years ago|reply
[+] [-] teekert|2 years ago|reply
[0] https://en.wikipedia.org/wiki/Humankind:_A_Hopeful_History
[+] [-] dang|2 years ago|reply
I gave commit rights to someone I didn't know - https://news.ycombinator.com/item?id=12522654 - Sept 2016 (100 comments)
[+] [-] boxed|2 years ago|reply
- instar. I had two guys basically rewrite the entire thing and make it WAY better. I had a good vision for the API but my implementation was pretty bad.
- mutmut. I would never have gotten windows support going without help. (Although I am thinking of abandoning windows anyway soon...)
- iommi. This project is much more complex and has a certain philosophy, but we gave commit access to one developer pretty fast as it was super obvious from the first PR what kind of deep thinking he did.
All in all, great success.
[+] [-] jrochkind1|2 years ago|reply
In 2016 I think it wasn't yet/wasn't recognized.
I am very sympathetic to the suggestion in OP prior to recognizing that there may be people actually actively trying to abuse your trust to intentionally inject malware.
[+] [-] londons_explore|2 years ago|reply
And if it did, sorting out the mess and reverting a malicious commit wouldn't be the end of the world.
[+] [-] ptx|2 years ago|reply
[+] [-] griffinmb|2 years ago|reply
[^1]: https://github.com/nccgroup/sobelow
[+] [-] VBprogrammer|2 years ago|reply
[+] [-] BehindTheMath|2 years ago|reply
https://twitter.com/MoOx/status/955903710617620482?t=BvPIWQ-...
[+] [-] mizzao|2 years ago|reply
https://felixge.de/2013/03/11/the-pull-request-hack/
[+] [-] rorykirchner|2 years ago|reply
[+] [-] ptx|2 years ago|reply
Maybe we need a way to declare in the package and repository metadata that the maintainer considers it world-writable and it shouldn't be installed or updated without very carefully reviewing the code of every new version.
[+] [-] ranting-moth|2 years ago|reply
I'm glad it worked for him, but just want to remind people of survivorship bias: https://xkcd.com/1827/