Repository: borgbackup/borg

Issue:
version parsing code blows up with a dirty workdir

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/borgbackup/borg/pull/2617?src=pr&el=h1) Report
> Merging [#2617](https://codecov.io/gh/borgbackup/borg/pull/2617?src=pr&el=desc) into [master](https://codecov.io/gh/borgbackup/borg/commit/88669611b184b17fb6b72840e32d7a0cc97ce37b?src=pr&el=desc) will **decrease** coverage by `0.64%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/borgbackup/borg/pull/2617/graphs/tree.svg?width=650&height=150&src=pr&token=LssEpmW3o4)](https://codecov.io/gh/borgbackup/borg/pull/2617?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #2617      +/-   ##
==========================================
- Coverage   83.62%   82.98%   -0.65%     
==========================================
  Files          22       22              
  Lines        8225     8227       +2     
  Branches     1395     1396       +1     
==========================================
- Hits         6878     6827      -51     
- Misses        962     1017      +55     
+ Partials      385      383       -2
```


| [Impacted Files](https://codecov.io/gh/borgbackup/borg/pull/2617?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/borg/version.py](https://codecov.io/gh/borgbackup/borg/pull/2617?src=pr&el=tree#diff-c3JjL2JvcmcvdmVyc2lvbi5weQ==) | `57.69% <100%> (+3.52%)` | :arrow_up: |
| [src/borg/xattr.py](https://codecov.io/gh/borgbackup/borg/pull/2617?src=pr&el=tree#diff-c3JjL2JvcmcveGF0dHIucHk=) | `56% <0%> (-23.5%)` | :arrow_down: |
| [src/borg/platform/\_\_init\_\_.py](https://codecov.io/gh/borgbackup/borg/pull/2617?src=pr&el=tree#diff-c3JjL2JvcmcvcGxhdGZvcm0vX19pbml0X18ucHk=) | `73.68% <0%> (-15.79%)` | :arrow_down: |
| [src/borg/platform/base.py](https://codecov.io/gh/borgbackup/borg/pull/2617?src=pr&el=tree#diff-c3JjL2JvcmcvcGxhdGZvcm0vYmFzZS5weQ==) | `72.61% <0%> (-10.72%)` | :arrow_down: |
| [src/borg/helpers.py](https://codecov.io/gh/borgbackup/borg/pull/2617?src=pr&el=tree#diff-c3JjL2JvcmcvaGVscGVycy5weQ==) | `87.11% <0%> (-0.18%)` | :arrow_down: |
| [src/borg/remote.py](https://codecov.io/gh/borgbackup/borg/pull/2617?src=pr&el=tree#diff-c3JjL2JvcmcvcmVtb3RlLnB5) | `76.73% <0%> (+0.19%)` | :arrow_up: |
| [src/borg/archive.py](https://codecov.io/gh/borgbackup/borg/pull/2617?src=pr&el=tree#diff-c3JjL2JvcmcvYXJjaGl2ZS5weQ==) | `81.72% <0%> (+0.55%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/borgbackup/borg/pull/2617?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/borgbackup/borg/pull/2617?src=pr&el=footer). Last update [8866961...7838fb1](https://codecov.io/gh/borgbackup/borg/pull/2617?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ThomasWaldmann: @RonnyPfannschmidt ok, parametrize is there. not sure what you wanted with the dict?
textshell: @RonnyPfannschmidt is there any upstream documentation on the various formats used in different setup tools versions? It seems that would be very useful to have a complete parser without surprises (not just for borg)
ThomasWaldmann: @textshell and if it is `1.1.0+dYYYMMMDD` now, was it `1.1.0-dYYYMMMDD` before setuptools 8.0?
textshell: I have no clue. Maybe it can't even generate a version like this. I tried looking in the implementation but really don't grok what is going on.
I guess we can take wild guesses, just chop off everything starting with the first `+` or try to get some more information.

I think i originally tried just going with choping off everything starting with `+`. Maybe just add that back to the code that new also understands other but disjunkt special cases?

Or maybe split on `.`, `+` and `-` and discard from the first item that does not match `([0-9]+)(a|b|rc)?([0-9]+)`?
enkore: Why are we using this automation? It does not seem to save time or troubles.
textshell: i don't do any borg release work. But i would not mind if borg would just have the version somewhere in a file tracked in git.
(Besides, looking at a random git commit in github and having some way to see what version span that commit came from is harder if the version is nowhere in the files, borg of course has a changelog that usually works to find the version)

One nice thing about a scm based version is that in theory it allows to see where something that is not a real version is roughly on the development time line. i.e. for debug output.
ThomasWaldmann: i like that the version is derived from git. that way, it is immediately obvious which version people run (not just roughly as the case if we would update manually now and then).
ThomasWaldmann: I switched to a different approach:
use a regex to start parsing from the left, only parse as long as it matches stuff we actually want.
and do not support x.y version numbers any more, we do not use them anyway and it is much simpler that way.
textshell: I wonder if we want to terminate the regex with something that constrains the following chars. currently a version like 1.1.0abc would parse as 1.1.0. But greediness currently ensures that all digits are picked up. So i'm not sure if something like `$|[^a-zA-Z0-9] would be of any practical use at the end of the regex.

Explicitly restricting the parser to major.minor.patch seems like a good simplification. Much less guessing. We just need to make sure not to have versions that don't fit without adjusting that code. Preferably by not having version that do not fit into that form.
ThomasWaldmann: well, practically we only have to parse what setuptools_scm would generate (or what we would put into some constant in the src).
textshell: indeed. We only need to prove against anything setuptools_scm did produce or might produce in the future.
I think this should work in the current form.

Patch:
diff --git a/docs/changes.rst b/docs/changes.rst
index 51170348..6e3394cd 100644
--- a/docs/changes.rst
+++ b/docs/changes.rst
@@ -172,7 +172,6 @@ New features:
 - add --debug-profile option (and also "borg debug convert-profile"), #2473
 
 Fixes:
-
 - hashindex: read/write indices >2 GiB on 32bit systems, better error
   reporting, #2496
 - repository URLs: implement IPv6 address support and also more informative
diff --git a/docs/faq.rst b/docs/faq.rst
index 71cdc014..cb9c3a2a 100644
--- a/docs/faq.rst
+++ b/docs/faq.rst
@@ -266,7 +266,6 @@ See :ref:`encrypted_repos` for more details.
 .. _password_env:
 .. note:: Be careful how you set the environment; using the ``env``
           command, a ``system()`` call or using inline shell scripts
-          (e.g. ``BORG_PASSPHRASE=hunter12 borg ...``)
           might expose the credentials in the process list directly
           and they will be readable to all users on a system. Using
           ``export`` in a shell script file should be safe, however, as
diff --git a/docs/quickstart.rst b/docs/quickstart.rst
index 43d62fc9..ffc42e35 100644
--- a/docs/quickstart.rst
+++ b/docs/quickstart.rst
@@ -169,8 +169,8 @@ may be surprised that the following ``export`` has no effect on your command::
    export BORG_PASSPHRASE='complicated & long'
    sudo ./yourborgwrapper.sh  # still prompts for password
 
-For more information, refer to the sudo(8) man page and ``env_keep`` in
-the sudoers(5) man page.
+For more information, see sudo(8) man page. Hint: see ``env_keep`` in
+sudoers(5), or try ``sudo BORG_PASSPHRASE='yourphrase' borg`` syntax.
 
 .. Tip::
     To debug what your borg process is actually seeing, find its PID
diff --git a/src/borg/helpers.py b/src/borg/helpers.py
index 1e79f63a..db66b822 100644
--- a/src/borg/helpers.py
+++ b/src/borg/helpers.py
@@ -1995,23 +1995,6 @@ def secure_erase(path):
     os.unlink(path)
 
 
-def truncate_and_unlink(path):
-    """
-    Truncate and then unlink *path*.
-
-    Do not create *path* if it does not exist.
-    Open *path* for truncation in r+b mode (=O_RDWR|O_BINARY).
-
-    Use this when deleting potentially large files when recovering
-    from a VFS error such as ENOSPC. It can help a full file system
-    recover. Refer to the "File system interaction" section
-    in repository.py for further explanations.
-    """
-    with open(path, 'r+b') as fd:
-        fd.truncate()
-    os.unlink(path)
-
-
 def popen_with_error_handling(cmd_line: str, log_prefix='', **kwargs):
     """
     Handle typical errors raised by subprocess.Popen. Return None if an error occurred,
diff --git a/src/borg/platform/base.py b/src/borg/platform/base.py
index be4b694e..0d2fb51b 100644
--- a/src/borg/platform/base.py
+++ b/src/borg/platform/base.py
@@ -1,8 +1,6 @@
 import errno
 import os
 
-from borg.helpers import truncate_and_unlink
-
 """
 platform base module
 ====================
@@ -159,7 +157,7 @@ def __init__(self, path, binary=False):
     def __enter__(self):
         from .. import platform
         try:
-            truncate_and_unlink(self.tmppath)
+            os.unlink(self.tmppath)
         except FileNotFoundError:
             pass
         self.fd = platform.SyncFile(self.tmppath, self.binary)
@@ -169,7 +167,7 @@ def __exit__(self, exc_type, exc_val, exc_tb):
         from .. import platform
         self.fd.close()
         if exc_type is not None:
-            truncate_and_unlink(self.tmppath)
+            os.unlink(self.tmppath)
             return
         os.replace(self.tmppath, self.path)
         platform.sync_dir(os.path.dirname(self.path))
diff --git a/src/borg/repository.py b/src/borg/repository.py
index a5806373..597d3ca5 100644
--- a/src/borg/repository.py
+++ b/src/borg/repository.py
@@ -18,7 +18,7 @@
 from .helpers import ProgressIndicatorPercent
 from .helpers import bin_to_hex
 from .helpers import hostname_is_unique
-from .helpers import secure_erase, truncate_and_unlink
+from .helpers import secure_erase
 from .locking import Lock, LockError, LockErrorT
 from .logger import create_logger
 from .lrucache import LRUCache
@@ -83,30 +83,6 @@ class Repository:
     dir/data/<X // SEGMENTS_PER_DIR>/<X>
     dir/index.X
     dir/hints.X
-
-    File system interaction
-    -----------------------
-
-    LoggedIO generally tries to rely on common behaviours across transactional file systems.
-
-    Segments that are deleted are truncated first, which avoids problems if the FS needs to
-    allocate space to delete the dirent of the segment. This mostly affects CoW file systems,
-    traditional journaling file systems have a fairly good grip on this problem.
-
-    Note that deletion, i.e. unlink(2), is atomic on every file system that uses inode reference
-    counts, which includes pretty much all of them. To remove a dirent the inodes refcount has
-    to be decreased, but you can't decrease the refcount before removing the dirent nor can you
-    decrease the refcount after removing the dirent. File systems solve this with a lock,
-    and by ensuring it all stays within the same FS transaction.
-
-    Truncation is generally not atomic in itself, and combining truncate(2) and unlink(2) is of
-    course never guaranteed to be atomic. Truncation in a classic extent-based FS is done in
-    roughly two phases, first the extents are removed then the inode is updated. (In practice
-    this is of course way more complex).
-
-    LoggedIO gracefully handles truncate/unlink splits as long as the truncate resulted in
-    a zero length file. Zero length segments are considered to not exist, while LoggedIO.cleanup()
-    will still get rid of them.
     """
 
     class DoesNotExist(Error):
@@ -1135,8 +1111,6 @@ def segment_iterator(self, segment=None, reverse=False):
                 filenames = [filename for filename in filenames if filename.isdigit() and int(filename) <= segment]
             filenames = sorted(filenames, key=int, reverse=reverse)
             for filename in filenames:
-                # Note: Do not filter out logically deleted segments  (see "File system interaction" above),
-                # since this is used by cleanup and txn state detection as well.
                 yield int(filename), os.path.join(data_path, dir, filename)
 
     def get_latest_segment(self):
@@ -1158,7 +1132,7 @@ def cleanup(self, transaction_id):
         self.segment = transaction_id + 1
         for segment, filename in self.segment_iterator(reverse=True):
             if segment > transaction_id:
-                truncate_and_unlink(filename)
+                os.unlink(filename)
             else:
                 break
 
@@ -1233,15 +1207,12 @@ def delete_segment(self, segment):
         if segment in self.fds:
             del self.fds[segment]
         try:
-            truncate_and_unlink(self.segment_filename(segment))
+            os.unlink(self.segment_filename(segment))
         except FileNotFoundError:
             pass
 
     def segment_exists(self, segment):
-        filename = self.segment_filename(segment)
-        # When deleting segments, they are first truncated. If truncate(2) and unlink(2) are split
-        # across FS transactions, then logically deleted segments will show up as truncated.
-        return os.path.exists(filename) and os.path.getsize(filename)
+        return os.path.exists(self.segment_filename(segment))
 
     def segment_size(self, segment):
         return os.path.getsize(self.segment_filename(segment))
@@ -1287,7 +1258,6 @@ def recover_segment(self, segment, filename):
         if segment in self.fds:
             del self.fds[segment]
         with open(filename, 'rb') as fd:
-            # XXX: Rather use mmap, this loads the entire segment (up to 500 MB by default) into memory.
             data = memoryview(fd.read())
         os.rename(filename, filename + '.beforerecover')
         logger.info('attempting to recover ' + filename)
diff --git a/src/borg/version.py b/src/borg/version.py
index 4eb0c77d..7e2e95b7 100644
--- a/src/borg/version.py
+++ b/src/borg/version.py
@@ -3,33 +3,33 @@
 
 def parse_version(version):
     """
-    simplistic parser for setuptools_scm versions
+    Simplistic parser for setuptools_scm versions.
 
-    supports final versions and alpha ('a'), beta ('b') and rc versions. It just discards commits since last tag
-    and git revision hash.
+    Supports final versions and alpha ('a'), beta ('b') and release candidate ('rc') versions.
+    It does not try to parse anything else than that, even if there is more in the version string.
 
     Output is a version tuple containing integers. It ends with one or two elements that ensure that relational
-    operators yield correct relations for alpha, beta and rc versions too. For final versions the last element
-    is a -1, for prerelease versions the last two elements are a smaller negative number and the number of e.g.
-    the beta.
-
-    Note, this sorts version 1.0 before 1.0.0.
+    operators yield correct relations for alpha, beta and rc versions, too.
+    For final versions the last element is a -1.
+    For prerelease versions the last two elements are a smaller negative number and the number of e.g. the beta.
 
     This version format is part of the remote protocol, don‘t change in breaking ways.
     """
-
-    parts = version.split('+')[0].split('.')
-    if parts[-1].startswith('dev'):
-        del parts[-1]
-    version = [int(segment) for segment in parts[:-1]]
-
-    prerelease = re.fullmatch('([0-9]+)(a|b|rc)([0-9]+)', parts[-1])
-    if prerelease:
-        version_type = {'a': -4, 'b': -3, 'rc': -2}[prerelease.group(2)]
-        version += [int(prerelease.group(1)), version_type, int(prerelease.group(3))]
+    version_re = r"""
+        (?P<major>\d+)\.(?P<minor>\d+)\.(?P<patch>\d+)   # version, e.g. 1.2.33
+        (?P<prerelease>(?P<ptype>a|b|rc)(?P<pnum>\d+))?  # optional prerelease, e.g. a1 or b2 or rc33
+    """
+    m = re.match(version_re, version, re.VERBOSE)
+    if m is None:
+        raise ValueError('Invalid version string %s' % version)
+    gd = m.groupdict()
+    version = [int(gd['major']), int(gd['minor']), int(gd['patch'])]
+    if m.lastgroup == 'prerelease':
+        p_type = {'a': -4, 'b': -3, 'rc': -2}[gd['ptype']]
+        p_num = int(gd['pnum'])
+        version += [p_type, p_num]
     else:
-        version += [int(parts[-1]), -1]
-
+        version += [-1]
     return tuple(version)
 
 


Test patch:
diff --git a/src/borg/testsuite/version.py b/src/borg/testsuite/version.py
new file mode 100644
index 00000000..b5f32e6e
--- /dev/null
+++ b/src/borg/testsuite/version.py
@@ -0,0 +1,38 @@
+import pytest
+
+from ..version import parse_version
+
+
+@pytest.mark.parametrize("version_str, version_tuple", [
+    # setuptools < 8.0 uses "-"
+    ('1.0.0a1.dev204-g8866961.d20170606', (1, 0, 0, -4, 1)),
+    ('1.0.0a1.dev204-g8866961', (1, 0, 0, -4, 1)),
+    ('1.0.0-d20170606', (1, 0, 0, -1)),
+    # setuptools >= 8.0 uses "+"
+    ('1.0.0a1.dev204+g8866961.d20170606', (1, 0, 0, -4, 1)),
+    ('1.0.0a1.dev204+g8866961', (1, 0, 0, -4, 1)),
+    ('1.0.0+d20170606', (1, 0, 0, -1)),
+    # pre-release versions:
+    ('1.0.0a1', (1, 0, 0, -4, 1)),
+    ('1.0.0a2', (1, 0, 0, -4, 2)),
+    ('1.0.0b3', (1, 0, 0, -3, 3)),
+    ('1.0.0rc4', (1, 0, 0, -2, 4)),
+    # release versions:
+    ('0.0.0', (0, 0, 0, -1)),
+    ('0.0.11', (0, 0, 11, -1)),
+    ('0.11.0', (0, 11, 0, -1)),
+    ('11.0.0', (11, 0, 0, -1)),
+])
+def test_parse_version(version_str, version_tuple):
+    assert parse_version(version_str) == version_tuple
+
+
+def test_parse_version_invalid():
+    with pytest.raises(ValueError):
+        assert parse_version('')  # we require x.y.z versions
+    with pytest.raises(ValueError):
+        assert parse_version('1')  # we require x.y.z versions
+    with pytest.raises(ValueError):
+        assert parse_version('1.2')  # we require x.y.z versions
+    with pytest.raises(ValueError):
+        assert parse_version('crap')