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

101–110 of 210 posts

Re: Reorient GitHub pull requests around changesets

#101

Post by piotrkaminski »

avita1 wrote: post #41
stefan_ wrote: post #36

Earlier quoted context omitted.

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…

> 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? In Github at least you can set the behavior to take the PR description by default as the squashed commit message. In fairness this is not the default. The default behavior for squash merges is to ask for a new commit message right…

> That said, one of my favorite features from gerrit at a past job was that the commit message itself could be reviewed.

Reviewable lets you review your commit messages just like any other file, BTW! (Disclosure: I'm the founder.)

Re: Reorient GitHub pull requests around changesets

#102

Post by epolanski »

RHSeeger wrote: post #83
epolanski wrote: post #62

Jm2c, but by far the best place I've ever worked (my current client) we don't do code reviews at all unless the author wants feedback. PRs are good ways to defend your code base from bad code, and they were born in open source where you literally have no clue who the contributor is, but years of experience left me convinced that I don't want such a system where there's constant need to overview each other's work. I w…

I trust and respect the people I work with. I also prefer to have code reviews, even (especially) of my own code. Code reviews can point out code that - Doesn't consider an edge case (NPEs being the most common) - Doesn't match up well with the "standard" way we do things (and, all things being equal, using the same approach in every place improves maintainability) - Has a simpler approach - Doesn't have a comment wh…

None of the things you said matters if your issue is time to market or you have little business.

Who cares about an edge case no user's gonna see?

Who cares about an edge case for a non business critical feature?

Who cares if there was a better way to implement something if it's not a priority?

Our work it's not meant to give us devs intellectual challenges but solve people's problems.

All the things you list are reasonable if you're making money or they are core to the product or they impact user's safety in any way.

Otherwise they don't matter at all.

And half the things you list are solved by having high hiring standards and paying people well.

Stuff may not be perfect, maybe there will be better ways, maybe it will be inconsistent at times but it will be more than good enough and move the business forward at much higher speed. Also, just to reiterate I've never stated that we don't have standards or make architectural decisions or don't give feedback. I merely stated that reviewing PRs for us is an exception, not a rule and that I prefer a system of trust.

Re: Reorient GitHub pull requests around changesets

#103

Post by devin-sills »

epolanski wrote: post #62

Jm2c, but by far the best place I've ever worked (my current client) we don't do code reviews at all unless the author wants feedback. PRs are good ways to defend your code base from bad code, and they were born in open source where you literally have no clue who the contributor is, but years of experience left me convinced that I don't want such a system where there's constant need to overview each other's work. I w…

Same experience here. Productivity unlocks like crazy, and amazingly the sky doesn't fall. It does require stringent hiring practices though and a flat, open culture. Oftentimes I don't need a formal syntactical process like a review but rather just need to bounce ideas off folks to get a good plan for a larger feature.

Re: Reorient GitHub pull requests around changesets

#105

Post by zerbinxx »

lifeisstillgood wrote: post #26

I am slowly convinced that comments on PRs are like comments on blog posts or youtube videos. Ephemeral, irrelevant and ineffective. If you really want to "reply", put up your own blog post or a "reacts" video. Same for code. unless it's simple typo fixes or improvements, deeper fixes come from writing code samples yourself. I recently commented on a juniors code, and put in about four lines of code showing how I wou…

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 obvious way to fix OP’s issue is to request developers to break their PR’s down into smaller chunks, pair on the parts that need help, and slowly merge things from there. YMMV of course - but any 1000 line PR is going to be a headache regardless of what methodology you’re using to review code.

Re: Reorient GitHub pull requests around changesets

#106

Post by bongobingo1 »

jacobegold wrote: post #93
fahhem wrote: post #88

Earlier quoted context omitted.

I agree, we work on Reviewable, which uses those same blobs. However, we add tags to commits we're referencing, just in case someone at GitHub gets around to implementing garbage collection!

They must have refs on the internal git servers -- they need them for the "force pushed from X to Y" timeline events

I've definitely clicked those links through to 404s.

Re: Reorient GitHub pull requests around changesets

#107

Post by akoboldfrying »

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.

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 gives you by default, which is that a given code change A is "in" a given branch B if and only if A is reachable from B. This guarantee is very helpful when debugging differences between versions, e.g., in figuring out which commits I've made locally are actually running in a shared environment like staging/UAT.

Re: Reorient GitHub pull requests around changesets

#108

Post by iimblack »

tmpX7dMeXU wrote: post #16
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.

I’ve been contemplating pulling the trigger on this one myself. I’m pretty much sold on it. I’ve been on board with squash merging for years. Best thing I’ve ever done for our project. I eventually came to the realisation that the majority of people who were against it were so because some purist greybeard had beat it into them.

Majority of people aren’t making atomic commits. In the off chance they are it’s not hard to switch from squash for those one-offs.

Re: Reorient GitHub pull requests around changesets

#109

Post by arwineap »

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.

If you're going to squash on merge, why would you not allow squashing on the branch?

Re: Reorient GitHub pull requests around changesets

#110

Post by XorNot »

zerbinxx wrote: post #105
lifeisstillgood wrote: post #26

I am slowly convinced that comments on PRs are like comments on blog posts or youtube videos. Ephemeral, irrelevant and ineffective. If you really want to "reply", put up your own blog post or a "reacts" video. Same for code. unless it's simple typo fixes or improvements, deeper fixes come from writing code samples yourself. I recently commented on a juniors code, and put in about four lines of code showing how I wou…

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.

Post reply on HN