Submitter | Iulian Stana |
---|---|
Date | Sept. 1, 2013, 6:43 p.m. |
Message ID | <0d7faa1c7ad6bc680fe2.1378061002@doppler> |
Download | mbox | patch |
Permalink | /patch/2300/ |
State | Superseded |
Headers | show |
Comments
2013/9/1 Iulian Stana <julian.stana@gmail.com> > # HG changeset patch > # User Iulian Stana <julian.stana@gmail.com> > # Date 1378056765 -10800 > # Sun Sep 01 20:32:45 2013 +0300 > # Node ID 0d7faa1c7ad6bc680fe2bc2cdad0c3becaabd603 > # Parent 301aad54936be170f1915a034893911f4e742c93 > model (1): c-hglib: hg_log() level 1 function > > This mechanism could be called model (1): > > [...] > + > +/* The cbuf next step. Getting the next changeset. */ > +int hg_fetch_cset_entry(hg_csetstream_buffer *cbuf, hg_cset_entry *centry) > +{ > I am not sure that this function behaves as it is suppose to. In particular, I am afraid you assume that changeset boundaries and 'o' writes boundaries coincide (and they don't, in the general case). You should assume that boundaries of 'o' chunks and changeset delimiters (null bytes \0) are totally unrelated events in the data stream. Could you please test it against the python script below, which is supposed to emulate the command server? You run it giving as argument the name of the test function you want to execute (eg 'python testserv t1'). GGhh -- -- >8 -- -- >8 -- -- >8 -- -- cut here -- -- >8 -- -- >8 -- -- >8 import sys import struct import os """ memo: the template is "\0{rev}\n{node}\n{tags}\n{branch}\n{author}\n{date|isodate}\n{desc}" The test changeset data: \00\nXoooooo\nfoo\ndefault\nBabar\nJan 1st, 1970\nI am the King of Celesteville. \01\noXooooo\nboo\ndefault\nCeleste\nJan 2nd, 1970\nI am the Queen of Celesteville. \02\nooXoooo\nwoo\ndefault\nArthur\nJan 3rd, 1970\nI am Celeste's brother. \03\noooXooo\nhoo\ndefault\nPom\nJan 4th, 1970\nI am the oldest of the kids. \04\nooooXoo\ndoo\ndefault\nFlora\nJan 5th, 1970\nPom and Alexander make fun of me. \05\noooooXo\nmoo\ndefault\nAlexander\nJan 6th, 1970\nI love playing with Pom. \06\nooooooX\npoo\ndefault\nIsabelle\nJan 7th, 1970\nI am the yungest, but I'm very smart. \07\nooooooo\nyoo\ndefault\nLord Rataxes\nJan 8th, 1970\nI am the king of Rhinoland. The docstring of each test function describes the output. """ def t1(): """ \0aaaaaa """ chunk1 = ("\00\nXoooooo\nfoo\ndefault\nBabar\n" + "Jan 1st, 1970\nI am the King of Celesteville.") sys.stdout.write(struct.pack('>cI', 'o', len(chunk1))) sys.stdout.write(chunk1) sys.stdout.write(struct.pack('>cI', 'r', 4)) sys.stdout.write('\0\0\0\0') sys.stdout.flush() def t2(): """ \0aaaaaa\0bbbb """ chunk1 = ("\01\noXooooo\nboo\ndefault\nCeleste\n" + "Jan 2nd, 1970\nI am the Queen of Celesteville." + "\02\nooXoooo\nwoo\ndefault\nArthur\n" + "Jan 3rd, 1970\nI am Celeste's brother.") sys.stdout.write(struct.pack('>cI', 'o', len(chunk1))) sys.stdout.write(chunk1) sys.stdout.write(struct.pack('>cI', 'r', 4)) sys.stdout.write('\0\0\0\0') sys.stdout.flush() def t3(): """ \0aaaa aaaaaa aa """ chunk1 = "\03\noooXooo\nhoo\ndef" chunk2 = "ault\nPom\nJan 4th, 19" chunk3 = "70\nI am the oldest of the kids." sys.stdout.write(struct.pack('>cI', 'o', len(chunk1))) sys.stdout.write(chunk1) sys.stdout.write(struct.pack('>cI', 'o', len(chunk2))) sys.stdout.write(chunk2) sys.stdout.write(struct.pack('>cI', 'o', len(chunk3))) sys.stdout.write(chunk3) sys.stdout.write(struct.pack('>cI', 'r', 4)) sys.stdout.write('\0\0\0\0') sys.stdout.flush() def t4(): """ \0aaaaa aaa\0bbbb\0ccc\0ddd ddd """ chunk1 = "\04\nooooXoo\ndoo\ndefault\nFlora\nJan 5th, " chunk2 = ("1970\nPom and Alexander make fun of me." + "\05\noooooXo\nmoo\ndefault\nAlexander\nJan 6th, " "1970\nI love playing with Pom." + "\06\nooooooX\npoo\ndefault\nIsabe" + " lle\nJan 7th, 1970\nI am the yungest, " + "but I'm very smart." + "\07\nooooooo\nyoo\ndefault\nLord ") chunk3 = "Rataxes\nJan 8th, 1970\nI am the king of Rhinoland." sys.stdout.write(struct.pack('>cI', 'o', len(chunk1))) sys.stdout.write(chunk1) sys.stdout.write(struct.pack('>cI', 'o', len(chunk2))) sys.stdout.write(chunk2) sys.stdout.write(struct.pack('>cI', 'o', len(chunk3))) sys.stdout.write(chunk3) sys.stdout.write(struct.pack('>cI', 'r', 4)) sys.stdout.write('\0\0\0\0') sys.stdout.flush() test = {'t1': t1, 't2': t2, 't3': t3, 't4': t4} if __name__ == '__main__': test[sys.argv[1]]() os.read(0, 1) -- -- >8 -- -- >8 -- -- >8 -- -- cut here -- -- >8 -- -- >8 -- -- >8
Hi Iulian, this is a long review. I add a table of contents so to make random access easier. 1. "#define _GNU_SOURCE", wat 2. log template is better changed 3. Rename the 'adding_data()' function 4. Rename "new_cset_size" and "cset" local variable in 'erase_cset()' 5. Check correct usage of free(3) 6. No reason why hg_log() should return a hg_csetstream_buffer* 7. Discuss return value of hg_fetch_cset_entry() 8. Do we really need to expose hg_csetstream_buffer* to the user code 9. Review your logic in hg_fetch_cset_entry(), make it more "linear" 10. hg_log() should handle error message, too 11. strlen(3) is not a null byte detector, don't use it as such 12. hg_exitcode() to be called by user code, not inside hg_fetch_cset_entry() :::: diff --git a/client.c b/client.c :::: new file mode 100644 :::: --- /dev/null :::: +++ b/client.c :::: @@ -0,0 +1,175 @@ :::: +#define _GNU_SOURCE I would prefer not to see the _GNU_SOURCE macro identifier defined in your source. Either make your code work without it, or provide a thorough explanation of why it is so desperately needed. :::: +#include <errno.h> :::: +#include <fcntl.h> :::: +#include <stdlib.h> :::: +#include <stdio.h> :::: +#include <string.h> :::: +#include <unistd.h> :::: +#include <sys/types.h> :::: +#include <sys/wait.h> :::: +#include <signal.h> :::: + :::: + :::: +#include "client.h" :::: +#include "utils.h" :::: + :::: +#define HGPATH "hg" :::: +#define CHANGESET "\\0{rev}\\n{node}\\n{tags}\\n{branch}\\n{author}\ :::: + \\n{date|isodate}\\n{desc}" As I write below, I think that hg_fetch_cset_entry() is a little less cumbersome to implement if the '\0' is at the end of the changeset data instead of being at the beginning. Reason: in either case, one changeset of your history has to be "special cased". If the '\0' marker is at the beginning, the last changeset has to be treated exceptionally (where will it end?). If the '\0' marker is at the end, the first changeset has to be treated exceptionally (where will it begin?). But in the latter case, you know the answer, since the changeset data begins right after you issue the command 'hg log' to cmdsrv. We chosed the above template since it's the same as in JavaHG at commit https://bitbucket.org/aragost/javahg/commits/7855d21c99e1#chg-src/main/java/com/aragost/javahg/Changeset.java But I went checking again, and it looks like they ended up with '\0' at the end: https://bitbucket.org/aragost/javahg/src/tip/src/main/resources/styles/changesets.style?at=default Martin Geisler please correct me if I'm wrong. :::: + :::: + :::: + :::: +/** :::: + * \brief 'Parse a changeset'. It's more like pointing to the correct position. :::: + * :::: + * The changeset could be found on buff pointer. To not duplicate the data I :::: + * choose to point every hg_cset_entry field to the right position. :::: + * \param cset The pointer where changeset could be found. :::: + * \param ce The hg_cset_entry structure where the changeset will be parse. :::: + * \retval 0 if successful. :::: + * */ :::: +int parse_changeset(char *cset, hg_cset_entry *ce) :::: +{ :::: + char *position = cset; :::: + /* set pointer for revision position */ :::: + ce->rev = cset; :::: + position = strstr(position, "\n"); :::: + cset[position - cset] = '\0'; :::: + :::: + /* set pointer for node position */ :::: + ce->node = position + 1; :::: + position = strstr(position + 1, "\n"); :::: + cset[position - cset] = '\0'; :::: + :::: + /* set pointer for tag position */ :::: + ce->tags = position + 1; :::: + position = strstr(position + 1, "\n"); :::: + cset[position - cset] = '\0'; :::: + :::: + /* set pointer for branch position */ :::: + ce->branch = position + 1; :::: + position = strstr(position + 1, "\n"); :::: + cset[position - cset] = '\0'; :::: + :::: + /* set pointer for author position */ :::: + ce->author = position + 1; :::: + position = strstr(position + 1, "\n"); :::: + cset[position - cset] = '\0'; :::: + :::: + /* set pointer for data position */ :::: + ce->date = position + 1; :::: + position = strstr(position + 1, "\n"); :::: + cset[position - cset] = '\0'; :::: + :::: + /* set pointer for description position */ :::: + ce->desc = position + 1; :::: + /* */ :::: + return 0; :::: +} :::: + :::: +/* Adding to the destination pointer the source pointer. */ :::: +int adding_data(char **dest, char *source, int dsize, int ssize) Rename to 'append_data'. Please don't use the gerund tense in function names, the simple present tense is better. :::: +{ :::: + if(*dest == NULL){ :::: + *dest = malloc(ssize + 1); :::: + memcpy(*dest, source, ssize + 1); :::: + } else { :::: + *dest = realloc(*dest, dsize + ssize + 2); :::: + memcpy(*dest + dsize, source, ssize + 1); :::: + } :::: + return 0; :::: +} :::: + :::: +/* Erase the top cset from cset pointer. */ :::: +int erase_cset(char **cset, int buf_size, int first_cset_size) :::: +{ :::: + int new_cset_size = buf_size - first_cset_size; (*) "new_cset_size" it is *not* the size of a changeset, it's the size of the buffer. Name it accordingly, like "new_buf_size". (*) 'cset' is not changeset data, is the content of a raw unparsed buffer. Name it accordingly. :::: + char *new_cset = malloc(new_cset_size + 1); :::: + memcpy(new_cset, *cset + first_cset_size, new_cset_size + 1); :::: + free(*cset); The *cset you're free'ing here was malloc'ed in the 'adding_data' function, as far as I can tell. There you where appending a chunk of the input stream to your internal buffer; this chunk could possibly contain more than one changeset segment. Please convince me that here you're not free'ing data below the "first changeset" boundary, because it looks very much like you are doing so. :::: + *cset = new_cset; :::: + return new_cset_size; :::: +} :::: + :::: + :::: +/* The high level log command for hglib API. */ :::: +hg_csetstream_buffer *hg_log(hg_handle *handle, char *option[]) :::: +{ I really do not understand why hg_log() has to return a hg_csetstream_buffer*. There is no special information necessary to initialize the hg_csetstream_buffer* that is available only from inside hg_log(). It is basically just the hg_handle, which is all over the place. And, as I write a few lines below, I think hg_csetstream_buffer* is an internal that should not be exposed to the user code. :::: + hg_csetstream_buffer *cbuf = malloc(sizeof(hg_csetstream_buffer)); :::: + cbuf->handle = handle; :::: + :::: + cbuf->command = cmdbuilder("log", option, "--template", CHANGESET, :::: + NULL); :::: + :::: + if(hg_rawcommand(handle, cbuf->command) < 0){ :::: + return NULL; :::: + } :::: + :::: + cbuf->buffer = NULL; :::: + cbuf->buf_size = 0; :::: + cbuf->is_send = 0; :::: + :::: + return cbuf; :::: +} :::: + :::: +/* The cbuf next step. Getting the next changeset. */ :::: +int hg_fetch_cset_entry(hg_csetstream_buffer *cbuf, hg_cset_entry *centry) :::: +{ Let's look at things from 100 kilometers away: here our goal is to write a function that enables the pattern while( hg_fetch_cset_entry( cset_entry ) ) { /* ...do stuff with cset_entry... */ } cset_entry is "something" that contains info about *one* and *only one* changeset. The return value of hg_fetch_cset_entry() should be such that (*) it's bool eval-ed to 1 if there is stuff to fetch, (*) it's bool eval-ed to 0 if there is nothing more to fetch, (*) and it can give clues if something goes wrong. Pretty much like read(2). Looking at your signature, I see two "output parameters": (1) hg_csetstream_buffer *cbuf (2) hg_cset_entry *centry Now, the pointer cbuf look *a lot* like something the user could not care less about. It's an "internal". I wander if we can get rid of it, i.e. do not expose it on the function prototype. I *do understand* that the only reason we keep cbuf out of hg_fetch_cset_entry() is because we need it to be "persistent" across multiple function calls... I just don't like it and wander if it can be done better and cleaner. If, on the other side, I am wrong and cbuf is something the user code will need at some time (but I doubt it), then hg_fetch_cset_entry() is producing two different outputs (cbuf and centry), which is a clue that probably it's doing too much. :::: + hg_header head = hg_head(cbuf->handle); :::: + int exitcode; :::: + char *get_data; :::: + int read_size; :::: + :::: + /* Erase the first cset from cset pointer. :::: + * This cset was already pass to user.*/ :::: + if(cbuf->is_send && cbuf->buf_size){ :::: + cbuf->buf_size = erase_cset(&cbuf->buffer, cbuf->buf_size, :::: + cbuf->first_cset_size); :::: + } You start by cleaning up. I don't get it. Can't you do this right after you parse the stream segment that correspond to a full changeset? I think the field hg_csetstream_buffer.is_send (which should be spelled 'is_sent', btw) is useless and confusing. If you clean up right after you use the data, no need to keep track of what is sent or not. More generally, I think the whole function needs a rewrite. It's convoluted, difficult to follow, the intent is not clear. To an external reader it looks like everytime you encountered a bug you added an 'if-then' statement here and there. At a first glance, I could not even say if all conditions are covered (if you put an 'if-then' without an 'else', you're likely to be looking for troubles). I suggest you rewrite the body with something like: if (buffer contains null byte){ consume data; return something; } else { read until you get a null byte; } (the above works if the null byte '\0' is at the end of the changeset template. :::: + while(head.channel != 'r'){ I already told you this, just a reminder: you're forgetting the error channel 'e' here. Use the callback strategy to handle error messages. :::: + /* If there is a cset in cset pointer, then parse it and send :::: + * it to user.*/ :::: + if(cbuf->buffer && strlen(cbuf->buffer + 1) < cbuf->buf_size -1){ I *absolutely* don't like the use of strlen as a "null-byte" detector. As you can imagine, strlen keeps a local pointer that runs until it finds '\0': https://sourceware.org/git/?p=glibc.git;a=blob;f=string/strlen.c Since there is no guarantee that in your case the '\0' will ever be found, this pointer can go a long way before strlen will return a value. Please look for '\0' explicitely, like -- -- >8 -- -- cut here -- -- >8 -- -- >8 char *char_ptr; int null_byte_found = 0; size_t cset_size = 0; for (char_ptr = cbuf->buffer; char_ptr < cbuf->buffer + cbuf->buf_size; ++char_ptr) if (*char_ptr == '\0') int null_byte_found = 1; break; if (null_byte_found) cset_size = char_ptr - cbuf->buffer; -- -- >8 -- -- cut here -- -- >8 -- -- >8 :::: + cbuf->first_cset_size = strlen(cbuf->buffer + 1) + 1; :::: + parse_changeset(cbuf->buffer + 1, centry); :::: + cbuf->is_send = 1; :::: + return head.length; Here you return head.length, which makes very little sense: it's just the lenght of the last 'o' chunk you got from cmdsrv, while the parsed changeset you're returning (written in *centry) might have been built up by multiple 'o' chunks one sticked after the other. Another thing concerns me (we've discussed it over IRC, just reporting here for posterity): in your current hg_rawread implementation, if cmdsrv sends you a chunk bigger than the size you're willing to read (i.e. you read in 4K chunks), what you do is *writing* the amount of bytes still to be read into hg_header.length (see http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/62348/focus=62351) So: either we find a way to "const-ize" all variables of type hg_header, either we don't refer to those values out of the blue when various side-effects are playing jokes under the hood. :::: + } :::: + else{ :::: + /* Getting the next data from cmdserver and put on the :::: + * end of the cset pointer. */ :::: + get_data = malloc(head.length + 1); :::: + if(read_size = hg_rawread(cbuf->handle, get_data, :::: + head.length), read_size < 0){ :::: + return -1; :::: + } :::: + adding_data(&cbuf->buffer, get_data, cbuf->buf_size, :::: + read_size); :::: + cbuf->buf_size += read_size; :::: + head = hg_head(cbuf->handle); :::: + free(get_data); :::: + } :::: + } :::: + /* After, receiveing the last message, there still could be some :::: + * csets on cset pointer. */ :::: + if(cbuf->buffer && strlen(cbuf->buffer + 1) == cbuf->buf_size -1){ A rather obscure condition to say "in the first (cbuf->buf_size) bytes I don't have any \0 character". Please make things more explicit, and please don't use strlen as "null byte detector". :::: + cbuf->first_cset_size = strlen(cbuf->buffer + 1) + 1; :::: + parse_changeset(cbuf->buffer + 1, centry); :::: + cbuf->buf_size = 0; :::: + cbuf->is_send = 0; :::: + return head.length; :::: + /* Parse first cset from the remaining data. */ :::: + }else if(cbuf->buf_size && cbuf->is_send){ :::: + cbuf->first_cset_size = strlen(cbuf->buffer + 1) + 1; :::: + parse_changeset(cbuf->buffer + 1, centry); :::: + cbuf->is_send = 1; :::: + return head.length; :::: + } :::: + :::: + exitcode = hg_exitcode(cbuf->handle); :::: + free(cbuf->command); :::: + free(cbuf->buffer); :::: + free(cbuf); :::: + return exitcode; Ok this must be a leftover from previous iterations; a few lines above you're returning a byte length, here you return a status code. More discipline please. More over, hg_exitcode() will be called by user code, not by this function. :::: + :::: +} :::: diff --git a/client.h b/client.h :::: --- a/client.h :::: +++ b/client.h :::: @@ -69,6 +69,24 @@ :::: char *out_data; :::: } hg_handle; :::: :::: +typedef struct hg_csetstream_buffer{ :::: + hg_handle *handle; :::: + char **command; :::: + char *buffer; :::: + int buf_size; :::: + int is_send; After my remark above, I don't think you need this flag. It should be spelled "is_sent", anyway (note the `t`). :::: + int first_cset_size; :::: +}hg_csetstream_buffer; :::: + :::: +typedef struct hg_cset_entry{ :::: + char *author; :::: + char *branch; :::: + char *date; :::: + char *desc; :::: + char *node; :::: + char *rev; :::: + char *tags; :::: +}hg_cset_entry; :::: :::: /** :::: * \brief Open the connection with the mercurial command server. :::: @@ -215,4 +233,63 @@ :::: * */ :::: int hg_exitcode(hg_handle *handle); :::: :::: +/** :::: + * \brief hg_log command for hglib API. :::: + * :::: + * It's an advance function to get revision history. It's more like the start :::: + * point of the action, this function will prepare the query question and will :::: + * send it to the cmd-server. :::: + * :::: + * Return the revision history of the specified files or the entire project. :::: + * File history is shown without following rename or copy history of files. :::: + * Use follow with a filename to follow history across renames and copies. :::: + * follow without a filename will only show ancestors or descendants of the :::: + * starting revision. followfirst only follows the first parent of merge :::: + * revisions. :::: + * :::: + * If revrange isn't specified, the default is "tip:0" unless follow is set, :::: + * in which case the working directory parent is used as the starting :::: + * revision. :::: + * :::: + * \param handle The handle of the connection, wherewith I want to communicate :::: + * \param option The option list for mercurial log command. :::: + * \retval hg_csetstream_buffer A pointer to hg_csetstream_buffer structure if :::: + * successful :::: + * \retval NULL to indicate an error, with errno set appropriately. :::: + * :::: + * errno can be: :::: + * - hg_rawcommand errors :::: + * */ :::: +hg_csetstream_buffer *hg_log(hg_handle *handle, char *option[]); :::: + :::: +/** :::: + * \brief The iterator step. Getting the next changeset. :::: + * :::: + * The revision history could have a huge mass of data. You cannot pass the :::: + * entire history in one call, so we use an iterator-like mechanism. Calling :::: + * the hg_fetch_log_entry. The next changeset will be read from cmd-server, :::: + * parse and pass to hg_cset_entry structure. :::: + * The cset_entry structure will handle a changeset with the following string :::: + * fields: :::: + * - rev :::: + * - node :::: + * - tags (space delimited) :::: + * - branch :::: + * - author :::: + * - desc :::: + * :::: + * \param hg_csetstream_buffer The buffer structure to store cset data. :::: + * \param centry The hg_cset_entry structure where the changeset will be stored :::: + * and pass :::: + * \retval number The lenght for the pass changeset. :::: + * \retval exitcode To indicate the end of current_command. :::: + * \retval -1 to indicate an error, with errno set appropriately. :::: + * :::: + * errno can be: :::: + * - EINVAL - Invalid argument (handle it's set to a null pointer) :::: + * - read(2) command errors :::: + * - read_header error :::: + * */ :::: +int hg_fetch_cset_entry(hg_csetstream_buffer *cbuf, hg_cset_entry *centry); :::: + :::: #endif :::: diff --git a/main.c b/main.c :::: new file mode 100644 :::: --- /dev/null :::: +++ b/main.c :::: @@ -0,0 +1,128 @@ :::: +#include <stdio.h> :::: +#include <stdlib.h> :::: +#include <string.h> :::: + :::: +#include <sys/wait.h> :::: +#include <sys/types.h> :::: +#include <unistd.h> :::: +#include <sys/stat.h> :::: +#include <fcntl.h> :::: + :::: +#include "client.h" :::: +#include "utils.h" :::: + :::: +#define INIT_REPO "init_test_repo" :::: + :::: +/****** Convenience functions. *******/ :::: + :::: +/** :::: + * \brief Create and setup the tmp directory where the acction will happends. :::: + * */ :::: +void setup_tmp() :::: +{ :::: + system("hg init tmp"); :::: + chdir("tmp"); :::: +} :::: + :::: +/** :::: + * \brief Remove the tmp directory and all his files. :::: + * */ :::: +void clean_tmp() :::: +{ :::: + chdir(".."); :::: + system("rm -rf tmp"); :::: +} :::: + :::: +/** :::: + * \brief Fill the current repository with commits for log command. :::: + * */ :::: +void setup_log() :::: +{ :::: + system("touch foo ; hg add foo ; hg commit -m 'foo file'"); :::: + system("echo baloo > foo ; hg commit -m 'baloo text'"); :::: + system("touch voodoo ; hg add voodoo ; hg commit -m voodoo"); :::: + system("echo voodoo > voodoo ; hg commit -m 'voodoo text'"); :::: +} :::: + :::: +/******* Examples using level 1 implementations. ******/ :::: + :::: +/** :::: + * \brief Log command example. :::: + * :::: + * \param handle The handle of the connection, wherewith I want to communicate :::: + * \retval exitcode :::: + * */ :::: +int hg_log_example(hg_handle *handle) :::: +{ :::: + char *option[] = {"-v", NULL}; :::: + int nc; :::: + :::: + /* hg_log function will a iterator. */ :::: + hg_csetstream_buffer *log_iterator = hg_log(handle, option); :::: + :::: + /* you need to alloc some space for log_entry_t structure */ :::: + hg_cset_entry *le = malloc(sizeof(hg_cset_entry)); :::: + :::: + /* Getting the next changeset using the iterator-like mechanism. :::: + Print the changest from log_entry structure.*/ :::: + while(nc = hg_fetch_cset_entry(log_iterator, le), nc > 0){ :::: + printf("rev = %s \n", le->rev); :::: + printf("node = %s \n", le->node); :::: + printf("tags = %s \n", le->tags); :::: + printf("branch = %s \n", le->branch); :::: + printf("author = %s \n", le->author); :::: + printf("date = %s \n", le->date); :::: + printf("desc = %s \n", le->desc); :::: + printf("\n"); :::: + } :::: + :::: + free(le); :::: + /* last call for hg_fetch_log_entry will pass the exitcode */ :::: + return nc; :::: +} :::: + :::: +/** \brief Printing the welcome message. :::: + * :::: + * Will print the options that you will have in this example. :::: + **/ :::: +void print_select_case() :::: +{ :::: + printf("Select test case to run:\n"); :::: + printf("1) log \n"); :::: + printf("\n"); :::: + printf("Your choice: "); :::: +} :::: + :::: + :::: + :::: +/***** Main function. *******/ :::: +/** :::: + * \brief The main function :::: + * */ :::: +int main(int argc, char **argv) :::: +{ :::: + int select_case; :::: + hg_handle *handle; :::: + :::: + print_select_case(); :::: + scanf("%d", &select_case); :::: + if(select_case < 1 || select_case > 1){ :::: + printf("Your choice is not an option...\n"); :::: + return -1; :::: + } :::: + :::: + switch(select_case){ :::: + case 1: :::: + setup_tmp(); :::: + setup_log(); :::: + handle = hg_open(NULL, ""); :::: + :::: + hg_log_example(handle); :::: + :::: + hg_close(&handle); :::: + clean_tmp(); :::: + break; :::: + } :::: + :::: + return 0; :::: +} cheers, GGhh
>>> >>> if (buffer contains null byte){ >>> consume data; >>> return something; >>> } else { >>> read until you get a null byte; >>> } >>> the above is incorrect. Less wrong: ------------------------------------------ if (buffer *does not* contains null byte){ read until you get a null byte; } consume data; return something; ------------------------------------------ I mean, you got it. GGhh
I will answer only where it needs. For the rest of observations you can assume that I agree with you. > :::: +#define CHANGESET "\\0{rev}\\n{node}\\n{tags}\\n{branch}\\n{author}\ > :::: + \\n{date|isodate}\\n{desc}" > > As I write below, I think that hg_fetch_cset_entry() is a little less > cumbersome to implement if the '\0' is at the end of the changeset data > instead of being at the beginning. > > Reason: > in either case, one changeset of your history has to be "special cased". > If the '\0' marker is at the beginning, the last changeset has to be > treated > exceptionally (where will it end?). > If the '\0' marker is at the end, the first changeset has to be treated > exceptionally (where will it begin?). > But in the latter case, you know the answer, since the changeset data > begins > right after you issue the command 'hg log' to cmdsrv. > > Yes I agree with you that I will treat changesets in the almost same way, it the '\0' character is on the end or at the beginning. And you present those two cases almost perfect. We will want to use the hg_fetch_cset_entry() function and the CHANGESET template everywhere where it's possible and it's needed (i.e. heads, parents, incoming, outgoing commands). For the last two commands there will be some problems if we use the '\0' marker at the end of template, because this command will deliver some outdata before starting delivering the csets. e.g: $ hg outgoing --template "--cset-begin--\n{node}\n{rev}\n--cset-end--\n" comparing with ssh://hg@bitbucket.org/istana/c-hglib searching for changes --cset-begin-- 2ad0741f6a6793019e6e113c41c4cb2dfe37a294 64 --cset-end-- :::: + char *new_cset = malloc(new_cset_size + 1); > > :::: + memcpy(new_cset, *cset + first_cset_size, new_cset_size + 1); > :::: + free(*cset); > > The *cset you're free'ing here was malloc'ed in the 'adding_data' function, > as far as I can tell. > There you where appending a chunk of the input stream to your internal > buffer; > this chunk could possibly contain more than one changeset segment. > > Please convince me that here you're not free'ing data below the "first > changeset" boundary, > because it looks very much like you are doing so. > When the program will receive this point, the cset (unparse buffer data) will contain some data like: '\0aaaa\0bbbbbbbbbbb' and what I am trying to do here is to release the memory for the first changeset( 'a' cset) first_cset_size will tell me how much space this cset is occupy. And in this way I know how much data I must save and from where. Also when I am sending the 'first cset' to user I know how much data I must parse and how much space the first cset is occupy. :::: +/* The high level log command for hglib API. */ > :::: +hg_csetstream_buffer *hg_log(hg_handle *handle, char *option[]) > :::: +{ > > I really do not understand why hg_log() has to return a > hg_csetstream_buffer*. > There is no special information necessary to initialize the > hg_csetstream_buffer* > that is available only from inside hg_log(). > It is basically just the hg_handle, which is all over the place. > I think that it's more intuitive for users, to know that they need to perform some hg_fetch_cset_entry calls in order to call other API commands. > And, as I write a few lines below, I think hg_csetstream_buffer* is an > internal > that should not be exposed to the user code. > I think that we must make a bigger discussion about this issue. I agree with you that hg_csetstream_buffer is internal and users should not change this structure from their code. I also think that hg_header and hg_handle are not suppose to be directly access from users. > Looking at your signature, I see two "output parameters": > (1) hg_csetstream_buffer *cbuf > (2) hg_cset_entry *centry > > Now, the pointer cbuf look *a lot* like something > the user could not care less about. It's an "internal". > > I wander if we can get rid of it, i.e. do not expose it on the function > prototype. > I *do understand* that the only reason we keep cbuf out of > hg_fetch_cset_entry() > is because we need it to be "persistent" across multiple function calls... > I just don't like it and wander if it can be done better and cleaner. > > If, on the other side, I am wrong and cbuf is something the user code > will need at some time (but I doubt it), then hg_fetch_cset_entry() > is producing two different outputs (cbuf and centry), which is a clue > that probably it's doing too much. > Like I said before hg_csetstream_buffer *cbuf, probably will be intuitive for user. Also we still need a store unite, I think that we cannot put all hg_csetstream_buffer fields in hg_handle structure, will make this structure more harder to understand. > :::: + hg_header head = hg_head(cbuf->handle); > > :::: + int exitcode; > :::: + char *get_data; > :::: + int read_size; > :::: + > :::: + /* Erase the first cset from cset pointer. > :::: + * This cset was already pass to user.*/ > :::: + if(cbuf->is_send && cbuf->buf_size){ > :::: + cbuf->buf_size = erase_cset(&cbuf->buffer, cbuf->buf_size, > :::: + cbuf->first_cset_size); > :::: + } > > You start by cleaning up. I don't get it. > Can't you do this right after you parse the stream segment that correspond > to a full changeset? > When I am passing the first cset to the user I am not creating a new memory space where I am putting this cset I just give him some address pointer where data is stored. I think that it's user duty to say "I need this data I will store it or I just want to verify it and I don't need to store it". Also I think that will give more work to user to deal with, I mean if I am creating this additional memory he will need to call a destructor every time. Also I can put this data in cset_entry structure and free the memory every time if I assume that the user will give me the same cset_entry every time. > I think the field hg_csetstream_buffer.is_send (which should be spelled > 'is_sent', btw) > is useless and confusing. > If you clean up right after you use the data, no need to keep track of > what is sent or not. > > > More generally, I think the whole function needs a rewrite. > It's convoluted, difficult to follow, the intent is not clear. > To an external reader it looks like everytime you encountered a bug > you added an 'if-then' statement here and there. > At a first glance, I could not even say if all conditions are covered > (if you put an 'if-then' without an 'else', you're likely to be looking > for troubles). > > I suggest you rewrite the body with something like: > > if (buffer contains null byte){ > consume data; > return something; > } else { > read until you get a null byte; > } > > (the above works if the null byte '\0' is at the end of the changeset > template. > > OK. I will start changing the implementation. Also I think that will work as well with '\0' byte at the beginning. > :::: + cbuf->first_cset_size = strlen(cbuf->buffer + 1) + 1; > > :::: + parse_changeset(cbuf->buffer + 1, centry); > :::: + cbuf->is_send = 1; > :::: + return head.length; > > Here you return head.length, which makes very little sense: > it's just the lenght of the last 'o' chunk you got from cmdsrv, > while the parsed changeset you're returning (written in *centry) > might have been built up by multiple 'o' chunks one sticked after the > other. > > Another thing concerns me (we've discussed it over IRC, just reporting here > for posterity): in your current hg_rawread implementation, if cmdsrv sends > you a chunk bigger than the size you're willing to read (i.e. you read in > 4K chunks), > what you do is *writing* the amount of bytes still to be read into > hg_header.length > (see > http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/62348/focus=62351) > > So: either we find a way to "const-ize" all variables of type hg_header, > either we don't refer to those values out of the blue when various > side-effects > are playing jokes under the hood. > > Yes we must discuss, I think that we must encapsulate hg_header structure and hg_handle structure. > :::: + exitcode = hg_exitcode(cbuf->handle); > :::: + free(cbuf->command); > :::: + free(cbuf->buffer); > :::: + free(cbuf); > :::: + return exitcode; > > Ok this must be a leftover from previous iterations; a few lines above > you're > returning a byte length, here you return a status code. More discipline > please. > More over, hg_exitcode() will be called by user code, not by this function. > > I think that it's a bit nasty to return exitcode in the end. But I think that users don't want to know about hg_exitcode() function. If he use level 1 he want's just to receive data, some parse data. Probably he don't want to be forced to call hg_exitcode function in the end. I don't really know what values is hg_exitcode returning but I assume that he return 0 for success and 1 for error, nothing else. We can get exitcode in hg_cset_fetch_entry() function and verify it and also return it in other form. e.g: >>(*) it's bool eval-ed to 1 if there is stuff to fetch, >>(*) it's bool eval-ed to 0 if there is nothing more to fetch, >>(*) and it can give clues if something goes wrong. cheers, > GGhh > >
Patch
diff --git a/client.c b/client.c new file mode 100644 --- /dev/null +++ b/client.c @@ -0,0 +1,175 @@ +#define _GNU_SOURCE +#include <errno.h> +#include <fcntl.h> +#include <stdlib.h> +#include <stdio.h> +#include <string.h> +#include <unistd.h> +#include <sys/types.h> +#include <sys/wait.h> +#include <signal.h> + + +#include "client.h" +#include "utils.h" + +#define HGPATH "hg" +#define CHANGESET "\\0{rev}\\n{node}\\n{tags}\\n{branch}\\n{author}\ + \\n{date|isodate}\\n{desc}" + + + +/** + * \brief 'Parse a changeset'. It's more like pointing to the correct position. + * + * The changeset could be found on buff pointer. To not duplicate the data I + * choose to point every hg_cset_entry field to the right position. + * \param cset The pointer where changeset could be found. + * \param ce The hg_cset_entry structure where the changeset will be parse. + * \retval 0 if successful. + * */ +int parse_changeset(char *cset, hg_cset_entry *ce) +{ + char *position = cset; + /* set pointer for revision position */ + ce->rev = cset; + position = strstr(position, "\n"); + cset[position - cset] = '\0'; + + /* set pointer for node position */ + ce->node = position + 1; + position = strstr(position + 1, "\n"); + cset[position - cset] = '\0'; + + /* set pointer for tag position */ + ce->tags = position + 1; + position = strstr(position + 1, "\n"); + cset[position - cset] = '\0'; + + /* set pointer for branch position */ + ce->branch = position + 1; + position = strstr(position + 1, "\n"); + cset[position - cset] = '\0'; + + /* set pointer for author position */ + ce->author = position + 1; + position = strstr(position + 1, "\n"); + cset[position - cset] = '\0'; + + /* set pointer for data position */ + ce->date = position + 1; + position = strstr(position + 1, "\n"); + cset[position - cset] = '\0'; + + /* set pointer for description position */ + ce->desc = position + 1; + /* */ + return 0; +} + +/* Adding to the destination pointer the source pointer. */ +int adding_data(char **dest, char *source, int dsize, int ssize) +{ + if(*dest == NULL){ + *dest = malloc(ssize + 1); + memcpy(*dest, source, ssize + 1); + } else { + *dest = realloc(*dest, dsize + ssize + 2); + memcpy(*dest + dsize, source, ssize + 1); + } + return 0; +} + +/* Erase the top cset from cset pointer. */ +int erase_cset(char **cset, int buf_size, int first_cset_size) +{ + int new_cset_size = buf_size - first_cset_size; + char *new_cset = malloc(new_cset_size + 1); + memcpy(new_cset, *cset + first_cset_size, new_cset_size + 1); + free(*cset); + *cset = new_cset; + return new_cset_size; +} + + +/* The high level log command for hglib API. */ +hg_csetstream_buffer *hg_log(hg_handle *handle, char *option[]) +{ + hg_csetstream_buffer *cbuf = malloc(sizeof(hg_csetstream_buffer)); + cbuf->handle = handle; + + cbuf->command = cmdbuilder("log", option, "--template", CHANGESET, + NULL); + + if(hg_rawcommand(handle, cbuf->command) < 0){ + return NULL; + } + + cbuf->buffer = NULL; + cbuf->buf_size = 0; + cbuf->is_send = 0; + + return cbuf; +} + +/* The cbuf next step. Getting the next changeset. */ +int hg_fetch_cset_entry(hg_csetstream_buffer *cbuf, hg_cset_entry *centry) +{ + hg_header head = hg_head(cbuf->handle); + int exitcode; + char *get_data; + int read_size; + + /* Erase the first cset from cset pointer. + * This cset was already pass to user.*/ + if(cbuf->is_send && cbuf->buf_size){ + cbuf->buf_size = erase_cset(&cbuf->buffer, cbuf->buf_size, + cbuf->first_cset_size); + } + while(head.channel != 'r'){ + /* If there is a cset in cset pointer, then parse it and send + * it to user.*/ + if(cbuf->buffer && strlen(cbuf->buffer + 1) < cbuf->buf_size -1){ + cbuf->first_cset_size = strlen(cbuf->buffer + 1) + 1; + parse_changeset(cbuf->buffer + 1, centry); + cbuf->is_send = 1; + return head.length; + } + else{ + /* Getting the next data from cmdserver and put on the + * end of the cset pointer. */ + get_data = malloc(head.length + 1); + if(read_size = hg_rawread(cbuf->handle, get_data, + head.length), read_size < 0){ + return -1; + } + adding_data(&cbuf->buffer, get_data, cbuf->buf_size, + read_size); + cbuf->buf_size += read_size; + head = hg_head(cbuf->handle); + free(get_data); + } + } + /* After, receiveing the last message, there still could be some + * csets on cset pointer. */ + if(cbuf->buffer && strlen(cbuf->buffer + 1) == cbuf->buf_size -1){ + cbuf->first_cset_size = strlen(cbuf->buffer + 1) + 1; + parse_changeset(cbuf->buffer + 1, centry); + cbuf->buf_size = 0; + cbuf->is_send = 0; + return head.length; + /* Parse first cset from the remaining data. */ + }else if(cbuf->buf_size && cbuf->is_send){ + cbuf->first_cset_size = strlen(cbuf->buffer + 1) + 1; + parse_changeset(cbuf->buffer + 1, centry); + cbuf->is_send = 1; + return head.length; + } + + exitcode = hg_exitcode(cbuf->handle); + free(cbuf->command); + free(cbuf->buffer); + free(cbuf); + return exitcode; + +} diff --git a/client.h b/client.h --- a/client.h +++ b/client.h @@ -69,6 +69,24 @@ char *out_data; } hg_handle; +typedef struct hg_csetstream_buffer{ + hg_handle *handle; + char **command; + char *buffer; + int buf_size; + int is_send; + int first_cset_size; +}hg_csetstream_buffer; + +typedef struct hg_cset_entry{ + char *author; + char *branch; + char *date; + char *desc; + char *node; + char *rev; + char *tags; +}hg_cset_entry; /** * \brief Open the connection with the mercurial command server. @@ -215,4 +233,63 @@ * */ int hg_exitcode(hg_handle *handle); +/** + * \brief hg_log command for hglib API. + * + * It's an advance function to get revision history. It's more like the start + * point of the action, this function will prepare the query question and will + * send it to the cmd-server. + * + * Return the revision history of the specified files or the entire project. + * File history is shown without following rename or copy history of files. + * Use follow with a filename to follow history across renames and copies. + * follow without a filename will only show ancestors or descendants of the + * starting revision. followfirst only follows the first parent of merge + * revisions. + * + * If revrange isn't specified, the default is "tip:0" unless follow is set, + * in which case the working directory parent is used as the starting + * revision. + * + * \param handle The handle of the connection, wherewith I want to communicate + * \param option The option list for mercurial log command. + * \retval hg_csetstream_buffer A pointer to hg_csetstream_buffer structure if + * successful + * \retval NULL to indicate an error, with errno set appropriately. + * + * errno can be: + * - hg_rawcommand errors + * */ +hg_csetstream_buffer *hg_log(hg_handle *handle, char *option[]); + +/** + * \brief The iterator step. Getting the next changeset. + * + * The revision history could have a huge mass of data. You cannot pass the + * entire history in one call, so we use an iterator-like mechanism. Calling + * the hg_fetch_log_entry. The next changeset will be read from cmd-server, + * parse and pass to hg_cset_entry structure. + * The cset_entry structure will handle a changeset with the following string + * fields: + * - rev + * - node + * - tags (space delimited) + * - branch + * - author + * - desc + * + * \param hg_csetstream_buffer The buffer structure to store cset data. + * \param centry The hg_cset_entry structure where the changeset will be stored + * and pass + * \retval number The lenght for the pass changeset. + * \retval exitcode To indicate the end of current_command. + * \retval -1 to indicate an error, with errno set appropriately. + * + * errno can be: + * - EINVAL - Invalid argument (handle it's set to a null pointer) + * - read(2) command errors + * - read_header error + * */ +int hg_fetch_cset_entry(hg_csetstream_buffer *cbuf, hg_cset_entry *centry); + #endif diff --git a/main.c b/main.c new file mode 100644 --- /dev/null +++ b/main.c @@ -0,0 +1,128 @@ +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#include <sys/wait.h> +#include <sys/types.h> +#include <unistd.h> +#include <sys/stat.h> +#include <fcntl.h> + +#include "client.h" +#include "utils.h" + +#define INIT_REPO "init_test_repo" + +/****** Convenience functions. *******/ + +/** + * \brief Create and setup the tmp directory where the acction will happends. + * */ +void setup_tmp() +{ + system("hg init tmp"); + chdir("tmp"); +} + +/** + * \brief Remove the tmp directory and all his files. + * */ +void clean_tmp() +{ + chdir(".."); + system("rm -rf tmp"); +} + +/** + * \brief Fill the current repository with commits for log command. + * */ +void setup_log() +{ + system("touch foo ; hg add foo ; hg commit -m 'foo file'"); + system("echo baloo > foo ; hg commit -m 'baloo text'"); + system("touch voodoo ; hg add voodoo ; hg commit -m voodoo"); + system("echo voodoo > voodoo ; hg commit -m 'voodoo text'"); +} + +/******* Examples using level 1 implementations. ******/ + +/** + * \brief Log command example. + * + * \param handle The handle of the connection, wherewith I want to communicate + * \retval exitcode + * */ +int hg_log_example(hg_handle *handle) +{ + char *option[] = {"-v", NULL}; + int nc; + + /* hg_log function will a iterator. */ + hg_csetstream_buffer *log_iterator = hg_log(handle, option); + + /* you need to alloc some space for log_entry_t structure */ + hg_cset_entry *le = malloc(sizeof(hg_cset_entry)); + + /* Getting the next changeset using the iterator-like mechanism. + Print the changest from log_entry structure.*/ + while(nc = hg_fetch_cset_entry(log_iterator, le), nc > 0){ + printf("rev = %s \n", le->rev); + printf("node = %s \n", le->node); + printf("tags = %s \n", le->tags); + printf("branch = %s \n", le->branch); + printf("author = %s \n", le->author); + printf("date = %s \n", le->date); + printf("desc = %s \n", le->desc); + printf("\n"); + } + + free(le); + /* last call for hg_fetch_log_entry will pass the exitcode */ + return nc; +} + +/** \brief Printing the welcome message. + * + * Will print the options that you will have in this example. + **/ +void print_select_case() +{ + printf("Select test case to run:\n"); + printf("1) log \n"); + printf("\n"); + printf("Your choice: "); +} + + + +/***** Main function. *******/ +/** + * \brief The main function + * */ +int main(int argc, char **argv) +{ + int select_case; + hg_handle *handle; + + print_select_case(); + scanf("%d", &select_case); + if(select_case < 1 || select_case > 1){ + printf("Your choice is not an option...\n"); + return -1; + } + + switch(select_case){ + case 1: + setup_tmp(); + setup_log(); + handle = hg_open(NULL, ""); + + hg_log_example(handle); + + hg_close(&handle); + clean_tmp(); + break; + } + + return 0; +}