Hi,
thanks for your patch. See some comments below:
On Sat, May 19, 2012 at 03:23:36PM +0200, Daniel Dehennin wrote:
[..snip..]
> From 48a9ba5b6c8f1fd3ed9e946281b82447caadad0e Mon Sep 17 00:00:00 2001
> From: Daniel Dehennin <daniel.dehen...@baby-gnu.org>
> Date: Mon, 14 May 2012 18:15:06 +0200
> Subject: [PATCH 2/2] Create debian/changelog if it does not exist.
>
> The main purpose of this patch is to permit the creation of an empty
> changelog object.
>
> An empty ChangeLog object can be created if the changelog file is
> inexistant, null size is not enought for "dch --create".
>
> The main difference results in the need to:
> - guess the version number if none was provided on the command line
> - try harder to guess the commit to start from
> - take care of access to inexistant ChangeLog attributes.
>
> * tests/11_test_changelog_create.py: Test several senarios of
> debian/changelog creation.
>
> * gbp/deb/changelog.py (__init__): Default changelog filename to
> "debian/changelog". Permit to create empty changelog, take care that
> cp['Changes'] and cp['Version'] must be strings.
> (is_empty): Test if the changelog is present, the dch tool only support
> "--create" when the changelog file is absent, not if its size is null.
I wonder if it wouldn't be easier to just add a simple ChangeLog.creates
method that creats a changelog? Something like:
sripts.dch:
try:
cp = ChangeLog(filename=changelog)
expcept NoChangeLogError:
if options.create_changelog:
cp = None
else:
raise
# Once you have all the necessary data
cp = ChangeLog.create(...)
gbp.deb.changelog:
class ChangeLog
...
@classmethod
def create(klass, filename, content=''):
<... create changelog (e.g. by calling dch --create >
return klass(filename=filename)
this would get you out of the whole is_empty mess. I could add the
part in gbp.deb.ChangeLog if this helps?
> * gbp/scripts/dch.py: Import new gbp.deb.control.
> (spawn_dch): Set environnement variable EDITOR to /bin/true when
> calling dch with "--create".
> (add_changelog_section): We need to guess version from upstream is the
> changelog is empty and no version was provided. Take care to create the
> changelog if it's empty.
> (guess_snapshot_commit): Can not call repo.find_version() if the
> changelog is empty.
> (guess_version_from_upstream): cp is None when called by
> ChangeLog.create().
> (main): Require the options "upstream-tree" and "upstream-branch".
> Try hard to guess the last commit, last to merge-base.
I'd recommend splitting the ChangeLog work from the dch changes since
those should be mostly independent and so things are easier to review
and merge.
Cheers,
-- Guido
> ---
> gbp/deb/changelog.py | 98 +++++++++++++++++------------
> gbp/scripts/dch.py | 40 ++++++++++--
> tests/11_test_changelog_create.py | 123
> +++++++++++++++++++++++++++++++++++++
> 3 files changed, 216 insertions(+), 45 deletions(-)
> create mode 100644 tests/11_test_changelog_create.py
>
> diff --git a/gbp/deb/changelog.py b/gbp/deb/changelog.py
> index 7dc51d9..dec8fc3 100644
> --- a/gbp/deb/changelog.py
> +++ b/gbp/deb/changelog.py
> @@ -62,55 +62,66 @@ class ChangeLogSection(object):
> class ChangeLog(object):
> """A Debian changelog"""
>
> - def __init__(self, contents=None, filename=None):
> + def __init__(self, contents=None, filename="debian/changelog"):
We don't want to change the default signature.
> """
> Parse an existing changelog, Either contents, containing the contents
> of a changelog file, or filename, pointing to a changelog file must
> be
> passed.
> """
> - # Check that either contents or filename is passed (but not both)
> - if (not filename and not contents) or (filename and contents):
> - raise Exception("Either filename or contents must be passed")
> -
> - # If a filename was passed, check if it exists
> - if filename and not os.access(filename, os.F_OK):
> - raise NoChangeLogError, "Changelog %s not found" % (filename, )
> -
> - # If no filename was passed, let's read from stdin
> - if not filename:
> - filename = '-'
> -
> - # Note that if contents is None, stdin will just be closed right
> - # away by communicate.
> - cmd = subprocess.Popen(['dpkg-parsechangelog', '-l%s' % filename],
> - stdin=subprocess.PIPE,
> - stdout=subprocess.PIPE,
> - stderr=subprocess.PIPE)
> - (output, errors) = cmd.communicate(contents)
> - if cmd.returncode:
> - raise ParseChangeLogError("Failed to parse changelog. "
> - "dpkg-parsechangelog said:\n%s" %
> (errors, ))
> - # Parse the result of dpkg-parsechangelog (which looks like
> - # email headers)
> - cp = email.message_from_string(output)
> - try:
> - if ':' in cp['Version']:
> - cp['Epoch'], cp['NoEpoch-Version'] =
> cp['Version'].split(':', 1)
> - else:
> - cp['NoEpoch-Version'] = cp['Version']
> - if '-' in cp['NoEpoch-Version']:
> - cp['Upstream-Version'], cp['Debian-Version'] =
> cp['NoEpoch-Version'].rsplit('-', 1)
> - else:
> - cp['Debian-Version'] = cp['NoEpoch-Version']
> - except TypeError:
> - raise ParseChangeLogError, output.split('\n')[0]
>
> - self._cp = cp
> + # Parse contents first or filename if non empty.
> + empty = False
This add much churn for no need. Wouldn't it be nice if we knew that
ther's something to parse? We could special case the empty changelog but
in that case we need to just return much earlier instead of adding much
if ... else ...
> if contents:
> + read_from = '-'
> self._contents = contents[:]
> - else:
> + elif os.access(filename, os.F_OK) and os.stat(filename).st_size > 0:
> + read_from = filename
> with file(filename) as f:
> self._contents = f.read()
> + elif os.access(os.path.realpath(os.path.dirname(filename)), os.W_OK):
> + # Use os.path.realpath to cope with
> + # os.path.dirname('changelog') returning ''
> + #
> + # Empty changelog but can be created
> + empty = True
> + else:
> + raise NoChangeLogError("Empty contents and changelog %s, parent
> changelog directory not writable" % filename)
> +
> + output = ''
> + if not empty:
> + # Note that if contents is None, stdin will just be closed right
> + # away by communicate.
> + cmd = subprocess.Popen(['dpkg-parsechangelog', '-l%s' %
> read_from],
> + stdin=subprocess.PIPE,
> + stdout=subprocess.PIPE,
> + stderr=subprocess.PIPE)
> + (output, errors) = cmd.communicate(contents)
> + if cmd.returncode:
> + raise ParseChangeLogError("Failed to parse changelog. "
> + "dpkg-parsechangelog said:\n%s" %
> (errors, ))
> +
> + # Parse the result of dpkg-parsechangelog (which looks like
> + # email headers)
> + cp = email.message_from_string(output)
> +
> + if not empty:
> + try:
> + if ':' in cp['Version']:
> + cp['Epoch'], cp['NoEpoch-Version'] =
> cp['Version'].split(':', 1)
> + else:
> + cp['NoEpoch-Version'] = cp['Version']
> + if '-' in cp['NoEpoch-Version']:
> + cp['Upstream-Version'], cp['Debian-Version'] =
> cp['NoEpoch-Version'].rsplit('-', 1)
> + else:
> + cp['Debian-Version'] = cp['NoEpoch-Version']
> + except TypeError:
> + raise ParseChangeLogError, output.split('\n')[0]
> + else:
> + cp['Changes'] = ''
> + cp['Version'] = ''
> +
> + self._cp = cp
> + self.filename = filename
>
> def __getitem__(self, item):
> return self._cp[item]
> @@ -204,3 +215,12 @@ class ChangeLog(object):
> Get sections in the changelog
> """
> return list(self.sections_iter)
> +
> + def is_empty(self):
> + """
> + Whether the changelog file does not exist
> +
> + @return: C{True} if the changelog file is missing, C{False} otherwise
> + @rtype: C{bool}
> + """
> + return not os.access(self.filename, os.F_OK)
Why not use the number of sections in the ChangeLog - it also works for
the content case (no filename).
> diff --git a/gbp/scripts/dch.py b/gbp/scripts/dch.py
> index 14dff29..e8b2817 100644
> --- a/gbp/scripts/dch.py
> +++ b/gbp/scripts/dch.py
> @@ -31,6 +31,7 @@ from gbp.errors import GbpError
> from gbp.deb import compare_versions
> from gbp.deb.git import GitRepositoryError, DebianGitRepository
> from gbp.deb.changelog import ChangeLog, NoChangeLogError
> +from gbp.deb.control import Control
>
> user_customizations = {}
> snapshot_re = re.compile("\s*\*\* SNAPSHOT build
> @(?P<commit>[a-z0-9]+)\s+\*\*")
> @@ -75,6 +76,9 @@ def spawn_dch(msg=[], author=None, email=None,
> newversion=False, version=None,
> if author and email:
> env = """DEBFULLNAME="%s" DEBEMAIL="%s" """ % (author, email)
>
> + if dch_options.find("--create") != -1:
> + env += "EDITOR=/bin/true"
> +
Why would this be noninteractive?
> if distribution:
> distopt = "--distribution=%s" % distribution
>
> @@ -124,10 +128,14 @@ def guess_version_from_upstream(repo,
> upstream_tag_format, cp):
> def add_changelog_section(msg, distribution, repo, options, cp,
> author=None, email=None, version={},
> dch_options=''):
> """Add a new section to the changelog"""
> - if not version and not cp.is_native():
> + if not version and (cp.is_empty() or not cp.is_native()):
> v = guess_version_from_upstream(repo, options.upstream_tag, cp)
> if v:
> version['version'] = v
> + if cp.is_empty():
> + control = Control()
> + dch_options += " --create --package=%s" % control.name
> +
Your other patch converted dch_options to a list that was much nicer!
> spawn_dch(msg=msg, newversion=True, version=version, author=author,
> email=email, distribution=distribution,
> dch_options=dch_options)
>
> @@ -269,7 +277,7 @@ def guess_snapshot_commit(cp, repo, options):
> # If the current topmost changelog entry has already been tagged rely on
> # the version information only. The upper level relies then on the
> version
> # info anyway:
> - if repo.find_version(options.debian_tag, cp.version):
> + if not cp.is_empty() and repo.find_version(options.debian_tag,
> cp.version):
> return None
Is there any point in trying to guess a snapshot commit from an empty
changelog? If not we shouldn't ever end up here.
> # If we didn't find a snapshot header we look at the point the changelog
> # was last touched.
> @@ -357,6 +365,8 @@ def main(argv):
> parser.add_option_group(custom_group)
>
> parser.add_boolean_config_file_option(option_name = "ignore-branch",
> dest="ignore_branch")
> + naming_group.add_config_file_option(option_name="upstream-tree",
> dest="upstream_tree")
> + naming_group.add_config_file_option(option_name="upstream-branch",
> dest="upstream_branch")
> naming_group.add_config_file_option(option_name="debian-branch",
> dest="debian_branch")
> naming_group.add_config_file_option(option_name="upstream-tag",
> dest="upstream_tag")
> naming_group.add_config_file_option(option_name="debian-tag",
> dest="debian_tag")
> @@ -429,7 +439,7 @@ def main(argv):
> since = options.since
> else:
> since = ''
> - if options.auto:
> + if options.auto or cp.is_empty():
> since = guess_snapshot_commit(cp, repo, options)
Does this make any sense for an empty changelog?
> if since:
> gbp.log.info("Continuing from commit '%s'" % since)
> @@ -437,7 +447,24 @@ def main(argv):
> else:
> gbp.log.info("Couldn't find snapshot header, using
> version info")
> if not since:
> - since = repo.find_version(options.debian_tag, cp['Version'])
> + # Take care of empty debian/changelog
> + if cp.is_empty():
> + pattern = options.upstream_tag % dict(version='*')
> + upstream = None
> + try:
> + upstream = repo.find_tag('HEAD', pattern=pattern)
> + gbp.log.debug('Found upstream tag %s' % upstream)
> + except GitRepositoryError:
> + gbp.log.debug('No upstream tag found, try upstream
> branch')
> + if upstream == None or options.upstream_tree == 'branch':
> + upstream = options.upstream_branch
> + since = repo.get_merge_base('HEAD', upstream)
> + gbp.log.debug("Found merge-base %s" % since)
> + if since and options.snapshot:
> + # Snapshot can not be guessed
> + found_snapshot_header = True
We did not find a snapshot header, did we?
This whole block should go to a separate function since it's only
purpose is guessing the commits in case of an empty changelog.
If we also set options.new_version here...
> + else:
> + since = repo.find_version(options.debian_tag,
> cp['Version'])
> if not since:
> raise GbpError, "Version %s not found" % cp['Version']
>
> @@ -448,16 +475,17 @@ def main(argv):
> commits.reverse()
>
> # add a new changelog section if:
> - if options.new_version or options.bpo or options.nmu or options.qa:
> + if cp.is_empty() or options.new_version or options.bpo or
> options.nmu or options.qa:
... we don't need to special case here.
> if options.bpo:
> version_change['increment'] = '--bpo'
> elif options.nmu:
> version_change['increment'] = '--nmu'
> elif options.qa:
> version_change['increment'] = '--qa'
> - else:
> + elif options.new_version:
> version_change['version'] = options.new_version
> # the user wants to force a new version
> + # or the changelog was empty and we need to guess version
> add_section = True
> elif cp['Distribution'] != "UNRELEASED" and not
> found_snapshot_header and commits:
> # the last version was a release and we have pending commits
> diff --git a/tests/11_test_changelog_create.py
> b/tests/11_test_changelog_create.py
> new file mode 100644
> index 0000000..b613ce5
> --- /dev/null
> +++ b/tests/11_test_changelog_create.py
> @@ -0,0 +1,123 @@
> +# vim: set fileencoding=utf-8 :
> +
> +"""Test L{Changelog}'s create"""
> +
> +import unittest
> +
> +from testutils import DebianGitTestRepo
> +
> +from gbp.scripts import dch
> +from gbp.deb.changelog import ChangeLog
> +
> +import os
> +import re
> +
> +snap_header =
> r'^test-package\s\(1.0-1~1\.gbp([0-9a-f]{6})\)\sUNRELEASED;\surgency=low'
> +snap_mark = r'\s{2}\*{2}\sSNAPSHOT\sbuild\s@'
> +
> +class TestScriptDch(DebianGitTestRepo):
> + """Test git-dch"""
> +
> + def setUp(self):
> + DebianGitTestRepo.setUp(self)
> + self.add_file("foo", "bar")
> + self.repo.create_tag("upstream/1.0", msg="upstream version 1.0")
> + self.repo.create_branch("debian")
> + self.repo.set_branch("debian")
> + self.upstream_tag = "upstream/%(version)s"
> + self.top = os.path.abspath(os.path.curdir)
> + os.mkdir(os.path.join(self.repo.path, "debian"))
> + os.chdir(self.repo.path)
> + self.add_file("debian/control", """Source: test-package\nSection:
> test\n""")
> +
> + def tearDown(self):
> + os.chdir(self.top)
> + DebianGitTestRepo.tearDown(self)
> +
> + def test_create_from_dch_main(self):
> + """Test dch.py like git-dch script does: must not include upstream
> log in debian/changelog"""
> + ret = dch.main(["--upstream-tag=%s" % self.upstream_tag,
> "--debian-branch=debian"])
> + self.assertEqual(ret, 0)
> + lines = file("debian/changelog").readlines()
> + assert "test-package (1.0-1) UNRELEASED; urgency=low\n" in lines
> + assert """ * added debian/control\n""" in lines
This is still using assert instead of the unit test helpers.
> +
> + def test_create_from_dch_main_with_auto(self):
> + """Test dch.py like git-dch script does: guess last commit"""
> + ret = dch.main(["--upstream-tag=%s" % self.upstream_tag,
> "--debian-branch=debian", "-a"])
> + self.assertEqual(ret, 0)
> + lines = file("debian/changelog").readlines()
> + assert "test-package (1.0-1) UNRELEASED; urgency=low\n" in lines
> + assert """ * added debian/control\n""" in lines
> +
> + def test_create_from_dch_main_with_snapshot(self):
> + """Test dch.py like git-dch script does: snashot mode"""
> + ret = dch.main(["--upstream-tag=%s" % self.upstream_tag,
> "--debian-branch=debian", "-S"])
> + self.assertEqual(ret, 0)
> + lines = file("debian/changelog").readlines()
> + header = re.search(snap_header, lines[0])
> + self.assertEqual(header.lastindex, 1)
> + assert re.search(snap_mark + header.group(1), lines[2])
> + assert """ * added debian/control\n""" in lines
> +
> + def test_create_from_dch_main_after_delete(self):
> + """Test dch.py like git-dch script does: debian/changelog was
> deleted"""
> + self.add_file("debian/changelog", "fake")
> + self.repo.remove_files("debian/changelog")
> + self.repo.commit_files(self.repo.path, "removed debian/changelog")
> + ret = dch.main(["--upstream-tag=%s" % self.upstream_tag,
> "--debian-branch=debian"])
> + self.assertEqual(ret, 0)
> + lines = file("debian/changelog").readlines()
> + assert "test-package (1.0-1) UNRELEASED; urgency=low\n" in lines
> + assert """ * removed debian/changelog\n""" not in lines
> + assert """ * UNRELEASED\n""" in lines
> +
> + def test_create_from_dch_main_with_auto_snapshot(self):
> + """Test dch.py like git-dch script does: guess last commit,
> snashot"""
> + ret = dch.main(["--upstream-tag=%s" % self.upstream_tag,
> "--debian-branch=debian", "-a", "-S"])
> + self.assertEqual(ret, 0)
> + lines = file("debian/changelog").readlines()
> + header = re.search(snap_header, lines[0])
> + self.assertEqual(header.lastindex, 1)
> + assert re.search(snap_mark + header.group(1), lines[2])
> + assert """ * added debian/control\n""" in lines
> +
> + def test_create_from_dch_main_with_auto_after_delete(self):
> + """Test dch.py like git-dch script does: guess last commit,
> debian/changelog was deleted"""
> + self.add_file("debian/changelog", "fake")
> + self.repo.remove_files("debian/changelog")
> + self.repo.commit_files(self.repo.path, "removed debian/changelog")
> + ret = dch.main(["--upstream-tag=%s" % self.upstream_tag,
> "--debian-branch=debian", "-a"])
> + self.assertEqual(ret, 0)
> + lines = file("debian/changelog").readlines()
> + assert "test-package (1.0-1) UNRELEASED; urgency=low\n" in lines
> + assert """ * removed debian/changelog\n""" not in lines
> + assert """ * UNRELEASED\n""" in lines
> +
> + def test_create_from_dch_main_with_snapshot_after_delete(self):
> + """Test dch.py like git-dch script does: snapshot, debian/changelog
> was deleted"""
> + self.add_file("debian/changelog", "fake")
> + self.repo.remove_files("debian/changelog")
> + self.repo.commit_files(self.repo.path, "removed debian/changelog")
> + ret = dch.main(["--upstream-tag=%s" % self.upstream_tag,
> "--debian-branch=debian", "-S"])
> + self.assertEqual(ret, 0)
> + lines = file("debian/changelog").readlines()
> + header = re.search(snap_header, lines[0])
> + self.assertEqual(header.lastindex, 1)
> + assert re.search(snap_mark + header.group(1), lines[2])
> + assert """ * removed debian/changelog\n""" not in lines
> + assert """ * UNRELEASED\n""" in lines
> +
> + def test_create_from_dch_main_with_auto_snapshot_after_delete(self):
> + """Test dch.py like git-dch script does: guess last commit,
> snapshot, debian/changelog was deleted"""
> + self.add_file("debian/changelog", "fake")
> + self.repo.remove_files("debian/changelog")
> + self.repo.commit_files(self.repo.path, "removed debian/changelog")
> + ret = dch.main(["--upstream-tag=%s" % self.upstream_tag,
> "--debian-branch=debian", "-a", "-S"])
> + self.assertEqual(ret, 0)
> + lines = file("debian/changelog").readlines()
> + header = re.search(snap_header, lines[0])
> + self.assertEqual(header.lastindex, 1)
> + assert re.search(snap_mark + header.group(1), lines[2])
> + assert """ * removed debian/changelog\n""" not in lines
> + assert """ * UNRELEASED\n""" in lines
> --
> 1.7.10
>
>
> --
> Daniel Dehennin
> Récupérer ma clef GPG:
> gpg --keyserver pgp.mit.edu --recv-keys 0x7A6FE2DF
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
