[eru]

Everyone committing to the main branch all the time sounds like a nightmare to me, to be honest. I would probably seriously consider quitting if this was forced on me.

(Or rather, I would probably just run on my private git copy, and only pull every once in a while, and ignore that the main branch always changes.)

When / how do you do code review in your suggested workflow?

[Arch-TK]

To be fair. I have never really heard of pushing to master as a CI technique for git. You would push to your own publicly visible branch and some script would automatically attempt to merge into a CI branch unless there's merge conflicts in which case you get notified. Then as you add things you get notified if the whole thing is breaking and get made aware of upcoming issues.

[jaggederest]

Yeah when I set up a repo, I set up two or three builds per PR:

- the code exactly as it is in that branch

- that branch merged into current main

- that branch merged into what is currently running in production (in the case that it's not what is in main e.g. tagged or branched)

This really helps to see whether it's an issue in "your" code, or whether it's a merge issue conflict causing failures.

[eru]

Making those provisionally merge commits (and testing them) is a good idea, if you can spare the computational resources.

See https://bors.tech/ for a similar idea.

What I would object to is changing the 'official' master every few minutes automatically.

[jaggederest]

Yes, I would not advocate for that either. I like having PRs be a more complete thought, ideally, though sometimes you're left with 3 PRs for "one change" if you need e.g. a database change, a migration for the data, and changing the code to use the new column, which is frustrating but doable.

[eru]

I'd like my PRs to tell a very sanitised story of how I could have come up with the change, with the power of foresight.

Basically, first you write your code however you see fit. Then you use git to rewrite history to make the reviewers life easy, and then you give it to the reviewer.

The reviewer doesn't need to know that my original version had a bug that I fixed later. I can make it look like I came up with a bug free version from the start.

[jaggederest]

I used to do that but these days I prefer the whole story be in there so I can reference it later if I run into the same bug in a different place.

In general I find my past efforts to maintain a clean git history were probably not that useful, as long as you're not running e.g. 4 or 5 branches in parallel with crossing history. Branch off, make change, merge back main, PR, is fine.

[eru]

If there's no merge conflict (and the tests pass), would the tool also change master to the new merge commit?

Or are all the merge commits just made provisionally to inform you, but don't change master?

[Arch-TK]

>would the tool also change master to the new merge commit

Not in my experience, no.

The goal is just to keep you aware of what problems you're likely to run into if you tried to push for your change to be merged in at the present moment. If these problems are due to work another team is doing the idea is then that you might begin conversations with that other team to discuss how best to sort these things out.

[crdrost]

One of the easiest tools in this regard is just a “test” that blocks merging if the request is n commits behind master—just enforcing a rebase whenever you update a pull request. Combine with forbidding fast forward merges for pull requests (just let the code review tool generate a merge commit) and your `git log --graph` becomes much mor bounded in terms of how bad it can get.

[eru]

Do you see `git log --graph` becoming entangled as a problem in itself, or as a symptom of a problem?

Your suggestion seems to help with the former, but not really the latter, or does it?

You might like https://bors.tech/ however.

[gizzlon]

You can have branches, but the ""rule"" is that no branch should live for more than, say, a day.

This trades the integration complexity and problems for some new.. challenges :)

[pmontra]

Do people start working on something new past 3 PM or do they start discussing what to do the next day?

[bobthepanda]

My team uses this pattern and generally speaking it looks something like this

* everybody is good at breaking down code into changes small enough review quickly but not small enough to become tedious and trivial

* people work on CRs whenever they have time. They do generally not post them after people have started going offline, and wait til next morning as a courtesy, since there is no difference between publishing for review at 5pm vs 9am the next day

One side effect of this workflow is that because the pieces are more manageable, less uninterrupted “flow” time is required to complete a bunch of small things than to make one really big change. And others digest the smaller changes easier and knowledge spreads more effectively. And with the time its easy to say “don’t make people review outside working hours.”

[mannykannot]

They are not new challenges, they are the same old challenge that led to version control procedures and systems in the first place: your incomplete or broken code is interfering with my attempts to complete or fix mine!

[bobthepanda]

Whether the broken merge happens in a branch or as part of a rebase is just moving things around. The trick is to enforce good code cleanliness when pushing, whenever that may be.

My team does stuff on mainline and requires a new review on rebase.

[PH95VuimJjqBqy]

It's called trunk-based development.

https://www.atlassian.com/continuous-delivery/continuous-int...

[thfuran]

Let alone pushing every couple of minutes.

[crdrost]

So since I was the one who brought it up there's like three parts to this in my mind.

Main caveat is that “prod” is a handful of central places.

So how does code review fit in?

- The first thing is that your main branch needs to be able to tolerate merging incomplete code without breakage. This would probably be resolved with feature toggles, ideally named to match issue/ticket numbers, so that you have to clean it up before closing the ticket. Automatic tests can catch syntax issues but code review would check that your code was indeed either removing a feature toggle (in which case the team should have agreed that the code is mature and turned it on in production) or properly isolated behind a toggle.

- The second thing is that the team needs to be working as a team. This means that the sprint goals are co-owned by multiple people, ideally the whole team takes responsibility to deliver. It never made much sense to me that we segmented out features to individual developers, as if “oh she is out sick this week, well she wasn't working on anything urgent, the work can wait for her,” I understand that this makes performance review slightly easier but I am not convinced by that argument... So what this means for development is that the “war room” that you see for a typical prod outage with a bunch of people chasing down different leads, I want to bring that mentality (without the concomitant stress!) to feature development. So this means that ideally the whole team knows what everyone is working on and code reviews are just part of the organic process of working on a feature together. «Hey you are calling this “accept_states” and I called it “success_states,” I really don't like the fact that you're using “accept” as an imperative verb here, but I could go with “states_to_accept” or “acceptable_states” or we could use “success_states,” which would you rather?» ... You read through each other's code because it legitimately impacts what you are doing elsewhere in the codebase.

- The final ingredient would be release staging. The way a lot of people do branching makes it really easy to forget commits or to roll something back and not rollback the rollback, et cetera. Because we merge everything into main, we get monotonic flow: a commit that is still relevant is merged into main first because everything is, then it gets cherry-picked into the appropriate release branches. (The only exceptions are bugfixing code that is no longer part of the latest version.) The exact tooling to make this doable seems tractable, but perhaps not easy. Per point 1, before the feature branch is permanently flipped, all the code related to it is marked as such and can be merged into a patch release without actually triggering a semver violation (just leave the toggle turned off!)... So in theory the actual feature release is just a single commit. I’d have to see how the rest of this works before I would know exactly how this plays out.

Note that just about every Git-based CI/CD platform does CI/CD incorrectly, things like ACLs and CI/CD config are central statements about who has access to what privileges and should not be branched. A Jenkins which pulls its config from the `ci` or `main` branches will usually save you a lot of headache.

[ilyt]

From my experience it only work in CM repos where you have a lot of very small changes and usually unrelated.