Summary

Fix for commits without a parent not returning a diff

Review Request #7054 — Created March 12, 2015 and submitted March 23, 2015, 2:25 p.m.

Information

Owner

jyuen

Repository

rb-gateway

Branch

master

Bugs

Depends On

Blocks

7053

Commit

023801e...

Reviewers

Groups

rb-gateway, students

People

Description

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

Testing Done

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

Issues

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 …	brennie	March 19, 2015, 4:33 p.m.
I don't know if this is against gofmt but can you align the parameters like we do in Python, e.g. …	brennie	March 19, 2015, 4:33 p.m.
Can this be refactored so that we only have one call of gitRepo.DiffTreeToTree (after the if/else)?	brennie	March 19, 2015, 6:45 p.m.

Tool: PEP8 Style Checker
Ignored Files:
    git_repository.go



Tool: Pyflakes
Ignored Files:
    git_repository.go

git_repository.go (Diff revision 1)

The issue has been dropped. Show all issues

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

david March 18, 2015, 2:22 p.m.

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

git_repository.go (Diff revision 1)

The issue has been resolved. Show all issues

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.

jyuen March 12, 2015, 7:51 p.m.

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)

jyuen March 12, 2015, 7:54 p.m.

Oh I should also mention that Go has no line limit, but I've been trying to wrap at 80.

david March 18, 2015, 2:22 p.m.

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

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.

jyuen March 13, 2015, 7:18 p.m.

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.

Chester March 13, 2015, 7:23 p.m.

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.

brennie March 17, 2015, 11:30 p.m.

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.

Change Summary:

Minor format/style changes

Commit:

3ebc652a2efac878e992254562fd8d88b00fa482

9e744ef0de54ea047920f07364ce32ac724e8de9

Diff:

Revision 2 (+35 -28)

Show changes

git_repository.go

Tool: PEP8 Style Checker
Ignored Files:
    git_repository.go



Tool: Pyflakes
Ignored Files:
    git_repository.go

Ship it!

One minor refactor and then I say we Ship it!

git_repository.go (Diff revision 2)

The issue has been resolved. Show all issues

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