What git commands should we execute when checking out a PR

[jcansdale]

This issue is to discuss what git commands should be executed by default when checking out a PR.

At the moment when we checkout a PR, we don't update any submodules that might be in the repo.

Issues with the current situation

• If the SHA of a contained submodule has changed the user won't be able to checkout another PR.
• If any user files are created in a submodule that aren't in .gitignore the user won't be able to checkout another PR (this happens a lot with NCrunch).

What commands might we want to run

• git submodule update
• git submodule init
• git submodule sync
• git clean (in each submodule)

git submodule update

This would fix the most common issue when checking out a PR where there have been commits to a contained submodule.

git submodule init

If a submodule has been added?

git submodule sync

If a submodule has been moved?

git clean (in each submodule)

If NCrunch or a build has left user config or build artifacts in a submodule.

git checkout

Maybe we should allow the user to ignore untracked files (if git clean was deemed too dangerous).
(this has been implemented in #1302)

Hopefully the resident Git gurus will chime in with pros/cons and other ideas! 😄

See also #826

[grokys]

From what I understand, GHfM does this, but GHfW doesn't. @joshaber @shiftkey @niik @haacked, do you remember why you decided not to do this in GHfW? Was it because of problems in GHfM or something else?

[shiftkey]

@grokys the overhead of time spent updating submodules in the background is one of the reasons why you should be conservative when updating things.

• git submodule init

In theory, you can skip this step by passing --init to git submodule update and save yourself a command. HOWEVER, I had a look at some of our CI jobs but they do git submodule init first, because, and I quote the docs:

This command does not alter existing information in .git/config.

Why does this matter?

• git submodule sync

Git stores the submodule remote information in two places - .gitmodules and .git/config. As soon as these settings change in .gitmodules, you'll need to sync to ensure the .git/config entries are up to date. Maybe you can compare the .gitmodules blob SHA when switching between commits to see if this call is necessary, or maybe it's easier to just let Git do this.

And so, once you have the correct remote information setup for each submodule, you can now update each submodule in the working tree.

• git submodule update

👍 definitely needed, but don't forget to include a --recursive here in case your submodules also have submodules, dawg.

So yes, I think all three steps are essential, I'd just do them in this order after a checkout:

• git submodule init
• git submodule sync --recursive
• git submodule update --recursive

As to the others:

• git clean (in each submodule)

This feels like a reasonable thing to do, but isn't it going to come back to haunt you as soon as NCrunch does another build in the submodule and puts those files back? Isn't the submodule project going to need to have the right .gitignore rules to make this pain go away completely?

git checkout

Same thing. Or perhaps I'm missing some context on the workflows.

[jcansdale]

@shiftkey,

I should have been clearer, re: git clean (in each submodule). Should we give the user the option to do this before checking out a new PR (if we can see untracked files in a submodule).

[shiftkey]

@jcansdale it depends. Do you think your typical submodule workflows will include making changes to the submodule refs (i.e. committing changes within the submodule and then pushing them back to the remote)? Or is this just about keeping git status as clean as possible?

In general, I'd defer any non-essential work the user needs to do until as late as possible in the flow. Seeing the submodule marked as "dirty" isn't great, but it's also hard to guess what the state of the submodule will be until after you've updated it.

[jcansdale]

@shiftkey This use-case is reviewing a PR to the current repo. I don't think committing changes within a submodule would be relevant in this scenario.

I guess another question is, what if a PR contains a change to .gitmodules? Does all submodule commit bumping, adding, removal and moving happen within .gitmodules? Or is there extra magic going on?

[shiftkey]

@jcansdale

I guess another question is, what if a PR contains a change to .gitmodules?

The .gitmodules file will only tell you where the submodule is located in the working directory and where the remote repository can be found. .git/config contains the internal state of the submodule configuration for the repository (assume this is always in flux). And the commit to use for a submodule is stored in the tree associated with a given commit in the Git object database.

• Best case scenario: no .gitmodules change, so you just need to update when git status reports "new commit"
• Worst-case scenario: the .gitmodules file has some change (maybe need to init, maybe need to sync, and then update)
• Horrible-case scenario: someone forgot to commit the .gitmodules change, and the commit for a given submodule can't be found, so the repository is in a broken state
  • no .gitmodules file added -> unable to find remote -> ???
  • (less common).gitmodules change missing -> current remote doesn't have commit -> unable to checkout correct commit

[StanleyGoldman]

Need this too: #1205

[meaghanlewis]

I think this can be closed out now that #1392 is merged. What do you think @jcansdale?

[jcansdale]

Yes, I think. The answer is that we shouldn't automatically execute any commands, but we should give the user the option to sync. Now now do this in #1392. 🎉