fix: Use Magisk mirror only when it is really possible

[energypatrikhu]

Updated the mount script's magisk mirror check, because when using the mount argument via the CLI, the mount script fails with a 'No such file or directory' error.

[laur89]

Haven't used magisk for a few years now, but their docs suggest current implementation is sound.

If it's really the mirror that's not existing, then either

1. we should raise it w/ Magisk project, or;
2. make the check safer by adding to line #58 something like:

   [ -e "${'$'}MIRROR" ] || unset MIRROR

[energypatrikhu]

In general what should the mirror folder contain?
In the docs it says it is used as Partition mirrors, but as for me it is empty..

Shouldn't we check if it is there AND has content?

Currently it is checked as a FILE not a directory too,
this code snippet was copied from the revanced-manager's mount script.

[oSumAtrIX]

It is empty for me as well. Not sure why it is empty though. I guess we can check the contents of mirror folder. If it is empty, unset it. Please move the code block back to its original location at the bottom though.

[energypatrikhu]

As a temp fix for my own project, I added a check for that, if it's good maybe I can add it to the code?
I check if the mirror folder don't exists or if it is, but it's empty

MAGISKTMP="$(magisk --path)" || MAGISKTMP=/sbin
MIRROR="$MAGISKTMP/.magisk/mirror"
if [ ! -d "$MIRROR" ] || [ -z "$(ls -A "$MIRROR" 2>/dev/null)" ]; then
    MIRROR=""
fi

[oSumAtrIX]

Can't you just check if it is not empty? Because if it doesn't exist, then it will return false anyways

[energypatrikhu]

Ohh that's true, then the folder check is not required here,
then using

if [ -z "$(ls -A "$MIRROR" 2>/dev/null)" ]; then

should be enough, no?

Or is there a better way to check if it's empty?

[laur89]

is_dir_empty() {
  find -L "$1" -mindepth 1 -maxdepth 1 -print -quit | grep -q .
  [ $? -eq 1 ]
}

[oSumAtrIX]

should be enough, no?

I guess so

[energypatrikhu]

Should I update the code to use the folder contents check instead of the folder exists check?

[oSumAtrIX]

yes

[oSumAtrIX]

There is no reason to first set MIRROR and then unset it again. Invert the if condition and only set the variable then.

[energypatrikhu]

Inverting the if check means we would have to write the path twice

then this would change

MIRROR="${'$'}MAGISKTMP/.magisk/mirror"
if [ -z "$(ls -A "${'$'}MIRROR" 2>/dev/null)" ]; then
    MIRROR=""
fi

to

if [ -n "$(ls -A "${'$'}MAGISKTMP/.magisk/mirror" 2>/dev/null)" ]; then
    MIRROR="${'$'}MAGISKTMP/.magisk/mirror"
fi

as we need to check if the .magisk/mirror is not empty.

I think the how it is currently is good because it avoids the need to write the path twice,
but if you prefer the inverted logic I can change it.

[energypatrikhu]

Added back check for magisk installation (0820352), because using this script with other root methods would fail, removing it was my bad.

Also probably would have to fix RV Manager too, as there is no check for magisk installation either, and fix check at Line 138 too.
Should I create a PR for that?

[welcome]

Thank you for contributing to ReVanced. Join us on Discord to receive a role for your contribution.

[oSumAtrIX]

Thanks!