From 7334ee21fcb09cf6a1b1c3e5f2b6374dbffbad9e Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 16:34:43 -0600 Subject: [PATCH 1/2] fix(cli): exit codes, option handling and Ctrl+C for accounts and dry runs; the log follows GCLO_DATA_DIR - `gclo sync --dry-run` canceled while the repositories were being listed exited 0 with an empty plan. It now says it was canceled, prints no plan, and exits 1. - `gclo accounts`: a failure of the accounts file or of Windows Credential Manager ended with exit code 70 ("please report it") where the help promises 2. It is now a fatal error with its reason (2), or "retry later" (4) when another gclo process was changing the accounts. - `gclo accounts -h` was an unknown subcommand. `--description=` was rejected although `--description ""` works. - `gclo accounts edit` accepted a blank --rename, --org, or --target. - A relative --target was stored as typed, so an account meant a different folder depending on where it was used from. It is stored as a full path. - The CLI's activity log ignored GCLO_DATA_DIR while accounts and settings followed it. The log folder is now "logs" under the data folder for the app and the CLI alike (GcloPaths.LogDirectory). - Ctrl+C: the first one was held back ("Canceling...") also for commands that never look at the request (`accounts`) and while waiting at the token prompt, and then nothing happened. It is held back only where something acts on it (CancelKeyPolicy). Closes #159 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- docs/CLI.md | 21 ++- gclo.Cli.Tests/AccountsCommandTests.cs | 199 +++++++++++++++++++++++++ gclo.Cli.Tests/CancelKeyPolicyTests.cs | 44 ++++++ gclo.Cli.Tests/OptionReaderTests.cs | 34 +++++ gclo.Cli.Tests/SyncCommandTests.cs | 23 +++ gclo.Cli.Tests/TokenOptionsTests.cs | 43 ++++++ gclo.Cli/AccountsCommand.cs | 89 ++++++++++- gclo.Cli/CancelKeyPolicy.cs | 27 ++++ gclo.Cli/OptionReader.cs | 16 +- gclo.Cli/OrgsCommand.cs | 2 +- gclo.Cli/Program.cs | 8 +- gclo.Cli/ReposCommand.cs | 2 +- gclo.Cli/SyncCommand.cs | 28 +++- gclo.Cli/Terminal.cs | 17 +++ gclo.Cli/TokenOptions.cs | 15 +- gclo.Engine.Tests/GcloPathsTests.cs | 20 +++ gclo.ViewModels/GcloPaths.cs | 8 + gclo.ViewModels/WorkspaceViewModel.cs | 2 +- 18 files changed, 571 insertions(+), 27 deletions(-) create mode 100644 gclo.Cli.Tests/CancelKeyPolicyTests.cs create mode 100644 gclo.Cli/CancelKeyPolicy.cs diff --git a/docs/CLI.md b/docs/CLI.md index 400b9f1..b7a339d 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -100,7 +100,10 @@ broken-repo Failed remote authentication failed **Ctrl+C** cancels gracefully: in-flight git operations stop, remaining repositories are marked `Canceled`, and the summary still prints. A second -Ctrl+C aborts the process immediately. +Ctrl+C aborts the process immediately. So does the first one while gclo waits +for the token at its prompt (`--token-stdin` in a terminal), and in +`gclo accounts`, which has nothing to finish. A `--dry-run` that is canceled +while the repositories are being listed prints no plan and exits 1. #### Windows-invalid paths and `--sanitize-paths` @@ -290,7 +293,10 @@ gh auth token | gclo accounts add --name work --org contoso --target C:\src\cont ``` `--name`, `--org`, and `--target` are required; `--parallel` (1-64, default 8), -`--org-subfolder`, and `--description` are optional. The token comes from a +`--org-subfolder`, and `--description` are optional. A relative `--target` is +stored as a full path, resolved against the current directory, because the +account is used later from other places. `--description=` (or `--description ""`) +is an empty description. The token comes from a token option exactly as for `gclo sync` (default: the `GITHUB_TOKEN` environment variable) and is written to Windows Credential Manager — never put it on the command line. A name that another account already uses (case-insensitive) @@ -393,16 +399,17 @@ still wins for that run (the stored token is left untouched). Every invocation appends to a daily activity log file, `gclo-yyyy-MM-dd.log`, under `%LOCALAPPDATA%\KofTwentyTwo\gclo\logs` (on Linux/macOS: the platform's -local application data folder). It records the run's parameters, per-repository +local application data folder), or under `logs` in the folder `GCLO_DATA_DIR` +names when that variable is set, the same folder the desktop app logs to. It records the run's parameters, per-repository failures, and path-sanitization notes — the same log the desktop app shows under **View > Activity log**. Tokens are never written to the log, or anywhere else on disk; see [SECURITY.md](../SECURITY.md). Daily files are kept for 30 days, today included; older ones are removed the -first time something is logged on a given day. When the desktop app and the -CLI log to the same file (they do unless `GCLO_DATA_DIR` moves the app's data), -an entry that finds the file in use is tried again for a moment rather than -dropped; if the file stays in use, entries are dropped until it is free again. +first time something is logged on a given day. The desktop app and the CLI +log to the same file; an entry that finds the file in use is tried again for a +moment rather than dropped, and if the file stays in use, entries are dropped +until it is free again. Each entry is one line that starts with its timestamp: local time, followed by its offset from UTC (`2026-11-10 14:03:22.123 -06:00 [INFO] ...`). Text that came from a repository, such as a file path, is written with control characters shown as `\uXXXX`, and the lines of an exception are indented under their entry, so nothing in a repository can add or imitate an entry. Everything the CLI prints as text shows such characters the same way, and on a console whose code page cannot represent a character (code page 437 or 850, for example) that character is shown in the same form too; the JSON modes use JSON's own escaping. diff --git a/gclo.Cli.Tests/AccountsCommandTests.cs b/gclo.Cli.Tests/AccountsCommandTests.cs index 7b0038d..2c95aec 100644 --- a/gclo.Cli.Tests/AccountsCommandTests.cs +++ b/gclo.Cli.Tests/AccountsCommandTests.cs @@ -567,4 +567,203 @@ public sealed class AccountsCommandTests : IDisposable Assert.Throws(() => AccountsCommand.Run([], failing, new NullLog())); } + + + + // ---------------------------------------------------------------- option handling (#159) + + [Fact] + public void ShortHelp_IsHelp_NotAnUnknownSubcommand() + { + using var console = new ConsoleCapture(); + + int code = AccountsCommand.Run(["-h"], Open, new NullLog()); + + Assert.Equal(0, code); + Assert.Contains("gclo accounts", console.Out); + } + + + + [Fact] + public void Add_AnEmptyDescription_IsAccepted_InEitherForm() + { + using IDisposable _ = TokenEnv("ghp_added"); + using var console = new ConsoleCapture(); + + Assert.Equal(0, AccountsCommand.Run( + ["add", "--name", "one", "--org", "acme", "--target", @"C:\src", "--description=", "--token-env", EnvVar], Open, new NullLog())); + Assert.Equal(0, AccountsCommand.Run( + ["add", "--name", "two", "--org", "acme", "--target", @"C:\src", "--description", "", "--token-env", EnvVar], Open, new NullLog())); + + Assert.All(Store.GetAll(), a => Assert.Equal("", a.Description)); + } + + + + [Fact] + public void Edit_AnEmptyDescription_ClearsIt() + { + Seed("work", "acme", @"C:\src"); + Store.Save(Store.FindByName("work")! with { Description = "day job" }, token: null); + using var console = new ConsoleCapture(); + + int code = AccountsCommand.Run(["edit", "--name", "work", "--description="], Open, new NullLog()); + + Assert.Equal(0, code); + Assert.Equal("", Store.FindByName("work")!.Description); + } + + + + [Theory] + [InlineData("--rename")] + [InlineData("--org")] + [InlineData("--target")] + public void Edit_ABlankNameOrganizationOrFolder_IsAUsageError_AndChangesNothing(string option) + { + Seed("work", "acme", @"C:\src"); + Account before = Store.FindByName("work")!; + + CliUsageException ex = Assert.Throws( + () => AccountsCommand.Run(["edit", "--name", "work", option, " "], Open, new NullLog())); + + Assert.Equal($"{option} expects a value that is not blank.", ex.Message); + Assert.Equal(before, Store.FindByName("work")); + } + + + + [Fact] + public void ARelativeTarget_IsStoredAsAFullPath_ByAddAndByEdit() + { + using IDisposable _ = TokenEnv("ghp_added"); + using var console = new ConsoleCapture(); + + AccountsCommand.Run( + ["add", "--name", "work", "--org", "acme", "--target", "repos", "--token-env", EnvVar], Open, new NullLog()); + string added = Store.FindByName("work")!.TargetRoot; + AccountsCommand.Run(["edit", "--name", "work", "--target", Path.Combine("other", "place")], Open, new NullLog()); + string edited = Store.FindByName("work")!.TargetRoot; + + // The account will be used from other places; "repos" must mean one folder. + Assert.Equal(Path.GetFullPath("repos"), added); + Assert.Equal(Path.GetFullPath(Path.Combine("other", "place")), edited); + Assert.True(Path.IsPathFullyQualified(added)); + } + + + + [Fact] + public void ATargetThatIsNotAPath_IsAUsageError() + { + using IDisposable _ = TokenEnv("ghp_added"); + string notAPath = "bad" + (char)0 + "path"; + + CliUsageException ex = Assert.Throws(() => AccountsCommand.Run( + ["add", "--name", "work", "--org", "acme", "--target", notAPath, "--token-env", EnvVar], Open, new NullLog())); + + Assert.Contains("is not a usable folder path", ex.Message); + Assert.Empty(Store.GetAll()); + } + + + + // ---------------------------------------------------------------- failures of the file or the credential store (#159) + + /// Makes the next write of accounts.json fail: a folder sits where its temporary file goes. + private void BlockAccountsFile() => Directory.CreateDirectory(Path.Combine(_dir, "accounts.json.tmp")); + + + + [Fact] + public void AFailureToWriteTheAccountsFile_IsAFatalError_NotAnUnexpectedOne() + { + using IDisposable _ = TokenEnv("ghp_added"); + Seed("work", "acme", @"C:\src"); + BlockAccountsFile(); + + CliErrorException add = Assert.Throws(() => AccountsCommand.Run( + ["add", "--name", "other", "--org", "acme", "--target", @"C:\src", "--token-env", EnvVar], Open, new NullLog())); + CliErrorException edit = Assert.Throws( + () => AccountsCommand.Run(["edit", "--name", "work", "--org", "elsewhere"], Open, new NullLog())); + CliErrorException remove = Assert.Throws( + () => AccountsCommand.Run(["remove", "--name", "work"], Open, new NullLog())); + + Assert.All(new[] { add, edit, remove }, ex => + { + Assert.Equal(ExitCodes.Fatal, ex.ExitCode); + Assert.IsType(ex.InnerException); + }); + Assert.Equal("acme", Store.FindByName("work")!.Organization); + Assert.Equal("tok-work", _vault.TryRetrieve(Store.FindByName("work")!.Id)); // the removal was undone + } + + + + /// A vault that fails the way Credential Manager does when it cannot be used. + private sealed class BrokenVault : ITokenVault + { + public void Store(Guid accountId, string token) + => throw new System.ComponentModel.Win32Exception(1312, "A specified logon session does not exist."); + + public string? TryRetrieve(Guid accountId) => null; + + public void Delete(Guid accountId) + { + } + } + + + + [Fact] + public void AFailureOfTheCredentialStore_IsAFatalError_WithItsReason() + { + using IDisposable _ = TokenEnv("ghp_added"); + var vault = new BrokenVault(); + var store = new AccountsStore(vault, _dir, new NullLog()); + + CliErrorException ex = Assert.Throws(() => AccountsCommand.Run( + ["add", "--name", "work", "--org", "acme", "--target", @"C:\src", "--token-env", EnvVar], _ => (store, vault), new NullLog())); + + Assert.Equal(ExitCodes.Fatal, ex.ExitCode); + Assert.Contains("logon session", ex.Message); + Assert.Empty(store.GetAll()); + } + + + + [Theory] + [InlineData("list")] + [InlineData("add")] + [InlineData("edit")] + [InlineData("remove")] + public void AnAccountsFileThatAnotherGcloProcessIsChanging_IsRetryLater(string subcommand) + { + using IDisposable _ = TokenEnv("ghp_added"); + Func busy = + _ => throw new AccountsFileBusyException("The accounts file is in use by another gclo process.", new IOException("sharing violation")); + string[] args = subcommand switch + { + "add" => ["add", "--name", "work", "--org", "acme", "--target", @"C:\src", "--token-env", EnvVar], + "edit" => ["edit", "--name", "work", "--org", "elsewhere"], + "remove" => ["remove", "--name", "work"], + _ => ["list"], + }; + + CliErrorException ex = Assert.Throws(() => AccountsCommand.Run(args, busy, new NullLog())); + + Assert.Equal(ExitCodes.Transient, ex.ExitCode); + Assert.Contains("in use by another gclo process", ex.Message); + } + + + + [Fact] + public void AFailureThatIsNeitherTheFileNorTheStore_StaysWhatItIs() + { + Func bug = _ => throw new InvalidOperationException("a bug"); + + Assert.Throws(() => AccountsCommand.Run(["list"], bug, new NullLog())); + } } diff --git a/gclo.Cli.Tests/CancelKeyPolicyTests.cs b/gclo.Cli.Tests/CancelKeyPolicyTests.cs new file mode 100644 index 0000000..ee16dd2 --- /dev/null +++ b/gclo.Cli.Tests/CancelKeyPolicyTests.cs @@ -0,0 +1,44 @@ +/* + * Copyright (c) 2026 James Maes (KofTwentyTwo) + * SPDX-License-Identifier: MIT + */ + +namespace gclo.Cli.Tests; + + +/// +/// The first Ctrl+C is held back only where something will act on it: a command +/// that watches for cancellation, not waiting at a prompt, not already asked (#159). +/// +public sealed class CancelKeyPolicyTests +{ + [Theory] + [InlineData("sync")] + [InlineData("repos")] + [InlineData("orgs")] + public void ACommandThatWatchesForCancellation_FinishesGracefully(string command) + => Assert.True(CancelKeyPolicy.FinishesGracefully(command, isReadingInput: false, alreadyRequested: false)); + + + + [Theory] + [InlineData("accounts")] + [InlineData("help")] + [InlineData("--version")] + [InlineData("no-such-command")] + [InlineData(null)] + public void ACommandThatNeverLooks_EndsAtOnce(string? command) + => Assert.False(CancelKeyPolicy.FinishesGracefully(command, isReadingInput: false, alreadyRequested: false)); + + + + [Fact] + public void WhileWaitingForALineOfInput_TheProcessEndsAtOnce() + => Assert.False(CancelKeyPolicy.FinishesGracefully("sync", isReadingInput: true, alreadyRequested: false)); + + + + [Fact] + public void ASecondCtrlC_EndsTheProcess() + => Assert.False(CancelKeyPolicy.FinishesGracefully("sync", isReadingInput: false, alreadyRequested: true)); +} diff --git a/gclo.Cli.Tests/OptionReaderTests.cs b/gclo.Cli.Tests/OptionReaderTests.cs index 43253b3..d108e67 100644 --- a/gclo.Cli.Tests/OptionReaderTests.cs +++ b/gclo.Cli.Tests/OptionReaderTests.cs @@ -105,4 +105,38 @@ public sealed class OptionReaderTests reader.RejectValue(); // must not throw } + + + + // ---------------------------------------------------------------- a value that may be empty (#159) + + [Fact] + public void RequireValueOrEmpty_AcceptsAnEmptyInlineValue_AndAnEmptyArgument() + { + var inline = new OptionReader(["--note="]); + Assert.True(inline.MoveNext()); + Assert.Equal("", inline.RequireValueOrEmpty()); + + var separate = new OptionReader(["--note", ""]); + Assert.True(separate.MoveNext()); + Assert.Equal("", separate.RequireValueOrEmpty()); + + var given = new OptionReader(["--note=text"]); + Assert.True(given.MoveNext()); + Assert.Equal("text", given.RequireValueOrEmpty()); + } + + + + [Fact] + public void RequireValueOrEmpty_StillNeedsAValue_AndRequireValueStillRefusesAnEmptyOne() + { + var missing = new OptionReader(["--note"]); + Assert.True(missing.MoveNext()); + Assert.Throws(() => missing.RequireValueOrEmpty()); + + var empty = new OptionReader(["--name="]); + Assert.True(empty.MoveNext()); + Assert.Throws(() => empty.RequireValue()); + } } diff --git a/gclo.Cli.Tests/SyncCommandTests.cs b/gclo.Cli.Tests/SyncCommandTests.cs index 695230f..4d252dd 100644 --- a/gclo.Cli.Tests/SyncCommandTests.cs +++ b/gclo.Cli.Tests/SyncCommandTests.cs @@ -754,4 +754,27 @@ public sealed class SyncCommandTests : IDisposable Assert.Equal(1, code); Assert.Contains("and 5 more invalid paths", console.Error); } + + + + // ---------------------------------------------------------------- a dry run that was canceled (#159) + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task DryRun_CanceledWhileListing_SaysSo_AndDoesNotPrintAnEmptyPlan(bool json) + { + using var console = new ConsoleCapture(); + using var cts = new CancellationTokenSource(); + cts.Cancel(); + string[] args = json + ? ["--org", "acme", "--target", Target, "--token-env", EnvVar, "--dry-run", "--json"] + : ["--org", "acme", "--target", Target, "--token-env", EnvVar, "--dry-run"]; + + int code = await SyncCommand.RunAsync(args, new FakeRepoLister { Result = [Repo("a")] }, new FakeGit(), Open, new NullLog(), cts.Token); + + Assert.Equal(ExitCodes.Partial, code); + Assert.Contains("Canceled.", console.Error); + Assert.Equal("", console.Out); // no plan, and no document that says "nothing to do" + } } diff --git a/gclo.Cli.Tests/TokenOptionsTests.cs b/gclo.Cli.Tests/TokenOptionsTests.cs index abaffe8..7b293cb 100644 --- a/gclo.Cli.Tests/TokenOptionsTests.cs +++ b/gclo.Cli.Tests/TokenOptionsTests.cs @@ -209,4 +209,47 @@ public sealed class TokenOptionsTests Assert.Throws(() => new TokenOptions().TryConsume(reader)); } + + + + // ---------------------------------------------------------------- waiting for a line (#159) + + /// Standard input that notes, when it is read, whether the process says it is waiting for input. + private sealed class WatchfulInput(string line) : TextReader + { + public bool? WasReadingInput { get; private set; } + + public override string? ReadLine() + { + WasReadingInput = Terminal.IsReadingInput; + return line; + } + } + + + + [Fact] + public void WhileTheTokenIsReadFromStandardInput_TheProcessSaysItIsWaitingForInput_AndNotAfterwards() + { + TextReader original = Console.In; + var input = new WatchfulInput("ghp_from_stdin"); + Console.SetIn(input); + try + { + var options = new TokenOptions(); + var reader = new OptionReader(["--token-stdin"]); + Assert.True(reader.MoveNext()); + Assert.True(options.TryConsume(reader)); + + string token = options.Resolve(); + + Assert.Equal("ghp_from_stdin", token); + Assert.True(input.WasReadingInput); + Assert.False(Terminal.IsReadingInput); + } + finally + { + Console.SetIn(original); + } + } } diff --git a/gclo.Cli/AccountsCommand.cs b/gclo.Cli/AccountsCommand.cs index a7de39e..370df51 100644 --- a/gclo.Cli/AccountsCommand.cs +++ b/gclo.Cli/AccountsCommand.cs @@ -85,7 +85,7 @@ internal static class AccountsCommand /// Composition root: wires the real Credential Manager store and file log. [System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage( Justification = "Wires the real Credential Manager and file log; delegates to the covered core.")] - public static int Run(string[] args) => Run(args, Open, new FileActivityLog()); + public static int Run(string[] args) => Run(args, Open, new FileActivityLog(gclo.ViewModels.GcloPaths.LogDirectory)); @@ -93,8 +93,9 @@ internal static class AccountsCommand { // The first argument names the subcommand unless it is an option, in which // case it is 'list' (the original, option-only form of the command). - string subcommand = args.Length > 0 && !args[0].StartsWith("--", StringComparison.Ordinal) ? args[0] : "list"; - string[] rest = string.Equals(subcommand, "list", StringComparison.Ordinal) && (args.Length == 0 || args[0].StartsWith("--", StringComparison.Ordinal)) + // ('-h' is an option too: 'gclo accounts -h' asks for help, it names no subcommand.) + string subcommand = args.Length > 0 && !args[0].StartsWith('-') ? args[0] : "list"; + string[] rest = string.Equals(subcommand, "list", StringComparison.Ordinal) && (args.Length == 0 || args[0].StartsWith('-')) ? args : args[1..]; @@ -171,6 +172,11 @@ internal static class AccountsCommand } return ExitCodes.Success; } + catch(Exception ex) when(IsStorageFailure(ex)) + { + log.Error($"accounts failed: {ex.Message}", ex); + throw StorageFailure(ex); + } catch(Exception ex) { // Fatal path: unusable accounts store or non-Windows platform. @@ -216,7 +222,7 @@ internal static class AccountsCommand target = reader.RequireValue(); break; case "--description": - description = reader.RequireValue(); + description = reader.RequireValueOrEmpty(); break; case "--parallel": parallel = OptionReader.ParseParallel(reader.RequireValue()); @@ -257,7 +263,7 @@ internal static class AccountsCommand Name = name.Trim(), Description = description.Trim(), Organization = org.Trim(), - TargetRoot = target.Trim(), + TargetRoot = AbsoluteTarget(target), CreateOrgSubfolder = orgSubfolder, MaxConcurrency = parallel, TokenSource = tokenOptions.UseDefault ? TokenSource.Default : TokenSource.Own, @@ -266,6 +272,11 @@ internal static class AccountsCommand Terminal.Out($"Account '{account.Name}' added" + (tokenOptions.UseDefault ? " (uses the default token)." : ".")); return ExitCodes.Success; } + catch(Exception ex) when(IsStorageFailure(ex)) + { + log.Error($"accounts add failed: {ex.Message}", ex); + throw StorageFailure(ex); + } catch(Exception ex) { log.Error($"accounts add failed: {ex.Message}", ex); @@ -313,7 +324,7 @@ internal static class AccountsCommand target = reader.RequireValue(); break; case "--description": - description = reader.RequireValue(); + description = reader.RequireValueOrEmpty(); break; case "--parallel": parallel = OptionReader.ParseParallel(reader.RequireValue()); @@ -342,6 +353,10 @@ internal static class AccountsCommand + "--description, --org-subfolder/--no-org-subfolder, or a token option."); } + RequireNotBlank(rename, "--rename"); + RequireNotBlank(org, "--org"); + RequireNotBlank(target, "--target"); + log.Info($"accounts edit started: name='{name.Trim()}'"); try { @@ -360,7 +375,7 @@ internal static class AccountsCommand { Name = rename?.Trim() ?? existing.Name, Organization = org?.Trim() ?? existing.Organization, - TargetRoot = target?.Trim() ?? existing.TargetRoot, + TargetRoot = target is null ? existing.TargetRoot : AbsoluteTarget(target), Description = description?.Trim() ?? existing.Description, MaxConcurrency = parallel ?? existing.MaxConcurrency, CreateOrgSubfolder = orgSubfolder ?? existing.CreateOrgSubfolder, @@ -373,6 +388,11 @@ internal static class AccountsCommand Terminal.Out($"Account '{updated.Name}' updated" + tokenNote); return ExitCodes.Success; } + catch(Exception ex) when(IsStorageFailure(ex)) + { + log.Error($"accounts edit failed: {ex.Message}", ex); + throw StorageFailure(ex); + } catch(Exception ex) { log.Error($"accounts edit failed: {ex.Message}", ex); @@ -418,6 +438,11 @@ internal static class AccountsCommand Terminal.Out($"Account '{existing.Name}' removed."); return ExitCodes.Success; } + catch(Exception ex) when(IsStorageFailure(ex)) + { + log.Error($"accounts remove failed: {ex.Message}", ex); + throw StorageFailure(ex); + } catch(Exception ex) { log.Error($"accounts remove failed: {ex.Message}", ex); @@ -453,6 +478,56 @@ internal static class AccountsCommand + /// + /// True for a failure of the accounts file or of the credential store: not a bug + /// in gclo and not a usage mistake, but something on this machine that the + /// message names (a file that cannot be written, an entry that cannot be read). + /// + private static bool IsStorageFailure(Exception ex) + => ex is IOException or UnauthorizedAccessException or AccountConsistencyException or System.ComponentModel.Win32Exception; + + + + /// + /// The error a storage failure ends the command with: fatal (2), as the help + /// says, or "retry later" (4) when another gclo process was changing the + /// accounts at that moment. + /// + private static CliErrorException StorageFailure(Exception ex) + => new(ex.Message, ex, ex is AccountsFileBusyException ? ExitCodes.Transient : ExitCodes.Fatal); + + + + /// A value that was given must say something: a blank name, organization, or folder is a mistake. + private static void RequireNotBlank(string? value, string option) + { + if(value is not null && string.IsNullOrWhiteSpace(value)) + { + throw new CliUsageException($"{option} expects a value that is not blank."); + } + } + + + + /// + /// The target folder as a full path. An account is used later from other places + /// (the desktop app, a scheduled task, another working directory); a relative + /// folder would mean a different one each time, so it is resolved once, here. + /// + private static string AbsoluteTarget(string target) + { + try + { + return Path.GetFullPath(target.Trim()); + } + catch(Exception ex) when(ex is ArgumentException or NotSupportedException or PathTooLongException) + { + throw new CliUsageException($"--target '{target}' is not a usable folder path: {ex.Message}"); + } + } + + + /// A duplicate name is the one validation the store itself enforces; everything else is fatal as is. private static void SaveOrFail(AccountsStore store, Account account, string? token) { diff --git a/gclo.Cli/CancelKeyPolicy.cs b/gclo.Cli/CancelKeyPolicy.cs new file mode 100644 index 0000000..9225eb4 --- /dev/null +++ b/gclo.Cli/CancelKeyPolicy.cs @@ -0,0 +1,27 @@ +/* + * Copyright (c) 2026 James Maes (KofTwentyTwo) + * SPDX-License-Identifier: MIT + */ + +namespace gclo.Cli; + + +/// +/// What the first Ctrl+C does. A command that watches for cancellation finishes +/// gracefully (in-flight work stops, the summary still prints). Everything else +/// ends at once: holding the key press back for a command that never looks, or +/// while the process sits in a prompt waiting for a line, would print +/// "Canceling..." and then carry on as if nothing was pressed. +/// +internal static class CancelKeyPolicy +{ + /// + /// True when the key press is to be held back and turned into a cancellation + /// request; false when the process is to end the way Ctrl+C ends any process. + /// + /// The command being run (the first argument), or null when there is none. + /// Whether the process is waiting for a line on standard input right now. + /// Whether an earlier Ctrl+C was already turned into a request. + public static bool FinishesGracefully(string? command, bool isReadingInput, bool alreadyRequested) + => !alreadyRequested && !isReadingInput && command is "sync" or "repos" or "orgs"; +} diff --git a/gclo.Cli/OptionReader.cs b/gclo.Cli/OptionReader.cs index 215a0a1..2b19401 100644 --- a/gclo.Cli/OptionReader.cs +++ b/gclo.Cli/OptionReader.cs @@ -58,11 +58,23 @@ internal sealed class OptionReader /// argument. A following token that itself starts with '--' is treated as a /// missing value (use '--name=--literal' to pass such a value literally). /// - public string RequireValue() + public string RequireValue() => RequireValue(mayBeEmpty: false); + + + + /// + /// The value for , which may be empty: '--note=' means the + /// same as '--note ""', an empty text. + /// + public string RequireValueOrEmpty() => RequireValue(mayBeEmpty: true); + + + + private string RequireValue(bool mayBeEmpty) { if(_inlineValue is not null) { - if(_inlineValue.Length == 0) + if(_inlineValue.Length == 0 && !mayBeEmpty) { throw new CliUsageException($"Option '{Current}' requires a value."); } diff --git a/gclo.Cli/OrgsCommand.cs b/gclo.Cli/OrgsCommand.cs index cec1117..532cba7 100644 --- a/gclo.Cli/OrgsCommand.cs +++ b/gclo.Cli/OrgsCommand.cs @@ -48,7 +48,7 @@ internal static class OrgsCommand [System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage( Justification = "Wires the real network lister and file log; delegates to the covered core.")] public static Task RunAsync(string[] args, CancellationToken cancellationToken) - => RunAsync(args, new GitHubOrganizationLister(), new FileActivityLog(), cancellationToken); + => RunAsync(args, new GitHubOrganizationLister(), new FileActivityLog(gclo.ViewModels.GcloPaths.LogDirectory), cancellationToken); diff --git a/gclo.Cli/Program.cs b/gclo.Cli/Program.cs index 1c043b8..f8596d5 100644 --- a/gclo.Cli/Program.cs +++ b/gclo.Cli/Program.cs @@ -72,7 +72,7 @@ internal static class Program using var cts = new CancellationTokenSource(); Console.CancelKeyPress += (_, e) => { - if(!cts.IsCancellationRequested) + if(CancelKeyPolicy.FinishesGracefully(args.Length > 0 ? args[0] : null, Terminal.IsReadingInput, cts.IsCancellationRequested)) { // Finish gracefully: in-flight git operations stop, remaining // repositories are marked Canceled, and the summary still prints. @@ -80,7 +80,9 @@ internal static class Program cts.Cancel(); Terminal.Error("Canceling... (press Ctrl+C again to abort immediately)"); } - // Second Ctrl+C: e.Cancel stays false and the process terminates. + // Otherwise (a second Ctrl+C, a command that does not watch for + // cancellation, a prompt waiting for a line): e.Cancel stays false and + // the process terminates. }; try @@ -127,7 +129,7 @@ internal static class Program string details = ""; try { - var log = new FileActivityLog(); + var log = new FileActivityLog(gclo.ViewModels.GcloPaths.LogDirectory); log.Error($"unexpected error ({ex.GetType().FullName}): {ex.Message}", ex); details = log.CurrentLogFilePath; } diff --git a/gclo.Cli/ReposCommand.cs b/gclo.Cli/ReposCommand.cs index df91851..18ca467 100644 --- a/gclo.Cli/ReposCommand.cs +++ b/gclo.Cli/ReposCommand.cs @@ -51,7 +51,7 @@ internal static class ReposCommand [System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage( Justification = "Wires the real network lister and file log; delegates to the covered core.")] public static Task RunAsync(string[] args, CancellationToken cancellationToken) - => RunAsync(args, new GitHubRepositoryLister(), new FileActivityLog(), cancellationToken); + => RunAsync(args, new GitHubRepositoryLister(), new FileActivityLog(gclo.ViewModels.GcloPaths.LogDirectory), cancellationToken); diff --git a/gclo.Cli/SyncCommand.cs b/gclo.Cli/SyncCommand.cs index eca1bde..2f8675f 100644 --- a/gclo.Cli/SyncCommand.cs +++ b/gclo.Cli/SyncCommand.cs @@ -256,7 +256,7 @@ internal static class SyncCommand public static Task RunAsync(string[] args, CancellationToken cancellationToken) => RunAsync( args, - new GitHubRepositoryLister(), new LibGit2GitClient(), AccountsCommand.Open, new FileActivityLog(), + new GitHubRepositoryLister(), new LibGit2GitClient(), AccountsCommand.Open, new FileActivityLog(gclo.ViewModels.GcloPaths.LogDirectory), cancellationToken); @@ -353,7 +353,7 @@ internal static class SyncCommand if(dryRun) { - return PrintDryRun(repositories, git, targetRoot, machineReadable, log); + return EndDryRun(canceledWhileListing, repositories, git, targetRoot, machineReadable, log); } OutputMode mode = jsonLines ? OutputMode.JsonLines @@ -425,6 +425,30 @@ internal static class SyncCommand + /// + /// What a dry run ends with: the plan, or, when the listing was canceled, the + /// fact that it was. Nothing was listed then, and an empty plan with exit code 0 + /// would say the organization has no repositories. + /// + private static int EndDryRun( + bool canceledWhileListing, + IReadOnlyList repositories, + IGitClient git, + string targetRoot, + bool machineReadable, + IActivityLog log) + { + if(canceledWhileListing) + { + Terminal.Error("Canceled."); + log.Info("sync --dry-run: canceled while listing repositories."); + return ExitCodes.Partial; + } + return PrintDryRun(repositories, git, targetRoot, machineReadable, log); + } + + + /// /// '--dry-run': says what the run would do with each selected repository and /// touches nothing. Exit 0 even for an empty selection. diff --git a/gclo.Cli/Terminal.cs b/gclo.Cli/Terminal.cs index 41ae5be..b5761c2 100644 --- a/gclo.Cli/Terminal.cs +++ b/gclo.Cli/Terminal.cs @@ -29,6 +29,10 @@ namespace gclo.Cli; /// internal static class Terminal { + private static bool s_isReadingInput; + + + /// Writes one line of run-time text to standard output, made safe for the terminal. public static void Out(string text) => Console.Out.WriteLine(Safe(text, Console.Out.Encoding)); @@ -52,6 +56,19 @@ internal static class Terminal + /// + /// True while the process waits for a line on standard input (the token prompt). + /// A Ctrl+C then ends the process: there is nothing to finish gracefully, and + /// the read would otherwise go on waiting. + /// + public static bool IsReadingInput + { + get => Volatile.Read(ref s_isReadingInput); + set => Volatile.Write(ref s_isReadingInput, value); + } + + + /// Ends the current line on standard error. public static void ErrorLineBreak() => Console.Error.WriteLine(); diff --git a/gclo.Cli/TokenOptions.cs b/gclo.Cli/TokenOptions.cs index 0bc97de..6d17c65 100644 --- a/gclo.Cli/TokenOptions.cs +++ b/gclo.Cli/TokenOptions.cs @@ -153,9 +153,18 @@ internal sealed class TokenOptions private static string FromStdin() { - string? line = Console.IsInputRedirected - ? Console.In.ReadLine()?.Trim() - : ReadInteractiveToken(); + string? line; + Terminal.IsReadingInput = true; + try + { + line = Console.IsInputRedirected + ? Console.In.ReadLine()?.Trim() + : ReadInteractiveToken(); + } + finally + { + Terminal.IsReadingInput = false; + } if(string.IsNullOrEmpty(line)) { diff --git a/gclo.Engine.Tests/GcloPathsTests.cs b/gclo.Engine.Tests/GcloPathsTests.cs index 1bf500f..a9af527 100644 --- a/gclo.Engine.Tests/GcloPathsTests.cs +++ b/gclo.Engine.Tests/GcloPathsTests.cs @@ -57,4 +57,24 @@ public sealed class GcloPathsTests Assert.False( GcloPaths.DefaultDataRoot.StartsWith(installRoot + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase)); } + + + + /// The log folder follows the data folder, so the app and the CLI log side by side wherever that is (#159). + [Fact] + public void LogDirectory_IsLogsUnderTheDataRoot_AlsoWhenTheDataRootIsMoved() + { + string moved = Path.Combine(Path.GetTempPath(), "gclo-tests", Guid.NewGuid().ToString("N")); + Environment.SetEnvironmentVariable("GCLO_DATA_DIR", moved); + try + { + Assert.Equal(Path.Combine(moved, "logs"), GcloPaths.LogDirectory); + } + finally + { + Environment.SetEnvironmentVariable("GCLO_DATA_DIR", null); + } + + Assert.Equal(Path.Combine(GcloPaths.DefaultDataRoot, "logs"), GcloPaths.LogDirectory); + } } diff --git a/gclo.ViewModels/GcloPaths.cs b/gclo.ViewModels/GcloPaths.cs index 77404a8..aa8c19d 100644 --- a/gclo.ViewModels/GcloPaths.cs +++ b/gclo.ViewModels/GcloPaths.cs @@ -33,6 +33,14 @@ public static class GcloPaths + /// + /// Where the activity log is written: "logs" under , for the + /// desktop app and the CLI alike, so both follow GCLO_DATA_DIR together. + /// + public static string LogDirectory => Path.Combine(DataRoot, "logs"); + + + /// /// The installed edition: "" for the stable app (Velopack app id gclo) and /// "dev" for the dev-channel app (gclo-dev), which installs beside the diff --git a/gclo.ViewModels/WorkspaceViewModel.cs b/gclo.ViewModels/WorkspaceViewModel.cs index 278b793..e3838a4 100644 --- a/gclo.ViewModels/WorkspaceViewModel.cs +++ b/gclo.ViewModels/WorkspaceViewModel.cs @@ -118,7 +118,7 @@ public sealed partial class WorkspaceViewModel : ObservableObject, IDisposable _orgLister = orgLister ?? new GitHubOrganizationLister(); _progressFactory = progressFactory ?? (handler => new Progress(handler)); _orgLookupDebounce = orgLookupDebounce ?? TimeSpan.FromMilliseconds(600); - _log = log ?? new FileActivityLog(); + _log = log ?? new FileActivityLog(GcloPaths.LogDirectory); _account = account; _accountsStore = accountsStore; Organization = ""; -- 2.54.0 From 7d55b249061e3275bd5938e760b000bd35025ef7 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 16:53:00 -0600 Subject: [PATCH 2/2] fix(cli): address the independent review of the command-line fixes An edit of an account removed meanwhile, and credential-store or accounts file failures in 'sync --account', are fatal errors with their reason, not unexpected ones. A change to the accounts is let through to its end on Ctrl+C. The target folder is checked before the token is read, and what cannot be a folder is refused. A credential-store failure says what its number means. Help and docs match. Refs #159 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- docs/CLI.md | 22 ++++-- gclo.Cli.Tests/AccountsCommandTests.cs | 101 +++++++++++++++++++++++-- gclo.Cli.Tests/CancelKeyPolicyTests.cs | 45 +++++++---- gclo.Cli.Tests/SyncCommandTests.cs | 54 +++++++++++++ gclo.Cli/AccountsCommand.cs | 93 +++++++++++++---------- gclo.Cli/CancelKeyPolicy.cs | 31 +++++--- gclo.Cli/CliErrorException.cs | 28 +++++++ gclo.Cli/Program.cs | 13 ++-- gclo.Cli/SyncCommand.cs | 38 +++++++++- 9 files changed, 340 insertions(+), 85 deletions(-) diff --git a/docs/CLI.md b/docs/CLI.md index b7a339d..7f8639f 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -101,9 +101,12 @@ broken-repo Failed remote authentication failed **Ctrl+C** cancels gracefully: in-flight git operations stop, remaining repositories are marked `Canceled`, and the summary still prints. A second Ctrl+C aborts the process immediately. So does the first one while gclo waits -for the token at its prompt (`--token-stdin` in a terminal), and in -`gclo accounts`, which has nothing to finish. A `--dry-run` that is canceled -while the repositories are being listed prints no plan and exits 1. +for the token at its prompt (`--token-stdin` in a terminal), where nothing has +been started yet. In `gclo accounts`, the first Ctrl+C lets a change that is +under way finish (it writes to the credential store and to the accounts file, +and is not cut off between the two). A `--dry-run` that is canceled while the +repositories are being listed prints no plan, and with `--json` or +`--json-lines` no document, and exits 1. #### Windows-invalid paths and `--sanitize-paths` @@ -295,8 +298,10 @@ gh auth token | gclo accounts add --name work --org contoso --target C:\src\cont `--name`, `--org`, and `--target` are required; `--parallel` (1-64, default 8), `--org-subfolder`, and `--description` are optional. A relative `--target` is stored as a full path, resolved against the current directory, because the -account is used later from other places. `--description=` (or `--description ""`) -is an empty description. The token comes from a +account is used later from other places; a target that cannot be a folder +(a character no folder name can have, a device name, an unexpanded `~` or +`%NAME%`) is refused. `--description=` (or `--description ""`) is an empty +description. The token comes from a token option exactly as for `gclo sync` (default: the `GITHUB_TOKEN` environment variable) and is written to Windows Credential Manager — never put it on the command line. A name that another account already uses (case-insensitive) @@ -407,9 +412,10 @@ else on disk; see [SECURITY.md](../SECURITY.md). Daily files are kept for 30 days, today included; older ones are removed the first time something is logged on a given day. The desktop app and the CLI -log to the same file; an entry that finds the file in use is tried again for a -moment rather than dropped, and if the file stays in use, entries are dropped -until it is free again. +log to the same file (the stable desktop app, that is; a development build +of the app keeps its own data folder); an entry that finds the file in use is +tried again for a moment rather than dropped, and if the file stays in use, +entries are dropped until it is free again. Each entry is one line that starts with its timestamp: local time, followed by its offset from UTC (`2026-11-10 14:03:22.123 -06:00 [INFO] ...`). Text that came from a repository, such as a file path, is written with control characters shown as `\uXXXX`, and the lines of an exception are indented under their entry, so nothing in a repository can add or imitate an entry. Everything the CLI prints as text shows such characters the same way, and on a console whose code page cannot represent a character (code page 437 or 850, for example) that character is shown in the same form too; the JSON modes use JSON's own escaping. diff --git a/gclo.Cli.Tests/AccountsCommandTests.cs b/gclo.Cli.Tests/AccountsCommandTests.cs index 2c95aec..23adf18 100644 --- a/gclo.Cli.Tests/AccountsCommandTests.cs +++ b/gclo.Cli.Tests/AccountsCommandTests.cs @@ -654,14 +654,48 @@ public sealed class AccountsCommandTests : IDisposable + [Theory] + [InlineData("a|b", "a character no folder name can have")] + [InlineData("a", "a character no folder name can have")] + [InlineData("repos*", "a character no folder name can have")] + [InlineData("~", "are not expanded by gclo")] + [InlineData(@"~\src", "are not expanded by gclo")] + [InlineData(@"%USERPROFILE%\src", "are not expanded by gclo")] + [InlineData("repos:x", "a colon belongs after a drive letter only")] + [InlineData(@"C:\src:stream", "a colon belongs after a drive letter only")] + [InlineData("con", "it names a device, not a folder")] + public void ATargetThatCannotBeAFolder_IsAUsageError_BeforeTheTokenIsEvenRead(string target, string reason) + { + if(!OperatingSystem.IsWindows() && string.Equals(target, "con", StringComparison.Ordinal)) + { + return; // device names are a Windows notion + } + using var stdin = new StdinRedirect("ghp_not_to_be_read\n"); + + CliUsageException add = Assert.Throws(() => AccountsCommand.Run( + ["add", "--name", "work", "--org", "acme", "--target", target, "--token-stdin"], Open, new NullLog())); + Seed("existing", "acme", @"C:\src"); + CliUsageException edit = Assert.Throws(() => AccountsCommand.Run( + ["edit", "--name", "existing", "--target", target, "--token-stdin"], Open, new NullLog())); + + Assert.Contains("is not a usable folder path", add.Message); + Assert.Contains(reason, add.Message); + Assert.Contains(reason, edit.Message); + Assert.Null(Store.FindByName("work")); + Assert.Equal(@"C:\src", Store.FindByName("existing")!.TargetRoot); + Assert.Equal("ghp_not_to_be_read", Console.In.ReadLine()); // the token line is still there: nothing read it + } + + + [Fact] - public void ATargetThatIsNotAPath_IsAUsageError() + public void ATargetLongerThanAnyPathCanBe_IsAUsageError() { using IDisposable _ = TokenEnv("ghp_added"); - string notAPath = "bad" + (char)0 + "path"; + string tooLong = new('a', 40_000); CliUsageException ex = Assert.Throws(() => AccountsCommand.Run( - ["add", "--name", "work", "--org", "acme", "--target", notAPath, "--token-env", EnvVar], Open, new NullLog())); + ["add", "--name", "work", "--org", "acme", "--target", tooLong, "--token-env", EnvVar], Open, new NullLog())); Assert.Contains("is not a usable folder path", ex.Message); Assert.Empty(Store.GetAll()); @@ -669,6 +703,23 @@ public sealed class AccountsCommandTests : IDisposable + [Fact] + public void ATargetWithADriveLetter_OrAFullPath_IsKeptAsItIs() + { + if(!OperatingSystem.IsWindows()) + { + return; // drive letters + } + using IDisposable _ = TokenEnv("ghp_added"); + using var console = new ConsoleCapture(); + + AccountsCommand.Run(["add", "--name", "work", "--org", "acme", "--target", @"D:\mirror\acme", "--token-env", EnvVar], Open, new NullLog()); + + Assert.Equal(@"D:\mirror\acme", Store.FindByName("work")!.TargetRoot); + } + + + // ---------------------------------------------------------------- failures of the file or the credential store (#159) /// Makes the next write of accounts.json fail: a folder sits where its temporary file goes. @@ -704,8 +755,9 @@ public sealed class AccountsCommandTests : IDisposable /// A vault that fails the way Credential Manager does when it cannot be used. private sealed class BrokenVault : ITokenVault { + // What CredentialManagerVault throws: the call that failed and the number, no words for it. public void Store(Guid accountId, string token) - => throw new System.ComponentModel.Win32Exception(1312, "A specified logon session does not exist."); + => throw new System.ComponentModel.Win32Exception(1312, "CredWriteW failed (error 1312) for target 'gclo:account:test'."); public string? TryRetrieve(Guid accountId) => null; @@ -727,12 +779,51 @@ public sealed class AccountsCommandTests : IDisposable ["add", "--name", "work", "--org", "acme", "--target", @"C:\src", "--token-env", EnvVar], _ => (store, vault), new NullLog())); Assert.Equal(ExitCodes.Fatal, ex.ExitCode); - Assert.Contains("logon session", ex.Message); + Assert.StartsWith("CredWriteW failed (error 1312)", ex.Message); + if(OperatingSystem.IsWindows()) + { + // The number alone says nothing; what Windows means by it is added. + Assert.Contains("logon session", ex.Message); + } Assert.Empty(store.GetAll()); } + [Fact] + public void AnEdit_OfAnAccountAnotherGcloProcessRemovedMeanwhile_IsAFatalError_NotAnUnexpectedOne() + { + Seed("work", "acme", @"C:\src"); + Account work = Store.FindByName("work")!; + // The store this command works with still lists the account; the file no longer does. + new AccountsStore(_vault, _dir, new NullLog()).Delete(work.Id); + + CliErrorException ex = Assert.Throws( + () => AccountsCommand.Run(["edit", "--name", "work", "--org", "elsewhere"], Open, new NullLog())); + + Assert.Equal(ExitCodes.Fatal, ex.ExitCode); + Assert.Contains("was removed by another gclo process", ex.Message); + } + + + + [Fact] + public void AChange_WhileAnotherGcloProcessHoldsTheAccountsLock_IsRetryLater_AfterTheStoresOwnWait() + { + // The real path, once: the store waits its few seconds for the lock, then says so. + Seed("work", "acme", @"C:\src"); + using var held = new FileStream(Path.Combine(_dir, "accounts.json.lock"), FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None); + + CliErrorException ex = Assert.Throws( + () => AccountsCommand.Run(["remove", "--name", "work"], Open, new NullLog())); + + Assert.Equal(ExitCodes.Transient, ex.ExitCode); + Assert.Contains("in use by another gclo process", ex.Message); + Assert.NotNull(Store.FindByName("work")); + } + + + [Theory] [InlineData("list")] [InlineData("add")] diff --git a/gclo.Cli.Tests/CancelKeyPolicyTests.cs b/gclo.Cli.Tests/CancelKeyPolicyTests.cs index ee16dd2..52e429f 100644 --- a/gclo.Cli.Tests/CancelKeyPolicyTests.cs +++ b/gclo.Cli.Tests/CancelKeyPolicyTests.cs @@ -7,8 +7,9 @@ namespace gclo.Cli.Tests; /// -/// The first Ctrl+C is held back only where something will act on it: a command -/// that watches for cancellation, not waiting at a prompt, not already asked (#159). +/// The first Ctrl+C is held back for the commands that have something to finish +/// (a run that stops gracefully, a change to the accounts that must not be cut off +/// between its two stores), not while waiting at a prompt, and not twice (#159). /// public sealed class CancelKeyPolicyTests { @@ -16,29 +17,43 @@ public sealed class CancelKeyPolicyTests [InlineData("sync")] [InlineData("repos")] [InlineData("orgs")] - public void ACommandThatWatchesForCancellation_FinishesGracefully(string command) - => Assert.True(CancelKeyPolicy.FinishesGracefully(command, isReadingInput: false, alreadyRequested: false)); + [InlineData("accounts")] + public void ACommandWithSomethingToFinish_IsNotCutOff(string command) + => Assert.True(CancelKeyPolicy.HoldsBack(command, isReadingInput: false, alreadyRequested: false)); [Theory] - [InlineData("accounts")] [InlineData("help")] [InlineData("--version")] [InlineData("no-such-command")] [InlineData(null)] - public void ACommandThatNeverLooks_EndsAtOnce(string? command) - => Assert.False(CancelKeyPolicy.FinishesGracefully(command, isReadingInput: false, alreadyRequested: false)); + public void AnythingElse_EndsAtOnce(string? command) + => Assert.False(CancelKeyPolicy.HoldsBack(command, isReadingInput: false, alreadyRequested: false)); + + + + [Theory] + [InlineData("sync")] + [InlineData("accounts")] + public void WhileWaitingForALineOfInput_TheProcessEndsAtOnce(string command) + => Assert.False(CancelKeyPolicy.HoldsBack(command, isReadingInput: true, alreadyRequested: false)); + + + + [Theory] + [InlineData("sync")] + [InlineData("accounts")] + public void ASecondCtrlC_EndsTheProcess(string command) + => Assert.False(CancelKeyPolicy.HoldsBack(command, isReadingInput: false, alreadyRequested: true)); [Fact] - public void WhileWaitingForALineOfInput_TheProcessEndsAtOnce() - => Assert.False(CancelKeyPolicy.FinishesGracefully("sync", isReadingInput: true, alreadyRequested: false)); - - - - [Fact] - public void ASecondCtrlC_EndsTheProcess() - => Assert.False(CancelKeyPolicy.FinishesGracefully("sync", isReadingInput: false, alreadyRequested: true)); + public void TheNotice_SaysWhatIsHappening_WhichForAnAccountsChangeIsNotACancellation() + { + Assert.StartsWith("Canceling...", CancelKeyPolicy.Notice("sync")); + Assert.StartsWith("Canceling...", CancelKeyPolicy.Notice(null)); + Assert.StartsWith("Finishing the change to the accounts...", CancelKeyPolicy.Notice("accounts")); + } } diff --git a/gclo.Cli.Tests/SyncCommandTests.cs b/gclo.Cli.Tests/SyncCommandTests.cs index 4d252dd..22fe283 100644 --- a/gclo.Cli.Tests/SyncCommandTests.cs +++ b/gclo.Cli.Tests/SyncCommandTests.cs @@ -777,4 +777,58 @@ public sealed class SyncCommandTests : IDisposable Assert.Contains("Canceled.", console.Error); Assert.Equal("", console.Out); // no plan, and no document that says "nothing to do" } + + + + // ---------------------------------------------------------------- sync --account when the stores cannot be used (#159) + + /// A vault whose reads fail the way Credential Manager's do when it cannot be used. + private sealed class UnreadableVault : ITokenVault + { + public void Store(Guid accountId, string token) + { + } + + public string? TryRetrieve(Guid accountId) + => throw new System.ComponentModel.Win32Exception(1312, "CredReadW failed (error 1312) for target 'gclo:account:test'."); + + public void Delete(Guid accountId) + { + } + } + + + + [Fact] + public async Task Account_WhoseTokenCannotBeReadFromTheCredentialStore_IsAFatalError_NotAnUnexpectedOne() + { + Seed("work", "acme", orgSubfolder: false); + var vault = new UnreadableVault(); + var lister = new FakeRepoLister { Result = [Repo("a")] }; + + CliErrorException ex = await Assert.ThrowsAsync(() => SyncCommand.RunAsync( + ["--account", "work"], lister, new FakeGit(), _ => (Store, vault), new NullLog(), CancellationToken.None)); + + Assert.Equal(ExitCodes.Fatal, ex.ExitCode); + Assert.StartsWith("CredReadW failed (error 1312)", ex.Message); + Assert.Null(lister.LastToken); // nothing was listed + } + + + + [Fact] + public async Task Account_WhenTheAccountsCannotBeOpened_IsAFatalError_OrRetryLater() + { + Func unreadable = _ => throw new UnauthorizedAccessException("Access to the path is denied."); + Func busy = + _ => throw new AccountsFileBusyException("The accounts file is in use by another gclo process.", new IOException("sharing violation")); + + CliErrorException denied = await Assert.ThrowsAsync(() => SyncCommand.RunAsync( + ["--account", "work"], new FakeRepoLister(), new FakeGit(), unreadable, new NullLog(), CancellationToken.None)); + CliErrorException retry = await Assert.ThrowsAsync(() => SyncCommand.RunAsync( + ["--account", "work"], new FakeRepoLister(), new FakeGit(), busy, new NullLog(), CancellationToken.None)); + + Assert.Equal(ExitCodes.Fatal, denied.ExitCode); + Assert.Equal(ExitCodes.Transient, retry.ExitCode); + } } diff --git a/gclo.Cli/AccountsCommand.cs b/gclo.Cli/AccountsCommand.cs index 370df51..2df5058 100644 --- a/gclo.Cli/AccountsCommand.cs +++ b/gclo.Cli/AccountsCommand.cs @@ -19,6 +19,11 @@ namespace gclo.Cli; /// internal static class AccountsCommand { + /// Characters that no folder path can contain on Windows, wherever gclo runs. + private static readonly System.Buffers.SearchValues s_neverInAFolderPath = System.Buffers.SearchValues.Create("|<>\"*?"); + + + private const string HelpText = """ Usage: gclo accounts [list] [--json] gclo accounts add --name --org --target [options] @@ -44,7 +49,8 @@ internal static class AccountsCommand add --name Display name; must be unique (case-insensitive). --org GitHub organization or user account to sync. - --target Target root folder. + --target Target root folder. A relative folder is stored as + a full path, resolved against the current directory. --parallel Parallel clone/pull count, 1-64 (default 8). --org-subfolder Clone under \ instead of . --description Optional note. @@ -77,7 +83,9 @@ internal static class AccountsCommand Exit codes: 0 done 2 fatal: bad arguments, unknown account, duplicate name, missing token, - or not running on Windows + not running on Windows, or the accounts file or Windows Credential + Manager could not be used (the message says why) + 4 another gclo process was changing the accounts; run the command again """; @@ -172,10 +180,10 @@ internal static class AccountsCommand } return ExitCodes.Success; } - catch(Exception ex) when(IsStorageFailure(ex)) + catch(Exception ex) when(CliErrorException.IsStorageFailure(ex)) { log.Error($"accounts failed: {ex.Message}", ex); - throw StorageFailure(ex); + throw CliErrorException.FromStorage(ex); } catch(Exception ex) { @@ -249,6 +257,8 @@ internal static class AccountsCommand throw new CliUsageException("--target is required."); } + string targetRoot = AbsoluteTarget(target); + log.Info($"accounts add started: name='{name.Trim()}', org='{org.Trim()}', token={(tokenOptions.UseDefault ? "default" : "own")}"); try { @@ -263,7 +273,7 @@ internal static class AccountsCommand Name = name.Trim(), Description = description.Trim(), Organization = org.Trim(), - TargetRoot = AbsoluteTarget(target), + TargetRoot = targetRoot, CreateOrgSubfolder = orgSubfolder, MaxConcurrency = parallel, TokenSource = tokenOptions.UseDefault ? TokenSource.Default : TokenSource.Own, @@ -272,10 +282,10 @@ internal static class AccountsCommand Terminal.Out($"Account '{account.Name}' added" + (tokenOptions.UseDefault ? " (uses the default token)." : ".")); return ExitCodes.Success; } - catch(Exception ex) when(IsStorageFailure(ex)) + catch(Exception ex) when(CliErrorException.IsStorageFailure(ex)) { log.Error($"accounts add failed: {ex.Message}", ex); - throw StorageFailure(ex); + throw CliErrorException.FromStorage(ex); } catch(Exception ex) { @@ -356,6 +366,7 @@ internal static class AccountsCommand RequireNotBlank(rename, "--rename"); RequireNotBlank(org, "--org"); RequireNotBlank(target, "--target"); + string? targetRoot = target is null ? null : AbsoluteTarget(target); log.Info($"accounts edit started: name='{name.Trim()}'"); try @@ -375,7 +386,7 @@ internal static class AccountsCommand { Name = rename?.Trim() ?? existing.Name, Organization = org?.Trim() ?? existing.Organization, - TargetRoot = target is null ? existing.TargetRoot : AbsoluteTarget(target), + TargetRoot = targetRoot ?? existing.TargetRoot, Description = description?.Trim() ?? existing.Description, MaxConcurrency = parallel ?? existing.MaxConcurrency, CreateOrgSubfolder = orgSubfolder ?? existing.CreateOrgSubfolder, @@ -388,10 +399,10 @@ internal static class AccountsCommand Terminal.Out($"Account '{updated.Name}' updated" + tokenNote); return ExitCodes.Success; } - catch(Exception ex) when(IsStorageFailure(ex)) + catch(Exception ex) when(CliErrorException.IsStorageFailure(ex)) { log.Error($"accounts edit failed: {ex.Message}", ex); - throw StorageFailure(ex); + throw CliErrorException.FromStorage(ex); } catch(Exception ex) { @@ -438,10 +449,10 @@ internal static class AccountsCommand Terminal.Out($"Account '{existing.Name}' removed."); return ExitCodes.Success; } - catch(Exception ex) when(IsStorageFailure(ex)) + catch(Exception ex) when(CliErrorException.IsStorageFailure(ex)) { log.Error($"accounts remove failed: {ex.Message}", ex); - throw StorageFailure(ex); + throw CliErrorException.FromStorage(ex); } catch(Exception ex) { @@ -478,26 +489,6 @@ internal static class AccountsCommand - /// - /// True for a failure of the accounts file or of the credential store: not a bug - /// in gclo and not a usage mistake, but something on this machine that the - /// message names (a file that cannot be written, an entry that cannot be read). - /// - private static bool IsStorageFailure(Exception ex) - => ex is IOException or UnauthorizedAccessException or AccountConsistencyException or System.ComponentModel.Win32Exception; - - - - /// - /// The error a storage failure ends the command with: fatal (2), as the help - /// says, or "retry later" (4) when another gclo process was changing the - /// accounts at that moment. - /// - private static CliErrorException StorageFailure(Exception ex) - => new(ex.Message, ex, ex is AccountsFileBusyException ? ExitCodes.Transient : ExitCodes.Fatal); - - - /// A value that was given must say something: a blank name, organization, or folder is a mistake. private static void RequireNotBlank(string? value, string option) { @@ -513,30 +504,56 @@ internal static class AccountsCommand /// The target folder as a full path. An account is used later from other places /// (the desktop app, a scheduled task, another working directory); a relative /// folder would mean a different one each time, so it is resolved once, here. + /// What cannot be a folder is refused instead of being stored: characters no + /// folder name can have, a device name, and text that a shell was meant to expand + /// and did not ('~', '%NAME%'), which would otherwise become a folder of that name. /// private static string AbsoluteTarget(string target) { - try + string trimmed = target.Trim(); + string? problem = trimmed.AsSpan().ContainsAny(s_neverInAFolderPath) || trimmed.Any(char.IsControl) + ? "it has a character no folder name can have" + : trimmed.StartsWith('~') || trimmed.Contains('%') + ? "'~' and '%NAME%' are not expanded by gclo; give the folder itself" + : !trimmed.StartsWith(@"\\", StringComparison.Ordinal) && trimmed.Where((c, i) => c == ':' && i != 1).Any() + ? "a colon belongs after a drive letter only" + : null; + + string full = ""; + if(problem is null) { - return Path.GetFullPath(target.Trim()); + try + { + full = Path.GetFullPath(trimmed); + } + catch(Exception ex) when(ex is ArgumentException or PathTooLongException) + { + problem = ex.Message; + } } - catch(Exception ex) when(ex is ArgumentException or NotSupportedException or PathTooLongException) + if(problem is null && full.StartsWith(@"\\.\", StringComparison.Ordinal)) { - throw new CliUsageException($"--target '{target}' is not a usable folder path: {ex.Message}"); + problem = "it names a device, not a folder"; } + + return problem is null + ? full + : throw new CliUsageException($"--target '{target}' is not a usable folder path: {problem}."); } - /// A duplicate name is the one validation the store itself enforces; everything else is fatal as is. + /// Saves, turning what the store refuses (a duplicate name, an account removed meanwhile) into a fatal error with its reason. private static void SaveOrFail(AccountsStore store, Account account, string? token) { try { store.Save(account, token); } - catch(ArgumentException ex) + catch(Exception ex) when(ex is ArgumentException or InvalidOperationException) { + // A name that is taken, or an account that another gclo process removed + // while this command was waiting: the store says which, and nothing was saved. throw new CliErrorException(ex.Message, ex); } } diff --git a/gclo.Cli/CancelKeyPolicy.cs b/gclo.Cli/CancelKeyPolicy.cs index 9225eb4..def2544 100644 --- a/gclo.Cli/CancelKeyPolicy.cs +++ b/gclo.Cli/CancelKeyPolicy.cs @@ -7,21 +7,32 @@ namespace gclo.Cli; /// -/// What the first Ctrl+C does. A command that watches for cancellation finishes -/// gracefully (in-flight work stops, the summary still prints). Everything else -/// ends at once: holding the key press back for a command that never looks, or -/// while the process sits in a prompt waiting for a line, would print -/// "Canceling..." and then carry on as if nothing was pressed. +/// What the first Ctrl+C does. For the commands gclo knows, the key press is held +/// back and the command gets to finish: a sync, a listing, or an organization +/// lookup watches for the request and stops gracefully (the summary still prints); +/// a change to the accounts does not watch, and is simply let through to its end, +/// because it writes to two places (the credential store and the accounts file) and +/// must not be cut off between them. While the process sits in a prompt waiting for +/// a line, nothing has been started yet, and it ends at once; so does anything on a +/// second Ctrl+C. /// internal static class CancelKeyPolicy { /// - /// True when the key press is to be held back and turned into a cancellation - /// request; false when the process is to end the way Ctrl+C ends any process. + /// True when the key press is to be held back; false when the process is to end + /// the way Ctrl+C ends any process. /// /// The command being run (the first argument), or null when there is none. /// Whether the process is waiting for a line on standard input right now. - /// Whether an earlier Ctrl+C was already turned into a request. - public static bool FinishesGracefully(string? command, bool isReadingInput, bool alreadyRequested) - => !alreadyRequested && !isReadingInput && command is "sync" or "repos" or "orgs"; + /// Whether an earlier Ctrl+C was already held back. + public static bool HoldsBack(string? command, bool isReadingInput, bool alreadyRequested) + => !alreadyRequested && !isReadingInput && command is "sync" or "repos" or "orgs" or "accounts"; + + + + /// What the user is told when the key press was held back for . + public static string Notice(string? command) + => string.Equals(command, "accounts", StringComparison.Ordinal) + ? "Finishing the change to the accounts... (press Ctrl+C again to abort immediately)" + : "Canceling... (press Ctrl+C again to abort immediately)"; } diff --git a/gclo.Cli/CliErrorException.cs b/gclo.Cli/CliErrorException.cs index c3cd9cb..e8a0174 100644 --- a/gclo.Cli/CliErrorException.cs +++ b/gclo.Cli/CliErrorException.cs @@ -37,4 +37,32 @@ internal sealed class CliErrorException : Exception /// public static CliErrorException FromEngine(InvalidOperationException ex) => new(ex.Message, ex, ex is GitHubAccessException access ? ExitCodes.ForAccess(access.Kind) : ExitCodes.Fatal); + + + + /// + /// True for a failure of the accounts file or of the credential store: not a bug + /// in gclo and not a usage mistake, but something on this machine that the + /// message names (a file that cannot be written, an entry that cannot be read). + /// + public static bool IsStorageFailure(Exception ex) + => ex is IOException or UnauthorizedAccessException or gclo.ViewModels.AccountConsistencyException + or System.ComponentModel.Win32Exception; + + + + /// + /// The error a storage failure ends a command with: fatal (2), or "retry later" + /// (4) when another gclo process was changing the accounts at that moment. A + /// failure of the credential store carries only a number; what Windows says that + /// number means is added. + /// + public static CliErrorException FromStorage(Exception ex) + { + string message = ex is System.ComponentModel.Win32Exception native + ? $"{ex.Message} ({new System.ComponentModel.Win32Exception(native.NativeErrorCode).Message})" + : ex.Message; + return new CliErrorException( + message, ex, ex is gclo.ViewModels.AccountsFileBusyException ? ExitCodes.Transient : ExitCodes.Fatal); + } } diff --git a/gclo.Cli/Program.cs b/gclo.Cli/Program.cs index f8596d5..77a3f28 100644 --- a/gclo.Cli/Program.cs +++ b/gclo.Cli/Program.cs @@ -72,17 +72,18 @@ internal static class Program using var cts = new CancellationTokenSource(); Console.CancelKeyPress += (_, e) => { - if(CancelKeyPolicy.FinishesGracefully(args.Length > 0 ? args[0] : null, Terminal.IsReadingInput, cts.IsCancellationRequested)) + string? command = args.Length > 0 ? args[0] : null; + if(CancelKeyPolicy.HoldsBack(command, Terminal.IsReadingInput, cts.IsCancellationRequested)) { // Finish gracefully: in-flight git operations stop, remaining - // repositories are marked Canceled, and the summary still prints. + // repositories are marked Canceled, and the summary still prints; a + // change to the accounts runs to its end. e.Cancel = true; cts.Cancel(); - Terminal.Error("Canceling... (press Ctrl+C again to abort immediately)"); + Terminal.Error(CancelKeyPolicy.Notice(command)); } - // Otherwise (a second Ctrl+C, a command that does not watch for - // cancellation, a prompt waiting for a line): e.Cancel stays false and - // the process terminates. + // Otherwise (a second Ctrl+C, a prompt waiting for a line, nothing that + // could be cut off half-way): e.Cancel stays false and the process terminates. }; try diff --git a/gclo.Cli/SyncCommand.cs b/gclo.Cli/SyncCommand.cs index 2f8675f..569fd34 100644 --- a/gclo.Cli/SyncCommand.cs +++ b/gclo.Cli/SyncCommand.cs @@ -296,8 +296,7 @@ internal static class SyncCommand ITokenVault? vault = null; if(accountName is not null) { - (store, vault) = openAccounts(log); - account = store.FindByName(accountName) ?? throw UnknownAccount(accountName, store); + (account, store, vault) = OpenAccount(openAccounts, accountName, log); org ??= account.Organization; target ??= account.CreateOrgSubfolder ? Path.Combine(account.TargetRoot, org) : account.TargetRoot; parallel ??= account.MaxConcurrency; @@ -481,6 +480,28 @@ internal static class SyncCommand + /// + /// Opens the accounts and finds the one named. A failure of the accounts file or + /// of the credential store is a fatal error with its reason (or "retry later" + /// when another gclo process holds the file), as it is for 'gclo accounts'. + /// + private static (Account Account, AccountsStore Store, ITokenVault Vault) OpenAccount( + Func openAccounts, string accountName, IActivityLog log) + { + try + { + (AccountsStore store, ITokenVault vault) = openAccounts(log); + Account account = store.FindByName(accountName) ?? throw UnknownAccount(accountName, store); + return (account, store, vault); + } + catch(Exception ex) when(CliErrorException.IsStorageFailure(ex)) + { + throw CliErrorException.FromStorage(ex); + } + } + + + /// /// The token for this run. Any explicit token option wins; otherwise a saved /// account's token comes from Windows Credential Manager; plain runs fall back @@ -494,7 +515,18 @@ internal static class SyncCommand } // vault is non-null whenever account is: both come from AccountsCommand.Open. - string? token = new AccountTokenResolver(vault!).TryResolve(account); + string? token; + try + { + token = new AccountTokenResolver(vault!).TryResolve(account); + } + catch(Exception ex) when(CliErrorException.IsStorageFailure(ex)) + { + // The credential store did not answer: say so with its reason (exit 2), + // not as an unexpected error inside gclo. + throw CliErrorException.FromStorage(ex); + } + if(token is null) { string fix = account.UsesDefaultToken -- 2.54.0