[PATCH] D65697: [lit] Fix internal env calling internal commands

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

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


================
Comment at: llvm/utils/lit/lit/TestRunner.py:247
         # Support for the -u flag (unsetting) for env command
         # e.g., env -u FOO -u BAR will remove both FOO and BAR
         # from the environment.
----------------
rnk wrote:
> Hm, I bet we shouldn't handle `-u` for `export`.
Yeah, that seems wrong.  Fixing that surely belongs in a separate patch though.  Agreed?

Thanks for the review.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:803
+
+    # Extract env commands and their arguments so we don't overlook any shell
+    # builtin.  Don't modify the original args as we'll need to process the
----------------
rnk wrote:
> I don't like the way we have to process env prefixes twice, once for in-process shell builtins and once for out-of-process builtins. I think we should try to make things more uniform by moving the in-process builtins into the loop between 'env' handling and processRedirects. I'd suggest reducing the boilerplate by building a set of in-process builtins (`inproc_builtins`?), checking against the set, and then calling a helper function that returns a ShellCommandResult or throws. You can do the check for "cannot be part of a pipeline" in the shared code and raise the same InternalShellErrors. Does that seem straightforward?
> 
> Speaking of which, I see that `diff` actually can take one of the files to compare on stdin, so technically we should move it out of process.
> Does that seem straightforward?

I'll give it a shot.

By the way, a lot of the `ValueError`s here really should be `InternalShellError`s, right?

> Speaking of which, I see that diff actually can take one of the files to compare on stdin, so technically we should move it out of process.

Sorry, I'm not following.  Would that be an extension to what's implemented now for lit's internal diff?


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

https://reviews.llvm.org/D65697