Allow creation of a configuration object in an in-memory repository#4263
Allow creation of a configuration object in an in-memory repository#4263
Conversation
|
Found this when trying to update LibGit2Sharp with the new 0.26 release candidate, so I would like to bring this in for 0.26. |
aa0fd40 to
10a221e
Compare
pks-t
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for cleaning up after me 🙂 Just one tiny issue with tabs vs spaces in the test.
| void test_network_remote_local__anonymous_remote_inmemory_repo(void) | ||
| { | ||
| git_repository *inmemory; | ||
| git_remote *remote; |
There was a problem hiding this comment.
Thanks! My editor set up is wacky on this machine.
10a221e to
db26671
Compare
|
Sorry for having the change requests tickle in, but I just noticed that the tests are added before the issue is fixed. This would break bisectability, so I'd prefer if the test commit is moved last here. |
|
I can change this. I've done the test-first pattern for a while, as has nulltoken. I actually don't know @carlosmn 's opinion here. I know that he's opened some PRs that have failing tests, but I don't know if he ends up rewriting the history after he's made them 💚 . I've always felt like keeping things able to be bisected for any particular test is important. That is to say that we should certainly never commit a build break. But introducing a failing unit test doesn't bother me. My assumption here is that most people are running |
|
I certainly think it provides some value to have a "clean" history here. I'm actually kind of inspired by upstream git, as they also very carefully avoid having commits in between that break the test suite in any way. They do have the benefit of being able to tell a real story, though:
What I like about it is that this chain demonstrates both that there actually was a failure before and that it is fixed by the respective commit, so that one can exactly pinpoint both points in time. But we're missing this feature in the clar, and my pull request has been rejected when I tried to introduce it. But yeah. It's only been the first time that I noticed the inverted pattern here. So while I think it's a nice to have, I'll leave it to you to judge whatever suits us most. |
|
What PR was that? I must have forgotten about this. |
Disambiguate error values: return `GIT_ENOTFOUND` when the item cannot exist in the repository (perhaps because the repository is inmemory or otherwise not backed by a filesystem), return `-1` when there is a hard failure.
When in an in-memory repository - without a configuration file - do not fail to create a configuration object.
Given a wholly in-memory repository, ensure that we can create an anonymous remote and perform actions on it.
|
Oh, it wasn't the lbigit2 repo but the clar repo. See clar-test/clar#77 |
db26671 to
fe9a5dd
Compare
Whitespace issue was fixed, introducing failing tests is under discussion
|
Thanks for the background. I hadn't seen that PR, or had forgotten about it completely. I'll have a think on this. I did reorder these commits here but I think we should have a larger thought about how we want to do this or if we care. I feel like we might be getting past the point of diminishing returns, but that might just be me. |
Regression: we cannot create a configuration object against an in-memory remote, since the path to the repository configuration cannot be created.
Change
git_repository_item_pathto returnGIT_ENOTFOUNDwhen the path could not be constructed because the underlying directory does not exist. (For example, when requesting theconfigfile on an in-memory repository.Add a unit test for a concrete scenario: create an anonymous remote on an in-memory repository (to allow for emulating
git ls-remote).