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

The Theatre of Pull Requests and Code Review

meks.quest

Post reply on HN
Print view View original

341–350 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#341

Post by TeMPOraL »

pcthrowaway wrote: post #332
ViewTrick1002 wrote: post #310

Earlier quoted context omitted.

”Updated something tiny and ran CI again until it failed on some other step” Together with backing up your work. Sure you can keep amending your last commit but whenever you detour to another problem in the same PR that turns into a mess. Easier to just treat the PR as the atomic unit of work and squash away all that intermediate noise. It also ensures that CI will pass on every commit on the main branch.

A lot of engineers do what you suggest rather than `git add; git commit --amend` This is why commits are often noise. If people are using commits well, they tell a story. The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one

> The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one

What do you consider correct usage of git, and why? In this very discussion, I can see at least two distinct purposes that, more often than not, are mutually exclusive:

- To "tell a story" for other people

- To checkpoint units of work as individual perceives them, helping them deal with interruptions (which include running out of work day).

Storytelling is a skill in itself, doing it is a distinct kind of extra work, so you can't really have people use git for both at the same time. Which is where the whole commit history management idea comes from - it's to separate the two into distinct phases; first you commit for yourself, then you rework it to tell a story for others.

Re: The Theatre of Pull Requests and Code Review

#342

Post by BurningFrog »

jonahx wrote: post #317
William_BB wrote: post #256

Earlier quoted context omitted.

PR is the atomic level of work. I'd argue PR-level history (i.e. squash) is often enough and is way cleaner. Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.? If I want that granularity, I'd go read the original PR and the discussion that took place.

> PR is the atomic level of work. The atomic level of work should be a single, logically coherent change to the codebase. It's not managerial, it's explanatory. As you work things naturally arise. Over here a reformatted file, over there comments to clarify an old function that confused you, to help the next developer who encounters it. Cleaning, preparatory refactoring that is properly viewed as a separate action, a…

These opinions could use some arguments for why they're useful.

Re: The Theatre of Pull Requests and Code Review

#343

Post by bvrmn »

If you could make a good "story by commits" I assure you: a combined single commit would be as much readable as multiple. Good story is a result of composable changes highly visible without artificial splitting.

Funny thing I never seen a good PR with meaningful multiple commits. At least you need to be quite proactive in history rewrite to make it presentable. Usually it's mere fix/fix/fix looking amazingly bad without squash.

If you want to comment on this matter, you are welcome to share your github examples.

Re: The Theatre of Pull Requests and Code Review

#344

Post by herval »

1-more wrote: post #288
herval wrote: post #150

I find the sort of opinions on this post quite common on a subset of engineers - namely mid levels with some time in the career, who start to consider themselves senior engineers and want everyone to follow the same set of strict rules they decided make sense. It’s the same mindset that makes people pedantically apply DRY to every situation or forcing others to TDD basic apps. In practice: - smaller PRs aren’t necess…

> nobody reads intermediate commit messages one by one on a PR, period. Very common practice at my old company, and one I continue in my current role. > “every commit must compile” sucks ass for anyone else trying to rebase your branch onto the update main/master when they don't. Once your PR is out of "working on the feature" and into the "getting it merged" phase, do a little `git rebase -i` and squash your really…

> sucks ass for anyone else trying to rebase your branch onto the update main

why would anyone else rebase your branches? YOU should rebase your branches.

Re: The Theatre of Pull Requests and Code Review

#345

Post by imiric »

sfn42 wrote: post #313
singpolyma3 wrote: post #271

Earlier quoted context omitted.

If the commits don't matter why did you make them separate to begin with?

I'm not the person you asked, but I often don't. I personally don't care about git history - I read code not commit messages. I don't really care how it got the way it is I care about what it is, maybe once in a blue moon there could potentially be some useful information in a commit message but it's not enough. So, I'll often just make lots of changes and then commit them all at once with some vague commit message a…

> I personally don't care about git history - I read code not commit messages.

Honest question: why do you even use version control? What do you get out of it?

Based on your workflow, you could just as well not use it at all, and create zip files and multiple copies of files with names like `_final3_working_20250925`.

Change history is the entire point of version control. It gives you the ability to revert to a specific point in time, to branch off and work on experiments, to track down the source of issues, and, perhaps most useful of all, to see why a specific change was done.

A commit gives you the ability to add metadata to a change that would be out of place in the code itself. This is often the best place to describe why the change was done, and include any background or pertinent information that can help future developers—including yourself—in many ways. Adding this to the code base in a comment or another document would be out of place, and difficult to manage and discover.

You may rarely need to use these abilities, but when you do, they are invaluable IME. And if you don't have them at that point, you'll be kicking yourself for not leveraging a VCS to its full potential.

> I have tried to be more disciplined with commits and stuff like that but I find that it just slows me down and makes the work feel more difficult.

Of course it slows you down. Taking care of development history requires time and effort, but the ROI of doing that is many times greater.

I encourage you to try to be disciplined for a while, and see how you feel about it. I use conventional commits and create atomic commits with descriptive messages even on personal projects that nobody else will work on. Mainly because it gives me the chance to reflect on changes, and include information that will be useful to my future self, months or years from now, after I inevitably abandon the project and eventually get back to it.

Here is an example from a project I was working on recently[1]. It's practically a blog post, but it felt good to air my grievances. :)

[1]: https://github.com/hackfixme/sesame/commit/10cd8597559b5f478...

Re: The Theatre of Pull Requests and Code Review

#346

Post by m000 »

t-writescode wrote: post #159
hypeatei wrote: post #153

Earlier quoted context omitted.

git add --patch ...is your friend if you want to leave all your changes unstaged for awhile then break it out into multiple commits later.

To add, when I’m breaking my changes down into multiple parts for review, I tend to: * squash everything I’ve done into one commit * create a new branch off main/master that will be the “first commit” * cherry-pick changes (easy from some git guis) that represent a modular change. * push and make an MR from the new branch * rebase “the big commit” on top of the partial change. * wash, rinse and repeat for each change…

How about:

   * squash into one commit
   * git reset HEAD~1
   * git add -p
   * git commit -m commit1
   * repeat until no changes are left
   * add any file deletions/additions
I use this because you can have several commits marked e.g. "commit1". Then you make a final interactive rebase to squash them together.

Re: The Theatre of Pull Requests and Code Review

#347

Post by herval »

gwbas1c wrote: post #223
herval wrote: post #150

I find the sort of opinions on this post quite common on a subset of engineers - namely mid levels with some time in the career, who start to consider themselves senior engineers and want everyone to follow the same set of strict rules they decided make sense. It’s the same mindset that makes people pedantically apply DRY to every situation or forcing others to TDD basic apps. In practice: - smaller PRs aren’t necess…

> “every commit must compile” - again, unnecessary overzealousness. Every commit on the MAIN branch definitely should compile. Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing (With few exceptions,) I generally follow this practice; BUT, I think enforcing this on other developers feels like micromanagement. That being said, with few exceptions, committing code th…

that's the whole point.

A _branch_ is a unit of work that should be merged when done.

As the owner of a branch, an engineer has the ability to move into intermediate states. The larger the codebase, the larger the possibility of something unexpected breaking or not compiling. Just like editing a large body of text - you will have "incomplete sentences" through the process. It's part of writing. Expecting others to write their drafts the same way you like is just silly - it's putting rigid principles ahead of anything else that matters.

Re: The Theatre of Pull Requests and Code Review

#348

Post by haglin »

m000 wrote: post #338
haglin wrote: post #335

Splitting up a PR is a lot of work, especially if you want each commit to compile, but it would be great if you could categorize changes using 5–10 colors and then have checkboxes that you can toggle to hide code corresponding to a color. What each color means would depend on the PR, but, for instance, yellow = refactoring, brown = test code, blue = drive-by fix, orange = more efficient data structure etc. The colors…

How it started: "It would be great if you could categorize changes using 5–10 colors and then have checkboxes that you can toggle to hide code corresponding to a color." How it's going: https://gitmoji.dev/ IMHO, the novelty wears out fast. Especially when your git history starts looking like a Messages thread.

It would be in the "Files changed" section on GitHub, not in the git history. If you don't want to see the context, you can turn it off.

Re: The Theatre of Pull Requests and Code Review

#349

Post by m000 »

herval wrote: post #339
m000 wrote: post #333

Earlier quoted context omitted.

> PRs are emails to your team and to your future self. This should be commits though. Typically, developers would look for clues in this order: code -> code comment -> commit message -> PR text -> external document So commit messages puts the information closer to the user. One hop doesn't seem much, but the time saved adds up as you go. Also, as some other reader mentioned anecdotally, PRs may not be there forever.…

In most sane development cycles I've seen (from 2-people teams to 100k people teams), intermediate commits disappear as soon as you merge your branch (in other words, you do short lived branches + squash merge). If you decide to do merges without squashing, then yes, you gotta have to have more hygiene on each individual commit. It creates a lot of unnecessary friction and it's guaranteed to be slower (devs can't use…

> devs can't use commits as checkpoints/savepoints on their work

But they can! In git you can do whatever you want with your local/remote working branch. And after you're done it's pretty straightforward to massage it into a coherent series of commits (especially if you had been working with that in mind).

> each commit becomes a fully fleshed out "intermediate final state"

This is really a team decision. You can allow intermediate commits to e.g. fail the tests, and add a tag to your main/master after each merge. Then you know that only the tagged commits are guaranteed to be fully functional.

Re: The Theatre of Pull Requests and Code Review

#350

Post by osigurdson »

roguecoder wrote: post #184
osigurdson wrote: post #38

Unless you have a broader context, reviewing 300 line PRs in 5 minutes is going to be surface level at best. Plus that time comes with an expensive context switch so the actual cost is likely more like 20 - 30 minutes. At this point, I think a reasonable question is why not just use AI for shallow reviews like this? This would free up bandwidth in situations where you really want code reviewed by someone else.

One of the advantages of small, frequent code reviews is that a team shares the broader context. Which is far more valuable than ten minutes of extra typing.

There are better ways to get broader context. Such as getting people to work in other areas of the code. A 5 minute review isn't going to do much in this regard.
Post reply on HN