Teach gitscanner how to filter in ScanTree()#1745
Conversation
ttaylorr
left a comment
There was a problem hiding this comment.
Just the one comment, otherwise looks good 👍
commands/command_checkout.go
Outdated
| } | ||
|
|
||
| func checkoutFromFetchChan(gitscanner *lfs.GitScanner, filter *filepathfilter.Filter, in chan *lfs.WrappedPointer) { | ||
| func checkoutFromFetchChan(in chan *lfs.WrappedPointer) { |
There was a problem hiding this comment.
I think this function still needs a *filepathfilter.Filter. Additionally, we should probably put a safety net around this and guard it with an integration test.
There was a problem hiding this comment.
Basically, a pull operation is split into two steps:
fetch: CallScanTree()and download unique LFS objects. This uses the filters properly.checkout: CallScanTree()again, and ensure all filenames matching the downloaded OIDs have their LFS pointers converted.
There is a subtle bug that my test does not catch:
Let's say I have a repo with the following duplicate LFS objects:
- file1.bin
- file1-copy.bin
- dupe.bin
If I run git lfs pull --include=file*.bin, it should download 1 object, but only checkout file1.bin and file1-copy.bin. I don't think it necessarily hurts anything to convert dupe.bin, but it feels inconsistent.
Also, I think I can rewrite pull() to run the command a single time, download unique LFS objects just once, but checkout duplicate LFS objects with different filenames.
|
Added a test that guards the original behavior: bce58fc I really want to refactor |
In commit d5fd152 of PR git-lfs#1745 the "git lfs fetch" command, among others, was revised to set the Filter element of the GitScanner structure, and no longer pass a common GitScanner created in the main fetchCommand() function to various other functions such as scanAll() and fetchAll(). As a result, the GitScanner structure created in the fetchCommand() function was no longer used, but it was not fully removed at the time, as was done in the "git lfs checkout" command's checkoutCommand() function, for instance. Therefore we can simply remove this unused structure's initialization now in the fetchCommand() function.
In commit d5fd152 of PR git-lfs#1745 the "git lfs fetch" command, among others, was revised to set the Filter element of the GitScanner structure, and no longer pass a common GitScanner created in the main fetchCommand() function to various other functions such as scanAll() and fetchAll(). As a result, the GitScanner structure created in the fetchCommand() function was no longer used, but it was not fully removed at the time, as was done in the "git lfs checkout" command's checkoutCommand() function, for instance. Therefore we can simply remove this unused structure's initialization now in the fetchCommand() function.
This removes the inline filepathfilter checking from the
commands, relying on the gitscanner to do it.