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

Submodules cause all kinds of issues when checking out a PR #826

Closed
jcansdale opened this Issue Feb 1, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@jcansdale
Contributor

jcansdale commented Feb 1, 2017

  • GitHub Extension for Visual Studio version: 2.2.0.6
  • Visual Studio version: 2015

There are many ways a submodule can become dirty/changed without the user intending to touch the module. Here is one example:

  1. Open the github/VisualStudio solution and GitHub tool window
  2. Click the title of PR 813
  3. Click Checkout fixes/move-submodules
  4. Click Pull Requests icon.
  5. Click the title of PR 734
  6. Click Checkout fixes/move-submodules
  7. Hovering over the Checkout button shows the following:
    image

This is disconcerting because the user hasn't made any changes. To fix it from inside Visual Studio, they'll need to.

  1. Open the Team Explorer tool window
  2. Click on the Changes icon
  3. Right-click on the script module and Submodule Update.
  4. On the GitHub tool window, click the refresh icon
  5. The Checkout button will now be clickable

This seems like a lot of ceremony to go though have not intentionally changed anything! Now, I know the user should have done a Submodule Update (or submodule sync) after checking out PR 813, but they won't necessarily know they have to on other projects.

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Feb 1, 2017

Another scenario is if someone enables NCrunch on a solution that contains projects in a submodule (not an uncommon situation). If you you do that with the github\VisualStudio solution, you end up in a superficially similar situation to the one above.

image

Except, this time there is no obvious way to checkout or undo changes in the module from inside Visual Studio. The user will need to drop to the command line and git submodule foreach git clean -xdf. This isn't at all obvious for someone who hasn't knowingly touched a submodule and isn't familiar with now to maintain them!

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Feb 1, 2017

Obviously these are limitations of Visual Studio's Git support, but they're issues that will prevent our GitHub users from easily moving between PRs.

I wonder if we could offer some kind of submodule nuke functionality that would get submodules into a clean state so that another PR can be checked out?

Maybe an option to clean submodules here?
image

@jcansdale jcansdale added the usability label Feb 1, 2017

@Haacked

This comment has been minimized.

Contributor

Haacked commented Feb 1, 2017

Now, I know the user should have done a Submodule Update (or submodule sync) after checking out PR 813, but they won't necessarily know they have to on other projects.

That's pretty much what we do on Desktop. When you check out a branch, we update the submodules. Are we using Visual Studio's API to check out branches or implementing this ourselves.

Note: git clean -xdf is a dangerous command to run. It will delete ignored files. Files that may be important to the user. We should pretty much never run that command ourselves.

If there are changes in a submodule that are legitimate changes, there's not much we can do other than potentially prompt the user to reset the changes or stash them.

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Feb 5, 2017

Are we using Visual Studio's API to check out branches or implementing this ourselves.

We're actually blocking them before they can even attempt a check out a PR.

image

That's pretty much what we do on Desktop. When you check out a branch, we update the submodules. Are we using Visual Studio's API to check out branches or implementing this ourselves.

I presume that would avoid this from happening (which caused the message above)? I'm not sure which API we're using for check out.

image

If there are changes in a submodule that are legitimate changes, there's not much we can do other than potentially prompt the user to reset the changes or stash them.

How about we give the user the option to do:

git submodule foreach 'git stash --include-untracked'

At least that would give them a non-destructive way out of their pickle with the possibility of rollback if they removed something important.

Alternatively we could simply prompt them with @shana's cell phone number. 😉

@grokys

This comment has been minimized.

Contributor

grokys commented Feb 6, 2017

That's pretty much what we do on Desktop. When you check out a branch, we update the submodules.

@Haacked On the current GHfW Desktop? That never seems to happen for me.

I think this may illustrate the fact that working with submodules is difficult. We could trying doing a stash/unstash, but from what I understand from Desktop, they're moving away from automatic stashing as there are gotchas, is that right @Haacked?

In addition, the rest of the VS workflow for changing branches isn't under our control, so these problem would remain there.

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Feb 6, 2017

I think this may illustrate the fact that working with submodules is difficult.

From a brief glance at the code, it looks like we can detect when it's an issue with submodules? Rather than attempting to do something clever, maybe we could give the user a link to a help page. This could discuss some of the issues and how to fix them on the command line.

At the moment all someone needs to do is enable NCrunch and they won't be able to checkout a new PR using the extension. Although this situation is easy to fix, it's not at all obvious what has happened or how to go about fixing it.

In addition, the rest of the VS workflow for changing branches isn't under our control, so these problem would remain there.

How does VS behave at the moment? I think it default to doing a submodule update when someone first clones a repo, but does it do anything with submodules when they move between branches?

@Haacked

This comment has been minimized.

Contributor

Haacked commented Feb 6, 2017

@Haacked On the current GHfW Desktop? That never seems to happen for me.

Hmm, maybe I'm wrong about this. I sometimes confuse what we intend to do with what we actually did. Or Mac and Windows behavior. @github/desktop anyone know the actual behavior?

but from what I understand from Desktop, they're moving away from automatic stashing as there are gotchas, is that right @Haacked?

I believe so. At least when changing branches.

@joshaber

This comment has been minimized.

Member

joshaber commented Feb 7, 2017

@github/desktop anyone know the actual behavior?

Yes, on Mac we update submodules after switching branches. I can't speak for Windows. We haven't talked about what we'll do on Desktop TNG.

@grokys

This comment has been minimized.

Contributor

grokys commented Feb 7, 2017

From a brief glance at the code, it looks like we can detect when it's an issue with submodules?

Thing is, it's not always knowable whether submodules will be clean before you update the submodules. For example, checkout master since the grokys/ncrunch PR has been merged and ensure submodules are up-to-date and clean. Now check out release/2.1.0 and update submodules. The submodules are now unclean because the changes from the ncrunch branch are still present.

Now checkout master and update submodules again - and again you'll see that you have a dirty working directory because a submodule was removed in the time between 2.1.0 and now.

So my worry is that there are so many edge cases here - most of which require user intervention - that trying to do these things manually a) wouldn't work a lot of the time and b) would introduce complexity that would probably result in problems as there are so many different combinations of things that can go wrong.

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Feb 10, 2017

The thing I find terrible from a usability POV is:

  1. Checkout a PR
  2. Don't touch anything
  3. User finds they're blocked from checking out another PR
  4. Navigate to Changes view on Team Explorer
  5. See there are changed submodules (user thinks WTF?)
  6. Right-click and 'Submodule Update' on each individual submodule
  7. Navigate back to GitHub tool window
  8. Click Refresh button to refresh the Checkout link
  9. Finally able to checkout another PR

This is a lot of ceremony to simply navigate between two PRs! Chances are the user won't know what's going on or how to fix it. The only reason I know how is because I spend half my day doing it. 😉

What are the risks of doing a submodule update on a clean repo? Are there many scenarios when the user won't want to do this?

Could we have a 'Checkout Submodules' checkbox that defaults to being checked when the user checks out a PR? This would mirror the default behavior when Visual Studio originally cloned their repo:
image

@jcansdale jcansdale added the bug label Feb 10, 2017

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Feb 10, 2017

I've changed this one to 'bug' because @shana agrees we should (at the very least) be doing a submodule update when checking out a PR.

We might also want to do a submodule init (in case a submodule has been added) and/or submodule sync (in case a submodule has moved). I'll create a new issue to discuss this. #848

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Dec 12, 2017

This will be fixed by #1371.

@meaghanlewis meaghanlewis moved this from Medium Priority to In Progress in BUGS Dec 13, 2017

@grokys grokys closed this in #1392 Feb 9, 2018

BUGS automation moved this from In Progress to Done Feb 9, 2018

@meaghanlewis meaghanlewis removed this from Done in BUGS Mar 12, 2018

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