[PATCH] D66574: [lit] Make internal diff work in pipelines

[Joel E. Denny via Phabricator via llvm-commits]

jdenny marked 2 inline comments as done.
jdenny added a comment.

In D66574#1641050 <https://reviews.llvm.org/D66574#1641050>, @probinson wrote:

> +Maggie who did the original diff implementation, AFAICT.


Good idea.

Thanks for the review.



================
Comment at: llvm/utils/lit/lit/builtin_commands/diff.py:196
+    try:
+        opts, args = getopt.gnu_getopt(args, "wbur", ["strip-trailing-cr"])
+    except getopt.GetoptError as err:
----------------
probinson wrote:
> I've also seen "a" and "U1" in my searches.
> Does this correctly handle combined short options? e.g. "-aub" which I see used a fair amount.
> I didn't notice `--strip-trailing-cr` anywhere but presumably the original author needed it, so probably should keep it.
> I was going to say we don't need to bother with "r" but then I found a case that used it!
> I've also seen "a" and "U1" in my searches.

Looks like those will be easy to implement.  `difflib` appears to provide functionality for the latter.  I'll work on them.

> Does this correctly handle combined short options? e.g. "-aub" which I see used a fair amount.

I believe so, but I'll add some tests to be sure.

Thanks for researching the current usage!


================
Comment at: llvm/utils/lit/tests/shtest-shell.py:7
 # RUN: cat %t.out
-# RUN: FileCheck --input-file %t.out %s
+# RUN: FileCheck -dump-input=fail -vv -color --input-file %t.out %s
 #
----------------
probinson wrote:
> Is this for your temporary benefit or did you intend to make this change permanent? "-color" in particular seems odd here.
Thanks for catching that.  Sorry, I meant to remove it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66574/new/

https://reviews.llvm.org/D66574