Prefer `diff -u` to `tkdiff` as the default when running buildifier on a single file from the command line

[bolinfest]

I believe the relevant code is here:

https://github.com/bazelbuild/buildifier/blob/7679b82ab766294a27b3b439160277286d8efa1d/differ/diff.go#L96-L113

I suspect tkdiff is installed by default at Google on Goobuntu, but tkdiff isn't available by default on OS X, and I'm pretty sure it isn't on Windows (or even vanilla Ubuntu, for that matter).

I think that diff -u is a better default because the output from regular diff does not provide enough information when you pass multiple files to buildifier. Currently, I am running buildifier with BUILDIFIER_DIFF="diff -u" to achieve this, but again, I think it should be the default.

For the code that I highlighted, I believe what's happening is:

• knowMultiDiff := false on line 96.
• if !knowMultiDiff { is true on line 109.
• d.MultiDiff = isatty(1) && os.Getenv("DISPLAY") != "" on 110, such that d.MultiDiff is true.
• if d.MultiDiff { is true on line 112, so d.Cmd = "tkdiff" on line 113.

[aljabrr]

@bolinfest I want to work on this. Could you please give me more information on the current status of this issue?

[gertvdijk]

This issue is still present and bugging me. See also #124 (possible dupe?). If anyone @ Google could shed a light on what's the background of the dependency on a custom tkdiff and how we can safely detect this, that would be wonderful.

FWIW, it makes sense to me to either:

• Make diff -u the default. This would also display filenames already and makes commit 41d89cd7c8328bb912f3b8f50d2dc970805d21f8 unnecessary actually.
• Try to execute the original multidiff-tkdiff command, and if it fails, fall back to diff -u.
• Allow users to set their own diff command in the rule. E.g. https://github.com/gertvdijk/buildtools/compare/master...gertvdijk:buildifier-rules-add-diff-cmd-args - I could create a PR if that's a direction we would like take. Oh, wow, I just saw #495 with another approach, using args. Perhaps we would the combination of both?

^{And perhaps a check like isatty(1) && os.Getenv("DISPLAY") != "" to include --color=always (as the default, auto fails for me when run using Bazel).}

[brown]

I would prefer the default diff behavior to remain "diff" not "diff -u".

[gertvdijk]

@brown

I would prefer the default diff behavior to remain "diff" not "diff -u".

Okay, but any compelling reason? My reasons to prefer unified diff:

• It provides context lines. If Buildifier tells you to remove three lines, the non-unified diff just shows three <, each on a line. which isn't very useful at all to locate those lines.
  ^{Well, you could get context lines in non-unified mode, but I've never seen that format used in practice at all.}
• In the past decade I've only seen tools producing unified diffs (except for diff), it seems the de facto standard for producing diffs.
  

  making unified diff format the most common format for exchange between software developers ^source

[brown]

Maybe I've been viewing unified diffs all along .... I'm happy so long as I can specify "-diff_command=diff" or "-diff_command='diff -e'" in order to get exactly the kind of diff output I want.

[sudoforge]

FWIW @brown, here's the visual difference between unified and non-unified diffs:

➜ \diff {a,b}/README.md
1c1
< hello world
---
> goodbye space


➜ \diff -u {a,b}/README.md
--- a/README.md 2020-12-02 08:34:58.417797162 -0700
+++ b/README.md 2020-12-02 08:35:06.497771771 -0700
@@ -1 +1 @@
-hello world
+goodbye space

[sudoforge]

Personally I specify diff -urN --color but that's because all of my host systems used GNU tools and I don't need to worry about any cross-compatibility issues. In a multi-user environment, especially one where multiple platforms are used, it's not "safe" to do this, and we should instead be using a bazel toolchain that builds an appropriate tool for the execution platform.

Or use something baked into buildifier.