RPC calls not available for fork repository

[gitblit]

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]

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]

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]

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]

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]

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

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

[gitblit]

Merged to develop.

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

[gitblit]

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

• Status changed: Queued

[gitblit]

1.6.0 released.

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

• Status changed: Done