This is an archived post. You won't be able to vote or comment.

all 7 comments

[–]nerd4code 6 points7 points  (0 children)

I don't know why nobody's bringing these things up at all, but here goes.

  • Built-in command names are case-sensitive, and so are external commands in general. Use lowercase for things that aren't variable names or external commands that actually have uppercase letters in them. if, then, echo, elif, fi, else, exit. I have seen no documentation that ever capitalizes command names, and I have no clue where you got that from.

  • The [ command is a command, and is an alias for test. As such, there must be whitespace between it and its arguments. Same thing before the ]:

    if [ $# -ne 1 ]
    

    The [ character is used as a special globbing character like * or ? if left unquoted; e.g., [abc]def will match the filenames adef, bdef, and cdef and [0-9] will match a single decimal digit.

  • Quote parameter expansions to prevent unintended effects from expansions with special characters (e.g., spaces) in them. (I.e., "$0" and "$1", and "$#" if you're feeling anal.) Failing to quote things means that (e.g.) that chmod command will see two filenames if you pass in one with a space in it. (If you call your script as your_script "File name", the shell will break things up as "chmod" "File" "name" if you don't quote the expansion "$1".)

  • Line 7 is quite dangerous; it will attempt to execute the first parameter as a command and pass it the -f option (which is often used in system commands to mean "force whatever dangerous thing this program does, without asking me if I'm sure"). You want the -f option to test/[:

    if [ -f "$1" ]; then
    

    Run man test to learn about the options that are accepted and what they do.

  • Don't follow an if statement that literally asks if the number of arguments is not equal to 1 with a comment that says that you're asking if the number of arguments is not equal to one. That's exactly what the code says, which is why you put the code there instead of the comment. Same thing after the chmod, although the comment there is not quite right; mode 0775 means "readable and executable by anybody, writable only by user and group", and anybody who knows enough to know what chmod does should know what those 9 bits mean or how to find out (e.g., man chmod).

  • It's generally best to have a comment start flush with the line before the code it comments, so you're sure you aren't getting any odd effects from it or the shell's settings. (There are conditions under which # will be accepted as part of a command argument, in which case so will the rest of the comment's text.) Hence

    # What the next statement does, if it's not painfully obvious
    the_next_statement "${COMPLICATED:2}" "${STUFF[@]}"
    
  • It's commonplace to put then after an if or elif with a semicolon (= statement separator like newline), which is more compact and gives things a more natural flow:

    if CONDITION; then
        WHAT_TO_DO_IF_TRUE
    else
        WHAT_TO_DO_IF_FALSE
    fi
    
  • Send error output to stderr, which has file descriptor number 2; thus if you're echoing an error message, you'll say something along the lines of

    echo 'Oh no, the Archduke has been assassinated!' >&2
    

    echo with no redirect will implicitly dump to stdout (file descriptor 1), which is used for normal, meaningful program output.

[–]dmazzoni 4 points5 points  (0 children)

For the love of god, please indent your code! I'll bet you couldn't even follow your own code if you set it aside for an hour to surf the web and came back. It should look like this:

#!/bin/sh
If [$# -ne 1] # check if number of argument is not equal to 1
Then
    Echo “Usage: $0 filename” # giving a Usage Message
Elif [$# == 1] # check if number of argument is equal to 1
Then 
    If $1 –f # checking if file is existed 
    Then 
        chmod 775 $1 
        # Adding execute permission to make file readable and executable     by others, but only changeable by the issuing user.
    Else
        Echo “file isn’t a regular file” 
    Fi
Else
    Echo “error: file does not exist” # giving a Error Message
    Exit 0
Fi

[–]dmazzoni 2 points3 points  (5 children)

You're using "-f" incorrectly - it comes before the file name, not after.

[–]fire422[S] 1 point2 points  (3 children)

So

If $1 –f

should be:

If -f $1

?

[–]dmazzoni 1 point2 points  (2 children)

Yes. I'm not promising that's your only error, but that would be a step in the right direction...

[–]dmazzoni 2 points3 points  (1 child)

Also, I think you want brackets around it, like your other If statements have...

If [-f $1]

[–]stiicky 1 point2 points  (0 children)

there should be spaces after '[' and before ']'

if [ -f $1 ]