[PATCH 2 of 3 RFC -V3] model (1): c-hglib: hg_log() level 1 function
Giovanni Gherdovich
g.gherdovich at gmail.com
Sun Sep 1 04:34:59 CDT 2013
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
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.
+ * 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 -- --
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20130901/724a1baa/attachment.html>
More information about the Mercurial-devel
mailing list