Why curl closes PRs on GitHub(daniel.haxx.se)
daniel.haxx.se
Why curl closes PRs on GitHub
https://daniel.haxx.se/blog/2024/06/11/why-curl-closes-prs-on-github/
40 comments
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.
Coming from those better systems, the review interface of Github feels like a badly implemented afterthought.
> 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.
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.
> 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.
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.
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.
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.
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.
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.
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
Disclosure: I used to work at Reviewable
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.
> Thanks! Merged in 67b421.
So the author can find the final version merge in the main branch.
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?
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.
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?
Bisecting is more efficient when individual commits are preserved, IMO/E.
Inevitably you will encounter (a) a junior developer who considers commits to be 'save states' rather than individual logical changes or (b) someone who is used to squash merge and did not think commit history mattered.
Without squash merge, it then becomes very likely most commits do not even build making bisecting a bit of a nightmare.
Without squash merge, it then becomes very likely most commits do not even build making bisecting a bit of a nightmare.
That is where you teach that developer to rebase his commits into logical and self-contained units before merging the PR.
That seems like significantly more work for marginal (if any) benefit over just having a single squash commit per PR.
I don't think I've ever found myself in a situation where I needed more granularity than the PR level when looking back through the history of a repo.
What are the situations where this is actually useful enough to make it worth the effort?
I don't think I've ever found myself in a situation where I needed more granularity than the PR level when looking back through the history of a repo.
What are the situations where this is actually useful enough to make it worth the effort?
I've used it many times. Not just for my changes, but over others' changes. In CMake, we rewrite branches heavily to keep a sensible history within MRs. Not all changes make sense landing commit-by-commit yet also work as a single commit. It also allows for much easier reverting of specific changes in case some part of a topic needs removed.
Some examples:
- https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9486...
One commit to improve messages; another to add a test case that also uses these messages. Forcing separate CI runs for these dependent commits doesn't make sense, but they also don't belong in a single commit.
- https://gitlab.kitware.com/cmake/cmake/-/merge_requests/8996...
Adds two test cases for a regression and then finally reverts the specific regression-exposing commit from https://gitlab.kitware.com/cmake/cmake/-/merge_requests/8197... while keeping the still-good parts. If the 8197 merge had been squashed, one would have had to manually bisect the hunks to find out which one actually caused the problem.
Some examples:
- https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9486...
One commit to improve messages; another to add a test case that also uses these messages. Forcing separate CI runs for these dependent commits doesn't make sense, but they also don't belong in a single commit.
- https://gitlab.kitware.com/cmake/cmake/-/merge_requests/8996...
Adds two test cases for a regression and then finally reverts the specific regression-exposing commit from https://gitlab.kitware.com/cmake/cmake/-/merge_requests/8197... while keeping the still-good parts. If the 8197 merge had been squashed, one would have had to manually bisect the hunks to find out which one actually caused the problem.
> It also allows for much easier reverting of specific changes in case some part of a topic needs removed.
I guess I struggle to see where reverting entire commits makes more sense than just deleting the offending code in a new commit.
I guess I struggle to see where reverting entire commits makes more sense than just deleting the offending code in a new commit.
Even if that were the case, being able to bisect using `git bisect` over the logical hunks instead of having to manually bisect over them can help determine which code needs deleted without going overboard.
I guess that one disadvantage is that PR's are not a git feature, but a feature of the git provider, so you might get locked in with a provider, I think you can export your PR history, but that means the provider where you are migrating needs to support also that feature.
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.
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...
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...
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.
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.
They explicitly call out not wanting to do this in the blog post.
I didn't notice that they were referring to that feature, it's too late to delete my comment now...
No worries, I won’t tell anyone :p
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.
[deleted]
[deleted]
[deleted]
> 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.
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.
> 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.
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.
> 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
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
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.
> 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, ...?
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, ...?
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.
... then point to your commits in the curl repo on Github?
I'm not expressing a preference here; this isn't a problem I have. But I do also suspect that a project with as high a profile as curl sees a lot of trivial PRs with less intent to improve the software, than to have the merge show up on the contributor's behalf in Github's various recruiter-friendly metrics.
How that intent plays with Git as Git is done, versus how Github does it, I have no idea. But the existence of the article, and the perspective toward which it seems to be written, suggest to me the answer is "not well" and that Stenberg et al may be seeing the same questions and complaints frequently recurring about this.
How that intent plays with Git as Git is done, versus how Github does it, I have no idea. But the existence of the article, and the perspective toward which it seems to be written, suggest to me the answer is "not well" and that Stenberg et al may be seeing the same questions and complaints frequently recurring about this.
But that's my point, what are these metrics that supposedly don't show the contributions anymore? Because the (IMHO) important ones all still show it, because they go by commit authorship in the repo, so you still get listed in the important places and can point to them.
Well, fair enough: as I said, I don't know. The commenter with whom you originally raised the question can probably answer better there.
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.