gitPullOrClone - shallow cloning only for GitHub repos

[hhromic]

Hi @joolswills ,

I noticed that gitPullOrClone limits the usage of shallow-cloning, e.g. passing --depth 1, to GitHub repositories only. I went to collect all the calls to this function to find out what other repository services are being used by the scriptmodules and I found (via testing) that in addition to GitHub, also GitLab and BitBucket repos support shallow cloning correctly.

Maybe these could be added too to the whitelist for passing --depth 1 here?

RetroPie-Setup/scriptmodules/helpers.sh

Lines 363 to 365 in 307db2d

	if [[ "$__persistent_repos" -ne 1 && "$repo" == *github* && -z "$commit" ]]; then
	git+=" --depth 1"
	fi

It should be a simple change, therefore I'm just filing an issue here. If you don't have time but agree on adding these, I can make a PR too. Just let me know. Thanks!

[hhromic]

To obtain all unique remote http(s) repos used by gitPullOrClone calls:

$ grep -h 'gitPullOrClone ' -r scriptmodules | grep -Eo 'https?://(\S+)/' | cut -d'/' -f3 | sort -u
bitbucket.org
github.com
gitlab.com
salsa.debian.org
www.6809.org.uk

To obtain non-http(s) repos such as variables usage:

$ grep 'gitPullOrClone ' -r scriptmodules | grep -vE 'https?://(\S+)/'
scriptmodules/ports/abuse.sh:    gitPullOrClone "$md_build" git://github.com/Xenoveritas/abuse
scriptmodules/supplementary/emulationstation.sh:    gitPullOrClone "$md_build" "$repo" "$branch"
scriptmodules/supplementary/gamecondriver.sh:        gitPullOrClone "$module_name" "$github_url/$module_name"

All the results use GitHub repositories, so there is no problem.

Testing shallow-cloning support of the results:

$ git clone --depth 1 https://bitbucket.org/linuxwolf6/splitwolf.git  # works
$ git clone --depth 1 https://salsa.debian.org/bluetooth-team/bluez   # works
$ git clone --depth 1 http://www.6809.org.uk/git/xroar.git            # fails
fatal: dumb http transport does not support shallow capabilities

Conclusion: only www.6809.org.uk seems to not support shallow-cloning.

[joolswills]

I think I'd still prefer a whitelist. Means we don't have to worry if we add another repo that doesn't support shallow clones. Or - can we just default to shallow and catch the return code if it fails?

[joolswills]

Yeah I think we should actually try depth 1 then if it errors do a full clone. What do you think?

[joolswills]

Return code is 128 which isn't too helpful as it returns this for other issues afair. String matching is ok so long as this error message isn't localised. We could just try again without depth on first error I guess.

[joolswills]

Sorry for spam. I did some code for this but it feels messy with the additional logic. So my vote is for a whitelist again :-)

[joolswills]

This was my quick test

diff --git a/scriptmodules/helpers.sh b/scriptmodules/helpers.sh
index 38e5c840..15cdd150 100644
--- a/scriptmodules/helpers.sh
+++ b/scriptmodules/helpers.sh
@@ -355,6 +355,8 @@ function gitPullOrClone() {
     local branch="$3"
     [[ -z "$branch" ]] && branch="master"
     local commit="$4"
+    local depth="$5"
+    [[ -z "$depth" && "$__persistent_repos" -ne 1 && -z "$commit" ]] && depth=1
 
     if [[ -d "$dir/.git" ]]; then
         pushd "$dir" > /dev/null
@@ -364,12 +366,17 @@ function gitPullOrClone() {
         popd > /dev/null
     else
         local git="git clone --recursive"
-        if [[ "$__persistent_repos" -ne 1 && "$repo" == *github* && -z "$commit" ]]; then
-            git+=" --depth 1"
+        if [[ "$depth" -ne -1 ]]; then
+            git+==" --depth $depth"
         fi
         git+=" --branch $branch"
         printMsgs "console" "$git \"$repo\" \"$dir\""
-        runCmd $git "$repo" "$dir"
+        if ! runCmd $git $depth_param "$repo" "$dir"; then
+            if [[ "$depth" -ne -1 ]]; then
+                unset md_ret_errors[${#md_ret_errors[@]}-1]
+                gitPullOrClone "$1" "$2" "$3" "$4" -1
+            fi
+        fi
     fi
 
     if [[ -n "$commit" ]]; then

[joolswills]

Simplified code by making it recursive. Doesn't seem too bad but I don't like the idea of calling git twice for other errors.

[hhromic]

Yes I also thought at some point to make logic to auto-detect if shallow-cloning didn't work, but like you I also concluded that it gets more messy for no real reason.

However after looking at your quick test code, I got thinking that is not a bad idea to actually have an optional parameter to gitPullOrClone to control depth explicitly, by default set to 1 like you already did to preserve current behaviour. We won't need to do recursive logic at all.

Then, for these hostings that we know don't support shallow-cloning, we explicitly pass 0 to their call to gitPullOrClone from the scriptmodule: At the moment the only one needing to do this would be xroar:

gitPullOrClone "$md_build" http://www.6809.org.uk/git/xroar.git 0.35.4 "" 0

In the future, when developing scriptmodules, if anyone gets an error because of shallow-cloning we just need to disable it via this extra optional argument.

What do you think? Otherwise, yes we can just go with the current whitelist. In this case I think github and gitlab are by far the most used hosts so I would just add gitlab to the whitelist.

[hhromic]

Closing after fix from #2840 . Thanks!