It turns out that only the check-copyright-year script was exiting properly, but it was not doing the deferred exit that other tests were doing. Other tests would set errors=1, then later check the result and exit if it's non-zero, however the errors variable was set in a sub-shell, since it was on the right-hand side of a pipe. --- check | 81 ++++++++++++++----------------------------------- scripts/check-silliness | 63 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 59 deletions(-) create mode 100755 scripts/check-silliness diff --git a/check b/check index 160515f..4fd77cb 100755 --- a/check +++ b/check @@ -42,6 +42,28 @@ case "$PYTHONPATH" in esac export PYTHONPATH +# Run the style checks + +errors=0 +if [ -d .git ]; +then + echo "Checking copyright statements" + if ! (git ls-files -z | xargs -0r scripts/check-copyright-year); then + errors=1 + fi + + echo 'Checking source code for silliness' + if ! (git ls-files | + grep -v '\.gz$' | + grep -Ev 'tests[^/]*/.*\.std(out|err)' | + grep -vF 'tests.build/build-system-autotools.script' | + xargs -r scripts/check-silliness); then + errors=1 + fi +fi +if [ "$errors" != 0 ]; then + exit "$errors" +fi # Clean up artifacts from previous (possibly failed) runs, build, # and run the tests. @@ -97,62 +119,3 @@ then else echo "NOT RUNNING tests.as-root (requires PyYAML)" fi - -if [ -d .git ]; -then - echo "Checking copyright statements" - git ls-files | xargs scripts/check-copyright-year - - echo 'Checking source code for silliness' - git ls-files | - grep -v '\.gz$' | - grep -Ev 'tests[^/]*/.*\.std(out|err)' | - grep -vF 'tests.build/build-system-autotools.script' | - while read x - do - if tr -cd '\t' < "$x" | grep . > /dev/null - then - echo "ERROR: $x contains TAB characters" 1>&2 - grep -n -F "$(printf "\t")" "$x" 1>&2 - errors=1 - fi - - case "$x" in - # Excluding yarn files since it's not possible to split up the - # IMPLEMENTS lines of them - *.yarn) ;; - *) - if awk 'length > 79' "$x" | grep . > /dev/null - then - echo "ERROR: $x has lines longer than 79 chars" 1>&2 - awk 'length > 79 { print NR, $0 }' "$x" 1>&2 - errors=1 - fi - ;; - esac - - case "$x" in - *.py) - if head -1 "$x" | grep '^#!' > /dev/null - then - echo "ERROR: $x has a hashbang" 1>&2 - errors=1 - fi - if grep except: "$x" - then - echo "ERROR: $x has a bare except:" 1>&2 - errors=1 - fi - ;; - esac - done - - echo 'Checking for executable *.py files' - find . -type f -name '*.py' -perm +111 | - while read x - do - echo "ERROR: $x is executable" 1>&2 - errors=1 - done -fi -exit $errors diff --git a/scripts/check-silliness b/scripts/check-silliness new file mode 100755 index 0000000..f956e64 --- /dev/null +++ b/scripts/check-silliness @@ -0,0 +1,63 @@ +#!/bin/sh +# +# Does the file contain any of the code constructs deemed silly? +# +# Copyright (C) 2013 Codethink Limited +# +# 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; version 2 of the License. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with this program; if not, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +errors=0 + +for x; do + if tr -cd '\t' < "$x" | grep . > /dev/null + then + echo "ERROR: $x contains TAB characters" 1>&2 + grep -n -F "$(printf "\t")" "$x" 1>&2 + errors=1 + fi + + case "$x" in + # Excluding yarn files since it's not possible to split up the + # IMPLEMENTS lines of them + *.yarn) ;; + *) + if awk 'length > 79' "$x" | grep . > /dev/null + then + echo "ERROR: $x has lines longer than 79 chars" 1>&2 + awk 'length > 79 { print NR, $0 }' "$x" 1>&2 + errors=1 + fi + ;; + esac + + case "$x" in + *.py) + if head -1 "$x" | grep '^#!' > /dev/null + then + echo "ERROR: $x has a hashbang" 1>&2 + errors=1 + fi + if [ -x "$x" ]; then + echo "ERROR: $x is executable" 1>&2 + errors=1 + fi + if grep except: "$x" + then + echo "ERROR: $x has a bare except:" 1>&2 + errors=1 + fi + ;; + esac +done +exit "$errors" -- 1.8.5.rc2

--- morphlib/plugins/branch_and_merge_new_plugin.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/morphlib/plugins/branch_and_merge_new_plugin.py b/morphlib/plugins/branch_and_merge_new_plugin.py index 9c4cd53..09dd5bd 100644 --- a/morphlib/plugins/branch_and_merge_new_plugin.py +++ b/morphlib/plugins/branch_and_merge_new_plugin.py @@ -351,18 +351,6 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin): logging.debug('All strata loaded') return morphset - def _invent_new_branch(self, cached_repo, default_name): - counter = 0 - candidate = default_name - while True: - try: - cached_repo.resolve_ref(candidate) - except morphlib.cachedrepo.InvalidReferenceError: - return candidate - else: - counter += 1 - candidate = '%s-%s' % (default_name, counter) - def edit(self, args): '''Edit or checkout a component in a system branch. -- 1.8.5.rc2

On 15/11/13 18:19, Richard Maw wrote: Could we replace this entire class with this guy? http://pythonhosted.org/fs/tempfs.html

On Mon, Nov 18, 2013 at 09:42:14AM +0000, Dan Firth wrote: +0 on the use of pyfilesystem for this. It can be done as part of this patch series or as a follow-up (perhaps by Dan) later. Either is fine by me. -- http://www.codethink.co.uk/ http://wiki.baserock.org/ http://www.baserock.com/

--- morphlib/gitdir.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/morphlib/gitdir.py b/morphlib/gitdir.py index 7ee64f3..a3e5b6f 100644 --- a/morphlib/gitdir.py +++ b/morphlib/gitdir.py @@ -100,10 +100,6 @@ class GitDirectory(object): return self._runcmd( ['git', 'cat-file', obj_type, '%s:%s' % (ref, filename)]) - def update_remotes(self): # pragma: no cover - '''Update remotes.''' - self._runcmd(['git', 'remote', 'update', '--prune']) - def update_submodules(self, app): # pragma: no cover '''Change .gitmodules URLs, and checkout submodules.''' morphlib.git.update_submodules(app, self.dirname) -- 1.8.5.rc2

This moves the step of joining a treeish to a file path to locate an object outside the cat_file call, since cat_file is useful for just having the commit or blob sha1. --- morphlib/gitdir.py | 6 +++--- morphlib/plugins/branch_and_merge_new_plugin.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/morphlib/gitdir.py b/morphlib/gitdir.py index 5a62253..d731db2 100644 --- a/morphlib/gitdir.py +++ b/morphlib/gitdir.py @@ -96,9 +96,9 @@ class GitDirectory(object): parsed_head = self._runcmd(['git', 'rev-parse', 'HEAD']).strip() return parsed_ref == parsed_head - def cat_file(self, obj_type, ref, filename): # pragma: no cover + def cat_file(self, obj_type, object): # pragma: no cover return self._runcmd( - ['git', 'cat-file', obj_type, '%s:%s' % (ref, filename)]) + ['git', 'cat-file', obj_type, object]) def update_submodules(self, app): # pragma: no cover '''Change .gitmodules URLs, and checkout submodules.''' @@ -190,7 +190,7 @@ class GitDirectory(object): with open(os.path.join(self.dirname, filename)) as f: return f.read() tree = self._rev_parse_tree(ref) - return self.cat_file('blob', tree, filename) + return self.cat_file('blob', '%s:%s' % (tree, filename)) @property def HEAD(self): diff --git a/morphlib/plugins/branch_and_merge_new_plugin.py b/morphlib/plugins/branch_and_merge_new_plugin.py index 09dd5bd..bd33fc9 100644 --- a/morphlib/plugins/branch_and_merge_new_plugin.py +++ b/morphlib/plugins/branch_and_merge_new_plugin.py @@ -305,9 +305,9 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin): def _load_morphology_from_git(self, loader, gd, ref, filename): try: - text = gd.cat_file('blob', ref, filename) + text = gd.cat_file('blob', '%s:%s' % (ref, filename)) except cliapp.AppException: - text = gd.cat_file('blob', 'origin/%s' % ref, filename) + text = gd.cat_file('blob', 'origin/%s:%s' % (ref, filename)) return loader.load_from_string(text, filename) def _load_stratum_morphologies(self, loader, sb, system_morph): -- 1.8.5.rc2

Some APIs should take SHA1 object IDs, there needs to be a way to get one from a ref. To handle this, we add rev_parse to the GitDirectory API. `git rev-parse` does a lot more than just resolving revisions, but we're exposing it as just that. --- morphlib/gitdir.py | 8 +++++--- morphlib/gitdir_tests.py | 7 +++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/morphlib/gitdir.py b/morphlib/gitdir.py index d731db2..d6a2f4d 100644 --- a/morphlib/gitdir.py +++ b/morphlib/gitdir.py @@ -161,13 +161,15 @@ class GitDirectory(object): else: return self._list_files_in_ref(ref) - def _rev_parse_tree(self, ref): + def rev_parse(self, ref): try: - return self._runcmd(['git', 'rev-parse', '--verify', - '%s^{tree}' % ref]).strip() + return self._runcmd(['git', 'rev-parse', '--verify', ref]).strip() except cliapp.AppException as e: raise InvalidRefError(self, ref) + def _rev_parse_tree(self, ref): + return self.rev_parse('%s^{tree}' % ref) + def _list_files_in_work_tree(self): for dirpath, subdirs, filenames in os.walk(self.dirname): if dirpath == self.dirname and '.git' in subdirs: diff --git a/morphlib/gitdir_tests.py b/morphlib/gitdir_tests.py index 803f1b3..f99569c 100644 --- a/morphlib/gitdir_tests.py +++ b/morphlib/gitdir_tests.py @@ -148,6 +148,13 @@ class GitDirectoryContentsTests(unittest.TestCase): gd.checkout('foo') self.assertEqual(gd.HEAD, 'foo') + def test_rev_parse(self): + # Just tests that you get a commit back, since checking the actual + # value of the commit requires foreknowledge of the result or + # re-implementing the body in the test. + gd = morphlib.gitdir.GitDirectory(self.dirname) + self.assertEqual(len(gd.rev_parse(gd.HEAD)), 40) + def test_uncommitted_changes(self): gd = morphlib.gitdir.GitDirectory(self.dirname) self.assertEqual(sorted(gd.get_uncommitted_changes()), -- 1.8.5.rc2