Patchwork [1,of,7,c-hglib] hg_read_header: return NULL if header not present

login
register
mail settings
Submitter Giovanni Gherdovich
Date Nov. 23, 2014, 5:54 p.m.
Message ID <0dc61b8b4a873ba6174f.1416765285@tosh>
Download mbox | patch
Permalink /patch/6830/
State Accepted
Delegated to: Pierre-Yves David
Headers show

Comments

Giovanni Gherdovich - Nov. 23, 2014, 5:54 p.m.
# 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.

Patch

diff -r d36ee5026669 -r 0dc61b8b4a87 hglib/client.c
--- a/hglib/client.c	Fri Oct 10 19:17:56 2014 +0200
+++ b/hglib/client.c	Sat Nov 22 11:14:37 2014 +0100
@@ -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;
 }