Skip to content

Conversation

@norbertbuchmueller
Copy link
Contributor

Incrementally deepen shallow clones of submodules if the ref recorded for the submodule is not found. Similarly to how it's already handled for the main repo (see #205).

This fixes errors like this:

Cloning into '/tmp/build/get'...
fetch: Fetching reference HEAD
057d021 Merge pull request #249 from example/mybranch
Submodule 'mysubmodule' (git@github.com:example/mysubmodule.git) registered for path 'mysubmodule'
Cloning into '/tmp/build/get/mysubmodule'...
error: Server does not allow request for unadvertised object 0090ef08371edab8e18ee5e11327a0bed1ba8ab1
Fetched in submodule path 'mysubmodule', but it did not contain 0090ef08371edab8e18ee5e11327a0bed1ba8ab1. Direct fetching of that commit failed.

@norbertbuchmueller norbertbuchmueller force-pushed the make-depth-work-correctly-with-submodules branch from 149ea85 to 71b81b4 Compare September 7, 2018 14:26
@norbertbuchmueller
Copy link
Contributor Author

@vito what's your opinion of the new implementation?

@vito
Copy link
Member

vito commented Sep 12, 2018

@norbertbuchmueller Hey, sorry, been tuned out a bit on PRs as we try to squeeze a next release out before our big code restructure. I'll get back to this soon!

@norbertbuchmueller
Copy link
Contributor Author

Hey, sorry, been tuned out a bit on PRs as we try to squeeze a next release out before our big code restructure. I'll get back to this soon!

@vito How does it look now, would you have time to look into this soon?

@jeremyhuiskamp
Copy link

Hey @vito thanks for your review thus far! Do you have time to move this one along? It would be very helpful to us.

To be re-used for shallow submodule deepening.
Similarly to how it's handled for the main repo.
@norbertbuchmueller norbertbuchmueller force-pushed the make-depth-work-correctly-with-submodules branch from 71b81b4 to 02442f1 Compare November 2, 2018 08:54
@vito
Copy link
Member

vito commented Nov 19, 2018

@jeremyhuiskamp @norbertbuchmueller Sorry, things dragged on a bit longer than I expected. The latest changes look good to me, at least for now. Thanks for adding tests; to be honest without them (and without the existing ones) it'd be kinda hard for me to trust the code, as the bash-fu is getting pretty large.

I'm hoping the scary amount of bash will be shortlived as we redo this resource for the resources v2 interface in a proper language, so I'm not too bothered by merging this in. Thanks for your patience and sorry again for stringing y'all along for so long!

@vito vito merged commit 607f337 into concourse:master Nov 19, 2018
@nat-henderson
Copy link

I'm pretty sure this is broken if the user doesn't set a params.depth value and selects some but not all of their submodules. In that path, the submodules use the new script unconditionally. However, the script does nothing if the depth value is 0, so this breaks the contract of the submodule update script and leads to a staged delete for every file in the submodule (a pretty bad state!).

Might want to try to replicate it yourself, though - I'm only pretty sure. This is happening to me, but I have added a few tweaks to the version of this resource in my concourse pipeline (#172, while I'm waiting on spatial resource flows, for instance).

@nat-henderson
Copy link

I want this feature, so I'm trying to get my repo to work with it, but there are more problems. It also has opposite behavior with respect to re-setting submodule update values - it will use --unset if the repo previously had a value set, and --set back to the empty string if it didn't.

@norbertbuchmueller
Copy link
Contributor Author

I'm pretty sure this is broken if the user doesn't set a params.depth value [...]

@ndmckinley You are right. Sorry. (Actually this behavior does not even depend on whether all or just some submodules are selected.).

It also has opposite behavior with respect to re-setting submodule update values - it will use --unset if the repo previously had a value set, and --set back to the empty string if it didn't.

That's also correct. Sorry.

See #226 for the fixes for both of those bugs. It would be great if you could also test it with your repo.

nat-henderson added a commit to nat-henderson/git-resource that referenced this pull request Nov 21, 2018
@nat-henderson
Copy link

nat-henderson commented Nov 21, 2018

Added a few comments on that PR - it's pretty close to what I wound up running with yesterday.

(edit: although it got linked there, you definitely shouldn't use what I wound up running with - the branch logic I have written to work around the lack of concourse/concourse#1707 is pretty entwined with everything everywhere.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants