From e8792cec3c7ac20bedd44365694bc4aed1714cef Mon Sep 17 00:00:00 2001 From: Mathieu Bridon Date: May 26 2015 12:01:02 +0000 Subject: [PATCH 1/3] gitignore: Properly handle adding matching lines The GitIgnore.add method was trying to avoid adding lines which already are in the file. However, it wasn't trying to match wildcards, using the GitIgnore.match method. In addition, the GitIgnore user (Commands.upload) was trying to GitIgnore.match before trying to add the line. This commit makes GitIgnore.add check for matching lines properly, and simplifies Commands.upload in consequence. --- diff --git a/src/pyrpkg/__init__.py b/src/pyrpkg/__init__.py index 596d1b4..adae60f 100644 --- a/src/pyrpkg/__init__.py +++ b/src/pyrpkg/__init__.py @@ -2224,10 +2224,7 @@ class Commands(object): } raise rpkgError(msg) - # Add this file to .gitignore if it's not already there: - if not gitignore.match(file_basename): - gitignore.add('/%s' % file_basename) - + gitignore.add('/%s' % file_basename) self.lookasidecache.upload(self.module_name, f, file_hash) sourcesf.write() @@ -2466,12 +2463,12 @@ class GitIgnore(object): """ Add a line to .gitignore, but check if it's a duplicate first. """ - line = self.__ensure_newline(line) + if self.match(line): + return - # Add this line if it doesn't already exist: - if line not in self.__lines: - self.__lines.append(line) - self.modified = True + line = self.__ensure_newline(line) + self.__lines.append(line) + self.modified = True def match(self, line): line = line.lstrip('/').rstrip('\n') diff --git a/test/test_gitgnore.py b/test/test_gitgnore.py index 938abfd..dd55fc7 100644 --- a/test/test_gitgnore.py +++ b/test/test_gitgnore.py @@ -11,6 +11,27 @@ class GitIgnoreTestCase(unittest.TestCase): def tearDown(self): shutil.rmtree(self.workdir) + def test_add_existing_line(self): + import pyrpkg + gi = pyrpkg.GitIgnore(os.path.join(self.workdir, 'gitignore')) + gi.add('a new line') + self.assertTrue(gi.modified) + + # Cheat a bit for unit tests + gi.modified = False + + gi.add('a new line') + self.assertFalse(gi.modified) + + gi.add('*') + self.assertTrue(gi.modified) + + # Cheat a bit for unit tests + gi.modified = False + + gi.add('something different') + self.assertFalse(gi.modified) + def test_match_empty(self): import pyrpkg gi = pyrpkg.GitIgnore(os.path.join(self.workdir, 'gitignore')) From 49f269dd62a940e1c4aa22852ae0856b12a846dd Mon Sep 17 00:00:00 2001 From: Mathieu Bridon Date: May 26 2015 12:01:02 +0000 Subject: [PATCH 2/3] Modernize the gitignore-handling code This bundles a couple of minor changes. First, all file handling is now made with context managers, which are nicer and more correct, as they handle closing the file and error cases better. All API functions are now documented using the same standard docstring format as used in pyrpkg.lookaside. If we ever start generating API doc with something like Sphinx, then this class will already be covered. --- diff --git a/src/pyrpkg/__init__.py b/src/pyrpkg/__init__.py index adae60f..83416aa 100644 --- a/src/pyrpkg/__init__.py +++ b/src/pyrpkg/__init__.py @@ -2427,41 +2427,39 @@ class Commands(object): self.load_kojisession() class GitIgnore(object): - """ Smaller wrapper for managing a .gitignore file and it's entries. """ - + """A class to manage a .gitignore file""" def __init__(self, path): - """ - Create GitIgnore object for the given full path to a .gitignore file. + """Constructor - File does not have to exist yet, and will be created if you write out - any changes. + Args: + path (str): The full path to the .gitignore file. If it does not + exist, the file will be created when running GitIgnore.write() + for the first time. """ self.path = path # Lines of the .gitignore file, used to check if entries need to be # added or already exist. self.__lines = [] - if os.path.exists(self.path): - gitignore_file = open(self.path, 'r') - for line in gitignore_file: - self.__lines.append(self.__ensure_newline(line)) - gitignore_file.close() + if os.path.exists(self.path): + with open(self.path, 'r') as f: + for line in f: + self.__lines.append(self.__ensure_newline(line)) # Set to True if we end up making any modifications, used to # prevent unnecessary writes. self.modified = False def __ensure_newline(self, line): - """Append a newline character if the given line didn't have one""" - if line.endswith('\n'): - return line - - return '%s\n' % line + return line if line.endswith('\n') else '%s\n' % line def add(self, line): - """ - Add a line to .gitignore, but check if it's a duplicate first. + """Add a line + + Args: + line (str): The line to add to the file. It will not be added if + it already matches an existing line. """ if self.match(line): return @@ -2471,18 +2469,34 @@ class GitIgnore(object): self.modified = True def match(self, line): + """Check whether the line matches an existing one + + This uses fnmatch to match against wildcards. + + Args: + line (str): The new line to match against existing ones. + + Returns: + True if the new line matches, False otherwise. + """ line = line.lstrip('/').rstrip('\n') + for entry in self.__lines: entry = entry.lstrip('/').rstrip('\n') if fnmatch.fnmatch(line, entry): return True + return False def write(self): - """ Write the new .gitignore file if any modifications were made. """ + """Write the file to the disk + + This will only actually write if necessary, that is if lines have been + added since the last time the file was written. + """ if self.modified: - gitignore_file = open(self.path, 'w') - for line in self.__lines: - gitignore_file.write(line) - gitignore_file.close() + with open(self.path, 'w') as f: + for line in self.__lines: + f.write(line) + self.modified = False From 467527264470e64efdac6429f69fb2e3bc7c8860 Mon Sep 17 00:00:00 2001 From: Mathieu Bridon Date: May 26 2015 12:01:43 +0000 Subject: [PATCH 3/3] Move the GitIgnore class to its own module This reduces a bit the pressure on the main pyrpkg/__init__.py file, separating the concerns for readers of the code. --- diff --git a/src/pyrpkg/__init__.py b/src/pyrpkg/__init__.py index 83416aa..0021f25 100644 --- a/src/pyrpkg/__init__.py +++ b/src/pyrpkg/__init__.py @@ -37,6 +37,7 @@ except ImportError: from pyrpkg.errors import HashtypeMixingError, rpkgError, rpkgAuthError, \ UnknownTargetError +from .gitignore import GitIgnore from pyrpkg.lookaside import CGILookasideCache from pyrpkg.sources import SourcesFile from pyrpkg.utils import cached_property, warn_deprecated @@ -2425,78 +2426,3 @@ class Commands(object): finally: (self.build_client, self.kojiconfig) = koji_session_backup self.load_kojisession() - -class GitIgnore(object): - """A class to manage a .gitignore file""" - def __init__(self, path): - """Constructor - - Args: - path (str): The full path to the .gitignore file. If it does not - exist, the file will be created when running GitIgnore.write() - for the first time. - """ - self.path = path - - # Lines of the .gitignore file, used to check if entries need to be - # added or already exist. - self.__lines = [] - - if os.path.exists(self.path): - with open(self.path, 'r') as f: - for line in f: - self.__lines.append(self.__ensure_newline(line)) - - # Set to True if we end up making any modifications, used to - # prevent unnecessary writes. - self.modified = False - - def __ensure_newline(self, line): - return line if line.endswith('\n') else '%s\n' % line - - def add(self, line): - """Add a line - - Args: - line (str): The line to add to the file. It will not be added if - it already matches an existing line. - """ - if self.match(line): - return - - line = self.__ensure_newline(line) - self.__lines.append(line) - self.modified = True - - def match(self, line): - """Check whether the line matches an existing one - - This uses fnmatch to match against wildcards. - - Args: - line (str): The new line to match against existing ones. - - Returns: - True if the new line matches, False otherwise. - """ - line = line.lstrip('/').rstrip('\n') - - for entry in self.__lines: - entry = entry.lstrip('/').rstrip('\n') - if fnmatch.fnmatch(line, entry): - return True - - return False - - def write(self): - """Write the file to the disk - - This will only actually write if necessary, that is if lines have been - added since the last time the file was written. - """ - if self.modified: - with open(self.path, 'w') as f: - for line in self.__lines: - f.write(line) - - self.modified = False diff --git a/src/pyrpkg/gitignore.py b/src/pyrpkg/gitignore.py new file mode 100644 index 0000000..db3f3a5 --- /dev/null +++ b/src/pyrpkg/gitignore.py @@ -0,0 +1,90 @@ +# Copyright (c) 2015 - Red Hat Inc. +# +# This program is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by the +# Free Software Foundation; either version 2 of the License, or (at your +# option) any later version. See http://www.gnu.org/copyleft/gpl.html for +# the full text of the license. + + +"""Manage a .gitignore file""" + + +import fnmatch +import os + + +class GitIgnore(object): + """A class to manage a .gitignore file""" + def __init__(self, path): + """Constructor + + Args: + path (str): The full path to the .gitignore file. If it does not + exist, the file will be created when running GitIgnore.write() + for the first time. + """ + self.path = path + + # Lines of the .gitignore file, used to check if entries need to be + # added or already exist. + self.__lines = [] + + if os.path.exists(self.path): + with open(self.path, 'r') as f: + for line in f: + self.__lines.append(self.__ensure_newline(line)) + + # Set to True if we end up making any modifications, used to + # prevent unnecessary writes. + self.modified = False + + def __ensure_newline(self, line): + return line if line.endswith('\n') else '%s\n' % line + + def add(self, line): + """Add a line + + Args: + line (str): The line to add to the file. It will not be added if + it already matches an existing line. + """ + if self.match(line): + return + + line = self.__ensure_newline(line) + self.__lines.append(line) + self.modified = True + + def match(self, line): + """Check whether the line matches an existing one + + This uses fnmatch to match against wildcards. + + Args: + line (str): The new line to match against existing ones. + + Returns: + True if the new line matches, False otherwise. + """ + line = line.lstrip('/').rstrip('\n') + + for entry in self.__lines: + entry = entry.lstrip('/').rstrip('\n') + if fnmatch.fnmatch(line, entry): + return True + + return False + + def write(self): + """Write the file to the disk + + This will only actually write if necessary, that is if lines have been + added since the last time the file was written. + """ + if self.modified: + with open(self.path, 'w') as f: + for line in self.__lines: + f.write(line) + + self.modified = False diff --git a/test/test_gitgnore.py b/test/test_gitgnore.py index dd55fc7..2fc7c17 100644 --- a/test/test_gitgnore.py +++ b/test/test_gitgnore.py @@ -12,8 +12,9 @@ class GitIgnoreTestCase(unittest.TestCase): shutil.rmtree(self.workdir) def test_add_existing_line(self): - import pyrpkg - gi = pyrpkg.GitIgnore(os.path.join(self.workdir, 'gitignore')) + from pyrpkg.gitignore import GitIgnore + + gi = GitIgnore(os.path.join(self.workdir, 'gitignore')) gi.add('a new line') self.assertTrue(gi.modified) @@ -33,9 +34,9 @@ class GitIgnoreTestCase(unittest.TestCase): self.assertFalse(gi.modified) def test_match_empty(self): - import pyrpkg - gi = pyrpkg.GitIgnore(os.path.join(self.workdir, 'gitignore')) + from pyrpkg.gitignore import GitIgnore + gi = GitIgnore(os.path.join(self.workdir, 'gitignore')) self.assertFalse(gi.match('this does not exist')) # The empty string could match an empty file, but we don't want it to @@ -47,9 +48,9 @@ class GitIgnoreTestCase(unittest.TestCase): with open(gi_path, 'w') as f: f.write('this line exists\n') - import pyrpkg - gi = pyrpkg.GitIgnore(gi_path) + from pyrpkg.gitignore import GitIgnore + gi = GitIgnore(gi_path) self.assertTrue(gi.match('this line exists')) self.assertTrue(gi.match('this line exists\n')) self.assertTrue(gi.match('/this line exists')) @@ -58,16 +59,18 @@ class GitIgnoreTestCase(unittest.TestCase): self.assertFalse(gi.match('but this line does not')) def test_match_unwritten_line(self): - import pyrpkg - gi = pyrpkg.GitIgnore(os.path.join(self.workdir, 'gitignore')) + from pyrpkg.gitignore import GitIgnore + + gi = GitIgnore(os.path.join(self.workdir, 'gitignore')) gi.add('here is a new line') self.assertTrue(gi.modified) self.assertTrue(gi.match('here is a new line')) def test_match_glob(self): - import pyrpkg - gi = pyrpkg.GitIgnore(os.path.join(self.workdir, 'gitignore')) + from pyrpkg.gitignore import GitIgnore + + gi = GitIgnore(os.path.join(self.workdir, 'gitignore')) gi.add('*') self.assertTrue(gi.match('Surely this is matched by a wildcard?')) @@ -75,8 +78,9 @@ class GitIgnoreTestCase(unittest.TestCase): def test_write_new_file(self): gi_path = os.path.join(self.workdir, 'gitignore') - import pyrpkg - gi = pyrpkg.GitIgnore(gi_path) + from pyrpkg.gitignore import GitIgnore + + gi = GitIgnore(gi_path) gi.add('here is a new line') gi.write() @@ -93,8 +97,9 @@ class GitIgnoreTestCase(unittest.TestCase): with open(gi_path, 'w') as f: f.write(lines[0]) - import pyrpkg - gi = pyrpkg.GitIgnore(gi_path) + from pyrpkg.gitignore import GitIgnore + + gi = GitIgnore(gi_path) gi.add(lines[1]) gi.write()