Repository: pre-commit/pre-commit

Issue:
Deprecate and remove some (useless?) options
I find the following don't really have any good use cases (and don't come up in normal day-to-day) and are undocumented beyond `--help`.  **I'm proposing removing these options**:

## `pre-commit run --no-stash`

This disables the auto-stashing of files when running -- though this is already the case for `pre-commit run --all-files` and `pre-commit run --files ...`.

The behaviour of `--no-stash` (without using `--no-stash`) can be achieved via `git diff --name-only | xargs pre-commit run --files`

It was added [along with the avoiding behaviour](https://github.com/pre-commit/pre-commit/pull/80) in the same pull request.  I want to say this was my first idea for "fixing" the [original problem](https://github.com/pre-commit/pre-commit/issues/68) and then I forgot to undo it.

## `pre-commit run --allow-unstaged-config`

This (unfortunately) collides with `pre-commit run --all` (prefix match of `pre-commit run --all-files`) so I've wanted to get rid of it anyway.

This allows one to run with an unstaged configuration, which then (in most cases) gets the changes swiped out from under you and causes confusing situations where the hooks that are run aren't what was on disk at the time of running.  The warning that's printed when doing this also explains this.

This was [originally my idea](https://github.com/pre-commit/pre-commit/issues/157#issuecomment-99080756) but now I think we can just do without the option at all -- requiring the pre-commit configuration to be staged when running pre-commit.

Discussion:

Patch:
diff --git a/pre_commit/commands/run.py b/pre_commit/commands/run.py
index c18f2aa..55d2b12 100644
--- a/pre_commit/commands/run.py
+++ b/pre_commit/commands/run.py
@@ -217,7 +217,7 @@ def _has_unstaged_config(runner):
 
 
 def run(runner, args, environ=os.environ):
-    no_stash = args.no_stash or args.all_files or bool(args.files)
+    no_stash = args.all_files or bool(args.files)
 
     # Check if we have unresolved merge conflict files and fail fast.
     if _has_unmerged_paths(runner):
@@ -227,20 +227,11 @@ def run(runner, args, environ=os.environ):
         logger.error('Specify both --origin and --source.')
         return 1
     if _has_unstaged_config(runner) and not no_stash:
-        if args.allow_unstaged_config:
-            logger.warn(
-                'You have an unstaged config file and have specified the '
-                '--allow-unstaged-config option.\n'
-                'Note that your config will be stashed before the config is '
-                'parsed unless --no-stash is specified.',
-            )
-        else:
-            logger.error(
-                'Your .pre-commit-config.yaml is unstaged.\n'
-                '`git add .pre-commit-config.yaml` to fix this.\n'
-                'Run pre-commit with --allow-unstaged-config to silence this.',
-            )
-            return 1
+        logger.error(
+            'Your .pre-commit-config.yaml is unstaged.\n'
+            '`git add .pre-commit-config.yaml` to fix this.\n',
+        )
+        return 1
 
     # Expose origin / source as environment variables for hooks to consume
     if args.origin and args.source:
diff --git a/pre_commit/main.py b/pre_commit/main.py
index 3a2fee1..0b00a86 100644
--- a/pre_commit/main.py
+++ b/pre_commit/main.py
@@ -135,10 +135,6 @@ def main(argv=None):
     _add_color_option(run_parser)
     _add_config_option(run_parser)
     run_parser.add_argument('hook', nargs='?', help='A single hook-id to run')
-    run_parser.add_argument(
-        '--no-stash', default=False, action='store_true',
-        help='Use this option to prevent auto stashing of unstaged files.',
-    )
     run_parser.add_argument(
         '--verbose', '-v', action='store_true', default=False,
     )
@@ -154,13 +150,6 @@ def main(argv=None):
         '--commit-msg-filename',
         help='Filename to check when running during `commit-msg`',
     )
-    run_parser.add_argument(
-        '--allow-unstaged-config', default=False, action='store_true',
-        help=(
-            'Allow an unstaged config to be present.  Note that this will '
-            'be stashed before parsing unless --no-stash is specified.'
-        ),
-    )
     run_parser.add_argument(
         '--hook-stage', choices=('commit', 'push', 'commit-msg'),
         default='commit',
@@ -173,7 +162,7 @@ def main(argv=None):
     run_mutex_group = run_parser.add_mutually_exclusive_group(required=False)
     run_mutex_group.add_argument(
         '--all-files', '-a', action='store_true', default=False,
-        help='Run on all the files in the repo.  Implies --no-stash.',
+        help='Run on all the files in the repo.',
     )
     run_mutex_group.add_argument(
         '--files', nargs='*', default=[],


Test patch:
diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py
index c360fde..924d097 100644
--- a/tests/commands/run_test.py
+++ b/tests/commands/run_test.py
@@ -54,10 +54,8 @@ def _get_opts(
         color=False,
         verbose=False,
         hook=None,
-        no_stash=False,
         origin='',
         source='',
-        allow_unstaged_config=False,
         hook_stage='commit',
         show_diff_on_failure=False,
         commit_msg_filename='',
@@ -70,10 +68,8 @@ def _get_opts(
         color=color,
         verbose=verbose,
         hook=hook,
-        no_stash=no_stash,
         origin=origin,
         source=source,
-        allow_unstaged_config=allow_unstaged_config,
         hook_stage=hook_stage,
         show_diff_on_failure=show_diff_on_failure,
         commit_msg_filename=commit_msg_filename,
@@ -332,38 +328,6 @@ def test_origin_source_error_msg(
         assert warning_msg not in printed
 
 
-@pytest.mark.parametrize(
-    ('no_stash', 'all_files', 'expect_stash'),
-    (
-        (True, True, False),
-        (True, False, False),
-        (False, True, False),
-        (False, False, True),
-    ),
-)
-def test_no_stash(
-        cap_out,
-        repo_with_passing_hook,
-        no_stash,
-        all_files,
-        expect_stash,
-        mock_out_store_directory,
-):
-    stage_a_file()
-    # Make unstaged changes
-    with open('foo.py', 'w') as foo_file:
-        foo_file.write('import os\n')
-
-    args = _get_opts(no_stash=no_stash, all_files=all_files)
-    ret, printed = _do_run(cap_out, repo_with_passing_hook, args)
-    assert ret == 0
-    warning_msg = b'[WARNING] Unstaged files detected.'
-    if expect_stash:
-        assert warning_msg in printed
-    else:
-        assert warning_msg not in printed
-
-
 @pytest.mark.parametrize(('output', 'expected'), (('some', True), ('', False)))
 def test_has_unmerged_paths(output, expected):
     mock_runner = mock.Mock()
@@ -715,37 +679,19 @@ def modified_config_repo(repo_with_passing_hook):
     yield repo_with_passing_hook
 
 
-def test_allow_unstaged_config_option(
+def test_error_with_unstaged_config(
         cap_out, modified_config_repo, mock_out_store_directory,
 ):
-    args = _get_opts(allow_unstaged_config=True)
-    ret, printed = _do_run(cap_out, modified_config_repo, args)
-    expected = (
-        b'You have an unstaged config file and have specified the '
-        b'--allow-unstaged-config option.'
-    )
-    assert expected in printed
-    assert ret == 0
-
-
-def test_no_allow_unstaged_config_option(
-        cap_out, modified_config_repo, mock_out_store_directory,
-):
-    args = _get_opts(allow_unstaged_config=False)
+    args = _get_opts()
     ret, printed = _do_run(cap_out, modified_config_repo, args)
     assert b'Your .pre-commit-config.yaml is unstaged.' in printed
     assert ret == 1
 
 
 @pytest.mark.parametrize(
-    'opts',
-    (
-        {'allow_unstaged_config': False, 'no_stash': True},
-        {'all_files': True},
-        {'files': [C.CONFIG_FILE]},
-    ),
+    'opts', ({'all_files': True}, {'files': [C.CONFIG_FILE]}),
 )
-def test_unstaged_message_suppressed(
+def test_no_unstaged_error_with_all_files_or_files(
         cap_out, modified_config_repo, mock_out_store_directory, opts,
 ):
     args = _get_opts(**opts)
diff --git a/tests/git_test.py b/tests/git_test.py
index 4ffccee..0500a42 100644
--- a/tests/git_test.py
+++ b/tests/git_test.py
@@ -137,8 +137,7 @@ def test_get_conflicted_files_in_submodule(in_conflicting_submodule):
 
 
 def test_get_conflicted_files_unstaged_files(in_merge_conflict):
-    # If they for whatever reason did pre-commit run --no-stash during a
-    # conflict
+    """This case no longer occurs, but it is a useful test nonetheless"""
     resolve_conflict()
 
     # Make unstaged file.