Repository: globus/globus-cli

Issue:
globus transfer --batch: accept filename
Currently globus transfer --batch is defined to take the list of input files on stdin.  Rather than forcing stdin, why not define it as "--batch INPUTFILE", where INPUTFILE can be "-" to mean stdin?  That seems somewhat more flexible and easier, if my file list is in a file.

Discussion:
rudyardrichter: I'm going to have to do some more digging before this is actually mergeable, there seems to be something odd happening between click's `File` and batch values for Windows filepaths:
```
E           Exception: CliTest run_line exit_code assertion failed!
E           Line:
E           globus transfer -F json --batch C:\Users\RUNNER~1\AppData\Local\Temp\tmpels9pcwf --dry-run ddb59aef-6d04-11e5-ba46-22000b92c6ec ddb59af0-6d04-11e5-ba46-22000b92c6ec
E           exited with 2 when expecting 0
E           stdout:
E           
E           stderr:
E           Usage: globus transfer [OPTIONS] SOURCE_ENDPOINT_ID[:SOURCE_PATH]
E                                  DEST_ENDPOINT_ID[:DEST_PATH]
E           
E           Error: Invalid value for '--batch': 'C:UsersRUNNER~1AppDataLocalTemptmpels9pcwf': No such file or directory
```
I see there are other issues with `tempfile` on Windows, but this seems to be a quoting issue. I thought initially the POSIX setting for `shlex.split` could be the culprit, but 89d06d4 doesn't seem to have fixed it. I think we want to keep that change anyways?
sirosen: > I think we want to keep that change anyways?

I don't think so. `shlex` in non-POSIX mode has some behaviors which users are likely to find surprising, like quote character preservation.
It also means that we'd be treating the same input file differently on different platforms, which means that if you move the same "batchfile" around, you could see different behavior from the same command.

I don't think that's where the error is coming from, in any case.
The testsuite has that `run_line` fixture which wraps a click `CliRunner`. There, we're doing [`shlex.split`](https://github.com/globus/globus-cli/blob/f35b947b322f992398b5be08e393c5fed7bee586/tests/conftest.py#L133) on the input. I would bet that's what's going wrong.

One easy option would be to make `run_line` take a string or list of strings:
```diff
-        args = shlex.split(line)
+        args = shlex.split(line) if isinstance(line, str) else line
```

Switching the test to pass a list of strings would skip that call entirely.

Or we could toggle POSIX mode in `run_line` based on platform. It's not user-facing (to my thoughts above about not wanting to use it in the batch processing) and we're emulating CLI parsing on that platform, so it seems appropriate.


If I get to it, I'll try this in a branch on my fork, and tweak the workflow to just run two jobs (one linux, one windows). That should let me narrow this down and figure out where those backslashes are without waiting too much for extra jobs.
rudyardrichter: @sirosen you could take another look now; the tests work and I've cleaned up the commit history. `posix=False` caused issues in several other tests, so I went with passing a list to `run_line`.
rudyardrichter: Ok, I take that back, the Windows test is still fragile for some reason...

Patch:
diff --git a/src/globus_cli/commands/delete.py b/src/globus_cli/commands/delete.py
index cc0c055d..7b4cb2db 100644
--- a/src/globus_cli/commands/delete.py
+++ b/src/globus_cli/commands/delete.py
@@ -7,7 +7,7 @@ from globus_cli.parsing import (
     TaskPath,
     command,
     delete_and_rm_options,
-    shlex_process_stdin,
+    shlex_process_stream,
     task_submission_options,
 )
 from globus_cli.services.transfer import autoactivate, get_client
@@ -43,7 +43,7 @@ Use the batch input method to transfer multiple files and or dirs.
 [source,bash]
 ----
 $ ep_id=ddb59af0-6d04-11e5-ba46-22000b92c6ec
-$ globus delete $ep_id --batch --recursive
+$ globus delete $ep_id --batch - --recursive
 ~/myfile1.txt
 ~/myfile2.txt
 ~/myfile3.txt
@@ -97,9 +97,10 @@ def delete_command(
     single-file or single-directory delete task. This has
     behavior similar to `rm` and `rm -r`, across endpoints.
 
-    Using `--batch`, *globus delete* can submit a task which deletes
-    multiple files or directories. Lines are taken from stdin, respecting quotes,
-    and every line is treated as a path to a file or directory to delete.
+    Using `--batch`, *globus delete* can submit a task which deletes multiple files or
+    directories. The value for `--batch` can be a file to read from, or the character
+    `-` which will read from stdin. From either the file or stdin, each line is treated
+    as a path to a file or directory to delete, respecting quotes.
 
     \b
     Lines are of the form
@@ -111,9 +112,8 @@ def delete_command(
 
     Empty lines and comments beginning with '#' are ignored.
 
-    Batch only requires an ENDPOINT before passing lines, on stdin, but if you pass
-    an ENPDOINT:PATH on the original command, this path will be used as a prefixes
-    to all paths on stdin.
+    Batch only requires an ENDPOINT on the "base" command, but you may pass an
+    ENPDOINT:PATH to prefix all the paths read in the batch with that path.
 
     {AUTOMATIC_ACTIVATION}
     """
@@ -155,7 +155,9 @@ def delete_command(
             """
             delete_data.add_item(str(path))
 
-        shlex_process_stdin(process_batch_line, "Enter paths to delete, line by line.")
+        shlex_process_stream(
+            process_batch_line, batch, "Enter paths to delete, line by line."
+        )
     else:
         if not star_silent and enable_globs and path.endswith("*"):
             # not intuitive, but `click.confirm(abort=True)` prints to stdout
diff --git a/src/globus_cli/commands/transfer.py b/src/globus_cli/commands/transfer.py
index 5ef2ff7f..40036019 100644
--- a/src/globus_cli/commands/transfer.py
+++ b/src/globus_cli/commands/transfer.py
@@ -7,7 +7,7 @@ from globus_cli.parsing import (
     TaskPath,
     command,
     mutex_option_group,
-    shlex_process_stdin,
+    shlex_process_stream,
     task_submission_options,
 )
 from globus_cli.services.transfer import autoactivate, get_client
@@ -41,7 +41,7 @@ Use the batch input method to transfer multiple files and directories:
 ----
 $ source_ep=ddb59aef-6d04-11e5-ba46-22000b92c6ec
 $ dest_ep=ddb59af0-6d04-11e5-ba46-22000b92c6ec
-$ globus transfer $source_ep $dest_ep --batch
+$ globus transfer $source_ep $dest_ep --batch -
 # lines starting with '#' are comments
 # and blank lines (for spacing) are allowed
 
@@ -64,7 +64,7 @@ above, but much more concise):
 ----
 $ source_ep=ddb59aef-6d04-11e5-ba46-22000b92c6ec
 $ dest_ep=ddb59af0-6d04-11e5-ba46-22000b92c6ec
-$ globus transfer $source_ep:/share/ $dest_ep:~/ --batch
+$ globus transfer $source_ep:/share/ $dest_ep:~/ --batch -
 godata/file1.txt myfile1.txt
 godata/file2.txt myfile2.txt
 godata/file3.txt myfile3.txt
@@ -93,7 +93,7 @@ dest_ep=ddb59af0-6d04-11e5-ba46-22000b92c6ec
 
 task_id="$(globus transfer $source_ep $dest_ep \
     --jmespath 'task_id' --format=UNIX \
-    --batch < my_file_batch.txt)"
+    --batch my_file_batch.txt)"
 
 echo "Waiting on 'globus transfer' task '$task_id'"
 globus task wait "$task_id" --timeout 30
@@ -152,13 +152,13 @@ fi
 )
 @click.option(
     "--batch",
-    is_flag=True,
+    type=click.File("r"),
     help=(
-        "Accept a batch of source/dest path pairs on stdin (i.e. "
-        "run in batchmode). "
-        "Uses SOURCE_ENDPOINT_ID and DEST_ENDPOINT_ID as passed "
-        "on the commandline. Commandline paths are still allowed "
-        "and are used as prefixes to the batchmode inputs."
+        "Accept a batch of source/dest path pairs from a file. Use the special `-` "
+        "value to read from stdin; otherwise opens the file from the argument and "
+        "passes through lines from that file. Uses SOURCE_ENDPOINT_ID and "
+        "DEST_ENDPOINT_ID as passed on the commandline. Commandline paths are still "
+        "allowed and are used as prefixes to the batchmode inputs."
     ),
 )
 @click.option(
@@ -271,10 +271,10 @@ def transfer_command(
     will submit a single-file or single-directory transfer task.
     This has behavior similar to `cp` and `cp -r` across endpoints.
 
-    Using `--batch`, `globus transfer` can submit a task which transfers
-    multiple files or directories. Paths to transfer are taken from stdin.
-    Lines are split on spaces, respecting quotes, and every line is treated as
-    a file or directory to transfer.
+    Using `--batch`, `globus transfer` can submit a task which transfers multiple files
+    or directories. The value for `--batch` can be a file to read from, or the
+    character `-` which will read from stdin. From either the file or stdin, each line
+    is treated as a path to a file or directory to delete, respecting quotes.
 
     \b
     Lines are of the form
@@ -284,7 +284,7 @@ def transfer_command(
 
     \b
     If you use `--batch` and a commandline SOURCE_PATH and/or DEST_PATH, these
-    paths will be used as dir prefixes to any paths on stdin.
+    paths will be used as dir prefixes to any paths read from the batch source.
 
     \b
     === Sync Levels
@@ -396,13 +396,15 @@ def transfer_command(
                 recursive=recursive,
             )
 
-        shlex_process_stdin(
+        shlex_process_stream(
             process_batch_line,
+            batch,
             (
                 "Enter transfers, line by line, as\n\n"
                 "    [--recursive] [--external-checksum TEXT] SOURCE_PATH DEST_PATH\n"
             ),
         )
+
     else:
         transfer_data.add_item(
             cmd_source_path,
diff --git a/src/globus_cli/parsing/__init__.py b/src/globus_cli/parsing/__init__.py
index a3c5c33a..05f5a9f2 100644
--- a/src/globus_cli/parsing/__init__.py
+++ b/src/globus_cli/parsing/__init__.py
@@ -2,7 +2,7 @@ from globus_cli.parsing.commands import command, group, main_group
 from globus_cli.parsing.explicit_null import EXPLICIT_NULL
 from globus_cli.parsing.mutex_group import MutexInfo, mutex_option_group
 from globus_cli.parsing.one_use_option import one_use_option
-from globus_cli.parsing.process_stdin import shlex_process_stdin
+from globus_cli.parsing.process_stdin import shlex_process_stream
 from globus_cli.parsing.shared_options import (
     collection_id_arg,
     delete_and_rm_options,
@@ -46,5 +46,5 @@ __all__ = [
     "synchronous_task_wait_options",
     "security_principal_opts",
     "no_local_server_option",
-    "shlex_process_stdin",
+    "shlex_process_stream",
 ]
diff --git a/src/globus_cli/parsing/process_stdin.py b/src/globus_cli/parsing/process_stdin.py
index 3d05450c..d7e311c2 100644
--- a/src/globus_cli/parsing/process_stdin.py
+++ b/src/globus_cli/parsing/process_stdin.py
@@ -4,7 +4,7 @@ import sys
 import click
 
 
-def shlex_process_stdin(process_command, helptext):
+def shlex_process_stream(process_command, stream, helptext):
     """
     Use shlex to process stdin line-by-line.
     Also prints help text.
@@ -28,7 +28,7 @@ Terminate input with Ctrl+D or <EOF>
     # use readlines() rather than implicit file read line looping to force
     # python to properly capture EOF (otherwise, EOF acts as a flush and
     # things get weird)
-    for line in sys.stdin.readlines():
+    for line in stream.readlines():
         # get the argument vector:
         # do a shlex split to handle quoted paths with spaces in them
         # also lets us have comments with #
diff --git a/src/globus_cli/parsing/shared_options.py b/src/globus_cli/parsing/shared_options.py
index d2029af4..d1590263 100644
--- a/src/globus_cli/parsing/shared_options.py
+++ b/src/globus_cli/parsing/shared_options.py
@@ -240,12 +240,14 @@ def delete_and_rm_options(
     if supports_batch:
         f = click.option(
             "--batch",
-            is_flag=True,
+            type=click.File("r"),
             help=(
-                "Accept a batch of paths on stdin (i.e. run in "
-                "batchmode). Uses ENDPOINT_ID as passed on the "
-                "commandline. Any commandline PATH given will be used "
-                "as a prefix to all paths given"
+                "Accept a batch of source/dest path pairs from a file. Use the "
+                " special `-` value to read from stdin; otherwise opens the file from "
+                "the argument and passes through lines from that file. Uses "
+                "SOURCE_ENDPOINT_ID and DEST_ENDPOINT_ID as passed on the commandline. "
+                "Commandline paths are still allowed and are used as prefixes to the "
+                "batchmode inputs. "
             ),
         )(f)
     return f


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index 46f21333..ac3d8bef 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -130,7 +130,7 @@ def run_line(cli_runner, request, patch_tokenstorage):
         from globus_cli import main
 
         # split line into args and confirm line starts with "globus"
-        args = shlex.split(line)
+        args = shlex.split(line) if isinstance(line, str) else line
         assert args[0] == "globus"
 
         # run the line. globus_cli.main is the "globus" part of the line
diff --git a/tests/functional/test_basics.py b/tests/functional/test_basics.py
index cdec5d7e..1fbc460f 100644
--- a/tests/functional/test_basics.py
+++ b/tests/functional/test_basics.py
@@ -138,7 +138,7 @@ def test_transfer_call(run_line, load_api_fixtures, register_api_route, go_ep1_i
     assert "home/" in result.output
 
 
-def test_transfer_batchmode_dryrun(run_line, load_api_fixtures, go_ep1_id, go_ep2_id):
+def test_transfer_batch_stdin_dryrun(run_line, load_api_fixtures, go_ep1_id, go_ep2_id):
     """
     Dry-runs a transfer in batchmode, confirms batchmode inputs received
     """
@@ -148,7 +148,7 @@ def test_transfer_batchmode_dryrun(run_line, load_api_fixtures, go_ep1_id, go_ep
 
     batch_input = "abc /def\n/xyz p/q/r\n"
     result = run_line(
-        "globus transfer -F json --batch --dry-run " + go_ep1_id + " " + go_ep2_id,
+        "globus transfer -F json --batch - --dry-run " + go_ep1_id + " " + go_ep2_id,
         stdin=batch_input,
     )
     for src, dst in [("abc", "/def"), ("/xyz", "p/q/r")]:
@@ -156,6 +156,32 @@ def test_transfer_batchmode_dryrun(run_line, load_api_fixtures, go_ep1_id, go_ep
         assert f'"destination_path": "{dst}"' in result.output
 
 
+def test_transfer_batch_file_dryrun(
+    run_line, load_api_fixtures, go_ep1_id, go_ep2_id, tmp_path
+):
+    # put a submission ID and autoactivate response in place
+    load_api_fixtures("get_submission_id.yaml")
+    load_api_fixtures("transfer_activate_success.yaml")
+    temp = tmp_path / "batch"
+    temp.write_text("abc /def\n/xyz p/q/r\n")
+    result = run_line(
+        [
+            "globus",
+            "transfer",
+            "-F",
+            "json",
+            "--batch",
+            temp,
+            "--dry-run",
+            go_ep1_id,
+            go_ep2_id,
+        ]
+    )
+    for src, dst in [("abc", "/def"), ("/xyz", "p/q/r")]:
+        assert f'"source_path": "{src}"' in result.output
+        assert f'"destination_path": "{dst}"' in result.output
+
+
 def test_delete_batchmode_dryrun(run_line, load_api_fixtures, go_ep1_id):
     """
     Dry-runs a delete in batchmode
@@ -165,7 +191,9 @@ def test_delete_batchmode_dryrun(run_line, load_api_fixtures, go_ep1_id):
     load_api_fixtures("transfer_activate_success.yaml")
 
     batch_input = "abc/def\n/xyz\nabcdef\nabc/def/../xyz\n"
-    result = run_line("globus delete --batch --dry-run " + go_ep1_id, stdin=batch_input)
+    result = run_line(
+        "globus delete --batch - --dry-run " + go_ep1_id, stdin=batch_input
+    )
     assert (
         "\n".join(
             ("Path   ", "-------", "abc/def", "/xyz   ", "abcdef ", "abc/xyz", "")
@@ -175,7 +203,7 @@ def test_delete_batchmode_dryrun(run_line, load_api_fixtures, go_ep1_id):
 
     batch_input = "abc/def\n/xyz\n../foo\n"
     result = run_line(
-        f"globus delete --batch --dry-run {go_ep1_id}:foo/bar/./baz",
+        f"globus delete --batch - --dry-run {go_ep1_id}:foo/bar/./baz",
         stdin=batch_input,
     )
     assert (
diff --git a/tests/functional/test_task_submit.py b/tests/functional/test_task_submit.py
index 020c9c50..3a12985c 100644
--- a/tests/functional/test_task_submit.py
+++ b/tests/functional/test_task_submit.py
@@ -32,22 +32,43 @@ def test_exlude_recursive(run_line, load_api_fixtures, go_ep1_id, go_ep2_id):
     # would be better if this could fail before we make any api calls, but
     # we want to build the transfer_data object before we parse batch input
     load_api_fixtures("get_submission_id.yaml")
-
-    expected_error = "--exclude can only be used with --recursive transfers"
-
-    # single
     result = run_line(
         "globus transfer --exclude *.txt " "{}:/ {}:/".format(go_ep1_id, go_ep1_id),
         assert_exit_code=2,
     )
-    assert expected_error in result.stderr
+    assert "--exclude can only be used with --recursive transfers" in result.stderr
+
 
-    # batch
-    batch_input = "abc /def\n"
+def test_exlude_recursive_batch_stdin(
+    run_line, load_api_fixtures, go_ep1_id, go_ep2_id
+):
+    load_api_fixtures("get_submission_id.yaml")
     result = run_line(
-        "globus transfer --exclude *.txt --batch "
+        "globus transfer --exclude *.txt --batch - "
         "{}:/ {}:/".format(go_ep1_id, go_ep1_id),
-        stdin=batch_input,
+        stdin="abc /def\n",
+        assert_exit_code=2,
+    )
+    assert "--exclude can only be used with --recursive transfers" in result.stderr
+
+
+def test_exlude_recursive_batch_file(
+    run_line, load_api_fixtures, go_ep1_id, go_ep2_id, tmp_path
+):
+    load_api_fixtures("get_submission_id.yaml")
+    temp = tmp_path / "batch"
+    temp.write_text("abc /def\n")
+    result = run_line(
+        [
+            "globus",
+            "transfer",
+            "--exclude",
+            "*.txt",
+            "--batch",
+            temp,
+            f"{go_ep1_id}:/",
+            f"{go_ep1_id}:/",
+        ],
         assert_exit_code=2,
     )
-    assert expected_error in result.stderr
+    assert "--exclude can only be used with --recursive transfers" in result.stderr