pagure

Clone

#4306 Fix displaying diffs that contain symlinks. Fixes #4006
Merged 5 years ago by pingou. Opened 5 years ago by bkabrda.
bkabrda/pagure fix-symlink-diffs  into  master

file modified
+11 -9 
@@ -2664,20 +2664,22 @@ 
 

          output["old_id"] = str(patch.old_id)
 

      elif hasattr(patch, "delta"):
 

          # Newer pygit2
 

-         if patch.delta.new_file.mode == 0 and patch.delta.old_file.mode in [
 

-             33188,
 

-             33261,
 

-         ]:
 

+         # we recognize non-executable file, executable file and symlink
 

+         expected_modes = [33188, 33261, 40960]
 

+         if (
 

+             patch.delta.new_file.mode == 0
 

+             and patch.delta.old_file.mode in expected_modes
 

+         ):
 

              status = "D"
 

          elif (
 

-             patch.delta.new_file.mode in [33188, 33261]
 

+             patch.delta.new_file.mode in expected_modes
 

              and patch.delta.old_file.mode == 0
 

          ):
 

              status = "A"
 

-         elif patch.delta.new_file.mode in [
 

-             33188,
 

-             33261,
 

-         ] and patch.delta.old_file.mode in [33188, 33261]:
 

+         elif (
 

+             patch.delta.new_file.mode in expected_modes
 

+             and patch.delta.old_file.mode in expected_modes
 

+         ):
 

              status = "M"
 

          if patch.delta.new_file.path != patch.delta.old_file.path:
 

              status = "R"

file modified
+10 -8 
@@ -16,6 +16,8 @@ 
 

  {% endblock %}
 

  {% block repo %}
 

  {% set splitted_message = commit.message.split('\n') %}
 

+ {# we recognize non-executable file, executable file and symlink #}
 

+ {% set expected_modes = [33188, 33261, 40960] %}
 

  <div class="row">
 

    <div class="col">
 

        {% block overviewtabs %}{{ super() }}{% endblock %}
 
@@ -189,13 +191,13 @@ 
 

                        {%- elif patch | hasattr('delta') -%}
 

                          {%- if patch.delta.new_file.path == patch.delta.old_file.path -%}
 

                            {%- if patch.delta.new_file.mode == 0
 

-                               and patch.delta.old_file.mode in [33188, 33261] -%}
 

+                               and patch.delta.old_file.mode in expected_modes -%}
 

                              {{ changesdeletedfile(patch.delta.new_file.path, linesadded, linesremoved, loop.index) }}
 

-                           {%- elif patch.delta.new_file.mode in [33188, 33261]
 

+                           {%- elif patch.delta.new_file.mode in expected_modes
 

                                 and patch.delta.old_file.mode == 0 -%}
 

                              {{ changesaddedfile( patch.delta.new_file.path, linesadded, linesremoved, loop.index) }}
 

-                           {%- elif patch.delta.new_file.mode in [33188, 33261]
 

-                                and patch.delta.old_file.mode in [33188, 33261] -%}
 

+                           {%- elif patch.delta.new_file.mode in expected_modes
 

+                                and patch.delta.old_file.mode in expected_modes -%}
 

                              {{ changeschangedfile(patch.delta.new_file.path, linesadded, linesremoved, loop.index) }}
 

                            {%-endif-%}
 

                          {%- else -%}
 
@@ -397,7 +399,7 @@ 
 

                {%- elif patch | hasattr('delta') -%}
 

                  {%- if patch.delta.new_file.path == patch.delta.old_file.path -%}
 

                    {%- if patch.delta.new_file.mode == 0
 

-                       and patch.delta.old_file.mode in [33188, 33261] -%}
 

+                       and patch.delta.old_file.mode in expected_modes -%}
 

                      {% set patchtype = "removed"%}
 

                      <div>
 

                          {{ viewfilelink(patch.delta.new_file.path) }}
 
@@ -408,7 +410,7 @@ 
 

                        {{ viewfilelinkbutton(patch.delta.new_file.path, disabled=True) }}
 

                        {{ diffcollapsebtn() }}
 

                      </div>
 

-                   {%-elif patch.delta.new_file.mode in [33188, 33261]
 

+                   {%-elif patch.delta.new_file.mode in expected_modes
 

                        and patch.delta.old_file.mode == 0 -%}
 

                      {% set patchtype = "added"%}
 

                        <div>
 
@@ -422,8 +424,8 @@ 
 

                            {{ diffcollapsebtn() }}
 

                          {% endif %}
 

                        </div>
 

-                   {%-elif patch.delta.new_file.mode in [33188, 33261]
 

-                       and patch.delta.old_file.mode in [33188, 33261] -%}
 

+                   {%-elif patch.delta.new_file.mode in expected_modes
 

+                       and patch.delta.old_file.mode in expected_modes -%}
 

                      {% set patchtype = "changed"%}
 

                      <div>
 

                          {{ viewfilelink(patch.delta.new_file.path) }}

file modified
+9 -3 
@@ -878,15 +878,21 @@ 
 

  

  

  def add_commit_git_repo(folder, ncommits=10, filename='sources',
 

-                         branch='master'):
 

+                         branch='master', symlink_to=None):
 

      """ Create some more commits for the specified git repo. """
 

      repo, newfolder, branch_ref_obj = _clone_and_top_commits(
 

          folder, branch, branch_ref=True)
 

  

      for index in range(ncommits):
 

          # Create a file in that git repo
 

-         with open(os.path.join(newfolder, filename), 'a') as stream:
 

-             stream.write('Row %s\n' % index)
 

+         if symlink_to:
 

+             os.symlink(
 

+                 symlink_to,
 

+                 os.path.join(newfolder, filename),
 

+             )
 

+         else:
 

+             with open(os.path.join(newfolder, filename), 'a') as stream:
 

+                 stream.write('Row %s\n' % index)
 

          repo.index.add(filename)
 

          repo.index.write()

file modified
+38 
@@ -2163,6 +2163,20 @@ 
 

                  '<div class="btn btn-outline-danger disabled opacity-100 border-0 font-weight-bold">\n'
 

                  '    file removed\n', output_text)
 

  

+         def compare_with_symlink(c3, c4):
 

+             # View comparison of commits with symlink
 

+             # we only test that the patch itself renders correctly,
 

+             # the rest of the logic is already tested in the other functions
 

+             output = self.app.get('/test/c/%s..%s' % (c3.oid.hex, c4.oid.hex))
 

+             self.assertEqual(output.status_code, 200)
 

+             output_text = output.get_data(as_text=True)
 

+             print(output_text)
 

+             self.assertIn(
 

+                 '<title>Diff from %s to %s - test\n - Pagure</title>'
 

+                 % (c3.oid.hex, c4.oid.hex),
 

+                 output_text)
 

+             self.assertIn('<pre class="alert-success"><code>+ Source </code></pre>', output_text)
 

+ 

          output = self.app.get('/foo/bar')
 

          # No project registered in the DB
 

          self.assertEqual(output.status_code, 404)
 
@@ -2196,14 +2210,22 @@ 
 

              ncommits=1, filename='Šource')
 

          c3 = repo.revparse_single('HEAD')
 

  

+         tests.add_commit_git_repo(
 

+             os.path.join(self.path, 'repos', 'test.git'),
 

+             ncommits=1, filename='Source-sl', symlink_to='Source'
 

+         )
 

+         c4 = repo.revparse_single('HEAD')
 

+ 

          compare_first_two(c1, c2)
 

          compare_all(c1, c3)
 

+         compare_with_symlink(c3, c4)
 

  

          user = tests.FakeUser()
 

          # Set user logged in
 

          with tests.user_set(self.app.application, user):
 

              compare_first_two(c1, c2)
 

              compare_all(c1, c3)
 

+             compare_with_symlink(c3, c4)
 

  

      def test_view_file(self):
 

          """ Test the view_file endpoint. """
 
@@ -2780,6 +2802,12 @@ 
 

  

          repo = pygit2.Repository(os.path.join(self.path, 'repos', 'test.git'))
 

          commit = repo.revparse_single('HEAD')
 

+         # Add a symlink to test that it displays correctly
 

+         tests.add_commit_git_repo(
 

+             os.path.join(self.path, 'repos', 'test.git'),
 

+             ncommits=1, filename='sources-sl', symlink_to='sources'
 

+         )
 

+         commit_sl = repo.revparse_single('HEAD')
 

  

          # View another commit
 

          output = self.app.get('/test/c/%s' % commit.oid.hex)
 
@@ -2791,6 +2819,16 @@ 
 

          self.assertIn('Authored by Alice Author', output_text)
 

          self.assertIn('Committed by Cecil Committer', output_text)
 

  

+         # Make sure that diff containing symlink displays the header correctly
 

+         output = self.app.get('/test/c/%s' % commit_sl.oid.hex)
 

+         self.assertEqual(output.status_code, 200)
 

+         output_text = output.get_data(as_text=True)
 

+         # check the link to the file
 

+         self.assertIn('>sources-sl</a>', output_text)
 

+         # check that the header contains "file added" and a +1 for one added line
 

+         self.assertIn('>file added</span>', output_text)
 

+         self.assertIn('>+1</span>', output_text)
 

+ 

          #View the commit when branch name is provided
 

          output = self.app.get('/test/c/%s?branch=master' % commit.oid.hex)
 

          self.assertEqual(output.status_code, 200)

Basically, the issue is that current code doesn't expect 40960 (0o120000), which is the code for symlinks. This PR fixes all ocurences of this problem - displaying headers for diffs with symlink on commit details page and also any page containing diffs (comparing two commits, displaying pull request).

Fixes #4006

Code looks neat, thanks for fixing this.

Let's wait for jenkins and get this in :)

 
11:46:00  FAILED test: py3-test_pagure_flask_ui_repo

rebased onto dade63b
5 years ago

:thumbsup:

Pull-Request has been merged by pingou
5 years ago
Changes Summary 4
+11 -9
file changed
pagure/lib/git.py
+10 -8
file changed
pagure/templates/commit.html
+9 -3
file changed
tests/__init__.py
+38 -0
file changed
tests/test_pagure_flask_ui_repo.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.