Share objects when possible during git checkout from localmirrors - #251
Conversation
This avoids the need to separately check this in other code.
This directly clones the refs that are desired. Where the localmirrors exist on the same system as the new clone the objects are merely referenced instead of being copied, making it much faster, especially on high latency filesystems. A small test on my VDI gives a 3x speedup: | Command | Mean [s] |Min [s]|Max [s]| Relative | |:-------------------------------------------------------------------|--------------:|------:|------:|------------:| | `git clone --branch vn14.2 localmirrors:MetOffice/um.git` | 2.524 ± 0.802 | 1.988 | 4.641 | 3.16 ± 1.00 | | `git clone --shared --branch vn14.2 localmirrors:MetOffice/um.git` | 0.799 ± 0.014 | 0.770 | 0.821 | 1.00 | The `--shared` flag won't do anything for remote repositories that can't directly share objects and will be silently ignored.
People not having their mirrors setup is a common support question; hopefully this will encourage people to actively seek out our written guidance on this. I have however left it fairly generic as the process will be different at different sites.
github-actions
Bot
added
the
cla-required
github-actions
Bot
added
cla-signed
James Frost (jfrost-mo)
changed the title
James Bruten (james-bruten-mo)
left a comment
James Bruten (james-bruten-mo)
left a comment
There was a problem hiding this comment.
Thanks James, if my understanding of --shared is correct then this seems a sensible addition. Just one comment from me about the use of --branch
| # Clone if the repo doesn't exist. If the mirror is local we don't copy | ||
| # the objects to make it much faster. | ||
| try: | ||
| run_command(f"git clone --shared --branch {fetch} {mirror_loc} {loc}") |
There was a problem hiding this comment.
We need to be able to checkout hashes as well as tags/branches, so I don't think we can do this in one step with --branch?
There was a problem hiding this comment.
Good catch. There is the --revision option to clone which would do what we want here, except that it is relatively new, only being adding in git 2.49 in early 2025. Therefore we don't have it on the HPCs.
Given that I'll switch this to using an explicit checkout command.
There was a problem hiding this comment.
Fixed in d4ca08f. I decided to avoid adding extra complexity by accepting some performance lost from checking out the files twice, rather than having a second code path just hashes. I've also left a comment suggesting this code is upgraded when we can assume at least git 2.49.
There was a problem hiding this comment.
I think the script is already handling branch, tag, hash and we can achieve the shared optimisation without the updated git version.
For example:
REPO="um"
REF="vn14.2" # or 23785d7b7fc65364d4160e211f3593d5ec45a08c or stable
git clone --shared --no-checkout localmirrors:MetOffice/${REPO}.git # only initialise the clone
git -C $REPO fetch origin $REF
git -C $REPO checkout $REFYou can also avoid the detached HEAD warning, by giving a local branch name, e.g.,
...
git -C $REPO fetch origin $REF:test
git -C $REPO checkout testThere was a problem hiding this comment.
Do we need the extra fetch?
git clone --shared --no-checkout localmirrors:MetOffice/um.git && git -C ./um checkout mainseems to work just fine.
There was a problem hiding this comment.
Yes and No. I suggested this as a safety net, in case the localmirror indexing has not completed during the sync cycle. Anyway, in such case the checkout should fail too.
The `--branch` option to git clone only takes branchs or tags.
James Frost (jfrost-mo)
changed the title
James Bruten (james-bruten-mo)
left a comment
James Bruten (james-bruten-mo)
left a comment
There was a problem hiding this comment.
Thanks James. Looks good to me. I've also checked that the merging functionality hasn't been affected, and all looks good.
9cb84d6
into
MetOffice:main
4 participants
PR Summary
Sci/Tech Reviewer:
Code Reviewer: James Bruten (@james-bruten-mo)
This directly clones the refs that are desired. Where the localmirrors exist on the same system as the new clone the objects are merely referenced instead of being copied, making it much faster, especially on high latency filesystems. A small test on my VDI gives a 3x speedup:
git clone --branch vn14.2 localmirrors:MetOffice/um.gitgit clone --shared --branch vn14.2 localmirrors:MetOffice/um.gitThe
--sharedflag won't do anything for remote repositories that can't directly share objects and will be silently ignored.I have also included an extra error message when cloning from mirrors fails. People not having their mirrors setup is a common support question; hopefully this will encourage people to actively seek out our written guidance on this. I have however left it fairly generic as the process will be different at different sites.
Code Quality Checklist
Testing
I have run an existing workflow (
u-dn674) with this change and observed no change in behaviour. Clones not from the localmirror are unchanged.Security Considerations
The only potential security relevant change is the sharing of objects, as that makes it possible for the owner of the local mirror to change objects after the repository has been cloned. This won't affect the checkout out code, but could affect repositories that switch between branches. Git can detect this tampering through its hashes, however I do not believe that is verified on a regular checkout.
This is unlikely to be a major issue as the owner of a mirror can already change the content of the repository before it is checked out. Additionally write access to the mirror's underlying files is a sensitive permission that should be restricted; at least at the Met Office this is restricted to only a few trusted individuals.
AI Assistance and Attribution
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review