|
New Yorp New Yorp posted:I thought it was super narrowly scoped to just code review. Which as far as I can tell, makes it strictly inferior to basically every other Git hosting platform out there, but I've never used it so maybe I'm missing something about it. Yes reviews, but before you can review you need to build the thing, right? No point in doing anything if you cannot even compile. I haven't administered it, just used it in the past, so it had to have a way of kicking off a build somewhere, somehow.
|
#
?
Mar 4, 2020 22:36
|
|
|
|
| # ? Apr 26, 2024 02:36 |
|
|
I would imagine that the ideal scenario is hooking it in to your existing CI infrastructure, rather than standing up some separate build infrastructure just for code reviews.
|
|
#
?
Mar 4, 2020 23:21
|
|
|
Ralith posted:What are you comparing it to? Completely unadorned git? Sounds like previously it was just standard github Flow for stuff. There may be something about really liking the ability to make changes in several repos at once, under a common feature umbrella, that gerrit picks up?
|
|
#
?
Mar 5, 2020 08:22
|
|
|
Cuntpunch posted:Sounds like previously it was just standard github Flow for stuff. There may be something about really liking the ability to make changes in several repos at once, under a common feature umbrella, that gerrit picks up? Replacing GitHub with Gerrit for any reason other than "we really don't want our code in anyone else's hands" definitely seems weird. Though I think the code review handles iteration a bit better.
|
|
#
?
Mar 5, 2020 17:57
|
|
|
Ralith posted:Replacing GitHub with Gerrit for any reason other than "we really don't want our code in anyone else's hands" definitely seems weird. Though I think the code review handles iteration a bit better. Github flow is a branching strategy that's not related to using github at all though?
|
|
#
?
Mar 5, 2020 18:22
|
|
|
New Yorp New Yorp posted:Github flow is a branching strategy that's not related to using github at all though? Sure, it can be, but then that doesn't really answer the question of what software they were using for repo administration and code review.
|
|
#
?
Mar 5, 2020 18:25
|
|
|
Yeah, Gerrit was just my way of saying "don't use raw Git repos". Two pairs of eyes on every code change is a Good Thing, not primarily for control but for knowledge spread within the team. We do peer reviews in my team, where everyone has +2 permissions (the ability of final approval on a patch) except if someone is very new. The submitter chooses one or two reviewers. Part of the trick to making it work is to not always have the same guy review the frontend code, etc. Gerrit uses "patch sets" while Github/Gitlab use "pull requests" or "merge requests", but they are essentially the same thing. After a pull request is merged you usually have a new series of commits on your master, while if you've merged a Gerrit patch set, there is normally only one big squashed commit. This isn't mandatory but Gerrit doesn't really work for approving a whole branch. It mostly comes down to preference.
|
|
#
?
Mar 5, 2020 19:46
|
|
|
Hippie Hedgehog posted:Yeah, Gerrit was just my way of saying "don't use raw Git repos". Two pairs of eyes on every code change is a Good Thing, not primarily for control but for knowledge spread within the team. I'm trying really, really hard to keep an open mind. I want to believe there's some revelation about the value of Gerrit. But from what I can tell it's basically just the source control equivalent of a huge speedbump. The developer writing code is encouraged strongly to only push once for any given file until review is completed. Because otherwise you might find yourself rebasing commits from 3 days ago for code that no longer exists just to make a pretty commit history. 
|
|
#
?
Mar 9, 2020 15:42
|
|
|
I don't know what your actual process is, but your description of your issues definitely makes me feel like you're not using the tool correctly. Could you walk through what you're attempting to do, and what the major pain points are?
|
|
#
?
Mar 9, 2020 16:14
|
|
|
Jabor posted:I don't know what your actual process is, but your description of your issues definitely makes me feel like you're not using the tool correctly. I am coming in blind to Gerrit. Never even heard about it before last week, when starting this position. Apparently the flow of things is: - Dev opens local branch off master - Dev makes (small and focused) commits - Dev pushes to gerrit HEAD:/ref/for/master with some topic. - Gerrit runs build for each commit in the push. -1 autoapplied if a given commit doesn't build. - Reviewer reviews the individual commits - In the case of changes requested... - Developer rebases against the commit under review and fixes it according to review - Reviewer takes another look and confirms the fixes - In the case of acceptance, commit is merged. However, in doing some fresh development including prototyping some ideas in domain modelling, this literally resulted in certain old things getting flagged for review. Despite the fact that 3 commits down the chain, the code-under-review *no longer existed*. But the take from the loudest voices in favor of Gerrit was to fix the commit because a pure history trumped wasting reviewer time by asking them to review those commits in the first place. Except 'ok I won't commit until I am sure I have stable code' was met with "commit often" and "won't that result in a lot of rebase fixing of ancient commits?" was met with "well don't commit things that aren't final". idgi.
|
|
#
?
Mar 9, 2020 16:49
|
|
|
The workflow Gerrit is designed around is that you edit patches to fix problems with them rather than creating new patches. Each commit is supposed to be an atomic thing which leaves the code base in a working and sensible state. If something is broken and then removed 3 commits later then you're probably making your changes in the wrong order and should rearrange them. I'm not sure why you think this is incompatible with "commit often".
|
|
#
?
Mar 9, 2020 22:13
|
|
|
Plorkyeran posted:The workflow Gerrit is designed around is that you edit patches to fix problems with them rather than creating new patches. Each commit is supposed to be an atomic thing which leaves the code base in a working and sensible state. If something is broken and then removed 3 commits later then you're probably making your changes in the wrong order and should rearrange them. 1. Spin up a branch to explore some new stuff 2. Create some new types. Commit. Push. 3. Go off to handle serialization concerns elsewhere. Commit. Push. 4. Realize that the types in #2 need some additional work. Commit. Push. 5. Reviewer looks at #2 and goes "we need to rename some of these fields". These fields no longer exist at all. 6. Rebase? To rename things? That don't exist in the most current state of the branch? It seems to lead to a logic of: A. Don't commit unless you're nigh certain you won't end up needing to tweak it before review is done. B. But if you have committed something for any reason, then don't touch that again until review is done on that commit. This might be sensible and great when you're working on an entrenched codebase where you're mostly just twisting dials to tune things, or subtly altering behavior in a predictable way. But for greenfield work, it seems bizarre - doubly so when it leaves you needing to (I presume, it's on my TODO list now) jack around at some level with fundamental git tooling settings just to get it to work(since you're pushing to a ref path rather than to a branch, git push doesn't actually work directly) - and when the advocate who thinks its golden is also taking 90+ minutes to rebase his own work in the sub +/- 100 line range because there are 2-3 commits that need tweaking.
|
|
#
?
Mar 9, 2020 22:28
|
|
|
If you're making a bunch of local commits, you should squash them down into the single reviewable unit that you want to push to master, and push that to Gerrit. Nobody wants to see all your intermediate prototyping commits cluttering up their bisects. It's also worth noting that Gerrit isn't actually causing any additional rebasing - you'd need to rebase that code when you actually merge it into master anyway. Or just figure out that you can use commit --amend.
|
|
#
?
Mar 9, 2020 22:34
|
|
|
Cuntpunch posted:1. Spin up a branch to explore some new stuff You should be modifying the original commit so the reviewer sees the latest iteration, rather than adding new commits on top that confuse everything. Or better yet, do that and also don't push until you have something that's not going to change ten minutes later. "Commit often" and "keep commits small" doesn't mean just blindly commit and push to review every time you save a file; it means organize your changes into small self contained units. This isn't always easy and it does involve lots of history rewriting as you refine a branch, but it makes life way easier for everyone else.
|
|
#
?
Mar 9, 2020 23:01
|
|
|
Cuntpunch posted:1. Spin up a branch to explore some new stuff Step #4 should be "Realize that the types in #2 need some additional work. Commit. Merge the new commit into the one created in Step 2. Push.". With Gerrit's workflow you edit your existing patches to fix problems in them rather than create new ones. There's no reason that the first version of a commit you push needs to be the one that makes it to master (and it typically [i]shouldn't[i] be), and it's perfectly fine to push new versions of a commit while the current version is being reviewed.
|
|
#
?
Mar 10, 2020 00:43
|
|
|
I've never understood the Git obsession with rewriting history and "clean" history before. I know about bisecting and everything, but that just seems like a thing that people CAN do rather than a thing that people DO do. In my experience, people just do a bunch of commits, push them, open a PR, and then squash merge it all down and call it a day, and that's never been inadequate.
|
|
#
?
Mar 10, 2020 02:56
|
|
|
New Yorp New Yorp posted:and then squash merge it all down and call it a day, So you've ... always worked with people that put the effort in to keep a clean history in master?
|
|
#
?
Mar 10, 2020 03:49
|
|
|
Ironically it takes some skill to keep history clean, but you usually only want a clean history on key branches if you make lots of mistakes and need to revert atomic changes quickly. Most teams just tag releases and revert to last known good release. Depends on what and how you deploy.
|
|
#
?
Mar 10, 2020 04:26
|
|
|
Jabor posted:So you've ... always worked with people that put the effort in to keep a clean history in master? See the above few posts about how people are committing "wrong". That's what I'm talking about. Squashing upon completion of a PR takes 3 seconds and results in a clean history, fine, easy enough. Having to spend 45 minutes going back and obsessively rebasing and "fixing" prior commits so that a code review tool will work properly seems insane and oppressive and like maybe the tool itself is lovely.
|
|
#
?
Mar 10, 2020 05:19
|
|
|
The equivalent of squashing your PR to a single commit with Gerrit is just only ever having one commit and just amending it rather than adding new commits, which takes zero extra minutes. Maybe negative extra minutes. Even when you do actually want to land several atomic commits from a branch that are being reviewed together, turning a mess of WIP commits into something sensible is more like 5 minutes of work for a large and complicated branch, not 45.
|
|
#
?
Mar 10, 2020 05:40
|
|
|
I mean, having to sidestep and subvert git itself in order to establish a work Flow (commit hooks, ignoring branches in favor of shadow ref pushing, etc) strikes me as a sign something may be wrong. Single commit for entire branch? That would be great if the attitude was not "commits should do single easily reviewed things and will be rejected if they are arbitrarily too large" That said, how often do people *really* go back to anything other than tagged state?
|
|
#
?
Mar 10, 2020 07:04
|
|
|
Your commit that you push to Gerrit should be a single reviewable unit of work. It doesn't necessarily need to be the entire branch (but generally it will be, since you send it out for review once it's ready to send out instead of doing a bunch of work on top of it first). When you iterate on those review comments, you amend that commit, then if you had other work building on that you rebase your further work on top of the amended commit. If you're pushing commits to review that you don't actually want people to review then you're definitely doing it totally wrong. I personally use bisect a lot, when you have a thousand developers making changes you can't just go around asking everyone what change might have caused a particular regression.
|
|
#
?
Mar 10, 2020 08:09
|
|
|
Jabor posted:Your commit that you push to Gerrit should be a single reviewable unit of work. It doesn't necessarily need to be the entire branch (but generally it will be, since you send it out for review once it's ready to send out instead of doing a bunch of work on top of it first). When you iterate on those review comments, you amend that commit, then if you had other work building on that you rebase your further work on top of the amended commit. See, if I understand what you're saying, I wholeheartedly understand. You do some feature work on local, commit for safety along the way, and then when it's ready for review you squash down to a single commit and push to Gerrit. Anything that comes up in review ends up as an amend on that commit for easy tracking of only the differences between the two commits. Review passes, commit is merged, all is well with the world. This. Makes. Sense. But this is not what I'm being taught the Gerrit flow is. What I'm being taught is that: You need to do feature work on local. Spin up the basic skeleton of your types? Commit. Add some functions? Commit. Add a new type? Commit. Oh hey, I can simplify this by removing some functions. Commit. Rename a few things. Commit. Put on some polish. Commit. Ok ready for review. Push. Review comes back that the names of the functions that were later removed were less than optimal, how about changing them. No, it doesn't matter that the code was removed later. Why did you push code you were going to remove? What are you? Deficient? Go back and fix the now-removed code in the earlier commit! And then rebase the whole stack and then prepare to keep rebasing the entire git history again and again and again because you keep mucking about with the tree.
|
|
#
?
Mar 10, 2020 16:04
|
|
|
Cuntpunch posted:What I'm being taught is that: This bit makes sense. You're working on a local branch, making frequent commits to track your own stuff, all hunky-dorey. quote:Ok ready for review. Push. Here's where you missed a step. You want to squash all your local commits down to a single reviewable unit before pushing. An easy way to do that is to have a separate local "review branch", and use squash merges to pull multiple commits from your working branch into a single commit on your review branch. Then you push your review branch to Gerrit. quote:Review comes back that the names of the functions that were later removed were less than optimal, how about changing them. No, it doesn't matter that the code was removed later. Why did you push code you were going to remove? What are you? Deficient? Go back and fix the now-removed code in the earlier commit! And then rebase the whole stack and then prepare to keep rebasing the entire git history again and again and again because you keep mucking about with the tree. And all these issues are from all your local commits ending up being pushed to Gerrit instead of being restructured into reviewable units first.
|
|
#
?
Mar 10, 2020 16:25
|
|
|
Jabor posted:Here's where you missed a step. You want to squash all your local commits down to a single reviewable unit before pushing. this does not seem easier than rebasing the topic branch on trunk
|
|
#
?
Mar 10, 2020 16:28
|
|
|
Jabor posted:This bit makes sense. You're working on a local branch, making frequent commits to track your own stuff, all hunky-dorey. Yeah, that's what seems to be at-odds here. I'm being told *not* to squash before pushing to Gerrit. At least not 'in general'. Make lots of small commits, and then push all of them to Gerrit, but make sure all of them are individually reviewable as well, and if there's something off, then go back and fix them and rebase the entire stack again and again and again. "Squash then push" makes a lot of sense to me. I can commit however-the-gently caress I want, but before merge it's all treated as "a thing" and if I need to fix that thing, I can amend the (squashed) commit as necessary, and Gerrit will go "ok hey, I know the diff between the first push and the second push of this commit, so the reviewer has an easier time of it." But having a single 'feature branch' of work needing its entire (local) history to be pristine - especially in super early prototyping and iterative development phase where things are shifting around a bit as new concerns are discovered. I'm...baffled at how anyone is supposed to get work done when you spend all day rebasing.
|
|
#
?
Mar 11, 2020 07:59
|
|
|
If you're being asked to push all the sausage-making then the reviewers should understand they should be judging the end product and not the intermediate steps. Don't review individual commits if the commits are subject to rejiggering.
|
|
#
?
Mar 11, 2020 08:59
|
|
|
nielsm posted:If you're being asked to push all the sausage-making then the reviewers should understand they should be judging the end product and not the intermediate steps. Don't review individual commits if the commits are subject to rejiggering. Yep, this is my confusion. I'm literally being asked to do things like rename things that get deleted 3 commits down the branch. Because reviewing is a step-by-step task and reviewing each commit atomically one-by-one and ensuring each commit, atomically, one-by-one is pure is...important?
|
|
#
?
Mar 11, 2020 09:17
|
|
|
It sounds like your team is morons, lol Try squash merging your changes into one huge blob, then separating them out into individual meaningful atomic commits, and see how long it takes them to notice that you're not actually pushing all your behind-the-scenes commits.
|
|
#
?
Mar 11, 2020 12:03
|
|
|
It can be useful to push the sausage making if you have CI integration hanging of gerrit or something like that, but that generally requires co-workers that are capable of basic human communication and know not to waste everyone's time reviewing stuff that wasn't ready.
|
|
#
?
Mar 11, 2020 13:03
|
|
|
Cuntpunch posted:Yeah, that's what seems to be at-odds here. I'm being told *not* to squash before pushing to Gerrit. At least not 'in general'. Make lots of small commits, and then push all of them to Gerrit, but make sure all of them are individually reviewable as well, and if there's something off, then go back and fix them and rebase the entire stack again and again and again. I suspect your coworkers were used to people squashing a dozen different logical changes down into one mega commit, so when they say "don't squash" they mean "don't squash like we used to", or something like that. So, just do what you think makes sense, because whatever you thought they meant (or what they thought they meant) isn't right.
|
|
#
?
Mar 11, 2020 15:38
|
|
|
Jethro posted:I suspect your coworkers were used to people squashing a dozen different logical changes down into one mega commit, so when they say "don't squash" they mean "don't squash like we used to", or something like that. So, just do what you think makes sense, because whatever you thought they meant (or what they thought they meant) isn't right. I think the distinction is that their definition of 'logical changes' is extremely fine-grained - possibly to a fanatical degree. I'm talking "if you rename a thing, that should be a reviewed standalone commit." level of granularity.
|
|
#
?
Mar 11, 2020 16:23
|
|
|
Cuntpunch posted:I think the distinction is that their definition of 'logical changes' is extremely fine-grained - possibly to a fanatical degree. I'm talking "if you rename a thing, that should be a reviewed standalone commit." level of granularity. There's a couple of different ways you can structure a git workflow, some of which are footguns and some of which work pretty well if everyone is on board. git is first designed to make it easy to email patch files full of commits to a central maintainer after all, which is a workflow pretty much no one uses. The various third party tools are usually more opinionated. Gerrit's patch set workflow specifically expects that you'll have fairly large commits that are functionally separate from one-another and can be independently edited in response to feedback during the review process. Highly granular histories are not really a good match. You can end up resolving a ton of rebase conflicts after an amend and it can be hard to match post-review fixes to specific commits, as you noticed. I'm used to rebase-heavy workflows -- we ran Critic as our code review tool which is built around review-by-commit plus using git rebase -i --autosquash and fixup! commits for review revisions, which is the sort of thing that gives people nightmares -- so I don't think rebasing a feature with a couple of commits in it is that bad. Frequently rebasing a large feature while preserving individual renames in individual commits sure does sound painful and insane though. I guess in your shoes I'd have a chat with my coworkers about what problems I've been having with what I've been interpreting as their preferred workflow combined with Gerrit's amend-and-rebase reviews, and what I'd like to do differently (e.g. squash harder for a cleaner history). It sounds a lot like they're making life difficult for themselves, but maybe they have a plan or are open to feedback. If their reply is "shut up, noob" or some version thereof: well, uh ... poo poo.
|
|
#
?
Mar 12, 2020 00:10
|
|
|
Cuntpunch posted:Yeah, that's what seems to be at-odds here. I'm being told *not* to squash before pushing to Gerrit. At least not 'in general'. Make lots of small commits, and then push all of them to Gerrit, but make sure all of them are individually reviewable as well, and if there's something off, then go back and fix them and rebase the entire stack again and again and again. This reads like trying to apply a github style review approach to gerrit, which is pretty ugly. I've worked in Gerrit for a while now and much prefer it to the lots of tiny fixup commits style and lots of small commits building the feature isn't really the norm. A feature will still involve multiple CLs (that you might be working on at once, but when your active stack is <2-3 deep rebase -i + amend isn't really much effort). You probably shouldn't have a 2k CL of "Complete feature X", but stuff like "Add API definition for X" -> "Implement X.{a,b,c}" -> ... is pretty normal for largish features but you shouldn't need to merge the entire stack for the build to be green/work. If you have deep chains in gerrit at once that's generally a sign you're either really outstripping the reviewers, you're basing commits on the same branch without needing to, or something is janky in your process.
|
|
#
?
Mar 12, 2020 06:49
|
|
|
Not really a question but I just learned some lovely news and felt like posting about it. This seemed the closest related thread I guess. Bountysource* have updated their TOS that says if a bounty hasn't been claimed in 2 years, they will steal the money for themselves. Apparently they silently made this change a month ago, and just yesterday sent out emails to previous backers etc notifying them of the change, basically giving "2 weeks notice" before their cash grab deadline. https://github.com/bountysource/core/pull/1480/files posted:2.13 Bounty Time-Out. If no Solution is accepted within two years after a Bounty is posted, then the Bounty will be withdrawn and the amount posted for the Bounty will be retained by Bountysource. For Bounties posted before June 30, 2018, the Backer may redeploy their Bounty to a new Issue by contacting support@bountysource.com before July 1, 2020. If the Backer does not redeploy their Bounty by the deadline, the Bounty will be withdrawn and the amount posted for the Bounty will be retained by Bountysource. So if you work on any project that uses BountySource or have backed anything through them, I would recommend withdrawing any funds and finding a less lovely alternative. I contribute to a free software project that is still maintained but development moves rather slowly, and has quite a few old bounties like this. So the community around our project is understandably upset at this change. * In case you are not familiar, its a sort of crowd-funding platform where people can put up money for specific issues/feature requests, and a developer who resolves it gets to keep the cash. They have integration with github, gitlab, maybe others, which is why I figured the news was version control adjacent.
|
|
#
?
Jun 17, 2020 22:45
|
|
|
Back in the day, I learned hg and git each for a couple weeks, and chose hg. Unfortunately (for me), git has "won", and now we're hurtling towards losing hg support in BitBucket. Let's assume I want to continue to use hg locally. Here's how I think I might do this. I could convert my hg repo to a git repo with a tool like "hg-fast-export" and host that on BitBucket. Then, as I commit changesets to my local hg repo, I could push them to the git repo with an hg extension, "hg-git". Let's assume I only ever need to push changesets to git, never pull. (We could host all our hg repos internally, the software team would continue to use hg, and push/pull changes to internally hosted hg repos, and periodically those hg repos are pushed into BitBucket's git repos.) Is this all reasonable, so far? epswing fucked around with this message at 19:45 on Jun 18, 2020 |
|
#
?
Jun 18, 2020 19:37
|
|
|
epalm posted:Is this all reasonable, so far? No, it's jumping through crazy hoops to continue using a fading tool. Just use Git.
|
|
#
?
Jun 18, 2020 20:09
|
|
|
Is the bitbucket repo in this setup just for off-network backup in case of disaster or will your CI/releases build from it? If the former, I’d just use a regular file backup service. If the latter I’d just swap the team to git or find a different host. If you absolutely can’t I would just nest the entire hg repo inside a git repo, that seems less error prone than translating commit graphs between systems forever. Edit: but yes agree above that the real answer is drop your assumption that you need to keep using Hg.
|
|
#
?
Jun 18, 2020 20:15
|
|
|
New Yorp New Yorp posted:No, it's jumping through crazy hoops to continue using a fading tool. Just use Git. Yeah, I know, "just use git" is the correct answer. Having a building full of middle aged devs doesn't help though. If I can buy us more time I will, but yes, the plan ultimately will be to move off the fading tool. Eggnogium posted:Is the bitbucket repo in this setup just for off-network backup in case of disaster or will your CI/releases build from it? If the former, I'd just use a regular file backup service. If the latter I'd just swap the team to git or find a different host. If you absolutely can't I would just nest the entire hg repo inside a git repo, that seems less error prone than translating commit graphs between systems forever. It's for a couple things: off-network backup like you said, but also integration with Jira. It's nice to open an issue, and then pull up and jump to all the related commits, which are sometimes in more than one repo. This works by mentioning the Jira issue key in the commit message, and Jira periodically pulls from the repo you specify and hunts for those issue keys.
|
|
#
?
Jun 18, 2020 21:32
|
|
|
|
| # ? Apr 26, 2024 02:36 |
|
|
As a long time git user who recently had to adopt hg for work, they're really not that different.
|
|
#
?
Jun 19, 2020 00:44
|
|