From 888a1b8c578bfd4f0481bafbc5bcdb67cd178f7c Mon Sep 17 00:00:00 2001
From: Ryan Roden-Corrent <ryan@rcorre.net>
Date: Fri, 22 Sep 2017 08:30:41 -0400
Subject: [PATCH] Append multiple history backups on import.

Previously, a successful import of the text history into sqlite would
move 'history' to 'history.bak'. If history.bak already existed, this
would overwrite it on unix and fail on windows.

With this patch, the most recently imported history is appended to
history.bak to avoid data loss.

Resolves #3005.

A few other options I considered:

1. os.replace:
    - fast, simple, no error on Windows
    - potential data loss
2. numbered backups (.bak.1, .bak.2, ...):
    - fast, no data loss, but more complex
3. append each line to the backup as it is read:
    - more efficient than current patch (no need to read history twice)
    - potentially duplicate data if backup fails
---
 qutebrowser/browser/history.py     | 15 +++++++++++----
 tests/unit/browser/test_history.py | 16 ++++++++++++++++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py
index c9609bc03..ca62d9e6c 100644
--- a/qutebrowser/browser/history.py
+++ b/qutebrowser/browser/history.py
@@ -252,10 +252,7 @@ class WebHistory(sql.SqlTable):
                 except ValueError as ex:
                     message.error('Failed to import history: {}'.format(ex))
                 else:
-                    bakpath = path + '.bak'
-                    message.info('History import complete. Moving {} to {}'
-                                 .format(path, bakpath))
-                    os.rename(path, bakpath)
+                    self._write_backup(path)
 
         # delay to give message time to appear before locking down for import
         message.info('Converting {} to sqlite...'.format(path))
@@ -287,6 +284,16 @@ class WebHistory(sql.SqlTable):
         self.insert_batch(data)
         self.completion.insert_batch(completion_data, replace=True)
 
+    def _write_backup(self, path):
+        bak = path + '.bak'
+        message.info('History import complete. Appending {} to {}'
+                     .format(path, bak))
+        with open(path, 'r', encoding='utf-8') as infile:
+            with open(bak, 'a', encoding='utf-8') as outfile:
+                for line in infile:
+                    outfile.write('\n' + line)
+        os.remove(path)
+
     def _format_url(self, url):
         return url.toString(QUrl.RemovePassword | QUrl.FullyEncoded)
 
diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py
index 51157fccb..1454e259b 100644
--- a/tests/unit/browser/test_history.py
+++ b/tests/unit/browser/test_history.py
@@ -284,6 +284,22 @@ def test_import_txt(hist, data_tmpdir, monkeypatch, stubs):
     assert (data_tmpdir / 'history.bak').exists()
 
 
+def test_import_txt_existing_backup(hist, data_tmpdir, monkeypatch, stubs):
+    monkeypatch.setattr(history, 'QTimer', stubs.InstaTimer)
+    histfile = data_tmpdir / 'history'
+    bakfile = data_tmpdir / 'history.bak'
+    histfile.write('12345 http://example.com/ title')
+    bakfile.write('12346 http://qutebrowser.org/')
+
+    hist.import_txt()
+
+    assert list(hist) == [('http://example.com/', 'title', 12345, False)]
+
+    assert not histfile.exists()
+    assert bakfile.read().split('\n') == ['12346 http://qutebrowser.org/',
+                                          '12345 http://example.com/ title']
+
+
 @pytest.mark.parametrize('line', [
     '',
     '#12345 http://example.com/commented',