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

Large pull requests slow down development

graphite.dev

Post reply on HN
Print view View original

71–79 of 79 posts

Re: Large pull requests slow down development

#71

Post by 0x6c6f6c »

appplication wrote: post #47
HDThoreaun wrote: post #40

Earlier quoted context omitted.

I mean this is mostly just a devops issue though no? PR’s used to take a solid week to merge at my company with a 10 hour CI that had a ~50% success rate when the code worked. But we put a bunch of resources toward build times and ci run time and now it mostly works. Yea, you can circumvent a shitty pr system by just making massive pr’s but wouldn’t it be easier to have the devops team fix your broken system?

> devops team Ah… about that

Basically the top antipattern in the book

Re: Large pull requests slow down development

#72

Post by ckdarby »

mvdtnz wrote: post #51
ckdarby wrote: post #18

I'm writing a book, "Bytes Of Wisdom: From Start To Merged" that focuses around code reviews. I agree that larger PRs can increase the latency of getting feedback and getting quality feedback. I also have another view. Most code reviews struggle not from the reviewing part and shockingly not the authoring part, but the laying groundwork part. The laying the groundwork part is before any hands to the keyboard to write…

I agree with you for significant changes. But most day to day programmers are working on large and/or mature codebases, and I would think that this level of groundwork should not be required for a typical change. Indeed I would hope I don't need to bring multiple stakeholders along on the journey in order to add a couple of columns to a report or upgrade an API schema version.

I understand and it is a bit of a struggle I'm facing in the writing to have the reader believe that a large part of what is holding them back is this very thing. A lot of my technical audience has the sense of, I should just be able to make my change and shouldn't need multiple eyes along the journey for feedback.

I had an experience very similar to your, "just add a couple of columns". That experience which I'll explain was the final straw to getting me to start drafting the book.

An individual at an enterprise company made a PR to change the code from handling a single item to a list of items. Of course this showed promising performance because it reduced a bunch of the overhead operations that used to be O(N) down to some sub N amount.

It took me about 3 hours to fully understand the surrounding systems. At that point I did not review the implementation, BUT instead the problem. A system was falling behind and timing out. This implementation did help fix the issue by batching API calls. While I as the reviewer did the "laying groundwork" portion I realized that a lot of the API calls themselves did not need to be triggered in the first place.

Looking purely at the implementation for review was a LGTM (Looks Good To Me) kind of review because I could not weigh out the solutions to the problem without the information on the systems to understand where the problem could be.

Re: Large pull requests slow down development

#73

Post by CuriousCosmic »

mock-possum wrote: post #70
sb8244 wrote: post #36

Earlier quoted context omitted.

I agree big time with this. Organizing by clean commits is definitely important.

Why’s that? Do you step through commits diff by diff when reviewing?

This is actually the intended way of using git. Pick any random mailing list of the lore [1] and select a thread that has [PATCH] in the name to see this in action.

I just grabbed a random patchset off of the git mailing list and linked it below [2] to demonstrate this.

You'll notice that on top of the overall patchset (equivalent to a PR) having a detailed description (in the coverletter, i.e. PATCH 00/xx), each commit has a descriptive name, message detailing what the changes within do, and signoffs by everyone who contributed to it.

Then as each reviewer comes in, they review each individual patch (i.e. commit) separately, replying to the message for that patch. And any high level concerns can be in reply to the coverletter (addressing the entire patchset).

As the contributor responds to comments and makes the requested changes, those changes get made per patch/commit via rebasing rather than just added on top. And when they are finished making the revisions, a new v2 patchset is released in reply to the cover letter of the first patchset, now also containing a diff per commit/patch against the previous revision.

Then the cycle repeats until everyone is happy. At that point the maintainer will merge the changes into their incubation branch (the git devs call theirs `next`) and after some time has passed, they merge it into master/main and it becomes established history.

The worst part of the whole workflow is that it uses email but other than that it is a far preferable reviewing experience to using github's stock pull request workflow.

1. https://lore.kernel.org/

2. https://lore.kernel.org/git/20231010123847.2777056-1-christi...

Re: Large pull requests slow down development

#74

Post by mewpmewp2 »

CuriousCosmic wrote: post #69
mewpmewp2 wrote: post #66

Earlier quoted context omitted.

But if you are doing by commit and it later gets iteratively refactored, wouldn't that be a huge waste of time? How would you know what to critically evaluate in such case?

I'm confused what you mean? If you are talking about refactoring prior to merging into the tree? then no it's not a waste of time. That's the intended workflow. You make your changes to the commits or add new commits in between using rebase. This is how all the development for linux is done. If you are talking about after they are merged into the mainline? That's also not a waste of time. You don't have to go back an…

I mean for example if I need to create a new feature:

1. I open a branch.

2. I start coding.

3. During coding I do various commits.

4. I realise I need to refactor/rethink something, I do more commits, overriding the code/ideas of previous commits.

5. Then I finally put it up for code review. But in such case there's no point in going through all commits, since a lot of that gets changed anyhow.

6. Usually I would squash everything into a single commit and merge after doing that. Because a lot of my commits would've been pointless anyway.

What is the suggestion exactly?

Re: Large pull requests slow down development

#75

Post by AlotOfReading »

withinboredom wrote: post #64
AlotOfReading wrote: post #49

Earlier quoted context omitted.

Depends on what your code does. A lot of my code runs robots. You can't scale horizontally because you can only fit so many systems in a room before you run afoul of production limits, safety requirements or cooling capacity. You can't scale vertically because zoning. You can't run the tests faster because physics. So, you either run the expensive tests asynchronously and all that entails or you allow CI to take fore…

In that case, would it make more sense to have a small PR that you start with, get it reviewed, then open smaller PRs to your original PR? That way your approach/code can be reviewed while in your larger PR you tweak and fix tests.

Not as a (good) general solution. Some things are just hard to break down into smaller logical units. E.g. a complicated new driver might introduce thousands of lines of changes. Half a driver usually won't compile, but you could break things down into PRs over individual functions (in call order so there aren't compiler warnings). That's a wildly unnecessary amount of effort though. You could write a skeleton driver first too, but that's rote declarations barely worth reviewing even as part of a larger PR.

Re: Large pull requests slow down development

#76

Post by mock-possum »

d_sem wrote: post #12

The key is to define the term "large" in this context. Pull requests size is an artifact of a humans brain ability to process changes. Therefore its best to tailor the content of a PR based on how difficult you think it is for the stakeholders you are merging can understand the change. "large" is more a measure of complexity. When I make changes to a codebase that very few people are experts in and is generally perce…

Yeah I will always advocate for smaller, more easily digestible PRs - but the underlying principle is good old “know your audience.”

Re: Large pull requests slow down development

#77

Post by CuriousCosmic »

mewpmewp2 wrote: post #74
CuriousCosmic wrote: post #69

Earlier quoted context omitted.

I'm confused what you mean? If you are talking about refactoring prior to merging into the tree? then no it's not a waste of time. That's the intended workflow. You make your changes to the commits or add new commits in between using rebase. This is how all the development for linux is done. If you are talking about after they are merged into the mainline? That's also not a waste of time. You don't have to go back an…

I mean for example if I need to create a new feature: 1. I open a branch. 2. I start coding. 3. During coding I do various commits. 4. I realise I need to refactor/rethink something, I do more commits, overriding the code/ideas of previous commits. 5. Then I finally put it up for code review. But in such case there's no point in going through all commits, since a lot of that gets changed anyhow. 6. Usually I would sq…

Ohhh yeah. My workflow is:

1. I open a branch. i.e. `ID-XXXX-branch_name-working`.

2. I start coding.

3. I make a ton of quick tiny commits. (I generally label these commits "NO-MERGE: ").

4. I make a bunch of changes.

5. I finally get everything done.

6. I now create a new branch `ID-XXXX-branch_name` from `ID-XXXX-branch_name-working`.

7. I rebase that branch to get my code cleanly formatted by commit with each discrete feature or change getting its own commit.

8. My code goes up for review.

9. I get changes requested.

10. I make a new branch `ID-XXXX-branch_name-v2-working` from `ID-XXXX-branch_name`.

11. I make the requested changes as a bunch of new small "NO-MERGE: " commits.

12. I am now ready for re-review.

13. I now create a new branch `ID-XXXX-branch_name-v2` from `ID-XXXX-branch_name-v2-working`.

14. I rebase those NO-MERGE changes into my "presentable" commits, adding or removing well documented commits as necessary.

15. I now send out my v2 revision to the mailing list or I change the HEAD of my PR from `ID-XXXX-branch_name` to `ID-XXXX-branch_name-v2`. If I'm using a PR workflow, I link a diff between the two revision branches (you can do this in github using `https://github.com/org/repo/compare`). That isn't necessary with patchsets since I can easily do a range diff there. I suppose I could do a cover letter with range diff and paste it to github but it somehow doesn't seem as nice.

16. Rinse repeat steps 8-15 as necessary for each new revision.

17. "LGTM"

18. Merge into `main`/`master` (like actual merge, not rebase or squash merge) and close PR.

19. Clean up branches. Either save them somewhere for prosperity if you have trust issues like me or just delete them.

This looks like a lot but I was trying to be as detailed about that workflow as I can be. Realistically it's not so bad and you can get a hold of it very quickly.

Also with regards to rebasing changes into well documented, discrete commits, if when you are doing your development you make your commits small and self contained, with `git rebase -i` you can actually just reorder the list of commits to chunk together the related commits and 99% of the time it'll rebase with little to no merge conflicts. Then you can just squash those chunks down into your presentable commits. This also applies to your v2 changes and on. You can just move those commits in the rebase TODO to put them after the commit you want to squash/fixup them into and if they are small clean changes, they should rebase without conflict. Things only get nasty and break when your commits are spanning multiple unrelated files and you try to break those up.

I'd estimate rebasing new changes into an already documented set of commits probably takes me 1-2 minutes on average so I consider it well worth the extra 30 minutes spent over the course of the week.

Re: Large pull requests slow down development

#78

Post by eru »

mewpmewp2 wrote: post #68
eru wrote: post #4

Earlier quoted context omitted.

> [...] but reality is there are a lot of smart devs who are "big thinker" types and struggle with incremental development and lots small PRs towards a bigger goal. It doesn't really matter. You can start with a big change initially as a 'big thinker'. You just have to break it down afterwards. I often have a bit of feature creep when working on a change, and add all kinds of incidental fixes I find along the way. Bu…

> It doesn't really matter. You can start with a big change initially as a 'big thinker'. You just have to break it down afterwards. But that's extremely difficult I think and requires some out of the box extreme creative thinking on how to split it up after the fact. And I would also think it doesn't help at all. I usually like to imagine how I build the new feature in head. Then I vomit out whole bunch of code, but…

> But that's extremely difficult I think and requires some out of the box extreme creative thinking on how to split it up after the fact.

Thanks for the compliments, but I don't think I'm such a genius, and I manage this regularly. Splitting gets easier with practice.

It's the same as writing any technical document or a scientific paper: you have some idea, and then you decide how to split up the presentation so that your readers can make sense of it.

> E.g. if I split it in a way where I only show shared components, it won't be understandable why I made them shared in certain way because you won't see the actual other logic that uses them, so reviewers are kind of left to trust that these will be used in later PRs.

You can explain that with PR descriptions. Or you can have multiple clean-up commits in a single PR.

> A reviewer won't have understanding of the reasoning without going through the process themselves on how something might be reused.

You are allowed to tell them. That's what the PR description and code comments are for.

(It's better to put as much as is reasonable into code comments, because they are easier to see for future folks trying to understand the code. And, of course, even better is to make the code self-explanatory, where feasible. It's a hierarchy of descriptions.)

> And then if they were to criticise my shared logic, and they want me to change anything, which could even be a change they request because of misunderstanding I will have to change all the other logic down the other PRs as well.

That's always a problem, if you make a big change and only give it to review afterwards. No matter how you present that to the reviewer.

Yes, it is easier to write code that incorporates review feedback, if you can get review feedback early.

Re: Large pull requests slow down development

#79

Post by gavmor »

mvdtnz wrote: post #52
gavmor wrote: post #38

Earlier quoted context omitted.

Well that sounds like a good read! I've been privileged to spend my 10+ years writing software among pair-programmers, and have never been made to suffer from this "code review" ritual. Maybe it's not so bad? I'd love to read a book about it before my luck runs out and somebody foists it upon me. Just to be prepared, you know. Seems to me like it just can't work. Show a programmer a 5 line program, as the adage goes,…

Code review is not a "ritual". Our code is our output, and it is paid work. Most professions have some level of review. Go speak to a chartered accountant or an engineer and complain about the "ritual" of peer review and see how far you get. We are professionals and it's about time we started acting like it.

Of course code review is a ritual. All professionalism is. We establish norms and rites the performance of which increases shared understanding and predictable outcomes. I am not sure I understand what malign connotation this word carries for you.
Post reply on HN