Why curl closes PRs on GitHub

https://daniel.haxx.se/blog/2024/06/11/why-curl-closes-prs-on-github/

76 37 2 years ago

daniel.haxx.se

7bit• 2 years ago

In order to make sure the commit message are correct, and in fact that the entire commit looks correct, we merge pull requests manually.

I thought maintainers can edit pull request? Why is that not used here?

The also say they don't want GitHub dictating them how to use git. I'd say, don't use GitHub then. Not pulling someones PR also means he does not get any public attribution to it. He doesn't show up as a contributor, too. I find that problematic and would say, there are simple ways around all the issues that have been listed, but the maintainers are just too stubborn to implement them.

jraph• 2 years ago

I thought maintainers can edit pull request? Why is that not used here?

The article says:

That is however a clunky, annoying and time-consuming extra-step that not only requires that we (always) push code to other people’s branches, it also triggers a whole new round of CI jobs. This, only to get a purple blob instead of a red one. Not worth it.
Not pulling someones PR also means he does not get any public attribution to it

I don't believe this is the case. Edited commits can keep the author (Author:, Co-Authored-By:).

I'd say, don't use GitHub then

I'd say this would be throwing the baby with the bathwater.

(I'd also say amen to that though.)

the maintainers are just too stubborn to implement them.

A gross mischaracterization of Daniel, from what I see. I was lucky to attend to his presentation at FOSDEM in February, he really seems to be a nice guy.

detaro3• 2 years ago

Not pulling someones PR also means he does not get any public attribution to it. He doesn't show up as a contributor, too

Manually merging a commit with fixups does not remove any attribution, what are you talking about? Who determines "committers to this project" by looking at the list of PRs on Github instead of the contributor list in the repo, the commits in the repo, ...?

throwanem4• 2 years ago

I suspect the concern is less about who's credited in the curl repository on Github, and more about who gets to point to evidence on Github of having contributed to curl.

throwanem4• 2 years ago

Commit messages are a Git feature. PR descriptions are a Github innovation and do not exist in repositories. curl's development is git-centric and not Github-centric.

mathstuf4• 2 years ago

I thought maintainers can edit pull request? Why is that not used here?

I don't know if Github has it, but GitLab supports "push options"[1] where you can say "skip CI". There are other mechanisms, but they stay in the history since they live in the commit message (and the summary of all places!).

Github seems allergic to any features that are mostly useful to "rewrite history" workflows though, so I wouldn't be surprised if that wasn't around.

Not pulling someones PR also means he does not get any public attribution to it.

We use self-hosted GitLab and mirror on Github; I get boxes filled in on my Github account's grid and properly attributed in Github's stats despite only ever showing up there via `git push` commands.

[1]https://docs.gitlab.com/ee/user/project/push_options.html

zabil• 2 years ago

I set up GitHub to squash and merge commits by default, and to use the pull request title and description as the commit message instead of the individual commit messages. Won't that be helpful here?

mathstuf4• 2 years ago

Squashing (by default) loses any individual commit information by squeezing out anything outside of the summary of each component commit. Editing from that state is usually far more work in the web UI than in a local editor.

vault• 2 years ago

We also do squash and merge by default in my company. Each PR is supposed to represent one and only one feature and/or change. Can you please better explain the disadvantages of this? Of which editing are you talking about? What needs to be edited after the squash and merge?

ryukoposting2• 2 years ago

Interesting. I made some contributions to the Zephyr RTOS project early this year, and they had a different approach to the same problems.

It seems like Curl makes it the maintainers' job to do the rebasing and commit-style corrections. Zephyr, on the other hand, put it on the pull request's creator. I ended up force-pushing and rebasing half a dozen times because I had to learn all their rules the hard way.

I don't think either method is necessarily better than the other - it depends on the project. Zephyr's approach lets the maintainers focus on more important things than commit-style yak shaving, while curl's approach lowers the barrier to entry for inexperienced contributors.

zdw• 2 years ago

There are some code systems that force a rebase-centric workflow like Gerrit, where you literally can't push more changes, unless you want to make separate review requests that get queued (which can be desirable)

Coming from those better systems, the review interface of Github feels like a badly implemented afterthought.

account423• 2 years ago

Coming from those better systems, the review interface of Github feels like a badly implemented afterthought.

This really goes for most of GitHub's additions. Issues, releases, wiki all feel like the bare minimum needed to claim it has those features.

account423• 2 years ago

Zephyr's approach lets the maintainers focus on more important things than commit-style yak shaving, while curl's approach lowers the barrier to entry for inexperienced contributors.

Depends on the person I guess but I often find it less exhausting to just fix up minor issues myself than to get a driveby contributor to adhere to the project style.

ryukoposting2• 2 years ago

I guess I should clarify - the maintainers didn't poke me to follow any guides. They had a wall of GitHub actions yelling at me in their stead.

gus_massa• 2 years ago

From time to time I merge PR in Racket, and they need a rebase or a tiny tweak. In those case I post a closing comment like

Thanks! Merged in 67b421.

So the author can find the final version merge in the main branch.

rany_2• 2 years ago

Couldn't they just edit the PR branch directly? When I make PRs I have an option "Allow edits by maintainers" which I assume would give them write access to my branch?

Edit: "We COULD but we won’t" actually addresses that, but it just wasn't obvious to me that they were referring to that feature. I can't delete this comment anymore.

corytheboyd2• 2 years ago

They explicitly call out not wanting to do this in the blog post.

rany_2• 2 years ago

I didn't notice that they were referring to that feature, it's too late to delete my comment now...

phoenix616• 2 years ago

Besides what they mentioned in the blog post you also can't push to PR branches owned by organizations on GitHub without getting manually added to the fork repo.

graton• 2 years ago

I'm glad to see someone else annoyed that GitHub doesn't allow commenting on the commit messages directly. One of the things I miss from Gerrit.

giancarlostoro• 2 years ago

I was working at a company that hired a lot of people new to dev. So one guy, comes to us saying his commits arent matching the standard they want. I see his commits, and I see like 50 of them that just say stuff like "fixing the bug", "finally fixing the bug", I tried to tell him, try to describe what the code is changing / doing with this commit as if you had to explain it to you mom. He gave me a look like I was crazy and proceeded to ask someone else for help. I assume he's still committing with awful messaging that matches their desired format.

rezonant• 2 years ago

I think the big take away is that GitHub should add a "Merges #X" feature similar to Closes #X as Daniel suggests.

An additional problem with closing a pull request instead of "purple merging" it is that it gives off the wrong end result when you view the pull request from the pull requests view, as an activity comment in a related issue, or by viewing the top of a pull request. Red should mean change rejected and purple should mean change accepted, regardless of the mechanism by which the code was accepted.

fahhem• 2 years ago

Is the issue just that the commit messages aren't considered important in Github's UI? There's a few tools that do this better, notable Reviewable.io makes the commit message(s) into a virtual file that can be commented on.

Disclosure: I used to work at Reviewable

MaskRay• 2 years ago

For people who prefer rebase and force push, https://getcord.github.io/spr/ is a great tool resembling Phabricator's `arc diff`.

https://github.com/orgs/community/discussions/3478 ("Improve workflow when force-pushing during code reviews") could use more support.

I've also got lots of complaints in this section when LLVM switched to GitHub PR: https://maskray.me/blog/2023-09-09-reflections-on-llvm-switc...

Why curl closes PRs on GitHub

Related Stories