5 ms·

That looks like the PR from hell - 190 files changed, 143 commits? Mostly with names like "tidy" and "wip" Props to whoever actually reviewed that, you are a w

That looks like the PR from hell - 190 files changed, 143 commits? Mostly with names like "tidy" and "wip" Props to whoever actually reviewed that, you are a warrior

4death4 3y ago
What if it was 190 files changed in 1 commit, would that make a difference?
- rgoulter 3y ago
  It might. With commits like "typo", you might as well squash these into the commit which introduced the typo in the changeset. If there are changes across many files, and the changes were made automatically with some search-and-replace (or some refactoring tool).. by having a commit that's only that automatic change, it's easy to look at that commit and tell what the changes were. -- Presumably, non-automatic changes are going to be smaller. I guess roughly, if it makes sense to apply a changeset that changes 5 things, you'd want 5 commits. Having commits like "typo" means there are more commits; but squashing those 5 things together makes it harder to discern the granular change.
numbsafari 3y ago
> Props to whoever actually reviewed that, you are a warrior Or a ghost.
lrx 3y ago
I prefer to read the unified diff and commits don't matter as much.
- tehlike 3y ago
  This is the answer.
- readline_prompt 3y ago
  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.
- _heimdall 3y ago
  Commit your code and commit it often. There's no reason not to.
  - lolinder 3y ago
    Commit and commit often, but then clean up the history into discrete, readable chunks. If your PRs are tiny it's not a big deal, but with 190 files changed in this one, it absolutely should have been rebased into a more reasonable commit history.
  - h0l0cube 3y ago
    Also continuously integrate (from trunk) if you want to hit that moving target sooner.
  - aantix 3y ago
    Unless you’d like to maintain your train of thought. I don’t want to interrupt my flow with intermediary commits.
  - nkozyra 3y ago
    Sure, but then there's nothing wrong with rebasing it and making a nicer story for other people that want to review it. Diffs are great but sometimes they're just as overwhelming in a huge PR. It's nice to first follow 5-10 commits in chunks of logical change.
    - dilyevsky 3y ago
      Dont send huge prs. They are hard/impossible to review anyway with good commit history or not
    - arein3 3y ago
      I don't know why people are obsessed with squash merging. I always rebase (when needed) to preserve commit history. It's a good best practice, and makes it easier to spot errors after fixing conflicts. I suspect squashers use the wrong tools. Use source tree, or, if you are on linux, smartgit. You can see a detailed log, which makes it much easier.
- MenhirMike 3y ago
  Same. Do whatever you want in your feature branch, what matters is the Files list and the description in the PR. The whole thing gets squashed into a single commit anyway (which also makes reverting much easier).
  - eitland 3y ago
    Reverts are also easy even if one merges the whole branch. Just revert the merge commit. I almost never look at them, but once in a while it is really great to see the thought process that led to something.
  - dathinab 3y ago
    I my experence with teams as long as you 1. require reviews before merging 2. have not very disjunct PRs (sometimes for e.g. legacy maintenance projects you mostly have disjunct PRs normaly you do not) then you need stacked PRs for productivity, i.e. you need to be able to continue working on a new PR based on the old PR before that is fully merged (or reviwed). In this case in my experience three workflow work: 1. you (may) squash commits, and rebase stacked PRs once the previous PR has been merged (or sometimes majorly modified, but that quite advanced rebase usage). This works but has some major pain points: 1) rebased during reviews are terrible bad handled by github, 2) git doesn't keep track of the original start of a branch, this can lead to issues if you squash the commits when merging, 3) no good build in tooling for it 2. All forms of history manipulation are forbidden including rebasing and squashing. It's merge only because of this git doesn't get confused when merging squashed commits and everything seems fine... Until you now realize that follow up changes from reviews of a parent PR happen in the git history chronologically after your follow up PR and that can be a total pain depending on what changes. (Through you are allowed to fully rebase your history before marking a PR as ready for review so as long as the "stack" of PRs isn't too deep it's fine). 3. you agree with Linus that github PRs have major issues and go with a patch based approach for merging, now you need completely different tooling which often less nice modern UI but id doesn't have any of the issues of point 1 or 2 It's was quite a wtf are you doing industry moment when I realized that the most widely used contributions flows (weather in open source or in companies) are either quite flawed (1&2), productivity nightmares (no stacked commits) or quite inconvenient (3).
- throwaway892238 3y ago
  I don't think any method is gonna make it easy to grok 3,336 added lines and 5,421 removed
- penguin_booze 3y ago
  You do you but, at the point of publishing a branch for review, I'd insist the changes are presented as a story, with well-written commit messages that helps the reader/reviewer orient themselves and presents a coherent narrative. Anthing else, I call it a landfill site, not a maintained repository. In fact, I'd go as far as using their commit habit as a measure of a candidate's consideration for their colleagues.
jakebsky 3y ago
Those two work very closely together, so probably not as nightmarish as it may appear to an observer. But, the two of them are most certainly warriors.
scientaster2 3y ago
lgtm
deleted 3y ago
[deleted]

That looks like the PR from hell - 190 files changed, 143 commits? Mostly with names like "tidy" and "wip" Props to whoever actually reviewed that, you are a warrior

4death4 3y ago

What if it was 190 files changed in 1 commit, would that make a difference?

rgoulter 3y ago
It might. With commits like "typo", you might as well squash these into the commit which introduced the typo in the changeset. If there are changes across many files, and the changes were made automatically with some search-and-replace (or some refactoring tool).. by having a commit that's only that automatic change, it's easy to look at that commit and tell what the changes were. -- Presumably, non-automatic changes are going to be smaller. I guess roughly, if it makes sense to apply a changeset that changes 5 things, you'd want 5 commits. Having commits like "typo" means there are more commits; but squashing those 5 things together makes it harder to discern the granular change.

numbsafari 3y ago

> Props to whoever actually reviewed that, you are a warrior Or a ghost.

lrx 3y ago

I prefer to read the unified diff and commits don't matter as much.

tehlike 3y ago
This is the answer.
readline_prompt 3y ago
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.
_heimdall 3y ago
Commit your code and commit it often. There's no reason not to.
- lolinder 3y ago
  Commit and commit often, but then clean up the history into discrete, readable chunks. If your PRs are tiny it's not a big deal, but with 190 files changed in this one, it absolutely should have been rebased into a more reasonable commit history.
- h0l0cube 3y ago
  Also continuously integrate (from trunk) if you want to hit that moving target sooner.
- aantix 3y ago
  Unless you’d like to maintain your train of thought. I don’t want to interrupt my flow with intermediary commits.
- nkozyra 3y ago
  Sure, but then there's nothing wrong with rebasing it and making a nicer story for other people that want to review it. Diffs are great but sometimes they're just as overwhelming in a huge PR. It's nice to first follow 5-10 commits in chunks of logical change.
  - dilyevsky 3y ago
    Dont send huge prs. They are hard/impossible to review anyway with good commit history or not
  - arein3 3y ago
    I don't know why people are obsessed with squash merging. I always rebase (when needed) to preserve commit history. It's a good best practice, and makes it easier to spot errors after fixing conflicts. I suspect squashers use the wrong tools. Use source tree, or, if you are on linux, smartgit. You can see a detailed log, which makes it much easier.
MenhirMike 3y ago
Same. Do whatever you want in your feature branch, what matters is the Files list and the description in the PR. The whole thing gets squashed into a single commit anyway (which also makes reverting much easier).
- eitland 3y ago
  Reverts are also easy even if one merges the whole branch. Just revert the merge commit. I almost never look at them, but once in a while it is really great to see the thought process that led to something.
- dathinab 3y ago
  I my experence with teams as long as you 1. require reviews before merging 2. have not very disjunct PRs (sometimes for e.g. legacy maintenance projects you mostly have disjunct PRs normaly you do not) then you need stacked PRs for productivity, i.e. you need to be able to continue working on a new PR based on the old PR before that is fully merged (or reviwed). In this case in my experience three workflow work: 1. you (may) squash commits, and rebase stacked PRs once the previous PR has been merged (or sometimes majorly modified, but that quite advanced rebase usage). This works but has some major pain points: 1) rebased during reviews are terrible bad handled by github, 2) git doesn't keep track of the original start of a branch, this can lead to issues if you squash the commits when merging, 3) no good build in tooling for it 2. All forms of history manipulation are forbidden including rebasing and squashing. It's merge only because of this git doesn't get confused when merging squashed commits and everything seems fine... Until you now realize that follow up changes from reviews of a parent PR happen in the git history chronologically after your follow up PR and that can be a total pain depending on what changes. (Through you are allowed to fully rebase your history before marking a PR as ready for review so as long as the "stack" of PRs isn't too deep it's fine). 3. you agree with Linus that github PRs have major issues and go with a patch based approach for merging, now you need completely different tooling which often less nice modern UI but id doesn't have any of the issues of point 1 or 2 It's was quite a wtf are you doing industry moment when I realized that the most widely used contributions flows (weather in open source or in companies) are either quite flawed (1&2), productivity nightmares (no stacked commits) or quite inconvenient (3).
throwaway892238 3y ago
I don't think any method is gonna make it easy to grok 3,336 added lines and 5,421 removed
penguin_booze 3y ago
You do you but, at the point of publishing a branch for review, I'd insist the changes are presented as a story, with well-written commit messages that helps the reader/reviewer orient themselves and presents a coherent narrative. Anthing else, I call it a landfill site, not a maintained repository. In fact, I'd go as far as using their commit habit as a measure of a candidate's consideration for their colleagues.

jakebsky 3y ago

Those two work very closely together, so probably not as nightmarish as it may appear to an observer. But, the two of them are most certainly warriors.

scientaster2 3y ago

lgtm

deleted 3y ago

[deleted]