Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defend against the waking nightmare that is git submodules #60

Merged
merged 5 commits into from
Jan 4, 2017

Conversation

sdboyer
Copy link
Member

@sdboyer sdboyer commented Dec 4, 2016

Seriously, this makes me want to die. But Masterminds/glide#698 is correct - we can't just ignore it.

I think this covers it. Probably. Maybe. ugh. I'm just so super glad that every single checkout/pull will now have to run three additional commands afterwards in order to be safe.

@sdboyer sdboyer changed the title Defend against the waking nightmare that is submodules Defend against the waking nightmare that is git submodules Dec 4, 2016
@mattfarina
Copy link
Member

@sdboyer FYI, had to rerun the CI jobs. Made no sense why they failed and upon re-run they passed.

@sdboyer
Copy link
Member Author

sdboyer commented Dec 6, 2016

oh yeah thanks, i meant to bump this with a note about that. i tried to kick them myself, but i didn't have perms

@tamird
Copy link

tamird commented Dec 6, 2016

Not to be a ball buster, but there are some fun git commands and flags here that I haven't seen before and make me wonder "what is the minimum required git version?".

@sdboyer
Copy link
Member Author

sdboyer commented Dec 6, 2016

@tamird totally reasonable question. i had the same concern while initially writing this, but proceeded because all of these turned out to be quite old. In fact, I don't even know how old for most of them, because I didn't feel like chasing it down past the release notes reorg that happened in the git repo 6 years ago...

  • git checkout-index, and all the opts to it, are basically original plumbing
  • git submodule foreach was introduced in 1.6.1
  • git clean -xdf appears to all be basically OG. The second -f is necessary for killing subdirs that have their own .git, as of 1.6.4.2
  • git clone --recursive was introduced in 1.6.5, along with git submodule ops learning --recursive

@tamird
Copy link

tamird commented Dec 6, 2016 via email

return NewLocalError("Unexpected error while defensively cleaning up after possible derelict submodule directories", err, string(out))
}
// Then, repeat just in case there are any nested submodules that went away.
out, err = s.RunFromDir("git", "submodule", "foreach", "--recursive", "clean", "-x", "-d", "-f", "-f")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you experience the issue where a nested submodule wen't away from the git clean? I'm curious of the motivation to run this command again.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the same command, but run against all submodules, recursively. If we submodule A@v1, and A@v1 submodules B. If we switch our version to something that submodules A@v2, which does NOT submodule B, then there'll be a derelict A/B dir that needs to be cleaned up. The first git clean won't get it, because it doesn't recurse - only this second one will.

@mattfarina
Copy link
Member

@sdboyer I'm really like a submodule test or 2. One with and one without nested submodules. Is that something you have time to write?

@sdboyer
Copy link
Member Author

sdboyer commented Dec 13, 2016

pretty swamped atm not sure if i'll have the time to put together the fixture repos for that this week

@sdboyer
Copy link
Member Author

sdboyer commented Dec 29, 2016

glad we added tests :)

@sdboyer
Copy link
Member Author

sdboyer commented Dec 30, 2016

please kick those tests - failure was erroneous

@mattfarina
Copy link
Member

@sdboyer kicked and now passing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants