5 ms·

Just to be clear, and you probably won't disagree with this, there's nothing wrong with commits like "temp" or "checkpoint" or "WIP" (Work In Progress). I often

Just to be clear, and you probably won't disagree with this, there's nothing wrong with commits like "temp" or "checkpoint" or "WIP" (Work In Progress). I often make these sorts of commits as I'm working on stuff. The issue is in submitting an MR/PR with those commits. There's an expectation among professionals that you make your work presentable before submitting it for review, although those who are new to the profession don't realize that this cleanup step is necessary (how could they? the intro courses don't teach this and they're usually struggling enough with the code). I just wanted to throw this comment in here in case some newbie sees this and comes away thinking "oh, I can't have 'temp' or 'checkpoint' in my commit messages"

taberiand 2y ago
I encourage developers to clean up their commits but so often they're either unduly nervous about breaking things or have a fascination with "preserving the history" (of their branch, which is not yet merged in anywhere). I partly blame the excessive fear mongering around rebasing, where the strict Never Rebase a Pushed Branch rule is drilled into them and they never learn why or when they can break the rule safely. So it's an uphill fight but I just try to teach by demonstrating, frequently, exactly how they can tidy up for the merge request.
- magicalhippo 2y ago
  > I just try to teach by demonstrating, frequently, exactly how they can tidy up for the merge request. Some recommended resources for this?
hyperbolablabla 2y ago
I couldn't care less about the commit messages on a PR, I care about the diff. as long as the messages are cleaned up in the squash and merge with main
- Cthulhu_ 2y ago
  But that's the thing, in most cases there's no review step anymore after the squash/merge has been made; while it's in a branch / MR, you can still edit the commit messages and content, but in most cases the squash and merge is an atomic, unreviewed step. Of course, maybe the merge commit should be generated, or should be filled in and reviewed as part of the main code review.
  - mkesper 2y ago
    Why is squash and merge a trend now? This destroys all advantages of git in my view.
    - keybored 2y ago
      Yeah, I’ve noticed that many around the Internet just comment with “the squash and merge” as if it’s a given that that’s the strategy that is used (even mandated).
- OJFord 2y ago
  If it's large I want to be able to look at the commits in isolation and understand the work in the logical chunks that made sense to the author. If what made sense to them is temp, checkpoint, temp, temp, undo the temp, fix test, try this, that didn't work try other thing, maybe?, temp, fix test - then I don't stand a chance. Recently I reviewed one that had multiple 'rebase' commits, I have no idea.
  - scott_w 2y ago
    > If it's large Then make the PR small.
    - OJFord 2y ago
      1. I'm reading not writing. 2. Is our 'large' the same? 3. I'd rather the PR were whatever size it needs to be to entirely do the thing it's supposed to do (and nothing else) than conform to some arbitrary size requirement.
      - scott_w 2y ago
        You're over-thinking it. Large PRs have a known effect of reviewers' eyes just glaze over and the PR just gets a cursory glance and LGTM :+1: on it. That's when you know a PR is "large." What's stopping you breaking such a PR into smaller chunks? Some arbitrary "it does what it's supposed to do" definition?
    - keybored 2y ago
      This is a conversation that keep repeating - Many temp commits - Doesn’t matter: just squash - But sometimes I want to have a few distinct (isolated commits for my PR) - Then make the PR small - But I have several changes - Then make several small PRs We keep coming back to “just make more PRs”. Which is curious, given that GitHub doesn’t even support dependent PRs. The thing you need immediately when your PRs start depending on each other (like refactor X before implementing Y, where both touch the same code). But I can’t come to any other conclusion than that this is because of the over-focus on PR as the one and only reviewable unit. Thanks to GitHub. I can’t really get on the small PR train. A PR has overhead and I often want to do small changes that I only happen to notice are necessary when I am in the middle of doing that one PR.

taberiand 2y ago

I encourage developers to clean up their commits but so often they're either unduly nervous about breaking things or have a fascination with "preserving the history" (of their branch, which is not yet merged in anywhere). I partly blame the excessive fear mongering around rebasing, where the strict Never Rebase a Pushed Branch rule is drilled into them and they never learn why or when they can break the rule safely. So it's an uphill fight but I just try to teach by demonstrating, frequently, exactly how they can tidy up for the merge request.

magicalhippo 2y ago
> I just try to teach by demonstrating, frequently, exactly how they can tidy up for the merge request. Some recommended resources for this?

hyperbolablabla 2y ago

I couldn't care less about the commit messages on a PR, I care about the diff. as long as the messages are cleaned up in the squash and merge with main

Cthulhu_ 2y ago
But that's the thing, in most cases there's no review step anymore after the squash/merge has been made; while it's in a branch / MR, you can still edit the commit messages and content, but in most cases the squash and merge is an atomic, unreviewed step. Of course, maybe the merge commit should be generated, or should be filled in and reviewed as part of the main code review.
- mkesper 2y ago
  Why is squash and merge a trend now? This destroys all advantages of git in my view.
  - keybored 2y ago
    Yeah, I’ve noticed that many around the Internet just comment with “the squash and merge” as if it’s a given that that’s the strategy that is used (even mandated).
OJFord 2y ago
If it's large I want to be able to look at the commits in isolation and understand the work in the logical chunks that made sense to the author. If what made sense to them is temp, checkpoint, temp, temp, undo the temp, fix test, try this, that didn't work try other thing, maybe?, temp, fix test - then I don't stand a chance. Recently I reviewed one that had multiple 'rebase' commits, I have no idea.
- scott_w 2y ago
  > If it's large Then make the PR small.
  - OJFord 2y ago
    1. I'm reading not writing. 2. Is our 'large' the same? 3. I'd rather the PR were whatever size it needs to be to entirely do the thing it's supposed to do (and nothing else) than conform to some arbitrary size requirement.
    - scott_w 2y ago
      You're over-thinking it. Large PRs have a known effect of reviewers' eyes just glaze over and the PR just gets a cursory glance and LGTM :+1: on it. That's when you know a PR is "large." What's stopping you breaking such a PR into smaller chunks? Some arbitrary "it does what it's supposed to do" definition?
  - keybored 2y ago
    This is a conversation that keep repeating - Many temp commits - Doesn’t matter: just squash - But sometimes I want to have a few distinct (isolated commits for my PR) - Then make the PR small - But I have several changes - Then make several small PRs We keep coming back to “just make more PRs”. Which is curious, given that GitHub doesn’t even support dependent PRs. The thing you need immediately when your PRs start depending on each other (like refactor X before implementing Y, where both touch the same code). But I can’t come to any other conclusion than that this is because of the over-focus on PR as the one and only reviewable unit. Thanks to GitHub. I can’t really get on the small PR train. A PR has overhead and I often want to do small changes that I only happen to notice are necessary when I am in the middle of doing that one PR.