yashsingh1/xdaforums huggingface dataset:
[Asus Transformer TF700] becomingx:
Code review as requested (full_pedantic_mode=on :)).

My comments are prefixed with `[X]' for easy deletion.

#!/system/bin/sh

#Add browser app list below
BROWSER_LIST="com.android.browser com.dolphin.browser com.mx.browser"
#com.android.chrome
#********************************************************************************************************

# [X] add a debug option?
# Set the following variable to "true" (case-sensitive, quotes optional) to enable debugging output
DEBUG=false

if $DEBUG
then
STDERR="2"
else
STDERR="-"
fi

# [X] for CACHE_FULL_PATH in $(find /data/data/ | grep '.*/cache$')
# [X] can be simplified to:
for CACHE_FULL_PATH in $(find /data/data -name 'cache' -type d)
# [X] removes the unnecessary grep(1) call and ensures we only get directories (-type d)
do
$DEBUG && echo "CACHE_FULL_PATH=$CACHE_FULL_PATH"
# [X] CACHE_NAME=${CACHE_FULL_PATH/'/data/data/'/''}
# [X] can be simplified to:
CACHE_NAME=${CACHE_FULL_PATH/'/data/data'}
# [X] the empty substitution can be omitted
# [X] --
# [X] change this:
# [X] CACHE_NAME=${CACHE_NAME/'/'/'_'}
# [X] to:
CACHE_NAME=${CACHE_NAME//'/'/_}
# [X] two forward slashes (//) after the variable name will do a "replace-all" rather than "replace-first"
$DEBUG && echo "CACHE_NAME=$CACHE_NAME"
# [X] no harm in using a more descriptive variable name here:
for BROWSER in $BROWSER_LIST
do
$DEBUG && echo "BROWSER=$BROWSER"
if [ "$CACHE_FULL_PATH" = "/data/data/$BROWSER/cache" ]
then
umount $CACHE_FULL_PATH 2>&$STDERR
# [X] this was CACHE_NAME, should be CACHE_FULL_PATH, no?
rm -rf $CACHE_FULL_PATH/* 2>&$STDERR
mount -t tmpfs -o size=50m $CACHE_NAME $CACHE_FULL_PATH
fi
done
done

# RESET VARIABLES / ARRAYS TO NIL ***************************************************************************
# [X] can be collapsed into a single command:
unset BROWSER BROWSER_LIST CACHE_FULL_PATH CACHE_NAME STDERR
[Asus Transformer TF700] elfaure:
[MENTION=2446267]becomingx[/MENTION] - Thanks for your review and edits. I will review them later and make any corrections needed and some changes for efficiency as outlined but please do realize the script works as-is (at least the mount part). There are no errors generated and the mounts are created for the BROWSER_LIST.

Good catch on the rm line! I didn't even notice that (@shardsx). When looking at variables its all a blur.
[Asus Transformer TF700] becomingx:
Sorry, wasn't trying to hijack your show, I just thought you wanted me to have a look to help clean it up. I will admit I can get carried away sometimes...

:)
[Asus Transformer TF700] elfaure:
Nah, it's not like that at all buddy. This show is a public play. I'm just the orchestrator. Sorry if my tone made you think differently. The peasant taught by the pedant becomes the prophet. Haptic verbose and near-anally retentive feedback, always! When it comes to syntax there is no room for error or lack of efficiency. It is especially important for me right now at this novice stage that I learn how better to do things from you masters so I'm programmed to think that way by default but I will always try to hack it out first for myself or its just not fun.
[Asus Transformer TF700] elfaure:
_that - I've already done it. I just need to clean it up. See below.
[Asus Transformer TF700] _that:
Interesting to see your approach, because I would do it completely differently. :)

My approach would be very simple, like:

function mount_browser_cache()
{
datadir=/data/data/$1/cache
if [ -d $datadir ]; then
umount ...
rm ...
mount ...
fi
}

mount_browser_cache com.android.browser
mount_browser_cache com.whatever.other.browser
mount_browser_cache com.just.another.browser
....
[Asus Transformer TF700] elfaure:
Interesting. _That is very contrite and efficient! Thanks for the tip! Not sure I'll use it though for this script.

I think you are forgetting how green I am at this. This is my first script, remember? Just figuring out how to work with variables, arrays and loops took me 6 hours. Working out the syntax errors took me an additional 2. That's where my Memorial Day went. Until I just read your code I had never heard of a "function" before except in Calculus! It'll come with time.

At this point I'm just happy to figure out any way to get it done and I'm pleased I was able to make it work. I do not have the ability yet to conceive of multiple solutions, compare and contrast, and choose the most efficient one. _That comes with experience I do not have. You are privy to a much larger toolbox - mine just has a single screw driver right now.
[Asus Transformer TF700] elfaure:
Where is your mount name variable? Or just use {$1}_cache?
[Asus Transformer TF700] _that:
A shell function is like another script in a script that you can call from your main script. (Yo dawg... :))

http://www.gnu.org/software/bash/manual/bashref.html#Shell-Functions

If all you have is a screwdriver and every problem looks like a nail... ;)

I'm not sure if the name even needs to be unique (and too lazy to look it up or try it out). You can also pass another parameter in every call (separated by a space) and refer to it as $2.
[Asus Transformer TF700] elfaure:
Agreed. But at this point I would turn over the screw driver and use the handle as a hammer. If that didn't work, then the toolbox itself but I'd get it in there somehow. I'm far more determined than I am capable.
[Asus Transformer TF700] elfaure:
Simple Variable Script

Duplicate post removed
[Asus Transformer TF700] elfaure:
Simple Variable Script

Here is a simple variable script to replace the last try. No [MENTION=4878287]_that[/MENTION], no functions yet but an improvement nonetheless. Actually I think this is better than an (external) function because its self-contained in a single script. This must be getting easier for me because this one took me 10 min.

#Enter your browser apk list below separated with spaces. Do NOT add firefox to this list.
LIST="com.android.browser com.mx.browser"

for BROWSER in $LIST
do
if [ -d /data/data/$BROWSER ]; then
umount /data/data/$BROWSER/cache 2>&-
rm -rf /data/data/$BROWSER/cache/* 2>&-
mount -t tmpfs -o size=50m "$BROWSER"_cache /data/data/$BROWSER/cache
fi
done

FIREFOX_CACHE_DIR="$(ls -d /data/data/org.mozilla.firefox/files/mozilla/*.default/Cache 2>/dev/null)"
if [ ! -z "$FIREFOX_CACHE_DIR" ] && [ "$FIREFOX_CACHE_DIR" != "." ] && [ "$FIREFOX_CACHE_DIR" != "/" ] && [ -d $FIREFOX_CACHE_DIR ];
then
umount $FIREFOX_CACHE_DIR 2>&-
rm -rf $FIREFOX_CACHE_DIR/* 2>&-
mount -t tmpfs -o size=50m firefox_cache $FIREFOX_CACHE_DIR
fi

unset FIREFOX_CACHE_DIR
unset LIST
[Asus Transformer TF700] _that:
A function is also part of the single script. Otherwise it's not a function but just a second script.
[Asus Transformer TF700] elfaure:
Simple Function Script

Despite the learning curve being so steep it caused me a migrane, thanks for accepting nothing less than efficient code. I think you turned me into a decent scripter now. You can be a real PITA to please sometimes [MENTION=4878287]_that[/MENTION] but there is a definitely a method to your madness. Looks pretty damn contrite if I do say so myself.

Question: Could the function definition have come before the for loop? I'm too burned out on this script to even test that out.

Note: Final script has been released for an 18 browser list with this structure. If you open up the script you will find the value separated list variable named LIST=" " which is where users can add more (heaven forbid) browsers between the quotes and the code will mount their caches to RAM without the user having to edit ANY code.

#Enter your browser apk list below separated with spaces. Do NOT add firefox to this list.
LIST="com.android.browser com.mx.browser"

for BROWSER in $LIST
do

function mount_browser_cache()
{
datadir=/data/data/$BROWSER/cache
if [ -d $datadir ]; then
umount $datadir 2>&-
rm -rf $datadir/* 2>&-
mount -t tmpfs -o size=50m ${BROWSER}_cache $datadir
fi
}

mount_browser_cache $BROWSER
done

FIREFOX_CACHE_DIR="$(ls -d /data/data/org.mozilla.firefox/files/mozilla/*.default/Cache 2>/dev/null)"
if [ ! -z "$FIREFOX_CACHE_DIR" ] && [ "$FIREFOX_CACHE_DIR" != "." ] && [ "$FIREFOX_CACHE_DIR" != "/" ] && [ -d $FIREFOX_CACHE_DIR ];
then
umount $FIREFOX_CACHE_DIR 2>&-
rm -rf $FIREFOX_CACHE_DIR/* 2>&-
mount -t tmpfs -o size=50m firefox_cache $FIREFOX_CACHE_DIR
fi

unset FIREFOX_CACHE_DIR
unset LIST
[Asus Transformer TF700] gordonbennet:
I only need to install the autostart script in Script manager right?
[Asus Transformer TF700] elfaure:
Correct. The umount_all script is only (for devs) in case you might need to unmount all the mounts (up to 18) created by this script for any reason which is a manual hassle. I created this for myself to facilitate testing and shouldn't be required by most users but threw it up there just in case anyone might need it or find it useful. Mainly for troubleshooting purposes so you can unload all the B2R mounts to find and fix other problems by first eliminating this script as the possible culprit without having to mess around with your SManager or init.d setup and reboot.
[Asus Transformer TF700] sbdags:
Cheers [MENTION=4197304]elfaure[/MENTION] this latest version will be integrated into CROMi-X :good::good:
[Asus Transformer TF700] becomingx:
The function definition *should* come before the loop, but...

The function declaration here serves no purpose, other than to bloat execution. Since you're using a loop to eliminate duplicated code, you don't need a function. It's essentially one or the other (use a function or use a loop), and the loop is the best bet here.

_that's suggestion would be fine if you still had each browser with its own six-line code block (i.e. the code inside the function above), but since you don't anymore, just do away with the function declaration and the call to it.

Technical aside: In fact, the function isn't even using the variable you pass to it. Shell script functions use variables named $1, $2, $3, etc. to access the parameters passed to them. Here, you're passing the value of $BROWSER (which would be accessed via the variable $1 while inside the function), but then you go and get the value again by referencing it ($BROWSER) globally.
[Asus Transformer TF700] elfaure:
Ah, I guess I should (always) have consulted with you guys before I released the new (final?) version. [MENTION=4878287]_that[/MENTION] used the $1 for the function definition he provided but being the newb I thought _that was just generic nebulous communication and _that I was supposed to (find the clue) to substitute my actual variable name there instead. I guess I got _that wrong and should have done some *research* into how functions worked (hey, what a concept) but no I just threw some code together because it looked straightforward and it worked the first time so I just ass-u-me-d it was correctly applied. Dammit. When you figure it out for yourself and it works its hard to know you've done anything wrong. Appreciate you pointing that out.

So then, it would be better to go back to the previous "simple variable script" structure for this case?

Regarding the function definition coming before the loop, since the variable $BROWSER hadn't been defined yet, it seemed like that couldn't happen and would error.
[Asus Transformer TF700] becomingx:
Yeah, what you posted here looks great! Although, I would add the $datadir variable you've got here if it isn't in the final script. it's much cleaner and it's good practice to not have duplicated string literals.

Nah, the function only executes once you call it. At that point $BROWSER is defined so there's no problem.

Technical gotcha: Shell scripting uses what's called implicit variable declaration. This means that you can reference any named variable you want any time you want, but if it hasn't been defined to anything it will resolve to nothing. No errors are thrown if you reference a variable that's never been explicitly defined.
[Asus Transformer TF700] elfaure:
[MENTION=2446267]becomingx[/MENTION] : When using both functions and variables in the same script, what is the best way to distinguish between them? Upper case for variables, lower case for functions?

And just so I am absolutely clear on this, I'm hearing you say that its NEVER efficient to use a function in a loop correct?
[Asus Transformer TF700] becomingx:
If you mean in the naming of, yeah, lowercase for function names and uppercase for variable names seems to be the common convention for shell scripts. Code in most other languages that I'm familiar with tend to use lowercase for both.

Normally the dollar sign ($) prefix would be a dead giveaway for a variable, but this language has the weird quirk that you can't use the dollar sign when declaring or assigning to a variable (unless you purposely mean to, won't get into that here ;)). If you try do something like `$BROWSER="firefox"', the shell will resolve the variable $BROWSER (which is empty) and the statement will be `="firefox"', which produces an error as it makes no sense.

Oh, and since we're talking on such a technical level, I feel I should point out for any casual readers that the shell scripting we're talking about is Bourne shell scripting. There are other shells with scripting capabilities, such as the Korn and C shells, which have similar but different syntaxes and conventions for their scripting. Everything here is for Bourne shell scripting though.

Well, the way the code was written, each iteration through the loop will re-define/declare/create the function. This is unnecessary and wasteful because the code inside the function never changes. Calling a function from inside a loop is perfectly fine and commonplace, but declaring a function inside a loop is pretty rare and probably not the right way to do things.
[Asus Transformer TF700] elfaure:
Great that you point that out for our readers. You didn't mention bash (Bourne again shell) which is the next closest to Bourne is it not?

So if I take the function definition outside and before the for loop but leave the function call in the for loop rather than using variables its copacetic? This seems more efficient to me than looping the actual 4 code lines (if -d, umount, rm, mount) because its only 1 function being called not 4 individual command lines. Or during the expansion of the function does it make it all about the same?
[Asus Transformer TF700] _that:
If you call the function, then every line inside the function still has to be executed.

The main purpose of a shell function is that you can call it multiple times and pass different arguments to it. Remember, in my original example, I didn't use a loop at all, I simply called the function once for each browser.

Main reasons for choosing a function were that I wanted to avoid the long list of all browsers in a single space-separated string that you need for the loop (even if you can split it to multiple lines), and that it is easy to pass more than one parameter per call if you need it. Also, when I saw your original script with the repeated blocks, a function was the most obvious solution to reuse a code block.
[Asus Transformer TF700] elfaure:
Bottom line, is it more, less, or the same efficiency to call a function inside a loop vs. just the code that function contains in the same loop?
[Asus Transformer TF700] _that:
In theory, calling the function should be marginally slower that inlining all the commands inside the loop, but you would see this only if you call the function millions of times and for a function that doesn't do anything.

As a general guideline, don't worry about performance unless something is too slow. Then measure, and only then optimize. Especially in a shell script, most of the time will be spent in the executed commands. If you need really complex logic, then rewriting the script in a proper programming language is usually the way to go.
[Asus Transformer TF700] elfaure:
That being the case, I plan to move the function definition before the for loop and call this script done unless I hear any objections. If its practically (effectively) a push on efficiency and if using a function in a loop is commonplace and not considered newb code structure then I would like to finish this work and move on to other things I'd like to accomplish. This script has taken me far too long and it is getting old on my eyes now but its been a great learning platform no doubt.

I want to sincerely thank especially you and [MENTION=2446267]becomingx[/MENTION] but also anyone and everyone who has contributed in any way to its success. Its been a great XDA community effort in helping me understand first how this script works and second educating me on how to code this script with variables and optimize its structure. I feel very very privileged to have had the opportunity *especially* as a newb to run with the big dogs on this project and hopefully you all feel in the end I did not get trampled too badly and pulled the maximum amount of the load I could handle at any given time. One of my greatest fears was that I would get stuck near the end somewhere having hit my mental wall and then you or someone else would have to complete it. I'm very happy I made it to the finish line without keeling over on this marathon. Usually when I set my mind at doing something, failure is not an option if given enough time and access to Google. Like I've said before, I am far more determined than I am capable. I know I couldn't have pulled it off by myself (without a night or online class) or timed its initial release and polished development any better WRT the Android community needing a 4.2 replacement for B2R if I tried. Its been real and its been fun guys, you are all extremely good. I think this thread has a lot to offer the newb looking to get into scripting. There really is a lot of excellent information contained within it now since all the basics have been covered with working examples.
[Asus Transformer TF700] becomingx:
Sorta, Bash is the GNU project's own implementation of/replacement for the original Bourne shell. Bash integrates conventions and ideas from the Bourne shell as well as a number of other different shells (C, Korn).

Many shells (e.g. ash, bash, mksh, zsh) have a Bourne shell compatibility mode which is usually activated when the shell is invoked as `sh'. If you do a `ls -l /system/bin/sh', you'll see that `sh' is a symbolic link to mksh (MirBSD Korn shell). Calling `mksh' directly will make it act like the Korn shell, calling it as `sh' will make it act more like a Bourne shell.

---------- Post added at 05:52 AM ---------- Previous post was at 05:42 AM ----------

No objections on that. Although, I do have a note about your exit code checking at the end of the script if you'd like ot hear it. :)
[Asus Transformer TF700] elfaure:
Great info thanks. Good to know that mksh works as a multi-call binary like busybox in a sense where it acts differently depending on how you call it. Regarding exit codes or any useful feedback you would care to provide me, please turn ON your verbose pedantic feedback mode and throw away the key. I always want to hear it. Speak to me GuruX.
[Asus Transformer TF700] becomingx:
OK, so $? is set to the exit code of the last command executed. When you check that variable in your script the last command executed is `unset'. Which used the way it is, will always be successful. So the "script error" part will never get executed. Also, in looking at it closer, I see the statement `echo 2>$1' inside the "script error" portion, which I think may be a typo of some kind?

I would just dump that whole part of the script. The `mount' commands don't get their stderr redirected so if a mount command fails, the user can tell. Otherwise, you'd need to check the return value of each individual command executed in the script, and set a flag variable if one of them happens to fail, and then check that variable at the end of the script, and report on it, and ... a lot of extra code. ;)
[Asus Transformer TF700] becomingx:
2>&1 will (note the ampersand, you've got a dollar sign), but the line you've got in the script doesn't really do anything.

`echo 2>&1' will redirect stderr to stdout, but only for that echo command (which isn't echoing anything anyway).

Ah, now I see... I kept noticing this error from your unmount script:

/Removable/MicroSD/Scripts/umount_all.sh[52]: can't create : No such file or directory

The command `echo 2>$1' will try to echo the numeral `2' to $1. Outside of a function, the $1, $2, etc. variables are the parameters passed to the script on the command-line. Since you aren't passing any, $1 is undefined and it's trying to echo `2' to a file with no name. lol

You can't inside the script, it hasn't exited yet. ;) If you want to see it from the command-line you can do:

script.sh; echo $?

Since you're doing a bunch of mount or unmount commands that aren't tied to or dependent on each other, overall successfulness of the script is hard to define. If one fails but the rest succeed, is that considered an overall script failure? What about if all but one fail?

As I said, I would just drop that part of the script. If one or more does fail, the error will be printed to the command-line for the user to see. If the script is run at boot, the user will probably never see the output of the script anyway.
[Asus Transformer TF700] elfaure:
I may drop the error checking, but I would prefer to figure it out and implement it personally. Why didn't either of these work?

function on_error_exit
{
echo "$1" 1>&2
exit 1
}

function mount_browser_cache()
{
datadir=/data/data/$BROWSER/cache
if [ -d $datadir ]; then
umount $datadir 2>&- || on_error_exit "umount failed; exiting"

#if [ $? != 0 ]; then on_error_exit "umount failed; exiting"
#fi

rm -rf $datadir/* 2>&-
mount -t tmpfs -o size=50m ${BROWSER}_cache $datadir
fi
}

I see that I forgot the () for the function. Oops. Fixed that, but still neither one works.

[Edit] Both do work but I had to reverse the logic. [ $? = 0] and &&. But why??

[Edit] Ah, when you remove the 2>&- for the umount you will see it already fails because either nothing is mounted initially or it fails because device or resource is busy if you have a mounted browser running. So effectively the error trapping has to be reversed then it works. Talk about counter-intuitive, my god.
[Asus Transformer TF700] elfaure:
Or we can elimate the complex error handling and do it a smarter way - by not creating the errors in the first place. At the end of the day I like this best. It runs a lot slower but its more verbose at the terminal as well. For a startup it shouldn't make much difference I would think because there is no tty output. It runs faster at the terminal if you suppress the stdout for the mountpoint with 1>&- appended.

function mount_browser_cache()
{
datadir=/data/data/$BROWSER/cache
if [ -d $datadir ]; then
if mountpoint $datadir; then
umount $datadir
# 2>&- && on_error_exit "umount failed; exiting"
fi
#if [ $? = 0 ]; then on_error_exit "umount failed; exiting"
#fi

rm -rf $datadir/* 2>&-
mount -t tmpfs -o size=50m ${BROWSER}_cache $datadir
fi
}
[Asus Transformer TF700] becomingx:
Cool, I like!

I've never used the `mountpoint' command before, I've always just grepped from /proc/mounts. I may have to adopt it. :good:
[Asus Transformer TF700] elfaure:
I like its verbose informative stdout(put). But I think mount | grep or find is faster. I tried an "if [ mount | grep ${datadir} ]" but it didn't work thought. Grep is a universally awesome command for piping directory structure patterns. I know its designed to find text in files but I find I use it often to replace "find". Maybe find | grep is the way to go?

You may want to do some reading on the reliability of this method for finding mountpoints. It's been awhile, but when I was researching this I ran across discussions of it failing for remote mounts and there were other lower level methods that were deemed more universally reliable to check for mountpoints but this was easy, quick, reliable and effective for this script.
[Asus Transformer TF700] xaueious:
For some reason I couldn't get this script to work on a TF300 on Android 4.2.1 with any APK based script managers. I had to end up using the install-recovery.sh method, which seems to work.