Typedef git_pkt_type and clarify recv_pkt return type#4514
Conversation
pks-t
left a comment
pks-t
left a comment
There was a problem hiding this comment.
This is definitly much more readable, so I'm very happy with your change overall. Thanks! Just some comments to make it even more readable, even though your changes already go most of the way
| git__free(pkt); | ||
|
|
||
| return pkt_type; | ||
| return error; |
There was a problem hiding this comment.
👍 This is our preferred style and definitly easier to handle than the previous interface
| } | ||
|
|
||
| static int recv_pkt(git_pkt **out, gitno_buffer *buf) | ||
| static int recv_pkt(git_pkt **out, git_pkt_type *pkt_type, gitno_buffer *buf) |
There was a problem hiding this comment.
Maybe name it out_type instead? Just to make the parameter's intention a bit more clear
| if (pkt_type == GIT_PKT_ACK) { | ||
| error = recv_pkt(NULL, &pkt_type, buf); | ||
| if (error < 0) { | ||
| goto on_error; |
There was a problem hiding this comment.
I'd propose to make it if ((error = recv_pkt(NULL, &pkt_type, buf)) < 0) goto on_error;. This would further disentangle handling of errors and the packet type.
| if (pkt_type < 0) { | ||
| return pkt_type; | ||
| if (error < 0) { | ||
| return error; |
tiennou
force-pushed
the
fix/pkt-type-enum
branch
from
35c2ce8 to
eb2d9b9
Compare
|
Rebased against master, and fixed review comments. I stumbled on a (small) memory leak when checking uses of |
| goto on_error; | ||
| } else if (pkt_type == GIT_PKT_ACK) { | ||
|
|
||
| if (pkt_type == GIT_PKT_ACK) { |
There was a problem hiding this comment.
You're using spaces instead of tabs here
| return error; | ||
| } else if (pkt_type != GIT_PKT_ACK && pkt_type != GIT_PKT_NAK) { | ||
|
|
||
| if (pkt_type != GIT_PKT_ACK && pkt_type != GIT_PKT_NAK) { |
There was a problem hiding this comment.
And here as well, spaces instead of tab
| if (git_vector_insert(&t->common, pkt) < 0) { | ||
| git__free(pkt); | ||
| return -1; | ||
| } |
tiennou
force-pushed
the
fix/pkt-type-enum
branch
from
eb2d9b9 to
a2124a2
Compare
tiennou
force-pushed
the
fix/pkt-type-enum
branch
from
a2124a2 to
13a7727
Compare
|
Fixup-ed spaces into tabs ! |
|
I like this cleanup, thanks for doing it (and thanks for your patience in getting it merged). |
4 participants
This is extracted from my WIP shallow/smart-transport branch (a.k.a. shallow clones).
I was very confused by the "error handling" around that function, so hopefully this makes it clearer.