Add --refetch option to fetch
#5975
Conversation
|
I usually avoid commenting on draft PRs, since I know folks don't always appreciate comments on work-in-progress, so I'll keep this brief! I just happened to notice that in the final commit here, which builds on the work from PR #5973, there's a mention in one comment of an option named The Apologies for steering you in the wrong direction there, and thanks for spotting the inconsistency! |
ah, thanks a lot for this comment! Yeah, I think I hesitated between |
redsun82
force-pushed
the
fetch-force
branch
from
82317e4 to
aa8b8a9
Compare
redsun82
force-pushed
the
fetch-force
branch
from
aa8b8a9 to
428d6df
Compare
chrisd8088
left a comment
chrisd8088
left a comment
There was a problem hiding this comment.
Wow, thank you again for all the work on these PRs! This one also looks great.
I had a few suggestions here and there, but nothing at all significant.
Thank you again so much for writing up a proposal in #5888 and seeing it through to completion. We sincerely appreciate the help in developing new features for Git LFS!
t/t-fetch.sh
Outdated
| end_test | ||
|
|
||
|
|
||
| begin_test "fetch (empty file)" |
There was a problem hiding this comment.
While experimenting with the new tests in this PR, I discovered (as you may have already!) that the new fetch --refetch with remote and branches test you've added validates the behaviour of the --refetch option with empty files, namely that it correctly ignores them.
I was a bit surprised to realize, though, that the reason this occurs is solely due to the ordering of these tests. If the new p.Size == 0 conditional is removed from the pointers()/pointersToFetch() function in the Go code, the test fails, as we would expect ... but only if the test is located after the existing fetch (empty file) test. That's because the fetch (empty file) test adds an empty file to the Git repository which is shared by many of the tests in the t/t-fetch.sh script.
(Reusing a test repository between tests is not really a practice we follow any more, but the origins of the current t/t-fetch.sh script go back a long way.)
If we were ever to reorder the tests, we might silently lose the validation of the --refetch option's behaviour with regard to empty files; the test would continue to pass, even if we subsequently changed the implementation of the command such that we accidentally disabled the code which skips empty files when re-fetching.
That's a bit unlikely, I admit, but to be on the safe side, would you be willing to add a comment here along the lines of the following?
| begin_test "fetch (empty file)" | |
| # NOTE: Do not reorder this test below the subsequent tests, as they depend on | |
| # the empty file it creates in the shared test repository. | |
| begin_test "fetch (empty file)" |
There was a problem hiding this comment.
Yeah, that was how I was made aware of the empty file special case. Your comment about test reordering makes a lot of sense!
I'm applying your suggestion for now, but maybe another approach would be to just add the empty file up in the init for fetch tests test, and remove this test altogether? What do you think?
There was a problem hiding this comment.
I went ahead and did that in its own commit, I can revert that if you don't think it's a good idea 🙂
There was a problem hiding this comment.
Cool, that works too and means we test the empty file case in all the interdependent tests, which is great.
2 participants
This adds a
--refetchoption that makesfetchrefetch objects that are already present locally. This can be useful especially in combination with the--dry-runoption to see what is fetchable regardless of the current local status, or with--jsonfrom #5974 (which this is built on top of) to get download information.