diff --git a/releasenotes/notes/fix-git-log-ordering-0e52f95f66c8db5b.yaml b/releasenotes/notes/fix-git-log-ordering-0e52f95f66c8db5b.yaml new file mode 100644 index 0000000..d0aef66 --- /dev/null +++ b/releasenotes/notes/fix-git-log-ordering-0e52f95f66c8db5b.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - Fixes `bug 1522153 + `__ so that notes + added in commits that are merged after tags are associated with + the correct version. diff --git a/reno/scanner.py b/reno/scanner.py index 053c626..e8593da 100644 --- a/reno/scanner.py +++ b/reno/scanner.py @@ -79,7 +79,70 @@ def _get_unique_id(filename): return uniqueid -# TODO(dhellmann): Add branch arg? +# The git log output from _get_tags_on_branch() looks like this sample +# from the openstack/nova repository for git 1.9.1: +# +# git log --simplify-by-decoration --pretty="%d" +# (HEAD, origin/master, origin/HEAD, gerrit/master, master) +# (apu/master) +# (tag: 13.0.0.0b1) +# (tag: 12.0.0.0rc3, tag: 12.0.0) +# +# And this for git 1.7.1 (RHEL): +# +# $ git log --simplify-by-decoration --pretty="%d" +# (HEAD, origin/master, origin/HEAD, master) +# (tag: 13.0.0.0b1) +# (tag: 12.0.0.0rc3, tag: 12.0.0) +# (tag: 12.0.0.0rc2) +# (tag: 2015.1.0rc3, tag: 2015.1.0) +# ... +# (tag: folsom-2) +# (tag: folsom-1) +# (essex-1) +# (diablo-2) +# (diablo-1) +# (2011.2) +# +# The difference in the tags with "tag:" and without appears to be +# caused by some being annotated and others not. +# +# So we might have multiple tags on a given commit, and we might have +# lines that have no tags or are completely blank, and we might have +# "tag:" or not. This pattern is used to find the tag entries on each +# line, ignoring tags that don't look like version numbers. +TAG_RE = re.compile('(?:[(]|tag: )([\d.ab]+)[,)]') + + +def _get_version_tags_on_branch(reporoot, branch): + """Return tags from the branch, in date order. + + Need to get the list of tags in the right order, because the topo + search breaks the date ordering. Use git to ask for the tags in + order, rather than trying to sort them, because many repositories + have "non-standard" tags or have renumbered projects (from + date-based to SemVer), for which sorting would require complex + logic. + + """ + tags = [] + tag_cmd = [ + 'git', 'log', + '--simplify-by-decoration', + '--pretty="%d"', + ] + if branch: + tag_cmd.append(branch) + LOG.debug('running %s' % ' '.join(tag_cmd)) + tag_results = utils.check_output(tag_cmd, cwd=reporoot) + LOG.debug(tag_results) + for line in tag_results.splitlines(): + LOG.debug('line %r' % line) + for match in TAG_RE.findall(line): + tags.append(match) + return tags + + def get_notes_by_version(reporoot, notesdir, branch=None): """Return an OrderedDict mapping versions to lists of notes files. @@ -91,13 +154,26 @@ def get_notes_by_version(reporoot, notesdir, branch=None): LOG.debug('scanning %s/%s (branch=%s)' % (reporoot, notesdir, branch)) + # Determine all of the tags known on the branch, in their date + # order. We scan the commit history in topological order to ensure + # we have the commits in the right version, so we might encounter + # the tags in a different order during that phase. + versions_by_date = _get_version_tags_on_branch(reporoot, branch) + LOG.debug('versions by date %r' % (versions_by_date,)) versions = [] earliest_seen = collections.OrderedDict() - # Determine the current version, which might be an unreleased or dev - # version. + # Determine the current version, which might be an unreleased or + # dev version if there are unreleased commits at the head of the + # branch in question. Since the version may not already be known, + # make sure it is in the list of versions by date. And since it is + # the most recent version, go ahead and insert it at the front of + # the list. current_version = _get_current_version(reporoot, branch) LOG.debug('current repository version: %s' % current_version) + if current_version not in versions_by_date: + LOG.debug('adding %s to versions by date' % current_version) + versions_by_date.insert(0, current_version) # Remember the most current filename for each id, to allow for # renames. @@ -105,7 +181,12 @@ def get_notes_by_version(reporoot, notesdir, branch=None): # FIXME(dhellmann): This might need to be more line-oriented for # longer histories. - log_cmd = ['git', 'log', '--pretty=%x00%H %d', '--name-only'] + log_cmd = [ + 'git', 'log', + '--topo-order', # force traversal order rather than date order + '--pretty=%x00%H %d', # output contents in parsable format + '--name-only' # only include the names of the files in the patch + ] if branch is not None: log_cmd.append(branch) LOG.debug('running %s' % ' '.join(log_cmd)) @@ -188,20 +269,26 @@ def get_notes_by_version(reporoot, notesdir, branch=None): except KeyError: # Unable to find the file again, skip it to avoid breaking # the build. - print( - '[reno] unable to find file associated ' - 'with unique id %r, skipping' % - uniqueid, - file=sys.stderr, - ) + msg = ('[reno] unable to find file associated ' + 'with unique id %r, skipping') % uniqueid + LOG.debug(msg) + print(msg, file=sys.stderr) # Only return the parts of files_and_tags that actually have # filenames associated with the versions. - trimmed = { - k: list(reversed(v)) - for (k, v) - in files_and_tags.items() - if v - } + trimmed = collections.OrderedDict() + for ov in versions_by_date: + if not files_and_tags.get(ov): + continue + # Sort the notes associated with the version so they are in a + # deterministic order, to avoid having the same data result in + # different output depending on random factors. Earlier + # versions of the scanner assumed the notes were recorded in + # chronological order based on the commit date, but with the + # change to use topological sorting that is no longer + # necessarily true. We want the notes to always show up in the + # same order, but it doesn't really matter what order that is, + # so just sort based on the unique id. + trimmed[ov] = sorted(files_and_tags[ov]) return trimmed diff --git a/reno/tests/test_scanner.py b/reno/tests/test_scanner.py index fc0b124..1a334a2 100644 --- a/reno/tests/test_scanner.py +++ b/reno/tests/test_scanner.py @@ -17,6 +17,7 @@ import logging import os.path import re import subprocess +import textwrap from reno import create from reno import scanner @@ -24,6 +25,7 @@ from reno.tests import base from reno import utils import fixtures +import mock from testtools.content import text_content @@ -458,6 +460,145 @@ class BasicTest(Base): ) +class MergeCommitTest(Base): + + def test_1(self): + # Create changes on master and in the branch + # in order so the history is "normal" + n1 = self._add_notes_file() + self._run_git('tag', '-s', '-m', 'first tag', '1.0.0') + self._run_git('checkout', '-b', 'test_merge_commit') + n2 = self._add_notes_file() + self._run_git('checkout', 'master') + self._add_other_file('ignore-1.txt') + self._run_git('merge', '--no-ff', 'test_merge_commit') + self._add_other_file('ignore-2.txt') + self._run_git('tag', '-s', '-m', 'second tag', '2.0.0') + raw_results = scanner.get_notes_by_version( + self.reporoot, + 'releasenotes/notes', + ) + results = { + k: [f for (f, n) in v] + for (k, v) in raw_results.items() + } + self.assertEqual( + {'1.0.0': [n1], + '2.0.0': [n2]}, + results, + ) + self.assertEqual( + ['2.0.0', '1.0.0'], + list(raw_results.keys()), + ) + + def test_2(self): + # Create changes on the branch before the tag into which it is + # actually merged. + self._add_other_file('ignore-0.txt') + self._run_git('checkout', '-b', 'test_merge_commit') + n1 = self._add_notes_file() + self._run_git('checkout', 'master') + n2 = self._add_notes_file() + self._run_git('tag', '-s', '-m', 'first tag', '1.0.0') + self._add_other_file('ignore-1.txt') + self._run_git('merge', '--no-ff', 'test_merge_commit') + self._add_other_file('ignore-2.txt') + self._run_git('tag', '-s', '-m', 'second tag', '2.0.0') + raw_results = scanner.get_notes_by_version( + self.reporoot, + 'releasenotes/notes', + ) + results = { + k: [f for (f, n) in v] + for (k, v) in raw_results.items() + } + self.assertEqual( + {'1.0.0': [n2], + '2.0.0': [n1]}, + results, + ) + self.assertEqual( + ['2.0.0', '1.0.0'], + list(raw_results.keys()), + ) + + def test_3(self): + # Create changes on the branch before the tag into which it is + # actually merged, with another tag in between the time of the + # commit and the time of the merge. This should reflect the + # order of events described in bug #1522153. + self._add_other_file('ignore-0.txt') + self._run_git('checkout', '-b', 'test_merge_commit') + n1 = self._add_notes_file() + self._run_git('checkout', 'master') + n2 = self._add_notes_file() + self._run_git('tag', '-s', '-m', 'first tag', '1.0.0') + self._add_other_file('ignore-1.txt') + self._run_git('tag', '-s', '-m', 'second tag', '1.1.0') + self._run_git('merge', '--no-ff', 'test_merge_commit') + self._add_other_file('ignore-2.txt') + self._run_git('tag', '-s', '-m', 'third tag', '2.0.0') + self._add_other_file('ignore-3.txt') + raw_results = scanner.get_notes_by_version( + self.reporoot, + 'releasenotes/notes', + ) + results = { + k: [f for (f, n) in v] + for (k, v) in raw_results.items() + } + # Since the 1.1.0 tag has no notes files, it does not appear + # in the output. It's only there to trigger the bug as it was + # originally reported. + self.assertEqual( + {'1.0.0': [n2], + '2.0.0': [n1]}, + results, + ) + self.assertEqual( + ['2.0.0', '1.0.0'], + list(raw_results.keys()), + ) + + def test_4(self): + # Create changes on the branch before the tag into which it is + # actually merged, with another tag in between the time of the + # commit and the time of the merge. This should reflect the + # order of events described in bug #1522153. + self._add_other_file('ignore-0.txt') + self._run_git('checkout', '-b', 'test_merge_commit') + n1 = self._add_notes_file() + self._run_git('checkout', 'master') + n2 = self._add_notes_file() + self._run_git('tag', '-s', '-m', 'first tag', '1.0.0') + self._add_other_file('ignore-1.txt') + n3 = self._add_notes_file() + self._run_git('tag', '-s', '-m', 'second tag', '1.1.0') + self._run_git('merge', '--no-ff', 'test_merge_commit') + self._add_other_file('ignore-2.txt') + self._run_git('tag', '-s', '-m', 'third tag', '2.0.0') + self._add_other_file('ignore-3.txt') + raw_results = scanner.get_notes_by_version( + self.reporoot, + 'releasenotes/notes', + ) + results = { + k: [f for (f, n) in v] + for (k, v) in raw_results.items() + } + self.assertEqual( + {'1.0.0': [n2], + '1.1.0': [n3], + '2.0.0': [n1]}, + results, + ) + self.assertEqual( + ['2.0.0', '1.1.0', '1.0.0'], + list(raw_results.keys()), + ) + + class UniqueIdTest(Base): def test_legacy(self): @@ -533,3 +674,165 @@ class BranchTest(Base): }, results, ) + + +class GetTagsParseTest(base.TestCase): + + EXPECTED = [ + '2.0.0', + '1.8.1', + '1.8.0', + '1.7.1', + '1.7.0', + '1.6.0', + '1.5.0', + '1.4.0', + '1.3.0', + '1.2.0', + '1.1.0', + '1.0.0', + '0.11.2', + '0.11.1', + '0.11.0', + '0.10.1', + '0.10.0', + '0.9.0', + '0.8.0', + '0.7.1', + '0.7.0', + '0.6.0', + '0.5.1', + '0.5.0', + '0.4.2', + '0.4.1', + '0.4.0', + '0.3.2', + '0.3.1', + '0.3.0', + '0.2.5', + '0.2.4', + '0.2.3', + '0.2.2', + '0.2.1', + '0.2.0', + '0.1.3', + '0.1.2', + '0.1.1', + '0.1.0', + ] + + def test_keystoneclient_ubuntu_1_9_1(self): + # git 1.9.1 as it produces output on ubuntu for python-keystoneclient + # git log --simplify-by-decoration --pretty="%d" + tag_list_output = textwrap.dedent(""" + (HEAD, origin/master, origin/HEAD, gerrit/master, master) + (apu/master) + (tag: 2.0.0) + (tag: 1.8.1) + (tag: 1.8.0) + (tag: 1.7.1) + (tag: 1.7.0) + (tag: 1.6.0) + (tag: 1.5.0) + (tag: 1.4.0) + (uncap-requirements) + (tag: 1.3.0) + (tag: 1.2.0) + (tag: 1.1.0) + (tag: 1.0.0) + (tag: 0.11.2) + (tag: 0.11.1) + (tag: 0.11.0) + (tag: 0.10.1) + (tag: 0.10.0) + (tag: 0.9.0) + (tag: 0.8.0) + (tag: 0.7.1) + (tag: 0.7.0) + (tag: 0.6.0) + (tag: 0.5.1) + (tag: 0.5.0) + (tag: 0.4.2) + (tag: 0.4.1) + (tag: 0.4.0) + (tag: 0.3.2) + (tag: 0.3.1) + (tag: 0.3.0) + (tag: 0.2.5) + (tag: 0.2.4) + (tag: 0.2.3) + (tag: 0.2.2) + (tag: 0.2.1) + (tag: 0.2.0) + + (origin/feature/keystone-v3, gerrit/feature/keystone-v3) + (tag: 0.1.3) + (tag: 0.1.2) + (tag: 0.1.1) + (tag: 0.1.0) + (tag: folsom-1) + (tag: essex-rc1) + (tag: essex-4) + (tag: essex-3) + """) + with mock.patch('reno.utils.check_output') as co: + co.return_value = tag_list_output + actual = scanner._get_version_tags_on_branch('reporoot', + branch=None) + self.assertEqual(self.EXPECTED, actual) + + def test_keystoneclient_rhel_1_7_1(self): + # git 1.7.1 as it produces output on RHEL 6 for python-keystoneclient + # git log --simplify-by-decoration --pretty="%d" + tag_list_output = textwrap.dedent(""" + (HEAD, origin/master, origin/HEAD, master) + (tag: 2.0.0) + (tag: 1.8.1) + (tag: 1.8.0) + (tag: 1.7.1) + (tag: 1.7.0) + (tag: 1.6.0) + (tag: 1.5.0) + (tag: 1.4.0) + (tag: 1.3.0) + (tag: 1.2.0) + (tag: 1.1.0) + (tag: 1.0.0) + (tag: 0.11.2) + (tag: 0.11.1) + (tag: 0.11.0) + (tag: 0.10.1) + (tag: 0.10.0) + (tag: 0.9.0) + (tag: 0.8.0) + (tag: 0.7.1) + (tag: 0.7.0) + (tag: 0.6.0) + (tag: 0.5.1) + (tag: 0.5.0) + (tag: 0.4.2) + (tag: 0.4.1) + (tag: 0.4.0) + (tag: 0.3.2) + (tag: 0.3.1) + (tag: 0.3.0) + (tag: 0.2.5) + (tag: 0.2.4) + (tag: 0.2.3) + (tag: 0.2.2) + (tag: 0.2.1) + (tag: 0.2.0) + (tag: 0.1.3) + (0.1.2) + (tag: 0.1.1) + (0.1.0) + (tag: folsom-1) + (tag: essex-rc1) + (essex-4) + (essex-3) + """) + with mock.patch('reno.utils.check_output') as co: + co.return_value = tag_list_output + actual = scanner._get_version_tags_on_branch('reporoot', + branch=None) + self.assertEqual(self.EXPECTED, actual) diff --git a/test-requirements.txt b/test-requirements.txt index 5956652..c352436 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -4,6 +4,8 @@ hacking<0.11,>=0.10.0 +mock>=1.2 + coverage>=3.6 discover python-subunit>=0.0.18