Patchwork [2,of,3,RFC,-V3] model (1): c-hglib: hg_log() level 1 function

login
register
mail settings
Submitter Giovanni Gherdovich
Date Sept. 1, 2013, 9:34 a.m.
Message ID <CAJN7YmOb8LGFVHU7ouUj9aOofnyncmbL1yjogfwQQQHpT28+eg@mail.gmail.com>
Download mbox | patch
Permalink /patch/2298/
State Superseded
Headers show

Comments

Giovanni Gherdovich - Sept. 1, 2013, 9:34 a.m.
Hello Iulian,

in your code you use a very incosistent naming for
your datatypes and variables.

Please be more careful in the future.
If you want other people to have a look at your code,
you need to put *a lot more* energy into making
your intentions clear through what you write.
Other reviewers might not try to understand your code
as hard as I do.

I have renamed your structs and variable in a way that
makes more sense to me. Before I can send my comments
on your implementation of hg_log(), please apply the patch below.
Beware, that patch applies on the top of 1/3, 2/3, and 3/3 of your
patch series; what I ask you is to cherry-pick my renames
and apply them in the relevant changeset of your series
(I guess most of them applies to 2/3, but didn't check).

Apply the renaming, and resend everything as V4.
Then I'll get into the details of your algorithm.

What my renaming is about:

(*) Fixing leftovers of a renaming that you have done yourself
    but didn't complete.

(*) changed "hg_cset_iterator" into "hg_logdata_buffer";
    I already observed here http://markmail.org/message/wkpl2vvwrpaniab4
    that your "hg_cset_iterator" is not an iterator.
    It is a buffer where you store what's coming out of cmdserv.
    The real "iterator-like" thing here is read(2);
    or, at a higher level, your "hg_fetch_cset_entry" function.
    Some reading on iterators:
    (*)    http://en.wikipedia.org/wiki/Iterator
    (*)
http://journal.stuffwithstuff.com/2013/01/13/iteration-inside-and-out/

(*) renaming fields of the above struct in something that makes
    more sense to me.

(*) dropping '_t' suffixes; they're reserved for the system implementors
    by the POSIX standard, 2.2.2 at
http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html
    (thanks Triskelios for pointing that out)


My patch follows.

Cheers,
GGhh


-- -- >8 -- -- >8 -- -- cut here -- -- >8 -- -- >8 -- --
# HG changeset patch
# User gghh
# Date 1378024909 -7200
#      Sun Sep 01 10:41:49 2013 +0200
# Node ID acc6e5c04cf0b83acab37cfdc1d4577946deee2e
# Parent  0af244046a1e11621daabb9f8d94e1755d6ce094
renames

+ * The cset_entry structure will handle a changeset with the following
string
  * fields:
  *         - rev
  *         - node
@@ -308,7 +308,7 @@
  *         - desc
  *
  * \param log_iterator The iterator for log command.
- * \param log_entry The hg_log_entry_t structure where the changeset will
be
+ * \param cset_entry The hg_cset_entry structure where the changeset will
be
  *                   pass
  * \retval number The lenght for the pass changeset.
  * \retval exitcode To indicate the end of log_command.
@@ -319,7 +319,7 @@
  *      - read(2) command errors
  *      - read_header error
  * */
-int hg_fetch_cset_entry(hg_cset_iterator *iterator, hg_cset_entry_t
*entry_t);
+int hg_fetch_cset_entry(hg_logdata_buffer *lbuf, hg_cset_entry *centry);

 #endif
 -- -- >8 -- -- >8 -- -- cut here -- -- >8 -- -- >8 -- --

Patch

diff -r 0af244046a1e -r acc6e5c04cf0 client.c
--- a/client.c    Thu Aug 22 17:12:58 2013 +0300
+++ b/client.c    Sun Sep 01 10:41:49 2013 +0200
@@ -47,46 +47,46 @@ 
  * \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 log_entry field to the right position.
- * \param buff The pointer where changeset could be found.
- * \param le   The log_entry_t structure where the changeset will be parse.
+ * 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_log_entry_t *le)
+int parse_changeset(char *cset, hg_cset_entry *ce)
 {
     char *position = cset;
     /* set pointer for revision position */
-    le->rev = cset;
+    ce->rev = cset;
     position = strstr(position, "\n");
     cset[position - cset] = '\0';

     /* set pointer for node position */
-    le->node = position + 1;
+    ce->node = position + 1;
     position = strstr(position + 1, "\n");
     cset[position - cset] = '\0';

     /* set pointer for tag position */
-    le->tags = position + 1;
+    ce->tags = position + 1;
     position = strstr(position + 1, "\n");
     cset[position - cset] = '\0';

     /* set pointer for branch position */
-    le->branch = position + 1;
+    ce->branch = position + 1;
     position = strstr(position + 1, "\n");
     cset[position - cset] = '\0';

     /* set pointer for author position */
-    le->author = position + 1;
+    ce->author = position + 1;
     position = strstr(position + 1, "\n");
     cset[position - cset] = '\0';

     /* set pointer for data position */
-    le->date = position + 1;
+    ce->date = position + 1;
     position = strstr(position + 1, "\n");
     cset[position - cset] = '\0';

     /* set pointer for description position */
-    le->desc = position + 1;
+    ce->desc = position + 1;
     /* */
     return 0;
 }
@@ -177,82 +177,82 @@ 
 }

 /* The high level log command for hglib API. */
-hg_cset_iterator *hg_log(hg_handle *handle, char *option[])
+hg_logadata_buffer *hg_log(hg_handle *handle, char *option[])
 {
-    hg_cset_iterator *iterator = malloc(sizeof(hg_cset_iterator));
+    hg_logadata_buffer *lbuf = malloc(sizeof(hg_logadata_buffer));

-    iterator->handle = handle;
-        iterator->command = cmdbuilder("log", option, CHANGESET);
+    lbuf->handle = handle;
+        lbuf->command = cmdbuilder("log", option, CHANGESET);

-    if(hg_rawcommand(handle, iterator->command) < 0){
+    if(hg_rawcommand(handle, lbuf->command) < 0){
         return NULL;
     }

-    iterator->cset = NULL;
-    iterator->cset_size = 0;
-    iterator->cset_send = 0;
+    lbuf->buffer = NULL;
+    lbuf->buf_size = 0;
+    lbuf->is_sent = 0;

-    return iterator;
+    return lbuf;
 }

 /* The iterator next step. Getting the next changeset. */
-int hg_fetch_cset_entry(hg_cset_iterator *iter, hg_cset_entry_t *le)
+int hg_fetch_cset_entry(hg_logadata_buffer *lbuf, hg_cset_entry *centry)
 {
-    hg_header head = hg_head(iter->handle);
+    hg_header head = hg_head(lbuf->handle);
     int exitcode;
     char *get_data;
     int read_size;

     /* Erase the first cset from cset pointer.
      * This cset was already pass to user.*/
-    if(iter->cset_send && iter->cset_size){
-        iter->cset_size = erase_cset(&iter->cset, iter->cset_size,
-                        iter->top_cset_size);
-        iter->cset_send = 0;
+    if(lbuf->is_sent && lbuf->buf_size){
+        lbuf->buf_size = erase_cset(&lbuf->cset, lbuf->buf_size,
+                        lbuf->first_cset_size);
+        lbuf->is_sent = 0;
     }
     while(head.channel != 'r'){
         /* If there is a cset in cset pointer, then parse it and send
          * it to user.*/
-        if(iter->cset && strlen(iter->cset + 1) < iter->cset_size -1){
-            iter->top_cset_size = strlen(iter->cset + 1) + 1;
-            parse_changeset(iter->cset + 1, le);
-            iter->cset_send = 1;
+        if(lbuf->buffer && strlen(lbuf->buffer + 1) < lbuf->buf_size -1){
+            lbuf->first_cset_size = strlen(lbuf->buffer + 1) + 1;
+            parse_buffer(lbuf->buffer + 1, centry);
+            lbuf->is_sent = 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(iter->handle, get_data,
+            if(read_size = hg_rawread(lbuf->handle, get_data,
                         head.length), read_size < 0){
                 return -1;
             }
-            adding_data(&iter->cset, get_data, iter->cset_size,
+            adding_data(&lbuf->cset, get_data, lbuf->buf_size,
                                 read_size);
-            iter->cset_size += read_size;
+            lbuf->buf_size += read_size;
             free(get_data);
         }
     }
     /* After, receiveing the last message, there still could be some
      * csets on cset pointer. */
-    if(iter->cset && strlen(iter->cset + 1) == iter->cset_size -1){
-        iter->top_cset_size = strlen(iter->cset + 1) + 1;
-        parse_changeset(iter->cset + 1, le);
-        iter->cset_size = 0;
-        iter->cset_send = 1;
+    if(lbuf->buffer && strlen(lbuf->buffer + 1) == lbuf->buf_size -1){
+        lbuf->first_cset_size = strlen(lbuf->buffer + 1) + 1;
+        parse_buffer(lbuf->buffer + 1, centry);
+        lbuf->buf_size = 0;
+        lbuf->is_sent = 1;
         return head.length;
     /* Parse first cset from the remaining data. */
-    }else if(iter->cset_size && iter->cset_send){
-        iter->top_cset_size = strlen(iter->cset + 1) + 1;
-        parse_changeset(iter->cset + 1, le);
-        iter->cset_send = 1;
+    }else if(lbuf->buf_size && lbuf->is_sent){
+        lbuf->first_cset_size = strlen(lbuf->buffer + 1) + 1;
+        parse_buffer(lbuf->buffer + 1, centry);
+        lbuf->is_sent = 1;
         return head.length;
     }

-    exitcode = hg_exitcode(iter->handle);
-    free(iter->command);
-    free(iter->cset);
-    free(iter);
+    exitcode = hg_exitcode(lbuf->handle);
+    free(lbuf->command);
+    free(lbuf->buffer);
+    free(lbuf);
     return exitcode;

 }
diff -r 0af244046a1e -r acc6e5c04cf0 client.h
--- a/client.h    Thu Aug 22 17:12:58 2013 +0300
+++ b/client.h    Sun Sep 01 10:41:49 2013 +0200
@@ -69,16 +69,16 @@ 
     char *debug_data;
 } hg_handle;

-typedef struct hg_cset_iterator{
+typedef struct hg_logdata_buffer{
     hg_handle *handle;
     char **command;
-    char *changeset;
-    int cset_size;
-    int cset_send;
-    int top_cset_size;
-}hg_cset_iterator;
+    char *buffer;
+    int buf_size;
+    int is_sent;
+    int first_cset_size;
+}hg_logdata_buffer;

-typedef struct hg_cset_entry_t{
+typedef struct hg_cset_entry{
     char *author;
     char *branch;
     char *date;
@@ -86,7 +86,7 @@ 
     char *node;
     char *rev;
     char *tags;
-}hg_cset_entry_t;
+}hg_cset_entry;

 /**
  * \brief Open the connection with the mercurial command server.
@@ -289,16 +289,16 @@ 
  * errno can be:
  *      - hg_rawcommand errors
  * */
-hg_cset_iterator *hg_log(hg_handle *handle, char *option[]);
+hg_logdata_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 can 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_log_entry_t structure.
- * The log_entry structure will handle a changeset with the following
string
+ * hg_fetch_cset_entry, the next changeset will be read from cmd-server,
parse
+ * and pass to hg_cset_entry structure.