271–278 of 278 posts
Earlier quoted context omitted.
That sounds like what `git add -p` is for, stage part of the current changes.
That still requires you to manually select hunks. The point of `hg absorb` is to automatically select hunks even if these hunks are to be squashed into different commits.
Earlier quoted context omitted.
Commits should be a contained change that can be understood as a logical piece of history, and reverted if necessary. If you have to look at multiple commits for 1 logical change to the code, it's much more difficult to figured out what the intention was, if it was correct, and how it can be reverted.
Do you always make all of your logical code changes in single, atomic commits? What if you have to modify a feature later on? I'm sorry, but this is ridiculous. There's nothing wrong with having multiple commits in a row modifying something. That's how git works. GitHub's PR merge workflow really messed up the meta game, I tell you what...
Earlier quoted context omitted.
I don't think you're arguing against the point being made. Logical change 1, implement Hello World: puts "Hello, World!" Logical change 2, make it a method: def hello_world puts "Hello, World" end Logical change 3, take an argument for the greeting: def hello_world(greeting) puts "#{greeting}, World" end Logical change 4, take an argument for the recipient: def hello_world(greeting, recipient) puts "#{greeting}, #{re…
If those are all different commits, now imagine the fun of interactively rebasing them on top of a main branch where some other thing changed "Hello, World" to "Greetings dear Globe."
Earlier quoted context omitted.
The use case is when you look at a branch as a series of patches. Reviewing a clean set of commits is much easier than a branch full of mistakes and wrong paths taken. Useful when we optimize for reviewing and good history for future maintenance. This has been important and useful when I’ve worked on big mission critical backend system, but I also understand it might not be the most important factor for a new project…
A branch with some base is already a series of commits. I don’t get where the conceptual re-imagining is here.
Earlier quoted context omitted.
> Maybe we do, maybe we don't. If I'm in a workplace like that then I wouldn't be surprised to find a poor git flow. > How are you going to track that dependency. The commit graph (which is linear due to rebasing). The extra parameter commit comes before the commits using it. If you want to remove that parameter then you have to revert the features that rely on it or refactor them, there's no magical way to keep depe…
If the second parameter isn't supposed to be there I'd just make another PR removing it. If we use commit ancestry to determine logical dependence then this "revert the bug commit" thing won't work unless the bug just so happens to be in the very last commit.
As I've already pointed out, I'm not sure what you're arguing against but it's none of the points made in this thread.
Earlier quoted context omitted.
A branch with some base is already a series of commits. I don’t get where the conceptual re-imagining is here.
Conceptually, a branch is a series of tree states. The patches are derived from that.
Earlier quoted context omitted.
What are your thoughts on the "ship, show, ask" workflow? [1] In that workflow, small stuff is simply pushed, which allows PRs to be more single focused and more atomic. Perhaps your only objection is direct pushes to master? I am really curious if that workflow otherwise addresses all of the downsides you stated while still allowing for all PRs to be uniformly rebase-squash merged. [1] https://martinfowler.com/artic…
Why is the preferred goal to “rebase-squash” (redundant) merge? The implied onus here seems to be for the other party to move towards that strategy. But why this practice should be the goal does not seem to be mentioned. Step one isn’t to find some way to accept squash-merge. Step one is to find some reason for squash-merge.
> always squashing PRs is not a good strategy [because of the following reason]
Hence my question,if workflow X mostly solves the following reasons, then always squashing is potentially a reasonable strategy.
I think you are asking, why do that at all?
The answer is IMO quite involved, nuanced, and not at all one sized fits all.
FWIW: (1) all commits to master become atomic if a passing CI is required before rebase squash-merge.
(2) rebase creates a linear history. Non linear history is very difficult to bisect, and can really be complicated. I don't see much positives from that complexity.
(3) generally PRs are functional sized pieces of work. (That is ambiguous.) If it helps, PRs are more boulders than pebbles. Having boulders in the history is more useful than every small pebble. Net benefit is the history then becomes a smattering of well marked minor changes and a set of functional changes.
Side note, I like anything that helps the workflow of "touch clean code only, touch only the code you need to, clean the code before you touch it". (Clean defined there as "not a pile of illogical, obtusely written crap - riddled with latent bugs). In other words, make the code simple and obvious first, then modify the simple and obvious code. I give little credit to those that spend days studying code to make a one-line "masterful" change. That does not scale, everyone has to do that, and there are thousands of lines. All things in their own context though...
(4) utility and communication for commits. IMO ideally a PR is initiated with a single commit that is written "for the history". This commit description is both descriptive for the reviewers, the git history, and is the complete text for the PR. This creates the most value IMO. I view PRs as ephemeral, any extra text written for the sake of PR is overhead. Then, any following commits are pushed to communicate to the reviewers. The audience is no longer a future maintainer, nor someone looking at a PR to get a handle of the in flight changes, but is someone that has looked at the diff. Thus, commit comments like, "address TOCTOU concern", "fix typo", "add null check", "add test case", "address feedback" all then make sense. On squash-merge, those comments are discarded and the merge is just the "commit written for the history". Otherwise, without squash - those commits have a much broader audience and become long lived. I believe this typically creates a much more useful history, eliminates a lot of noise, clarifies the audience of each commit message, and finally removes any overhead where a detailed description is written in a PR but then never captured in the history (wasteful &puts context in a location that is ephemeral and a step removed)
Earlier quoted context omitted.
Why is the preferred goal to “rebase-squash” (redundant) merge? The implied onus here seems to be for the other party to move towards that strategy. But why this practice should be the goal does not seem to be mentioned. Step one isn’t to find some way to accept squash-merge. Step one is to find some reason for squash-merge.
I think you are coming at this from quite the different angle. To semi quote what I was responding to: > always squashing PRs is not a good strategy [because of the following reason] Hence my question,if workflow X mostly solves the following reasons, then always squashing is potentially a reasonable strategy. I think you are asking, why do that at all? The answer is IMO quite involved, nuanced, and not at all one si…
> > always squashing PRs is not a good strategy [because of the following reason]
I am at least coming from the same position that the OP is. Because I agree with this:
> > IMO always squashing PRs is not a good strategy. Sometimes you do want to preserve the change history, particularly if the PR does more than a single atomic change, which in practice is very common. There shouldn't be a static merge type preference at all, and this should be chosen on a case-by-case basis.
> Hence my question,if workflow X mostly solves the following reasons, then always squashing is potentially a reasonable strategy.
If OP has no problems with their current strategy (where they may squash but they are free not to) then you aren’t solving any problems. For them.
They don’t have a problem that they need to solve. You are the one who wants to fit this squash-square into their circle.
What onus do they have to read some Everything Is A Pattern dot com link in order to come to terms with a policy that they don’t need?
> I think you are asking, why do that at all?
> The answer is IMO quite involved, nuanced, and not at all one sized fits all.
Unlike squashing.
> FWIW: (1) all commits to master become atomic if a passing CI is required before rebase squash-merge.
I don’t know what atomic means. But `git log --first-parent` is equivalent to a fully squashed history. Which means that `git log --first-parent` is atomic if the squashed alternative reality is.
> (2) rebase creates a linear history.
`git bisect start --first-parent`.
There seems to be a pattern here.
> Non linear history is very difficult to bisect, and can really be complicated. I don't see much positives from that complexity.
Non-linear history can involve everything from twenty-head octopus merges to eight different root commits to backmerging twelve times before a feature branch is merged. But does saying no to the straightjacket of always-squash mean that you have to support every DAG under the sun? No.
Most sane everyday histories are limited to branching off the main branch, rebasing until it is done and then merging it back. One merge. Granted some people don’t like rebase or don’t know how to use it so they make back merges.
... And if those people make completely useless histories on that branch? Then they can squash of course. Because we’re arguing against the corporate always-squash policy. Not saying that you should never do it.
> (3) generally PRs are functional sized pieces of work. (That is ambiguous.) If it helps, PRs are more boulders than pebbles. Having boulders in the history is more useful than every small pebble. Net benefit is the history then becomes a smattering of well marked minor changes and a set of functional changes.
People can make however many commits are appropriate for the scope of the PR. It can be twenty or one.
See how the squash-always have to make the world around their “merge” strategy pristine (according to them) in order for it to work? “Just have perfect-sized PRs.” “Just have perfect-sized issues.”
> Side note, I like anything that helps the workflow of "touch clean code only, touch only the code you need to, clean the code before you touch it". (Clean defined there as "not a pile of illogical, obtusely written crap - riddled with latent bugs). In other words, make the code simple and obvious first, then modify the simple and obvious code. I give little credit to those that spend days studying code to make a one-line "masterful" change. That does not scale, everyone has to do that, and there are thousands of lines. All things in their own context though...
Okay.
> (4) utility and communication for commits. IMO ideally a PR is initiated with a single commit that is written "for the history". This commit description is both descriptive for the reviewers, the git history, and is the complete text for the PR. This creates the most value IMO.
Okay. You have still gotten to the explanation of why always-squash-merge is necessary for PRs. (We have covered the previous stuff. `--first-parent`)
We could talk about the ideal world. In an ideal world we could choose better tools that are good for this very granular PR structure. But in the real world we often end up with GitHub or some lookalike. Which is not good for granular PRs, dependent PRs, “stacked PRs” or whatever the trendy name is for the people who fetishize PRs over everything else.
In the real world we still have git(1) to be flexible around the limitations of these team-imposed tools.
And even with better tools I like being able to use several commits for one PR. Because sometimes you end up needing five preparation commits in order to solve the issue you were setting out to do. And that is often a hell of a lot more “agile” then going back to the issue tracker and making five issues in preparation for that final commit. Then making six pull requests that depend on each other.
> I view PRs as ephemeral, any extra text written for the sake of PR is overhead. Then, any following commits are pushed to communicate to the reviewers. The audience is no longer a future maintainer, nor someone looking at a PR to get a handle of the in flight changes, but is someone that has looked at the diff.
Okay.
I think that a PR can evolve to encompass more “atomic” changes then what was initially planned. That has happened to me. Me and the reviewer find out that it makes sense to make some more changes. Not “fix typos” on that same PR but changes that deserve to be commits in their own right.
There is also room for “fix typos” commits that you can then rebase away once you are done. With Git you have that flexibility.
> Thus, commit comments like, "address TOCTOU concern", "fix typo", "add null check", "add test case", "address feedback" all then make sense. On squash-merge, those comments are discarded and the merge is just the "commit written for the history". Otherwise, without squash - those commits have a much broader audience and become long lived.
Do we really have to tediously go through the same thing every time the squash-always camp tries to argue for their one-true-policy? Look at what OP said:
> > Sometimes you do want to preserve the change history, particularly if the PR does more than a single atomic change, which in practice is very common.
Which is what I just went through.
Yes. Rebase that typo fix which you introduced. That’s noise.
Probably don’t rebase that commit which made a refactor which should have been done three years ago.
> I believe this typically creates a much more useful history, eliminates a lot of noise, clarifies the audience of each commit message, and finally removes any overhead where a detailed description is written in a PR but then never captured in the history (wasteful &puts context in a location that is ephemeral and a step removed)
What me and OP has argued for allows you to do all that. And in addition to that it is also much more flexible.