Merge ~racb/git-ubuntu:changelog-date-parsing into git-ubuntu:master

[Robie Basak]

Make Jenkins happy

[Robie Basak (racb)]

This branch is ready except for the final commit. I don't think the assertion is correct. We might also have to ensure that CI is running in C.UTF-8 - otherwise the assertion (when fixed) will fail.

[Robie Basak (racb)]

Another minor issue might be that git-ubuntu.self-test will start to fail when run in a different locale. I'm not sure how this should work - ideally git-ubuntu.self-test would mainly run in the user's locale except for the changelog date parsing.

[Robie Basak (racb)]

Thanks to Colin in #ubuntu-devel for clarifying how it's supposed to work. I think I've now got it correct and with a force push the branch is now ready.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:f5cdca0822e95d6954ffaf43db6a94819ea4bd3e
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/530/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/530//rebuild

review: Approve (continuous-integration)

[Bryce Harrington (bryce)]

8d447a009bc64585603751c5ec766927cb3b565f
  - In code docs, param changelog_date_timestamp should be changelog_timestamp_string
  - strptime() throws a ValueError on parsing error. May be worth testing for in git_authorship()?

26a3f3b66b4ae429116f11f1c7c493fc4ff7fcc4
  - Looks good, a concise solution to a rather messy problem. The set of regex's provided seem to cover the various combinations, and looks easy to add more cases to.
  - In the assertion message, why is %r used ("Could not parse date %r" % changelog_timestamp_string,) ? It appears to behave properly, but I would have used %s so I'm curious.
  - Thanks for documenting the encountered cases for the invalid dates in the test case.

f5cdca0822e95d6954ffaf43db6a94819ea4bd3e
  - Setting LC_ALL also may be changing collation, which can change sorting behavior. I doubt that will be an issue at all for git-ubuntu, but if odd things start happening could look unsetting LC_ALL and instead setting LC_TIME and LC_CTYPE.
  - That aside, LGTM +1

review: Approve

[Robie Basak (racb)]

Thanks for the review! I've addressed your comments in a new branch, and
I'll merge that as soon as CI passes.

On Wed, Jul 22, 2020 at 04:57:12PM -0000, Bryce Harrington wrote:
> Review: Approve
>
> 8d447a009bc64585603751c5ec766927cb3b565f
> - In code docs, param changelog_date_timestamp should be changelog_timestamp_string

Fixed, thanks.

> - strptime() throws a ValueError on parsing error. May be worth testing for in git_authorship()?

I added a new test that verifies the known bad cases that should fail.

> 26a3f3b66b4ae429116f11f1c7c493fc4ff7fcc4
> - Looks good, a concise solution to a rather messy problem. The set of regex's provided seem to cover the various combinations, and looks easy to add more cases to.
> - In the assertion message, why is %r used ("Could not parse date %r" % changelog_timestamp_string,) ? It appears to behave properly, but I would have used %s so I'm curious.

%r ensures to escape things that might be awkward for the terminal. As
we're reporting something wrong, it seems more likely than average that
this might happen here - for example with control characters or similar
that might not appear in a human-readable way in logs otherwise.

> - Thanks for documenting the encountered cases for the invalid dates in the test case.
>
> f5cdca0822e95d6954ffaf43db6a94819ea4bd3e
> - Setting LC_ALL also may be changing collation, which can change sorting behavior. I doubt that will be an issue at all for git-ubuntu, but if odd things start happening could look unsetting LC_ALL and instead setting LC_TIME and LC_CTYPE.

I agree there's a risk of disrupting things here. But ultimately we
probably want stable behaviour, so I think it's better to risk this
disruption now rather than later.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:6ff151dc208809e1b8e3108c69c05090174e6221
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/532/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/532//rebuild

review: Approve (continuous-integration)

diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py
index 4c24176..43c8355 100644
--- a/gitubuntu/git_repository.py
+++ b/gitubuntu/git_repository.py
@@ -8,6 +8,7 @@ import datetime
 import enum
 from functools import lru_cache
 import itertools
+import locale
 import logging
 import os
 import posixpath
@@ -773,6 +774,75 @@ class Changelog:
             return ret
         return self._shell_srcpkg
 
+    @staticmethod
+    def _parse_changelog_date(changelog_timestamp_string):
+        """Convert changelog timestamp into datetime object
+
+        This function currently requires the locale to have been set to C.UTF-8
+        by the caller. This would typically be done at the main entry point to
+        the importer.
+
+        :param str changelog_timestamp_string: the timestamp part of the the
+            signoff line from a changelog entry
+        :rtype: datetime.datetime
+        :returns: the timestamp as a datetime object
+        :raises ValueError: if the string could not be parsed
+        """
+        # We avoid using something like dateutil.parser here because the
+        # parsing behaviour of malformed or unusually formatted dates must be
+        # precisely as specified and not ever change behaviour. If it did, then
+        # imports would no longer be reproducible.
+        #
+        # However, adding new a form of parsing an unambigious date is
+        # acceptable if the spec is first updated accordingly since that would
+        # only introduce new imports that would have previously failed.
+        #
+        # time.strptime ignores time zones, so we must use datetime.strptime()
+
+        # strptime doesn't support anything other than standard locale names
+        # for days of the week, so handle the "Thur" abbreviation as a special
+        # case as defined in the spec as it is unambiguous.
+        adjusted_changelog_timestamp_string = re.sub(
+            r'^Thur,',
+            'Thu,',
+            changelog_timestamp_string,
+        )
+
+        # Ensure that strptime will parse using the standard English names for
+        # weekdays and months. We actually want to check for C.UTF-8, but
+        # something seems to convert that to en_US.UTF-8. Either will do. It
+        # would be an improvement for this function to become
+        # locale-setting-independent, but that would mean finding some other
+        # way to reproducibly parse changelog timestamps and finding that isn't
+        # a priority right now.
+        assert any([
+            locale.getlocale(locale.LC_TIME) == ('C', 'UTF-8'),
+            locale.getlocale(locale.LC_TIME) == ('en_US', 'UTF-8'),
+            locale.getlocale(locale.LC_TIME) == (None, None)
+        ])
+
+        acceptable_date_formats = [
+            '%a, %d %b %Y %H:%M:%S %z',  # standard
+            '%A, %d %b %Y %H:%M:%S %z',  # full day of week
+            '%d %b %Y %H:%M:%S %z',      # missing day of week
+            '%a, %d %B %Y %H:%M:%S %z',  # full month name
+            '%A, %d %B %Y %H:%M:%S %z',  # full day of week and month name
+            '%d %B %Y %H:%M:%S %z',      # missing day of week with full month
+                                         # name
+        ]
+        for date_format in acceptable_date_formats:
+            try:
+                return datetime.datetime.strptime(
+                    adjusted_changelog_timestamp_string,
+                    date_format,
+                )
+            except ValueError:
+                pass
+        else:
+            raise ValueError(
+                "Could not parse date %r" % changelog_timestamp_string,
+            )
+
     @property
     def git_authorship(self):
         """Extract last changelog entry's maintainer and timestamp
@@ -789,12 +859,7 @@ class Changelog:
         if m is None:
             raise ValueError('Cannot get authorship')
 
-        # time.strptime ignores time zones, so we must use datetime.strptime()
-        parsed_date = datetime.datetime.strptime(
-            self.date,
-            '%a, %d %b %Y %H:%M:%S %z',
-        )
-
+        parsed_date = self._parse_changelog_date(self.date)
         epoch_seconds = int(parsed_date.timestamp())
 
         # seconds -> minutes
diff --git a/gitubuntu/git_repository_test.py b/gitubuntu/git_repository_test.py
index 2637915..4f52339 100644
--- a/gitubuntu/git_repository_test.py
+++ b/gitubuntu/git_repository_test.py
@@ -108,6 +108,65 @@ def test_changelog_maintainer_invalid():
         test_changelog.maintainer
 
 
[e-mail](['input_date_string', 'expected_result'], [
+    # The normal complete form
+    ('Mon, 12 May 2016 08:14:34 -0700', (2016, 5, 12, 8, 14, 34, -7)),
+    # Day of week missing, such as in:
+    #     datefudge 1.12
+    ('12 May 2016 08:14:34 -0700', (2016, 5, 12, 8, 14, 34, -7)),
+    # Full (not abbreviated) month name, such as in:
+    #     dnsmasq 2.32-2
+    #     dropbear 0.42-1
+    #     e2fsprogs 1.42.11-1
+    #     efibootmgr 0.5.4-7
+    #     hunspell-br 0.11-1
+    #     kubuntu-default-settings 1:6.06-22
+    #     libvformat 1.13-4
+    ('12 June 2016 08:14:34 -0700', (2016, 6, 12, 8, 14, 34, -7)),
+    # Full (not abbreviated) day of week name, such as in:
+    #     logcheck 1.2.22a
+    ('Thursday, 15 May 2016 08:14:34 -0700', (2016, 5, 15, 8, 14, 34, -7)),
+    # Part-abbreviated day of week name, such as in:
+    #     kubuntu-meta 1.76
+    ('Thur, 15 May 2016 08:14:34 -0700', (2016, 5, 15, 8, 14, 34, -7)),
+])
+def test_parse_changelog_date(input_date_string, expected_result):
+    """_parse_changelog_date should parse a basic date string correctly
+
+    :param str input_date_string: the timestamp part of the changelog signoff
+        line
+    :param tuple(int, int, int, int, int, int, int) expected_result: the
+        expected parse result in (year, month, day, hour, minute, second,
+        timezone_offset_in_hours) form. The actual expected result needs to be
+        a datetime.datetime object; to avoid duplication in test parameters
+        this will be instantiated within the test.
+    """
+    actual_result = target.Changelog._parse_changelog_date(input_date_string)
+    expected_result_datetime = datetime.datetime(
+        *expected_result[:6],
+        tzinfo=datetime.timezone(datetime.timedelta(hours=expected_result[6])),
+    )
+    assert actual_result == expected_result_datetime
+
+
[e-mail](['input_date_string'], [
+    ('Mon, 30 Feb 2020 15:50:58 +0200',),  # ghostscript 9.50~dfsg-5ubuntu4
+    ('Mon, 03 Sep 2018 00:43:25 -7000',),  # lxqt-config 0.13.0-0ubuntu4
+    ('Tue, 17 May 2008 10:93:55 -0500',),  # iscsitarget
+                                           #     0.4.15+svn148-2.1ubuntu1
+    ('Monu, 22 Jan 2007 22:10:50 -0500',), # mail-spf-perl 2.004-0ubuntu1
+    ('Wed, 29 Augl 2007 16:14:11 +0200',), # nut 2.2.0-2
+])
+def test_changelog_date_parse_errors(input_date_string):
+    """_parse_changelog_date should raise ValueError on illegal dates
+
+    :param str input_date_string: the timestamp part of the changelog signoff
+        line
+    """
+    with pytest.raises(ValueError):
+        target.Changelog._parse_changelog_date(input_date_string)
+
+
 @pytest.mark.parametrize(
     'changelog_name, name, email, epoch_seconds, offset', [
         (
diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py
index 014d6fd..880c834 100644
--- a/gitubuntu/importer.py
+++ b/gitubuntu/importer.py
@@ -28,6 +28,7 @@ import argparse
 import datetime
 import functools
 import getpass
+import locale
 import logging
 import os
 import re
@@ -474,6 +475,10 @@ def main(
     Returns 0 on successful import (which includes non-fatal failures);
     1 otherwise.
     """
+    # For import reproduciblity, we must use the C.UTF-8 locale to parse dates,
+    # etc.
+    locale.setlocale(locale.LC_ALL, 'C.UTF-8')
+
     logging.info('Ubuntu Server Team importer v%s' % VERSION)
 
     if not repo: