Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RPC calls not available for fork repository #667

Closed
gitblit opened this issue Aug 12, 2015 · 8 comments
Closed

RPC calls not available for fork repository #667

gitblit opened this issue Aug 12, 2015 · 8 comments

Comments

@gitblit
Copy link
Owner

@gitblit gitblit commented Aug 12, 2015

Originally reported on Google Code with ID 371

Currently this is available only as a UI option.
As discussed in the google gitblit group discussion [1], I have implemented this. 

The patch is attached below. 
Please review this and commit to the main branch. 


[1]. https://groups.google.com/forum/#!topic/gitblit/PjMwz5CyY_I

Thanks
Manisha

Reported by manisha@wso2.com on 2014-02-18 11:33:24


- _Attachment: [gitblit.patch](https://storage.googleapis.com/google-code-attachments/gitblit/issue-371/comment-0/gitblit.patch)_
@gitblit gitblit self-assigned this Aug 12, 2015
@gitblit
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

Hi Manisha,

Thanks for the patch.  Unfortunately it has several issues, the most important one
being this patch does not actually contain a server-side RPC implementation for forking
a repository.  Perhaps your IDE didn't include all your modified files when the patch
was generated.

My other issues with the current patch are:

1. Unnecessary "GenericGitblitException" class, use GitBlitException instead
2. Unnecessary changes to JSONUtils - perhaps automatically introduced by your IDE
3. Redundant client-side logic in RPCUtils.forkRepository().  The server needs to verify
all inputs and authorize forking so this logic must be server-side.  I think it is
fair to assume callers of RPCUtils.forkRepository() wouldn't allow forking repositories
that do not exist and if they do, the server will reject those requests.
4. There is a subtle permission requirement noted in the RPC request enum about permission
level and order of request constants.  In your patch, only an administrator can fork
a repository using the RPC.
5. There is no unit test to confirm that the missing server-side fork RPC implementation
actually works.
6. I am undecided on the utility of the "composite" class (which does not implement
Serializable, btw).  I would have to see the server-side code to decide if I thought
there was a simpler way to achieve the same thing.

It would be helpful for me on the next revision of your patch if you could create a
pull request instead of attaching a patch.  It makes first-round reviewing easier.
 If you can't do that, then we'll make do with patch reviews.

Thanks,
-J

Reported by James.Moger on 2014-02-18 16:53:31

@gitblit
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

Hi James, 

Thanks for the feedback. 

1. The reason I introduced a GenericGitblitException is, GitBlitException was handling
only IOExceptions and wanted a place to handle other generic exceptions as well. 

2. I had to add the changes in the JSONUtils because, the code was not compiling for
me, because of the generics errors as below:
no unique maximal instance exists for type variable T with upper bounds X,java.lang.Object
Therefore, I added the casting as explained in 
http://stackoverflow.com/questions/1609531/same-source-code-eclipse-build-success-but-maven-javac-fails

3. Had to introduce the client side validation since I was getting NPE when I passed
non existing user/repo

4. I was in the mindset of only the admin is given fork permissions. Will fix this
as well .

5. Sure. Will add a unit test to the scenario. 

6. I have missed implementing the Serializable interface, but worked as expected. However,
will fix this as well. 
I tried using a TypeToken with a map of UserModel and RepositoryModel. But it was giving
some json parsing errors. So to keep things simple, I introduced the Composite class.


Also please share a git location where I can clone the code. I tried cloning the code
 from https://dev.gitblit.com/r/gitblit.git but it does not contain the Ticket related
 code. Therefore I downloaded the source pack from the commitID https://dev.gitblit.com/tree/gitblit.git/9f8d019e9379959929bfbab8789174e1f82f91c9.
If you could please show me the proper location to fork, I can create a PR instead
of creating a patch like this, which would make both of our lives easy. :)

Have a good day. 
Thanks
Manisha

Reported by manisha@wso2.com on 2014-02-19 07:53:59

@gitblit
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

2.  Interesting.  I compile with 6 & 7 on Win, Linux, & Mac and I've never come across
that.  What Eclipse, Java, & OS are you running?

3. NPE.  Hmmm.  I'd have to see the server-side code.

4. Fork permissions can be given to any user and repositories can prohibit forking,
if they want.

6. Serializable with id of 1 is my preference, but GSON will serialize most anything
you throw at it without being explicitly Serializable.

I did share the tickets code two weeks ago; it's in ticket 1.  I have not merged it
yet so please do not create patches/PRs against it.  Forking on GitHub is probably
the easiest right now.

Reported by James.Moger on 2014-02-19 13:04:07

@gitblit
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

Please I also want this method in RpcUtil. Can you implement this and share with us
? Highly appreciate.

Thanks

Reported by harsha.thirimanna on 2014-03-26 13:35:02

@gitblit
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

Reported by James.Moger on 2014-05-08 15:09:59

  • Status changed: Started
  • Labels added: Milestone-1.6.0

@gitblit
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

Merged to develop.

Reported by James.Moger on 2014-05-08 15:21:09

@gitblit
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

Reported by James.Moger on 2014-05-15 14:37:15

  • Status changed: Queued

@gitblit
Copy link
Owner Author

@gitblit gitblit commented Aug 12, 2015

1.6.0 released.

Reported by James.Moger on 2014-06-17 00:24:34

  • Status changed: Done

@gitblit gitblit closed this as completed Aug 12, 2015
@flaix flaix added this to the 1.6.0 milestone Dec 13, 2016
@flaix flaix added this to the 1.6.0 milestone Dec 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants