PR#4718: Fix the view_commit endpoint when the identifier provided is a git tag - pagure

pagure

#4718 Fix the view_commit endpoint when the identifier provided is a git tag

Merged 4 years ago by pingou. Opened 4 years ago by pingou.

view_commit_tags into master

Fix the view_commit endpoint when the identifier provided is a git tag

Pierre-Yves Chibon • 4 years ago

b88d660

pagure/ui/repo.py

file modified

+12

		`@@ -889,6 +889,18 @@`
		`if isinstance(commit, pygit2.Blob):`
		`flask.abort(404, description="Commit not found")`

		`+ if isinstance(commit, pygit2.Tag):`
		`+ commit = commit.peel(pygit2.Commit)`
		`+ return flask.redirect(`
		`+ flask.url_for(`
		`+ "ui_ns.view_commit",`
		`+ repo=repo.name,`
		`+ username=username,`
		`+ namespace=repo.namespace,`
		`+ commitid=commit.hex,`
		`+ )`
		`+ )`
		`+`
		`if commit.parents:`
		`diff = repo_obj.diff(commit.parents[0], commit)`
		`else:`

tests/test_pagure_flask_ui_repo.py

file modified

+38

		`@@ -2282,6 +2282,44 @@`
		`self.assertIn("<title>Commits - test - Pagure</title>", output_text)`
		`self.assertEqual(output_text.count('<span id="commit-actions">'), 1)`

		`+ def test_view_commit_from_tag(self):`
		`+ """ Test the view_commit endpoint given a tag. """`
		`+`
		`+ tests.create_projects(self.session)`
		`+ tests.create_projects_git(os.path.join(self.path, "repos"), bare=True)`
		`+`
		`+ # Add a README to the git repo - First commit`
		`+ tests.add_readme_git_repo(os.path.join(self.path, "repos", "test.git"))`
		`+ repo = pygit2.Repository(os.path.join(self.path, "repos", "test.git"))`
		`+ first_commit = repo.revparse_single("HEAD")`
		`+ tagger = pygit2.Signature("Alice Doe", "adoe@example.com", 12347, 0)`
		`+ repo.create_tag(`
		`+ "0.0.1",`
		`+ first_commit.oid.hex,`
		`+ pygit2.GIT_OBJ_COMMIT,`
		`+ tagger,`
		`+ "Release 0.0.1",`
		`+ )`
		`+`
		`+ tests.add_readme_git_repo(os.path.join(self.path, "repos", "test.git"))`
		`+ repo = pygit2.Repository(os.path.join(self.path, "repos", "test.git"))`
		`+ project = pagure.lib.query.get_authorized_project(self.session, "test")`
		`+ tags = pagure.lib.git.get_git_tags_objects(project)`
		`+ tag_id = tags[0]["object"].oid`
		`+ commit_id = tags[0]["object"].peel(pygit2.Commit).hex`
		`+`
		`+ output = self.app.get("/test/c/%s" % tag_id)`
		`+ self.assertEqual(output.status_code, 302)`
		`+`
		`+ output = self.app.get("/test/c/%s" % tag_id, follow_redirects=True)`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output_text = output.get_data(as_text=True)`
		`+ self.assertIn(first_commit.oid.hex, output_text)`
		`+ self.assertIn(`
		`+ "<title>Commit - test - %s - Pagure</title>" % commit_id,`
		`+ output_text,`
		`+ )`
		`+`
		`def test_compare_commits(self):`
		`""" Test the compare_commits endpoint. """`

no initial comment

jlanda commented on line 5 of tests/test_pagure_flask_ui_repo.py 4 years ago

I miss some tests that can actually check that the content wé are rendering is from the tag and is not present on {master,default} branches. Could we put the tag on a different object than ref/head and check content?

Edited 4 years ago by jlanda

jlanda commented on line 58 of pagure/ui/repo.py 4 years ago

we have the commit already, why redirect? Rendering directly the tag on the same execution will be faster that redirecting to self and could allow use to render the tag label somewhere on the same time (after redirect we don't know that the user used a tagname as argument)

pingou commented on line 58 of pagure/ui/repo.py 4 years ago

At first I thought about doing as you say but then I figured that it was kinda wrong since the hash provided is not a commit, so rather than showing as a commit something that isn't one, I figured it would be more semantically correct to redirect the users to the proper page.

pingou commented on line 5 of tests/test_pagure_flask_ui_repo.py 4 years ago

The tag is on the first commit in the repo, which contains two commits. So at the time the tag is rendered, it's no longer HEAD.
Does that make sense?

9 new commits added

Fix the view_commit endpoint when the identifier provided is a git tag
Only consider the most recently active branches for new PR
Rework the UI for the file history page
Add unit-tests for the view file's history feature
Add a link to the file's history in the file's blame page
Add a link to the file's history in the file's view page
Add a new endpoint and page to see a file's history
Add a method to run git log using the system's git
Enable running a command in a specific folder and return the output

4 years ago

9 new commits added

Fix the view_commit endpoint when the identifier provided is a git tag
Only consider the most recently active branches for new PR
Rework the UI for the file history page
Add unit-tests for the view file's history feature
Add a link to the file's history in the file's blame page
Add a link to the file's history in the file's view page
Add a new endpoint and page to see a file's history
Add a method to run git log using the system's git
Enable running a command in a specific folder and return the output

4 years ago

jlanda commented 4 years ago

At first I thought about doing as you say but then I figured that it was kinda wrong since the hash provided is not a commit, so rather than showing as a commit something that isn't one, I figured it would be more semantically correct to redirect the users to the proper page.

I did not realized that. I like the reasoning

jlanda commented 4 years ago

The tag is on the first commit in the repo, which contains two commits. So at the time the tag is rendered, it's no longer HEAD.
Does that make sense?

:thumbsup: