[BUG] unused assignment in file.patch

[OrangeDog]

Description
The state file.patch documents an options argument.

Extra options to pass to patch. This should not be necessary in most cases.

However, the code never uses them. It sanitises them and reports errors, but the variable is unused after this line: https://github.com/saltstack/salt/blob/v3000.2/salt/states/file.py#L6192

options = sanitized_options

[DmitryKuzmenko]

@OrangeDog I see after the line the options argument is used in the inline function _patch here
https://github.com/saltstack/salt/blob/v3000.2/salt/states/file.py#L6263
The function is used next to pre-check and apply the state.
Could you please provide an example where the options aren't passed to the file.patch execution module function?

[OrangeDog]

@DmitryKuzmenko read more carefully. That's not the same options. It's a new parameter of the _patch function.

You can trivially construct any example:

patch example:
  - file.patch:
    - name: /path
    - source: salt://foo.patch
    - options: [--backup]

The option won't be used, and no backups will be made.

[DmitryKuzmenko]

@OrangeDog good catch. You're right, thank you.

[piterpunk]

I made this state to test this issue:

patch_tst:
  file.patch:
    - name: /srv/salt/filepatch/file
    - source: salt://filepatch/patch.diff
    - options: [--backup]

Then I executed the state.apply:

local:
----------
          ID: patch_tst
    Function: file.patch
        Name: /srv/salt/filepatch/file
      Result: True
     Comment: Patch successfully applied
     Started: 23:29:17.419073
    Duration: 358.606 ms
     Changes:   
              ----------
              pid:
                  20581
              retcode:
                  0
              stderr:
              stdout:
                  patching file /srv/salt/filepatch/file

Summary for local
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time: 358.606 ms

During execution, I could see the '--backup' option passed to 'patch' command:

[DEBUG   ] file.managed: {'changes': {'diff': '--- \n+++ \n@@ -0,0 +1,5 @@\n+--- file.orig\t2020-07-15 23:24:07.722616027 -0300\n++++ file.dest\t2020-07-15 23:24:29.290689009 -0300\n+@@ -1 +1 @@\n+-THIS IS THE ORIGINAL FILE\n++THIS IS THE PATCHED FILE\n'}, 'comment': 'File /tmp/__salt.tmp.fbzi3q53 updated', 'name': '/tmp/__salt.tmp.fbzi3q53', 'result': True}
[DEBUG   ] LazyLoaded cmd.run_all
[INFO    ] Executing command ['/usr/bin/patch', '--backup', '-N', '-r', '/tmp/__salt.tmp.kcisx1x0', '-o', '/tmp/__salt.tmp.gfnpreju', '-i', '/tmp/__salt.tmp.fbzi3q53', '/srv/salt/filepatch/file'] in directory '/root'
[DEBUG   ] stdout: patching file /tmp/__salt.tmp.gfnpreju (read from /srv/salt/filepatch/file)
[INFO    ] Executing command ['/usr/bin/patch', '--backup', '--forward', '--reject-file=-', '-i', '/tmp/__salt.tmp.fbzi3q53', '/srv/salt/filepatch/file'] in directory '/root'

And the backup file was created as expected:

-rw-r--r-- 1 root root 25 Jul 15 23:29 file
-rw-r--r-- 1 root root 26 Jul 15 23:25 file.orig

The code, indeed, doesn't use the "options" variable after setting "options = sanitized_options", but uses the original "sanitized_options":

        def _patch(patch_file, options=None, dry_run=False):
            patch_opts = copy.copy(sanitized_options)
            if options is not None:
                patch_opts.extend(options)
            return __salt__["file.patch"](
                name, patch_file, options=patch_opts, dry_run=dry_run
            )

Am I missing something?

[OrangeDog]

@piterpunk hmm, you are correct. Not sure how Dmitry and I convinced ourselves otherwise.

I think all that needs doing is remove the options = sanitized_options line to prevent future confusion.