Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Git-absorb: Git commit –fixup, but automatic

github.com

Post reply on HN
Print view View original

101–110 of 278 posts

Re: Git-absorb: Git commit –fixup, but automatic

#101

Post by imiric »

seadan83 wrote: post #48
imiric wrote: post #20

Earlier quoted context omitted.

Every team is free to choose what works best for them, but 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. At the risk of sounding judgemental, I think this pre…

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…

I wasn't aware of this particular workflow, but I've heard of similar ones before. The late Pieter Hintjens, of ZeroMQ fame, advocated for a strategy called "optimistic merging"[1], which essentially abandons the standard code review process in favor of merging changes ASAP, and fixing any issues as they arise later.

I'm not a fan of this. It allows contributors to abandon any established coding standards, while placing more burden on maintainers to fix issues later. This in practice never happens, so the quality of the codebase degrades over time. Not to mention that it allows malicious actors to merge changes much more easily.

As for "ship, show, ask" specifically, I have similar reservations. I think all changes should go through a review process, even if someone just glances at the changes. It not only gives the opportunity to leave feedback, but also serves as communication so that everyone is aware of the proposed changes. Also, making the decision of whether to choose "ship", "show" or "ask" might only work for senior and well disciplined developers. I think that in most teams you have a mixture of experience, so you'd inevitably end up in situations where a change should've been "show" or "ask", but was "ship", and viceversa. I don't think teams will ever align on a single strategy to make a correct decision, since it's based on subjective opinion to begin with. Always following the same workflow gets rid of these uncertainties.

[1]: http://hintjens.com/blog:106

Re: Git-absorb: Git commit –fixup, but automatic

#102

Post by DanielHB »

usr1106 wrote: post #99
alex23478 wrote: post #85

Earlier quoted context omitted.

I think this really boils down how your team is using Git and which code review tool you're using. (I've never used Gerrit personally, but as far as I understand it, we wouldn't have this conversation, since it aims to refine a single change by re-submitting a commit over and over again?) For GitHub/GitLab reviews, I'm totally with you - this makes it more convenient for the reviewer to check that/how you've responde…

Why are people talking about stacked PRs/MRs? Shouldn't they be called queued? A stack is LIFO and a queue is FIFO. (Of course in some special case you might want to merge a later one earlier, but I don't think that's the normal case people are talking about.)

Why is it a "Pull Request" instead of a "Push Request"?

Someone named it that way and it stuck.

Re: Git-absorb: Git commit –fixup, but automatic

#103

Post by IshKebab »

bradley13 wrote: post #56

Maybe I am being to much of a purist, but retroactively modifying commits and history? Why? Stuff happens, so do mistakes. Fix the mistakes, make another commit, and go on with your life.

The point of history is so that other people (or your future self) can look back at the history of the code and understand how it changed and why changes were made.

There's zero value in retaining mistakes that were never merged into `master`. In fact there is negative value because it makes the history harder to follow.

For example would you rather see "review fixes" in Git blame, or the actual useful commit message?

You can argue that you can't be bothered to make your history nice; fine. But you can't say it's wrong to do that.

(Personally I think you should squash commits for a MR; if it's too big to review as one commit it's too big for one MR - except for branches that multiple people have worked on over an extended period.)

Re: Git-absorb: Git commit –fixup, but automatic

#104

Post by usr1106 »

DanielHB wrote: post #102
usr1106 wrote: post #99

Earlier quoted context omitted.

Why are people talking about stacked PRs/MRs? Shouldn't they be called queued? A stack is LIFO and a queue is FIFO. (Of course in some special case you might want to merge a later one earlier, but I don't think that's the normal case people are talking about.)

Why is it a "Pull Request" instead of a "Push Request"? Someone named it that way and it stuck.

You request others/the maintainer to pull. That was the only way before the forges. I guess gitlab's merge request is more descriptive.

Re: Git-absorb: Git commit –fixup, but automatic

#105

Post by DanielHB »

alex23478 wrote: post #85
DanielHB wrote: post #78

Am I the only one who doesn't like atomic commits (or stacked PRs like graphite)? When I work on large PRs I often rewrite and move things around so much that trying to keep all commits in sync is a nightmare. I do try to split the work if it is very clearly isolated, but that usually means less than 3 PRs. I have tried graphite `gt absorb` (which might use this project?) and it still creates a mess. What I do that I…

I think this really boils down how your team is using Git and which code review tool you're using. (I've never used Gerrit personally, but as far as I understand it, we wouldn't have this conversation, since it aims to refine a single change by re-submitting a commit over and over again?) For GitHub/GitLab reviews, I'm totally with you - this makes it more convenient for the reviewer to check that/how you've responde…

I have used standard github and graphite reviews. I tend to prefer what I mentioned in my original post than graphite stacked PR review (which are essentially atomic commits)

Yes I also advocate for squash-before-merge, so a lot of little commits is doesn't show up in the main history.

> For me personally, the sweet spot is currently a mix of stacked-commits and the PR workflow: Use a single commit as the unit of review, and polish that commit using a PR, and use the commit descriptions within that PR to tell the story of you responding to feedback. Then, squash merge that commit.

To me time spent on commit polishing (and dealing with conflicts) is time not spent on product. Author comments on PR review, squash-before-merge, and sit-together with reviewer for big PRs to me seems a better compromise. I don't think super polished git history is worth the extra effort for most types of product, as long as I can track a change down to a PR discussion that is enough to me. From there I can track PR review commit changes individually if needed.

Like it is so uncommon for me to go digging on git history that deeply, usually all I care is "code behaving weird && line changed Of course if you are working on aviation software and the like maybe the priorities are different. But I have spent way too much time dealing with rebase conflicts when trying to chop up my PRs into smaller commits. Dealing with these conflicts often introduces bugs too.

Re: Git-absorb: Git commit –fixup, but automatic

#106

Post by usr1106 »

assiniboine wrote: post #98
usr1106 wrote: post #97

Earlier quoted context omitted.

Yes, atomic commits is not a very descriptive term. But main/master is stable is only a necessary condition, not a sufficient one. If you squash (or mix from the beginning) several unrelated changes into a single commit, main/master would be stable. AFAIK atomic commits means nothing can be taken away without breaking the change and nothing needs to be added to make the change work. How to express that without 2 clau…

Atomic commits ensure each change works independently.

That cannot be achieved in general. Later commits often depend on some earlier one.

Re: Git-absorb: Git commit –fixup, but automatic

#107

Post by talideon »

tpoacher wrote: post #80
notlinus wrote: post #69

Earlier quoted context omitted.

Criticism isn't negativity. We're not Pollyannas here, we're adults who can handle critique.

I had to look up the reference, and based on the wikipedia plot summary at least, I admit I don't quite get the relevance. I expected a plot where someone handles criticism quite badly and suffers as a result, but in fact the plot was actually about someone who handled criticism very well instead, and improved the lives of others as a result? So now I'm curious! In what way does Pollyanna relate to adults who can't h…

A Pollyanna is somebody who's cheerful and optimistic to a fault, i.e., even when it's unjustified.

The plot summary of the book is likely not what you should be reading as it's become an idiom. Something like Wiktionary or another dictionary would be a better place to look it up.

In this case, it's not about being able to receive criticism, but about being reticent about _giving_ it.

Re: Git-absorb: Git commit –fixup, but automatic

#108

Post by atq2119 »

kccqzy wrote: post #8
AndrewHampton wrote: post #3

FWIW, I've been using this alias for the past couple years for fixup commits, and I've been happy with it: > gfx='git commit --fixup $(git log $(git merge-base main HEAD)..HEAD --oneline| fzf| cut -d" " -f1)' It shows you the commits on the current branch and lets you select one via fzf. It then creates the fixup commit based on the commit you selected.

If it only lets you select one, that's strictly less powerful. What if I want some parts of it into one commit and another parts into another? The `hg absorb` works for this case.

Then you use `git gui`, which is part of the git distribution itself, or `tig` if TUIs are your thing. I have a key binding for `git commit --squash=%(commit)` in my tig config, so I can interactively select lines or hunks to stage and then the target commit for the squash.

Re: Git-absorb: Git commit –fixup, but automatic

#109

Post by snatchpiesinger »

saagarjha wrote: post #45
dmead wrote: post #42

This sounds great,but kind of an anti pattern in git. I definitely want to have a "fixes" commit on my feature branch. You should do whatever you want on a feature branch so long as your trunk has a clean history. This sounds like someone wanted to lift a feature of changesets in mercurial into git. I don't think this is safe and probably breaks a lot of people's mental model of git changelogs being an immutable data…

At some point the changes are going to get merged in, no? And that that point I would really like the commits to be nice.

git log --first-parent
Post reply on HN