remote: add typedef to normalize push_update_reference callback#4363
Conversation
pks-t
left a comment
There was a problem hiding this comment.
Hi @cjhoward92, thank you for your PR.
I guess the change itself is okay. There are some minor things that can be improved, though. It would be cool if you could address those issues. Furthermore, I'd appreciate it if you squash together all commits here, as they are logically a single unit of change, only. Additional kudos if you could prefix the commit message's subject with a scope, e.g. "remote: add typedef for git_push_update_reference_cb". Note also the tense, where we (adopted from core git) prefer to describe what the commit does instead of saying what we have done (so no past tense here).
If you address those things I'm happy to merge 👍
|
|
||
| /** | ||
| * Called for each updated reference on push. If `status` is | ||
| * not `NULL`, the update was rejected by the remote server |
There was a problem hiding this comment.
The style for function documentation is usually as follows:
/**
* Short summary of that thing
*
* A longer description which may span multiple lines
* and describes what that symbol actually is/does.
*
* @param frobnicate frobnicate all the stuff
* @return 0 on foo, otherwise an error
*/
So please separate the summary and descriptions here and add an @return description, such that developers know what they may return from that callback.
| */ | ||
| typedef int (*git_push_update_reference_cb)(const char *refname, const char *status, void *data); | ||
|
|
||
|
|
| @@ -456,7 +468,7 @@ struct git_remote_callbacks { | |||
| * not `NULL`, the update was rejected by the remote server | |||
| * and `status` contains the reason given. | |||
| */ | |||
There was a problem hiding this comment.
This comment is duplicated now. You may replace it with something like "See documentation of git_push_update_reference_cb".
Very many callbacks in libgit2 have some sort of typedef to normalize the name at git_<name_of_operation>_cb. Add a typedef for push_update_references in the remote so the name follows the same conventions.
4488036 to
7138ce3
Compare
|
@pks-t Thanks for the feedback! I would very much like to get this merged. Hopefully the changes I made suit the needs of libgit2 better. Let me know if there is anything else I can do for you! |
|
Thanks for fixing this up @cjhoward92. These changes look 👍 to me - appreciate you making them. Since this is a blocker for nodegit, I'm going to go ahead and merge this. If there are additional changes we can iterate from here. |
|
Thanks Ed! Right now we should be set. If you apply need someone to go through and add more callback typedefs or clean anything up I'd be more than happy to. One of those simple maintenance things that a new contributor like me can do! |
I added a typedef for
push_update_referenceinremote.hto make the callback fit with other well defined callbacks. If this is inappropriate or there is some reason for not having a typedef, let me know.