[feature] Allow choosing the diff command

[Sighery]

The default diff command is diff. Some people will use other diff tools, or install colordiff to get basically diff with colours.

Currently there's no way to change the diff command, other than manually modifying the local code.

There's also an option to pass in custom arguments to the diff call (-o --dopts=<opts>) but it's only available for the compare command, even though diffs can be displayed for install and possibly other commands too.

[deadc0de6]

Yeah that sounds like a good idea, I will look into it and keep you posted. Thanks for the suggestion!

[Sighery]

It could either be an environment variable, or a general config setting. For me both work, but generally it's more of a pain to set up persistent environment variables in the system than to just write it into the config file and forget about it.

One thing to note, though: It might make sense for not only the command itself to be configurable, but the arguments it takes as well. As in the same config key accepting both the command name and its arguments to then use:

config:
    diff_command: 'colordiff --tabsize=4 --unified'

I mention this because the default diff command is diff -r, which uses -r (-r, --recursive recursively compare any subdirectories found). Not every diff command might have this option under this same letter, so I believe it's just better to let the user specify it. And letting the user specify it also allows them to set their own custom arguments to the diff command on the config file and not on runtime through --dopts which currently is only supported in the compare subcommand.

[deadc0de6]

I have made a first attempt at this in branch diff_command.

The command line option (-o --dopts) is gone and a new config entry is available: diff_command. This allows to specify the diff command to use with the two arguments. The default is diff -r {0} {1} but you should be able to add your own, for example:

config:
  diff_command: "colordiff --tabsize=4 --unified {0} {1}"

or

config:
  diff_command: "diff -u {0} {1}"

This impacts the install and the compare commands (as these are the two commands using diff).

Can you give it a try?

[Sighery]

Tried it a little bit. I'll try it more extensively during the following days/weekend most likely.

So far it looks good, but there's a small issue with escaping:

=> compare f_vscode_settings: diffing with "~/.config/Code - OSS/User/settings.json"
diff: extra operand 'OSS/User/settings.json'
diff: Try 'diff --help' for more information.

I suggest you keep the double quotes on the default diff command. Here the patch (git apply patch.file) to apply:

diff --git a/dotdrop/settings.py b/dotdrop/settings.py
index bfc3ae6..0bef2b3 100644
--- a/dotdrop/settings.py
+++ b/dotdrop/settings.py
@@ -49,7 +49,7 @@ class Settings(DictParser):
                  upignore=[], cmpignore=[], instignore=[],
                  workdir='~/.config/dotdrop', showdiff=False,
                  minversion=None, func_file=[], filter_file=[],
-                 diff_command='diff -r {0} {1}'):
+                 diff_command='diff -r "{0}" "{1}"'):
         self.backup = backup
         self.banner = banner
         self.create = create

This fix isn't perfect, since Linux filenames can contain pretty much every character, " included (try to do touch t\"t.test, it will work). I'd have to check on the subprocess library, but I believe that if you pass in the parameters as a list, rather than as a string, it will take care of doing all the necessary and risky escaping:

subprocess.Popen(["mycommand", "-test-arg=yes"], shell=False)
# Instead of
subprocess.Popen("mycommand -test-arg=yes", shell=False)

However, I'm not 100% sure, it's been a while since I used subprocess. I can look into it once I test this feature more.

[deadc0de6]

Thanks, I've fixed that. I guess it should be ok since the command is passed through shlex.split. I will however document in the wiki that the diff_command should have its arguments quoted.

[Sighery]

I went through the code again and noticed these two cases in which the arguments aren't quoted, in utils.diff https://github.com/deadc0de6/dotdrop/compare/diff_command#diff-c4ad38bed18aacef0e5c926a77aad18fR74. But humour me for the rest of the post, we may be able to get rid of this requirement.

---

I've been looking into it. If you pass in your stuff as a list to subprocess, then it will do all the escaping necessary: https://docs.python.org/3.8/library/subprocess.html#frequently-used-arguments

I've tested this by:

echo "test1" > "t - t1.test"
echo "test2" > 't"t2.test'
diff -u 't - t1.test' 't"t2.test'

And I have the following Python script:

import sys
import subprocess

print(sys.argv)
try:
    print(subprocess.check_output(["diff", "-u", sys.argv[1], sys.argv[2]], shell=False, stderr=subprocess.STDOUT))
except subprocess.CalledProcessError as exc:
    print(exc)
    print(exc.output)

That I call using: python test.py t\ -\ t1.test t\"t2.test

(except is because diff exits with 1 when files are changed).

This will work. The issue with shlex is that, once you replace the format placeholders in your diff_cmd string, it no longer has any context of where the arguments start and end. You can somewhat solve this by quoting. But this is unreliable. The filename in Linux could contain your quote character, either ' or ", which then would break shlex again since it would think the filepath ends before it actually does.

Why it works on my small Python test file is because subprocess already has the separation by arguments it needs to be able to escape properly. As in, it knows sys.argv[1] is supposed to be a single argument, so if it has any characters that would make it not be a single argument, like spaces, and such, it will escape them.

So, easy solution I see for this: diff_cmd.split(), and then manually go over the list and fill in the format placeholders by the number they have inside the format placeholder (so {0} first argument, {1} second). Maybe the following inside utils.diff?

replacements = {
    "{0}": original,
    "{1}": modified,
}
cmd = [replacements.get(x, x) for x in diff_cmd.split()]
_, out = run(cmd, raw=raw, debug=debug)

With this change the quoting would no longer be necessary.

[deadc0de6]

Thanks for pointing that out, I indeed forgot to quote the arguments in the utils.diff function, sorry. Was however unable to make your solution work on dotdrop. If I understand right, this would support filenames with quotes in them?

[Sighery]

Strange, works for me. Let me send you a PR to this branch, since that'd probably be the fastest way to explain it.

[deadc0de6]

I have merged your changes. I will soon merge that into master and it will be part of the next release. Thanks again for your help!

[Sighery]

Quick question, I noticed you defaulted utils.diff to the unified format (diff -u) in this commit. However, this will always be overwritten by this line in settings.py that uses the default, non-unified format.

[deadc0de6]

you're absolutely right, changing that right now!