[colanderman]

    # 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.

[irishcoffee]

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

[colanderman]

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]

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]

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)".

[noarchy]

Do we know if they do code review? Are pull requests blindly accepted?

[irishcoffee]

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]

>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]

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.

[Guthur]

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.

[jjnoakes]

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.

[colanderman]

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!)

[pragmar]

I'm always annoyed when I see UUIDs in my directory structure, it's the same feeling I get when I see a big mac wrapper littered in my yard, or MSOCache in Windows system root for that matter.

[jjnoakes]

If you can't write a shell script without being sure your variables are set, then don't write it in shell.

A MANIFEST is not the right answer. Steam deals with any number of third-party games which may drop any number of files into these directories.

The best way to nuke a directory is 'rm -rf "$DIRECTORY"'. Any amount of "well what if $DIRECTORY points to the wrong place" has nothing to do with the removal operation itself.

[mikeash]

How about using a better language? She'll scripting is an awful, awful language. An error like this wouldn't have happened if the program had been written in C or Python or Perl or whatever your choice might be.

Shell scripting seems tremendously overused. It makes some things a bit easier, but it's so crazy it makes PHP a look like a pinnacle of good language design.

[kd0amg]

Using C or Python or Perl does not automatically keep people from failing to check whether some code that reaches out into the environment to prepare for later action actually succeeded.

[azernik]

It tends to fail much more noisily if you try to use an uninitialized variable - C will probably segfault (or end up removing a garbage string of probably-unprintable characters), Python and Perl will throw exceptions.

[mikeash]

I don't see any way to accidentally write code in C or Python (Perl may be a different beast as a sibling comment indicates) that deletes the user's home directory if an environment variable is unset. These languages don't keep you from failing to check, but they fail much better. An unset environment variable without a check means you'll probably crash, whereas with a shell script you just keep on going, with bad data.

[colanderman]

I don't disagree with you. (Although Perl exhibits exactly the same issue.) Unfortunately bash is the lowest common denominator on Linux and is often chosen on that basis.

[koenigdavidmj]

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]

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]

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.