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...