Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed Zulip source code clone process #304

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PeterDaveHello
Copy link

Specify the clone branch to "$ZULIP_GIT_REF" and clone depth to 1 here.

Specify the clone branch to "$ZULIP_GIT_REF" and clone depth to 1 here.
@timabbott
Copy link
Member

Thanks for working on this @PeterDaveHello!

This is a good thought, though I think it would break this logic (which is used to display what version of Zulip one is running in a way that handles forkes correctly):

$ cat tools/cache-zulip-git-version 
#!/usr/bin/env bash
set -eu

cd "$(dirname "$0")/.."
remote="$(git config zulip.zulipRemote)" || remote=upstream
{
    git describe --always --tags --match='[0-9]*'
    branches="$(git for-each-ref --format='%(objectname)' "refs/remotes/$remote/master" "refs/remotes/$remote/*.x")"
    mapfile -t branches <<<"$branches"
    if merge_base="$(git merge-base -- HEAD "${branches[@]}")"; then
        git describe --always --tags --match='[0-9]*' -- "$merge_base"
    fi
} >zulip-git-version

How much time does it save? If it's not material, it's probably better to skip than figure out a way to address that.

@PeterDaveHello
Copy link
Author

@timabbott sorry that I didn't notice this issue, the speed up is limited and depends on the network environment, this is the test result from my home:

Cloning into 'zulip'...
remote: Enumerating objects: 348268, done.
remote: Counting objects: 100% (2826/2826), done.
remote: Compressing objects: 100% (1181/1181), done.
remote: Total 348268 (delta 1852), reused 2402 (delta 1644), pack-reused 345442
Receiving objects: 100% (348268/348268), 298.17 MiB | 3.34 MiB/s, done.
Resolving deltas: 100% (268431/268431), done.
Checking out files: 100% (4895/4895), done.

real    3m12.784s
user    2m34.091s
sys     0m40.748s
$ time git clone --depth=1 https://github.com/zulip/zulip.git
Cloning into 'zulip'...
remote: Enumerating objects: 5317, done.
remote: Counting objects: 100% (5317/5317), done.
remote: Compressing objects: 100% (4141/4141), done.
remote: Total 5317 (delta 1122), reused 2968 (delta 953), pack-reused 0
Receiving objects: 100% (5317/5317), 24.57 MiB | 2.73 MiB/s, done.
Resolving deltas: 100% (1122/1122), done.
Checking out files: 100% (4895/4895), done.

real    0m17.023s
user    0m7.664s
sys     0m3.390s

@timabbott
Copy link
Member

We likely can't merge this without some extension of tools/cache-zulip-git-version to make Zulip report its own version number correctly.

$ cat tools/cache-zulip-git-version 
#!/usr/bin/env bash
set -eu

cd "$(dirname "$0")/.."
remote="$(git config zulip.zulipRemote)" || remote=upstream
{
    git describe --always --tags --match='[0-9]*'
    branches="$(git for-each-ref --format='%(objectname)' "refs/remotes/$remote/main" "refs/remotes/$remote/*.x")"
    mapfile -t branches <<<"$branches"
    if merge_base="$(git merge-base -- HEAD "${branches[@]}")"; then
        git describe --always --tags --match='[0-9]*' -- "$merge_base"
    fi
} >zulip-git-version

@klardotsh
Copy link
Member

git describe cannot (reliably) work on shallow clones (--depth=N), but as a StackOverflow answer helpfully describes, looping git fetch --depth=<N+1> until a tag (determined by git ls-remote) is found should be an applicable fix over in zulip/zulip. I should have cycles to take that on tomorrow and put up a PR which would then unblock this PR.

This said, as far as I can tell this PR would need to be held until a tag is cut on zulip/zulip that includes such a depth-aware tools/cache-zulip-git-version (meaning, presumably, a docker-zulip targeting 6.1 or beyond would be the first to benefit from this).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants