Refactored prompt behavior ⚓

- skipNonExistingSharedHooks introduced instead of failOnNonExistingSharedHooks. Failing is now default. - skipUntrustedHooks introduced. Failing on active, untrusted hooks is now default.
gabyx · Mar 5, 2021 · 521cd9c · 521cd9c
1 parent fa120ce
commit 521cd9c
Show file tree

Hide file tree

Showing 14 changed files with 457 additions and 129 deletions.
diff --git a/README.md b/README.md
@@ -343,8 +343,8 @@ on any local repositories. Any other local path will be used **directly and will
 Additionally, the update can also be triggered on other hook names by setting a comma-separated list of additional
 hook names in the Git configuration parameter `githooks.sharedHooksUpdateTriggers` on any configuration level.
 
-An additional global configuration parameter `githooks.failOnNonExistingSharedHooks` makes hooks fail with an error if any shared hook configured in `.shared.yaml` is missing, meaning `git hooks update` has not yet been called.
-See [`git hooks config fail-on-non-existing-shared-hooks --help`](docs/cli/git_hooks_config_fail-on-non-existing-shared-hooks.md)
+An additional global configuration parameter `githooks.skipNonExistingSharedHooks` makes Githooks skip any configured non-existing shared hooks. Otherwise, Githooks will fail and you need to update them by running `git hooks update`.
+See [`git hooks config skip-non-existing-shared-hooks --help`](docs/cli/skip-non-existing-shared-hooks.md)
 
 You can also manage and update shared hook repositories using the [`git hooks shared --help`](docs/cli/git_hooks_shared.md) tool.
 
@@ -406,8 +406,13 @@ Hooks in individual shared repositories can be disabled as well, with [`git hook
 ## Trusting Hooks
 
 To try and make things a little bit more secure, Githooks checks if any new hooks were added we haven't run before,
-or if any of the existing ones have changed. When they have, it will prompt for confirmation whether you accept
+or if any of the existing ones have changed. When they have, it will prompt for confirmation (trust prompt) whether you accept
 those changes or not, and you can also disable specific hooks to skip running them until you decide otherwise.
+The trust prompt is always **fatal** meaning that failing to answer the prompt, or any other prompt error, will result in a failing Git hook. To make the `runner` non-interactive, see [user prompts](#user-prompts).
+If a hook is still *active and untrusted* after the prompt, Githooks will fail by default. This is useful to be sure that all hooks get executed.
+However, you can skip active, untrusted hooks by setting [`git hooks config skip-untrusted-hooks --help`](docs/cli/git_hooks_config_skip-untrusted-hooks.md).
+
+
 The accepted checksums are maintained in the `.git/.githooks.checksum` directory, per local repository.
 You can however use a global checksum directory setup by specifing `githooks.checksumCacheDir`
 in any suitable Git config (can be different for each repository).
@@ -420,11 +425,7 @@ This is a per-repository setting.
 Consult [`git hooks trust --help`](docs/cli/git_hooks_trust.md) and [`git hooks config trust-all --help`](docs/cli/git_hooks_config_trusted.md)
 for more information.
 
-There is a caveat worth mentioning: if a terminal *(tty)* can't be allocated, then the default action is to accept the changes or new hooks.
-A terminal cannot be allocated for exmaple if you execute Git over a GUI such as VS Code or any other Git GUI.
-Let me know in an issue if you strongly disagree, and you think this is a big enough risk worth having slightly worse UX instead.
-
-You can also accept (trust) hooks by using [`git hooks trust hooks ---help`](docs/cli/git_hooks_trust_hooks.md).
+You can also trust individual hooks by using [`git hooks trust hooks --help`](docs/cli/git_hooks_trust_hooks.md).
 
 ## Disabling Githooks
 
@@ -701,11 +702,11 @@ The `runner` will show prompts, either in the terminal or as GUI dialog, in the
 2. **Update prompts**: The user is requested to accept a new update if automatic updates are enabled (`git hooks update --enable`): **non-fatal**.
     - Various other prompts when the updater is launched: **non-fatal**.
 
-User prompts during `runner` execution are sometimes not desirable (server infastructure, docker container, etc...) and need to be disabled. Setting `git hooks config runner-non-interactive --enable` will:
+User prompts during `runner` execution are sometimes not desirable (server infastructure, docker container, etc...) and need to be disabled. Setting `git hooks config runner-non-interactive --enable -global` will:
 
-- Take default answers for all **non-fatal** prompts.
+- Take default answers for all **non-fatal** prompts. No warnings are shown.
 - Take default answer for a **fatal prompt** if it is configured to do so.
-  The only fatal prompt is the **trust prompt** which can be configured to pass by setting `git hooks config trust-all --accept`.
+  The only fatal prompt is the **trust prompt** which can be configured to pass by executing `git hooks config trust-all --accept`
 
 
 ### Custom User Prompt

diff --git a/docs/cli/git_hooks_config.md b/docs/cli/git_hooks_config.md
@@ -23,10 +23,12 @@ git hooks config
 * [git hooks config clone-url](git_hooks_config_clone-url.md)	 - Changes the Githooks clone url used for any update.
 * [git hooks config delete-detected-lfs-hooks](git_hooks_config_delete-detected-lfs-hooks.md)	 - Change the behavior for detected LFS hooks during install.
 * [git hooks config disable](git_hooks_config_disable.md)	 - Disables Githooks in the current repository or globally.
-* [git hooks config fail-on-non-existing-shared-hooks](git_hooks_config_fail-on-non-existing-shared-hooks.md)	 - Updates the list of local or global shared hook repositories.
 * [git hooks config list](git_hooks_config_list.md)	 - Lists settings of the Githooks configuration.
+* [git hooks config non-interactive-runner](git_hooks_config_non-interactive-runner.md)	 - Enables/disables non-interactive execution of the runner.
 * [git hooks config search-dir](git_hooks_config_search-dir.md)	 - Changes the search directory used during installation.
 * [git hooks config shared](git_hooks_config_shared.md)	 - Updates the list of local or global shared hook repositories.
+* [git hooks config skip-non-existing-shared-hooks](git_hooks_config_skip-non-existing-shared-hooks.md)	 - Enable or disable skipping non-existing shared hooks.
+* [git hooks config skip-untrusted-hooks](git_hooks_config_skip-untrusted-hooks.md)	 - Enable/disable skipping active, untrusted hooks.
 * [git hooks config trust-all](git_hooks_config_trust-all.md)	 - Change trust settings in the current repository.
 * [git hooks config update](git_hooks_config_update.md)	 - Change Githooks update settings.
 * [git hooks config update-time](git_hooks_config_update-time.md)	 - Changes the Githooks update time.

diff --git a/docs/cli/git_hooks_config_fail-on-non-existing-shared-hooks.md b/docs/cli/git_hooks_config_fail-on-non-existing-shared-hooks.md
diff --git a/docs/cli/git_hooks_config_non-interactive-runner.md b/docs/cli/git_hooks_config_non-interactive-runner.md
@@ -0,0 +1,33 @@
+## git hooks config non-interactive-runner
+
+Enables/disables non-interactive execution of the runner.
+
+### Synopsis
+
+Enable or disables non-interactive execution of
+the Githooks runner executable.
+
+This will only default answer all non-fatal prompts. Fatal prompts (e.g. the trust prompts)
+still need to be configured to pass. See `git hooks config trust-all --help`.
+
+```
+git hooks config non-interactive-runner [flags]
+```
+
+### Options
+
+```
+      --print                                                  Print the setting.
+      --enable                                                 Enables non-interactive mode of the runner executable.
+      --disable                                                Disables non-interactive mode of the runner executable.
+      --Reset non-interactive mode of the runner executable.   Reset the setting.
+      --local                                                  Use the local Git configuration (default, except for `--print`).
+      --global                                                 Use the global Git configuration.
+  -h, --help                                                   help for non-interactive-runner
+```
+
+### SEE ALSO
+
+* [git hooks config](git_hooks_config.md)	 - Manages various Githooks configuration.
+
+###### Auto generated by spf13/cobra 
diff --git a/docs/cli/git_hooks_config_skip-non-existing-shared-hooks.md b/docs/cli/git_hooks_config_skip-non-existing-shared-hooks.md
@@ -0,0 +1,31 @@
+## git hooks config skip-non-existing-shared-hooks
+
+Enable or disable skipping non-existing shared hooks.
+
+### Synopsis
+
+Enable or disable failing hooks with an error when any
+shared hooks are missing. This usually means `git hooks shared update`
+has not been called yet.
+
+```
+git hooks config skip-non-existing-shared-hooks [flags]
+```
+
+### Options
+
+```
+      --print                                       Print the setting.
+      --enable                                      Enable skipping non-existing shared hooks.
+      --disable                                     Disable skipping non-existing shared hooks.
+      --Reset skipping non-existing shared hooks.   Reset the setting.
+      --local                                       Use the local Git configuration (default, except for `--print`).
+      --global                                      Use the global Git configuration.
+  -h, --help                                        help for skip-non-existing-shared-hooks
+```
+
+### SEE ALSO
+
+* [git hooks config](git_hooks_config.md)	 - Manages various Githooks configuration.
+
+###### Auto generated by spf13/cobra 
diff --git a/docs/cli/git_hooks_config_skip-untrusted-hooks.md b/docs/cli/git_hooks_config_skip-untrusted-hooks.md
@@ -0,0 +1,31 @@
+## git hooks config skip-untrusted-hooks
+
+Enable/disable skipping active, untrusted hooks.
+
+### Synopsis
+
+Enable or disable failing hooks with an error when any
+active, untrusted hooks are present.
+Mostly wanted if all hooks must be executed.
+
+```
+git hooks config skip-untrusted-hooks [flags]
+```
+
+### Options
+
+```
+      --print                                     Print the setting.
+      --enable                                    Enable skipping active, untrusted hooks.
+      --disable                                   Disable skipping active, untrusted hooks.
+      --Reset skipping active, untrusted hooks.   Reset the setting.
+      --local                                     Use the local Git configuration (default, except for `--print`).
+      --global                                    Use the global Git configuration.
+  -h, --help                                      help for skip-untrusted-hooks
+```
+
+### SEE ALSO
+
+* [git hooks config](git_hooks_config.md)	 - Manages various Githooks configuration.
+
+###### Auto generated by spf13/cobra 
diff --git a/githooks/apps/runner/runner.go b/githooks/apps/runner/runner.go
@@ -159,7 +159,9 @@ func setMainVariables(repoPath string) (HookSettings, UISettings) {
 		isTrusted = showTrustRepoPrompt(gitx, promptCtx)
 	}
 
-	failOnNonExistingHooks := gitx.GetConfig(hooks.GitCKFailOnNonExistingSharedHooks, git.Traverse) == git.GitCVTrue
+	nonInteractive := hooks.IsRunnerNonInteractive(gitx, git.Traverse)
+	skipNonExistingSharedHooks, _ := hooks.SkipNonExistingSharedHooks(gitx, git.Traverse)
+	skipUntrustedHooks, _ := hooks.SkipUntrustedHooks(gitx, git.Traverse)
 
 	s := HookSettings{
 		Args:               os.Args[2:],
@@ -176,7 +178,9 @@ func setMainVariables(repoPath string) (HookSettings, UISettings) {
 
 		IsRepoTrusted: isTrusted,
 
-		FailOnNonExistingSharedHooks: failOnNonExistingHooks}
+		SkipNonExistingSharedHooks: skipNonExistingSharedHooks,
+		SkipUntrustedHooks:         skipUntrustedHooks,
+		NonInteractive:             nonInteractive}
 
 	log.DebugF(s.toString())
 
@@ -244,10 +248,10 @@ func showTrustRepoPrompt(gitx *git.Context, promptCtx prompt.IContext) (isTruste
 		"Do you want to allow running every current and future hooks?"
 
 	var answer string
-	answer, err := promptCtx.ShowOptions(question, "(yes, No)", "y/N", "Yes", "No")
-	log.AssertNoErrorF(err, "Could not show prompt.")
+	answer, err := promptCtx.ShowOptions(question, "(yes, no)", "y/n", "Yes", "No")
+	log.AssertNoErrorPanicF(err, "Could not get trust prompt answer.")
 
-	if err == nil && answer == "y" || answer == "Y" {
+	if answer == "y" || answer == "Y" {
 		err := hooks.SetTrustAllSetting(gitx, true, false)
 		log.AssertNoErrorF(err, "Could not store trust setting.")
 		isTrusted = true
@@ -292,14 +296,19 @@ func updateGithooks(settings *HookSettings, uiSettings *UISettings) {
 		return
 	}
 
+	opts := []string{"--internal-auto-update"}
+	if settings.NonInteractive {
+		opts = append(opts, "--non-interactive")
+	}
+
 	updateAvailable, accepted, err := updates.RunUpdate(
 		settings.InstallDir,
 		updates.DefaultAcceptUpdateCallback(log, uiSettings.PromptCtx, false),
 		func() error {
 			return updates.RunUpdateOverExecutable(settings.InstallDir,
 				&settings.ExecX,
 				cm.UseStreams(nil, log.GetInfoWriter(), log.GetErrorWriter()),
-				"--internal-auto-update")
+				opts...)
 		})
 
 	log.AssertNoErrorPanic(err, "Running update failed.")
@@ -409,10 +418,20 @@ func executeOldHook(settings *HookSettings,
 	}
 
 	hook := hooks[0]
+
 	if hook.Active && !hook.Trusted {
-		// Active hook, but not trusted:
-		// Show trust prompt to let user trust it or disable.
-		showTrustPrompt(uiSettings, checksums, &hook)
+		if !settings.NonInteractive {
+			// Active hook, but not trusted:
+			// Show trust prompt to let user trust it or disable it.
+			showTrustPrompt(uiSettings, checksums, &hook)
+		}
+
+		log.PanicIf(!settings.SkipUntrustedHooks && hook.Active && !hook.Trusted,
+			"Hook '%s' is active and needs to be trusted first.\n"+
+				"Either trust the hook or disable it, or skip active,\n"+
+				"untrusted hooks by running:\n"+
+				"  $ git hooks config skip-untrusted-hooks --enable",
+			hook.NamespacePath)
 	}
 
 	if !hook.Active || !hook.Trusted {
@@ -490,7 +509,7 @@ func getRepoSharedHooks(
 	shared, err := hooks.LoadRepoSharedHooks(settings.InstallDir, settings.RepositoryDir)
 
 	if err != nil {
-		log.ErrorOrPanicF(settings.FailOnNonExistingSharedHooks, err,
+		log.ErrorOrPanicF(!settings.SkipNonExistingSharedHooks, err,
 			"Repository shared hooks are demanded but failed "+
 				"to parse the file:\n'%s'",
 			hooks.GetRepoSharedFile(settings.RepositoryDir))
@@ -531,7 +550,7 @@ func getConfigSharedHooks(
 	}
 
 	if err != nil {
-		log.ErrorOrPanicF(settings.FailOnNonExistingSharedHooks,
+		log.ErrorOrPanicF(!settings.SkipNonExistingSharedHooks,
 			err,
 			"Shared hooks are demanded but failed "+
 				"to parse the %s config:\n'%s'",
@@ -597,11 +616,11 @@ func checkSharedHook(
 			mess += "It does not exist."
 		}
 
-		if !settings.FailOnNonExistingSharedHooks {
+		if settings.SkipNonExistingSharedHooks {
 			mess += "\nContinuing..."
 		}
 
-		log.ErrorOrPanicF(settings.FailOnNonExistingSharedHooks, err, mess, hook.OriginalURL)
+		log.ErrorOrPanicF(!settings.SkipNonExistingSharedHooks, err, mess, hook.OriginalURL)
 
 		return false
 	}
@@ -620,11 +639,11 @@ func checkSharedHook(
 				"  $ git hooks shared purge\n" +
 				"  $ git hooks shared update"
 
-			if !settings.FailOnNonExistingSharedHooks {
+			if settings.SkipNonExistingSharedHooks {
 				mess += "\nContinuing..."
 			}
 
-			log.ErrorOrPanicF(settings.FailOnNonExistingSharedHooks,
+			log.ErrorOrPanicF(!settings.SkipNonExistingSharedHooks,
 				nil, mess, hook.OriginalURL, url)
 
 			return false
@@ -706,9 +725,19 @@ func getHooksIn(
 		hook := &allHooks[i]
 
 		if hook.Active && !hook.Trusted {
-			// Active hook, but not trusted:
-			// Show trust prompt to let user trust it or disable.
-			showTrustPrompt(uiSettings, checksums, hook)
+
+			if !settings.NonInteractive {
+				// Active hook, but not trusted:
+				// Show trust prompt to let user trust it or disable it.
+				showTrustPrompt(uiSettings, checksums, hook)
+			}
+
+			log.PanicIf(!settings.SkipUntrustedHooks && hook.Active && !hook.Trusted,
+				"Hook '%s' is active and needs to be trusted first.\n"+
+					"Either trust the hook or disable it, or skip active,\n"+
+					"untrusted hooks by running:\n"+
+					"  $ git hooks config skip-untrusted-hooks --enable",
+				hook.NamespacePath)
 		}
 
 		if !hook.Active || !hook.Trusted {
@@ -791,10 +820,10 @@ func showTrustPrompt(
 		question := mess + "\nDo you accept the changes?"
 
 		answer, err := uiSettings.PromptCtx.ShowOptions(question,
-			"(Yes, all, no, disable)",
-			"Y/a/n/d",
+			"(yes, all, no, disable)",
+			"y/a/n/d",
 			"Yes", "All", "No", "Disable")
-		log.AssertNoError(err, "Could not show prompt.")
+		log.AssertNoErrorPanic(err, "Could not get trust prompt answer.")
 
 		switch answer {
 		case "a":

diff --git a/githooks/apps/runner/settings.go b/githooks/apps/runner/settings.go
@@ -20,8 +20,10 @@ type HookSettings struct {
 	HookName string // Name of the hook.
 	HookDir  string // Directory of the hook.
 
-	IsRepoTrusted                bool // If the repository is a trusted repository.
-	FailOnNonExistingSharedHooks bool // If Githooks should fail if there are shared hooks demanded which are not existing.
+	IsRepoTrusted              bool // If the repository is a trusted repository.
+	SkipNonExistingSharedHooks bool // If Githooks should skip non-existing shared hooks.
+	SkipUntrustedHooks         bool // If Githooks should skip active untrusted hooks.
+	NonInteractive             bool // If all non-fatal prompts should be default answered.
 }
 
 func (s HookSettings) toString() string {