From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Tue, 01 Feb 2011 14:46:25 GMT
Subject: Re: [PATCH 1/3] vcs-svn: Introduce svnload, a dumpfile producer
Message-ID: <AANLkTikUUojgj_H0r0L=XBi7H714xUvAS=vPsgMRNNjr@mail.gmail.com>
In-Reply-To: <1296570403-9082-2-git-send-email-artagnon@gmail.com>

A very superficial review, because I don't have much time, and don't
know the surrounding code well. Sorry about that.

On Tue, Feb 1, 2011 at 3:26 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> diff --git a/vcs-svn/dir_cache.c b/vcs-svn/dir_cache.c
> new file mode 100644
> index 0000000..9a608ce
> --- /dev/null
> +++ b/vcs-svn/dir_cache.c
> @@ -0,0 +1,40 @@
> +/*
> + * Licensed under a two-clause BSD-style license.
> + * See LICENSE for details.
> + */
> +
> +#include "git-compat-util.h"
> +#include "string-list.h"
> +#include "line_buffer.h"
> +#include "dump_export.h"
> +
> +static struct string_list dirents = STRING_LIST_INIT_DUP;
> +static struct string_list_item *dir = NULL;
> +
> +void dir_cache_add(const char *path, enum node_kind kind) {

Style: we put the opening bracket of functions on the next line.

> +       dir = string_list_insert(&dirents, path);
> +       dir->util = malloc(sizeof(enum node_kind));
> +       *((enum node_kind *)(dir->util)) = kind;

Unchecked malloc; perhaps you should use xmalloc instead?

> +}
> +
> +void dir_cache_remove(const char *path) {

Same style-violation as above.

> +       dir = string_list_lookup(&dirents, path);
> +       if (dir)
> +               *((enum node_kind *)(dir->util)) = NODE_KIND_UNKNOWN;
> +}
> +
> +enum node_kind dir_cache_lookup(const char *path) {
> +       dir = string_list_lookup(&dirents, path);
> +       if (dir)
> +               return *((enum node_kind *)(dir->util));
> +       else
> +               return NODE_KIND_UNKNOWN;
> +}
> +
> +void dir_cache_init() {
> +       return;
> +}
> +
> +void dir_cache_deinit() {
> +       string_list_clear(&dirents, 1);
> +}

Three more.

> +static void populate_revprops(struct strbuf *props, size_t author_len,
> +                       const char *author, size_t log_len, const char *log,
> +                       size_t date_len, const char *date)
> +{
> +       strbuf_reset(props);
> +       strbuf_addf(props, "K 10\nsvn:author\nV %lu\n%s\n", author_len, author);
> +       strbuf_addf(props, "K 7\nsvn:log\nV %lu\n%s\n", log_len, log);
> +       if (date_len)
> +               /* SVN doesn't like an empty svn:date value */
> +               strbuf_addf(props, "K 8\nsvn:date\nV %lu\n%s\n", date_len, date);

Perhaps a scope around here will make this a bit easier to read. At
first glance it looked like a missing scope to me, due to the indented
comment...

> +       if (!val) die("Malformed author line");
> +       if (!(tz_off = strrchr(val, ' '))) goto error;
> +       *tz_off++ = '\0';
> +       if (!(t = strrchr(val, ' '))) goto error;

style: use
"if (x)
	do_stuff();"
instead of
"if (x) do_stuff();"


> +       *(t - 1) = '\0'; /* Ignore '>' from email */
> +       t++;
> +       tz_off_buf = atoi(tz_off);
> +       if (tz_off_buf > 1200 || tz_off_buf  < -1200) goto error;

same

> +       tm_time = time_to_tm(strtoul(t, NULL, 10), tz_off_buf);
> +       strftime(time_buf, SVN_DATE_LEN + 1, SVN_DATE_FORMAT, tm_time);
> +       strbuf_add(date, time_buf, SVN_DATE_LEN);
> +       if (!(t = strchr(val, '<'))) goto error;

same

> +int parse_filemodify_mode(char *val)
> +{
> +       char *t;
> +
> +       if (!(t = strchr(val, ' '))) goto error;

same

> +void svnload_read(void)
> +{
> +       char *t, *val;
> +       int len;
> +
> +       while ((t = buffer_read_line(&input))) {
> +               if ((val = strchr(t, ' ')))
> +                       *val++ = '\0';
> +               len = (val ? val - t - 1 : strlen(t));
> +
> +               switch (len) {
> +               case 1:
> +                       if (!memcmp(t, "D", 1)) {
> +                               node_ctx.action = NODE_ACTION_DELETE;
> +                       } else if (!memcmp(t, "C", 1)) {
> +                               node_ctx.action = NODE_ACTION_ADD;
> +                       } else if (!memcmp(t, "R", 1)) {
> +                               node_ctx.action = NODE_ACTION_REPLACE;
> +                       } else if (!memcmp(t, "M", 1)) {
> +                               if (!val) goto error;
> +                               node_ctx.action = NODE_ACTION_CHANGE;
> +                               val += parse_filemodify_mode(val);
> +                               if (!(t = strchr(val, ' '))) goto error;

same