5 ms·

> No one is ever going back and reading individual commits. I violently disagree with this. At a minimum, when I review PRs I look at the commit history to un

• nl 2mo ago
  > If the path that was taken to commit this is full of "oops" and "fix" messages great way to encourage people to rebase then!
• bobbylarrybobby 2mo ago
  Do you really care if someone forgot to format before committing? They can always squash and push locally if they need to.
  • locknitpicker 2mo ago
    > Do you really care if someone forgot to format before committing? Not OP but yes I definitely do. If you expect others to spend time reviewing your code, you are obligated to start off by reviewing it yourself. Posting a mess helps no one and makes code harder to audit.
  • LtWorf 2mo ago
    I really really really do not want the autoformatter stuff happening in the same commit as where the real thing happens. I don't care to review if the autoformatter is working properly.
• fcarraldo 2mo ago
  Mine aren’t full of “oops” and “fix” messages, because I squashed them.
  • bonzini 2mo ago
    So you're "spending effort in perfectly curating git history" and agreeing with your parent comment.
    • ozim 2mo ago
      Just squash everything before merging and call it a day That is also a line from top comment. Everyone read „perfectly curating git history” and went rage commenting instead of reading and understanding what OP wrote.
      • bonzini 2mo ago
        Nope. Perfectly curating history is indeed the opposite of squashing—you squash because you couldn't be bothered to curate your commits. Squashing is a workaround not an alternative solution.
        • ozim 2mo ago
          You just contradicted your previous comment that was pointing out that fcraaldo is „spending effort in perfectly curating history” … or your comment was a joke with no indication it is a joke.
          • skydhash 2mo ago
            Yep, the curated history is the main branch, which the PR targets. The commit log in the Pr reflects the workflow of the author, which I have no interest in. As the reviewer, I’m only interested in the content (the description and the composite diff of the whole PR). I don’t review commit by commit.
          • bonzini 2mo ago
            I interpreted "I squashed them" as "I used git rebase -i to squash the oopses and fixes". If that's not what the user does, and rather squashes the PRs, then indeed I would be disagreeing with him.
• GrandfatherTECH 2mo ago
  What's up with the fix commits? Maybe I misunderstood you, but there ain't nothing wrong in fixing stuff you offer in your PR. And there can also be multiple commits even before the PR while you're developing your PR.
  • locknitpicker 2mo ago
    > What's up with the fix commits? They shouldn't show up in the commit history. In a PR, you merge them in the commit that they actually fix. Otherwise when you use git blame to get the context of why a line of code was changed, all you see is a useless "fixup" message that is worse than having nothing. Anyone can do better than a fixup commit. And doing metter means merging them into the actual commits that are fixed.
    • gbalduzzi 2mo ago
      > Otherwise when you use git blame to get the context of why a line of code was changed, all you see is a useless "fixup" message Isn't this solved if you squash the commits when merging the PR? I personally don't care that much about the commits inside a PR, the are just temporary because when a PR is merged they are squashed and you only get one commit for the whole feature on the main branches
      • NateEag 2mo ago
        I once root-caused a revenue-losing bug that had the entire rest of my team stumped with ten minutes of git-bisect. It only worked that well because the team had all internalized my advice to write small, coherent commits, so the bisect landed on a ten-line change. I strongly dislike the recent trend towards squashing every branch into a single monster commit.
        • LtWorf 2mo ago
          Most people who squash things have never used git bisect, cannot solve a merge conflict and when there is one will just delete the directory and clone everything again. I've worked with such people. They can go on like this for an entire lifetime.
• danieltanfh95 2mo ago
  NGL AI usage is driven by friction in presentation and communication over petty details.
• InvertedRhodium 2mo ago
  Depends what the git history is supposed to show. Personally, I prefer people to leave their mistakes and reversions - though I'd require more description messages than "oops" or "fix", something that explained why it was being reverted or swapped out would be the minimum. Sometimes you try things one way and they don't work out, so you go in a different direction. Capturing why this happened and when can go a long way towards explaining downstream decisions that might seem confusing to someone with a fresh perspective.
  • bonzini 2mo ago
    The why for a commit belongs in the message or code comments not in the history.
  • drdec 2mo ago
    One part of me wishes for multiple levels of logical commits. When using GH we essentially have one level. The PR is the like a roll-up commit and then we have the component commits it consists of. It would be nice to be able to say this commit consists of N component commits. Then users can expand or collapse the commits depending on what level of detail they want. So user A who likes to keep a record of how they actually went through the process with all the warts can have those "messy" commits as component. And user B who likes to see a coherent story told by the commits without unnecessary steps can look at the higher level commit. But also, git is complicated enough so maybe not.
    • inigyou 2mo ago
      This happens when people insist on the (rare) always-merge policy for PRs. You end up with a shorter chain of merge commits (one per PR) directly chained to each other on one side, and their other sides have several real commits between each merge. It's not the easiest structure to work with on the command line but it's clear in any visualiser.
• gofreddygo 2mo ago
  Oh that is such a bad heuristic ! The commits and history of how a PR was put together is no indicator of the quality of the PR or the thought process that led to it. Thats the equivalent of rejecting a (handwritten) essay for having too many corrections. ridiculous. The code is all that should matter. Maybe comments for being nice to others and my future self. thats it.
  • locknitpicker 2mo ago
    > The commits and history of how a PR was put together is no indicator of the quality of the PR or the thought process that led to it. It is, because it means the person posting the PR didn't even bothered to review the changes they are forcing others to review. Just clean after yourself before asking others to read your stuff.
• JohnMakin 2mo ago
  yea I look at commits several times a week at least, especially when commits are tied to a ticketing system/project it helps a lot going back months later on a large codebase going “how/why did this change happen” I do tend to squash or make my entire change in one commit though so maybe I misunderstood your comment. If I have a fix commit often I’ll just tag a separate PR/ticket to keep the change history/change control clean
• ozim 2mo ago
  But parent wrote: Just squash everything before merging and call it a day. He didn’t write „leave a mess”. So it feels you wrote knee jerk comment or just writing whatever you wanted to write disregarding whatever was written.
• ansgar77 2mo ago
  I think that the path that was taken should include mistakes. It's natural that code at some point would contain bugs and mistakes. If anything, your approach would encourage to squash those commits into one just to make it being review-worthy, but that misses the point then.
  • account42 2mo ago
    It shouldn't be that path that was taken but the path that will be taken when the PR is merged, split up into as many self-contained steps as possible to ease review now as well as triage if problems are found later.
• CuriouslyC 2mo ago
  You sound pleasant to work with. I bet your coworkers route around you when they can, and when they can't they cherry pick from their working branch to deliver monolithic commits while rolling their eyes.
  • account42 2mo ago
    No, he sounds like a professional with standards.
    • CuriouslyC 2mo ago
      A professional with standards who wasn't also unpleasant would put the time in to review the content of the commits with a request to clean up the history. Someone who looks at the history, thinks to themselves "not how I like it" and just auto-rejects the entire PR without any further thought is just a bad coworker.
      • account42 2mo ago
        No the bad co worker is the one who is trying to offload his job on the reviewer.
        • CuriouslyC 2mo ago
          A programmer's job is to deliver business value to their employer. If you're slowing down PR turnaround by mindlessly auto-rejecting on stuff that the suits don't care about, you better have a rock solid case for why that is going to deliver business value down the line, otherwise you're actively sabotaging your employer to bikeshed your personal preferences, which is the hallmark of a bad employee. Rejecting an obviously bad PR after scanning the code quickly is one thing, burning business cycles on PR turnaround/latency to bikeshed bookkeeping without spending any time on the actual value producing portion of the PR is just bad. At the minimum you wasted an opportunity to give feedback on the proposed solution, thus probably necessitating another round of reviews, with the associated org latency.
• crabbone 2mo ago
  Parent doesn't have experience of working with codebases with slightly high than average code quality requirements. In products s.a. storage, avionics, medical appliances etc. it's very typical to have a requirement for each commit to compile and to apply tests retroactively. I.e. once a test is added against an existing feature, it is run against every commit since the feature creation (this is also why git-bisect exists). However, it's true that a lot of companies would probably do better with just rsync instead of Git. Their Git history is in such a bad state that it's basically useless. It just doesn't make sense to use such a complicated tool as Git to deal with the average workflow.