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

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

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


================
Comment at: llvm/utils/lit/lit/TestRunner.py:810
+    cmd0_args_pruned = list(cmd.commands[0].args)
+    while len(cmd0_args_pruned) and cmd0_args_pruned[0] == 'env':
+        cmd0_args_pruned = updateEnv(None, cmd0_args_pruned)
----------------
mgorny wrote:
> You don't need `len()` here, list will evaluate to whether it's non-empty in boolean context.
OK, I thought `len` was more explicit, but I'm not an experienced python programmer.  I'll change it.

Thanks for the review.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:814
+
+    if cmd0_args_pruned[0] == 'cd':
+        if cmd0_has_envs:
----------------
mgorny wrote:
> You are not checking whether it's empty here. I guess this means it's gonna crash if you pass just `env`.
Good catch.  It turns out lit's `env` had that problem even without this patch, but that bug is later.  I suppose there's no reason I shouldn't fix both as part of this patch.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:836
     # temporary file to sidestep blocking pipe write issues.
-    if cmd.commands[0].args[0] == 'echo' and len(cmd.commands) == 1:
+    if cmd0_args_pruned[0] == 'echo' and len(cmd.commands) == 1:
+        if cmd0_has_envs:
----------------
mgorny wrote:
> The RHS of this condition is verifying the original command, so I suppose it's never going to trigger with `env`.
By RHS, I think you mean `len(cmd.commands) == 1`.  It checks `cmd.commands` not `cmd.commands[0].args`, but `env` is pruned from the latter not the former.  The RHS checks if there's a pipeline not how many args there are in the first command from the pipeline, so I think this is right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65697