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

131–140 of 210 posts

Re: Reorient GitHub pull requests around changesets

#131

Post by u801e »

akoboldfrying wrote: post #112

It's surprising to me that GitHub currently doesn't attach review comments to specific commits, but only to timestamps, and I agree that it would be great to improve that. But I don't understand how a contributor can feel pressure to address all of the reviewers' comments in a single commit: They can commit as many times as they want, and only push when they feel it's ready. And in the case where they may want to off…

> It's surprising to me that GitHub currently doesn't attach review comments to specific commits

It is possible to comment on commits in github (you can do it by clicking on the sha1 of the commit and then making a comment on a line in the diff). But the comment won't show up in the main PR diff window.

> But I don't understand how a contributor can feel pressure to address all of the reviewers' comments in a single commit: They can commit as many times as they want, and only push when they feel it's ready.

Unfortunately, this leads to a lot of fixup commits in the branch that muddle up the history. A changeset consists of one or more commits where each makes one logical change where the what was done and why it was done that way are detailed in the commit message.

Re: Reorient GitHub pull requests around changesets

#132

Post by u801e »

jacobegold wrote: post #96
Aeolun wrote: post #28

Hmm, not saying Github is perfect, but I think there’s value in providing the simplest possible experience as the default.

I was lucky enough to work at a company with a great code review tool at one of my first positions -- and I am nowhere near convinced that GH's review interface is the simplest possible. Of course, everyone (including myself!) is probably biased to think that whatever they are used to is simplest, tbh.

> I am nowhere near convinced that GH's review interface is the simplest possible.

It wasn't initially designed with code review in mind (unlike systems like phabricator, gerrit, review board, etc).

Re: Reorient GitHub pull requests around changesets

#133

Post by da39a3ee »

u801e wrote: post #130
noirscape wrote: post #37

I'm pretty sure this approach is somewhat similar to the mailing list git approach, where patches usually get submitted once ready and then changed depending on feedback as a wholly new submitted patch (as part of a broader conversation). It'd be a useful thing to import without having to bring in the whole charade of using email and mailing lists (which most mail clients tend to be very unfavorable of in general now…

> It'd be a useful thing to import without having to bring in the whole charade of using email and mailing lists (which most mail clients tend to be very unfavorable of in general nowadays) This is a misconception. git itself has commands (git-format-patch and git-send-email) that automate the creation of patches and sending changeset email threads to the mailing list. The only thing one needs to do is set the approp…

No, it is not a misconception. How do I apply a patch from gmail to a specific git repo on my computer? (I would genuinely like to know the answer, but it must not involve mutt, gnus, or dovecot!)

Re: Reorient GitHub pull requests around changesets

#134

Post by da39a3ee »

XorNot wrote: post #110
zerbinxx wrote: post #105

Earlier quoted context omitted.

One practice I’ve gotten into with juniors is checking out their PR, branching it, PR-ing to their PR, and then having the discussion about what I want them to change there. This is good because it lets us isolate certain issues (and truly resolve them) rather than comments getting pulverized by lots of little commits, only to have someone else come in and say “LGTM!” and merge it with unresolved issues. Another obvi…

I like this approach too, but almost no one understands it (i.e. the idea it's possible is foreign) and UI support for it is non-existent in all the major products. It would be wonderful if we could ditch "change these lines" type comments in favour of just letting merge requests with the changes be easily surfaced.

I might be wrong but it sounds like you don't know about the GitHub PR feature where you select the lines, and GitHub allows you to edit them, and posts a diff when you're done, and if the author accepts it out becomes a jointly authored commit.

Re: Reorient GitHub pull requests around changesets

#135

Post by da39a3ee »

stefan_ wrote: post #36
oldtownroad wrote: post #10

I agree. As a way to minimise the pain on GitHub today, we disallow force pushing and enforce squash merging. Force pushing is a nightmarish behaviour, once a Pull Request is opened the branch must be append only.

So you have endless "Fix a" "Typo" "fixup" "revert redo" "add y missed in z" commits and then the squash pushes all that crap into the commit message for whatever the final mess will be? Here is a random series from DRM patchwork: https://patchwork.kernel.org/project/dri-devel/list/?series=... Would you squash that? Hell no, of course not. You would be mixing atomic changes in different subsystems. It breaks bisect a…

> Honestly, squash merges are frequently a sign of people who lack mastery in Git itself or make no effort to produce high-quality independent commits.

Otherwise known as "colleagues" :)

Re: Reorient GitHub pull requests around changesets

#136

Post by mikemcquaid »

Vendan wrote: post #127

Github had this planned in their old roadmap... But then they deleted it... https://web.archive.org/web/20220831234107/https://github.co...

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 through porting Rails views to use React, keeping most pages looking identical while introducing bugs and regressing previous usability improvements on a weekly basis. A real shame.

Re: Reorient GitHub pull requests around changesets

#137

Post by SleepyMyroslav »

a-dub wrote: post #128

it used to be worse. if you pushed changes that removed lines that had review comments on them, the review comments would simply disappear. but yes, i liked perforce too.

Corporate devs are still in perforce paired with some web review tools. It is a big cultural problem for a lots of devs out there that they are not familiar with open source flows and tools.

Re: Reorient GitHub pull requests around changesets

#138

Post by als0 »

mikemcquaid wrote: post #136
Vendan wrote: post #127

Github had this planned in their old roadmap... But then they deleted it... https://web.archive.org/web/20220831234107/https://github.co...

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…

Wow, why would they spend so much energy rewriting Rails code into React?

Re: Reorient GitHub pull requests around changesets

#139

Post by JohnKemeny »

da39a3ee wrote: post #133
u801e wrote: post #130

Earlier quoted context omitted.

> It'd be a useful thing to import without having to bring in the whole charade of using email and mailing lists (which most mail clients tend to be very unfavorable of in general nowadays) This is a misconception. git itself has commands (git-format-patch and git-send-email) that automate the creation of patches and sending changeset email threads to the mailing list. The only thing one needs to do is set the approp…

No, it is not a misconception. How do I apply a patch from gmail to a specific git repo on my computer? (I would genuinely like to know the answer, but it must not involve mutt, gnus, or dovecot!)

Copy the content of the email to your clipboard, type `git am` in your terminal, paste and C-d to complete.

Re: Reorient GitHub pull requests around changesets

#140

Post by sammorrowdrums »

da39a3ee wrote: post #133
u801e wrote: post #130

Earlier quoted context omitted.

> It'd be a useful thing to import without having to bring in the whole charade of using email and mailing lists (which most mail clients tend to be very unfavorable of in general nowadays) This is a misconception. git itself has commands (git-format-patch and git-send-email) that automate the creation of patches and sending changeset email threads to the mailing list. The only thing one needs to do is set the approp…

No, it is not a misconception. How do I apply a patch from gmail to a specific git repo on my computer? (I would genuinely like to know the answer, but it must not involve mutt, gnus, or dovecot!)

In the worst case you can copy paste a patch into a new local file in the repo and then apply it with git from there. I’ve had somebody slack me patches before and it is not a big lift.
Post reply on HN