44 ms·

# Scary! rm -rf "$STEAMROOT/"* Anybody who writes a line like this deserves their software engineer license revoked. This isn't the first time I've seen s

# Scary! rm -rf "$STEAMROOT/"* Anybody who writes a line like this deserves their software engineer license revoked. This isn't the first time I've seen shit like this (I've seen it in scripts expected to be run as root, no less); it makes my blood boil. Seriously. "xargs rm -df -- < MANIFEST" is not that hard. EDIT: I shouldn't be so harsh, if it weren't for the comment admitting knowing how poor an idea this line is.

Guthur 12y ago
Is there proof that the comment was committed with the line of code? Of course once it was a identified a concerted effort could have been made to rectify the situation. Sometimes a problem is identified working on some unrelated aspect of the software and the developer does not have time or scope to change the offending piece of code but wants to place a red flag. Of course this example does not appear to be systematic approach to marking "fixme", unless of course their fixme tag is actually #scary. Edit: Of course one would hope they have raised a rather high priority work item for this.
- deleted 12y ago
  [deleted]
irishcoffee 12y ago
The programmer committed a cardinal sin to be sure. But, so did everyone on the code review that let it slide.
- noarchy 12y ago
  Do we know if they do code review? Are pull requests blindly accepted?
  - irishcoffee 12y ago
    If valve doesn't do code reviews, and if pull requests are actually blindly accepted (is that actually a thing? Do people ever actually blindly accept pull requests?) I don't think I have the wherewithal to give a useful response.
    - noarchy 12y ago
      >is that actually a thing? Do people ever actually blindly accept pull requests? Speaking only for myself, I've seen it with internal git repos at companies that have some name recognition. Obviously they'd never want it known that this happens, but it does. So why does it happen? In the cases I saw, it was usually when people responsible for code reviews had "other things on their plate", and rather than hand off the review to someone else, they just merged it. During heavy pushes to get a build out the door, I saw a lot of this happening. Since I never said anything, I suppose I was part of the problem.
- clebio 12y ago
  Absolutely. Arguably, that's the main point of the code review -- you're sharing responsibility. If your team scape-goats, it's not going to be a team for very long.
- colanderman 12y ago
  You're right, but I don't retract my statement, because we know the dev knew how dangerous this line was thanks to his "Scary!" comment.
  - click170 12y ago
    I don't whole heartedly disagree with you but I think its grayer than that. Maybe there were time constraints. Maybe the coder explained that this code was dangerous to his boss but was not grantee the time to fix it. I think we agree that said code should never have been written, but there are any number of circumstances that place the blame squarely on management. If he explained the dangers of doing it that way but wasn't granted time to fix it (or no manifest was kept), theres little that johnny coder can do outside of their own time. None of us write perfect code the first time, and we all had to start somewhere. What's important is how far youve come and what you've learned. I think.
    - colanderman 12y ago
      You are right. Consider my original assertion (which is past its edit window) amended to read "engineer (or manager who prevented an engineer from fixing)".
deleted 12y ago
[deleted]
- colanderman 12y ago
  No, I'm a developer who pays attention to his code, knows what he doesn't know, double-checks what he thinks he knows, and isn't too lazy to ask for help when he isn't sure about something. Not to mention I've worked with someone before who wrote code exactly like this, despite my protestation, and after he managed to delete half the (thankfully backed-up) file share in an unrelated incident.
jjnoakes 12y ago
There's nothing wrong with that line as long as you know "$STEAMROOT" contains a directory that you wish to nuke. The issue is that this particular script did not set up $STEAMROOT correctly.
- koenigdavidmj 12y ago
  If you want to blow away a directory, do this just to avoid wildcard games: rm -rf $the_dir && mkdir $the_dir Yes, it's technically slower, but this is shell.
  - perturbation 12y ago
    Should mkdir be first (so that it short circuits if $the_dir is unset?) I haven't done anything in bash in a while but that seems to be how it works on my computer (Debian Wheezy, bash 4.2.37).
    - koenigdavidmj 12y ago
      The original broken code was using a wildcard to clear a directory. I proposed a safer way of doing that: delete the directory itself then recreate it.
- colanderman 12y ago
  The thing is, there are so many ways for STEAMROOT not to be set correctly in Bash. Just one typo in some future edit can bork everything if it's not tested thoroughly. Sure, you could somehow check that STEAMROOT is set to something resembling what you want to delete. But manifests are much more simple to get right. EDIT: Alternatively, pick a well-known UUID, and put everything under a directory with that name under STEAMROOT. Then, to remove: rm --one-file-system -rf "$STEAMROOT/d0a8936d-1faf-4b82-aac9-e5f104432b24" This won't do the wrong thing, as it will require that unique string to be present in the pathname. (--one-file-system for good measure.) (Don't put the UUID in a variable, or you're back to square one of having a potential bug!)
  - 12y ago

Guthur 12y ago

Is there proof that the comment was committed with the line of code? Of course once it was a identified a concerted effort could have been made to rectify the situation. Sometimes a problem is identified working on some unrelated aspect of the software and the developer does not have time or scope to change the offending piece of code but wants to place a red flag. Of course this example does not appear to be systematic approach to marking "fixme", unless of course their fixme tag is actually #scary. Edit: Of course one would hope they have raised a rather high priority work item for this.

deleted 12y ago
[deleted]

irishcoffee 12y ago

The programmer committed a cardinal sin to be sure. But, so did everyone on the code review that let it slide.

noarchy 12y ago
Do we know if they do code review? Are pull requests blindly accepted?
- irishcoffee 12y ago
  If valve doesn't do code reviews, and if pull requests are actually blindly accepted (is that actually a thing? Do people ever actually blindly accept pull requests?) I don't think I have the wherewithal to give a useful response.
  - noarchy 12y ago
    >is that actually a thing? Do people ever actually blindly accept pull requests? Speaking only for myself, I've seen it with internal git repos at companies that have some name recognition. Obviously they'd never want it known that this happens, but it does. So why does it happen? In the cases I saw, it was usually when people responsible for code reviews had "other things on their plate", and rather than hand off the review to someone else, they just merged it. During heavy pushes to get a build out the door, I saw a lot of this happening. Since I never said anything, I suppose I was part of the problem.
clebio 12y ago
Absolutely. Arguably, that's the main point of the code review -- you're sharing responsibility. If your team scape-goats, it's not going to be a team for very long.
colanderman 12y ago
You're right, but I don't retract my statement, because we know the dev knew how dangerous this line was thanks to his "Scary!" comment.
- click170 12y ago
  I don't whole heartedly disagree with you but I think its grayer than that. Maybe there were time constraints. Maybe the coder explained that this code was dangerous to his boss but was not grantee the time to fix it. I think we agree that said code should never have been written, but there are any number of circumstances that place the blame squarely on management. If he explained the dangers of doing it that way but wasn't granted time to fix it (or no manifest was kept), theres little that johnny coder can do outside of their own time. None of us write perfect code the first time, and we all had to start somewhere. What's important is how far youve come and what you've learned. I think.
  - colanderman 12y ago
    You are right. Consider my original assertion (which is past its edit window) amended to read "engineer (or manager who prevented an engineer from fixing)".

deleted 12y ago

[deleted]

colanderman 12y ago
No, I'm a developer who pays attention to his code, knows what he doesn't know, double-checks what he thinks he knows, and isn't too lazy to ask for help when he isn't sure about something. Not to mention I've worked with someone before who wrote code exactly like this, despite my protestation, and after he managed to delete half the (thankfully backed-up) file share in an unrelated incident.

jjnoakes 12y ago

There's nothing wrong with that line as long as you know "$STEAMROOT" contains a directory that you wish to nuke. The issue is that this particular script did not set up $STEAMROOT correctly.

koenigdavidmj 12y ago
If you want to blow away a directory, do this just to avoid wildcard games: rm -rf $the_dir && mkdir $the_dir Yes, it's technically slower, but this is shell.
- perturbation 12y ago
  Should mkdir be first (so that it short circuits if $the_dir is unset?) I haven't done anything in bash in a while but that seems to be how it works on my computer (Debian Wheezy, bash 4.2.37).
  - koenigdavidmj 12y ago
    The original broken code was using a wildcard to clear a directory. I proposed a safer way of doing that: delete the directory itself then recreate it.
colanderman 12y ago
The thing is, there are so many ways for STEAMROOT not to be set correctly in Bash. Just one typo in some future edit can bork everything if it's not tested thoroughly. Sure, you could somehow check that STEAMROOT is set to something resembling what you want to delete. But manifests are much more simple to get right. EDIT: Alternatively, pick a well-known UUID, and put everything under a directory with that name under STEAMROOT. Then, to remove: rm --one-file-system -rf "$STEAMROOT/d0a8936d-1faf-4b82-aac9-e5f104432b24" This won't do the wrong thing, as it will require that unique string to be present in the pathname. (--one-file-system for good measure.) (Don't put the UUID in a variable, or you're back to square one of having a potential bug!)
- 12y ago