<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I will answer only where it needs. For the rest of observations you can</div><div>assume that I agree with you.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div><div>:::: +#define CHANGESET "\\0{rev}\\n{node}\\n{tags}\\n{branch}\\n{author}\<br>
:::: + \\n{date|isodate}\\n{desc}"<br><br></div>As I write below, I think that hg_fetch_cset_entry() is a little less<br>cumbersome to implement if the '\0' is at the end of the changeset data<br>
instead of being at the beginning.<br><br>Reason:<br>in either case, one changeset of your history has to be "special cased".<br>If the '\0' marker is at the beginning, the last changeset has to be treated<br>
exceptionally (where will it end?).<br>If the '\0' marker is at the end, the first changeset has to be treated<br>exceptionally (where will it begin?).<br>But in the latter case, you know the answer, since the changeset data begins<br>
right after you issue the command 'hg log' to cmdsrv.<br><br></div></div></div></blockquote><div><br></div><div>Yes I agree with you that I will treat changesets in the almost same way, </div><div>it the '\0' character is on the end or at the beginning. And you present those</div>
<div>two cases almost perfect. </div><div><br></div><div>We will want to use the hg_fetch_cset_entry() function and the CHANGESET</div><div>template everywhere where it's possible and it's needed (i.e. heads, parents,</div>
<div>incoming, outgoing commands). </div><div><br></div><div>For the last two commands there will be some problems if we use the '\0' marker</div><div>at the end of template, because this command will deliver some outdata before</div>
<div>starting delivering the csets.</div><div><br></div><div>e.g:</div><div><div>$ hg outgoing --template "--cset-begin--\n{node}\n{rev}\n--cset-end--\n"</div></div><div><div>comparing with ssh://<a href="http://hg@bitbucket.org/istana/c-hglib" target="_blank">hg@bitbucket.org/istana/c-hglib</a></div>
<div>searching for changes</div><div>--cset-begin--</div><div>2ad0741f6a6793019e6e113c41c4cb2dfe37a294</div><div>64</div><div>--cset-end--</div></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div>:::: + char *new_cset = malloc(new_cset_size + 1);<div><br>:::: + memcpy(new_cset, *cset + first_cset_size, new_cset_size + 1);<br>
:::: + free(*cset);<br><br></div>The *cset you're free'ing here was malloc'ed in the 'adding_data' function,<br>as far as I can tell.<br>There you where appending a chunk of the input stream to your internal buffer;<br>
this chunk could possibly contain more than one changeset segment.<br><br>Please convince me that here you're not free'ing data below the "first changeset" boundary,<br>because it looks very much like you are doing so.<br>
</div></div></div></blockquote><div> </div><div>When the program will receive this point, the cset (unparse buffer data) will contain </div><div>some data like:</div><div>'\0aaaa\0bbbbbbbbbbb'</div><div>and what I am trying to do here is to release the memory for the first changeset( 'a' cset)</div>
<div>first_cset_size will tell me how much space this cset is occupy. And in this way I know</div><div>how much data I must save and from where. </div><div>Also when I am sending the 'first cset' to user I know how much data I must parse and</div>
<div>how much space the first cset is occupy. </div><div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div><div>:::: +/* The high level log command for hglib API. */<br>:::: +hg_csetstream_buffer *hg_log(hg_handle *handle, char *option[])<br>
:::: +{<br><br></div>I really do not understand why hg_log() has to return a hg_csetstream_buffer*.<br>There is no special information necessary to initialize the hg_csetstream_buffer*<br>that is available only from inside hg_log().<br>
It is basically just the hg_handle, which is all over the place.<br></div></div></div></blockquote><div> </div><div>I think that it's more intuitive for users, to know that they need to perform </div><div>some hg_fetch_cset_entry calls in order to call other API commands. </div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div>And, as I write a few lines below, I think hg_csetstream_buffer* is an internal<br>
that should not be exposed to the user code.<br></div></div></div></blockquote><div> </div><div>I think that we must make a bigger discussion about this issue. I agree with </div><div>you that hg_csetstream_buffer is internal and users should not change this</div>
<div>structure from their code. I also think that hg_header and hg_handle are not</div><div>suppose to be directly access from users. </div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div>Looking at your signature, I see two "output parameters":<br>(1) hg_csetstream_buffer *cbuf<br>(2) hg_cset_entry *centry<br><br>Now, the pointer cbuf look *a lot* like something<br>the user could not care less about. It's an "internal".<br>
<br>I wander if we can get rid of it, i.e. do not expose it on the function prototype.<br>I *do understand* that the only reason we keep cbuf out of hg_fetch_cset_entry()<br>is because we need it to be "persistent" across multiple function calls...<br>
I just don't like it and wander if it can be done better and cleaner.<br><br>If, on the other side, I am wrong and cbuf is something the user code<br>will need at some time (but I doubt it), then hg_fetch_cset_entry()<br>
is producing two different outputs (cbuf and centry), which is a clue<br>that probably it's doing too much.<br></div></div></div></blockquote><div><br></div><div>Like I said before hg_csetstream_buffer *cbuf, probably will be intuitive for</div>
<div>user. Also we still need a store unite, I think that we cannot put all </div><div>hg_csetstream_buffer fields in hg_handle structure, will make this structure </div><div>more harder to understand.</div><div><br></div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div>:::: + hg_header head = hg_head(cbuf->handle);<div>
<br>:::: + int exitcode;<br>:::: + char *get_data;<br>
:::: + int read_size;<br>:::: +<br>:::: + /* Erase the first cset from cset pointer.<br>:::: + * This cset was already pass to user.*/<br>:::: + if(cbuf->is_send && cbuf->buf_size){<br>:::: + cbuf->buf_size = erase_cset(&cbuf->buffer, cbuf->buf_size,<br>
:::: + cbuf->first_cset_size);<br>:::: + }<br><br></div>You start by cleaning up. I don't get it.<br>Can't you do this right after you parse the stream segment that correspond<br>to a full changeset?<br>
</div></div></div></blockquote><div><br></div><div>When I am passing the first cset to the user I am not creating a new memory</div><div>space where I am putting this cset I just give him some address pointer where </div>
<div>
data is stored. I think that it's user duty to say "I need this data I will store it </div><div>or I just want to verify it and I don't need to store it". Also I think that will give </div><div>more work to user to deal with, I mean if I am creating this additional memory</div>
<div>he will need to call a destructor every time. </div><div><br></div><div>Also I can put this data in cset_entry structure and free the memory </div><div>every time if I assume that the user will give me the same cset_entry</div>
<div>every time.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div>
I think the field hg_csetstream_buffer.is_send (which should be spelled 'is_sent', btw)<br>is useless and confusing.<br>If you clean up right after you use the data, no need to keep track of what is sent or not.<br>
<br></div></div></div></blockquote><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div>More generally, I think the whole function needs a rewrite.<br>It's convoluted, difficult to follow, the intent is not clear.<br>To an external reader it looks like everytime you encountered a bug<br>
you added an 'if-then' statement here and there.<br>
At a first glance, I could not even say if all conditions are covered<br>(if you put an 'if-then' without an 'else', you're likely to be looking for troubles).<br><br>I suggest you rewrite the body with something like:<br>
<br>if (buffer contains null byte){<br> consume data;<br> return something;<br>} else {<br> read until you get a null byte;<br>}<br><br></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div>(the above works if the null byte '\0' is at the end of the changeset template.<br>
<br></div></div></div></blockquote><div><br></div><div>OK. I will start changing the implementation. Also I think that will</div><div>work as well with '\0' byte at the beginning.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div><div><br></div>:::: + cbuf->first_cset_size = strlen(cbuf->buffer + 1) + 1;<div>
<br>
:::: + parse_changeset(cbuf->buffer + 1, centry);<br>:::: + cbuf->is_send = 1;<br>:::: + return head.length;<br><br></div>Here you return head.length, which makes very little sense:<br>
it's just the lenght of the last 'o' chunk you got from cmdsrv,<br>
while the parsed changeset you're returning (written in *centry)<br>might have been built up by multiple 'o' chunks one sticked after the other.<br><br>Another thing concerns me (we've discussed it over IRC, just reporting here<br>
for posterity): in your current hg_rawread implementation, if cmdsrv sends<br>you a chunk bigger than the size you're willing to read (i.e. you read in 4K chunks),<br>what you do is *writing* the amount of bytes still to be read into hg_header.length<br>
(see <a href="http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/62348/focus=62351" target="_blank">http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/62348/focus=62351</a> )<br><br>So: either we find a way to "const-ize" all variables of type hg_header,<br>
either we don't refer to those values out of the blue when various side-effects<br>are playing jokes under the hood.<br><br></div></div></div></blockquote><div><br></div><div>Yes we must discuss, I think that we must encapsulate hg_header structure and </div>
<div>hg_handle structure.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div><div>:::: + exitcode = hg_exitcode(cbuf->handle);<br>:::: + free(cbuf->command);<br>:::: + free(cbuf->buffer);<br>:::: + free(cbuf);<br>:::: + return exitcode;<br><br></div>
Ok this must be a leftover from previous iterations; a few lines above you're<br>
returning a byte length, here you return a status code. More discipline please.<br>More over, hg_exitcode() will be called by user code, not by this function.<br><br></div></div></div></blockquote><div><br></div><div>I think that it's a bit nasty to return exitcode in the end. But I think that</div>
<div>users don't want to know about hg_exitcode() function. If he use level 1</div><div>he want's just to receive data, some parse data. Probably he don't</div><div>want to be forced to call hg_exitcode function in the end.</div>
<div><br></div><div>I don't really know what values is hg_exitcode returning but I assume</div><div>that he return 0 for success and 1 for error, nothing else. </div><div><br></div><div>We can get exitcode in hg_cset_fetch_entry() function and verify it</div>
<div>and also return it in other form.</div><div>e.g:</div><div><br></div><div><span style="font-family:arial,sans-serif;font-size:13px">>>(*) it's bool eval-ed to 1 if there is stuff to fetch,</span><br style="font-family:arial,sans-serif;font-size:13px">
<span style="font-family:arial,sans-serif;font-size:13px">>>(*) it's bool eval-ed to 0 if there is nothing more to fetch,</span><br style="font-family:arial,sans-serif;font-size:13px"><span style="font-family:arial,sans-serif;font-size:13px">>>(*) and it can give clues if something goes wrong.</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div><div><div>cheers,<br></div></div>GGhh<br><br></div></div></div>
</blockquote></div><div><br></div>-- <div dir="ltr"><div>Iulian</div></div>
</div></div>