fix: rework sync bar moments

Let the `SyncBar` decide what to do with the fossil output, thus simplifying logic separation. Plus add tests.

src/repository.ts

@@ -21,7 +21,6 @@ import {
     Commit,
     CommitDetails,
     ConfigKey,
-    Distinct,
     FossilBranch,
     FossilCheckin,
     FossilClass,
@@ -71,8 +70,6 @@ import { localize } from './main';
 import type { ExecFailure, ExecResult, Reason } from './fossilExecutable';
 const iconsRootPath = path.join(path.dirname(__dirname), 'resources', 'icons');
 
-export type Changes = Distinct<string, 'changes from `fossil up --dry-run`'>;
-
 type AvailableIcons =
     | 'status-added'
     | 'status-clean'
@@ -279,6 +276,9 @@ export class Repository implements IDisposable, InteractionAPI {
         return this._sourceControl;
     }
 
+    /**
+     * An operation started or stopped running
+     */
     @memoize
     private get onDidChangeOperations(): Event<void> {
         return anyEvent(
@@ -715,8 +715,9 @@ export class Repository implements IDisposable, InteractionAPI {
 
     async update(checkin?: FossilCheckin): Promise<void> {
         // Update command can change everything
-        await this.runWithProgress(UpdateAll, () =>
-            this.repository.update(checkin)
+        await this.runWithProgress(
+            { syncText: 'Updating...', ...UpdateAll },
+            () => this.repository.update(checkin)
         );
     }
 
@@ -886,7 +887,8 @@ export class Repository implements IDisposable, InteractionAPI {
 
     private async runWithProgress<T>(
         sideEffects: SideEffects,
-        runOperation: () => Promise<T> = () => Promise.resolve<any>(null)
+        runOperation: () => Promise<T> = () => Promise.resolve<any>(null),
+        runSideEffects: (arg0: T) => boolean = () => true
     ): Promise<T> {
         if (this.state !== RepositoryState.Idle) {
             throw new Error('Repository not initialized');
@@ -901,10 +903,12 @@ export class Repository implements IDisposable, InteractionAPI {
 
                 try {
                     const operationResult = await runOperation();
-                    await this.updateModelState(
-                        sideEffects,
-                        'Triggered by previous operation' as Reason
-                    );
+                    if (runSideEffects(operationResult)) {
+                        await this.updateModelState(
+                            sideEffects,
+                            'Triggered by previous operation' as Reason
+                        );
+                    }
                     return operationResult;
                 } finally {
                     this._operations.delete(key);
@@ -1083,46 +1087,40 @@ export class Repository implements IDisposable, InteractionAPI {
         clearTimeout(this.autoSyncTimer);
         const nextSyncTime = new Date(Date.now() + interval);
         this.statusBar.onSyncTimeUpdated(nextSyncTime);
-        this.autoSyncTimer = setTimeout(() => this.sync(), interval);
+        this.autoSyncTimer = setTimeout(() => this.periodicSync(), interval);
     }
 
     /**
      * Reads `changes` from `fossil update --dry-run --latest`
      * and updates the status bar
      */
     private async updateChanges(reason: Reason): Promise<void> {
-        const updateOutput = await this.repository.update(
+        const updateResult = await this.repository.update(
             undefined,
             true,
             reason
         );
-        if (updateOutput.exitCode) {
-            this.statusBar.onChangesUpdated(
-                'error' as Changes,
-                updateOutput.stderr
-            );
-        } else {
-            const match = updateOutput.stdout.match(/^changes:\s*(.*)/m);
-            const changes = (match ? match[1] : 'unknown status') as Changes;
-            this.statusBar.onChangesUpdated(changes);
-        }
+        this.statusBar.onChangesReady(updateResult);
     }
 
-    /**
-     * runs `fossil sync`. when 'auto' is set to true, errors are ignored
-     * and exec reason is set to 'periodic sync'.
-     * after that the `changes` are updated and auto sync is rescheduled
-     */
-    async sync(auto?: true): Promise<void> {
-        await this.repository.exec(
-            ['sync'],
-            auto ? ('periodic sync' as Reason) : undefined,
-            { logErrors: !auto }
-        );
+    async periodicSync(): Promise<void> {
+        await this.repository.exec(['sync'], 'periodic sync' as Reason, {
+            logErrors: false,
+        });
         await this.updateChanges('sync happened' as Reason);
         this.updateAutoSyncInterval(typedConfig.autoSyncIntervalMs);
     }
 
+    async sync(): Promise<void> {
+        const res = await this.runWithProgress(
+            { changes: true, syncText: 'Syncing' },
+            async () => this.repository.exec(['sync']),
+            res => !res.exitCode
+        );
+        this.statusBar.onSyncReady(res);
+        this.updateAutoSyncInterval(typedConfig.autoSyncIntervalMs);
+    }
+
     dispose(): void {
         dispose(this.disposables);
     }

src/statusbar.ts

@@ -4,10 +4,13 @@
  *--------------------------------------------------------------------------------------------*/
 
 import type { Command, SourceControl } from 'vscode';
-import { Repository, Changes } from './repository';
+import { Repository } from './repository';
 import { ageFromNow, Old } from './humanise';
 import { localize } from './main';
-import { FossilStdErr } from './fossilExecutable';
+import { ExecResult } from './fossilExecutable';
+import { Distinct } from './openedRepository';
+
+type Changes = Distinct<string, 'changes from `fossil up --dry-run`'>;
 
 /**
  * A bar with 'sync' icon;
@@ -19,39 +22,55 @@ import { FossilStdErr } from './fossilExecutable';
  * - handle case when there's no sync URL
  */
 class SyncBar {
+    private icon: 'sync' | 'warning' = 'sync'; // 'sync-ignored' is nice, but not intuitive
     private text = '';
-    private errorText: FossilStdErr | undefined;
-    private changes: Changes = '' as Changes;
+    private syncMessage: `${string}\n` | '' = '';
+    /**
+     * match for /changes:\s*(string)/
+     * like `17 files modified.` or ` None. Already up-to-date`
+     */
+    private changes: string = ''; //
     private nextSyncTime: Date | undefined; // undefined = no auto syncing
 
     constructor(private repository: Repository) {}
 
-    /**
-     * @param changes like `17 files modified.` or ` None. Already up-to-date`
-     */
-    public onChangesUpdated(changes: Changes, errorText?: FossilStdErr) {
-        const numChanges = changes.match(/\d+/);
-        this.text = numChanges && errorText === undefined ? numChanges[0] : '';
-        this.changes = changes;
-        this.errorText = errorText;
+    public onChangesReady(updateResult: ExecResult) {
+        if (!updateResult.exitCode) {
+            const match = updateResult.stdout.match(/^changes:\s*((\d*).*)/m);
+            this.changes = (match?.[1] ?? 'unknown changes') as Changes;
+            this.text = match?.[2] ?? ''; // digits of nothing
+        } else {
+            this.changes = this.text = '';
+        }
+    }
+
+    public onSyncReady(result: ExecResult) {
+        this.icon = 'sync';
+        if (!result.exitCode) {
+            this.syncMessage = '';
+        } else {
+            if (/^Usage: /.test(result.stderr)) {
+                // likely only local repo
+                this.syncMessage = 'repository with no remote\n';
+            } else {
+                this.icon = 'warning';
+                this.syncMessage = `Sync error: ${result.stderr}\n`;
+            }
+        }
     }
 
     public onSyncTimeUpdated(date: Date | undefined) {
         this.nextSyncTime = date;
     }
 
     public get command(): Command {
-        const message = this.nextSyncTime
+        const timeMessage = this.nextSyncTime
             ? `Next sync ${this.nextSyncTime.toTimeString().split(' ')[0]}`
             : `Auto sync disabled`;
-        const tooltip =
-            this.errorText === undefined
-                ? `${this.changes}\n${message}\nUpdate`
-                : `Error: ${this.errorText}`;
         return {
             command: 'fossil.update',
-            title: `$(sync) ${this.text}`.trim(),
-            tooltip: tooltip,
+            title: `$(${this.icon}) ${this.text}`.trim(),
+            tooltip: `${timeMessage}\n${this.syncMessage}${this.changes}\nUpdate`,
             arguments: [this.repository satisfies Repository],
         };
     }
@@ -104,8 +123,8 @@ export class StatusBarCommands {
         this.update();
     }
 
-    public onChangesUpdated(changes: Changes, errorText?: FossilStdErr) {
-        this.syncBar.onChangesUpdated(changes, errorText);
+    public onChangesReady(updateResult: ExecResult) {
+        this.syncBar.onChangesReady(updateResult);
         this.update();
     }
 
@@ -114,6 +133,11 @@ export class StatusBarCommands {
         this.update();
     }
 
+    public onSyncReady(syncResult: ExecResult) {
+        this.syncBar.onSyncReady(syncResult);
+        this.update();
+    }
+
     /**
      * Should be called whenever commands text/actions/tooltips
      * are updated

src/test/suite/common.ts

@@ -190,7 +190,7 @@ export function fakeFossilStatus(execStub: ExecStub, status: string): ExecStub {
 
 export function fakeFossilBranch(
     execStub: ExecStub,
-    branch: 'refresh'
+    branch: 'refresh' | 'trunk'
 ): ExecStub {
     return execStub
         .withArgs(['branch', 'current'])

src/test/suite/stateSuite.ts

@@ -4,6 +4,7 @@ import {
     assertGroups,
     cleanupFossil,
     fakeExecutionResult,
+    fakeFossilBranch,
     fakeFossilChanges,
     fakeFossilStatus,
     getExecStub,
@@ -173,25 +174,26 @@ export function StatusBarSuite(this: Suite): void {
         assert.deepEqual(branchBar.arguments, [getRepository()]);
         assert.equal(syncBar.command, 'fossil.update');
         assert.equal(syncBar.title, '$(sync)');
+        assert.ok(syncBar.tooltip);
         assert.match(
-            syncBar.tooltip!,
-            /^None. Already up-to-date\nNext sync \d\d:\d\d:\d\d\nUpdate$/
+            syncBar.tooltip,
+            /^Next sync \d\d:\d\d:\d\d\nNone\. Already up-to-date\nUpdate$/
         );
         assert.deepEqual(syncBar.arguments, [getRepository()]);
     });
 
     test('Sync', async () => {
         const execStub = getExecStub(this.ctx.sandbox);
-        const syncCall = execStub.withArgs(['sync']).resolves();
+        const syncCall = execStub
+            .withArgs(['sync'])
+            .resolves(fakeExecutionResult());
         const changesCall = fakeFossilChanges(execStub, '18 files modified.');
         await commands.executeCommand('fossil.sync');
-        sinon.assert.calledOnceWithExactly(syncCall, ['sync'], undefined, {
-            logErrors: true,
-        });
+        sinon.assert.calledOnceWithExactly(syncCall, ['sync']);
         sinon.assert.calledOnceWithExactly(
             changesCall,
             ['update', '--dry-run', '--latest'],
-            'sync happened' as Reason,
+            'Triggered by previous operation' as Reason,
             { logErrors: false }
         );
         const nextSyncString = new Date(N.getTime() + 180 * 1000)
@@ -202,7 +204,68 @@ export function StatusBarSuite(this: Suite): void {
         assert.equal(syncBar.title, '$(sync) 18');
         assert.equal(
             syncBar.tooltip,
-            `18 files modified.\nNext sync ${nextSyncString}\nUpdate`
+            `Next sync ${nextSyncString}\n18 files modified.\nUpdate`
+        );
+    });
+
+    test('Icon spins when sync is in progress', async () => {
+        const execStub = getExecStub(this.ctx.sandbox);
+        const syncStub = execStub.withArgs(['sync']).callsFake(async () => {
+            const syncBar = statusBarCommands()[1];
+            assert.equal(syncBar.title, '$(sync~spin)');
+            return fakeExecutionResult();
+        });
+        const changeStub = fakeFossilChanges(
+            execStub,
+            'None. Already up-to-date'
+        );
+        await commands.executeCommand('fossil.sync');
+        sinon.assert.calledOnce(syncStub);
+        sinon.assert.calledOnce(changeStub);
+        sinon.assert.calledTwice(execStub);
+    });
+
+    test('Icon spins when update is in progress', async () => {
+        const execStub = getExecStub(this.ctx.sandbox);
+        const syncStub = execStub.withArgs(['update']).callsFake(async () => {
+            const syncBar = statusBarCommands()[1];
+            assert.equal(syncBar.title, '$(sync~spin)');
+            return fakeExecutionResult();
+        });
+        const changeStub = fakeFossilChanges(
+            execStub,
+            'None. Already up-to-date'
+        );
+        const statusStub = fakeFossilStatus(execStub, '');
+        const branchStub = fakeFossilBranch(execStub, 'trunk');
+        await commands.executeCommand('fossil.update');
+        sinon.assert.calledOnce(syncStub);
+        sinon.assert.calledOnce(changeStub);
+        sinon.assert.calledOnce(statusStub);
+        sinon.assert.calledOnce(branchStub);
+        sinon.assert.callCount(execStub, 4);
+    });
+
+    test('Error in tooltip when `sync` failed', async () => {
+        const execStub = getExecStub(this.ctx.sandbox);
+        const syncStub = execStub
+            .withArgs(['sync'])
+            .resolves(
+                fakeExecutionResult({ stderr: 'test failure', exitCode: 1 })
+            );
+        const changeStub = fakeFossilChanges(
+            execStub,
+            'None. Already up-to-date'
+        );
+        await commands.executeCommand('fossil.sync');
+        sinon.assert.calledOnce(syncStub);
+        sinon.assert.notCalled(changeStub); // sync failed, nothing changed
+        sinon.assert.calledOnce(execStub);
+        const syncBar = statusBarCommands()[1];
+        assert.ok(syncBar.tooltip);
+        assert.match(
+            syncBar.tooltip,
+            /Next sync \d\d:\d\d:\d\d\nSync error: test failure\nNone\. Already up-to-date\nUpdate/
         );
     });
 }