Help can't get script to work how I need it to...

Hi thank you for anyone who responds.

Here is my script:

for i in `ls -1 | grep $1 | grep  $2`
do
x=`echo $i | sed 's/\.Sent/\.Done/g'`
echo mv $i DONE/$x
echo "Is this OK?"
read user_response

case $user_response in

"y"|"Y")
        mv $i DONE/$x
        echo mv $i DONE/$x;;
*)
        echo "No changes made ...";;

esac

done

The list it grabs is more than one file.
When i run this it asks me if "Is this OK?" for each file. I want it to just spit out the list then ask me, if I say yes, then i want it to move all the files it lists.

Thank you!!

Please enclose code in

 tags. (Edit your original post.)


for i in `ls -1 | grep $1 | grep  $2`

[/quote]

[INDENT]
You don't need -1 when the output is not going to a terminal.

You don't need two instances of grep; use grep -e "$1" -e "$2".

You probably don't need ls, either, and it will break you script if any filenames contain spaces.

If you are trying to get files with a certain pattern, use wildcards, e.g.:

for i in *$1*$2*

You don't need sed:

x=${i%%.Sent*}.Done${i#*.Sent}

If you don't want to be asked for every file, don't put the question inside the loop. Build a list and present that outside the loop.

Thank you for your reply.

I thought my case statement was outside the loop?

The loop is everything between do and done. That's where you are asking.

Ok so I changed it but after it asks me it only moved one file?
Should I not be using a case statement to perform the move?
I am using it to ask the user if the list is correct.

I thought that inside the case statement I should put what shoudl be carried out if the user says no.
Sounds like maybe I shouldn't be using case or put that somewhere else in the script?

I've very new so please bear with me. Thank you!

What did you change it to?

A case statement cannot move anything. You use mv to do the move.

How are you using it? We're not mind readers.

...and what should be carried out when the use says yes.

Yes, you should use case.

Isn't that what I suggested?

Oh I'm sorry. I'm obviously clueless I know that!

here is my code

for i in `ls -1 | grep $1 | grep  $2`
do
x=`echo $i | sed 's/\.Sent/\.Done/g'`
echo mv $i DONE/$x
done
echo "Is this OK?"
read user_response

case $user_response in

"y"|"Y")
        mv $i DONE/$x
        echo moved $i to DONE/$x;;
*)
        echo "No changes made ...";;

esac

for i in `ls -1 | grep $1 | grep  $2`
do
x=`echo $i | sed 's/\.Sent/\.Done/g'`
echo mv $i DONE/$x
done

Here i am compliing my list that I want to rename from .Sent to .Done
And then outputing it to the screen

echo "Is this OK?"
read user_response

Here I ask if what outputed was what the user wants to move

case $user_response in

"y"|"Y")

        for i in `ls -1 | grep $1 | grep  $2`
        do
        x=`echo $i | sed 's/\.Sent/\.Done/g'`
        mv $i DONE/$x;;
*)
        echo "No changes made ...";;

esac

Here I want the files to be moved from .Sent to .Done and then if the user says Y
and to print to screen "No changes made" if the user says no

When I run the script it lists all the files
then asks me if it's ok
when I hit Y
And I do a listing only 1 of the files listed were moved not all.

Thanks again.

Read the comments I posted earlier.

Answer the questions I posed.

Why are you using:

for i in `ls -1 | grep $1 | grep  $2`

That is almost certainly the wrong way to do whatever you are trying to do.

Where are you compiling a list? All I see is the display of the individual commands, and nowhere do you put them into a list.

I put the done after I get my listing instead of the end of the script.

Can I put my mv statement inside my Case Statement?

You asked me "how are you using it? We're not mind readers" to my comment "I am using it to ask the user if the list is correct"
Not sure what you are asking me?
I want my case statement to ask the user if the list (mv commands plus name of files) outputed to the screen is the files that they want renamed and then moved.
I hope that answers that questions correctly.

And yes I meant that the case does carry out what is to happen if the user says yes or no.
Yes- rename file from .Sent to .Done and then move to a subdirectly called DONE
No - just output to the screen "no changes made..."

I did change the for string to what you suggested.

I will change:
x=`echo $i | sed 's/\.Sent/\.Done/g'`
TO:
x=${i%%.Sent*}.Done${i#*.Sent}
But can you explain this syntax to me? I want to understand it before I just change it.

AGain I appreciate all your help and patience.

Of course.

Make sure the code you use is correct; in the script you posted, it was incomplete.

Just saying you made a change is not enough; I need to see the code you are trying to execute.

The case statment does not carry out the action; it makes a decision about which action is to be carried out. You then need the code to carry out the action.

You might find it helpful to put sections of your code into functions so that they can be tested idependently.

For example, you might want these functions: build_list, show_list, ask_user and move_files.

After you have written and tested the functions, the rest of your script could look like this:

build_list <file pattern>
show_list
if ask_user
then
   move_files
else
   echo "No changes made ..."
fi

I didn't suggest anything. I posted an example of what might be a better way of doing it. I don't know whether it is correct or not because you haven't explained the file selection criteria.

Does it do what you want? Does the wildcard pattern match the files you want?

What values are $1 and $2 likely to contain?

Does the following code display the files you want to work with?

printf "%s\n" *$1*$2*

It is explained in the "parameter expansion" section of your shell's man page. To see what the expansion does, try printing the individual parts:

printf "%s\n" "$i" "${i%%.Sent*}" "${i#*.Sent}"

Ok here is what I changed it to. I haven't tested it because I wanted input first.

function build_list
{
for i in *$1*$2*
do
x=`echo $i | sed 's/\.Sent/\.Done/g'`
done
}

function show_list
{
echo mv $i DONE/$x
}

echo "Is this Ok?"
read user_response
if [$user_response="y"|"Y"]
then
 mv $i DONE/$x;;
else
if [$user_response="n"|"N"]
then
echo "No changes made ...";;
fi
fi

You need to have a loop that prints the list then waits for the input of "Y|y"... if it is yes then have another loop that actually moves the file...

I would have a loop that prints the files and if it is "yes" then in the if statement have another loop that moves the files... (same loop but remove the "echo")...

If you had tested it first, you would have found most of the errors.

If you cannot tell us how it is not working, how do you expect us to help?

That is not wrong in ksh and bash, but it is not the standard syntax for defining a function and will not work in other shells. Standard is:

build_list()

Tha function does not build a list; only the last filename that matched the pattern would be stored in $x.

It makes your script more undertandable if you use meaningful names; that function doesn't show a list, it moves files.

...and it will fail if any filenames contain spaces or other pathological characters.

..and there is no logic behind that code. Think about what you are doing.

There are five errors in the line above.

you don't have to change if you think the sed version is more understandable to you.