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

171–180 of 278 posts

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

#171

Post by barbazoo »

beart wrote: post #163
menaerus wrote: post #162

Earlier quoted context omitted.

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 >…

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.

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.

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

#172

Post by dpkirchner »

radlad wrote: post #160
dpkirchner wrote: post #155

Earlier quoted context omitted.

Every explanation of stacked diffs I've seen (including this one) makes me think there is some secret magic not being shared that will make the whole idea "click". If the problem is devs having to wait too long for PRs to be reviewed, breaking tasks up in to smaller diffs should exacerbate the problem, wouldn't it? If diff 2..n follow diff 1 then a review of 1 blocks everything else you're doing, eh? At least the tra…

> If the problem is devs having to wait too long for PRs to be reviewed, breaking tasks up in to smaller diffs should exacerbate the problem, wouldn't it? I find small diffs to be much easier to review and as a result I review them much practically as soon as they are posted. And I'm less likely to miss something.

For sure, I agree. However there is also a context switching cost and IMO switching between your IC work and, say, a dozen small diffs is rougher than a couple larger diffs. That opinion said I haven't experienced fast reviews myself, so maybe that's what I'm missing.

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

#173

Post by keybored »

gorset wrote: post #124
keybored wrote: post #114

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.

Patch series comes from the linux kernel workflow, which git was developed to support. https://kernelnewbies.org/PatchSeries In this workflow you review every commit and not just the branch diff. Each commit is crafted carefully, and a well crafter series of commits can make even very large changes a brief to review. It takes a certain skill to do this well. As the page above says > Crafting patches is one of the cor…

That’s a nice summary.

What I don’t like about the Git documentation as I’ve read it is that they go between “patch” and “commit” in some places without stopping and explaining what the difference is. It makes sense to them. It’s obvious. But it isn’t necessarily obvious to most people.

A patch is a patch proper plus a commit message encoded in a format that git am understands. That’s fine. And the core developers understand that you cannot transmit a commit snapshot via email (or you shouldn’t). But I prefer to mostly stick to “commit” in the abstract sense, whether that to-be-commit is from a pull or from an email (or: it’s in the form of an email and it could be applied as a commit).

git rebase talks about “patch series” I think. Without explaining it. Why not “commit series”?

Sometimes it seems like talking about your changes by the way it happened to be transmitted. It’s like talking about “attachments” instead of commits because you happened to send them via email as an attachment (instead of inline).

Then you now have “stacked diffs” or “stacked commits”. Which are just a series of commits. Or a branch of commits (implicitly grounded by a base commit). For a while I was wondering what stacked diffs/stacked PRs/stacked patches and if I was missing out. When it just turned out to be, as you explain, essentially the Linux Kernel style of being able to review a commit in isolation. But in a sort of context that pull request inhabitants can understand.

I prefer to mostly talk about these things as “commits”.

(At several times writing those paragraphs above I wondered if I would be able to string together them in a coherent way)

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

#174

Post by lr4444lr »

imiric wrote: post #20
lr4444lr wrote: post #13

you don't want to shove them all into an opaque commit that says fixes, because you believe in atomic commits. Sure I do. The whole branch will be squashed anyway before it's merged in, and a single "fixes" commit while still on its own branch will be easier to track in a PR for addressing everything pointed out earlier. I mean, don't let me stop anyone from using this or --fixup if this is your flow, but this solves…

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…

Since we're taking the risk of being judgmental, I'd suggest the preference for not squashing branches to preserve the developer's local commit history when merged into the main line suggests a misunderstanding between branches and commits, but if it works for you more power to you.

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

#175

Post by keybored »

masklinn wrote: post #139
keybored wrote: post #84

Earlier quoted context omitted.

The term Stacked PRs already sounds like a term that was invented specifically in order to communicate in a GitHub-influenced context. Because Stacked PRs are just a reinvention of being able to review a commit at a time (the stack part is straightforward).

Stacked PRs is a way to surface the lifetime of the proposition as they get fixed or updated following reviews. It has nothing to do with github.

It has nothing to do with GitHub in the sense that GitHub does not support it (I guess, I’m not up to touch). It does have something to do with GitHub in the sense that the name (PRs) and the benefits are framed from the standpoint of This is What GitHub Lacks.

Which is a GitHub-centric perspective.

https://news.ycombinator.com/item?id=41514663

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

#176

Post by homebrewer »

menaerus wrote: post #162
burntsushi wrote: post #36

The negativity in the comments here is unwarranted in my opinion. I've been using `git absorb` for years and it works amazingly well. I use it in addition to manual fixups. My most common uses of git-absorb, but definitely not the only, are when I submit a PR with multiple commits and it fails CI for whatever reason. If fixing CI requires changes across multiple commits (say, lint violations), then git-absorb will al…

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 >…

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

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

#177

Post by recursive »

baq wrote: post #164
menaerus wrote: post #162

Earlier quoted context omitted.

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 >…

> 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.

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.

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

#178

Post by psadauskas »

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.

I have this one in mine: https://github.com/paul/dotfiles/blob/master/git/.gitconfig#...

    # make a fixup commit for the last time the file was modified
    cff   = "!f() { [ -n $@ ] && git add $@ && git commit --fixup $(git last-sha $@); }; f"
    # Get latest sha for file(s)
    last-sha = log -n1 --pretty=format:%h --grep 'fixup!' --invert-grep
Given a file like `git cff path/to/file.rb`, It'll find the last commit that touched that file, and make a fixup commit for it. Its great for the try-this-change-on-CI cycle: Change the Dockerfile, git cff Dockerfile, push, repeat, without it screwing up because you changed a different file while you were working on it.

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

#179

Post by burntsushi »

recursive wrote: post #177
baq wrote: post #164

Earlier quoted context omitted.

> 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.

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.

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.

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

#180

Post by recursive »

brigandish wrote: post #128
Terr_ wrote: post #122

Earlier quoted context omitted.

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."

Why is someone else writing my feature? Do we not have a ticket system? ;-) More importantly, when using a feature branch one should really rebase master into the feature branch regularly, and any other branches that might touch the same places, it takes care of little surprises like this.

> Do we not have a ticket system?

Maybe we do, maybe we don't. Maybe there are two tickets with a mutual dependence.

Even without that, if you add a parameter to a method, and later someone uses it in a totally different PR, then you effectively can't revert the creation of the parameter. How are you going to track that dependency. I've heard the arguments about logical commits and the ability to cleanly revert them. I just haven't seen any code where that would actually work, even if the trouble was taking artisanally curate the commit history.

Post reply on HN