Fix for commits without a parent not returning a diff

Review Request #7054 — Created March 12, 2015 and submitted

jyuen
rb-gateway
master
7053
023801e...
rb-gateway, students

Currently, the route /repos/{repo}/commits/{id} will not return a diff if the commit has no parent. This fixes that.

Made sure that a diff is returned even when the commit has no parent.

Description From Last Updated

What happens in the case where there are multiple parents (i.e., a merge commit)? If we aren't going to support …

brenniebrennie

I don't know if this is against gofmt but can you align the parameters like we do in Python, e.g. …

brenniebrennie

Can this be refactored so that we only have one call of gitRepo.DiffTreeToTree (after the if/else)?

brenniebrennie
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        git_repository.go
    
    
    
    Tool: Pyflakes
    Ignored Files:
        git_repository.go
    
    
  2. 
      
brennie
  1. 
      
  2. git_repository.go (Diff revision 1)
     
     

    What happens in the case where there are multiple parents (i.e., a merge commit)? If we aren't going to support this we should error out somehow (that doesn't break RB).

    1. I believe I saw that you discussed this in Slack, but this should be OK.

  3. git_repository.go (Diff revision 1)
     
     
     

    I don't know if this is against gofmt but can you align the parameters like we do in Python, e.g.

    gitRepo.DiffTreeToTree(parentTree,
                           commitTree,
                           &diffOptions)
    

    If this is against gofmt, please drop this issue.

    1. I prefer the Python way too but it seems this is the best I can do with gofmt, not sure which is preferable:

          gitDiff, err := gitRepo.DiffTreeToTree(
              parentTree,
              commitTree,
              &diffOptions)
      
    2. Oh I should also mention that Go has no line limit, but I've been trying to wrap at 80.

    3. I think if we wrap any arguments, we should wrap all of them (meaning your "best I can do" is preferable).

  4. 
      
Chester
  1. I am interested in the path, is it for github only? or all git repositories in general? Because paths might vary in different git services.

    1. By path do you mean /repos/{repo}/commits/{id}? This is the URL route at http://<service>/repo/{repo}/commits/{id}. rb-gateway provides a service and there is only the API at this URL path to get the commits. I don't communicate with GitHub, nor is this URL path tied closely with git. Based on what kind of repository {repo} is in the rb-gateway configurations, the request will be handled appropriately. Hope that makes sense.

    2. oh ok, maybe it would be nicer if you say this is for rb-gateway in yor summary or description specifically. By the way, the error comes up when I fetch the diff for the first commit from GitLab repository.

    3. The repository on each review request is visible in the repository field; in this case, it is rb-gateway. This information is available on the dashboard and on the all review requests page. Also, including the repository name in the summary/description will end up with it carried over to commits in the repositories, which would be redundant.

  2. 
      
JY
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        git_repository.go
    
    
    
    Tool: Pyflakes
    Ignored Files:
        git_repository.go
    
    
  2. 
      
brennie
  1. One minor refactor and then I say we Ship it!

  2. git_repository.go (Diff revision 2)
     
     
     
     
     
     
     

    Can this be refactored so that we only have one call of gitRepo.DiffTreeToTree (after the if/else)?

  3. 
      
JY
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        git_repository.go
    
    
    
    Tool: Pyflakes
    Ignored Files:
        git_repository.go
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
JY
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (af687e0)
Loading...