Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

PLEASE, recognize that "atomic" commits as the author defines them (basically whole features), will actually jumble up a large part of the diff, however much advanced your diff program is, making verifying the changes a lot harder..!

If you like to see chains of commits each with a complete feature, just remember that merge commits are commits as well!

Sure, rebase a feature branch before merging it, but then merge it with a merge commit ! (merge --no-ff)

That way you'll have both your "atomic commits", and actually atomic commits, that is commits with changes that any diff program will be able to highlight correctly. Verification will be extremely easier, but you'll retain the ability to browse the history at a higher abstraction level (either use any decent graphical git browser, or simply --first-parent).



I prefer the exact opposite approach personally: no merge commits, only rebases and fast-forward merges to master. Of course I never merge more than a handful of commits to master at a time—almost always just one—and if it’s more than one, it’s specifically because the diff is more understandable broken apart that way.

Though to be honest I am not sure what you mean by “jumbled” diffs. If you’re pushing a bunch of intermediary commits that don’t make sense on their own, don’t; that’s what interactive rebase is for. If you’re seeing jumbled diffs it might be specifically because you’re relying on merge commits instead of rebasing and fast-forwarding; if you consistently and diligently rebase that isn’t a problem.


> I prefer the exact opposite approach personally: no merge commits, only rebases and fast-forward merges to master

I know that many do, unfortunately.

It's a legacy of how git was used initially, I guess, and of the use of e-mail for sending Linux patches

> Though to be honest I am not sure what you mean by “jumbled” diffs.

Simple modifications such as code moved or indendation changes wil usually only be recognized by diff programs if they're on their own, without other changes included.

It's unfortunate, but since that if the state of things, adapting to it by making small commits will make reviews massively simpler.


> Simple modifications such as code moved or indendation changes wil usually only be recognized by diff programs if they're on their own, without other changes included.

Yeah, those are some of the situations where I will push multiple commits. Though indentation changes are something I prefer to avoid by using autoformatters.


> Though indentation changes are something I prefer to avoid by using autoformatters.

Well when you extract a function, add an if, loop, or do any number of other things, you can't avoid changing the indentation (unless you want to leave a mess)


Sure, and if you also change the logic inside that loop, I don’t think the combined diff ends up being “jumbled” or misleading at all. Also you can hide whitespace changes.


Even with hiding whitespace changes, the differences often don't get recognized correctly.

Rather then misleading, the diffs are much harder to check, instead of showing that only white space changed (or in general, rather than highlighting only the actual thing that was changed).

Basically, it's little additional work which guarantees simpler reviews for everyone, I think it's warranted




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: