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

31–40 of 41 posts

Re: Why curl closes PRs on GitHub

#31

Post by boolemancer »

account42 wrote: post #29
slimsag wrote: post #24

Earlier quoted context omitted.

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.

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?

Re: Why curl closes PRs on GitHub

#32

Post by mathstuf »

boolemancer wrote: post #31
account42 wrote: post #29

Earlier quoted context omitted.

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

Re: Why curl closes PRs on GitHub

#33

Post by corytheboyd »

rany_ wrote: post #30
corytheboyd wrote: post #26

Earlier quoted context omitted.

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

Re: Why curl closes PRs on GitHub

#34

Post by boolemancer »

mathstuf wrote: post #32
boolemancer wrote: post #31

Earlier quoted context omitted.

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'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/948…

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

Re: Why curl closes PRs on GitHub

#35

Post by rezonant »

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.

Re: Why curl closes PRs on GitHub

#36

Post by phoenix616 »

rany_ wrote: post #18

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.

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.

Re: Why curl closes PRs on GitHub

#37

Post by fahhem »

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

Re: Why curl closes PRs on GitHub

#38

Post by norman784 »

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?

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.

Re: Why curl closes PRs on GitHub

#39

Post by mathstuf »

boolemancer wrote: post #34
mathstuf wrote: post #32

Earlier quoted context omitted.

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/948…

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

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.

Re: Why curl closes PRs on GitHub

#40

Post by MaskRay »

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

Post reply on HN