Fix submodule deepening #226
Conversation
It restored the wrong submodule.*.update Git config value - when it was originally unset, then after restoring it was set to the empty string, and when it was originally set, after restoring it was unset.
The shallow clone deepening script is no-op if depth is 0 (or not set in params). Setting the submodule update method to run this script when depth is 0 means the submodule will not be correctly updated.
`assets/in` should only call the script when deepening a shallow clone is actually needed - ie. when depth > 0. Now with the submodule shallow clone deepening already checking depth > 0, that's simple.
| if [ "$depth" -gt "$max_depth" ]; then | ||
| echo "Reached depth threshold ${max_depth}, falling back to deep clone..." | ||
| git fetch --unshallow origin | ||
|
|
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| # repo, Git silenty turns it into a deep clone | ||
| if [ ! -e "$git_dir"/shallow ]; then | ||
| echo "Reached max depth of the origin repo while deepening the shallow clone, it's a deep clone now" | ||
| break |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
assets/in
Outdated
| "$bin_dir"/deepen_shallow_clone_until_ref_is_found "$ref" | ||
| fi | ||
|
|
||
| git checkout -q $ref |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| if [ "$depth" -gt 0 ]; then | ||
| for submodule in $submodules; do | ||
| # remember submodule update config | ||
| update_conf_was_set=0 |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@norbertbuchmueller Thanks for following up on this! Do you have a chance to review the above feedback? (And thanks for the review, @ndmckinley!) |
This is a failing test for concourse#226 (comment)
This is a failing test for concourse#226 (comment)
Previously it did not check out the correct commit ref of the submodule.
IMHO it's more straightforward this way.
I could not help fixing it. :-)
|
@ndmckinley Thank you very much for the review! If you could have another look to verify my fixes that would be really great. @vito Sorry about the long delay. |
nat-henderson
left a comment
nat-henderson
left a comment
There was a problem hiding this comment.
I'm not a maintainer, but this looks right to me. @vito, right?
| git_dir="$(git rev-parse --git-dir)" | ||
| readonly git_dir | ||
|
|
||
| while ! git checkout -q "$ref" &>/dev/null; do |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| if [ "$depth" -gt 0 ]; then | ||
| for submodule in $submodules; do | ||
| # remember submodule update config | ||
| update_conf_was_set=0 |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@ndmckinley Thanks for reviewing! I think this is good to go too, so I'll merge this in to fix those bugs and ship a new version so y'all can test it out. Happy to review any further PRs! @norbertbuchmueller thanks again! |
|
@ndmckinley Thanks for the review!
Thanks @vito! Looking forward to try the next release. |
3 participants
The fix for deepening submodules in #212 introduced two bugs:
params.depthwas not set (or was set to 0) it failed to update (check out) the submodules, see Makeparams.depthwork correctly with submodules #212 (comment)params.depthwork correctly with submodules #212 (comment)This PR fixes those (plus an unrelated ancient bug in a test) and improves the tests of checking out submodules.