From a8de97c32719792980f7c07776a49d7535a70cfd Mon Sep 17 00:00:00 2001
From: Olivier Mangin <olivier@mangin.com>
Date: Thu, 9 Aug 2018 19:00:01 +0200
Subject: [PATCH 1/5] Fixes #144: behavior of add_copy mode during add.

- correctly handles add_copy mode and configuration,
- add option top force 'copy' mode (since the default had been changed
in between from 'copy' to 'move' this is now needed),
- fixes assumption in one test that the default is 'copy' (in other
words the test was broken and ensuring that the functionality was broken
too.),
- do not try to delete the source when it is an URL.
---
 pubs/commands/add_cmd.py | 42 +++++++++++++++------------
 tests/test_usecase.py    | 63 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 83 insertions(+), 22 deletions(-)

diff --git a/pubs/commands/add_cmd.py b/pubs/commands/add_cmd.py
index d193b5e..9487d05 100644
--- a/pubs/commands/add_cmd.py
+++ b/pubs/commands/add_cmd.py
@@ -34,10 +34,19 @@ def parser(subparsers, conf):
                         ).completer = CommaSeparatedTagsCompletion(conf)
     parser.add_argument('-k', '--citekey', help='citekey associated with the paper;\nif not provided, one will be generated automatically.',
                         default=None, type=p3.u_maybe)
-    parser.add_argument('-L', '--link', action='store_false', dest='copy', default=True,
-                        help="don't copy document files, just create a link.")
-    parser.add_argument('-M', '--move', action='store_true', dest='move', default=False,
-                        help="move document instead of of copying (ignored if --link).")
+    doc_add_group = parser.add_mutually_exclusive_group()
+    doc_add_group.add_argument(
+        '-L', '--link', action='store_const', dest='doc_add', const='link',
+        default=None,
+        help="don't copy document files, just create a link.")
+    doc_add_group.add_argument(
+        '-M', '--move', action='store_const', dest='doc_add', const='move',
+        default=None,
+        help="move document instead of of copying.")
+    doc_add_group.add_argument(
+        '-C', '--copy', action='store_const', dest='doc_add', const='copy',
+        default=None,
+        help="copy document instead of of move.")
     return parser
 
 
@@ -136,25 +145,20 @@ def command(conf, args):
                     '{}, using {} instead.').format(bib_docfile, docfile))
 
     # create the paper
-    copy = args.copy
-    if copy is None:
-        copy = conf['main']['doc_add'] in ('copy', 'move')
-    move = args.move
-    if move is None:
-        move = conf['main']['doc_add'] == 'move'
+    doc_add = args.doc_add
+    if doc_add is None:
+        doc_add = conf['main']['doc_add']
 
     rp.push_paper(p)
     ui.message('added to pubs:\n{}'.format(pretty.paper_oneliner(p)))
     if docfile is not None:
-        rp.push_doc(p.citekey, docfile, copy=copy or args.move)
-        if copy:
-            if move:
-                content.remove_file(docfile)
-
-        if copy:
-            if move:
-                ui.message('{} was moved to the pubs repository.'.format(docfile))
-            else:
+        rp.push_doc(p.citekey, docfile, copy=(doc_add in ('copy', 'move')))
+        if doc_add == 'move' and content.content_type(docfile) != 'url':
+            content.remove_file(docfile)
+
+        if doc_add == 'move':
+            ui.message('{} was moved to the pubs repository.'.format(docfile))
+        elif doc_add == 'copy':
                 ui.message('{} was copied to the pubs repository.'.format(docfile))
 
     rp.close()
diff --git a/tests/test_usecase.py b/tests/test_usecase.py
index 43cbf2c..fb8a109 100644
--- a/tests/test_usecase.py
+++ b/tests/test_usecase.py
@@ -292,9 +292,66 @@ class TestAdd(URLContentTestCase):
                 'pubs add data/pagerank.bib --link -d data/pagerank.pdf',
                 ]
         self.execute_cmds(cmds)
-        self.assertEqual(os.listdir(
-                os.path.join(self.default_pubs_dir, 'doc')),
+        self.assertEqual(
+            os.listdir(os.path.join(self.default_pubs_dir, 'doc')),
             [])
+        self.assertTrue(os.path.exists('data/pagerank.pdf'))
+
+    def test_add_doc_nocopy_from_config_does_not_copy(self):
+        self.execute_cmds(['pubs init'])
+        config = conf.load_conf()
+        config['main']['doc_add'] = 'link'
+        conf.save_conf(config)
+        cmds = ['pubs add data/pagerank.bib -d data/pagerank.pdf']
+        self.execute_cmds(cmds)
+        self.assertEqual(
+            os.listdir(os.path.join(self.default_pubs_dir, 'doc')),
+            [])
+        self.assertTrue(os.path.exists('data/pagerank.pdf'))
+
+    def test_add_doc_copy(self):
+        cmds = ['pubs init',
+                'pubs add data/pagerank.bib --copy -d data/pagerank.pdf',
+                ]
+        self.execute_cmds(cmds)
+        self.assertEqual(
+            os.listdir(os.path.join(self.default_pubs_dir, 'doc')),
+            ['Page99.pdf'])
+        self.assertTrue(os.path.exists('data/pagerank.pdf'))
+
+    def test_add_doc_copy_from_config(self):
+        self.execute_cmds(['pubs init'])
+        config = conf.load_conf()
+        config['main']['doc_add'] = 'copy'
+        conf.save_conf(config)
+        cmds = ['pubs add data/pagerank.bib -d data/pagerank.pdf']
+        self.execute_cmds(cmds)
+        self.assertEqual(
+            os.listdir(os.path.join(self.default_pubs_dir, 'doc')),
+            ['Page99.pdf'])
+        self.assertTrue(os.path.exists('data/pagerank.pdf'))
+
+    def test_add_doc_move(self):
+        cmds = ['pubs init',
+                'pubs add data/pagerank.bib --move -d data/pagerank.pdf',
+                ]
+        self.execute_cmds(cmds)
+        self.assertEqual(
+            os.listdir(os.path.join(self.default_pubs_dir, 'doc')),
+            ['Page99.pdf'])
+        self.assertFalse(os.path.exists('data/pagerank.pdf'))
+
+    def test_add_doc_move_from_config(self):
+        self.execute_cmds(['pubs init'])
+        config = conf.load_conf()
+        config['main']['doc_add'] = 'move'
+        conf.save_conf(config)
+        cmds = ['pubs add data/pagerank.bib -d data/pagerank.pdf']
+        self.execute_cmds(cmds)
+        self.assertEqual(
+            os.listdir(os.path.join(self.default_pubs_dir, 'doc')),
+            ['Page99.pdf'])
+        self.assertFalse(os.path.exists('data/pagerank.pdf'))
 
     def test_add_move_removes_doc(self):
         cmds = ['pubs init',
@@ -611,7 +668,7 @@ class TestUsecase(DataCommandTestCase):
     def test_first(self):
         correct = ['Initializing pubs in /paper_first\n',
                    'added to pubs:\n[Page99] Page, Lawrence et al. "The PageRank Citation Ranking: Bringing Order to the Web." (1999) \n'
-                   'data/pagerank.pdf was copied to the pubs repository.\n',
+                   'data/pagerank.pdf was moved to the pubs repository.\n',
                    '[Page99] Page, Lawrence et al. "The PageRank Citation Ranking: Bringing Order to the Web." (1999) \n',
                    '\n',
                    '',

From 8eef7bd77bff8ecf5c1d1753454d74479abb3d0d Mon Sep 17 00:00:00 2001
From: Olivier Mangin <olivier@mangin.com>
Date: Thu, 9 Aug 2018 19:19:29 +0200
Subject: [PATCH 2/5] Remove code duplication for command arguments.

---
 pubs/command_utils.py       | 20 ++++++++++++++++++++
 pubs/commands/add_cmd.py    | 15 ++-------------
 pubs/commands/import_cmd.py | 37 +++++++++++++++++++++----------------
 3 files changed, 43 insertions(+), 29 deletions(-)
 create mode 100644 pubs/command_utils.py

diff --git a/pubs/command_utils.py b/pubs/command_utils.py
new file mode 100644
index 0000000..445f203
--- /dev/null
+++ b/pubs/command_utils.py
@@ -0,0 +1,20 @@
+"""Contains code that is reused over commands, like argument definition
+or help messages.
+"""
+
+
+def add_doc_add_arguments(parser, move=True):
+    doc_add_group = parser.add_mutually_exclusive_group()
+    doc_add_group.add_argument(
+        '-L', '--link', action='store_const', dest='doc_add', const='link',
+        default=None,
+        help="don't copy document files, just create a link.")
+    doc_add_group.add_argument(
+        '-C', '--copy', action='store_const', dest='doc_add', const='copy',
+        default=None,
+        help="copy document (keep source).")
+    if move:
+        doc_add_group.add_argument(
+            '-M', '--move', action='store_const', dest='doc_add', const='move',
+            default=None,
+            help="move document (copy and remove source).")
diff --git a/pubs/commands/add_cmd.py b/pubs/commands/add_cmd.py
index 9487d05..4510db7 100644
--- a/pubs/commands/add_cmd.py
+++ b/pubs/commands/add_cmd.py
@@ -12,6 +12,7 @@ from .. import apis
 from .. import pretty
 from .. import utils
 from .. import endecoder
+from ..command_utils import add_doc_add_arguments
 from ..completion import CommaSeparatedTagsCompletion
 
 
@@ -34,19 +35,7 @@ def parser(subparsers, conf):
                         ).completer = CommaSeparatedTagsCompletion(conf)
     parser.add_argument('-k', '--citekey', help='citekey associated with the paper;\nif not provided, one will be generated automatically.',
                         default=None, type=p3.u_maybe)
-    doc_add_group = parser.add_mutually_exclusive_group()
-    doc_add_group.add_argument(
-        '-L', '--link', action='store_const', dest='doc_add', const='link',
-        default=None,
-        help="don't copy document files, just create a link.")
-    doc_add_group.add_argument(
-        '-M', '--move', action='store_const', dest='doc_add', const='move',
-        default=None,
-        help="move document instead of of copying.")
-    doc_add_group.add_argument(
-        '-C', '--copy', action='store_const', dest='doc_add', const='copy',
-        default=None,
-        help="copy document instead of of move.")
+    add_doc_add_arguments(parser)
     return parser
 
 
diff --git a/pubs/commands/import_cmd.py b/pubs/commands/import_cmd.py
index fa6236e..8f11bc9 100644
--- a/pubs/commands/import_cmd.py
+++ b/pubs/commands/import_cmd.py
@@ -8,9 +8,9 @@ from .. import endecoder
 from .. import bibstruct
 from .. import color
 from ..paper import Paper
-
 from ..uis import get_ui
 from ..content import system_path, read_text_file
+from ..command_utils import add_doc_add_arguments
 
 
 _ABORT_USE_IGNORE_MSG = "Aborting import. Use --ignore-malformed to ignore."
@@ -18,18 +18,22 @@ _IGNORING_MSG = " Ignoring it."
 
 
 def parser(subparsers, conf):
-    parser = subparsers.add_parser('import',
-            help='import paper(s) to the repository')
-    parser.add_argument('bibpath',
-            help='path to bibtex, bibtexml or bibyaml file (or directory)')
-    parser.add_argument('-L', '--link', action='store_false', dest='copy', default=True,
-            help="don't copy document files, just create a link.")
-    parser.add_argument('keys', nargs='*',
-            help="one or several keys to import from the file")
-    parser.add_argument('-O', '--overwrite', action='store_true', default=False,
-            help="Overwrite keys already in the database")
-    parser.add_argument('-i', '--ignore-malformed', action='store_true', default=False,
-            help="Ignore malformed and unreadable files and entries")
+    parser = subparsers.add_parser(
+        'import',
+        help='import paper(s) to the repository')
+    parser.add_argument(
+        'bibpath',
+        help='path to bibtex, bibtexml or bibyaml file (or directory)')
+    parser.add_argument(
+        'keys', nargs='*',
+        help="one or several keys to import from the file")
+    parser.add_argument(
+        '-O', '--overwrite', action='store_true', default=False,
+        help="Overwrite keys already in the database")
+    parser.add_argument(
+        '-i', '--ignore-malformed', action='store_true', default=False,
+        help="Ignore malformed and unreadable files and entries")
+    add_doc_add_arguments(parser, move=False)
     return parser
 
 
@@ -90,9 +94,10 @@ def command(conf, args):
 
     ui = get_ui()
     bibpath = args.bibpath
-    copy = args.copy
-    if copy is None:
-        copy = conf['main']['doc_add'] in ('copy', 'move')
+    doc_add = args.doc_add
+    if doc_add is None:
+        doc_add = conf['main']['doc_add']
+    copy = doc_add in ('copy', 'move')
 
     rp = repo.Repository(conf)
     # Extract papers from bib

From 7d1c678d3d1a73bdfb29401bc3ba6842237fba73 Mon Sep 17 00:00:00 2001
From: Olivier Mangin <olivier@mangin.com>
Date: Thu, 9 Aug 2018 19:22:54 +0200
Subject: [PATCH 3/5] Adds changelog entry

---
 changelog.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/changelog.md b/changelog.md
index 314ccbd..f21ba39 100644
--- a/changelog.md
+++ b/changelog.md
@@ -29,6 +29,8 @@
 
 ### Fixed bugs
 
+- [[#144]](https://github.com/pubs/pubs/issues/144) More robust handling of the `doc_add` options [(#159)](https://github.com/pubs/pubs/pull/159)
+
 - [[#149]](https://github.com/pubs/pubs/issues/149) More robust handling of parsing and citekey errors [(#87)](https://github.com/pubs/pubs/pull/87)
 
 - [[#148]](https://github.com/pubs/pubs/issues/148) Fix compatibility with Pyfakefs 3.7 [(#151)](https://github.com/pubs/pubs/pull/151)

From 668d30ffbf7f6d1abc35658b7ea6091b0a9ac8e1 Mon Sep 17 00:00:00 2001
From: Olivier Mangin <olivier@mangin.com>
Date: Mon, 20 Aug 2018 12:14:25 +0200
Subject: [PATCH 4/5] Allows move for import on explicit option.

- uses `copy` as a default (hence no need for the option),
- does not use `doc_add` config for import.
---
 changelog.md                |  2 ++
 pubs/command_utils.py       | 18 ++++++++----------
 pubs/commands/add_cmd.py    |  6 +++---
 pubs/commands/import_cmd.py | 16 ++++++++--------
 4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/changelog.md b/changelog.md
index f21ba39..4ab4d92 100644
--- a/changelog.md
+++ b/changelog.md
@@ -8,6 +8,8 @@
 
 ### Implemented enhancements
 
+- Adds `move`, and `link` options for handling of documents during import (copy being the default). [(#159)](https://github.com/pubs/pubs/pull/159)
+
 - Better dialog after editing paper [(#142)](https://github.com/pubs/pubs/issues/142)
 
 - Add a command to open urls ([#139](https://github.com/pubs/pubs/issues/139) by [ksunden](https://github.com/ksunden))
diff --git a/pubs/command_utils.py b/pubs/command_utils.py
index 445f203..0446824 100644
--- a/pubs/command_utils.py
+++ b/pubs/command_utils.py
@@ -3,18 +3,16 @@ or help messages.
 """
 
 
-def add_doc_add_arguments(parser, move=True):
+def add_doc_copy_arguments(parser, copy=True):
     doc_add_group = parser.add_mutually_exclusive_group()
     doc_add_group.add_argument(
-        '-L', '--link', action='store_const', dest='doc_add', const='link',
+        '-L', '--link', action='store_const', dest='doc_copy', const='link',
         default=None,
         help="don't copy document files, just create a link.")
-    doc_add_group.add_argument(
-        '-C', '--copy', action='store_const', dest='doc_add', const='copy',
-        default=None,
-        help="copy document (keep source).")
-    if move:
+    if copy:
         doc_add_group.add_argument(
-            '-M', '--move', action='store_const', dest='doc_add', const='move',
-            default=None,
-            help="move document (copy and remove source).")
+            '-C', '--copy', action='store_const', dest='doc_copy', const='copy',
+            help="copy document (keep source).")
+    doc_add_group.add_argument(
+        '-M', '--move', action='store_const', dest='doc_copy', const='move',
+        help="move document (copy and remove source).")
diff --git a/pubs/commands/add_cmd.py b/pubs/commands/add_cmd.py
index 4510db7..dfa9bb1 100644
--- a/pubs/commands/add_cmd.py
+++ b/pubs/commands/add_cmd.py
@@ -12,7 +12,7 @@ from .. import apis
 from .. import pretty
 from .. import utils
 from .. import endecoder
-from ..command_utils import add_doc_add_arguments
+from ..command_utils import add_doc_copy_arguments
 from ..completion import CommaSeparatedTagsCompletion
 
 
@@ -35,7 +35,7 @@ def parser(subparsers, conf):
                         ).completer = CommaSeparatedTagsCompletion(conf)
     parser.add_argument('-k', '--citekey', help='citekey associated with the paper;\nif not provided, one will be generated automatically.',
                         default=None, type=p3.u_maybe)
-    add_doc_add_arguments(parser)
+    add_doc_copy_arguments(parser)
     return parser
 
 
@@ -134,7 +134,7 @@ def command(conf, args):
                     '{}, using {} instead.').format(bib_docfile, docfile))
 
     # create the paper
-    doc_add = args.doc_add
+    doc_add = args.doc_copy
     if doc_add is None:
         doc_add = conf['main']['doc_add']
 
diff --git a/pubs/commands/import_cmd.py b/pubs/commands/import_cmd.py
index 8f11bc9..7cbf3b6 100644
--- a/pubs/commands/import_cmd.py
+++ b/pubs/commands/import_cmd.py
@@ -7,10 +7,11 @@ from .. import repo
 from .. import endecoder
 from .. import bibstruct
 from .. import color
+from .. import content
 from ..paper import Paper
 from ..uis import get_ui
 from ..content import system_path, read_text_file
-from ..command_utils import add_doc_add_arguments
+from ..command_utils import add_doc_copy_arguments
 
 
 _ABORT_USE_IGNORE_MSG = "Aborting import. Use --ignore-malformed to ignore."
@@ -33,7 +34,7 @@ def parser(subparsers, conf):
     parser.add_argument(
         '-i', '--ignore-malformed', action='store_true', default=False,
         help="Ignore malformed and unreadable files and entries")
-    add_doc_add_arguments(parser, move=False)
+    add_doc_copy_arguments(parser, copy=False)
     return parser
 
 
@@ -94,10 +95,7 @@ def command(conf, args):
 
     ui = get_ui()
     bibpath = args.bibpath
-    doc_add = args.doc_add
-    if doc_add is None:
-        doc_add = conf['main']['doc_add']
-    copy = doc_add in ('copy', 'move')
+    doc_import = args.doc_copy or 'copy'
 
     rp = repo.Repository(conf)
     # Extract papers from bib
@@ -111,7 +109,9 @@ def command(conf, args):
         if docfile is None:
             ui.warning("No file for {}.".format(p.citekey))
         else:
-            rp.push_doc(p.citekey, docfile, copy=copy)
-            # FIXME should move the file if configured to do so.
+            rp.push_doc(p.citekey, docfile,
+                        copy=(doc_import in ('copy', 'move')))
+            if doc_import == 'move' and content.content_type(docfile) != 'url':
+                content.remove_file(docfile)
 
     rp.close()

From c2e52ec4c45afa3e9c8e081cc718deb73dc14a4a Mon Sep 17 00:00:00 2001
From: Olivier Mangin <olivier@mangin.com>
Date: Mon, 20 Aug 2018 12:17:40 +0200
Subject: [PATCH 5/5] Makes 'copy' the default for the `add_doc` configuration
 option.

---
 changelog.md          | 2 +-
 pubs/config/spec.py   | 2 +-
 tests/test_usecase.py | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/changelog.md b/changelog.md
index 4ab4d92..0d8a214 100644
--- a/changelog.md
+++ b/changelog.md
@@ -8,7 +8,7 @@
 
 ### Implemented enhancements
 
-- Adds `move`, and `link` options for handling of documents during import (copy being the default). [(#159)](https://github.com/pubs/pubs/pull/159)
+- Adds `move`, and `link` options for handling of documents during `import` (copy being the default). Makes `copy` the default for document handling during `add`. [(#159)](https://github.com/pubs/pubs/pull/159)
 
 - Better dialog after editing paper [(#142)](https://github.com/pubs/pubs/issues/142)
 
diff --git a/pubs/config/spec.py b/pubs/config/spec.py
index d881898..76421a4 100644
--- a/pubs/config/spec.py
+++ b/pubs/config/spec.py
@@ -11,7 +11,7 @@ docsdir = string(default="docsdir://")
 
 # Specify if a document should be copied or moved in the docdir, or only
 # linked when adding a publication.
-doc_add = option('copy', 'move', 'link', default='move')
+doc_add = option('copy', 'move', 'link', default='copy')
 
 # the command to use when opening document files
 open_cmd = string(default=None)
diff --git a/tests/test_usecase.py b/tests/test_usecase.py
index fb8a109..a2f8f2f 100644
--- a/tests/test_usecase.py
+++ b/tests/test_usecase.py
@@ -668,7 +668,7 @@ class TestUsecase(DataCommandTestCase):
     def test_first(self):
         correct = ['Initializing pubs in /paper_first\n',
                    'added to pubs:\n[Page99] Page, Lawrence et al. "The PageRank Citation Ranking: Bringing Order to the Web." (1999) \n'
-                   'data/pagerank.pdf was moved to the pubs repository.\n',
+                   'data/pagerank.pdf was copied to the pubs repository.\n',
                    '[Page99] Page, Lawrence et al. "The PageRank Citation Ranking: Bringing Order to the Web." (1999) \n',
                    '\n',
                    '',