Fix git_submodule_sync with relative url#5322
Conversation
pks-t
left a comment
There was a problem hiding this comment.
Indeed, thanks for this straight forward fix! There is one nit with regards to formatting as you use spaces instead of tabs for the dispose, but as soon as that's fixed I'll be happy to merge.
Would you like to try and come up with a test? Please feel free to say "no" if you don't feel comfortable or are lacking time to implement one, that's perfectly fine.
Anyway, thanks for this lovely contribution!
src/submodule.c
Outdated
| } | ||
|
|
||
| git_buf_dispose(&key); | ||
| git_buf_dispose(&effective_submodule_url); |
There was a problem hiding this comment.
This line is using spaces instead of tabs :)
There was a problem hiding this comment.
oh sorry, will fix this asap ;)
|
I've modified a test in test_submodule_modify__sync() to check for this behaviour. |
|
@kdj0c I can confirm that this fixes #5301 when I patch the git2 rust crate with this branch 🎉 It does break some edge case however: if the superproject has no commits (i.e. it's a freshly initialized repository), running |
|
I looked into your edge case, I think the problem is that get_url_base() calls git_repository_head() which returns GIT_EUNBORNBRANCH error. |
|
@gdetrez can you try again with my latest patchset ? it "may" solve the edge case. |
git_submodule_sync should resolve submodule before writing to .git/config to have the same behavior as git_submodule_init, which does the right thing.
The submodule code has grown out-of-date regarding its coding style. Update `git_submodule_reload` and `git_submodule_sync` to more closely resemble what the rest of our code base uses.
When setting up relative URLs for a submodule, then we resolve it to the actual location and write that into ".git/config" instead of writing the relative value. We do not yet have a test to nail down this behaviour, which is now being added by this commit.
|
While your added test exercised the code path, it didn't verify that results are what we'd expect. I've thus replaced it with a new test case that explicitly checks resulting paths and pushed that on your branch. I've naturally retained authorship of your commits. I hope you don't mind that fixup :) The fix for unborn branches looks good to me, too, so this PR should be ready to go. I'll give it a few days to settle and wait for feedback. If nobody complains, I'll merge it towards the end of this week. Please feel free to reach out to us in case I forget. |
|
Thanks a lot, I'm looking forward to have it merged ;) |
git_submodule_sync should resolve submodule before writing to .git/config
to have the same behavior as git_submodule_init, which does the right thing.
This should solve issue #5301