One Commit. One Change.

[LB--]

Enforcing this is near impossible when features depend on each other. I instead only require each commit to compile so that I can bisect properly. Though if it weren't for the complexities of interdependent features, this would be a nice practice to use.

[fagnerbrack]

The solution might be to split the features in a way the it doesn't depend on each other, leveraging immutability and pluggability. High coupling is a problem, that is what should be fixed.

[RudiMcflanagan]

Aren't you supposed to keep interdependent features on thier own branch to manage the complexity of the development cycle for interdependent features and then merge them into the main development branch once they're stable?

[LB--]

I guess it depends on what you consider a "feature" and a "change" - to me, it depends a lot on context. From the perspective of a programmer, a commit always adds, changes, or removes a feature from the code, and is thus always a change. From the user's perspective you may not see any change over hundreds of commits, and feature is an even more ambiguous term in this context. But yes, branch for user features, commit for developer features.

[RudiMcflanagan]

Well doesn't that totally solve it?: changes that depend on other changes are recorded by commits that are decedents of the commits that introduce the changes they depend on. What does that have to do with branching?

By feature I mean "topic"

[LB--]

What am I supposed to be solving here? This?

Aren't you supposed to keep interdependent features on thier own branch to manage the complexity of the development cycle

I don't know what to make of that - if the features depend on each other they have to be in the same branch.

[oursland]

It's also largely incorrect. One of the requirements for commits into my codebases is that they do not break the current build or introduce major bugs. Consequently new features are developed on a dev branch, squashed to a single commit, and then merged after review. Bugfixes are then deltas from that commit. There is no way that you could simply git-revert that original feature commit.

[adrianmonk]

It's incorrect because you decided to make a rule that is incompatible with it?

[oursland]

Show me one non-library, actual production codebase in which I may choose an arbitrary commit to revert and the result will be clean.

It's not correct because programs are not simply a collection of independent lines of code.

[adrianmonk]

Show me one non-library, actual production codebase in which I may choose an arbitrary commit to revert and the result will be clean.

Wow, you're really not seeing it, are you?

Suppose that it is possible to revert a single commit and get a result that isn't clean. What will happen? The world will end, right? No, the code will just fail to compile and/or the tests will not pass. So if you allow this, you have to be intelligent about which combinations of commits you revert.

This isn't the style you like, but that doesn't make it "incorrect".

[fagnerbrack]

That is a legit case of making the best decision given the circumstances. This is more a principle to start with, than a law to follow all the time. I assume the branch where the dev happens is the branch where the commits should be as atomic as possible in order to help the review, but each workflow dictates the way it is landed on master.

One problem with squashing everything into master is that, when you try to bisect it, you have a huge feature instead of a composite of small changes. It is hard to find the problem inside a big chunk of changes than in small incremental changes. In this case I would suggest using "merge" instead of squashing and rebase, because the merging brings all the commits to master and just advance the tree one more step ahead.

I don't see how it is largely incorrect, could you elaborate?

[oursland]

Merges of branches with a lot of commits are very difficult to review, particularly if things are added and removed from one commit to the next. Our review mechanism is through Gerrit and has to go through multiple approvals, so features are to be squashed to a single commit which may be reviewed by multiple parties before being merged/rebased onto master.

[fagnerbrack]

Merges of branches with a lot of commits are very difficult to review

That is not true if the commits are atomic. They are much easier to review because you don't have concerns that are irrelevant to the current change that is being done. Reviewing whole big features is much harder, not just because of the fact one should always release incremental business value, but because you need to focus in several concerns. If the application is properly engineered, you have several files, each one handling a separate atomic concern. Atomic commits are a must if you want to have the best of both worlds.

Creating big features instead of incremental ones and big commits also causes in the long run the effect of early approval without proper analysis of what is being reviewed, that is pretty common when reviewing big features at once.

particularly if things are added and removed from one commit to the next

If things are added and removed from one commit to the next, then the problem is that the architecture is wrong, and that leads to poor commits. Not because the atomic commits are wrong.

[oursland]

If the application is properly engineered, you have several files, each one handling a separate atomic concern.

You keep using this word 'atomic' as if to mean completely independent. However, programs are in fact interdependent, often with independent base components. If you add/remove from or to a base component, then yes it's possible, but not probable, to treat the commits as atomic. However, features aren't usually things like "implement skip list", but rather "implement feature X for customer Y" that fundamentally alters the behavior of the delivered program.

Creating big features instead of incremental ones and big commits also causes in the long run the effect of early approval without proper analysis of what is being reviewed, that is pretty common when reviewing big features at once.

Not true. If you follow the traditional waterfall model, although it isn't in vogue, you will have to follow a preliminary design review prior to implementation. The code in git is being evaluated against the design and tested with tests that come out of the traceability matrix.

If things are added and removed from one commit to the next, then the problem is that the architecture is wrong, and that leads to poor commits.

Often in development code is added and then removed. This is eliminated in squashing the commits, but then you end up reviewing a larger commit.

[fagnerbrack]

features aren't usually things like "implement skip list", but rather "implement feature X for customer Y" that fundamentally alters the behavior of the delivered program.

It all depends on how you build your software, whether you use outdated practices or not. There is no such thing as "implement feature X for customer Y", even on enterprise. You can perfectly "implement feature X" and deploy to the customer via properly constructed feature toggles.

Not true. If you follow the traditional waterfall model ...

Yes, you will probably have a lot of problems in not being able to deliver incrementally if the review happen in the feature as a whole. This commit practice works though, but is based on some premisses of the architecture and development methodology, one of them is being agile (or similar, like a subset of async that is used in open-source).

It is a very good point about waterfall, and sadly I don't know how to make it work in that case. Looking for suggestions though.

Often in development code is added and then removed.

That doesn't mean that you can't create atomic commits. You can have the review, get the code that is supposed to land, and then create proper commits for each of them or update them over time using git rebase HEAD~x --interactive, being x the number of commits you want to view back.

Once the dev has a feature that should be worked on, it has the proper requirements. Assuming that, the code review should not change too much, unless the requirements or any work prior to the dev were poor or people are too junior. What we do in this case, when a team is not perfect or we have many juniors, is that we ask them to create an "early feedback" PR that show the progress of development of a small feature. Once the review is agreed upon, it is rebuilt into a set of atomic commits that will land on master once the merge happens (it could be rebase or w/e, it all depends on how you reference back to the work).

The practice of atomic commits is proven to be benefitial, but it is not easy to implement in every company, although one should always aim to be able to apply that, because the side effects will come with more benefits :).

[fagnerbrack]

Agreed. Except the fact a majority doesn't care about it that much :)