Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have just written my first git hook script. It is very simple that simply finds any URLs in the commit message and uses the Google URL shortener to rewrite the URL nicely.

it is located here.

I feel it could be improved immensely (as it is my first) and would love to have your input.

#! /bin/bash
message=`cat $1`
shorten=`sed -ne 's/.*\(http[^"]*\).*/\1/p' $1`
echo "Shortening Url $shorten ...."
new_url=`curl -s https://www.googleapis.com/urlshortener/v1/url \-H 'Content-Type: application/json' \-d "{'longUrl': '$shorten'}"`
latest=`echo $new_url | python -c 'import json,sys;obj=json.load(sys.stdin);print obj["id"]';`
final=${message/$shorten/$latest}
echo $final > $1
share|improve this question
7  
Why is that useful? A full URL usually has information in it. A shortened URL has no meaning (but is short). Its only useful in situations where you have limited space (like twitter). –  Loki Astari Jul 21 at 6:45

1 Answer 1

up vote 7 down vote accepted

First of all, the `cmd` form of running commands and capturing their output is deprecated. Use the recommended, modern way everywhere: $(cmd)

Safety

Be careful with spaces in filenames. These commands will break:

message=`cat $1`
shorten=`sed -ne 's/.*\(http[^"]*\).*/\1/p' $1`

It will be safer like this:

message=$(cat "$1")
shorten=$(sed -ne 's/.*\(http[^"]*\).*/\1/p' "$1")

The pattern there in the sed is not very safe. The text "add support for http protocol" will match. I think you want to be more strict, maybe something like:

longUrl=$(sed -ne 's/.*\(https\{0,1\}:\/\/[^"]*\).*/\1/p' "$1")

Note: if you have GNU sed (you're in Linux, or have gsed), then instead of the tedious https\{0,1\} you can simplify as https\?, though it's less portable.

And what if there are multiple URLs in the script? It will fail. You probably want to loop over the results of the sed. Or take a lazier approach and just ensure that sed will always produce at most one line:

longUrl=$(sed -ne 's/.*\(https\{0,1\}:\/\/[^"]*\).*/\1/p' "$1" | head -n 1)

What if there are no matches? An if would be good, as in that case you won't need the curl call, and it's better to not overwrite a file if you don't really have to.

When you echo $somevar, whitespaces inside like tabs and newlines would be replaced by spaces. To prevent that, quote the variable:

echo "$message" > "$1"

Naming

Your variable names are not so good:

  • shorten is the output of sed, a long url. Something like longUrl would have been better.
  • newUrl is the output of curl, a json text. Something like json would have been better.

Unnecessary things

In the curl command, you don't need the backslashes in \-H and \-d flags.

And instead of saving the output of the curl and then echo-ing to python, it would be better to skip that intermediary variable and just pipe it directly, like this:

shortUrl=$(curl -s https://www.googleapis.com/urlshortener/v1/url -H 'Content-Type: application/json' -d "{'longUrl': '$longUrl'}" | python -c 'import json, sys; print(json.load(sys.stdin)["id"])')

Also notice in that python that I skipped the intermediary obj variable and just used the ["id"] directly on json.load(...).

Suggested implementation

#! /bin/bash
message=$(cat "$1")
longUrl=$(sed -ne 's/.*\(https\{0,1\}:\/\/[^"]*\).*/\1/p' "$1" | head -n 1)
if test "$longUrl"; then
    echo "Shortening Url $longUrl ..."
    shortUrl=$(curl -s https://www.googleapis.com/urlshortener/v1/url -H 'Content-Type: application/json' -d "{'longUrl': '$longUrl'}" | python -c 'import json, sys; print(json.load(sys.stdin)["id"])')
    message=${message/$longUrl/$shortUrl}
    echo "$message" > "$1"
fi

This is still not perfect, because it doesn't handle multiple urls. I might add that later, gotta go now.

Online shell checker

This site is pretty awesome: http://www.shellcheck.net/#

Copy-paste your script in there, and it can spot many mistakes that are easy fix.

share|improve this answer
1  
https\{0,1\} Why would you write it like that, instead of the simpler https?? –  svick Jul 21 at 14:35
    
I didn't have much time when I wrote this, and couldn't get the simple https? working. I figured out now and updated the post. Thanks for spotting! (I was gonna improve but completely forgot...) –  janos Jul 21 at 15:35

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.