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

Reorient GitHub pull requests around changesets

mitchellh.com

Post reply on HN
Print view View original

201–210 of 210 posts

Re: Reorient GitHub pull requests around changesets

#201

Post by Joky »

jen20 wrote: post #185
mikemcquaid wrote: post #136

Earlier quoted context omitted.

I left GitHub earlier this year after a decade. I’ve seen mockups, hack week projects and proof of concepts of this for the last 5 years (at least). A lot of engineers there knew this is the future that PRs need but GitHub at this point seems organisationally incapable of delivering these sorts of large improvements (Microsoft is perhaps partly but definitely not wholly to blame for this). Instead, they are midway th…

This would explain something else too, probably: a few years ago I did a call with some GH folks talking about the idea of making the commit message applied to a squash merge part of the review itself. Apparently this was very common feedback and I know that at least five other people who were maintaining large scale open source at the time gave it that week too. It’s never gone anywhere though, and as a result I hav…

You can configure "squash&merge" to use the PR title and description for the commit message now, which makes it reviewable!

Re: Reorient GitHub pull requests around changesets

#202

Post by u801e »

jen20 wrote: post #187
akoboldfrying wrote: post #107

Earlier quoted context omitted.

I don't understand the appeal of squash commits. AFAICT, the only advantage of squash commits is a nice "linear history", but you can get that from regular old merge commits just by adding --first-parent to your `git log` command -- and you still have that additional detail available on the second-parent commit chain if you need it. As soon as you start squashing commits, you throw away a very useful guarantee git gi…

The point is it enables bisect workflows that don’t suck. If half the commits in a tree are “fix misc derp” or “test passes now”, that workflow isn’t possible. The short term and long term view of the work should be different.

> The point is [the squash commit method] enables bisect workflows that don’t suck. If half the commits in a tree are “fix misc derp” or “test passes now”, that workflow isn’t possible.

The problem with squash commits is that they tend to make changes to more files and make changes to more lines in a given file. This makes it harder to revert changes after more changes have been applied to the main branch. In contrast, organizing commits into a set of logical changes necessary to implement a feature in a given branch makes it much easier to revert one of those commits even after other feature branches have been merbed into the main branch because the commits affect one or just a few files and not many changes in a file).

Re: Reorient GitHub pull requests around changesets

#203

Post by u801e »

jen20 wrote: post #186
mmcnl wrote: post #171

Earlier quoted context omitted.

That's a better solution than changing the Git(Hub) model. If this works for you, then why the need to change anything?

Because squash merging results in horrifically poor commit messages that are use to no one in future, because GitHub can’t be bothered to implement anything beyond a simple textarea for editing them.

Not only that, they also result in commits that change a lot of files and make many changes in each file. Try to revert one of those commits is difficult after other changes have been merged into the main branch.

Re: Reorient GitHub pull requests around changesets

#204

Post by cyphactor »

I am personally a big fan of a patch stack style workflow.

I have created a tool called Git Patch Stack, https://git-ps.sh which makes it easier to manage a stack of patches, request review of them, re-request review of them, and many other features.

Checkout the the site and the documentation as it explains a lot. But if you have any questions feel free to join our Slack group and ask away.

Re: Reorient GitHub pull requests around changesets

#205

Post by jen20 »

u801e wrote: post #202
jen20 wrote: post #187

Earlier quoted context omitted.

The point is it enables bisect workflows that don’t suck. If half the commits in a tree are “fix misc derp” or “test passes now”, that workflow isn’t possible. The short term and long term view of the work should be different.

> The point is [the squash commit method] enables bisect workflows that don’t suck. If half the commits in a tree are “fix misc derp” or “test passes now”, that workflow isn’t possible. The problem with squash commits is that they tend to make changes to more files and make changes to more lines in a given file. This makes it harder to revert changes after more changes have been applied to the main branch. In contras…

Indeed, but trying to revert lots of tiny commits in one unit is no better.

Really it just requires discipline to either make entire related changes as patches or pull requests, which doesn’t matter.

The result of a pull request should be the equivalent of having built a single patch to start with, just with better review tooling.

Re: Reorient GitHub pull requests around changesets

#206

Post by u801e »

jen20 wrote: post #205
u801e wrote: post #202

Earlier quoted context omitted.

> The point is [the squash commit method] enables bisect workflows that don’t suck. If half the commits in a tree are “fix misc derp” or “test passes now”, that workflow isn’t possible. The problem with squash commits is that they tend to make changes to more files and make changes to more lines in a given file. This makes it harder to revert changes after more changes have been applied to the main branch. In contras…

Indeed, but trying to revert lots of tiny commits in one unit is no better. Really it just requires discipline to either make entire related changes as patches or pull requests, which doesn’t matter. The result of a pull request should be the equivalent of having built a single patch to start with, just with better review tooling.

> Indeed, but trying to revert lots of tiny commits in one unit is no better.

You shouldn't have to revert a lot of tiny commits if the bug is due to just one of those commits.

> The result of a pull request should be the equivalent of having built a single patch to start with, just with better review tooling.

In my experience, features take more than one commit to implement. The merge commit provides a way to group those multiple commits so that you can see all commits that went into implementing a feature.

If you're trying to make pull requests that are like small commits, then a feature branch becomes

    first-commit
    first-pr-merge
    second-commit
    second-pr-merge
    third-commit
    third-pr-merge
    ...
    nth-commit
    nth-pr-merge
with commits and their associated merge commits for other features interspersed with your set of commits.

Which basically introduces a lot of merge commits (effectively doubling the number of commits) where the first parent is the previous merge commit and the second parent is the single commit for that pull request. You have no way to really group related commits that were used to implement a feature since there's no merge commit that groups them all together. In that case, you could halve the number of commits by dispensing with merge commits entirely and just adding the #PR-number in the commit message to link to the PR discussion.

Re: Reorient GitHub pull requests around changesets

#207

Post by codeapprove »

jayroh wrote: post #53
codeapprove wrote: post #19

I agree 1000%. I’m the creator of what I believe is a better review interface for GitHub ( https://codeapprove.com ) but there are also many others: * CodeApprove (codeapprove.com) * Graphite (graphite.dev) * Reviewable (reviewable.io) * Axolo (axolo.co) * Viezly (viezly.com) * Mergeboard (mergeboard.com) * Codestream (codestream.com) * Pullpo (pullpo.io) * ReviewPad (reviewpad.com) * Planar (useplanar.com) * Visibly…

Question: does CodeApprove place related files in closer proximity during review? I would _love_ to have a class and its test next to each other instead of sorted alphabetically. I'm tired of jumping around all over the place, trying to traverse through my review thought process.

CodeApprove does not have that feature because we don't currently do any smart code parsing to understand what would make a file "related". I think Viezly (https://viezly.com/) does the best job at that.

Re: Reorient GitHub pull requests around changesets

#208

Post by lozenge »

fahhem wrote: post #179
lozenge wrote: post #176

Earlier quoted context omitted.

I agree as I used a tool Reviewable which is accurate about which version of the change is being commented on, which files you reviewed etc. It even supported rebases. And no comment was finally marked resolved until the original author marked it as such. It was great for skilled users to navigate with the keyboard and easy to see when everything was resolved. But if used as intended, like fixing some commented chunk…

We're still around and we've actually changed a lot. Our homepage is untouched (for now!) but you can check out our blog for proof of life :D http://blog.reviewable.io/

Sorry, I was going off the screenshot in the home page. It's just how I remember it.

Re: Reorient GitHub pull requests around changesets

#209

Post by juped »

atq2119 wrote: post #125
juped wrote: post #50

I love the amazing, advanced submission and review workflow you can see if you click anything with PATCH in it at https://public-inbox.org/git/ . The best part is that it's fully integrated with Git! It's distressing to me that Github spends so much money making Git worse. (There's some good parts of their whole product lineup, but the Git integration is supposed to be the centerpiece.)

I agree that this is how development should work. But let's be fair: The claim that this "is fully integrated with Git" is at least misleading. Yes, there's git format-patch, git send-email, and git am. But what I would really like to see in that link you shared is links that go directly to commit hashes that I can git fetch locally to see a patch or patch set in context; and links between different versions of a pat…

Patches aren't to specific revisions, that's the Github straitjacket talking. They're patches.

They often are to specific blobs; those are recorded. So if they don't apply, use am -3.

Specific revisions are Junio Hamano's job, not a patch submitter's.

Re: Reorient GitHub pull requests around changesets

#210

Post by 20after4 »

jacobegold wrote: post #75
neo2006 wrote: post #69

I often stack PRs to emulate the practice described by Mitchell but it's not ideal as if you need to change an underlying PR l, you need to rebase all of the dependent PRs.

There are tools that solve this problem! I work on one (Graphite), but there's also plenty of others like git-branchless and Sapling. All three of these are inspired by Facebook's internal fork of Mercurial (with Phabricator/"Diffs" for reviewing) -- Google has a similar model with Piper/Critique CLs, with Gerrit as the open source result.

So Facebook is still using Phabricator? Somehow I suspected that they had abandoned it since they seemingly stopped being involved in the open source Phabricator project.
Post reply on HN