Fish Trophy

david got a fish trophy!

Fish Trophy

Add support for getting diffs from GitLab APIv4.

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

Information

Review Board
release-3.0.x
f00f1f6...

Reviewers

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. Show all issues

    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)
     
     
    Show all issues

    Small nit: "URL"

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

    Too many blank lines.

  5. 
      
chipx86
  1. 
      
  2. Show all issues

    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
Commit:
a8a0ef55e90cc0b35132fd99dc371af5b832a19c
06befa01b3901172a63df5fdafe90b2015d0a3fd

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. Show all issues

    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)
     
     
     
     
    Show all issues

    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)
     
     
    Show all issues

    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
Commit:
da54956364bbf5d9dd3f4cef5a57f621224935a6
e762ddb00075b8bed5c73ff4106e9a593c0b90b9

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    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:
Completed
Change Summary:
Pushed to release-3.0.x (3298f5f)