Allow downloading of series patches to separate files

* Add '--separate' option to download each patch separately

  Add '--separate' and '--combined' option to download
  command of a series to download each patch in a separate
  file or all into one file. 'combined' is the default.

* Change the allow 'output' arg to be a directory when downloading.

  In the download commands, allow 'output' argument to be
  a directory. In that case, the file name will be set by
  the patch/series/bundle subject and wiil be downloaded
  into the given 'output' directory.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Co-authored-by: Stephen Finucane <stephen@that.guru>

Author: Dafna Hirschfeld

git_pw/api.py

@@ -5,7 +5,6 @@
 from functools import update_wrapper
 import logging
 import os.path
-import pty
 import re
 import sys
 import tempfile
@@ -209,7 +208,7 @@ def version() -> ty.Tuple[int, int]:
 
 
 def download(
-    url: str, params: Filters = None, output: ty.IO = None,
+    url: str, params: Filters = None, output: ty.Optional[str] = None,
 ) -> ty.Optional[str]:
     """Retrieve a specific API resource and save it to a file/stdout.
 
@@ -219,8 +218,10 @@ def download(
     Arguments:
         url: The resource URL.
         params: Additional parameters.
-        output: The output file. If provided, the caller is responsible for
-            closing. If None, a temporary file will be used.
+        output: The output file. If output is a directory then
+            the file name will be according to the patch subject and
+            will be downloaded into the output directory.
+            If None, a temporary file will be used.
 
     Returns:
         A path to an output file containing the content, else None if stdout
@@ -236,24 +237,27 @@ def download(
         LOG.error('Filename was expected but was not provided in response')
         sys.exit(1)
 
-    if output:
-        output_path = None
-        if output.fileno() != pty.STDOUT_FILENO:
-            LOG.debug('Saving to %s', output.name)
-            output_path = output.name
+    if output == '-':
+        output_path = output
+        output_file = sys.stdout.buffer
+    else:
+        if output:
+            output_path = output
+            if os.path.isdir(output):
+                output_path = os.path.join(output, header.group(1))
+        else:
+            output_path = os.path.join(
+                tempfile.mkdtemp(prefix='git-pw'), header.group(1),
+            )
+        LOG.debug('Saving to %s', output_path)
+        output_file = open(output_path, 'wb')
 
+    try:
         # we use iter_content because patches can be binary
         for block in rsp.iter_content(1024):
-            output.write(block)
-    else:
-        output_path = os.path.join(
-            tempfile.mkdtemp(prefix='git-pw'), header.group(1),
-        )
-        with open(output_path, 'wb') as output_file:
-            LOG.debug('Saving to %s', output_path)
-            # we use iter_content because patches can be binary
-            for block in rsp.iter_content(1024):
-                output_file.write(block)
+            output_file.write(block)
+    finally:
+        output_file.close()
 
     return output_path
 

git_pw/bundle.py

@@ -60,13 +60,17 @@ def apply_cmd(bundle_id: str, args: ty.Tuple[str]) -> None:
 
 @click.command(name='download')
 @click.argument('bundle_id')
-@click.argument('output', type=click.File('wb'), required=False)
-def download_cmd(bundle_id: str, output: ty.IO) -> None:
+@click.argument(
+    'output',
+    type=click.Path(file_okay=True, writable=True, readable=True),
+    required=False,
+)
+def download_cmd(bundle_id: str, output: ty.Optional[str]) -> None:
     """Download bundle in mbox format.
 
     Download a bundle but do not apply it. ``OUTPUT`` is optional and can be an
-    output path or ``-`` to output to ``stdout``. If ``OUTPUT`` is not
-    provided, the output path will be automatically chosen.
+    output full file path or a directory or ``-`` to output to ``stdout``. If
+    ``OUTPUT`` is not provided, the output path will be automatically chosen.
     """
     LOG.debug('Downloading bundle: id=%s', bundle_id)
 

git_pw/patch.py

@@ -3,8 +3,8 @@
 """
 
 import logging
-import pty
 import sys
+import os
 
 import arrow
 import click
@@ -58,7 +58,11 @@ def apply_cmd(patch_id, series, deps, args):
 
 @click.command(name='download')
 @click.argument('patch_id', type=click.INT)
-@click.argument('output', type=click.File('wb'), required=False)
+@click.argument(
+    'output',
+    type=click.Path(file_okay=True, writable=True, readable=True),
+    required=False
+)
 @click.option('--diff', 'fmt', flag_value='diff',
               help='Show patch in diff format.')
 @click.option('--mbox', 'fmt', flag_value='mbox', default=True,
@@ -67,19 +71,24 @@ def download_cmd(patch_id, output, fmt):
     """Download patch in diff or mbox format.
 
     Download a patch but do not apply it. ``OUTPUT`` is optional and can be an
-    output path or ``-`` to output to ``stdout``. If ``OUTPUT`` is not
-    provided, the output path will be automatically chosen.
+    output file path or a directory or ``-`` to output to ``stdout``. If
+    ``OUTPUT`` is not provided, the output path will be automatically chosen.
     """
     LOG.debug('Downloading patch: id=%d, format=%s', patch_id, fmt)
 
     path = None
     patch = api.detail('patches', patch_id)
 
     if fmt == 'diff':
-        if output:
-            output.write(patch['diff'])
-            if output.fileno() != pty.STDOUT_FILENO:
-                path = output.name
+        if output and not os.path.isdir(output):
+            if output == '-':
+                output_path = 0  # stdout fd
+            else:
+                output_path = output
+                path = output
+
+            with open(output_path, 'w') as output_file:
+                output_file.write(utils.ensure_str(patch['diff']))
         else:
             # TODO(stephenfin): We discard the 'diff' field so we can get the
             # filename and save to the correct file. We should expose this

git_pw/series.py

@@ -9,6 +9,8 @@
 
 from git_pw import api
 from git_pw import utils
+import os.path
+import sys
 
 LOG = logging.getLogger(__name__)
 
@@ -37,8 +39,20 @@ def apply_cmd(series_id, args):
 
 @click.command(name='download')
 @click.argument('series_id', type=click.INT)
-@click.argument('output', type=click.File('wb'), required=False)
-def download_cmd(series_id, output):
+@click.argument(
+    'output',
+    type=click.Path(file_okay=True, writable=True, readable=True),
+    required=False,
+)
+@click.option(
+    '--separate', 'fmt', flag_value='separate',
+    help='Download each series patch to a separate file',
+)
+@click.option(
+    '--combined', 'fmt', flag_value='combined', default=True,
+    help='Download all series patches to one file',
+)
+def download_cmd(series_id, output, fmt):
     """Download series in mbox format.
 
     Download a series but do not apply it. ``OUTPUT`` is optional and can be an
@@ -50,6 +64,23 @@ def download_cmd(series_id, output):
     path = None
     series = api.detail('series', series_id)
 
+    if fmt == 'separate':
+        if output and not os.path.isdir(output):
+            LOG.error(
+                'When downloading into separate files, OUTPUT can only be a '
+                'directoy'
+            )
+            sys.exit(1)
+
+        for patch in series.get('patches'):
+            path = api.download(patch['mbox'], output=output)
+            if path:
+                LOG.info(
+                    'Downloaded patch %s from series %s to %s',
+                    patch.get('id'), series.get('id'), path,
+                )
+        return
+
     path = api.download(series['mbox'], output=output)
 
     if path:

tests/test_bundle.py

@@ -1,7 +1,6 @@
 import unittest
 
 from click.testing import CliRunner as CLIRunner
-from click import utils as click_utils
 import mock
 
 from git_pw import bundle
@@ -125,7 +124,7 @@ def test_download_to_file(self, mock_download, mock_get_bundle):
         mock_get_bundle.assert_called_once_with('123')
         mock_download.assert_called_once_with(rsp['mbox'], output=mock.ANY)
         assert isinstance(
-            mock_download.call_args[1]['output'], click_utils.LazyFile,
+            mock_download.call_args[1]['output'], str,
         )
 
 

tests/test_patch.py

@@ -2,7 +2,6 @@
 
 import click
 from click.testing import CliRunner as CLIRunner
-from click import utils as click_utils
 import mock
 from packaging import version
 
@@ -126,7 +125,7 @@ def test_download_to_file(self, mock_log, mock_download, mock_detail):
         mock_detail.assert_called_once_with('patches', 123)
         mock_download.assert_called_once_with(rsp['mbox'], output=mock.ANY)
         assert isinstance(
-            mock_download.call_args[1]['output'], click_utils.LazyFile,
+            mock_download.call_args[1]['output'], str,
         )
         assert mock_log.info.called
 

tests/test_series.py

@@ -1,7 +1,6 @@
 import unittest
 
 from click.testing import CliRunner as CLIRunner
-from click import utils as click_utils
 import mock
 
 from git_pw import series
@@ -74,7 +73,34 @@ def test_download_to_file(self, mock_download, mock_detail):
         mock_detail.assert_called_once_with('series', 123)
         mock_download.assert_called_once_with(rsp['mbox'], output=mock.ANY)
         assert isinstance(
-            mock_download.call_args[1]['output'], click_utils.LazyFile,
+            mock_download.call_args[1]['output'], str,
+        )
+
+    def test_download_separate_to_dir(self, mock_download, mock_detail):
+        """Validate downloading seperate to a directory."""
+
+        rsp = {
+            'mbox': 'http://example.com/api/patches/123/mbox/',
+            'patches': [
+                {
+                    'id': 10539359,
+                    'mbox': 'https://example.com/project/foo/patch/123/mbox/',
+                }
+
+            ]
+        }
+        mock_detail.return_value = rsp
+
+        runner = CLIRunner()
+        result = runner.invoke(series.download_cmd, ['123', '--separate', '.'])
+
+        assert result.exit_code == 0, result
+        mock_detail.assert_called_once_with('series', 123)
+        mock_download.assert_called_once_with(
+            rsp['patches'][0]['mbox'], output=mock.ANY,
+        )
+        assert isinstance(
+            mock_download.call_args[1]['output'], str,
         )