Re: [PATCH 1/3] vcs-svn: Introduce svnload, a dumpfile producer
- From
Erik Faye-Lund <kusmabite@gmail.com>
- Date
- Feb 1, 2011, 14:46 UTC
- 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:
Show 20 quoted lines
> 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.
Show 20 quoted lines
> + 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.
Show 10 quoted lines
> +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
Show 5 quoted lines
> +int parse_filemodify_mode(char *val)
> +{
> + char *t;
> +
> + if (!(t = strchr(val, ' '))) goto error;same
Show 23 quoted lines
> +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