[Feature] Reviewing PR with submodule changes #1392

jcansdale · 2017-12-14T18:19:43Z

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 What git commands should we execute when checking out a PR #848):

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

To do

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

Merged PRs

Reviewing PR with submodule changes MVP: Reviewing PR with submodule changes MVP #1371

Test plan

Syncing submodules

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

Ignoring submodules

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

Git.exe not on PATH

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

Related issues

Submodules cause all kinds of issues when checking out a PR: Submodules cause all kinds of issues when checking out a PR #826
What git commands should we execute when checking out a PR: 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

If we checkout a PR branch, we should allow the user to move away from that branch (if they haven't touched anything).

Only show button when there are sunbmodules to sync.

Adding submodules using LibGit2Sharp wasn't working well.

Doesn't use libgit2sharp so GitClient didn't make sense.

This reverts commit c746313.

If git.exe can't be found or returns an error code, display stdout/err on UI.

Reviewing PR with submodule changes MVP

jcansdale · 2018-01-15T12:23:50Z

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:

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):

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

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.

Change IsSyncSubmodulesRequired to CountSubmodulesToSync.

donokuda · 2018-01-21T17:43:39Z

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

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.

jcansdale · 2018-01-23T16:28:08Z

@donokuda, Looking sweet! 😄

I've added a tooltip like this:

grokys · 2018-02-07T11:39:48Z

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

Which is strange because submodules/Octokit.GraphQL doesn't exist on this branch. Doing a git submodule update from the command line works fine however.

grokys

I think this looks good. I'm still a little concerned about a couple of things:

Shelling out to git.exe - though this looks simpler than I'd imagined probably isn't a problem
This is really Team Explorer/git functionality. In particular git.exe now has this feature. I can see that it's definitely useful however so maybe implementing it here isnt the wrong thing to do.

But having weighed everything up, I think I'm lightly 👍 on this feature. However, I can't approve it yet because the feature didn't work with one of the PRs I tried it on (see comment above).

grokys · 2018-02-07T11:46:23Z

src/GitHub.App/Services/PullRequestService.cs

+                    // If culture isn't English, user will see the local equivalent of:
+                    // "'git' is not recognized as an internal or external command"                    
+                    var message = output.ToString();
+                    if (exitCode == 1 && message.StartsWith("'git' is not recognized as an internal or external command,", StringComparison.Ordinal))


This strikes me as a bit of a hack. Might there be a way to check whether git.exe is on the path and display an error rather than parsing the (localizable) error message?

I can change it to use WHERE to find out where Git.exe is on the path.

grokys · 2018-02-07T11:46:38Z

src/GitHub.App/Services/PullRequestService.cs

+                    var message = output.ToString();
+                    if (exitCode == 1 && message.StartsWith("'git' is not recognized as an internal or external command,", StringComparison.Ordinal))
+                    {
+                        message =


This should be in a resources file.

jcansdale · 2018-02-07T14:56:08Z

@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 · 2018-02-07T15:05:28Z

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:

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 · 2018-02-07T15:31:22Z

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).

Check that Git.exe can be found and show error from resource string if it can't.

This should make it easier to display incremental progress.

jcansdale · 2018-02-07T17:24:52Z

@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 · 2018-02-07T18:27:01Z

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

jcansdale · 2018-02-07T18:36:19Z

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

Hide the icon and counter as well as the Sync button.

meaghanlewis · 2018-02-07T23:53:28Z

This is looking good to me 🎉

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

grokys

Nearly there I think!

grokys · 2018-02-08T10:11:35Z

src/GitHub.App/Services/PullRequestService.cs

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


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

grokys · 2018-02-08T10:17:06Z

src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs

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


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?

Fix where task

jcansdale · 2018-02-08T19:08:26Z

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?

done

grokys

Looks good to me!

jcansdale added 13 commits December 14, 2017 11:08

Allow PR branch checkout when submodule changed

0a89577

If we checkout a PR branch, we should allow the user to move away from that branch (if they haven't touched anything).

Add IsSyncSubmodulesRequired method and tests

873494c

Add sync submodules command

94f97bb

Only show button when there are sunbmodules to sync.

Don't pause after sync submodules

862f657

Refresh VM after SyncSubmodules completes

ff800b7

Send SyncSubmodules output to progress

c658ea0

Refactor helper methods into RepositoryHelpers

4dc92b2

Shell out to to add submodule

c746313

Adding submodules using LibGit2Sharp wasn't working well.

Move SyncSubmodules out of GitClient

fbfebe6

Doesn't use libgit2sharp so GitClient didn't make sense.

Don't write SyncSubmodules.cmd file

064590b

Mixing libgit2sharp and shelling out doesn't work well

39c27ed

This reverts commit c746313.

Display error if git command fails

5a79b53

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

fba23fd

Reviewing PR with submodule changes MVP

jcansdale added the feature label Dec 14, 2017

jcansdale assigned jcansdale and donokuda Dec 14, 2017

jcansdale added 4 commits December 18, 2017 14:32

Add usage tracking for SyncSubmodules

ae7960e

Merge branch 'master' into feature/submodule-changes

b4c08a3

Merge branch 'master' into feature/submodule-changes

065a51b

Merge branch 'master' into feature/submodule-changes

e61e110

jcansdale and others added 5 commits January 15, 2018 12:53

Show friendly error message if Git.exe not on PATH

7a75448

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.

Surface number of submodules to sync

a8ef148

Change IsSyncSubmodulesRequired to CountSubmodulesToSync.

Update XAML to show number of submodules to sync

17db3fd

Stub out submodule UI

2fd2f9a

Clean up

4cbe256

Meaghan Lewis added 2 commits January 22, 2018 10:22

Merge branch 'master' into feature/submodule-changes

41dafdd

Update PullRequestServiceTests

d107f23

grokys suggested changes Feb 7, 2018

View reviewed changes

jcansdale added 2 commits February 7, 2018 13:57

Merge branch 'master' into feature/submodule-changes

47adf68

Don't give up if submodule init or update fails

6a7c76d

jcansdale added 4 commits February 7, 2018 15:51

Find Git using where and use Resource string

4997e02

Check that Git.exe can be found and show error from resource string if it can't.

Convert to returing async Task rather than IObservable<Unit>

40b81dc

Convert to use IProgress<string> rather than Delegate<string>

f894e2a

Report progress using IProgress<string>

f589bfa

This should make it easier to display incremental progress.

Don't show Sync UI when there are no submodule changes

0cc03a2

Hide the icon and counter as well as the Sync button.

grokys previously requested changes Feb 8, 2018

View reviewed changes

jcansdale added 5 commits February 8, 2018 15:15

Don't block the main thread

ce00b9b

Fix where task

Remove redundant SuppressMessage attribute

eee289e

Set IsBusy=true while syncing submodules

fe20f56

Revert to using Action<string> instead of IProgress

2820be9

Merge branch 'master' into feature/submodule-changes

78fa298

jcansdale mentioned this pull request Feb 9, 2018

[SPIKE] Don't do "clean" check before attempting to check-out PR #1304

Closed

Merge branch 'master' into feature/submodule-changes

3ea7ea7

grokys approved these changes Feb 9, 2018

View reviewed changes

grokys merged commit 4a4d4e5 into master Feb 9, 2018

meaghanlewis mentioned this pull request Mar 8, 2018

What git commands should we execute when checking out a PR #848

Closed

hemangandhi mentioned this pull request Apr 19, 2020

Initial Edits to Add the Wiki as a Submodule hariamoor-zz/todo-cli#1

Merged

[Feature] Reviewing PR with submodule changes #1392

[Feature] Reviewing PR with submodule changes #1392

Uh oh!

Conversation

jcansdale commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Functionality

To do

Merged PRs

Test plan

Syncing submodules

Ignoring submodules

Git.exe not on PATH

Related issues

Uh oh!

jcansdale commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

donokuda commented Jan 21, 2018

Uh oh!

jcansdale commented Jan 23, 2018

Uh oh!

grokys commented Feb 7, 2018

Uh oh!

grokys left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grokys Feb 7, 2018

Choose a reason for hiding this comment

Uh oh!

jcansdale Feb 7, 2018

Choose a reason for hiding this comment

Uh oh!

grokys Feb 7, 2018

Choose a reason for hiding this comment

Uh oh!

jcansdale commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grokys commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcansdale commented Feb 7, 2018

Uh oh!

jcansdale commented Feb 7, 2018

Uh oh!

jcansdale commented Feb 7, 2018

Uh oh!

jcansdale commented Feb 7, 2018

Uh oh!

meaghanlewis commented Feb 7, 2018

Uh oh!

grokys left a comment

Choose a reason for hiding this comment

Uh oh!

grokys Feb 8, 2018

Choose a reason for hiding this comment

Uh oh!

grokys Feb 8, 2018

Choose a reason for hiding this comment

Uh oh!

jcansdale commented Feb 8, 2018

Uh oh!

grokys left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jcansdale commented Dec 14, 2017 •

edited

Loading

jcansdale commented Jan 15, 2018 •

edited

Loading

grokys left a comment •

edited

Loading

jcansdale commented Feb 7, 2018 •

edited

Loading

grokys commented Feb 7, 2018 •

edited

Loading