You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This looks good, thank you! I'd love to squeeze this into the upcoming v3.6.0 release.
My only reservation is that we don't have many tests using sparse checkouts (just a pair of tests in one script), and none which exercise this change in particular.
Of course we have lots of tests of git lfs checkout and git lfs pull, the only callers of the ScanLFSFiles() function, which is the only code path by which the LsFilesLFS() function is used, so we know this change it doesn't break anything, which is the most important point to confirm.
But it would also be nice to validate that it has the desired effect of increasing the efficiency of the git ls-files command when a user has a sparse checkout with sparse index.
I managed to write one test, which I could push to this PR's branch, or if you'd prefer to just include it in a commit, that's cool too. I figured it might go into the t/t-pull.sh test script. I've confirmed that it fails without the changes from this PR.
I'll try to craft a similar version for the t/t-checkout.sh script which exercises the git lfs checkout command instead of git lfs pull, unless you beat me to it!
begin_test "pull with partial clone and sparse checkout"
(
set -e
# Only test with Git version 2.42.0 as it introduced support for the
# "objecttype" format option to the "git ls-files" command, which our
# code requires.
ensure_git_version_isnt "$VERSION_LOWER" "2.42.0"
reponame="pull-sparse"
setup_remote_repo "$reponame"
clone_repo "$reponame" "$reponame"
git lfs track "*.dat"
contents1="a"
contents1_oid=$(calc_oid "$contents1")
contents2="b"
contents2_oid=$(calc_oid "$contents2")
contents3="c"
contents3_oid=$(calc_oid "$contents3")
mkdir in out
printf "%s" "$contents1" > a.dat
printf "%s" "$contents2" > in/b.dat
printf "%s" "$contents3" > out/c.dat
git add .
git commit -m "add files"
git push origin main
assert_server_object "$reponame" "$contents1_oid"
assert_server_object "$reponame" "$contents2_oid"
assert_server_object "$reponame" "$contents3_oid"
# Create a partial clone with a cone-mode sparse checkout of one directory
# and a sparse index, which is important because otherwise the "git ls-files"
# command ignores the --sparse option and lists all Git LFS files.
cd ..
git clone --filter=tree:0 --depth=1 --no-checkout \
"$GITSERVER/$reponame" "${reponame}-partial"
cd "${reponame}-partial"
git sparse-checkout init --cone --sparse-index
git sparse-checkout set in
git checkout main
[ -d "in" ]
[ ! -e "out" ]
assert_local_object "$contents1_oid" 1
assert_local_object "$contents2_oid" 1
refute_local_object "$contents3_oid"
git lfs pull 2>&1 | tee pull.log
grep -q "Downloading LFS objects" pull.log && exit 1
# Git LFS objects associated with files outside of the sparse cone
# should not have been pulled.
refute_local_object "$contents3_oid"
)
end_test
This looks good, thank you! I'd love to squeeze this into the upcoming v3.6.0 release.
My only reservation is that we don't have many tests using sparse checkouts (just a pair of tests in one script), and none which exercise this change in particular.
Of course we have lots of tests of
git lfs checkout
andgit lfs pull
, the only callers of theScanLFSFiles()
function, which is the only code path by which theLsFilesLFS()
function is used, so we know this change it doesn't break anything, which is the most important point to confirm.But it would also be nice to validate that it has the desired effect of increasing the efficiency of the
git ls-files
command when a user has a sparse checkout with sparse index.I managed to write one test, which I could push to this PR's branch, or if you'd prefer to just include it in a commit, that's cool too. I figured it might go into the
t/t-pull.sh
test script. I've confirmed that it fails without the changes from this PR.I'll try to craft a similar version for the
t/t-checkout.sh
script which exercises thegit lfs checkout
command instead ofgit lfs pull
, unless you beat me to it!Originally posted by @chrisd8088 in git-lfs/git-lfs#5796 (review)
The text was updated successfully, but these errors were encountered: