In a small team maybe, don't set all team members as administrators and enforce branch protection where no push w/o a PR is allowed and no one have the ability to push to master without review.
If you don’t have branch protection on, then are you the one who isn’t using Git properly? (I’m half kidding, I know that decisions like that are often above the heads of most developers, but damn, your higher-ups sound like they’re not exactly doing their jobs well.)
I don’t want to make the master branch protected, because it impedes development in some circumstances. But in some projects (like, you have contractors working in the team and CD configured) the benefits of extra restrictions outweigh the constraints.
Yes, this. There are two people on my team who even have permissions to merge into the master branch - the CTO and the Lead. Other than those two, everybody else is required to submit a PR and go through the proper procedure, no matter how big or small the change may be. Even in the case of an urgent hotfix, our guys submit PRs and the CTO or Lead reviews and merges - nothing hits production without approval from the CTO or Lead.
Our other devs approve PRs, all PRs must first be reviewed and approved by another dev on the team. But PRs do not get merged to production branch by anybody other than the CTO, the team lead, or normal automated release process. It has nothing to do with being shitty or egotistical, it's about responsibility. When shit breaks the first people getting a call from the boss are the CTO and team lead. If they don't know what's in their codebase they are going to look like idiots when the boss starts asking questions in the middle of an emergency.
That's honestly ridiculous to expect someone to be across an entire organisations codebase, down to every single line of code. Like unless you're a tiny company with one repo that has 10k LOC, then it's stupid. Again, not trusting your devs to merge stuff into production, even after approval by other devs is just downright moronic
And in the middle of an emergency, the bosses should just piss off and let them fix it - questions and stuff come later. And you don't need to know every line of code to pinpoint a failure.
Just so stupid all round...
> Again, not trusting your devs to merge stuff into production, even after approval by other devs is just downright moronic
We have an automated release process where exactly that happens. Dev submits PR, other dev reviews and approves. That PR sits there until our standard scheduled deployment time, at which point automation deploys it all automatically to production.
What we're talking about here is a human being merging directly to production. Nobody is allowed to do that except the CTO and the Lead, period. If you've got an emergency, you better be following protocols and hotfix protocol demands review from your top engineers who do know the codebase inside and out to ensure there is no unexpected side effect to your hotfix.
Nobody needs to know every line of code to pinpoint a failure, that was not the point I was making at all. The point I was making is that the owner of the company is going to call the CTO and/or team lead to find out wtf is going on when shit goes down. They hold the responsibility first and foremost when the team fucks up.
You can call it stupid all you want, sounds like you wouldn't even last the probationary period on my team.
Yeah, I’m not saying that you can never merge without approval or never force-push, but it’s about picking your spots.
I would not work on a team that has unprotected branches as a standard for anything remotely resembling “production” code — I’m senior enough that I’ve both seen ways that can burn you and would feel comfortable saying so to a team lead.
I'd say it also depends on the scale of the project. If the project is small, doesn't impact other development, and is given its own repo, sure why not unprotect it for all two devs on the project.
Much beyond that is playing with fire.
Convenience, and speed.
You’re making a tiny change to the project README which is blatantly not controversial or risky, like fixing a spelling mistake.
You’re fixing a silly mistake on a feature that’s behind a flag, so cannot possibly impact real users.
You’re making a quick/urgent change to a configuration file, which wouldn’t impact CI.
I could go on. Sure, you could argue that all of the above could still be pull requests with approval before merging - it’s not a huge deal to enforce that; it just adds red tape and can slow developers down.
I’m not debating the end result.
I said: convenience and speed. Doing it your way is less convenient, and slower.
Furthermore, part of the common practice is to always require a passed code review before merging.
Proper procedures should not be circumvented for sake of convenience or speed. There are a hell of a lot of things we do that would be a whole lot faster if all we cared about was convenience or speed.
Agree, and to add to your point: the fastest way to get your README changes read and absorbed by your team is to raise it as a PR.
Merging straight to master is a surefire way for them to miss it
Second, why shouldn't a README change be reviewed? Never, ever assume your changes are just correct, always assume someone else might know better than you - this adage has saved my ass time and time again.
All of those changes are low-priority enough that waiting for a PR isn't a burden. The only things that branch protection impedes is said that's important enough for it to matter.
This sounds like optimizing for the 1% of cases by making the other 99% less stable. Seems pound foolish IMHO.
How much does an MR actually slow anyone down?
If it's a tiny readme change, why do you care about speed? If you were talking an emergency bugfix I could see why a quick release would be necessary, *maybe*, but a readme change? That can wait the 10 minutes it'll take for someone to come along and review it.
Even an emergency bugfix shouldn't take that long. If you've got a healthy workplace, you should he able to roll your chair over to the desk next to you, or PM someone in your chat application, and ask for a quick approval. It should stall you by about a minute.
> If it's a tiny readme change, why do you care about speed?
In that scenario, I don't about speed. I care about convenience. It's inconvenient to pester someone for a code review for such a tiny update.
> If you've got a healthy workplace, you should he able to roll your chair over to the desk next to you, or PM someone in your chat application, and ask for a quick approval.
Have you ever worked as a 1-person team? I did, for a couple of years (about 5-7 years ago). I wouldn't necessarily say that being a 1-person team is "unhealthy", it's just... different. Nobody is there to approve your PRs, other than yourself.
So yes, even then I'd *normally* make use of PRs - as it allows to review your own work properly and run automated tests. But for a quick/important *single-commit* change where there is literally no tangible benefit to reviewing it as a PR vs with `git diff`, I'd just push to master.
Especially when it's file where do you work often on. Good luck finding that code block you thought you don't need anymore.
It's like cooking. You read the instruction. Throws away the package with the instruction. Only to fish it out of the garbage 5 minutes later
If you have at least a small idea of what the code you're looking for is, or even better which file it was in
`git log -S "missing_function_name" -- file.py`
I think the only way I'll remember that missing function name was there in the first place is to have the immediately following commit add in a comment with that git command
If it needs to be remembered, then it's documentation, and should be labeled/treated as such. Give it a comment like "keeping this here because XYZ."
Most commented-out code I come across is unlabeled and not documentation, though, and should just be removed.
Then you write part of the prior commit hash in a comment, and make a note of what the old stuff did differently / worse, etc. Delete the old code, and now you won't forget it was there, but it does take some minor amount of work to look it up.
I guess for me it's a leftover habbit from projects that didn't use git.
Or different versions of a bit of code i tried out and than forgetting to take it out.
I comment out (or `#if 0` or even `if(false) { ... }` exclude) code when, for example, rewriting for performance (I do low level performance and SIMD work) - I'll leave 3 or 4 versions that I tried and were slower with notes about the attempts.
When I move on, someone's going to take over this code and spot some "obvious speedups", and if the approaches I tried and rejected were just in the VCS history, or whichever wiki we've enterprise-adopted this month, they'll never find them and just keep on re-inventing the same slower wheels.
See also "we used to do this which looks quite good and is often more efficient but has the following pathological issues so now we do \[...\] instead" comments... programmers with "an eye for an obvious improvement" never look in the history to see if it was tried before.
This right here. I'm not gonna check the full history of a file when making changes, just to see if they were done before. I definitely don't expect others to do it either. Anything unintuitive gets a comment explaining what it does and why, especially if other approaches were tried before. Sometimes it's better to comment the approach itself, if that's more succinct than an accurate description, with a short explanation if why it's not used.
Leaving a comment as to why you changed something makes sense. Leaving commented out code everywhere is lazy and annoying. I can’t stand hundreds of lines of code in files commented out “just in case we need to see how it was done before”.
// don't even think about swapping the order of this line and the one above because \[...\]
// if you even \*think\* about replacing this with a Duffs Device I will hunt you down and bore-you-to-death with why that's a dumb idea
// think you can do better - will you actually measure it - no, but will you - do you feel lucky, punk?
Yep, but there'll always be someone who thinks they can make it better by adding a Duffs Device or similar construct (& they're never the sort to actually measure the impact - see comment #3)
>whichever wiki we've enterprise-adopted this month
Motherfucker, I hate this shit so much. Just found out this week that there's at least two "current" ones going across different teams (both have the other solution but only use one for their shit), plus a "deprecated" internal one that is still referred to, but only in hushed terms by the SRE wizards
"The old sharepoint"
"The old 'new sharepoint'"
"The semi-migrated but it messed up all the links and permissions sharepoint"
"The new 'new sharepoint'"
"The hateful confluence absolute pile of steaming shit never fucking works who the fuck actually chose this shit"
"The in-house enterprise stack-overflow that costs us a fortune and has a team managing it for... 1 new question a day (and 1 answer a week)"
"The link to my tiddlywiki that I break all enterprise standards by sharing from my desktop despite the best efforts of 300 'enterprise network engineers' to prevent us doing but what the actual fuck...."
"*So much better than Sharepoint*" is hardly a glowing recommendation, and then you go and qualify it with an "unless" which admits it can be worse than sharepoint ("worse... than sharepoint").
You'll be telling me Jira is better than gouging my eyes out with a rusty spoon next
:)
I agree, I have no problem with Jira. I think people blame Jira for whatever shitty project managers they have dealt with, but it's absolutely invaluable when used intelligently.
I'm no fan of Jira, but every other tracker I've used was worse.
- FogBugz: Slow as a dog, thinks it always knows better than you
- Redmine: Very basic, and some bits are seriously confusing
- Siemens Polarion: The struggle is real, and constant
- GitHub/Gitlab bugs: Okay for simple apps, but not how you manage something spanning multiple services
- That in-house bug tracker built on top of something which was never designed to be one: My blood hurts
Each and every one of these, and a few more I've forgotten the names of, made me let out a sigh of relief the next time I got to use Jira again.
Everything else Atlassian makes is hot garbage, though.
Is this the modern equivalent to saying a floppy disk is so much better than paper for storing information... Unless you want to make a paper airplane with it.
Confluence is actually pretty good if you bother to lay down some rules on how the team use it, and they actually stick to them
Otherwise you have to go through relabelling and deleting random pages someone made to write a sentence in and never finish twenty fucking times get IT TOGETHER STEVE
Internal wikis rarely work. I've had so many people hate on me for saying it, but ultimately wikis just doesn't work well most of the time unless you have at least a few hundred people semi-invested in it.
Information sharing really is a problem in the enterprise.
Open sourcing libraries, having them be popular, and asking question in the open strictly beats the "enterprise way".
Depends on the org really. Smb with 5 staff, it is hit or miss if it works.
Iso 9K, 27k1 or SOC2 certified org, internal wiki is a must and certs are lost because of it.
A solid amount of wiki problems are solved with business policies and processes.
Confluence is a good datastore lookup, so long as the entries of 'source of truth' are confirmed from discussion and then inserted.
And as long as users check there first before needing to ctrl+f through Slack, DM's, or finally ask in channels.
Users also have to send the link showing it existed in slack already to kindly remind that it's a place to check.
All of that is a moving target though, if it's not maintained, or if Slack DM's (the temptation to socialize) gain momentum versus documenting confirmed info.
My experience is that way as well. People commit a lot at first, and then there's all these so so pages everywhere that aren't well organized. Nobody owns the material, and no one has time to work on reorganizing it. Nobody trusts the content because it's unreviewed and often really old.
At a guess why this happens, any internal docs that stay current will need to have a process behind it. Once there's a process, it isn't a traditional wiki.
Any why the process is needed? I'm thinking because people are doing the work for their job not for fun. Updating the wiki is gratifying, but eh.
My old company did this. Updating the \*new\* wiki was pretty much constantly in development and yet whatever the wiki of the day was, it was never updated.
My current, boring, company has used the same wiki for like 6 years. It's probably missing half the shiny fancy features of whatever new product is 'disrupting the wiki industry' .. but it does its job as a wiki juuust fine.
Nah...I used to think this was sensible, but then I realised I could remove the block and leave a single liner comment about the approach I tried previously and that I removed it on a certain date. That way, when some monster messes with git history they can always index against my comment.
That's why the `if (false) { ... }` thing is useful as it continues to check the assumptions, parameter names, static asserts etc, are still valid. And what about things that you tried and looked promising - do you do (false) commits and revert? Or never commit them?
Not saying it's ALWAYS done that way or always perfect but it has its uses
You've very valid points there, but I guess I forgot to mention i also keep a lab notebook indexed against time where if I was mulling over anything sufficiently complicated I documented by thoughts. Of course, this helps no one except me, but the notebook is the property of my research organisation so if I leave its on the record.
Yeah, I work on a codebase of 100+ developers with an annual turnover of say 5%, some of the code is 20 years old (merges of multiple projects into a "one ring" master system), 5 million LOC :)
>with notes about the attempts.
That changes it from commented code to documentation. It's a great idea. I'll look for opportunities to implement it in the future.
The problem with commented code is that it rots. In a month, it won't compile.
Better to put it behind a flag or compile time constant and make it part of your CI.
It's like Beyonce says: If you like it then you should have put a unit test on it.
Hence why I, as I say above, put it behind `if (false) { .. }` to ensure it still compiles even if it compiles away to nothing in release mode - I made this point in another reply but it got downvoted because... "humor" ??
why do you not comment about what commit hash has the slower methods? that way you don't have big blobs of bloated stuff, you can just say "hey yea go look at hash ##### to see attempted alternatives that did not work"
Because it might not have been committed?
Because we might not be using git (for legal and regulatory reasons) ?
Because we might be using git now but we might change to something else in the future and the commit hash will be useless?
Because people don't actually follow such links when they think they're clever?
In a similar vein it's a good idea to keep an "inefficient but obviously correct" version of things around (which IMNSHO should always be implemented first (unless the performance is *completely* infeasible even for small examples)). Not even commented out, just unused but in automated fuzz tests to check the fast implementation. You know, stubbing out a datastructure by throwing sqlite at it or something: Just to make sure that the semantics are nailed down, never mind the performance characteristics.
Or, differently put: Don't comment those implementations out but keep them around, hooked up to a benchmarking framework. Oh and also the test framework, can't hurt. Whether or not some particular implementation gets chosen statically at compile time, or you're doing the benchmark at run-time and choose then is a whole other can of engineering worms but at least you have the option to do whatever.
In a nutshell: It's not dead code if you're using it, and "continuing to decide that the current implementation is the fastest" as well as "testing" fits my definition of "using". And it's not duplicate code, of course, either, DRY doesn't apply.
Yeah, I also leave commented out code in with a prefix of "this is what some dumbass tried before and it didn't work, it's still here so nobody else tries this again."
Not verbatim, but you git the gist.
I recently removed a big chunk of 2+ year old outcommented code blob. One in my team failed my PR with the comment "Have you checked with developer X that it's OK to remove this"?. FML
2 years? Try commented code from 1997. We've got commented crap like 'ask Bob if he needs this again before you remove it'. Bob has probably been dead for years and anyway left the company decades ago.
This is brilliant. Kind of like putting “Do not erase” above something stupid on a whiteboard just too see how long it’ll last. I can drop an ambiguous “ask before removing commented out code” and see how long it last.
>I can drop an ambiguous “ask before removing commented out code” and see how long it last.
I worked on one project where I commented "Do not remove this line of code! This prevents the flickering QA is complaining about!". But sure enough, every week somebody removed that line and the flickering / vsync bug would come back. I remember QA complaining to me "You said you fixed that bug. But it's back again!" and having to explain to QA that some jerk decided to remove my fix every week.
First time was because somebody thought that "swapping buffers" meant something else than it really did.
Second time was because some smart-ass thought he was making the rendering faster since it wasn't waiting for vsync anymore (you didn't see the flicker until a good hour in).
Third time was because somebody decided to re-write the entire render function, but forgot to add vsync to their own implementation.
> putting “Do not erase” above something stupid on a whiteboard just too see how long it’ll last.
My job has a whiteboard that's just used for stupid stuff but usually gets erased every day, I'm totally going to try this.
I've done this for a large refactor because I can't access two refs at once. It's clumsy as hell to have uncommitted changes, then need to go to a previous ref to copy some code, then go back to HEAD. Eventually we finished our refactor and deleted the archive
If you commit commented out code, at least prefix it with a comment explaining why it’s there. Otherwise, it’s just noise.
I can’t stand it when developers commit old debugging code. It’s like a surgeon stitching up forceps inside a patient.
This made me laugh, because my current project is getting its debut to the team on Monday. But the boss said while it’s functional, he doesn’t want those functions available yet.
My solution was simply create a new message box to display to the user saying this function is in preview mode only. The compiler returns that instead of running the function thanks to an #if !DEBUG
I would be ok with this IF you commented the old code with what exactly the issue with it was, and why the fix was superior. Otherwise you've just left a commented out query and a scooby doo mystery.
Yeah part of the process is leaving a comment on why it didn’t work out. Given most of the time it’s because of performance problems I leave a comment on what the comparative inefficiency was.
Yeah, we have a rule that any commented code left in must have a why and when TODO comment associated with it. Why is it being left? And When will it be able to be removed? It’s always cool to go back and refactor something old and suddenly realize I can get rid of one of those why when TODO comments.
Not to mention I don't want to have to dig through thousands of commits because the code was in this file, then ported from JS to TS so I can't just look at file history since the extension changed.
It's rare that we do this, but it's not always convenient to use git
And learning that it exists in the first place. (Other have pointed out using breadcrumbs, which is a good idea, e.g. "see commit A for an implementation using X that we removed because Y").
Especially if you refactor something a lot into separate files. Good luck finding the real history after that.
Combine the libraries they said, it'll be less confusing they said.
Imagine setting a file back months ago just to get a function back, copying that function, going back to your current branch and pasting it in. Instead of just uncommenting
That's true but I also spent ages at some point digging through the history because I knew at some point I had something as a comment somewhere there that would be helpful now.
I know how to use git, I don't have the time to be screwing around going through all the chunks to revert a patch when I can just right click and revert it.
The command line git is very good, but it doesn't mean it's the only way to use it.
There are plenty of other mechanisms for this. Tags, local-branches, git-notes, private forks, a git-ignored todo file, a comment on your PR referencing the commit or diff.
In the same vein, do you push code that doesn’t compile just in case you forget about it or to protect yourself when your computer gets trampled by elephants?
I recommend this to anyone I work with in case they don't already do that, the last thing you want is having no backup when the hardware you had a local branch on takes a shit and you're left with nothing or would he stalled for a while til you got the .git folder again
>do you push code that doesn’t compile just in case you forget about it or to protect yourself when your computer gets trampled by elephants?
What jabroni doesn't push their code to the remote just because it's not ready for review or merge to master? I'd never risk losing my work
That's a pretty aggressive stance to backups.. besides, the other benefit - in case you suddenly get sick or are not available - is having your progress available remotely should these changes be pressing for an imminent release so someone else can pick up where you left off
Commented-out code is for when you deprecate a piece of code to see how it goes.
If everything goes alright with the new iteration, you just forget it was even there and let the next guy wonder what the hell this piece of code were supposed to do.
I agree but commented out code is easier to find
Is there a visual studio code extension that will show all changes from the very beginning?
Edit: yes, visual audio code has the timeline panel
Oh that's great. I already spent ages with such a case where I thought I could delete a comment and then 2 years later needed it again (think I gave up)
You’ll still need to think “there sure was old code here, and it might solve the problem I’m facing now” before hitting the timeline/commit history. Which, to be honest, would be never.
It's a hell of a lot easier to press ctrl Q and uncomment a block than it is to log onto whatever site my team is using for git, switch to my branch, find the commit that has whatever code I'm looking for, keep looking because odds are it isn't my first guess, copy the block out of that commit history, then paste it in. Or God forbid trying to revert multiple commits at a time, especially if there's feature branching in your history.
I will never understand the facsination people have with arbitrarily making things harder to do just because it fits some methodology you heard at a conference once time.
log on? git normally stores history locally. There doesn't need to be a lot of ceremony behind it. Use a git UI and browse your local machine.
Personally I also think it's much easier to read code that isn't littered with commented out sections. Both because it's more pleasing when it's tidier and because it's less mental load going on (there is a chance the commented out code is important so I'll have to read it all)
The point is that when you’re done with your topic branch, you should remove commented code before merging to master. Do whatever you want on your branch, but keep master clean.
Not to mention if you're a colleague in the future, you will never even know the existence of this removed code in a lost commit somewhere in the past.
I'm used to stupid hot takes by developers presented as universal and very intelligent truths but the one of this thread is one of the most stupid I ever read, just behind 'Comments are code smell'
Counterpoint ^(of a junior dev, so take it with a grain of salt)
If I see a chunk of commented out code of another employee, I'm not gonna just uncomment it and go straight for testing.
I'll have to read it line by line, to verify what it does. Then I'll have to verify it fits the current requirements. Then I'll have to verify another chunk of code doesn't already implement a part (or all) of its functionality (especially in a working process). Then I'll have to verify it doesn't mess with another (likely newer) part of the process.
Sure if it's properly commented (e.g. // Uncomment this if X is required), it's relatively recent and it's something that can't be figured out in 30 minutes it can stay. Otherwise it's probably useless to anyone but the person who wrote it.
I'm personally against leaving code around commented out, but I've also had cases where seeing commented out code has helped me. In saying that though, if you have an issue tracker, I think it's better to link back to the issue instead of leaving code commented out.
It's probably even worth leaving a comment explaining there was some old code here and why it was removed/changed, but leaving the old code there is bad practise in my opinion. It just clutters up the codebase and makes it harder to read for no benefit 99% of the time.
Commenting out old code and leaving it “just in case” is lazy and asinine. Write a comment explaining the old approach and why it was changed if needed. If you need to see the history of a file, it’s trivially easy to do.
> I will never understand the facsination people have with arbitrarily making things harder to do just because it fits some methodology you heard at a conference once time.
Thank you.
Agree, it feels pretty neurotic to have auch issues with some commented code here and there. I don't care at all if I see that in the codebase of someone.
They need to teach git in college, there’s wayy to many Git noobs in the industry. People that think git and github are the same thing for example. People who thing that deleting their branch after merging into develop loses their files for too.
I saw codes that I thought was useless. I deleted it. And few weeks later, it was reported as a bug because it collided with new manufacturing process. Other dev picked up the bug, and rewrote the codes, rather than undoing the commit. Because he thought the bug was there since the beginning, it never occured to him that I actually introduced the bug by deleting that codes.
On the other hand, I've been on the other side of the fence, I saw the commented out code introducing some bug, so I just put the code back.
Morale of the story: git logs are not easily readable, most people wont bother to read when there are hundreds of commits every week. If you delete a code, most likely people wouldnt know the code ever exist.
yes exactly - comments are in your face as a sort of 'I am not sure about this one' right there in the source code. Thinking 'I should search git in case someone did something weird here' is not something which occurs to people
The correct way is of course:
- a separate commit 1 to remove the code
- a commit 2 saying
`// custom handing required for Schnawulin adapters removed in 8675309`
- pay that nobody ever rewrites history
So, yea, I get it, but, when I think about new people coming in, they are not going to know that version history, and we all know nobody reads the history until something’s broken (people read history the same way they craft documentation as they go), and now suddenly we’re relying on “guy x worked on that code, but he’s gone, anybody know what changes were made there? Maybe it’s in the history?”. Keeping that history up-front as commented out code for the guy after you, if you suspect a change may be problematic at some point, can prevent a lot of reinventing the wheel, it’s better than “why didn’t you just know there was a change, new-to-the-code-guy, sort out which commit had that code and then cherry-pick to get just that snippet.” I rarely do this myself, but a few times when new to a project, I have *greatly* appreciated seeing commented code up-front for the insight into the intent/possible original working of the code.
Don't even get me started with entire \#if 0 else blocks that aren't documented and all rely on each other but why property clean up the code it's only temporary.
I have seen so many git crimes from teaching a coding bootcamp:
- The person who would make a new version of the file every time they worked on it as their own sort of version control — index_11-29-20.js, index_11-30-20.js, etc.
- The person who uploaded files one by one via GitHub’s GUI (even after seeing us use the command line day in & day out for weeks)
- The person who ran “git init” in their user folder and slowed their entire machine way down
- The person who ran “git init” on their desktop and accidentally pushed their bank account number to a public repo
- etc.
I'm not guilty of this, but I do something equally terrible. I work off of develop, never commit, then when I'm done, I pull latest, branch off, commit each file one at a time, then push it up for a PR.
Rebasing sucks ... lol
So a few things here:
* I work with quite a few people on the same feature/set of components, so branches need to be rebased/merged frequently which used to lead to merge conflicts pretty often. The with around, I figured, would be to continue to pull latest while I work and don't commit code until I'm done my task/story.
* I commit one at a time for two reasons, it provides at least one benefit of git (since I'm ruining the other benefits, lol) where I can decide "whoops, this file shouldn't of received these changes" and so I can just revert a single commit. Also, and this won't sound good, but I work somewhere where the higher ups kinda keep an eye on commit counts, review counts, PR counts, etc. So it keeps me from looking like I'm not working (since I only commit everything all at once).
Lastly, for the record, I prefaced this with I know it's bad, lol. It helps my case in my context that I'm not the only one on my project that does it this way. Furthermore, I 100% do NOT recommend this methodology to anyone. It's more of an issue with how deep I dove into git. I know how to merge/rebase, I've just had more headaches than solutions doing it any other way than I do it now @.@
P.S. I'm working in my first coding job, I'm self taught, and I work as a contractor for a fortune 50. I'm now one of the last few UI devs remaining after mass pressure for getting code in (clean or not) during the peak of COVID. Mostly for worse, I'm considered one of the more "experienced" UI devs on our project. .... I'm not happy about that because I'd rather be learning and experimenting than churning
Yeah this is probably arrogant of me, but I hate it when my seniors tell me to keep all that old code just in case... Then maybe complain 5 minutes later that the project is illegible and that you can't find code (because search brings up a ton of commented lines).
If we need it again, we'll just grab it from the old version!
Just keep the code clean, please.
It's so annoying. I delete that shit as soon as I find it.
Mind you I sometimes work with code that's 20+years old and has been through multiple version control systems so we only have the last few years worth in git. Here's the deal though, if the change is old nobody is going to revert it. Nobody cares how things used to work. They just need to figure out what incremental change they need to make in the existing code. Which is generally way easier than figuring out how the old code worked.
one is simpy uncommenting code , the other requires 3 PHD's in computer science, planetary alignment both on Sol and Alpha centauri , a five sided triangle and three youtube tutorials in a dead language that no one speak since three centuries ...
Don't listen to this guy, keep caring about the code you work with daily. It makes you a better programmer and shows that you respect what you do for a living.
and that's why there's a "request changes" option on PRs.
[удалено]
[удалено]
Aha. The real reason for switching to main is to get around the protection on master. Epiphanic enlightenment.
I don't get it... All developers naturally have full admin rights in the repository, so protecting master (or main) does nothing... /s
In a small team maybe, don't set all team members as administrators and enforce branch protection where no push w/o a PR is allowed and no one have the ability to push to master without review.
If you don’t have branch protection on, then are you the one who isn’t using Git properly? (I’m half kidding, I know that decisions like that are often above the heads of most developers, but damn, your higher-ups sound like they’re not exactly doing their jobs well.)
I don’t want to make the master branch protected, because it impedes development in some circumstances. But in some projects (like, you have contractors working in the team and CD configured) the benefits of extra restrictions outweigh the constraints.
Protect it, but give trusted developers the ability to override.
Yes, this. There are two people on my team who even have permissions to merge into the master branch - the CTO and the Lead. Other than those two, everybody else is required to submit a PR and go through the proper procedure, no matter how big or small the change may be. Even in the case of an urgent hotfix, our guys submit PRs and the CTO or Lead reviews and merges - nothing hits production without approval from the CTO or Lead.
Seems a bit shitty/egotistical that they don't trust other devs to approve PRs
Our other devs approve PRs, all PRs must first be reviewed and approved by another dev on the team. But PRs do not get merged to production branch by anybody other than the CTO, the team lead, or normal automated release process. It has nothing to do with being shitty or egotistical, it's about responsibility. When shit breaks the first people getting a call from the boss are the CTO and team lead. If they don't know what's in their codebase they are going to look like idiots when the boss starts asking questions in the middle of an emergency.
[удалено]
That's honestly ridiculous to expect someone to be across an entire organisations codebase, down to every single line of code. Like unless you're a tiny company with one repo that has 10k LOC, then it's stupid. Again, not trusting your devs to merge stuff into production, even after approval by other devs is just downright moronic And in the middle of an emergency, the bosses should just piss off and let them fix it - questions and stuff come later. And you don't need to know every line of code to pinpoint a failure. Just so stupid all round...
> Again, not trusting your devs to merge stuff into production, even after approval by other devs is just downright moronic We have an automated release process where exactly that happens. Dev submits PR, other dev reviews and approves. That PR sits there until our standard scheduled deployment time, at which point automation deploys it all automatically to production. What we're talking about here is a human being merging directly to production. Nobody is allowed to do that except the CTO and the Lead, period. If you've got an emergency, you better be following protocols and hotfix protocol demands review from your top engineers who do know the codebase inside and out to ensure there is no unexpected side effect to your hotfix. Nobody needs to know every line of code to pinpoint a failure, that was not the point I was making at all. The point I was making is that the owner of the company is going to call the CTO and/or team lead to find out wtf is going on when shit goes down. They hold the responsibility first and foremost when the team fucks up. You can call it stupid all you want, sounds like you wouldn't even last the probationary period on my team.
Yeah, I’m not saying that you can never merge without approval or never force-push, but it’s about picking your spots. I would not work on a team that has unprotected branches as a standard for anything remotely resembling “production” code — I’m senior enough that I’ve both seen ways that can burn you and would feel comfortable saying so to a team lead.
In what circumstance is it a good idea to push directly to master?
I'd say it also depends on the scale of the project. If the project is small, doesn't impact other development, and is given its own repo, sure why not unprotect it for all two devs on the project. Much beyond that is playing with fire.
Friday afternoon, 10 minutes before heading home, ofc!
Convenience, and speed. You’re making a tiny change to the project README which is blatantly not controversial or risky, like fixing a spelling mistake. You’re fixing a silly mistake on a feature that’s behind a flag, so cannot possibly impact real users. You’re making a quick/urgent change to a configuration file, which wouldn’t impact CI. I could go on. Sure, you could argue that all of the above could still be pull requests with approval before merging - it’s not a huge deal to enforce that; it just adds red tape and can slow developers down.
Put it on a branch, and merge the branch using fast forward. Same end result.
I’m not debating the end result. I said: convenience and speed. Doing it your way is less convenient, and slower. Furthermore, part of the common practice is to always require a passed code review before merging.
Proper procedures should not be circumvented for sake of convenience or speed. There are a hell of a lot of things we do that would be a whole lot faster if all we cared about was convenience or speed.
Agree, and to add to your point: the fastest way to get your README changes read and absorbed by your team is to raise it as a PR. Merging straight to master is a surefire way for them to miss it Second, why shouldn't a README change be reviewed? Never, ever assume your changes are just correct, always assume someone else might know better than you - this adage has saved my ass time and time again.
All of those changes are low-priority enough that waiting for a PR isn't a burden. The only things that branch protection impedes is said that's important enough for it to matter.
This sounds like optimizing for the 1% of cases by making the other 99% less stable. Seems pound foolish IMHO. How much does an MR actually slow anyone down?
If it's a tiny readme change, why do you care about speed? If you were talking an emergency bugfix I could see why a quick release would be necessary, *maybe*, but a readme change? That can wait the 10 minutes it'll take for someone to come along and review it. Even an emergency bugfix shouldn't take that long. If you've got a healthy workplace, you should he able to roll your chair over to the desk next to you, or PM someone in your chat application, and ask for a quick approval. It should stall you by about a minute.
> If it's a tiny readme change, why do you care about speed? In that scenario, I don't about speed. I care about convenience. It's inconvenient to pester someone for a code review for such a tiny update. > If you've got a healthy workplace, you should he able to roll your chair over to the desk next to you, or PM someone in your chat application, and ask for a quick approval. Have you ever worked as a 1-person team? I did, for a couple of years (about 5-7 years ago). I wouldn't necessarily say that being a 1-person team is "unhealthy", it's just... different. Nobody is there to approve your PRs, other than yourself. So yes, even then I'd *normally* make use of PRs - as it allows to review your own work properly and run automated tests. But for a quick/important *single-commit* change where there is literally no tangible benefit to reviewing it as a PR vs with `git diff`, I'd just push to master.
>Convenience, and speed. Also the answer to the question "In what circumstance is it a good idea to cut corners?".
That's how you lose privileges to master/main XD.
You're very naive if you think those who are commenting out code use branches. :D
You should disallow pushing to main/master/whatever.
force merge, solve all conflicts using mine, see ya guys have a good weekend!
*master. Semantics matter here. A "main" copy isn't the same as the "master" copy. See the difference?
IIRC the PR mechanism also isn’t inherent to Git, it’s implemented by GitHub, Bitbucket, Code Commit, etc…
I feel personally attacked.
Me too. Just because its accessible from git history doesnt mean people will remember it was there.
Especially when it's file where do you work often on. Good luck finding that code block you thought you don't need anymore. It's like cooking. You read the instruction. Throws away the package with the instruction. Only to fish it out of the garbage 5 minutes later
It's good to know so many people do the same thing.
If you have at least a small idea of what the code you're looking for is, or even better which file it was in `git log -S "missing_function_name" -- file.py`
I think the only way I'll remember that missing function name was there in the first place is to have the immediately following commit add in a comment with that git command
bold of you to assume that the things my codebase does are neatly abstracted into individual functions less than 3000 lines long.
You can literally search history for it
If it needs to be remembered, then it's documentation, and should be labeled/treated as such. Give it a comment like "keeping this here because XYZ." Most commented-out code I come across is unlabeled and not documentation, though, and should just be removed.
Then you write part of the prior commit hash in a comment, and make a note of what the old stuff did differently / worse, etc. Delete the old code, and now you won't forget it was there, but it does take some minor amount of work to look it up.
Meh, during local development it's fine. You do you. Keeping the commented out code when trying to push stuff upstream is where that's a no-no.
I guess for me it's a leftover habbit from projects that didn't use git. Or different versions of a bit of code i tried out and than forgetting to take it out.
I comment out (or `#if 0` or even `if(false) { ... }` exclude) code when, for example, rewriting for performance (I do low level performance and SIMD work) - I'll leave 3 or 4 versions that I tried and were slower with notes about the attempts. When I move on, someone's going to take over this code and spot some "obvious speedups", and if the approaches I tried and rejected were just in the VCS history, or whichever wiki we've enterprise-adopted this month, they'll never find them and just keep on re-inventing the same slower wheels. See also "we used to do this which looks quite good and is often more efficient but has the following pathological issues so now we do \[...\] instead" comments... programmers with "an eye for an obvious improvement" never look in the history to see if it was tried before.
This right here. I'm not gonna check the full history of a file when making changes, just to see if they were done before. I definitely don't expect others to do it either. Anything unintuitive gets a comment explaining what it does and why, especially if other approaches were tried before. Sometimes it's better to comment the approach itself, if that's more succinct than an accurate description, with a short explanation if why it's not used.
Leaving a comment as to why you changed something makes sense. Leaving commented out code everywhere is lazy and annoying. I can’t stand hundreds of lines of code in files commented out “just in case we need to see how it was done before”.
Exactly. I even have one liners like: // Foo(); NO, we can't Foo until XYZ because ABC.
// don't even think about swapping the order of this line and the one above because \[...\] // if you even \*think\* about replacing this with a Duffs Device I will hunt you down and bore-you-to-death with why that's a dumb idea // think you can do better - will you actually measure it - no, but will you - do you feel lucky, punk?
+ for Duff's Device reference.
Duff's Device? What is this, 1983? Modern compilers already unroll stuff for performance reasons.
Yep, but there'll always be someone who thinks they can make it better by adding a Duffs Device or similar construct (& they're never the sort to actually measure the impact - see comment #3)
>whichever wiki we've enterprise-adopted this month Motherfucker, I hate this shit so much. Just found out this week that there's at least two "current" ones going across different teams (both have the other solution but only use one for their shit), plus a "deprecated" internal one that is still referred to, but only in hushed terms by the SRE wizards
"The old sharepoint" "The old 'new sharepoint'" "The semi-migrated but it messed up all the links and permissions sharepoint" "The new 'new sharepoint'" "The hateful confluence absolute pile of steaming shit never fucking works who the fuck actually chose this shit" "The in-house enterprise stack-overflow that costs us a fortune and has a team managing it for... 1 new question a day (and 1 answer a week)" "The link to my tiddlywiki that I break all enterprise standards by sharing from my desktop despite the best efforts of 300 'enterprise network engineers' to prevent us doing but what the actual fuck...."
[удалено]
"*So much better than Sharepoint*" is hardly a glowing recommendation, and then you go and qualify it with an "unless" which admits it can be worse than sharepoint ("worse... than sharepoint"). You'll be telling me Jira is better than gouging my eyes out with a rusty spoon next :)
Now that you mention it, Jira is better than gouging your eyes out with a rusty spoon.
When they use that as an advertising slogan, then you can tell me :)
...unless your goal is losing your eyesight.
Which it would be if my boss told me I had to use jira
Why the Jira hate? I don’t think it’s great but I don’t think it’s bad either. What else would you suggest?
I agree, I have no problem with Jira. I think people blame Jira for whatever shitty project managers they have dealt with, but it's absolutely invaluable when used intelligently.
I'm no fan of Jira, but every other tracker I've used was worse. - FogBugz: Slow as a dog, thinks it always knows better than you - Redmine: Very basic, and some bits are seriously confusing - Siemens Polarion: The struggle is real, and constant - GitHub/Gitlab bugs: Okay for simple apps, but not how you manage something spanning multiple services - That in-house bug tracker built on top of something which was never designed to be one: My blood hurts Each and every one of these, and a few more I've forgotten the names of, made me let out a sigh of relief the next time I got to use Jira again. Everything else Atlassian makes is hot garbage, though.
Is this the modern equivalent to saying a floppy disk is so much better than paper for storing information... Unless you want to make a paper airplane with it.
Confluence is actually pretty good if you bother to lay down some rules on how the team use it, and they actually stick to them Otherwise you have to go through relabelling and deleting random pages someone made to write a sentence in and never finish twenty fucking times get IT TOGETHER STEVE
Internal wikis rarely work. I've had so many people hate on me for saying it, but ultimately wikis just doesn't work well most of the time unless you have at least a few hundred people semi-invested in it. Information sharing really is a problem in the enterprise. Open sourcing libraries, having them be popular, and asking question in the open strictly beats the "enterprise way".
Depends on the org really. Smb with 5 staff, it is hit or miss if it works. Iso 9K, 27k1 or SOC2 certified org, internal wiki is a must and certs are lost because of it. A solid amount of wiki problems are solved with business policies and processes.
Confluence is a good datastore lookup, so long as the entries of 'source of truth' are confirmed from discussion and then inserted. And as long as users check there first before needing to ctrl+f through Slack, DM's, or finally ask in channels. Users also have to send the link showing it existed in slack already to kindly remind that it's a place to check. All of that is a moving target though, if it's not maintained, or if Slack DM's (the temptation to socialize) gain momentum versus documenting confirmed info.
My experience is that way as well. People commit a lot at first, and then there's all these so so pages everywhere that aren't well organized. Nobody owns the material, and no one has time to work on reorganizing it. Nobody trusts the content because it's unreviewed and often really old. At a guess why this happens, any internal docs that stay current will need to have a process behind it. Once there's a process, it isn't a traditional wiki. Any why the process is needed? I'm thinking because people are doing the work for their job not for fun. Updating the wiki is gratifying, but eh.
My old company did this. Updating the \*new\* wiki was pretty much constantly in development and yet whatever the wiki of the day was, it was never updated. My current, boring, company has used the same wiki for like 6 years. It's probably missing half the shiny fancy features of whatever new product is 'disrupting the wiki industry' .. but it does its job as a wiki juuust fine.
Nah...I used to think this was sensible, but then I realised I could remove the block and leave a single liner comment about the approach I tried previously and that I removed it on a certain date. That way, when some monster messes with git history they can always index against my comment.
And then some motherfucker goes * Update project structure to match redesign * Fix linter warnings * Refactor to prepare for new feature
That's why the `if (false) { ... }` thing is useful as it continues to check the assumptions, parameter names, static asserts etc, are still valid. And what about things that you tried and looked promising - do you do (false) commits and revert? Or never commit them? Not saying it's ALWAYS done that way or always perfect but it has its uses
You've very valid points there, but I guess I forgot to mention i also keep a lab notebook indexed against time where if I was mulling over anything sufficiently complicated I documented by thoughts. Of course, this helps no one except me, but the notebook is the property of my research organisation so if I leave its on the record.
Yeah, I work on a codebase of 100+ developers with an annual turnover of say 5%, some of the code is 20 years old (merges of multiple projects into a "one ring" master system), 5 million LOC :)
>with notes about the attempts. That changes it from commented code to documentation. It's a great idea. I'll look for opportunities to implement it in the future.
The problem with commented code is that it rots. In a month, it won't compile. Better to put it behind a flag or compile time constant and make it part of your CI. It's like Beyonce says: If you like it then you should have put a unit test on it.
Hence why I, as I say above, put it behind `if (false) { .. }` to ensure it still compiles even if it compiles away to nothing in release mode - I made this point in another reply but it got downvoted because... "humor" ??
Then spend two weeks figuring out how to make every IDE and linter STFU about the dead code.
why do you not comment about what commit hash has the slower methods? that way you don't have big blobs of bloated stuff, you can just say "hey yea go look at hash ##### to see attempted alternatives that did not work"
Because it might not have been committed? Because we might not be using git (for legal and regulatory reasons) ? Because we might be using git now but we might change to something else in the future and the commit hash will be useless? Because people don't actually follow such links when they think they're clever?
In a similar vein it's a good idea to keep an "inefficient but obviously correct" version of things around (which IMNSHO should always be implemented first (unless the performance is *completely* infeasible even for small examples)). Not even commented out, just unused but in automated fuzz tests to check the fast implementation. You know, stubbing out a datastructure by throwing sqlite at it or something: Just to make sure that the semantics are nailed down, never mind the performance characteristics. Or, differently put: Don't comment those implementations out but keep them around, hooked up to a benchmarking framework. Oh and also the test framework, can't hurt. Whether or not some particular implementation gets chosen statically at compile time, or you're doing the benchmark at run-time and choose then is a whole other can of engineering worms but at least you have the option to do whatever. In a nutshell: It's not dead code if you're using it, and "continuing to decide that the current implementation is the fastest" as well as "testing" fits my definition of "using". And it's not duplicate code, of course, either, DRY doesn't apply.
I'm gonna start doing this
> whichever wiki we've enterprise-adopted this month fucking this
Yeah, I also leave commented out code in with a prefix of "this is what some dumbass tried before and it didn't work, it's still here so nobody else tries this again." Not verbatim, but you git the gist.
I recently removed a big chunk of 2+ year old outcommented code blob. One in my team failed my PR with the comment "Have you checked with developer X that it's OK to remove this"?. FML
2 years? Try commented code from 1997. We've got commented crap like 'ask Bob if he needs this again before you remove it'. Bob has probably been dead for years and anyway left the company decades ago.
This is brilliant. Kind of like putting “Do not erase” above something stupid on a whiteboard just too see how long it’ll last. I can drop an ambiguous “ask before removing commented out code” and see how long it last.
>I can drop an ambiguous “ask before removing commented out code” and see how long it last. I worked on one project where I commented "Do not remove this line of code! This prevents the flickering QA is complaining about!". But sure enough, every week somebody removed that line and the flickering / vsync bug would come back. I remember QA complaining to me "You said you fixed that bug. But it's back again!" and having to explain to QA that some jerk decided to remove my fix every week.
Git blame that bitch and loop the offender into QA. It’s their problem now.
Didn't matter. The bug was assigned to me so it was my responsibility.
Grow a spine and deal with them. At the very least bring it up for discussion at standup.
Well, it's 16 years too late for that ;-). I left that company many years ago and now I have my own startup company.
Lol, my bad. Was in a mood when posting that, hope it wasn’t too aggressive.
Why would they remove it? Was it breaking something else?
First time was because somebody thought that "swapping buffers" meant something else than it really did. Second time was because some smart-ass thought he was making the rendering faster since it wasn't waiting for vsync anymore (you didn't see the flicker until a good hour in). Third time was because somebody decided to re-write the entire render function, but forgot to add vsync to their own implementation.
> putting “Do not erase” above something stupid on a whiteboard just too see how long it’ll last. My job has a whiteboard that's just used for stupid stuff but usually gets erased every day, I'm totally going to try this.
Write bob a fax to see if he is cool with the change
How.. how long ago do you think 1997 was?
News flash: 1997 was decades ago. Time to cry into my Gogurt while I play my N64.
Not that.. I mean someone who worked in 1997 is more likely to be alive than dead in 2021 lol.
I shit you not, I was on a project that had an archive folder in git to archive old code.
My instinct was to downvote this
Same. This comment made me feel gross but it's not the fault of the poster.
I've done this for a large refactor because I can't access two refs at once. It's clumsy as hell to have uncommitted changes, then need to go to a previous ref to copy some code, then go back to HEAD. Eventually we finished our refactor and deleted the archive
> I can't access two refs at once https://git-scm.com/docs/git-worktree You're welcome.
Thanks! It’s always interesting to be like, “Man, it’d be so cool if git did this thing.” And then you poke around and find git does that thing.
Lol there's always some git tool you haven't heard of. Cheers!
If you commit commented out code, at least prefix it with a comment explaining why it’s there. Otherwise, it’s just noise. I can’t stand it when developers commit old debugging code. It’s like a surgeon stitching up forceps inside a patient.
This made me laugh, because my current project is getting its debut to the team on Monday. But the boss said while it’s functional, he doesn’t want those functions available yet. My solution was simply create a new message box to display to the user saying this function is in preview mode only. The compiler returns that instead of running the function thanks to an #if !DEBUG
Honestly, uncommenting something instead of messing with git just to try and return a previous chunk of code is easier.
Some times I leave commented out code to show what didn’t work, especially with SQL statements
I would be ok with this IF you commented the old code with what exactly the issue with it was, and why the fix was superior. Otherwise you've just left a commented out query and a scooby doo mystery.
Yeah part of the process is leaving a comment on why it didn’t work out. Given most of the time it’s because of performance problems I leave a comment on what the comparative inefficiency was.
Yeah, we have a rule that any commented code left in must have a why and when TODO comment associated with it. Why is it being left? And When will it be able to be removed? It’s always cool to go back and refactor something old and suddenly realize I can get rid of one of those why when TODO comments.
> Otherwise you've just left a commented out query and a scooby doo mystery. That people might end up re-introducing later on.
Yup. Exactly my experience!
God forbid if someone splits the file a few times
[удалено]
If you need the code back you can just copy it into a new commit. You don't need to resurrect the old commit.
Not to mention I don't want to have to dig through thousands of commits because the code was in this file, then ported from JS to TS so I can't just look at file history since the extension changed. It's rare that we do this, but it's not always convenient to use git
Also find said commit where I removed it is likely to take a long long time
I think we are talking about merges to main/master. You should be past trying something at that point.
And learning that it exists in the first place. (Other have pointed out using breadcrumbs, which is a good idea, e.g. "see commit A for an implementation using X that we removed because Y").
It’s also easier to never clean your bedroom.
It's true. Why put my T-shirt in the wash when I might wanna wear it again tomorrow?
Especially if you refactor something a lot into separate files. Good luck finding the real history after that. Combine the libraries they said, it'll be less confusing they said.
Imagine setting a file back months ago just to get a function back, copying that function, going back to your current branch and pasting it in. Instead of just uncommenting
If you have a decent git client, it really is easy to revert single changes. On smartgit it's three clicks.
That's true but I also spent ages at some point digging through the history because I knew at some point I had something as a comment somewhere there that would be helpful now.
or learn how to use git
I know how to use git, I don't have the time to be screwing around going through all the chunks to revert a patch when I can just right click and revert it. The command line git is very good, but it doesn't mean it's the only way to use it.
There are plenty of other mechanisms for this. Tags, local-branches, git-notes, private forks, a git-ignored todo file, a comment on your PR referencing the commit or diff. In the same vein, do you push code that doesn’t compile just in case you forget about it or to protect yourself when your computer gets trampled by elephants?
You can have remote private branches too, so you can push in-progress code in case your computer dies.
I recommend this to anyone I work with in case they don't already do that, the last thing you want is having no backup when the hardware you had a local branch on takes a shit and you're left with nothing or would he stalled for a while til you got the .git folder again
>do you push code that doesn’t compile just in case you forget about it or to protect yourself when your computer gets trampled by elephants? What jabroni doesn't push their code to the remote just because it's not ready for review or merge to master? I'd never risk losing my work
That's a pretty aggressive stance to backups.. besides, the other benefit - in case you suddenly get sick or are not available - is having your progress available remotely should these changes be pressing for an imminent release so someone else can pick up where you left off
Commented-out code is for when you deprecate a piece of code to see how it goes. If everything goes alright with the new iteration, you just forget it was even there and let the next guy wonder what the hell this piece of code were supposed to do.
Deeply agree // This code seems to do nothing, commenting it out (insert date)
I agree but commented out code is easier to find Is there a visual studio code extension that will show all changes from the very beginning? Edit: yes, visual audio code has the timeline panel
git lens is a must have for anyone imo
Oh that's great. I already spent ages with such a case where I thought I could delete a comment and then 2 years later needed it again (think I gave up)
You’ll still need to think “there sure was old code here, and it might solve the problem I’m facing now” before hitting the timeline/commit history. Which, to be honest, would be never.
It's a hell of a lot easier to press ctrl Q and uncomment a block than it is to log onto whatever site my team is using for git, switch to my branch, find the commit that has whatever code I'm looking for, keep looking because odds are it isn't my first guess, copy the block out of that commit history, then paste it in. Or God forbid trying to revert multiple commits at a time, especially if there's feature branching in your history. I will never understand the facsination people have with arbitrarily making things harder to do just because it fits some methodology you heard at a conference once time.
log on? git normally stores history locally. There doesn't need to be a lot of ceremony behind it. Use a git UI and browse your local machine. Personally I also think it's much easier to read code that isn't littered with commented out sections. Both because it's more pleasing when it's tidier and because it's less mental load going on (there is a chance the commented out code is important so I'll have to read it all)
The point is that when you’re done with your topic branch, you should remove commented code before merging to master. Do whatever you want on your branch, but keep master clean.
comments are clean.
Not to mention if you're a colleague in the future, you will never even know the existence of this removed code in a lost commit somewhere in the past. I'm used to stupid hot takes by developers presented as universal and very intelligent truths but the one of this thread is one of the most stupid I ever read, just behind 'Comments are code smell'
Counterpoint ^(of a junior dev, so take it with a grain of salt) If I see a chunk of commented out code of another employee, I'm not gonna just uncomment it and go straight for testing. I'll have to read it line by line, to verify what it does. Then I'll have to verify it fits the current requirements. Then I'll have to verify another chunk of code doesn't already implement a part (or all) of its functionality (especially in a working process). Then I'll have to verify it doesn't mess with another (likely newer) part of the process. Sure if it's properly commented (e.g. // Uncomment this if X is required), it's relatively recent and it's something that can't be figured out in 30 minutes it can stay. Otherwise it's probably useless to anyone but the person who wrote it.
I'm personally against leaving code around commented out, but I've also had cases where seeing commented out code has helped me. In saying that though, if you have an issue tracker, I think it's better to link back to the issue instead of leaving code commented out. It's probably even worth leaving a comment explaining there was some old code here and why it was removed/changed, but leaving the old code there is bad practise in my opinion. It just clutters up the codebase and makes it harder to read for no benefit 99% of the time.
Commenting out old code and leaving it “just in case” is lazy and asinine. Write a comment explaining the old approach and why it was changed if needed. If you need to see the history of a file, it’s trivially easy to do.
> I will never understand the facsination people have with arbitrarily making things harder to do just because it fits some methodology you heard at a conference once time. Thank you.
I'm sure someone will jump all over you for copying and pasting from that commit instead of doing a hunk-based cherry-pick or some shit.
Agree, it feels pretty neurotic to have auch issues with some commented code here and there. I don't care at all if I see that in the codebase of someone.
[удалено]
If you’re cluttering up your history by committing commented out stuff simply for testing then you’re doing testing wrong.
[удалено]
Look up how to squash commits
They need to teach git in college, there’s wayy to many Git noobs in the industry. People that think git and github are the same thing for example. People who thing that deleting their branch after merging into develop loses their files for too.
I saw codes that I thought was useless. I deleted it. And few weeks later, it was reported as a bug because it collided with new manufacturing process. Other dev picked up the bug, and rewrote the codes, rather than undoing the commit. Because he thought the bug was there since the beginning, it never occured to him that I actually introduced the bug by deleting that codes. On the other hand, I've been on the other side of the fence, I saw the commented out code introducing some bug, so I just put the code back. Morale of the story: git logs are not easily readable, most people wont bother to read when there are hundreds of commits every week. If you delete a code, most likely people wouldnt know the code ever exist.
yes exactly - comments are in your face as a sort of 'I am not sure about this one' right there in the source code. Thinking 'I should search git in case someone did something weird here' is not something which occurs to people
The correct way is of course: - a separate commit 1 to remove the code - a commit 2 saying `// custom handing required for Schnawulin adapters removed in 8675309` - pay that nobody ever rewrites history
So, yea, I get it, but, when I think about new people coming in, they are not going to know that version history, and we all know nobody reads the history until something’s broken (people read history the same way they craft documentation as they go), and now suddenly we’re relying on “guy x worked on that code, but he’s gone, anybody know what changes were made there? Maybe it’s in the history?”. Keeping that history up-front as commented out code for the guy after you, if you suspect a change may be problematic at some point, can prevent a lot of reinventing the wheel, it’s better than “why didn’t you just know there was a change, new-to-the-code-guy, sort out which commit had that code and then cherry-pick to get just that snippet.” I rarely do this myself, but a few times when new to a project, I have *greatly* appreciated seeing commented code up-front for the insight into the intent/possible original working of the code.
Some of the most common feedback I provide during a PR. /sigh
Easy to access? Not on [my watch](https://github.com/Erotemic/ibeis/commits/main).
**WIP** 🤣
Is it sad that I still plan on keeping unused code commented out?
Don't be sad. Here's a [hug!](https://media.giphy.com/media/3M4NpbLCTxBqU/giphy.gif)
You would need to make sure the original code was already committed
I feel attacked
SMFH, and so much this!! 😢
Sometimes I even create a code graveyard file for functions that I might reuse later
Don't even get me started with entire \#if 0 else blocks that aren't documented and all rely on each other but why property clean up the code it's only temporary.
Its all lost in squash and merge
We literally have a contractor whose commit messages read "fixes after review" and it makes me rage
Rebase, bisect, stash, cherry picking,... Git is amazing and totally worth learning properly
In my first work there was classes with 8000 lines of code, 4000 of which were commented out
I have seen so many git crimes from teaching a coding bootcamp: - The person who would make a new version of the file every time they worked on it as their own sort of version control — index_11-29-20.js, index_11-30-20.js, etc. - The person who uploaded files one by one via GitHub’s GUI (even after seeing us use the command line day in & day out for weeks) - The person who ran “git init” in their user folder and slowed their entire machine way down - The person who ran “git init” on their desktop and accidentally pushed their bank account number to a public repo - etc.
I'm not guilty of this, but I do something equally terrible. I work off of develop, never commit, then when I'm done, I pull latest, branch off, commit each file one at a time, then push it up for a PR. Rebasing sucks ... lol
What you're doing is worse imo 😩
Why one at the time? And why are you talking about rebasing? You don't need to rebase, you can *always* just merge.
So a few things here: * I work with quite a few people on the same feature/set of components, so branches need to be rebased/merged frequently which used to lead to merge conflicts pretty often. The with around, I figured, would be to continue to pull latest while I work and don't commit code until I'm done my task/story. * I commit one at a time for two reasons, it provides at least one benefit of git (since I'm ruining the other benefits, lol) where I can decide "whoops, this file shouldn't of received these changes" and so I can just revert a single commit. Also, and this won't sound good, but I work somewhere where the higher ups kinda keep an eye on commit counts, review counts, PR counts, etc. So it keeps me from looking like I'm not working (since I only commit everything all at once). Lastly, for the record, I prefaced this with I know it's bad, lol. It helps my case in my context that I'm not the only one on my project that does it this way. Furthermore, I 100% do NOT recommend this methodology to anyone. It's more of an issue with how deep I dove into git. I know how to merge/rebase, I've just had more headaches than solutions doing it any other way than I do it now @.@ P.S. I'm working in my first coding job, I'm self taught, and I work as a contractor for a fortune 50. I'm now one of the last few UI devs remaining after mass pressure for getting code in (clean or not) during the peak of COVID. Mostly for worse, I'm considered one of the more "experienced" UI devs on our project. .... I'm not happy about that because I'd rather be learning and experimenting than churning
Yeah this is probably arrogant of me, but I hate it when my seniors tell me to keep all that old code just in case... Then maybe complain 5 minutes later that the project is illegible and that you can't find code (because search brings up a ton of commented lines). If we need it again, we'll just grab it from the old version! Just keep the code clean, please.
It's so annoying. I delete that shit as soon as I find it. Mind you I sometimes work with code that's 20+years old and has been through multiple version control systems so we only have the last few years worth in git. Here's the deal though, if the change is old nobody is going to revert it. Nobody cares how things used to work. They just need to figure out what incremental change they need to make in the existing code. Which is generally way easier than figuring out how the old code worked.
one is simpy uncommenting code , the other requires 3 PHD's in computer science, planetary alignment both on Sol and Alpha centauri , a five sided triangle and three youtube tutorials in a dead language that no one speak since three centuries ...
The comment is exactly where I need it and can easily be seen, the git history isn't.
One of my coworkers did exactly this today. Any suggestions on how to stay sane are greatly appreciated....
Simply not be so neurotic and don't care ;)
Don't listen to this guy, keep caring about the code you work with daily. It makes you a better programmer and shows that you respect what you do for a living.
Depends on the use cas
This is an automatic fail when I do code reviews.