From 469459ac1a4cd3bd7b5161fe8d71567190257b56 Mon Sep 17 00:00:00 2001 From: Lars Wirzenius Date: Wed, 23 Oct 2013 15:54:02 +0000 Subject: Fix CachedRepo.resolve_ref unit test for missing SHA1 CachedRepo.resolve_ref does, effectively, this: absref = git rev-parse --verify $ref git log -1 --format=format:%T $absref Roughly, "git rev-parse" takes any ref and returns the corresponding SHA1. If the ref looks like a SHA1 (40 hex digits), it is returned as-is, and despite --verify is not checked for existence. "git log" then takes the SHA1 and returns the **tree** SHA1, as opposed to the commit one, and if the commit doesn't exist, barfs. The unit test for resolve_ref with an invalid SHA1 currently succeeds for the wrong reason. The mocked _rev_parse fails for an unknown SHA1 (raising cliapp.AppException), which causes resolve_ref to raise InvalidReferenceError, which the unit test expects. However, the real implementation of _rev_parse wouldn't fail in that way, and so the unit test doesn't test the thing it's meant to test: that resolve_ref actually works the expected way for an unknown SHA1. What actually happens is that resolve_ref calls _show_tree_hash, which raises cliapp.AppException for an unknown SHA1, resulting in horror and despair, instead of resolve_ref raising InvalidReferenceError. This commit fixes the unit test so that it causes the right code path in resolve_ref to be executed. This makes the unit test suite to fail. --- morphlib/cachedrepo_tests.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/morphlib/cachedrepo_tests.py b/morphlib/cachedrepo_tests.py index 18704f58..b1825eba 100644 --- a/morphlib/cachedrepo_tests.py +++ b/morphlib/cachedrepo_tests.py @@ -32,8 +32,11 @@ class CachedRepoTests(unittest.TestCase): "kind": "chunk" }''' + bad_sha1_known_to_rev_parse = 'cafecafecafecafecafecafecafecafecafecafe' + def rev_parse(self, ref): output = { + self.bad_sha1_known_to_rev_parse: self.bad_sha1_known_to_rev_parse, 'a4da32f5a81c8bc6d660404724cedc3bc0914a75': 'a4da32f5a81c8bc6d660404724cedc3bc0914a75', 'e28a23812eadf2fce6583b8819b9c5dbd36b9fb9': @@ -159,7 +162,7 @@ class CachedRepoTests(unittest.TestCase): def test_fail_resolving_an_invalid_sha1_ref(self): self.assertRaises(cachedrepo.InvalidReferenceError, self.repo.resolve_ref, - '079bbfd447c8534e464ce5d40b80114c2022ebf4') + self.bad_sha1_known_to_rev_parse) def test_cat_existing_file_in_existing_ref(self): data = self.repo.cat('e28a23812eadf2fce6583b8819b9c5dbd36b9fb9', -- cgit v1.2.1 From bcb725108c291611d125f8c5048a368dd5cdadb4 Mon Sep 17 00:00:00 2001 From: Lars Wirzenius Date: Wed, 23 Oct 2013 16:21:42 +0000 Subject: Make CachedRepo.resolve_ref handle non-existent SHA1 This changes how CachedRepo runs git to get the SHA1 information it needs, based on a suggestion by Richard Maw. --- morphlib/cachedrepo.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/morphlib/cachedrepo.py b/morphlib/cachedrepo.py index 0a085bb2..e85b0059 100644 --- a/morphlib/cachedrepo.py +++ b/morphlib/cachedrepo.py @@ -1,4 +1,4 @@ -# Copyright (C) 2012 Codethink Limited +# Copyright (C) 2012-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 @@ -126,7 +126,11 @@ class CachedRepo(object): except cliapp.AppException: raise InvalidReferenceError(self, ref) - tree = self._show_tree_hash(absref) + try: + tree = self._show_tree_hash(absref) + except cliapp.AppException: + raise InvalidReferenceError(self, ref) + return absref, tree def cat(self, ref, filename): @@ -239,11 +243,12 @@ class CachedRepo(object): return self.app.runcmd(*args, **kwargs) def _rev_parse(self, ref): # pragma: no cover - return self._runcmd(['git', 'rev-parse', '--verify', ref])[0:40] + return self._runcmd( + ['git', 'rev-parse', '--verify', '%s^{commit}' % ref])[0:40] def _show_tree_hash(self, absref): # pragma: no cover return self._runcmd( - ['git', 'log', '-1', '--format=format:%T', absref]).strip() + ['git', 'rev-parse', '--verify', '%s^{tree}' % absref]).strip() def _ls_tree(self, ref): # pragma: no cover result = self._runcmd(['git', 'ls-tree', '--name-only', ref]) -- cgit v1.2.1 From 24e0e1ff64b1bcaede0366f9c0765bc740754da8 Mon Sep 17 00:00:00 2001 From: Lars Wirzenius Date: Wed, 23 Oct 2013 16:22:50 +0000 Subject: Fix app.status call in log_dict_diff This bug was triggered by the fix to CachedRepo.resolve_ref and without this fix, the resolve_ref fix will break the test suite. The bug is that log_dict_diff calls the status method with an msg keyword argument that may contain percentage characters. status interprets the value of msg as a format string, and the percentage characters trigger formatting to happen. The fix for that is to not interpolate the value of key and dictA[key] and dictB[key] into msg before calling status, but letting status do that. Thus the msg values are changed to reference %(key)s instead and passing in a value for key as a separate argument. Ditto for dictA[key] and dictB[key]. --- morphlib/util.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/morphlib/util.py b/morphlib/util.py index 3d9232d4..53a9e283 100644 --- a/morphlib/util.py +++ b/morphlib/util.py @@ -177,7 +177,8 @@ def log_dict_diff(app, cur, pre): # pragma: no cover dictB = pre for key in dictA.keys(): if key not in dictB: - app.status(msg="New environment: %s = %s" % (key, dictA[key]), + app.status(msg="New environment: %(key)s = %(value)s", + key=key, value=dictA[key], chatty=True) elif dictA[key] != dictB[key]: app.status(msg= \ @@ -185,7 +186,8 @@ def log_dict_diff(app, cur, pre): # pragma: no cover % {"key": key, "valA": dictA[key], "valB": dictB[key]}) for key in dictB.keys(): if key not in dictA: - app.status(msg="Environment removed: %s = %s" % (key, dictB[key])) + app.status(msg="Environment removed: %(key)s = %(value)s", + key=key, value=dictB[key]) # This acquired from rdiff-backup which is GPLv2+ and a patch from 2011 -- cgit v1.2.1