PR#941: Fix pagure.lib.git.get_revs_between - pagure

pagure

#941 Fix pagure.lib.git.get_revs_between

Merged 7 years ago by pingou. Opened 7 years ago by cverna.

cverna/pagure fix_get_revs into master

Add test case for get revs between 2 branches

Clement Verna • 7 years ago

469e2c3

Added test case for repo BASE is null

Clement Verna • 7 years ago

1901d54

Switched newrev and oldrev in logic in order to fix

Clement Verna • 7 years ago

439a2dc

rename get_revs_between arguments

Clement Verna • 7 years ago

2a58ed3

pagure/lib/git.py

file modified

+6 -6

		`@@ -840,18 +840,18 @@`
		`).splitlines(keepends)`


		`- def get_revs_between(torev, fromrev, abspath, forced=False):`
		`+ def get_revs_between(oldrev, newrev, abspath, forced=False):`
		`""" Yield revisions between HEAD and BASE. """`

		`- cmd = ['rev-list', '%s...%s' % (torev, fromrev)]`
		`+ cmd = ['rev-list', '%s...%s' % (oldrev, newrev)]`
		`if forced:`
		`head = get_default_branch(abspath)`
		`cmd.append('^%s' % head)`
		`- if set(fromrev) == set('0'):`
		`- cmd = ['rev-list', '%s' % torev]`
		`- elif set(torev) == set('0') or set(torev) == set('^0'):`
		`+ if set(oldrev) == set('0'):`
		`+ cmd = ['rev-list', '%s' % newrev]`
		`+ elif set(newrev) == set('0') or set(newrev) == set('^0'):`
		`head = get_default_branch(abspath)`
		`- cmd = ['rev-list', '%s' % fromrev, '^%s' % head]`
		`+ cmd = ['rev-list', '%s' % oldrev, '^%s' % head]`
		`return pagure.lib.git.read_git_lines(cmd, abspath)`

tests/test_pagure_lib_git.py

file modified

+52 -4

		`@@ -16,9 +16,10 @@`
		`import shutil`
		`import sys`
		`import os`
		`-`
		`+ import tempfile`
		`import pygit2`
		`from mock import patch`
		`+ from pagure.lib.repo import PagureRepo`

		`sys.path.insert(0, os.path.join(os.path.dirname(`
		`os.path.abspath(__file__)), '..'))`
		`@@ -1253,17 +1254,64 @@`
		`output = pagure.lib.git.read_git_lines(`
		`['log', '-3', "--pretty='%H'"], gitrepo)`
		`self.assertEqual(len(output), 2)`
		`- to_hash = output[0].replace("'", '')`
		`from_hash = output[1].replace("'", '')`

		`+ # Case 1, repo BASE is null and HEAD is equal to from_hash`
		`+ to_hash = '0'`
		`output1 = pagure.lib.git.get_revs_between(`
		`to_hash, from_hash, gitrepo)`
		`- self.assertEqual(output1, [to_hash])`
		`+ self.assertEqual(output1, [from_hash])`

		`+ # Case 2, get revs between two commits (to_hash, from_hash)`
		`+ to_hash = output[0].replace("'", '')`
		`output2 = pagure.lib.git.get_revs_between(`
		`- from_hash, to_hash, gitrepo)`
		`+ to_hash, from_hash, gitrepo)`
		`self.assertEqual(output2, [to_hash])`

		`+ # Case 3, get revs between two commits (from_hash, to_hash)`
		`+ output3 = pagure.lib.git.get_revs_between(`
		`+ from_hash, to_hash, gitrepo)`
		`+ self.assertEqual(output3, [to_hash])`
		`+`
		`+ # Case 4, get revs between two commits on two different branches`
		`+ newgitrepo = tempfile.mkdtemp(prefix='pagure-')`
		`+ newrepo = pygit2.clone_repository(gitrepo, newgitrepo)`
		`+ newrepo.create_branch('feature', newrepo.head.get_object())`
		`+`
		`+ with open(os.path.join(newgitrepo, 'sources'), 'w') as stream:`
		`+ stream.write('foo\n bar')`
		`+ newrepo.index.add('sources')`
		`+ newrepo.index.write()`
		`+`
		`+ # Commits the files added`
		`+ tree = newrepo.index.write_tree()`
		`+ author = pygit2.Signature(`
		`+ 'Alice Author', 'alice@authors.tld')`
		`+ committer = pygit2.Signature(`
		`+ 'Cecil Committer', 'cecil@committers.tld')`
		`+ newrepo.create_commit(`
		`+ 'refs/heads/feature', # the name of the reference to update`
		`+ author,`
		`+ committer,`
		`+ 'Add sources file for testing',`
		`+ # binary string representing the tree object ID`
		`+ tree,`
		`+ # list of binary strings representing parents of the new commit`
		`+ [to_hash]`
		`+ )`
		`+ branch_commit = newrepo.revparse_single('refs/heads/feature')`
		`+`
		`+ # Push to origin`
		`+ ori_remote = newrepo.remotes[0]`
		`+ PagureRepo.push(ori_remote, 'refs/heads/feature')`
		`+`
		`+ # Remove the clone`
		`+ shutil.rmtree(newgitrepo)`
		`+`
		`+ output4 = pagure.lib.git.get_revs_between(`
		`+ 'feature', '0', gitrepo)`
		`+ self.assertEqual(output4, [branch_commit.oid.hex])`
		`+`
		`def test_get_author(self):`
		`""" Test the get_author method of pagure.lib.git. """`

cverna commented 7 years ago

get_revs_between method returns an empty list, when you want to get the revision between a commit and the base of the repo.

pingou commented on line 22 of tests/test_pagure_lib_git.py 7 years ago

Should we document # Case 3?

3 new commits added

Added test case for repo BASE is null
Switched newrev and oldrev in logic in order to fix
rename get_revs_between arguments

7 years ago

cverna commented 7 years ago

@pingou
I had a look at adding test of get_revs_between for forced push and new branches, but it seems to be quite an effort. (at least for me :sweat_smile: )

I wonder if we should merge this PR and open a ticket to enhance the test later, or maybe it does not worth the effort to test these 2 cases.

The current test suite doesn't not cover the forced push and new branches cases.

pingou commented 7 years ago

The new branch case shouldn't be too hard no?