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

[Feature] Reviewing PR with submodule changes #1392

Merged
merged 41 commits into from Feb 9, 2018

Conversation

Projects
4 participants
@jcansdale
Contributor

jcansdale commented Dec 14, 2017

Functionality

  • Option to sync submodules after checking out a PR with submodule changes
  • Allows user to check out a different PR branch when there are submodule change
  • Usage tracking using NumberOfSyncSubmodules counter
  • When when the Sync button is pressed, the following GIt commands will be executed (see #848):
git submodule init
git submodule sync --recursive
git submodule update --recursive

image

To do

  • Write test plan for added, removed and changed submodules (paging @meaghanlewis)

Merged PRs

  • Reviewing PR with submodule changes MVP: #1371

Test plan

Syncing submodules

  1. Check out a PR
  2. Check out another PR with submodule changes (I've been using PR 614 for testing)
  3. Click the the [SyncSubmodules] button
  4. Once sync submodules completes, the branch should appear up to date

Ignoring submodules

  1. Check out a PR
  2. Check out another PR with submodule changes (I've been using PR 614 for testing)
  3. Ignore the [SyncSubmodules] button and check out original PR
  4. Original branch should appear up to date (previously this wouldn't have worked)

Git.exe not on PATH

  1. Remove Git.exe from PATH.
  2. Check out a PR
  3. Check out another PR with submodule changes (I've been using PR 614 for testing)
  4. Click the the [SyncSubmodules] button
  5. Something like this should appear:
    image

Related issues

  • Submodules cause all kinds of issues when checking out a PR: #826
  • What git commands should we execute when checking out a PR: #848
  • Workflow: Reviewing PRs that contain submodule changes: github/editor-tools#227
  • Zoom meeting - Reviewing a PR branch workflow: github/editor-tools#230

Fixes #826

jcansdale added some commits Dec 6, 2017

Allow PR branch checkout when submodule changed
If we checkout a PR branch, we should allow the user to move away from that branch (if they haven't touched anything).
Add sync submodules command
Only show button when there are sunbmodules to sync.
Shell out to to add submodule
Adding submodules using LibGit2Sharp wasn't working well.
Move SyncSubmodules out of GitClient
Doesn't use libgit2sharp so GitClient didn't make sense.
Display error if git command fails
If git.exe can't be found or returns an error code,  display stdout/err on UI.
Merge pull request #1371 from github/workflow/submodule-changes
Reviewing PR with submodule changes MVP

jcansdale added some commits Dec 18, 2017

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Jan 15, 2018

Deciding what to do when updating submodules and git.exe isn't on the users path.

Visual Studio 2018 will show the following if Git for Windows isn't installed:

image

image

The Install buttons simply opens the following in the users default browser:
https://git-scm.com/download/win

The current commit will show the following (it will show git not gitx):

image

I'm looking to make the message a little friendlier. The user will probably already have Git installed, so there is no need to go overboard with a solution!

Something along the lines of:

Couldn't find Git.exe on PATH.

Please install Git for Windows from:
https://git-scm.com/download/win

image

Show friendly error message if Git.exe not on PATH
VS 2017 will guide user to install Git for Windows as soon as Team Explorer is opened.
The user not having Git.exe on their PATH isn't the common case, so don't want to overengineer a solution.

@jcansdale jcansdale added this to In progress in 2.4.0 Jan 19, 2018

jcansdale and others added some commits Jan 19, 2018

Surface number of submodules to sync
Change IsSyncSubmodulesRequired to CountSubmodulesToSync.
@donokuda

This comment has been minimized.

Member

donokuda commented Jan 21, 2018

I pushed a couple of commits that clean up the sync submodules number. Here's what it looks like (with some stubbed info):

screen shot 2018-01-21 at 9 28 19 am

For the sake of space, I kept the action to "Sync." Could we use a tooltip that says something along the lines of "Sync [number] submodules?" I tried adding one myself, but couldn't figure out how to do it in the time I allotted for myself. (The GitHubActionLink component is based of a regular xaml button component if that's any help.)

I'm looking to make the message a little friendlier. The user will probably already have Git installed, so there is no need to go overboard with a solution!

@jcansdale I forgot if you mentioned this earlier, but we should move any errors from an action into an infobar.

meaghanlewis added some commits Jan 22, 2018

jcansdale added some commits Feb 7, 2018

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Feb 7, 2018

@grokys,

Just tried this on #1415 and I'm getting...

That was a bit of an edge case, but I'm glad you found it. I've tweaked it so that it won't give up quite so easily and will always do git submodule update. It now behaves as you expected.

@grokys

This comment has been minimized.

Contributor

grokys commented Feb 7, 2018

A few other things I've noticed

  • When you click the "Sync" button it can take quite a while to update the submodule, in which time there's no indication that anything is happening except for the fact you can't click on anything. This is also a problem for the "Push" and "Pull" buttons but as these operations are usually quicker you don't tend to notice. We should probably address this for all 3 operations in a separate PR
  • When there are submodules to sync, the "Push" and "Pull" buttons are always visible (though disabled) even if there is nothing to push and pull:

image

Is this correct or should we just be showing the "Sync" button in this state? Personally I don't mind the fact that you see the other 2 disabled buttons.

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Feb 7, 2018

When you click the "Sync" button it can take quite a while to update the submodule, in which time there's no indication that anything is happening except for the fact you can't click on anything.

Yes, I've noticed this as well. I'm hoping to add some incremental progress to give the user some idea of what's going on.

When there are submodules to sync, the "Push" and "Pull" buttons are always visible (though disabled) even if there is nothing to push and pull. Is this correct or should we just be showing the "Sync" button in this state?

This was intentional. The idea is that "Sync" is a peer of "Push" and "Pull", but only appears when there are submodules to sync (no point cluttering the interface for no reason).

jcansdale added some commits Feb 7, 2018

Find Git using where and use Resource string
Check that Git.exe can be found and show error from resource string if it can't.
Report progress using IProgress<string>
This should make it easier to display incremental progress.
@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Feb 7, 2018

@grokys,

This is also a problem for the "Push" and "Pull" buttons but as these operations are usually quicker you don't tend to notice. We should probably address this for all 3 operations in a separate PR

I've made some changes should should make reporting incremental progress easier. I agree we should probably address it in a separate PR and keep this one focused.

The issue that occurred when checking out #1415 has now been addressed. I checked it before updating and it successfully completed a git submodule update.

It now checks that Git.exe exists using WHERE and displays an error message (form a resource string) and aborts if it doesn't.

I think that's everything. 🤞

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Feb 7, 2018

I intended to hide all sync related UI when there are no submodules to sync. It appears this isn't the case. Oops!

image

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Feb 7, 2018

Here we go. Less clutter when there are no submodule changes:

image

Don't show Sync UI when there are no submodule changes
Hide the icon and counter as well as the Sync button.
@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented Feb 7, 2018

This is looking good to me 🎉

I also added some test scenarios here: github/editor-tools#268 can you take a 👀 @jcansdale?

@grokys

Nearly there I think!

using (var process = Process.Start(startInfo))
{
process.WaitForExit();

This comment has been minimized.

@grokys

grokys Feb 8, 2018

Contributor

This will block the UI thread waiting for the process to exit, probably not a good idea...

{
throw new ApplicationException(progress.ToString());
}
}

This comment has been minimized.

@grokys

grokys Feb 8, 2018

Contributor

Actually a low-impact way for now to show that something is happening during this method would be to do:

        async Task DoSyncSubmodules(object unused)
        {
            try
            {
                IsBusy = true;
                usageTracker.IncrementCounter(x => x.NumberOfSyncSubmodules).Forget();

                var progress = new CaptureProgress();
                var complete = await pullRequestsService.SyncSubmodules(LocalRepository, progress);
                if (!complete)
                {
                    throw new ApplicationException(progress.ToString());
                }
            }
            finally
            {
                IsBusy = false;
            }
        }

What do you think?

jcansdale added some commits Feb 8, 2018

@jcansdale

This comment has been minimized.

Contributor

jcansdale commented Feb 8, 2018

I've stopped it from blocking the main thread and changed it to use the busy spinner.

I did a spike where it where it shows progress on the status bar, which works very well but isn't ready for prime time. I'll address this in a separate PR so as not to block this one!

What do you reckon?

@grokys

grokys approved these changes Feb 9, 2018

Looks good to me!

@grokys grokys merged commit 4a4d4e5 into master Feb 9, 2018

3 checks passed

GitHub CLA @jcansdale has accepted the GitHub Contributor License Agreement.
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

2.4.0 automation moved this from Punted to Done Feb 9, 2018

2.4.3 automation moved this from In progress to Done Feb 9, 2018

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