4 ms·

What is wrong with simply pushing a "Fix linting issues" in a new commit? It's self-contained and very well describes the (single) purpose of the commit. I sha

What is wrong with simply pushing a "Fix linting issues" in a new commit? It's self-contained and very well describes the (single) purpose of the commit. I share the sentiment about the "logical small commits", and hence I don't see that adding a new fix commit is problematic as long as it is self-contained and purposeful, but perhaps I don't understand what is the problem that this tool is trying to solve. It says > You have fixes for the bugs, but you don't want to shove them all into an opaque commit that says fixes So my understanding is that bugs were found in the PR and now you want to fix those bugs by not introducing separate "Fix A", "Fix B" and "Fix C" commits but you want to rewrite the history of existing N commits into N' commits so that it blends those A, B and C fixes in. Maybe I can see this being somewhat useful only ever if those N commits are pushed either directly as series as patches to the main branch or as a single merge commit, and you want to keep the S/N ratio reasonable. But otherwise I think it's a little bit problematic since it makes it harder for the reviewer to inspect and understand what changes have been done. But it also makes it harder for a developer since not all fixes are context-free, e.g. a fix for an issue found in the PR cannot always be attributed to the self-contained and single commit in your branch but it's the composition of multiple commits that actually makes this bug appear.

beart 2y ago
In some teams, you are not allowed to submit any commit that breaks the build, and a lint failure would be considered a broken build.
- barbazoo 2y ago
  You're right, most places I've worked this applies only to a subset of branches, usually main/master and sometimes other branches considered "stable" such as for the staging env.
- codetrotter 2y ago
  Do these teams run the pipeline on all the commits when you push multiple commits at the same time to your branch? Say I have an open MR from a branch that I’ve pushed three commits to so far. I pushed these three commits individually and the pipeline ran green each time. A coworker of mine points out some errors to me. I have to touch files that I previously touched in the past three commits again. I am tempted to commit these changes in one commit. But I decide to try git absorb instead. So instead of adding one fourth, green commit to my MR, my use of git absorb rewrites all three of my previous commits. But actually, the changes I was about to put in the fourth commit only work when taken together. Splitting them up and rewriting the previous three commits will result in build failure if you try to build from the new first commit or the new second commit. I don’t notice that because I’m on the new third commit. I force push the three new commits to my branch for my MR. Gitlab runs the pipeline on the last of the commits. Everything looks fine to me. Your team lead approves the MR and pushes the merge button. Three months later he scolds me for a broken bisect.
- menaerus 2y ago
  We are talking about the commit, or series of commits thereof, during the development of code in feature/bug-fix branch and not about the commit that you push as a post-fix because one of your previous commits broke something. That was not the discussion as far as my understanding goes.
baq 2y ago
> What is wrong with simply pushing a "Fix linting issues" in a new commit? if you want every individual commit be buildable then it is a no-go. it's also a no-go if you don't squash your prs.
- recursive 2y ago
  What do the semantic commit purists do when a rebase causes some arbitrary commit to go from red to green? I've always wondered about that. Commit A becomes A' in a rebase. A is good, but A' is not. A' might be 20 commits ago in a feature branch.
  - burntsushi 2y ago
    It's hard to follow your example. You say "go from red to green" which I read as "go from failing to passing," but then you go on to say "A becomes A'" where "A is good, but A' is not." Either way, the answer is that if that commit is in your patch series that hasn't been merged yet, that it might make sense to just rewrite A'. But it might not. It could be a ton of work. But also, 20 commits in one branch is rather large, and maybe that's probably wrong too. I suppose a purist might say you should go clean up the history so that you have no failing commits, but I don't know anyone who is a true purist. You don't have to be. Instead of living in a word of black-or-white, join me in embracing the grey. I don't treat commit history as something that must be perfect. I just try to treat history like I treat the source code itself: I write it for other humans. Is the history always perfectly comprehensible? Nope. Just like the source code.
    - recursive 2y ago
      Yeah, that's what I meant. I guess at the end it's all weighted tradeoffs for me too. I just put less weight on legibility and more on the ability to work on branches co-operatively without force-pushing.
      - burntsushi 2y ago
        If you're working on a branch cooperatively, then yes, don't rewrite history. But maybe you do rewrite history before merging to main to clean it up, depending. And this is also why workflow questions are hard. Because "working on branches co-operatively" was previously unstated. It's not universal. I rarely work on branches co-operatively in a long term sort of way. Sometimes I might "take over" a branch to get it merged or similar, but rewriting history in that context is still fine. It is always the case, even among so-called purists, that you don't rewrite history that you're sharing with others. (Unless you've both agreed to it. But even then, it better be short-lived because it's annoying.)
burntsushi 2y ago
> What is wrong with simply pushing a "Fix linting issues" in a new commit? It's self-contained and very well describes the (single) purpose of the commit. Because I care a lot more about the logical history of the source code versus the actual history of the source code. Commits like "fix linting issues" are more like artifacts of the development process. The commit exists likely because you forgot to run the linter earlier, and only found the issue after creating the commit. Logically speaking, those fixes belong with the relevant change. Now, if you're just going to squash the PR you're working on, then sure. The extra commit totally doesn't matter. Make as many of those commits as you want. But if your PR is 5 commits that you want to "rebase & merge" as-is (which is something I do pretty frequently), and you care about the history being easy to navigate, then it can totally make sense to do fixups before merging instead of creating new "fix linting issues" commits. If the follow the latter, then your commit history winds up being littered with those sorts of things. And fewer commits will likely pass tests (because you created new commits to fix tests). I want to be VERY CLEAR, that I do not agree with your use of the word "wrong." I do not want to call your workflow wrong. Communicating the nuances and subtleties of workflows over text here is very difficult. For example, one take-away from my comment here is that you might think I always work this way. But I don't! I use "squash & merge" plenty myself, in which case, git-absorb is less useful. And sometimes breaking a change down into small atomic commits is actually a ton of work, and in that case, I might judge it to not be worth it. And still yet other times, I might stack PRs (although that's rare because I find it very annoying). It always depends. > But otherwise I think it's a little bit problematic since it makes it harder for the reviewer to inspect and understand what changes have been done. But it also makes it harder for a developer since not all fixes are context-free, e.g. a fix for an issue found in the PR cannot always be attributed to the self-contained and single commit in your branch but it's the composition of multiple commits that actually makes this bug appear. I'm having a hard time understanding your concern about the reviewer. But one aspect of breaking things down into commits is absolutely to make it easier for the reviewer. Most of my PRs should be read as a sequence of changes, and not as one single diff. This is extremely useful when, in order to make a change, you first need to do some refactoring. When possible (but not always, because sometimes it's hard), I like to split the refactor into one commit and then the actual interesting change into another commit. I believe this makes it easier for reviewers, because now you don't need to mentally separate "okay this change is just refactoring, so no behavior changes" and "okay this part is where the behavioral change is." In the case where you can't easily attribute a fix to one commit, then absolutely, create a new commit! There aren't any hard rules here. It doesn't have to be perfect. I just personally don't want to live in a world where there are tons of "fix lint" commits scattered through the project's history. But of course, this is in tension with workflow. Because there are multiple ways to avoid "fix lint" commits. One of those is "squash & merge." But if you don't want to use "squash & merge" in a particular case, then the only way to avoid "fix lint" commits is to do fixups. And git-absorb will help you find the commits to create fixups for automatically. That's it.
homebrewer 2y ago
It makes git bisect more difficult than it needs to be.
- Gabrys1 2y ago
  Exactly this
- codetrotter 2y ago
  Yeah. But for that we can squash everything into a single commit on merge. Instead of spending a bunch on time making every single commit in an MR perfect both in isolation and all-together. And if squashing everything in the MR causes a problem with the git history being too coarse, then that is almost certainly because the MR itself should have been split up into multiple MRs. Not because of how you organized the individual commits that belonged to one MR.
  - burntsushi 2y ago
    The goal isn't perfection. Splitting PRs has overhead, especially depending on what code hosting platform you're using. Do you advocate for making code easier to read and understand by humans? What would you do if someone told you, "no I don't want to waste my time trying to make it perfect." It's the same misunderstanding. I try to treat code history like I treat code: I do my best to make it comprehensible to humans. And yes, sometimes multiple PRs is better. But sometimes multiple commits in one PR are better. I use both strategies.
    - codetrotter 2y ago
      I make multiple commits in the PR and squash it all on merge. If the resulting squash is too big / touches too many things, it’s because I didn’t split the MR where I probably should have. Because to me, the bigger waste of time is fiddling with all of the commits in the MR just to make it so that the MR can be merged without squashing. If someone needs fine-grained history about how exactly a specific feature or bug fix came to be, they can always go to the MR itself and look at what happened there.
cryptonector 2y ago
The issue isn't `Fix linting issues`, the issue is a `Fix linting issues` commit for issues you introduced in the code you'll be pushing with that `Fix linting issues` commit. Nobody wants to see you pre-push dev history -- it's not at all interesting. So rebase and squash/fixup such commits, split/merge commits -- do what you have to to make the history you do push to the upstream useful. Now, if you find linting issues in the upstream and fix those, then a `Fix linting issues` commit is perfectly fine.
nothrabannosir 2y ago
“Fix lint” commits also taint git blame. You could perhaps add some kind of filter to git blame to automatically skip commits whose message is “fix lint” but at some point the cure is worse than the disease. I also see people argue that merge commits make git bisect harder than squashing in the first place but there is a third option: rebase and fast forward. If every commit in your branch is clean and stand-alone that’s viable. Linter fix commits break that paradigm.
paulddraper 2y ago
> What is wrong with simply pushing a "Fix linting issues" in a new commit? Everything. 1. git blame is obfuscated. "Fix lint" is not helpful or relevant. Tell me what actually changed. 2. git log is noisy. More stuff is harder to read than less stuff, and you're making me read more stuff. 3. git bisect is difficult. Interspersed broken commits requires more time to sift through. (Lint is a poor example, say it's "fix server config") 4. git cherry-pick is tedious. When you copy the dev change to a release branch (let's say you maintain 1.x, 2.x, etc), you must include errata.
- greg0ire 2y ago
  5. git revert is tedious When one commit turns out to be bad, you can't revert it cleanly because your "Fix linting issues" builds upon it.

beart 2y ago

In some teams, you are not allowed to submit any commit that breaks the build, and a lint failure would be considered a broken build.

barbazoo 2y ago
You're right, most places I've worked this applies only to a subset of branches, usually main/master and sometimes other branches considered "stable" such as for the staging env.
codetrotter 2y ago
Do these teams run the pipeline on all the commits when you push multiple commits at the same time to your branch? Say I have an open MR from a branch that I’ve pushed three commits to so far. I pushed these three commits individually and the pipeline ran green each time. A coworker of mine points out some errors to me. I have to touch files that I previously touched in the past three commits again. I am tempted to commit these changes in one commit. But I decide to try git absorb instead. So instead of adding one fourth, green commit to my MR, my use of git absorb rewrites all three of my previous commits. But actually, the changes I was about to put in the fourth commit only work when taken together. Splitting them up and rewriting the previous three commits will result in build failure if you try to build from the new first commit or the new second commit. I don’t notice that because I’m on the new third commit. I force push the three new commits to my branch for my MR. Gitlab runs the pipeline on the last of the commits. Everything looks fine to me. Your team lead approves the MR and pushes the merge button. Three months later he scolds me for a broken bisect.
menaerus 2y ago
We are talking about the commit, or series of commits thereof, during the development of code in feature/bug-fix branch and not about the commit that you push as a post-fix because one of your previous commits broke something. That was not the discussion as far as my understanding goes.

baq 2y ago

> What is wrong with simply pushing a "Fix linting issues" in a new commit? if you want every individual commit be buildable then it is a no-go. it's also a no-go if you don't squash your prs.

recursive 2y ago
What do the semantic commit purists do when a rebase causes some arbitrary commit to go from red to green? I've always wondered about that. Commit A becomes A' in a rebase. A is good, but A' is not. A' might be 20 commits ago in a feature branch.
- burntsushi 2y ago
  It's hard to follow your example. You say "go from red to green" which I read as "go from failing to passing," but then you go on to say "A becomes A'" where "A is good, but A' is not." Either way, the answer is that if that commit is in your patch series that hasn't been merged yet, that it might make sense to just rewrite A'. But it might not. It could be a ton of work. But also, 20 commits in one branch is rather large, and maybe that's probably wrong too. I suppose a purist might say you should go clean up the history so that you have no failing commits, but I don't know anyone who is a true purist. You don't have to be. Instead of living in a word of black-or-white, join me in embracing the grey. I don't treat commit history as something that must be perfect. I just try to treat history like I treat the source code itself: I write it for other humans. Is the history always perfectly comprehensible? Nope. Just like the source code.
  - recursive 2y ago
    Yeah, that's what I meant. I guess at the end it's all weighted tradeoffs for me too. I just put less weight on legibility and more on the ability to work on branches co-operatively without force-pushing.
    - burntsushi 2y ago
      If you're working on a branch cooperatively, then yes, don't rewrite history. But maybe you do rewrite history before merging to main to clean it up, depending. And this is also why workflow questions are hard. Because "working on branches co-operatively" was previously unstated. It's not universal. I rarely work on branches co-operatively in a long term sort of way. Sometimes I might "take over" a branch to get it merged or similar, but rewriting history in that context is still fine. It is always the case, even among so-called purists, that you don't rewrite history that you're sharing with others. (Unless you've both agreed to it. But even then, it better be short-lived because it's annoying.)

burntsushi 2y ago

> What is wrong with simply pushing a "Fix linting issues" in a new commit? It's self-contained and very well describes the (single) purpose of the commit. Because I care a lot more about the logical history of the source code versus the actual history of the source code. Commits like "fix linting issues" are more like artifacts of the development process. The commit exists likely because you forgot to run the linter earlier, and only found the issue after creating the commit. Logically speaking, those fixes belong with the relevant change. Now, if you're just going to squash the PR you're working on, then sure. The extra commit totally doesn't matter. Make as many of those commits as you want. But if your PR is 5 commits that you want to "rebase & merge" as-is (which is something I do pretty frequently), and you care about the history being easy to navigate, then it can totally make sense to do fixups before merging instead of creating new "fix linting issues" commits. If the follow the latter, then your commit history winds up being littered with those sorts of things. And fewer commits will likely pass tests (because you created new commits to fix tests). I want to be VERY CLEAR, that I do not agree with your use of the word "wrong." I do not want to call your workflow wrong. Communicating the nuances and subtleties of workflows over text here is very difficult. For example, one take-away from my comment here is that you might think I always work this way. But I don't! I use "squash & merge" plenty myself, in which case, git-absorb is less useful. And sometimes breaking a change down into small atomic commits is actually a ton of work, and in that case, I might judge it to not be worth it. And still yet other times, I might stack PRs (although that's rare because I find it very annoying). It always depends. > But otherwise I think it's a little bit problematic since it makes it harder for the reviewer to inspect and understand what changes have been done. But it also makes it harder for a developer since not all fixes are context-free, e.g. a fix for an issue found in the PR cannot always be attributed to the self-contained and single commit in your branch but it's the composition of multiple commits that actually makes this bug appear. I'm having a hard time understanding your concern about the reviewer. But one aspect of breaking things down into commits is absolutely to make it easier for the reviewer. Most of my PRs should be read as a sequence of changes, and not as one single diff. This is extremely useful when, in order to make a change, you first need to do some refactoring. When possible (but not always, because sometimes it's hard), I like to split the refactor into one commit and then the actual interesting change into another commit. I believe this makes it easier for reviewers, because now you don't need to mentally separate "okay this change is just refactoring, so no behavior changes" and "okay this part is where the behavioral change is." In the case where you can't easily attribute a fix to one commit, then absolutely, create a new commit! There aren't any hard rules here. It doesn't have to be perfect. I just personally don't want to live in a world where there are tons of "fix lint" commits scattered through the project's history. But of course, this is in tension with workflow. Because there are multiple ways to avoid "fix lint" commits. One of those is "squash & merge." But if you don't want to use "squash & merge" in a particular case, then the only way to avoid "fix lint" commits is to do fixups. And git-absorb will help you find the commits to create fixups for automatically. That's it.

homebrewer 2y ago

It makes git bisect more difficult than it needs to be.

Gabrys1 2y ago
Exactly this
codetrotter 2y ago
Yeah. But for that we can squash everything into a single commit on merge. Instead of spending a bunch on time making every single commit in an MR perfect both in isolation and all-together. And if squashing everything in the MR causes a problem with the git history being too coarse, then that is almost certainly because the MR itself should have been split up into multiple MRs. Not because of how you organized the individual commits that belonged to one MR.
- burntsushi 2y ago
  The goal isn't perfection. Splitting PRs has overhead, especially depending on what code hosting platform you're using. Do you advocate for making code easier to read and understand by humans? What would you do if someone told you, "no I don't want to waste my time trying to make it perfect." It's the same misunderstanding. I try to treat code history like I treat code: I do my best to make it comprehensible to humans. And yes, sometimes multiple PRs is better. But sometimes multiple commits in one PR are better. I use both strategies.
  - codetrotter 2y ago
    I make multiple commits in the PR and squash it all on merge. If the resulting squash is too big / touches too many things, it’s because I didn’t split the MR where I probably should have. Because to me, the bigger waste of time is fiddling with all of the commits in the MR just to make it so that the MR can be merged without squashing. If someone needs fine-grained history about how exactly a specific feature or bug fix came to be, they can always go to the MR itself and look at what happened there.

cryptonector 2y ago

The issue isn't `Fix linting issues`, the issue is a `Fix linting issues` commit for issues you introduced in the code you'll be pushing with that `Fix linting issues` commit. Nobody wants to see you pre-push dev history -- it's not at all interesting. So rebase and squash/fixup such commits, split/merge commits -- do what you have to to make the history you do push to the upstream useful. Now, if you find linting issues in the upstream and fix those, then a `Fix linting issues` commit is perfectly fine.

nothrabannosir 2y ago

“Fix lint” commits also taint git blame. You could perhaps add some kind of filter to git blame to automatically skip commits whose message is “fix lint” but at some point the cure is worse than the disease. I also see people argue that merge commits make git bisect harder than squashing in the first place but there is a third option: rebase and fast forward. If every commit in your branch is clean and stand-alone that’s viable. Linter fix commits break that paradigm.

paulddraper 2y ago

> What is wrong with simply pushing a "Fix linting issues" in a new commit? Everything. 1. git blame is obfuscated. "Fix lint" is not helpful or relevant. Tell me what actually changed. 2. git log is noisy. More stuff is harder to read than less stuff, and you're making me read more stuff. 3. git bisect is difficult. Interspersed broken commits requires more time to sift through. (Lint is a poor example, say it's "fix server config") 4. git cherry-pick is tedious. When you copy the dev change to a release branch (let's say you maintain 1.x, 2.x, etc), you must include errata.

greg0ire 2y ago
5. git revert is tedious When one commit turns out to be bad, you can't revert it cleanly because your "Fix linting issues" builds upon it.