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

Show git clone progress bar and percentage complete #71341

Merged
merged 1 commit into from Oct 17, 2019

Conversation

@jmbockhorst
Copy link
Contributor

jmbockhorst commented Mar 28, 2019

Adds functionality to show a progress bar and completion percentage during the git clone command. Closes #70467.

The notification shows a progress bar for both "Receiving objects" and "Resolving deltas" progress outputs, along with a percentage complete.

cloneProgress

@msftclas

This comment has been minimized.

Copy link

msftclas commented Mar 28, 2019

CLA assistant check
All CLA requirements met.

@joaomoreno joaomoreno added this to the Backlog milestone Mar 28, 2019
@joaomoreno joaomoreno added the git label Mar 28, 2019
@jmbockhorst jmbockhorst force-pushed the jmbockhorst:cloneStatus branch to fb0feb1 Mar 28, 2019
@@ -182,6 +185,9 @@ async function exec(child: cp.ChildProcess, cancellationToken?: CancellationToke
disposables.push(toDisposable(() => ee.removeListener(name, fn)));
};

const cloneProgressOutput = ['Receiving objects', 'Resolving deltas'];

This comment has been minimized.

Copy link
@joaomoreno

joaomoreno Oct 17, 2019

Member

This clone-specific work should not be done in the generic exec method. Clone should just switch to using stream instead.

This comment has been minimized.

Copy link
@joaomoreno

joaomoreno Oct 17, 2019

Member

Also, two more "events" are missing: Counting objects and Compressing objects.


if (progress) {
progress.report({
message: localize(cloneOutput.toLowerCase(), cloneOutput) + ': ' + inc + '%',

This comment has been minimized.

Copy link
@joaomoreno

joaomoreno Oct 17, 2019

Member

Localize can only be used with literal strings, there's no way this would ever work.

if (progress) {
progress.report({
message: localize(cloneOutput.toLowerCase(), cloneOutput) + ': ' + inc + '%',
increment: inc - prevInc

This comment has been minimized.

Copy link
@joaomoreno

joaomoreno Oct 17, 2019

Member

This progress is incorrect. It goes twice from 0 to 100, which causes the progress bar to reset halfway. Since we know there are two phases, we should just divide everything by 2.


// Check for git clone progress reporting
cloneProgressOutput.forEach(cloneOutput => {
if (s.startsWith(cloneOutput)) {

This comment has been minimized.

Copy link
@joaomoreno

joaomoreno Oct 17, 2019

Member

It is not guaranteed that output comes line by line. So you should use something like byline to get lines.

@joaomoreno

This comment has been minimized.

Copy link
Member

joaomoreno commented Oct 17, 2019

Despite all the comments, I didn't want to ask for changes and then come back and context switch once again... so I just fixed the PR and addressed the comments.

Thanks, good job! 🍻

@joaomoreno joaomoreno modified the milestones: Backlog, October 2019 Oct 17, 2019
@joaomoreno joaomoreno merged commit fb0feb1 into microsoft:master Oct 17, 2019
5 checks passed
5 checks passed
VS Code Build #20190328.36 succeeded
Details
VS Code (Linux) Linux succeeded
Details
VS Code (Windows) Windows succeeded
Details
VS Code (macOS) macOS succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.