Mastodawn

Act 2:

Said maintainer sends huge PR fixing that bug plus changing ALL THE THINGS. Some bugfixes, some just to introduce different tooling.

Yay, great news! 🎉 They want to contribute back!

As I always do when reviewing a PR, I start by thanking them for their contribution. I then kindly ask them to split the PR in more manageable chunks so they can be reviewed and/or merged independently.

All pretty standard in open source.

Act 3:

A few hours later, I get this email, basically saying it's too much hassle to follow typical open source etiquette and that I should either hand over the project to them, or they'll just maintain their own fork.

Of course it's within their rights to maintain their own fork. Forks are great, and the only way forwards for abandoned codebases, or an entirely different direction.

But what I love about open source is when we all work on something together and contribute to a common project.

Maintaining your own fork when the maintainer is generally happy to merge in PRs, just because you can't be bothered to send proper PRs upstream is just …not very nice. 😕

Lea Verou, PhD Mar 8, 2023

Update: This story does have a happy ending!

Apparently this blew up internally and several of his colleagues explained to him that this is not appropriate conduct.

He wrote back to apologize, and is now sending several smaller PRs, which I've so far been merging.

I think I'll add him as collaborator soon, organically, like I do with everyone that has several substantive merged PRs. 😊

Melanie Mar 8, 2023

@leaverou I am pleasantly surprised that there’s a happy ending and not just a hostile takeover. 😍

Sandor Szücs Mar 8, 2023

@leaverou that’s a nice turnaround. I hope it will stay like this in the future

Thomas Cannon Mar 6, 2023

@leaverou engineers will literally write a whole-ass email and be willing to maintain a fork instead of going to therapy 🙄

@tcannonfodder @leaverou "maintain a fork"... Bro's gonna apply the patch, disable the tests make a single release to their internal packing mirror using the same name as your package and then switch teams, leaving no docs and meaning every dev on that team in the future is going report bugs in his fork on your project

Even if you put "I don't work on team xxx" in your bug report template you'll still get your queue filled with possibility automated bug reports

Thomas Cannon Mar 7, 2023

@EndlessMason @leaverou dammit, hit the nail on the head there 🫠

@tcannonfodder Don't worry, I'm sure he'll raise a ticket about "getting back to mainline yourlib as soon as his PR is merged".

🐡🏳️‍🌈Mar 11, 2023

@tcannonfodder @leaverou bookmarking this golden line for future conversations

jr conlin —〰—Mar 6, 2023

@leaverou

It's also a fun way to introduce tons of future breaking changes and maintenance nightmares for whoever gets stuck dealing with what's using the fork.

Loads of fun for everyone.

Dan Burzo Mar 6, 2023

@leaverou that's just multiple levels of rude

Sara Joy

Mar 6, 2023

@leaverou I would go so far as to say Deeply Uncool. I'm so sorry and hope they don't follow through with that.

iamdtms Mar 6, 2023

@leaverou
Are there any part in GPL / CC Zero / whatever license against this attitude? It's not for 🚫 free robbery I think 💬, it's an intellectual property handling with rules. Not a raw binary copying.

@iamdtms Nearly all of my projects are MIT license. Forking is perfectly fine with most (all?) OSS licenses.

Being an asshole is not generally illegal. 🤷🏽‍♀️

Rick Rae Mar 7, 2023

@leaverou
Non-specific observation:

"Being an asshole is not generally illegal. 🤷🏽‍♀️"

It should be.

On the other hand, we think our prisons are overcrowded NOW...
@iamdtms

Hilko Bengen Mar 8, 2023

@iamdtms @leaverou Not in any of the standard licenses. However, if you want to avoid FAANG engineers annoying you for specific projects, just use AGPL. ;-) Doesn't work well for library code, though.

Christopher Kirk-Nielsen Mar 6, 2023

@leaverou Yikes. Very "my way or the high way" despite this being your project.

Also I'm sorry for being nosy but I got curious and… they threw TypeScript refactor in there? Really?? 😅

otsch (Christian Olear)Mar 6, 2023

@leaverou Wow, asking for maintainership is a little cocky to say the least.

@otsch The funny thing is, I’m pretty liberal with handing out co-maintainership to folks with a few substantive merged PRs. But they don't seem to want co- anything.

otsch (Christian Olear)Mar 6, 2023

@leaverou Yeah, it's certainly great for projects when there are multiple core contributors. But I also read it more like: is it OK for you if we take over and just do whatever we want with it?

Jason Levine Mar 6, 2023

@leaverou Well, that's entirely shitty (as is the engineer's decision to use email to reply to you, rather than the discussion thread on the PR itself). Google's gonna Google, of course, but it'd be nice if they weren't dicks about it.

@delfuego I suspect they know it's shitty, which is why they didn't want it posted in the open.

Ьλ∂λ Mar 7, 2023

@leaverou @delfuego Would it be petty to close the PR with the screenshot of his email as a reason?

I don't think so...

Jason Levine Mar 7, 2023

@leaverou I'm not gonna lie, I kinda want to send you a PR with *just* a fix for his initial issue, so you can commit it and create a diverging network for the project that he'll have to maintain over time with his special-snowflake fork.

AlisonW ♿🏳️‍🌈♾️Mar 6, 2023

@leaverou
My response would be along the lines of "fucking cheek, they can get lost."
They're basically wanting to steal your good name and the trust others have in your codebase. YMMV but I'd be telling them to fork off.

Brendan Hayward Mar 7, 2023

@leaverou
[x] lazy
[x] rude
[x] unprofessional
[ ] 1337 haxxor skillz

2¢Mar 7, 2023

@leaverou either their contributions are worthwhile in aggregate for your purposes or not. They have their style, you have yours. If you don't want their gift, refuse it. They don't owe you anything more, even if they started with your gift of code. They are disinclined to follow your rules about "proper" code revision procedures. Oh well. Indeed, forks are great.

@leaverou I've worked at Froogle in the past. I don't understand the "take over" offer but I think the whole thing might be miscommunication coming from very different perspective, that could be cleared up over a cup of tea. Allow me to explain:

@leaverou

GitHub's code review interface is bad. It's horrible. Surely it could be worse, and probably some people learned to love it because they're stuck with it, but it's still a lot of pain for me to use compared to Froogle internal tools.

That's why you need to split stacked commits in several PRs in GitHub. To keep each PR focused on single narrow topic with few comments, so that fix-up commits stay in the same PR and you can easily view the diff between each review round.

@leaverou

For someone used to Froogle tools, this makes no sense. If they have never reviewed a (large) PR on GitHub, they have no idea how painful it is. With a better review system, you can just comment on each *commit* of a single PR, as if they were independent PRs, use `git commit --amend` for fixing up review comments, and the system keeps track of things neatly for you, allowing you to see the diff of each amended commit individually. That's how Gerrit works: https://www.gerritcodereview.com/

Gerrit Code Review | Gerrit Code Review

@leaverou That's the missing part in your initial screenshot: yes, that have changed ~10k lines at once, but they did so in 14 commits. And it's possible they didn't realize that something that would be a routine review in a better-designed system is intractable in Github's.

(Full disclosure: I left Froogle2 years ago, don't know the author of the PR nor their team. And again I don't understand the last part of their email. I'm just trying to offer perspective in the hope of collaboration.)

@GabrielKerneis I see what you're saying, and thanks for the different perspective. However, this is supposed to be a maintainer of a pretty popular Froogle open source project, how can they not know how GitHub works?

@leaverou It could be that they use Froogle-internal tools to manage code and reviews, and merely push to GitHub using Froogle-internal tools that bridge the gap. Or it could be that they're just rude or have an agenda I don't understand. My guess at the take-over offer is because they really want to switch to TypeScript for some reason, and fear that's why you'd push back. But frankly, I didn't like that part of the email at all.

@GabrielKerneis Thanks for the background.

GitHub does allow you to review individual commits too these days. But the point about not being able to merge them at different speeds (or not merge some at all) still applies, right?

Also, reviewing commits is the wrong level of granularity, unless the requester has carefully gone through the log and squashed.

@leaverou You cannot really comment on each commit in GitHub: you can view them individually, but the comment is attached to the PR, not the commit, and there is no notion of commit identity that would survive `git commit --amend`. That's why reviewing at the commit level feels the bad granularity. But it's the common level of granularity for Linux for instance, that does reviews by email, or for projects using Gerrit as a review system.

Eg. see the "relation chain" on https://fuchsia-review.googlesource.com/c/fuchsia/+/316985

@leaverou And note the "patchset" drop-down that allows you to see iterations of the commit (basically one patchset for every "git push"). So you can compare any file at any specific point during the review, eg. between patchset 17 and 34 : https://fuchsia-review.googlesource.com/c/fuchsia/+/316985/17..34/src/ledger/bin/storage/impl/page_storage_impl.h

Green/Red are diffs with actual changes from this commit. Yellow/Blue are diffs due to a `git rebase` in between those patchsets.

@GabrielKerneis So the intended workflow is that you continuously force push and rewrite history in that branch, and comments survive this, then when the PR is merged you rebase that polished commit log? Fascinating.

But still, how do you decide to only merge some of the changes?

@leaverou You need to rewrite history to drop the commits you don't want. From a UI standpoint, the reviewer can reject commit B and approve A and C, but merging a commit automatically merges all ancestors, so approving C implicitly approves B even if it's hidden in the UI. That bit is not automated and requires some care as well as collaboration between reviewer and author (there may be server-side scripts that check that the whole chain has been approved when you merge, I can't remember).

@GabrielKerneis But what if you don't want to entirely wipe that change, but some changes can be fast-tracked, while others require lengthier review and more iterations? Why should the latter hold back the former?

@leaverou If they're independent of each other, you can rebase to move the ones that are ready at the bottom of the stack and merge that part already (but the UI won't do it for you — or maybe it can, you can even author your commits in the UI, but I never trusted it for that!). The trick is the `Change-Id` added to each commit message that allows tracking commit identity as you move them around branches while preserving review comments.

@leaverou The docs don't cover those topics exactly, but they are nice entry points:
- https://gerrit-review.googlesource.com/Documentation/intro-gerrit-walkthrough-github.html
- https://gerrit-review.googlesource.com/Documentation/intro-user.html#multiple-features

And you can apparently configure "cherry-pick" as a merge strategy for your project, but it'd be very error-prone in my opinion (you need to remember to merge in the right order): https://gerrit-review.googlesource.com/Documentation/config-project-config.html#cherry_pick

Basic Gerrit Walkthrough — For GitHub Users

@GabrielKerneis @leaverou it is not just an issue during review. That huge commit is an issue later too.
- merging other branches conflicts with it
- a git-bisect will likely always end there
- you can't talk about "a bug introduced commit badc0de" and have the reader of your message be sure which bug
- it's real hard to be sure all the code and docs match in a huge pr
- is it all tested
- it makes writing change notes for releases hard
- you can't ever revert it

@EndlessMason the actual comment by @leaverou asked to split the *PR*, not to split the *commits*. Which makes sense to me: the largest commit of the PR is a transition from Javascript to TypeScript: whether it's a good idea or not is to be decided by the maintainer, but there's not a lot of ways to break it up, it has to be an atomic change. And bug fixes were built on top of it, they would have to be rewritten in Javascript if the migration is rejected, not merely split in separate PRs.

@GabrielKerneis @leaverou to split hairs here, you can't split a pr without breaking your changes down into separate commits because you need a commit per PR.

If one wanted to allow the author to choose which fixes they want that can be achieved by having the "rewrite to type script" commit as the first commit in each of the other bug fix PRs, making them individually mergable.

(or talk to the owner before writing a bunch of code predicated on a change that might not be accepted)

@GabrielKerneis @leaverou (maybe I should have CW'ed that mspaint git diagram)

@EndlessMason @leaverou Agreed on both the technical and social sides here. But note again this boils down to GitHub's poor interface: other review systems let you easily manage trees of related PRs with a common root, and even plain old git-email lets you selectively pick and apply patches. (Also the original PR *was* already broken down into 14 commits.)

@GabrielKerneis @leaverou I mean, github could add an "include in merge" or "copy to new PR" type check boxes next to each commit, but I can only really imagine that creating load for the people who do helpdesk at github

@GabrielKerneis @leaverou In a case like this it might make sense for the PR to become the next major release?

I don't maintain anything public (not because of this, but in part because I've seen things like this at places I've worked) so I'm betting there are consequences for having a rewrite on either side of a support horizon

@EndlessMason @GabrielKerneis FWIW, I've been doing open source for 14+ years and the good far outweighs the bad. You just hear about the bad more frequently, because people are more likely to post to vent than to post "People are wonderful, someone just sent me a PR to add a whole testsuite to my project"

@leaverou @GabrielKerneis I am sure a lot of them are.

I do often find myself paired with someone who half-jokingly suggests following the "fix it later" methodology, only to gleefully plow ahead when I point out that submitting a PR with tests and adding a temp work around to the app is both less work short term and less cursed long term

You know, the old "oh, why do we depend-exact-version on a version of libxxx from 1997?" or "Is this just the whole of X::Y pasted in here"?

shironeko Mar 7, 2023

@leaverou are the commits digestible at least?

Arne Brasseur Mar 7, 2023

@leaverou I refer to these as drive-by PRs. People that want to submit code without looking back. No interest in starting a dialogue or responding to code review. Which usually also means that if there are any issues later on with that code, that it's up to you to maintain it.

Sheogorath Mar 7, 2023

@leaverou don't be upset, usually they come back. And if they don't, at least they don't bother you anymore. In any way possible it's a win for you ;)

David Beazley Mar 8, 2023

@leaverou You got a PR? I just got the complaints about my project during some froogler's keynote talk.

hrbrmstr 🇺🇦 🇬🇱 🇨🇦Mar 8, 2023

@leaverou I’ve seen corporations suck the very life out of FOSS, and I tend to avoid doing anything adjacent to certain corporation in my areas of interest.

It’s also fairly sad they didn’t even both to work with you. What’s worse, they likely wouldn’t have done any sponsoring, even if you had acquiesced to their massive PR demand.

Jeremy Kun Mar 8, 2023

@leaverou this wouldn't even pass Google's internal guides. They're very clear internally that if the owner asks you to split into small CLs, you do that.