Fish Trophy

david got a fish trophy!

Add support for getting diffs from GitLab APIv4.

Review Request #12221 — Created March 31, 2022 and submitted

david
Review Board
release-3.0.x
f00f1f6...
reviewboard

GitLab has been slowly decommissioning old API endpoints, especially on
the SaaS gitlab.org. They recently got rid of the old diff endpoint,
which we were still using because the v4 API doesn't have a great way to
get the diffs. Unfortunately, this means we need to actually implement
support for v4.

This works somewhat similarly to how the GitHub diff API works, where
instead of just getting a full copy of the diff, we get a JSON blob that
includes diff fragments for each file. This is somewhat complicated by
the fact that said diff fragments do not contain the blob SHAs for the
old and new revisions. We have to go get that information from the file
API, which is a bit chatty but at least we can just use a HEAD request
to get only the metadata instead of having to fetch the file content.

  • Added a bunch of GitLab repositories and created post-commit review
    requests for commits that included modified, added, and deleted files.
    Saw things work every time.
  • Ran unit tests.
Description From Last Updated

Actually, one big change I'd like to have here. In the event that GitLab maybe fixes their diff, we would …

chipx86chipx86

Can you add unit tests for some combinations of the headers we're looking for? Make sure we don't break if …

chipx86chipx86

Can you verify if this works on both Python 2 and 3?

chipx86chipx86

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

Small nit: "URL"

chipx86chipx86

Rather than searching each of these per file, maybe combine into one regex, so we only need to search once?

chipx86chipx86

I don't think - needs to be escaped.

chipx86chipx86

Too many blank lines.

chipx86chipx86

E501 line too long (82 > 79 characters)

reviewbotreviewbot

The index line may also contain a UNIX file mode at the end, so we may want to just avoid …

chipx86chipx86

F841 local variable 'diff_rsp' is assigned to but never used

reviewbotreviewbot

F841 local variable 'commit' is assigned to but never used

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. Can you verify if this works on both Python 2 and 3?

    1. So far this is only implemented/tested on 3.0.x, so Python 2 only. I'll do a test port to 4.0.x once other issues are resolved.

  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     

    Small nit: "URL"

  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
     
     
     

    Too many blank lines.

  5. 
      
chipx86
  1. 
      
  2. Actually, one big change I'd like to have here.

    In the event that GitLab maybe fixes their diff, we would end up re-corrupting it. So I think we should have logic like:

    if not diff.startswith('git --diff'):
        # Add the git --diff
    
        if not diff.startswith('---'):
            # Fetch and build the file headers...
    

    That way, if they end up adding the ---/+++ lines, we should be able to skip the API calls. And if they add the git --diff, we won't add a redundant one.

  3. 
      
david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. sddsfhjk sdfhjksdfh jksfhk jsdhf kasjldhf jklasdhfkjsdahf jksadhfkj asdfjh sadklfas
    

    def abc
    
  3. Can you add unit tests for some combinations of the headers we're looking for? Make sure we don't break if they add some or all of those?

  4. reviewboard/hostingsvcs/gitlab.py (Diff revisions 2 - 4)
     
     
     
     

    Rather than searching each of these per file, maybe combine into one regex, so we only need to search once?

  5. reviewboard/hostingsvcs/gitlab.py (Diff revisions 2 - 4)
     
     

    I don't think - needs to be escaped.

    1. Changed to just look for the +++ line since if we have one we can expect both.

  6. 
      
david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/gitlab.py (Diff revisions 4 - 6)
     
     

    The index line may also contain a UNIX file mode at the end, so we may want to just avoid the $.

    For example:

    index 3f786850e387550fdab836ed7e6dc881de23001b..89e6c98d92887913cadf06b2adb97f26cde4849b 100644
    
  3. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (3298f5f)
Loading...