Comments
Patch
@@ -20,40 +20,39 @@
uint32_t length;
char ch_char;
- if (!handle) {
+ if (!handle || handle->bytes_on_pipe != 0) {
errno = EINVAL;
return NULL;
}
+ if (read(handle->p_read, &ch_char, 1) < 0) {
+ return NULL;
+ }
+ if (read(handle->p_read, &length, sizeof(uint32_t)) < 0) {
+ return NULL;
+ }
- if (handle->bytes_on_pipe == 0) {
- if (read(handle->p_read, &ch_char, 1) < 0) {
- return NULL;
- }
- if (read(handle->p_read, &length, sizeof(uint32_t)) < 0) {
- return NULL;
- }
- handle->header->length = swap_uint32(length);
- handle->bytes_on_pipe = handle->header->length;
- switch(ch_char) {
- case 'o':
- handle->header->channel = o;
- break;
- case 'e':
- handle->header->channel = e;
- break;
- case 'r':
- handle->header->channel = r;
- break;
- case 'I':
- handle->header->channel = I;
- break;
- case 'L':
- handle->header->channel = L;
- break;
- default:
- handle->header->channel = wrong_channel;
- break;
- }
+ handle->header->length = swap_uint32(length);
+ handle->bytes_on_pipe = handle->header->length;
+
+ switch(ch_char) {
+ case 'o':
+ handle->header->channel = o;
+ break;
+ case 'e':
+ handle->header->channel = e;
+ break;
+ case 'r':
+ handle->header->channel = r;
+ break;
+ case 'I':
+ handle->header->channel = I;
+ break;
+ case 'L':
+ handle->header->channel = L;
+ break;
+ default:
+ handle->header->channel = wrong_channel;
+ break;
}
return handle->header;
}
# HG changeset patch # User Giovanni Gherdovich <g.gherdovich@gmail.com> # Date 1416651277 -3600 # Sat Nov 22 11:14:37 2014 +0100 # Branch refactor examples # Node ID 0dc61b8b4a873ba6174f3c56937d4ba4c960088c # Parent d36ee5026669f1176dcba92927128f8aa8b952cd hg_read_header: return NULL if header not present The current behaviour of hg_read_header() is to return the last header seen if there is no header to read in the pipe. This patch makes hg_read_header() do useful work only in case an header is actually the next content on the pipe, and returns NULL otherwise. Forthcoming patches rework all callers (only the examples, at the moment). The code in the examples/ directory makes heavy use of this unfortunate feature, i.e. examples are written following this pattern: -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 char *command = {"export", "-r", "0", NULL}; char buf[4096]; hg_handle *handle; hg_header *header; int n; handle = hg_open("some/repo"); hg_rawcommand(handle, command); while (header = hg_read_header(handle), header->channel == o) { if (n = hg_rawread(handle, buf, 4096), n > 0) { /* do something with buf */ } } -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 What happens with the snippet above is that if there are more than 4KB of data to be read on the pipe, hg_read_header() gets called with no header to read, and it returns the last header seen. This is a bad idea, because it confuses the programmer on what is the actual purpose of the hg_read_header() function. You call hg_read_header() because: * you know by byte counting that the last message just finished, hence a new header is coming next, and you want to read it and not to: * get the last header you saw passing * or possibly, if a new header is available on the pipe, read it. Note that the length of a command server chunk is unbound (but communicated in the header of the chunk itself). Therefore whatever buffer length is chosen for hg_rawread() (4KB in the snippet above) there is no guarantee that a single read will consume the complete chunk. In the pattern above the call to hg_read_header() could very well hit the case "data chunk not finished yet, not yet time to read new header, return last one seen". What should instead be considered as the correct way to use c-hglib is the following: -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 char *command = {"export", "-r", "0", NULL}; char buf[4096]; hg_handle *handle; hg_header *header; int n; handle = hg_open("some/repo"); hg_rawcommand(handle, command); header = hg_read_header(handle); if (header->channel == o) { while (n = hg_rawread(handle, buf, 4096), n > 0) { /* do something with buf */ } } -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 -- -- >8 Note that the current header can still be retrieved with handler->header.