Checking if there are anyways I could improve this bash script.

zerophase · 2016-06-15 19:26:46

I just wrote a bash script that uses reflector to update the rsync servers in use by powerpill. I also have a systemd service and timer for running it weekly. I'm just looking for advice on how to improve it.

#!/bin/bash

declare -a servers
servers=($(reflector -p rsync -f 30 -l 20 -p 20 --sort rate | grep -o 'rsync:\/\/.*'))

declare -i i=0
regex='^[[:blank:]]+\"rsync://.*'
( IFS='\n' 
while read -r line 
do
	if [[ ! $line =~ $regex ]]; then 
		echo "$line" 
	elif ((i < ${#servers[@]})) ; then
		lead=$( expr index "$line" "\""  )
	 	server=
		if ((i < ${#servers[@]} - 1)) ; then
			server="${servers[$i]}\","
		else
			server="${servers[$i]}\""
		fi
		echo "${line:0:$lead}${server}"
		((i++))
	fi
done < updateTest > o  
mv o updateTest )

eschwartz · 2016-06-15 20:54:42

Instead of that while loop, why don't you just use something simple like:

printf "\"%s\",\n" "${servers[@]}"|head -c -2

Slithery · 2016-06-15 22:51:15

Running it through ShellCheck throws up a couple of pointers.

zerophase · 2016-06-16 01:05:59

Eschwartz wrote:

Instead of that while loop, why don't you just use something simple like:
|head -c -2

This use of head just grabs the file from the end? Starting the write at the last lines?

eschwartz · 2016-06-16 01:38:17

printf "\"%s\",\n" "${servers[@]}"|head -c -2

printf -- print out each element in the "servers" array, in the format:

"element1",
"element2",
"element3",

Obviously, the trailing comma + linebreak is unwanted and specifically the comma will break json parsing. Rather than looping and checking each array index for the last element in order to switch between echo commands... just take all-but-the-last-two-bytes by piping it to the `head` command.

I don't know of any way to tell printf to use a different format string for the last argument, so that will have to do.

zerophase · 2016-06-16 01:50:28

Eschwartz wrote:

printf "\"%s\",\n" "${servers[@]}"|head -c -2
printf -- print out each element in the "servers" array, in the format:
"element1",
"element2",
"element3",
 
Obviously, the trailing comma + linebreak is unwanted and specifically the comma will break json parsing. Rather than looping and checking each array index for the last element in order to switch between echo commands... just take all-but-the-last-two-bytes by piping it to the `head` command.
I don't know of any way to tell printf to use a different format string for the last argument, so that will have to do.

That makes sense with how the server list ends up printing out.

The only thing I'm at a loss for how to do is overwriting part of a file starting at a certain line without a while loop.

Last edited by zerophase (2016-06-16 01:54:15)

eschwartz · 2016-06-16 02:49:51

printf is not a file, it goes to stdout. As I said, it is piped to `head`.
What file do you think is being overwritten?

...

Ah, I think I see it now.
I was having a hard time seeing what all that for loop did. But I guess the "updateTest" file is the powerpill config?
You can probably use something like sed to replace it -- might improve the readability.

zerophase · 2016-06-16 02:59:14

Oh I mean I was redirecting the output to the file I'm writing to with >>

I'm Just wondering if there is anyway to redirect to certain lines, other than sed.

zerophase · 2016-06-16 07:46:40

I ended up using perl over sed for it. The sed command needed was just more complicated to write out.

#!/bin/bash
# TODO rename to reflector-rsync.sh
declare -a servers
cd /home/zerophase # remove for final iteration
servers=($(reflector -p rsync -f 30 -l 20 -p 20 --sort rate | grep -o 'rsync:\/\/.*'))

linesToWrite=$(printf "\t\t\t\"%s\",\\\n" "${servers[@]}" | head -c -4 ) 
perl -i -0pE 'use strict; my $repo = "\$repo"; my $arch = "\$arch";'\
's|([[:blank:]]+\"rsync:\/\/[^\]]+)|'"${linesToWrite}"'\n\t\t|gs' testSed # change to powerpill.json

much nicer to read now.

By the way, is there a suggested place to put bash scripts in use by systemd? Just /bin so they're accessible from everywhere, or just /etc/systemd/system?

ukhippo · 2016-06-16 10:14:52

zerophase wrote:

By the way, is there a suggested place to put bash scripts in use by systemd? Just /bin so they're accessible from everywhere, or just /etc/systemd/system?

I created a /etc/systemd/scripts directory for mine.

ewaller · 2016-06-16 14:14:24

My only comment is that you create ./o without looking to see if it exists. Then, at the end, you move it to ./updateTest. You might consider checking to see if that interim file does not exist before you obliterate it.
Also, are the parentheses starting in front of IFS='\n' and ending after mv o updateTest not superfluous? [IDK, I am not a Bash Jockey]

eschwartz · 2016-06-16 14:26:20

The parentheses run that in a subshell, which is mostly pointless except it means the IFS change only happens in the subshell.
You could get around that by saving and restoring the IFS, assuming there was an actual concern that you'd need a default IFS within the same script anyway... still, as a style thing it isn't the worst idea ever to get used to always putting things back just in case.

Since the whole thing has been replaced by a perl substitution it doesn't matter anymore.

Last edited by eschwartz (2016-06-16 14:27:20)

ewaller · 2016-06-16 14:36:09

Eschwartz; Thanks, make sense.

zerophase · 2016-06-16 20:36:34

I just scoped the IFS since I didn't know if it could potentially escape my script and cause issues elsewhere. Just assumed avoid any potential headaches.

eschwartz · 2016-06-16 21:20:51

Scripts are run as a child bash shell (same as a subshell except subshells also get to see non-exported variables and a few other things). Variables cannot escape from a child process to a parent process.

In this case it was unnecessary. But like I said, it isn't a terrible idea to do that on general principle. Though I would use a SAVEIFS="$IFS" to backup and restore rather than adding an unnecessary subshell. It's also more readable.

Arch Linux

#1 2016-06-15 19:26:46

Checking if there are anyways I could improve this bash script.

#2 2016-06-15 20:54:42

Re: Checking if there are anyways I could improve this bash script.

#3 2016-06-15 22:51:15

Re: Checking if there are anyways I could improve this bash script.

#4 2016-06-16 01:05:59

Re: Checking if there are anyways I could improve this bash script.

#5 2016-06-16 01:38:17

Re: Checking if there are anyways I could improve this bash script.

#6 2016-06-16 01:50:28

Re: Checking if there are anyways I could improve this bash script.

#7 2016-06-16 02:49:51

Re: Checking if there are anyways I could improve this bash script.

#8 2016-06-16 02:59:14

Re: Checking if there are anyways I could improve this bash script.

#9 2016-06-16 07:46:40

Re: Checking if there are anyways I could improve this bash script.

#10 2016-06-16 10:14:52

Re: Checking if there are anyways I could improve this bash script.

#11 2016-06-16 14:14:24

Re: Checking if there are anyways I could improve this bash script.

#12 2016-06-16 14:26:20

Re: Checking if there are anyways I could improve this bash script.

#13 2016-06-16 14:36:09

Re: Checking if there are anyways I could improve this bash script.

#14 2016-06-16 20:36:34

Re: Checking if there are anyways I could improve this bash script.

#15 2016-06-16 21:20:51

Re: Checking if there are anyways I could improve this bash script.

Board footer