5 ms·

don't know why, but recent teams around me have always made strict rules about number of commits in PRs. I just wanted to tell them the same thing you said: "Wh

don't know why, but recent teams around me have always made strict rules about number of commits in PRs. I just wanted to tell them the same thing you said: "Why don't you just look at the diffs?" curious for other opinions. (sorry not really about this particular topic)

gonzo41 3y ago
Commit and push often. Put a novel explaining yourself in the PR. And that's enough IMO.
- caskstrength 3y ago
  > Commit and push often. Put a novel explaining yourself in the PR. And that's enough IMO. Someone reading the git changelog 5 years down the line most likely wouldn't be able to find your "novel" in the PR and definitely won't appreciate if instead of a "novel" you ended up with a "short call" with the assigned reviewer explained what you actually did in your 50 "wip" commits.
  - moron4hire 3y ago
    Someone reading 5 year old git logs is lost to begin with.
    - caskstrength 3y ago
      When debugging I routinely explore git blame and read the changelog. This sometimes leads to 3, 5 or even 10 years old code. Doesn't mean I'm lost.
FPGAhacker 3y ago
Squash is our git given right.
lolinder 3y ago
I prefer to have clear commits that tell a tidy story. For example: * Refactor function `foo` to accept a second parameter * Add function `bar` * Use `bar` and `foo` in component `Baz` to implement feature #X If you give me a commit history like this, I can easily validate that each step in your claimed process does what you describe. If you instead give me a messy history and ask me to read the diff, you might know that the change to file `Something.ts` on line 125 was conceptually part of the refactor to `foo`, but I'll have to piece that together myself. It's not obvious to the person who didn't write the code what the purpose of any given change was supposed to be. This isn't a huge deal if your team's process is such that each step above is a PR on its own, but if your PRs are at the coarseness of a full feature, it's helpful to break down the sub-steps into smaller (but sane and readable) diffs.
- 88913527 3y ago
  This is reasonable, but the problem I encounter is how stifling it seems to ask others to structure their work so specifically. By way of comparison, getting compliance on conventional commit messages is a challenge, and that's an appreciably smaller ask than this.
  - lolinder 3y ago
    Oh, for sure. This is how I structure my own PRs, but I've certainly never bothered to ask a coworker to do so, I just appreciate it when I see it. That said, OP is in an environment where it sounds like this kind of structure is already the cultural norm.
    - eitland 3y ago
      From another one who tries to do the same (but doesn't enforce it): Thanks!
- krainboltgreene 3y ago
  Funny that two of your commits don't actually tell us why they exist, one simply describes the diff (which you should never need lol?) and the other proxies that responsibility to some other system. You could have simply randomized the text in each commit, put the ticket id and the one "why" in the merge commit body and gotten the same end result amount of real information in the end.
ukzwc0 3y ago
A good practice is to rebase your commits before creating a PR into a single commit. You are free to commit as many times as you want to while doing your work. This minimizes the noise in the log.
- Hamuko 3y ago
  It's only a good practice if the PR is a single logical change.
recursive 3y ago
Easy workaround. Start with feature branch f. 1. Branch f-prime from master. 2. Squash merge f to f-prime. 3. Pull request f-prime to master. 4. Profit.
dathinab 3y ago
Maybe it's just an approach to try to force logically smaller PRs without trying to limit the number of lines changes. I.e. with an idea like: - if we try to commit so that each commit does a singular change - then by limiting the number of commits we limit the number of "logical" changes in a PR - and in turn make reviews and similar easier

gonzo41 3y ago

Commit and push often. Put a novel explaining yourself in the PR. And that's enough IMO.

caskstrength 3y ago
> Commit and push often. Put a novel explaining yourself in the PR. And that's enough IMO. Someone reading the git changelog 5 years down the line most likely wouldn't be able to find your "novel" in the PR and definitely won't appreciate if instead of a "novel" you ended up with a "short call" with the assigned reviewer explained what you actually did in your 50 "wip" commits.
- moron4hire 3y ago
  Someone reading 5 year old git logs is lost to begin with.
  - caskstrength 3y ago
    When debugging I routinely explore git blame and read the changelog. This sometimes leads to 3, 5 or even 10 years old code. Doesn't mean I'm lost.

FPGAhacker 3y ago

Squash is our git given right.

lolinder 3y ago

I prefer to have clear commits that tell a tidy story. For example: * Refactor function `foo` to accept a second parameter * Add function `bar` * Use `bar` and `foo` in component `Baz` to implement feature #X If you give me a commit history like this, I can easily validate that each step in your claimed process does what you describe. If you instead give me a messy history and ask me to read the diff, you might know that the change to file `Something.ts` on line 125 was conceptually part of the refactor to `foo`, but I'll have to piece that together myself. It's not obvious to the person who didn't write the code what the purpose of any given change was supposed to be. This isn't a huge deal if your team's process is such that each step above is a PR on its own, but if your PRs are at the coarseness of a full feature, it's helpful to break down the sub-steps into smaller (but sane and readable) diffs.

88913527 3y ago
This is reasonable, but the problem I encounter is how stifling it seems to ask others to structure their work so specifically. By way of comparison, getting compliance on conventional commit messages is a challenge, and that's an appreciably smaller ask than this.
- lolinder 3y ago
  Oh, for sure. This is how I structure my own PRs, but I've certainly never bothered to ask a coworker to do so, I just appreciate it when I see it. That said, OP is in an environment where it sounds like this kind of structure is already the cultural norm.
  - eitland 3y ago
    From another one who tries to do the same (but doesn't enforce it): Thanks!
krainboltgreene 3y ago
Funny that two of your commits don't actually tell us why they exist, one simply describes the diff (which you should never need lol?) and the other proxies that responsibility to some other system. You could have simply randomized the text in each commit, put the ticket id and the one "why" in the merge commit body and gotten the same end result amount of real information in the end.

ukzwc0 3y ago

A good practice is to rebase your commits before creating a PR into a single commit. You are free to commit as many times as you want to while doing your work. This minimizes the noise in the log.

Hamuko 3y ago
It's only a good practice if the PR is a single logical change.

recursive 3y ago

Easy workaround. Start with feature branch f. 1. Branch f-prime from master. 2. Squash merge f to f-prime. 3. Pull request f-prime to master. 4. Profit.

dathinab 3y ago

Maybe it's just an approach to try to force logically smaller PRs without trying to limit the number of lines changes. I.e. with an idea like: - if we try to commit so that each commit does a singular change - then by limiting the number of commits we limit the number of "logical" changes in a PR - and in turn make reviews and similar easier