• This site uses cookies. By continuing to use this site, you are agreeing to our use of cookies. Learn more.

Shell Help with code

mefizto

Well-Known Member

Thanks: 6
Messages: 372

#1
Greetings all,

I have a problem with the following code snippet:
Code:
tar_directory ()
{
if cd "$1"
then
  find . -mindepth $2 -maxdepth $2 -type $3 > directories
  if [ -s directories ]; then
    while read -r line
      do printf %s\\n "$1ine"
      tar cvf "$line".tar "$line"
    done < directories
    rm directories
  else
      .
      .
}
I have a test directory ~/test_dir than contains two sub-directories /test_dir_1 and /test_dir_2, each containing two files.

Once the function is invoked tar_directory ~/test_dir 1 d the directories file contains:
Code:
./test_dir_1
./test_dir_2
as expected.

To my consternation, the print statement outputs
Code:
/home/user/test_dirine
as the value of the variable $line, during both runs through the loop, instead of the expected
Code:
./test_dir_1
and
Code:
./test_dir_2
respectively; however, the tar cvf "$line".tar "$line" correctly creates /test_dir_1.tar and /test_dir_2.tar.

The extension ine is from the last three letters of the variable $line, as I have confirmed by experimenting with the variable name. I have run the code via devel/hs-ShellCheck, without any indication of a problem.

Any insight in the discrepancy between the value of the variable $line as printed and used by the tar would be appreciated.

Kindest regards,

M
 

SirDice

Administrator
Staff member
Administrator
Moderator

Thanks: 5,986
Best answers: 10
Messages: 26,733

#2
Instead of doing this:
Code:
if cd "$1"
then
  find . -mindepth $2 -maxdepth $2 -type $3 > directories
  if [ -s directories ]; then
    while read -r line
Why not this?
Code:
find $1 -mindepth $2 -maxdepth $2 -type $3 | while read -r line
do
 ....
 ...
done
This contains a typo:
Code:
      do printf %s\\n "$1ine"
      tar cvf "$line".tar "$line"
Note the variable $1ine should read $line. I suspect this might be due to your font, with some fonts it's really difficult to see the difference between 1 (one), l (lower case L) and I (uppercase I). So your print statement prints $1 (your first argument) followed by the text ine, hence it prints /home/user/test_dirine.
 

mefizto

Well-Known Member

Thanks: 6
Messages: 372

#3
Hi SirDice,

thank you very much. Yes it is a typo, and I spent literally hours re-writing the code. However, since I have copied some of the lines, the error propagated.

As to your alternate suggestion, (1) I am a beginner, this is my third or fourth script, so this is the best I can do, (2) some tutorials that I have read argue that it is preferable to avoid pipes, and (3) I want to check that the file directories is not empty to prevent useless processing.

Kindest regards,

M
 

SirDice

Administrator
Staff member
Administrator
Moderator

Thanks: 5,986
Best answers: 10
Messages: 26,733

#4
I am a beginner, this is my third or fourth script, so this is the best I can do
No worries, you should see some of my first scripts :D

some tutorials that I have read argue that it is preferable to avoid pipes
You just need to be aware they're spawning a sub-shell. Which means variables set inside the loop won't be known or set outside of it. But in your case this isn't a problem.

I want to check that the file directories is not empty to prevent useless processing.
If the find command doesn't find anything the loop is never executed. So there's really no need to check for it.

So, just some friendly advice, this is not good:
Code:
if cd "$var"; then
 ....
fi
This is a lot better:
Code:
if [ -d "$var" ]; then
 cd "$var"
 ...
 ...
fi
Your version will actually fail and exit the script if the directory doesn't exist. Which I think is what you're trying to test for. But, try not to cd inside your scripts unless you can't do it any other way. It can lead to very strange results and files being created in the wrong place. I personally find it quite annoying when I execute a script and my current directory is changed when the script ends. So "save" the current directory and change it back at the end of the script.

Code:
MYCWD=$(pwd)
...
...
 cd /some/where
...
 cd /some/other/place
...
cd ${MYCWD}
 

mefizto

Well-Known Member

Thanks: 6
Messages: 372

#5
Hi SirDice,

I really appreciate your time to answer and point the script's deficiencies.
You just need to be aware they're spawning a sub-shell. Which means variables set inside the loop won't be known or set outside of it. But in your case this isn't a problem.
Indeed, the other reason advanced was that spawning a sub-shell was inefficient.

If the find command doesn't find anything the loop is never executed.
True, the read finds an empty line and skips the loop.

So, just some friendly advice, this is not good:
I am not sure that I follow your reasoning. As I understand, you argue to always save and restore the current directory. And I agree because sometimes I am quite surprised where the output of the script is :rolleyes:. However, your code appears to argue against direct command testing. In that regard, some tutorials argue that direct command testing, e.g.:
Code:
if command; then
or, if a result of the command is important:
Code:
output=$(command)
if [$output = $expected_value]; then
is preferable to indirect testing, e.g.:
Code:
command
if [ echo $? = 0 ]; then
Kindest regards,

M
 

SirDice

Administrator
Staff member
Administrator
Moderator

Thanks: 5,986
Best answers: 10
Messages: 26,733

#6
There's a difference.

Code:
if [ -d "$var" ]; then
  cd "$var"
  ...
fi
Code:
if cd "$var"; then
 ...
fi
The first code uses a test(1) to see if it's actually a directory and only if it is does it change directory. This will prevent errors if the directory doesn't exist for example. The second code just assumes there's a directory and only if the change directory is successful will it execute the code inside the if... block. But this produces an error message if the directory doesn't exist or is not accessible.

Code:
dice@maelcum:~/test % cat test1.sh
#!/bin/sh

var="1"
if [ -d "$var" -a -x "$var" ]; then
  cd "$var"
  # some code
fi

var=2
if cd "$var"; then
  echo "Never gets here"
fi

echo "done"
dice@maelcum:~/test % ls -l
total 2
drwxr-xr-x  2 dice  dice    2 May  7 11:37 1
drwxr-xr-x  2 dice  dice    2 May  7 11:37 2
-rw-r--r--  1 dice  dice  130 Jun 13 17:33 test1.sh
Code:
dice@maelcum:~/test % sh -x test1.sh
+ var=1
+ [ -d 1 ]
+ cd 1
+ var=2
+ cd 2
cd: 2: No such file or directory
+ echo done
done
This little example also shows the danger of using change directory inside a script.

If you want to make sure the change directory actually works you can also do things like this:
Code:
cd "$var" && command
The command is only executed if cd "$var" was successful. But this will also produce an error message when the directory doesn't exist.

If you intended to see the error message that's fine. But most of the time you don't want cryptic errors or warnings cluttering up your output. So you use test(1) to check and verify. If the checks fail you can bail out gracefully, i.e. print a meaningful message why it failed.
 

mefizto

Well-Known Member

Thanks: 6
Messages: 372

#7
Hi SirDice,

I do clearly see the difference of what is tested; your proposed code tests existence of the directory, mine success/failure of the cd. But, I have to think about it more to understand whether there is a practical difference, because in both cases care must be taken in the event that the directory does not exist, which both tests will demonstrate.

Kindest regards,

M
 

SirDice

Administrator
Staff member
Administrator
Moderator

Thanks: 5,986
Best answers: 10
Messages: 26,733

#8
Yes, the practical difference is that your version prints an error message, mine doesn't, while functionally doing the same thing. But, like I said, if that's intentional that's fine. I personally like to code for those exceptions and print my own error messages.

Code:
if [ -d "$var" ]; then
 cd "$var"
 # do stuff
else
 echo "The ${var} directory doesn't exist, numbnuts!"
 exit 1
fi
:D
 

mefizto

Well-Known Member

Thanks: 6
Messages: 372

#9
Hi SirDice,

if I were as smart as you, I would say "Great minds think alike", but instead, here is the rest of my code:
Code:
tar_directory ()
{
if cd "$1"
then
  find . -mindepth $2 -maxdepth $2 -type $3 > directories
. . .
. . .
else
  printf %s\\n "Directory $1 does not exist you moron."
fi
}
But, I see your point, the shell prints an error, and afterwards my code prints the error message. Thank you for the education.

I wonder what is the most efficient manner of handling errors and exceptions, but that is a different topic.

Kindest regards,

M
 

SirDice

Administrator
Staff member
Administrator
Moderator

Thanks: 5,986
Best answers: 10
Messages: 26,733

#10
But, I see your point, the shell prints an error, and afterwards my code prints the error message.
Yes, exactly!

You could redirect the error message though, something like this:
Code:
if cd "$var" 2>/dev/null
then
  ...
fi
But this can get ugly really fast. Not only visually ugly but you can also fall into various redirection holes (when the redirection actually happens at a different point).

I wonder what is the most efficient manner of handling errors and exceptions, but that is a different topic.
Definitely. I often end up with silly constructs like:
Code:
if partA; then
  if partB; then
    if partC; then
      # code at last!
    fi
  fi
fi
When I read other people's code and see some of the solutions I often see programming as an art form. I believe code should be beautiful to look at and elegantly functional.
 
Top