feat(cone-mode): sparse checkout cone mode support #1497
Conversation
|
Ooh OK docstring parse errors, I'll return tomorrow 🙏 |
| @@ -0,0 +1,462 @@ | |||
| # sparse_patterns.py -- Sparse checkout pattern handling. | |||
There was a problem hiding this comment.
Please add tests for each of the functions/classes in this file.
There was a problem hiding this comment.
yep, this one is nice and simple to test as the file is just a readable list of patterns, will do tests next
|
Super, thanks will address ! 😄 |
…d with real repo)
…rns from dir paths
|
Whew, okay all of those comments were tidying/refactoring tasks that are all done now. Also I'd somehow managed to misread the meaning of "un-exclude", what the docs mean there is that there is always the following base paths: i.e. "include In other words it says "don't include any dirs until told to do so" So "un-exclude" is just a confusing way to say "include directories when told to do so". To be super clear, as far as I know you never put excludes on except in that one line as a special case. This is why cone mode is simpler, because you are only dealing with the one type of pattern. That should make it simple to test :-) |
|
existing tests are now all passing again |
|
Tests are almost all passing, will revisit and see if there are logic bugs or test bugs 🐛 🔨 |
|
🎉 Looks great overall - I'll take a closer look when I have some more time to do a full review. |
|
Whew okay all tests passing now, so that gives me confidence this is a functioning implementation of cone mode 😄 The one last tricky bit was ensuring that “patterns suffixed with a slash (meaning they are dirs) must only match directories but can match files in subpaths of the directory pattern”. My initial implementation had just said “let’s just treat these as only the directory and not deal with the subpaths for now”. I’ll be able to go over this again and refactor the logic here now that the functionality is pinned down by tests. |
|
I’d also point out (and have just edited the docstrings accordingly) that this is (still!) not the full implementation of git’s sparse checkout with all features, namely it is only the recursive cone mode, not the parent cone mode. I.e. when you say “add directory foo to the cone” you get all subdirs of foo, not just the top one. The recursive mode would be more work and I think it’s a perfectly fine MVP to introduce just this, and ensure we’re happy with this before any further round. PRs typically go wrong when you change course midway and this is looking stable here. The distinction is mentioned in this blog post: https://github.blog/open-source/git/bring-your-monorepo-down-to-size-with-sparse-checkout/
|
| elif not matched: | ||
| # Not anchored: we can do a simple wildcard match or a substring match. | ||
| # For simplicity, let's use Python's fnmatch: | ||
| matched = fnmatch(path_str, pattern) or fnmatch(path_str, f"*/{pattern}") |
There was a problem hiding this comment.
Any reason this can't use dulwich.ignore.Pattern instead, since fnmatch doesn't exactly match?
There was a problem hiding this comment.
ahhh have I reinvented a wheel 🤦♂️ I will try to edit it to use that then, my bad!
There was a problem hiding this comment.
Thanks! Otherwise LGTM, thanks for adding tests and refactoring.
(I appreciate this isn't complete for sparse checkout support, but it's a big step towards fully supporting it so that's great!)
|
Please also take a look at the failing tests - you probably want to compare sets rather than sequences? |
Following up on my previous submission #1495 (from #405) which laid the groundwork for cone mode I wanted to complete the task and now have got the test cases all working.
It's once again pretty late by the time I'm finishing up, so I'll review with fresh eyes but I've at least ran all the linters this time :-) Tests pass and are named normally.