5 ms·

Commit basically doesn't matter to me, I think in the terms of PR. I can make 100 commits or 1, it doesn't matter because when I merge I squash them into a sing

Commit basically doesn't matter to me, I think in the terms of PR. I can make 100 commits or 1, it doesn't matter because when I merge I squash them into a single commit that encompasses everything. There are some exceptions, as splitting a large merge into several smaller merges can help a lot, for example adding a database column in a pr, then adding in a separate merge usage of that column. In terms of commits, the commit message should be a good summary of what is there. In terms of a PR there should be several things. One is obviously tests present. The next is a what, why and a how, which should be explanations of why you are doing it, what the purpose is, how you are doing. Finally I think there should be a descsription of how this should be tested manually.

simonw 4y ago
Totally agree - if I'm working on a larger change I'll usually do that in a branch, which ends up as a PR which I then squash-merge into a "perfect commit" as described in the article.
neon_electro 4y ago
This is why I now love GitHub's "squash & merge"; it accomplishes so much of the ideals laid out in this "perfect commit" article.
- CuriousCosmic 4y ago
  I have to disagree. Mainly because I care what the contents of the branch were after the merge when I have to go back and figure out why a particular error is occurring. Mainly my workflow is: 1. Commit often with moderately useful commit titles/texts. 2. Rebase -i prior to pushing to remote. During rebase, squash/fixup together relevant commits and write succinct commit titles with useful commit texts. Then push. 3. Rebase -i again at the end of the PR to guarantee everything is coherent and that each commit that makes it in the merge at least builds and passes existing tests (i.e. that functionality before the branch isn't broken in the branch). 4. Merge into master. Merge commit should include the PR info (like PR text contents) in the merge text if at all possible. 5. Forget. Maybe move to a new git host. The years later when I identify a bug I can just: 1. Put together a reproducible example. 2. Git bisect --first-parent (only top level commits/merges) to identify the merge where the bug was introduced. 3. Git bisect the merge itself to identify the exact commit that introduced it. 4. From there I can determine the context of hows and whys behind the bug being introduced, identify a possible solution, and evaluate any process changes that could have prevented this bug from being introduced elsewhere in the project.
  - manbash 4y ago
    Step 3 is problematic. When you push change requests you also need to rebase i.e. put the fixing commits right after the affected commits. Otherwise, if you only rebase once the PR is complete, then you are likely to stumble upon conflicts due to the order of affected files. It's a waste of the developer's time either way, not to mention that rebasing i.e. changing history on an (upstream) PR hurts the review experience e.g. comments on lost commits.
    - seba_dos1 4y ago
      > rebasing i.e. changing history on an (upstream) PR hurts the review experience e.g. comments on lost commits. GitLab handles it well.
    - CuriousCosmic 4y ago
      I generally agree that it's not ideal. When I say rebasing, mostly I mean going back to clean up commit texts/titles and occassionally to squash/fixup commits that at the end of the dev process don't really make sense anymore and would probably only confuse people looking back at it (like squashing/fixuping down "do a, do b, revert a, do c, revert b, do d" into "do c, do d"). I make the distinction from step 2 because while I forgot to mention it (and in my head was considering it a part of step 3), I'll generally rebase right before I open the PR which cleans up most of the noise (such as "merge master even though no relevant files were touched" commits which don't add anything meaningful). Then the rebase at the end of the PR is only really squashing down a lot of the back and forth on the code review into meaningful chunks. On Github this does kinda mess with the code snippet/suggestion stuff but since it's right before merge, all of those should be closed as resolved/implemented/etc anyways. As for it being a waste of time or not, I'd disagree. I spend maybe 5 minutes at absolute most rebasing and often only about a minute (really just git rebase -i, mark the relevant commits e/s/f, let it do it's thing poking it where necessary, resigning w/ gpg, and pushing). If the rebase is throwing nasty conflicts while working on fairly linear single-dev feature branches, odds are you are rebasing wrong. And I find this saves future me a lot of time since things generally "just work" and I have all the important history still in git.
    - 4y ago
- seba_dos1 4y ago
  I believe that "squash & merge" is the most useless possible git forge feature that never makes sense to use except in cases of blatant laziness or incompetence. Its only purpose is to plaster over the mess that people who don't know how to use git well make with their branches, and relying on it is always net negative for the utility of your repository. The only case where I use "squash & merge" is when I get some trivial merge request from someone who clearly doesn't grok git well and posts tons of fix up commits. When their change would boil down to a single logical commit anyway, then sure, I can click that button instead of asking them to clean up their branch - saving time for both of us. Otherwise, there's absolutely no benefit to it that I can perceive. Every reason that I have ever heard from people using that workflow could be nullified by learning how to use git a bit better, often bringing lots of other benefits to the table. (there's one exception - "GitHub doesn't handle PR reviews well" is a somewhat valid reason, but that's a perfect excuse to look for alternatives or at least to complain to GitHub to make them finally fix their product; it's not the only forge out there, GitLab deals with branches getting rewritten during review well)
  - phist_mcgee 4y ago
    Woah, that's a lot of absolute statements you've made. Pulling out the old 'it's not the tool it's you' argument as well
    - seba_dos1 4y ago
      > absolute statements In case you haven't noticed, I started the whole comment with "I believe" ;) I'm clearly not an almighty source of all truth; that's just my impression after 15 years of experience with git and many projects using it with various workflows.
  - m000 4y ago
    Finally someone who gets it: history rewriting is the killer feature of git. If you don't curate your local commits before pushing, you're not using git properly. Squashed commits essentially reduce the development workflow/process to the level of previous generation VCS tools (e.g. subversion).
rektide 4y ago
Its tragic how either/or git is. Both are great in different ways. If Im trying to understand a complex tricky bit of code, a long string of small commits is ideal. If Im browsing a repo, large squashed commits or PRs are better. It's be great to have better heirarchy, to get both.
- drdec 4y ago
  You can have both if you keep the PR branches
  - Twisol 4y ago
    This is why I follow a rebase-then-merge approach. The commit graph looks like this: | * |\ | * | | | * |/ * |\ | * |/ | Each PR is neatly delineated, and I can quickly follow things at coarse-grained level (follow only the left parents; there's even a `git log` flag for this) or at a fine-grained level. This isn't just pretty -- this has been actively helpful to me many, many times. I can `git bisect` into a merged PR to more precisely identify where an issue arose, and `git rebase` onto intermediate commits instead of jumping all the way to the end if I find a whole PR to be problematic to rebase past in one go.
    - vvillena 4y ago
      I also try to adopt this approach whenever possible. The 'git log' flag you mention is '--first-parent'.
      - Twisol 4y ago
        Thanks for mentioning the specific flag! I didn't mention it because I don't use it often enough to remember it (the shape of the log is usually enough), but it does exist if you want it :D
    - taberiand 4y ago
      Semi-linear merge is my favourite too. I'll also often reset soft and hand craft the commits for the PR to lay out the work of the branch so that it builds up logically, and assist reviewers (and future-me) in understanding the structure of the change.

simonw 4y ago

Totally agree - if I'm working on a larger change I'll usually do that in a branch, which ends up as a PR which I then squash-merge into a "perfect commit" as described in the article.

neon_electro 4y ago

This is why I now love GitHub's "squash & merge"; it accomplishes so much of the ideals laid out in this "perfect commit" article.

CuriousCosmic 4y ago
I have to disagree. Mainly because I care what the contents of the branch were after the merge when I have to go back and figure out why a particular error is occurring. Mainly my workflow is: 1. Commit often with moderately useful commit titles/texts. 2. Rebase -i prior to pushing to remote. During rebase, squash/fixup together relevant commits and write succinct commit titles with useful commit texts. Then push. 3. Rebase -i again at the end of the PR to guarantee everything is coherent and that each commit that makes it in the merge at least builds and passes existing tests (i.e. that functionality before the branch isn't broken in the branch). 4. Merge into master. Merge commit should include the PR info (like PR text contents) in the merge text if at all possible. 5. Forget. Maybe move to a new git host. The years later when I identify a bug I can just: 1. Put together a reproducible example. 2. Git bisect --first-parent (only top level commits/merges) to identify the merge where the bug was introduced. 3. Git bisect the merge itself to identify the exact commit that introduced it. 4. From there I can determine the context of hows and whys behind the bug being introduced, identify a possible solution, and evaluate any process changes that could have prevented this bug from being introduced elsewhere in the project.
- manbash 4y ago
  Step 3 is problematic. When you push change requests you also need to rebase i.e. put the fixing commits right after the affected commits. Otherwise, if you only rebase once the PR is complete, then you are likely to stumble upon conflicts due to the order of affected files. It's a waste of the developer's time either way, not to mention that rebasing i.e. changing history on an (upstream) PR hurts the review experience e.g. comments on lost commits.
  - seba_dos1 4y ago
    > rebasing i.e. changing history on an (upstream) PR hurts the review experience e.g. comments on lost commits. GitLab handles it well.
  - CuriousCosmic 4y ago
    I generally agree that it's not ideal. When I say rebasing, mostly I mean going back to clean up commit texts/titles and occassionally to squash/fixup commits that at the end of the dev process don't really make sense anymore and would probably only confuse people looking back at it (like squashing/fixuping down "do a, do b, revert a, do c, revert b, do d" into "do c, do d"). I make the distinction from step 2 because while I forgot to mention it (and in my head was considering it a part of step 3), I'll generally rebase right before I open the PR which cleans up most of the noise (such as "merge master even though no relevant files were touched" commits which don't add anything meaningful). Then the rebase at the end of the PR is only really squashing down a lot of the back and forth on the code review into meaningful chunks. On Github this does kinda mess with the code snippet/suggestion stuff but since it's right before merge, all of those should be closed as resolved/implemented/etc anyways. As for it being a waste of time or not, I'd disagree. I spend maybe 5 minutes at absolute most rebasing and often only about a minute (really just git rebase -i, mark the relevant commits e/s/f, let it do it's thing poking it where necessary, resigning w/ gpg, and pushing). If the rebase is throwing nasty conflicts while working on fairly linear single-dev feature branches, odds are you are rebasing wrong. And I find this saves future me a lot of time since things generally "just work" and I have all the important history still in git.
  - 4y ago
seba_dos1 4y ago
I believe that "squash & merge" is the most useless possible git forge feature that never makes sense to use except in cases of blatant laziness or incompetence. Its only purpose is to plaster over the mess that people who don't know how to use git well make with their branches, and relying on it is always net negative for the utility of your repository. The only case where I use "squash & merge" is when I get some trivial merge request from someone who clearly doesn't grok git well and posts tons of fix up commits. When their change would boil down to a single logical commit anyway, then sure, I can click that button instead of asking them to clean up their branch - saving time for both of us. Otherwise, there's absolutely no benefit to it that I can perceive. Every reason that I have ever heard from people using that workflow could be nullified by learning how to use git a bit better, often bringing lots of other benefits to the table. (there's one exception - "GitHub doesn't handle PR reviews well" is a somewhat valid reason, but that's a perfect excuse to look for alternatives or at least to complain to GitHub to make them finally fix their product; it's not the only forge out there, GitLab deals with branches getting rewritten during review well)
- phist_mcgee 4y ago
  Woah, that's a lot of absolute statements you've made. Pulling out the old 'it's not the tool it's you' argument as well
  - seba_dos1 4y ago
    > absolute statements In case you haven't noticed, I started the whole comment with "I believe" ;) I'm clearly not an almighty source of all truth; that's just my impression after 15 years of experience with git and many projects using it with various workflows.
- m000 4y ago
  Finally someone who gets it: history rewriting is the killer feature of git. If you don't curate your local commits before pushing, you're not using git properly. Squashed commits essentially reduce the development workflow/process to the level of previous generation VCS tools (e.g. subversion).

rektide 4y ago

Its tragic how either/or git is. Both are great in different ways. If Im trying to understand a complex tricky bit of code, a long string of small commits is ideal. If Im browsing a repo, large squashed commits or PRs are better. It's be great to have better heirarchy, to get both.

drdec 4y ago
You can have both if you keep the PR branches
- Twisol 4y ago
  This is why I follow a rebase-then-merge approach. The commit graph looks like this: | * |\ | * | | | * |/ * |\ | * |/ | Each PR is neatly delineated, and I can quickly follow things at coarse-grained level (follow only the left parents; there's even a `git log` flag for this) or at a fine-grained level. This isn't just pretty -- this has been actively helpful to me many, many times. I can `git bisect` into a merged PR to more precisely identify where an issue arose, and `git rebase` onto intermediate commits instead of jumping all the way to the end if I find a whole PR to be problematic to rebase past in one go.
  - vvillena 4y ago
    I also try to adopt this approach whenever possible. The 'git log' flag you mention is '--first-parent'.
    - Twisol 4y ago
      Thanks for mentioning the specific flag! I didn't mention it because I don't use it often enough to remember it (the shape of the log is usually enough), but it does exist if you want it :D
  - taberiand 4y ago
    Semi-linear merge is my favourite too. I'll also often reset soft and hand craft the commits for the PR to lay out the work of the branch so that it builds up logically, and assist reviewers (and future-me) in understanding the structure of the change.