Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Why curl closes PRs on GitHub

daniel.haxx.se

Post reply on HN
Print view View original

11–20 of 41 posts

Re: Why curl closes PRs on GitHub

#11

Post by mathstuf »

7bit wrote: post #2

> 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 contr…

> 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

Re: Why curl closes PRs on GitHub

#12

Post by detaro »

throwanem wrote: post #6
detaro wrote: post #4

Earlier quoted context omitted.

> 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, ...?

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?

Re: Why curl closes PRs on GitHub

#13

Post by throwanem »

detaro wrote: post #12
throwanem wrote: post #6

Earlier quoted context omitted.

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.

Re: Why curl closes PRs on GitHub

#14

Post by zdw »

ryukoposting wrote: post #10

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…

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.

Re: Why curl closes PRs on GitHub

#15

Post by detaro »

throwanem wrote: post #13
detaro wrote: post #12

Earlier quoted context omitted.

... 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…

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.

Re: Why curl closes PRs on GitHub

#16

Post by throwanem »

detaro wrote: post #15
throwanem wrote: post #13

Earlier quoted context omitted.

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…

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.

Re: Why curl closes PRs on GitHub

#17

Post by gus_massa »

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.

Re: Why curl closes PRs on GitHub

#18

Post by rany_ »

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.

Re: Why curl closes PRs on GitHub

#19

Post by vault »

mathstuf wrote: post #9
zabil wrote: post #8

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?

Re: Why curl closes PRs on GitHub

#20

Post by dpkirchner »

vault wrote: post #19
mathstuf wrote: post #9

Earlier quoted context omitted.

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.
Post reply on HN