{"thread":{"id":"24349","subject":"[PATCH v2] Add svnrdump","startedAt":"2010-07-09T14:29:10Z","lastAt":"2010-07-21T19:03:24Z","messageCount":19,"participants":["Ramkumar Ramachandra","Stefan Sperling","C. Michael Pilato","Jonathan Nieder","Bert Huijben","Daniel Shahaf"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"145222","messageId":"20100709142910.GB20383@debian","threadId":"24349","inReplyTo":null,"subject":"[PATCH v2] Add svnrdump","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2010-07-09T14:29:10Z","receivedAt":"2010-07-09T14:29:10Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nAlong with the changes suggested by Bert and Daniel, this new version\nincludes a few small bugfixes and feature additions contributed by\nDavid and Will, among others. Unfortunately, a diff of the changes\nmade is not available due to whitespace/ style conversion issues:\nplease check the recent commits on my GitHub repository for a summary\nof these changes: ra-svn branch of\nhttp://github.com/artagnon/svn-dump-fast-export\n\nThanks.\n\n-- Ram\n\n------------------------8<---------------->8-----------------------------------\nIndex: svnrdump/dump_editor.c\n===================================================================\n--- svnrdump/dump_editor.c\t(revision 0)\n+++ svnrdump/dump_editor.c\t(working copy)\n@@ -0,0 +1,690 @@\n+/*\n+ * ====================================================================\n+ *    Licensed to the Apache Software Foundation (ASF) under one\n+ *    or more contributor license agreements.  See the NOTICE file\n+ *    distributed with this work for additional information\n+ *    regarding copyright ownership.  The ASF licenses this file\n+ *    to you under the Apache License, Version 2.0 (the\n+ *    \"License\"); you may not use this file except in compliance\n+ *    with the License.  You may obtain a copy of the License at\n+ *\n+ *      http://www.apache.org/licenses/LICENSE-2.0\n+ *\n+ *    Unless required by applicable law or agreed to in writing,\n+ *    software distributed under the License is distributed on an\n+ *    \"AS IS\" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY\n+ *    KIND, either express or implied.  See the License for the\n+ *    specific language governing permissions and limitations\n+ *    under the License.\n+ * ====================================================================\n+ */\n+\n+#include \"svn_pools.h\"\n+#include \"svn_repos.h\"\n+#include \"svn_path.h\"\n+#include \"svn_props.h\"\n+#include \"svn_dirent_uri.h\"\n+\n+#include \"svnrdump.h\"\n+#include \"dump_editor.h\"\n+\n+#define ARE_VALID_COPY_ARGS(p,r) ((p) && SVN_IS_VALID_REVNUM(r))\n+\n+/* Make a directory baton to represent the directory was path\n+   (relative to EDIT_BATON's path) is PATH.\n+\n+   CMP_PATH/CMP_REV are the path/revision against which this directory\n+   should be compared for changes.  If either is omitted (NULL for the\n+   path, SVN_INVALID_REVNUM for the rev), just compare this directory\n+   PATH against itself in the previous revision.\n+\n+   PARENT_DIR_BATON is the directory baton of this directory's parent,\n+   or NULL if this is the top-level directory of the edit.  ADDED\n+   indicated if this directory is newly added in this revision.\n+   Perform all allocations in POOL.  */\n+static struct dir_baton *\n+make_dir_baton(const char *path,\n+               const char *cmp_path,\n+               svn_revnum_t cmp_rev,\n+               void *edit_baton,\n+               void *parent_dir_baton,\n+               svn_boolean_t added,\n+               apr_pool_t *pool)\n+{\n+  struct dump_edit_baton *eb = edit_baton;\n+  struct dir_baton *pb = parent_dir_baton;\n+  struct dir_baton *new_db = apr_pcalloc(pool, sizeof(*new_db));\n+  const char *full_path;\n+  apr_array_header_t *compose_path = apr_array_make(pool, 2,\n+\t\t\t\t\t\t    sizeof(const char *));\n+\n+  /* A path relative to nothing?  I don't think so. */\n+  SVN_ERR_ASSERT_NO_RETURN(!path || pb);\n+\n+  /* Construct the full path of this node. */\n+  if (pb) {\n+    APR_ARRAY_PUSH(compose_path, const char *) = \"/\";\n+    APR_ARRAY_PUSH(compose_path, const char *) = path;\n+    full_path = svn_path_compose(compose_path, pool);\n+  }\n+  else\n+    full_path = apr_pstrdup(pool, \"/\");\n+\n+  /* Remove leading slashes from copyfrom paths. */\n+  if (cmp_path)\n+    cmp_path = ((*cmp_path == '/') ? cmp_path + 1 : cmp_path);\n+\n+  new_db->eb = eb;\n+  new_db->parent_dir_baton = pb;\n+  new_db->path = full_path;\n+  new_db->cmp_path = cmp_path ? apr_pstrdup(pool, cmp_path) : NULL;\n+  new_db->cmp_rev = cmp_rev;\n+  new_db->added = added;\n+  new_db->written_out = FALSE;\n+  new_db->deleted_entries = apr_hash_make(pool);\n+  new_db->pool = pool;\n+\n+  return new_db;\n+}\n+/*\n+ * Write out a node record for PATH of type KIND under EB->FS_ROOT.\n+ * ACTION describes what is happening to the node (see enum svn_node_action).\n+ * Write record to writable EB->STREAM, using EB->BUFFER to write in chunks.\n+ *\n+ * If the node was itself copied, IS_COPY is TRUE and the\n+ * path/revision of the copy source are in CMP_PATH/CMP_REV.  If\n+ * IS_COPY is FALSE, yet CMP_PATH/CMP_REV are valid, this node is part\n+ * of a copied subtree.\n+ */\n+static svn_error_t *\n+dump_node(struct dump_edit_baton *eb,\n+          const char *path,    /* an absolute path. */\n+          svn_node_kind_t kind,\n+          enum svn_node_action action,\n+          const char *cmp_path,\n+          svn_revnum_t cmp_rev,\n+          apr_pool_t *pool)\n+{\n+  /* Write out metadata headers for this file node. */\n+  SVN_ERR(svn_stream_printf(eb->stream, pool,\n+          SVN_REPOS_DUMPFILE_NODE_PATH \": %s\\n\",\n+          (*path == '/') ? path + 1 : path));\n+\n+  if (kind == svn_node_file)\n+    SVN_ERR(svn_stream_printf(eb->stream, pool,\n+                              SVN_REPOS_DUMPFILE_NODE_KIND \": file\\n\"));\n+  else if (kind == svn_node_dir)\n+    SVN_ERR(svn_stream_printf(eb->stream, pool,\n+                              SVN_REPOS_DUMPFILE_NODE_KIND \": dir\\n\"));\n+\n+  /* Remove leading slashes from copyfrom paths. */\n+  if (cmp_path)\n+    cmp_path = ((*cmp_path == '/') ? cmp_path + 1 : cmp_path);\n+\n+  switch (action) {\n+    /* Appropriately handle the four svn_node_action actions */\n+\n+  case svn_node_action_change:\n+    SVN_ERR(svn_stream_printf(eb->stream, pool,\n+                              SVN_REPOS_DUMPFILE_NODE_ACTION\n+                              \": change\\n\"));\n+    break;\n+\n+  case svn_node_action_replace:\n+    if (!eb->is_copy) {\n+      /* a simple delete+add, implied by a single 'replace' action. */\n+      SVN_ERR(svn_stream_printf(eb->stream, pool,\n+                                SVN_REPOS_DUMPFILE_NODE_ACTION\n+                                \": replace\\n\"));\n+\n+      eb->dump_props_pending = TRUE;\n+      break;\n+    }\n+    /* More complex case: eb->is_copy is true, and\n+       cmp_path/ cmp_rev are present: delete the original,\n+       and then re-add it */\n+\n+    /* the path & kind headers have already been printed;  just\n+       add a delete action, and end the current record.*/\n+    SVN_ERR(svn_stream_printf(eb->stream, pool,\n+                              SVN_REPOS_DUMPFILE_NODE_ACTION\n+                              \": delete\\n\\n\"));\n+\n+    /* recurse:  print an additional add-with-history record. */\n+    SVN_ERR(dump_node(eb, path, kind, svn_node_action_add,\n+                      cmp_path, cmp_rev, pool));\n+\n+    /* we can leave this routine quietly now, don't need to dump\n+       any content;  that was already done in the second record. */\n+    eb->must_dump_props = FALSE;\n+    eb->is_copy = FALSE;\n+    break;\n+\n+  case svn_node_action_delete:\n+    SVN_ERR(svn_stream_printf(eb->stream, pool,\n+                              SVN_REPOS_DUMPFILE_NODE_ACTION\n+                              \": delete\\n\"));\n+\n+    /* we can leave this routine quietly now, don't need to dump\n+       any content. */\n+    SVN_ERR(svn_stream_printf(eb->stream, pool, \"\\n\\n\"));\n+    eb->must_dump_props = FALSE;\n+    break;\n+\n+  case svn_node_action_add:\n+    SVN_ERR(svn_stream_printf(eb->stream, pool,\n+                              SVN_REPOS_DUMPFILE_NODE_ACTION \": add\\n\"));\n+\n+    if (!eb->is_copy) {\n+      /* eb->dump_props_pending for files is handled in\n+         close_file which is called immediately.\n+         However, directories are not closed until\n+         all the work inside them have been done;\n+         eb->dump_props_pending for directories is\n+         handled in all the functions that can\n+         possibly be called after add_directory:\n+         add_directory, open_directory,\n+         delete_entry, close_directory, add_file,\n+         open_file and change_dir_prop;\n+         change_dir_prop is a special case\n+         ofcourse */\n+\n+      eb->dump_props_pending = TRUE;\n+      break;\n+    }\n+\n+    SVN_ERR(svn_stream_printf(eb->stream, pool,\n+                              SVN_REPOS_DUMPFILE_NODE_COPYFROM_REV\n+                              \": %ld\\n\"\n+                              SVN_REPOS_DUMPFILE_NODE_COPYFROM_PATH\n+                              \": %s\\n\",\n+                              cmp_rev, cmp_path));\n+\n+    /* Dump the text only if apply_textdelta sets\n+       eb->must_dump_text */\n+\n+    /* UGLY hack: If a directory was copied from a\n+       previous revision, nothing else can be done, and\n+       close_file won't be called to write two blank\n+       lines; write them here */\n+    if (kind == svn_node_dir)\n+      SVN_ERR(svn_stream_printf(eb->stream, pool, \"\\n\\n\"));\n+\n+    eb->is_copy = FALSE;\n+\n+    break;\n+  }\n+\n+  /* Dump property headers */\n+  SVN_ERR(dump_props(eb, &(eb->must_dump_props), FALSE, pool));\n+\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *open_root(void *edit_baton,\n+\t\t\t      svn_revnum_t base_revision,\n+\t\t\t      apr_pool_t *pool,\n+\t\t\t      void **root_baton)\n+{\n+  /* Allocate a special pool for the edit_baton to avoid pool\n+     lifetime issues */\n+  struct dump_edit_baton *eb = edit_baton;\n+  eb->pool = svn_pool_create(pool);\n+  eb->properties = apr_hash_make(eb->pool);\n+  eb->del_properties = apr_hash_make(eb->pool);\n+  eb->propstring = svn_stringbuf_create(\"\", eb->pool);\n+  eb->is_copy = FALSE;\n+\n+  *root_baton = make_dir_baton(NULL, NULL, SVN_INVALID_REVNUM,\n+                               edit_baton, NULL, FALSE, pool);\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+delete_entry(const char *path,\n+             svn_revnum_t revision,\n+             void *parent_baton,\n+             apr_pool_t *pool)\n+{\n+  struct dir_baton *pb = parent_baton;\n+  const char *mypath = apr_pstrdup(pb->pool, path);\n+\n+  /* Some pending properties to dump? */\n+  SVN_ERR(dump_props(pb->eb, &(pb->eb->dump_props_pending), TRUE, pool));\n+\n+  /* remember this path needs to be deleted */\n+  apr_hash_set(pb->deleted_entries, mypath, APR_HASH_KEY_STRING, pb);\n+\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+add_directory(const char *path,\n+              void *parent_baton,\n+              const char *copyfrom_path,\n+              svn_revnum_t copyfrom_rev,\n+              apr_pool_t *pool,\n+              void **child_baton)\n+{\n+  struct dir_baton *pb = parent_baton;\n+  void *val;\n+  struct dir_baton *new_db\n+    = make_dir_baton(path, copyfrom_path, copyfrom_rev, pb->eb, pb, TRUE, pool);\n+\n+  /* Some pending properties to dump? */\n+  SVN_ERR(dump_props(pb->eb, &(pb->eb->dump_props_pending), TRUE, pool));\n+\n+  /* This might be a replacement -- is the path already deleted? */\n+  val = apr_hash_get(pb->deleted_entries, path, APR_HASH_KEY_STRING);\n+\n+  /* Detect an add-with-history */\n+  pb->eb->is_copy = ARE_VALID_COPY_ARGS(copyfrom_path, copyfrom_rev);\n+\n+  /* Dump the node */\n+  SVN_ERR(dump_node(pb->eb, path,\n+                    svn_node_dir,\n+                    val ? svn_node_action_replace : svn_node_action_add,\n+                    pb->eb->is_copy ? copyfrom_path : NULL,\n+                    pb->eb->is_copy ? copyfrom_rev : SVN_INVALID_REVNUM,\n+                    pool));\n+\n+  if (val)\n+    /* Delete the path, it's now been dumped */\n+    apr_hash_set(pb->deleted_entries, path, APR_HASH_KEY_STRING, NULL);\n+\n+  new_db->written_out = TRUE;\n+\n+  *child_baton = new_db;\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+open_directory(const char *path,\n+               void *parent_baton,\n+               svn_revnum_t base_revision,\n+               apr_pool_t *pool,\n+               void **child_baton)\n+{\n+  struct dir_baton *pb = parent_baton;\n+  struct dir_baton *new_db;\n+  const char *cmp_path = NULL;\n+  svn_revnum_t cmp_rev = SVN_INVALID_REVNUM;\n+  apr_array_header_t *compose_path = apr_array_make(pool, 2,\n+\t\t\t\t\t\t    sizeof(const char *));\n+\n+  /* Some pending properties to dump? */\n+  SVN_ERR(dump_props(pb->eb, &(pb->eb->dump_props_pending), TRUE, pool));\n+\n+  /* If the parent directory has explicit comparison path and rev,\n+     record the same for this one. */\n+  if (pb && ARE_VALID_COPY_ARGS(pb->cmp_path, pb->cmp_rev)) {\n+    APR_ARRAY_PUSH(compose_path, const char *) = pb->cmp_path;\n+    APR_ARRAY_PUSH(compose_path, const char *) =\n+\t    svn_relpath_basename(path, pool);\n+    cmp_path = svn_path_compose(compose_path, pool);\n+    cmp_rev = pb->cmp_rev;\n+  }\n+\n+  new_db = make_dir_baton(path, cmp_path, cmp_rev, pb->eb, pb, FALSE, pool);\n+  *child_baton = new_db;\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+close_directory(void *dir_baton,\n+                apr_pool_t *pool)\n+{\n+  struct dir_baton *db = dir_baton;\n+  struct dump_edit_baton *eb = db->eb;\n+  apr_hash_index_t *hi;\n+  apr_pool_t *subpool = svn_pool_create(pool);\n+\n+  /* Some pending properties to dump? */\n+  SVN_ERR(dump_props(eb, &(eb->dump_props_pending), TRUE, pool));\n+\n+  /* Dump the directory entries */\n+  for (hi = apr_hash_first(pool, db->deleted_entries); hi;\n+       hi = apr_hash_next(hi)) {\n+    const void *key;\n+    const char *path;\n+    apr_hash_this(hi, &key, NULL, NULL);\n+    path = key;\n+\n+    svn_pool_clear(subpool);\n+\n+    SVN_ERR(dump_node(db->eb, path,\n+                      svn_node_unknown, svn_node_action_delete,\n+                      NULL, SVN_INVALID_REVNUM, subpool));\n+  }\n+\n+  svn_pool_destroy(subpool);\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+add_file(const char *path,\n+         void *parent_baton,\n+         const char *copyfrom_path,\n+         svn_revnum_t copyfrom_rev,\n+         apr_pool_t *pool,\n+         void **file_baton)\n+{\n+  struct dir_baton *pb = parent_baton;\n+  void *val;\n+\n+  /* Some pending properties to dump? */\n+  SVN_ERR(dump_props(pb->eb, &(pb->eb->dump_props_pending), TRUE, pool));\n+\n+  /* This might be a replacement -- is the path already deleted? */\n+  val = apr_hash_get(pb->deleted_entries, path, APR_HASH_KEY_STRING);\n+\n+  /* Detect add-with-history. */\n+  pb->eb->is_copy = ARE_VALID_COPY_ARGS(copyfrom_path, copyfrom_rev);\n+\n+  /* Dump the node. */\n+  SVN_ERR(dump_node(pb->eb, path,\n+                    svn_node_file,\n+                    val ? svn_node_action_replace : svn_node_action_add,\n+                    pb->eb->is_copy ? copyfrom_path : NULL,\n+                    pb->eb->is_copy ? copyfrom_rev : SVN_INVALID_REVNUM,\n+                    pool));\n+\n+  if (val)\n+    /* delete the path, it's now been dumped. */\n+    apr_hash_set(pb->deleted_entries, path, APR_HASH_KEY_STRING, NULL);\n+\n+  /* Build a nice file baton to pass to change_file_prop and apply_textdelta */\n+  pb->eb->changed_path = path;\n+  *file_baton = pb->eb;\n+\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+open_file(const char *path,\n+          void *parent_baton,\n+          svn_revnum_t ancestor_revision,\n+          apr_pool_t *pool,\n+          void **file_baton)\n+{\n+  struct dir_baton *pb = parent_baton;\n+  const char *cmp_path = NULL;\n+  svn_revnum_t cmp_rev = SVN_INVALID_REVNUM;\n+\n+  /* Some pending properties to dump? */\n+  SVN_ERR(dump_props(pb->eb, &(pb->eb->dump_props_pending), TRUE, pool));\n+\n+  apr_array_header_t *compose_path = apr_array_make(pool, 2,\n+\t\t\t\t\t\t    sizeof(const char *));\n+  /* If the parent directory has explicit comparison path and rev,\n+     record the same for this one. */\n+  if (pb && ARE_VALID_COPY_ARGS(pb->cmp_path, pb->cmp_rev)) {\n+    APR_ARRAY_PUSH(compose_path, const char *) = pb->cmp_path;\n+    APR_ARRAY_PUSH(compose_path, const char *) =\n+\t    svn_relpath_basename(path, pool);\n+    cmp_path = svn_path_compose(compose_path, pool);\n+    cmp_rev = pb->cmp_rev;\n+  }\n+\n+  SVN_ERR(dump_node(pb->eb, path,\n+                    svn_node_file, svn_node_action_change,\n+                    cmp_path, cmp_rev, pool));\n+\n+  /* Build a nice file baton to pass to change_file_prop and apply_textdelta */\n+  pb->eb->changed_path = path;\n+  *file_baton = pb->eb;\n+\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+change_dir_prop(void *parent_baton,\n+                const char *name,\n+                const svn_string_t *value,\n+                apr_pool_t *pool)\n+{\n+  struct dir_baton *db = parent_baton;\n+\n+  if (svn_property_kind(NULL, name) != svn_prop_regular_kind)\n+    return SVN_NO_ERROR;\n+\n+  value ? apr_hash_set(db->eb->properties, apr_pstrdup(pool, name),\n+                       APR_HASH_KEY_STRING, svn_string_dup(value, pool)) :\n+    apr_hash_set(db->eb->del_properties, apr_pstrdup(pool, name),\n+                 APR_HASH_KEY_STRING, (void *)0x1);\n+\n+  /* This function is what distinguishes between a directory that is\n+     opened to merely get somewhere, vs. one that is opened because it\n+     actually changed by itself  */\n+  if (! db->written_out) {\n+    /* If eb->dump_props_pending was set, it means that the\n+       node information corresponding to add_directory has already\n+       been written; just don't unset it and dump_node will dump\n+       the properties before doing anything else. If it wasn't\n+       set, node information hasn't been written yet: so dump the\n+       node itself before dumping the props */\n+\n+    SVN_ERR(dump_node(db->eb, db->path,\n+                      svn_node_dir, svn_node_action_change,\n+                      db->cmp_path, db->cmp_rev, pool));\n+\n+    SVN_ERR(dump_props(db->eb, NULL, TRUE, pool));\n+    db->written_out = TRUE;\n+  }\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+change_file_prop(void *file_baton,\n+                 const char *name,\n+                 const svn_string_t *value,\n+                 apr_pool_t *pool)\n+{\n+  struct dump_edit_baton *eb = file_baton;\n+\n+  if (svn_property_kind(NULL, name) != svn_prop_regular_kind)\n+    return SVN_NO_ERROR;\n+\n+  apr_hash_set(eb->properties, apr_pstrdup(pool, name),\n+               APR_HASH_KEY_STRING, value ?\n+               svn_string_dup(value, pool): (void *)0x1);\n+  /* Dump the property headers and wait; close_file might need\n+     to write text headers too depending on whether\n+     apply_textdelta is called */\n+  eb->dump_props_pending = TRUE;\n+\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+window_handler(svn_txdelta_window_t *window, void *baton)\n+{\n+  struct handler_baton *hb = baton;\n+  struct dump_edit_baton *eb = hb->eb;\n+  static svn_error_t *err;\n+\n+  err = hb->apply_handler(window, hb->apply_baton);\n+  if (window != NULL && !err)\n+    return SVN_NO_ERROR;\n+\n+  if (err)\n+    SVN_ERR(err);\n+\n+  /* Write information about the filepath to hb->eb */\n+  eb->temp_filepath = apr_pstrdup(eb->pool,\n+          hb->temp_filepath);\n+\n+  /* Cleanup */\n+  SVN_ERR(svn_io_file_close(hb->temp_file, hb->pool));\n+  SVN_ERR(svn_stream_close(hb->temp_filestream));\n+  svn_pool_destroy(hb->pool);\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+apply_textdelta(void *file_baton, const char *base_checksum,\n+                apr_pool_t *pool,\n+                svn_txdelta_window_handler_t *handler,\n+                void **handler_baton)\n+{\n+  struct dump_edit_baton *eb = file_baton;\n+  apr_status_t apr_err;\n+  const char *tempdir;\n+\n+  /* Custom handler_baton allocated in a separate pool */\n+  apr_pool_t *handler_pool = svn_pool_create(pool);\n+  struct handler_baton *hb = apr_pcalloc(handler_pool, sizeof(*hb));\n+  hb->pool = handler_pool;\n+  hb->eb = eb;\n+\n+  /* Use a temporary file to measure the text-content-length */\n+  SVN_ERR(svn_io_temp_dir(&tempdir, hb->pool));\n+\n+  hb->temp_filepath = svn_dirent_join(tempdir, \"XXXXXX\", hb->pool);\n+  apr_err = apr_file_mktemp(&(hb->temp_file), hb->temp_filepath,\n+          APR_CREATE | APR_READ | APR_WRITE | APR_EXCL,\n+          hb->pool);\n+  if (apr_err != APR_SUCCESS)\n+    SVN_ERR(svn_error_wrap_apr(apr_err, NULL));\n+\n+  hb->temp_filestream = svn_stream_from_aprfile2(hb->temp_file, TRUE, hb->pool);\n+\n+  /* Prepare to write the delta to the temporary file */\n+  svn_txdelta_to_svndiff2(&(hb->apply_handler), &(hb->apply_baton),\n+                          hb->temp_filestream, 0, hb->pool);\n+  eb->must_dump_text = TRUE;\n+\n+  /* The actual writing takes place when this function has finished */\n+  /* Set the handler and handler_baton */\n+  *handler = window_handler;\n+  *handler_baton = hb;\n+\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+close_file(void *file_baton,\n+           const char *text_checksum,\n+           apr_pool_t *pool)\n+{\n+  struct dump_edit_baton *eb = file_baton;\n+  apr_file_t *temp_file;\n+  svn_stream_t *temp_filestream;\n+  apr_finfo_t *info = apr_pcalloc(pool, sizeof(apr_finfo_t));\n+\n+  /* We didn't write the property headers because we were\n+     waiting for file_prop_change; write them now */\n+  SVN_ERR(dump_props(eb, &(eb->dump_props_pending), FALSE, pool));\n+\n+  /* The prop headers have already been dumped in dump_node */\n+  /* Dump the text headers */\n+  if (eb->must_dump_text) {\n+    /* text-delta header */\n+    SVN_ERR(svn_stream_printf(eb->stream, pool,\n+            SVN_REPOS_DUMPFILE_TEXT_DELTA\n+            \": true\\n\"));\n+\n+    /* Measure the length */\n+    SVN_ERR(svn_io_stat(info, eb->temp_filepath, APR_FINFO_SIZE, pool));\n+\n+    /* text-content-length header */\n+    SVN_ERR(svn_stream_printf(eb->stream, pool,\n+            SVN_REPOS_DUMPFILE_TEXT_CONTENT_LENGTH\n+            \": %lu\\n\",\n+            (unsigned long)info->size));\n+    /* text-content-md5 header */\n+    SVN_ERR(svn_stream_printf(eb->stream, pool,\n+            SVN_REPOS_DUMPFILE_TEXT_CONTENT_MD5\n+            \": %s\\n\",\n+            text_checksum));\n+  }\n+\n+  /* content-length header: if both text and props are absent,\n+     skip this block */\n+  if (eb->must_dump_props || eb->dump_props_pending)\n+    SVN_ERR(svn_stream_printf(eb->stream, pool,\n+            SVN_REPOS_DUMPFILE_CONTENT_LENGTH\n+            \": %ld\\n\\n\",\n+            (unsigned long)info->size + eb->propstring->len));\n+  else if (eb->must_dump_text)\n+    SVN_ERR(svn_stream_printf(eb->stream, pool,\n+            SVN_REPOS_DUMPFILE_CONTENT_LENGTH\n+            \": %ld\\n\\n\",\n+            (unsigned long)info->size));\n+\n+  /* Dump the props; the propstring should have already been\n+     written in dump_node or above */\n+  if (eb->must_dump_props || eb->dump_props_pending) {\n+    SVN_ERR(svn_stream_write(eb->stream, eb->propstring->data,\n+           &(eb->propstring->len)));\n+\n+    /* Cleanup */\n+    eb->must_dump_props = eb->dump_props_pending = FALSE;\n+    apr_hash_clear(eb->properties);\n+    apr_hash_clear(eb->del_properties);\n+  }\n+\n+  /* Dump the text */\n+  if (eb->must_dump_text) {\n+\n+    /* Open the temporary file, map it to a stream, copy\n+       the stream to eb->stream, close and delete the\n+       file */\n+    SVN_ERR(svn_io_file_open(&temp_file, eb->temp_filepath,APR_READ,\n+\t\t\t     0600,pool));\n+    temp_filestream = svn_stream_from_aprfile2(temp_file, TRUE, pool);\n+    SVN_ERR(svn_stream_copy3(temp_filestream, eb->stream, NULL, NULL, pool));\n+\n+    /* Cleanup */\n+    SVN_ERR(svn_io_file_close(temp_file, pool));\n+    SVN_ERR(svn_stream_close(temp_filestream));\n+    SVN_ERR(svn_io_remove_file2(eb->temp_filepath, TRUE, pool));\n+    eb->must_dump_text = FALSE;\n+  }\n+\n+  SVN_ERR(svn_stream_printf(eb->stream, pool, \"\\n\\n\"));\n+\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+close_edit(void *edit_baton, apr_pool_t *pool)\n+{\n+  struct dump_edit_baton *eb = edit_baton;\n+  svn_pool_destroy(eb->pool);\n+  (eb->current_rev) ++;\n+\n+  return SVN_NO_ERROR;\n+}\n+\n+svn_error_t *\n+get_dump_editor(const svn_delta_editor_t **editor,\n+                void **edit_baton,\n+                svn_revnum_t from_rev,\n+                apr_pool_t *pool)\n+{\n+  struct dump_edit_baton *eb = apr_pcalloc(pool,\n+\t\t\t\t\t   sizeof(struct dump_edit_baton));\n+  eb->current_rev = from_rev;\n+  SVN_ERR(svn_stream_for_stdout(&(eb->stream), pool));\n+  svn_delta_editor_t *de = svn_delta_default_editor(pool);\n+\n+  de->open_root = open_root;\n+  de->delete_entry = delete_entry;\n+  de->add_directory = add_directory;\n+  de->open_directory = open_directory;\n+  de->close_directory = close_directory;\n+  de->change_dir_prop = change_dir_prop;\n+  de->change_file_prop = change_file_prop;\n+  de->apply_textdelta = apply_textdelta;\n+  de->add_file = add_file;\n+  de->open_file = open_file;\n+  de->close_file = close_file;\n+  de->close_edit = close_edit;\n+\n+  /* Set the edit_baton and editor */\n+  *edit_baton = eb;\n+  *editor = de;\n+\n+  return SVN_NO_ERROR;\n+}\nIndex: svnrdump/dump_editor.h\n===================================================================\n--- svnrdump/dump_editor.h\t(revision 0)\n+++ svnrdump/dump_editor.h\t(working copy)\n@@ -0,0 +1,104 @@\n+/*\n+ * ====================================================================\n+ *    Licensed to the Apache Software Foundation (ASF) under one\n+ *    or more contributor license agreements.  See the NOTICE file\n+ *    distributed with this work for additional information\n+ *    regarding copyright ownership.  The ASF licenses this file\n+ *    to you under the Apache License, Version 2.0 (the\n+ *    \"License\"); you may not use this file except in compliance\n+ *    with the License.  You may obtain a copy of the License at\n+ *\n+ *      http://www.apache.org/licenses/LICENSE-2.0\n+ *\n+ *    Unless required by applicable law or agreed to in writing,\n+ *    software distributed under the License is distributed on an\n+ *    \"AS IS\" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY\n+ *    KIND, either express or implied.  See the License for the\n+ *    specific language governing permissions and limitations\n+ *    under the License.\n+ * ====================================================================\n+ */\n+\n+#ifndef DUMP_EDITOR_H_\n+#define DUMP_EDITOR_H_\n+\n+struct dump_edit_baton {\n+  svn_stream_t *stream;\n+  svn_revnum_t current_rev;\n+\n+  /* pool is for per-edit-session allocations */\n+  apr_pool_t *pool;\n+\n+  /* Store the properties that changed */\n+  apr_hash_t *properties;\n+  apr_hash_t *del_properties; /* Value is always 0x1 */\n+  svn_stringbuf_t *propstring;\n+\n+  /* Was a copy command issued? */\n+  svn_boolean_t is_copy;\n+\n+  /* Path of changed file */\n+  const char *changed_path;\n+\n+  /* Temporary file to write delta to along with its checksum */\n+  char *temp_filepath;\n+  svn_checksum_t *checksum;\n+\n+  /* Flags to trigger dumping props and text */\n+  svn_boolean_t must_dump_props;\n+  svn_boolean_t must_dump_text;\n+  svn_boolean_t dump_props_pending;\n+};\n+\n+struct dir_baton {\n+  struct dump_edit_baton *eb;\n+  struct dir_baton *parent_dir_baton;\n+\n+  /* is this directory a new addition to this revision? */\n+  svn_boolean_t added;\n+\n+  /* has this directory been written to the output stream? */\n+  svn_boolean_t written_out;\n+\n+  /* the absolute path to this directory */\n+  const char *path;\n+\n+  /* the comparison path and revision of this directory.  if both of\n+     these are valid, use them as a source against which to compare\n+     the directory instead of the default comparison source of PATH in\n+     the previous revision. */\n+  const char *cmp_path;\n+  svn_revnum_t cmp_rev;\n+\n+  /* hash of paths that need to be deleted, though some -might- be\n+     replaced.  maps const char * paths to this dir_baton.  (they're\n+     full paths, because that's what the editor driver gives us.  but\n+     really, they're all within this directory.) */\n+  apr_hash_t *deleted_entries;\n+\n+  /* pool to be used for deleting the hash items */\n+  apr_pool_t *pool;\n+};\n+\n+struct handler_baton\n+{\n+  svn_txdelta_window_handler_t apply_handler;\n+  void *apply_baton;\n+  apr_pool_t *pool;\n+\n+  /* Information about the path of the tempoarary file used */\n+  char *temp_filepath;\n+  apr_file_t *temp_file;\n+  svn_stream_t *temp_filestream;\n+\n+  /* To fill in the edit baton fields */\n+  struct dump_edit_baton *eb;\n+};\n+\n+svn_error_t *\n+get_dump_editor(const svn_delta_editor_t **editor,\n+                void **edit_baton,\n+                svn_revnum_t to_rev,\n+                apr_pool_t *pool);\n+\n+#endif\nIndex: svnrdump/svnrdump.c\n===================================================================\n--- svnrdump/svnrdump.c\t(revision 0)\n+++ svnrdump/svnrdump.c\t(working copy)\n@@ -0,0 +1,204 @@\n+/*\n+ * ====================================================================\n+ *    Licensed to the Apache Software Foundation (ASF) under one\n+ *    or more contributor license agreements.  See the NOTICE file\n+ *    distributed with this work for additional information\n+ *    regarding copyright ownership.  The ASF licenses this file\n+ *    to you under the Apache License, Version 2.0 (the\n+ *    \"License\"); you may not use this file except in compliance\n+ *    with the License.  You may obtain a copy of the License at\n+ *\n+ *      http://www.apache.org/licenses/LICENSE-2.0\n+ *\n+ *    Unless required by applicable law or agreed to in writing,\n+ *    software distributed under the License is distributed on an\n+ *    \"AS IS\" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY\n+ *    KIND, either express or implied.  See the License for the\n+ *    specific language governing permissions and limitations\n+ *    under the License.\n+ * ====================================================================\n+ */\n+\n+#include \"svn_pools.h\"\n+#include \"svn_cmdline.h\"\n+#include \"svn_client.h\"\n+#include \"svn_ra.h\"\n+#include \"svn_repos.h\"\n+#include \"svn_path.h\"\n+\n+#include \"svnrdump.h\"\n+#include \"dump_editor.h\"\n+\n+static int verbose = 0;\n+static apr_pool_t *pool = NULL;\n+static svn_client_ctx_t *ctx = NULL;\n+static svn_ra_session_t *session = NULL;\n+\n+static svn_error_t *\n+replay_revstart(svn_revnum_t revision,\n+                void *replay_baton,\n+                const svn_delta_editor_t **editor,\n+                void **edit_baton,\n+                apr_hash_t *rev_props,\n+                apr_pool_t *pool)\n+{\n+  /* Editing this revision has just started; dump the revprops\n+     before invoking the editor callbacks */\n+  svn_stringbuf_t *propstring = svn_stringbuf_create(\"\", pool);\n+  svn_stream_t *stdout_stream;\n+\n+  /* Create an stdout stream */\n+  svn_stream_for_stdout(&stdout_stream, pool);\n+\n+        /* Print revision number and prepare the propstring */\n+  SVN_ERR(svn_stream_printf(stdout_stream, pool,\n+          SVN_REPOS_DUMPFILE_REVISION_NUMBER\n+          \": %ld\\n\", revision));\n+  write_hash_to_stringbuf(rev_props, FALSE, &propstring, pool);\n+  svn_stringbuf_appendbytes(propstring, \"PROPS-END\\n\", 10);\n+\n+  /* prop-content-length header */\n+  SVN_ERR(svn_stream_printf(stdout_stream, pool,\n+          SVN_REPOS_DUMPFILE_PROP_CONTENT_LENGTH\n+          \": %\" APR_SIZE_T_FMT \"\\n\", propstring->len));\n+\n+  /* content-length header */\n+  SVN_ERR(svn_stream_printf(stdout_stream, pool,\n+          SVN_REPOS_DUMPFILE_CONTENT_LENGTH\n+          \": %\" APR_SIZE_T_FMT \"\\n\\n\", propstring->len));\n+\n+  /* Print the revprops now */\n+  SVN_ERR(svn_stream_write(stdout_stream, propstring->data,\n+         &(propstring->len)));\n+\n+  svn_stream_close(stdout_stream);\n+\n+  /* Extract editor and editor_baton from the replay_baton and\n+     set them so that the editor callbacks can use them */\n+  struct replay_baton *rb = replay_baton;\n+  *editor = rb->editor;\n+  *edit_baton = rb->edit_baton;\n+\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+replay_revend(svn_revnum_t revision,\n+              void *replay_baton,\n+              const svn_delta_editor_t *editor,\n+              void *edit_baton,\n+              apr_hash_t *rev_props,\n+              apr_pool_t *pool)\n+{\n+  /* Editor has finished for this revision and close_edit has\n+     been called; do nothing: just continue to the next\n+     revision */\n+  if (verbose)\n+    fprintf(stderr, \"* Dumped revision %lu\\n\", revision);\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+open_connection(const char *url)\n+{\n+  SVN_ERR(svn_config_ensure (NULL, pool));\n+  SVN_ERR(svn_client_create_context (&ctx, pool));\n+  SVN_ERR(svn_ra_initialize(pool));\n+\n+  SVN_ERR(svn_config_get_config(&(ctx->config), NULL, pool));\n+\n+  /* Default authentication providers for non-interactive use */\n+  SVN_ERR(svn_cmdline_create_auth_baton(&(ctx->auth_baton), TRUE,\n+                NULL, NULL, NULL, FALSE,\n+                FALSE, NULL, NULL, NULL,\n+                pool));\n+  SVN_ERR(svn_client_open_ra_session(&session, url, ctx, pool));\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+replay_range(svn_revnum_t start_revision, svn_revnum_t end_revision)\n+{\n+  const svn_delta_editor_t *dump_editor;\n+  void *dump_baton;\n+\n+  SVN_ERR(get_dump_editor(&dump_editor,\n+                          &dump_baton, start_revision, pool));\n+\n+  struct replay_baton *replay_baton = apr_palloc(pool,\n+\t\t\t\t\t\t sizeof(struct replay_baton));\n+  replay_baton->editor = dump_editor;\n+  replay_baton->edit_baton = dump_baton;\n+  SVN_ERR(svn_cmdline_printf(pool, SVN_REPOS_DUMPFILE_MAGIC_HEADER \": %d\\n\",\n+           SVN_REPOS_DUMPFILE_FORMAT_VERSION));\n+  SVN_ERR(svn_ra_replay_range(session, start_revision, end_revision,\n+                              0, TRUE, replay_revstart, replay_revend,\n+                              replay_baton, pool));\n+  return SVN_NO_ERROR;\n+}\n+\n+static svn_error_t *\n+usage(FILE *out_stream)\n+{\n+  fprintf(out_stream,\n+    \"usage: svnrdump URL [-r LOWER[:UPPER]]\\n\\n\"\n+    \"Dump the contents of repository at remote URL to stdout in a 'dumpfile'\\n\"\n+    \"v3 portable format.  Dump revisions LOWER rev through UPPER rev.\\n\"\n+    \"LOWER defaults to 1 and UPPER defaults to the highest possible revision\\n\"\n+    \"if omitted.\\n\");\n+  return SVN_NO_ERROR;\n+}\n+\n+int\n+main(int argc, const char **argv)\n+{\n+  int i;\n+  const char *url = NULL;\n+  char *revision_cut = NULL;\n+  svn_revnum_t start_revision = svn_opt_revision_unspecified;\n+  svn_revnum_t end_revision = svn_opt_revision_unspecified;\n+\n+  if (svn_cmdline_init (\"svnrdump\", stderr) != EXIT_SUCCESS)\n+    return EXIT_FAILURE;\n+\n+  pool = svn_pool_create(NULL);\n+\n+  for (i = 1; i < argc; i++) {\n+    if (!strncmp(\"-r\", argv[i], 2)) {\n+      revision_cut = strchr(argv[i] + 2, ':');\n+      if (revision_cut) {\n+        start_revision = (svn_revnum_t) strtoul(argv[i] + 2, &revision_cut, 10);\n+        end_revision = (svn_revnum_t) strtoul(revision_cut + 1, NULL, 10);\n+      }\n+      else\n+        start_revision = (svn_revnum_t) strtoul(argv[i] + 2, NULL, 10);\n+    } else if (!strcmp(\"-v\", argv[i]) || !strcmp(\"--verbose\", argv[i])) {\n+      verbose = 1;\n+    } else if (!strcmp(\"help\", argv[i]) || !strcmp(\"--help\", argv[i])) {\n+      SVN_INT_ERR(usage(stdout));\n+      return EXIT_SUCCESS;\n+    } else if (*argv[i] == '-' || url) {\n+      SVN_INT_ERR(usage(stderr));\n+      return EXIT_FAILURE;\n+    } else\n+      url = argv[i];\n+  }\n+\n+  if (!url || !svn_path_is_url(url)) {\n+    usage(stderr);\n+    return EXIT_FAILURE;\n+  }\n+  SVN_INT_ERR(open_connection(url));\n+\n+  /* Have sane start_revision and end_revision defaults if unspecified */\n+  if (start_revision == svn_opt_revision_unspecified)\n+    start_revision = 1;\n+  if (end_revision == svn_opt_revision_unspecified)\n+    SVN_INT_ERR(svn_ra_get_latest_revnum(session, &end_revision, pool));\n+\n+  SVN_INT_ERR(replay_range(start_revision, end_revision));\n+\n+  svn_pool_destroy(pool);\n+\n+  return 0;\n+}\nIndex: svnrdump/svnrdump.h\n===================================================================\n--- svnrdump/svnrdump.h\t(revision 0)\n+++ svnrdump/svnrdump.h\t(working copy)\n@@ -0,0 +1,44 @@\n+/*\n+ * ====================================================================\n+ *    Licensed to the Apache Software Foundation (ASF) under one\n+ *    or more contributor license agreements.  See the NOTICE file\n+ *    distributed with this work for additional information\n+ *    regarding copyright ownership.  The ASF licenses this file\n+ *    to you under the Apache License, Version 2.0 (the\n+ *    \"License\"); you may not use this file except in compliance\n+ *    with the License.  You may obtain a copy of the License at\n+ *\n+ *      http://www.apache.org/licenses/LICENSE-2.0\n+ *\n+ *    Unless required by applicable law or agreed to in writing,\n+ *    software distributed under the License is distributed on an\n+ *    \"AS IS\" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY\n+ *    KIND, either express or implied.  See the License for the\n+ *    specific language governing permissions and limitations\n+ *    under the License.\n+ * ====================================================================\n+ */\n+\n+#ifndef SVNRDUMP_H_\n+#define SVNRDUMP_H_\n+\n+#include \"dump_editor.h\"\n+\n+struct replay_baton {\n+  const svn_delta_editor_t *editor;\n+  void *edit_baton;\n+};\n+\n+void\n+write_hash_to_stringbuf(apr_hash_t *properties,\n+                        svn_boolean_t deleted,\n+                        svn_stringbuf_t **strbuf,\n+                        apr_pool_t *pool);\n+\n+svn_error_t *\n+dump_props(struct dump_edit_baton *eb,\n+           svn_boolean_t *trigger_var,\n+           svn_boolean_t dump_data_too,\n+           apr_pool_t *pool);\n+\n+#endif\nIndex: svnrdump/util.c\n===================================================================\n--- svnrdump/util.c\t(revision 0)\n+++ svnrdump/util.c\t(working copy)\n@@ -0,0 +1,131 @@\n+/*\n+ * ====================================================================\n+ *    Licensed to the Apache Software Foundation (ASF) under one\n+ *    or more contributor license agreements.  See the NOTICE file\n+ *    distributed with this work for additional information\n+ *    regarding copyright ownership.  The ASF licenses this file\n+ *    to you under the Apache License, Version 2.0 (the\n+ *    \"License\"); you may not use this file except in compliance\n+ *    with the License.  You may obtain a copy of the License at\n+ *\n+ *      http://www.apache.org/licenses/LICENSE-2.0\n+ *\n+ *    Unless required by applicable law or agreed to in writing,\n+ *    software distributed under the License is distributed on an\n+ *    \"AS IS\" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY\n+ *    KIND, either express or implied.  See the License for the\n+ *    specific language governing permissions and limitations\n+ *    under the License.\n+ * ====================================================================\n+ */\n+\n+#include \"svn_pools.h\"\n+#include \"svn_cmdline.h\"\n+#include \"svn_client.h\"\n+#include \"svn_ra.h\"\n+#include \"svn_repos.h\"\n+\n+#include \"svnrdump.h\"\n+\n+void\n+write_hash_to_stringbuf(apr_hash_t *properties,\n+                        svn_boolean_t deleted,\n+                        svn_stringbuf_t **strbuf,\n+                        apr_pool_t *pool)\n+{\n+  apr_hash_index_t *this;\n+  const void *key;\n+  void *val;\n+  apr_ssize_t keylen;\n+  svn_string_t *value;\n+\n+  if (!deleted) {\n+    for (this = apr_hash_first(pool, properties); this;\n+         this = apr_hash_next(this)) {\n+      /* Get this key and val. */\n+      apr_hash_this(this, &key, &keylen, &val);\n+      value = val;\n+\n+      /* Output name length, then name. */\n+      svn_stringbuf_appendcstr(*strbuf,\n+             apr_psprintf(pool, \"K %\" APR_SSIZE_T_FMT \"\\n\",\n+                    keylen));\n+\n+      svn_stringbuf_appendbytes(*strbuf, (const char *) key, keylen);\n+      svn_stringbuf_appendbytes(*strbuf, \"\\n\", 1);\n+\n+      /* Output value length, then value. */\n+      svn_stringbuf_appendcstr(*strbuf,\n+             apr_psprintf(pool, \"V %\" APR_SIZE_T_FMT \"\\n\",\n+                    value->len));\n+\n+      svn_stringbuf_appendbytes(*strbuf, value->data, value->len);\n+      svn_stringbuf_appendbytes(*strbuf, \"\\n\", 1);\n+    }\n+  }\n+  else {\n+    /* Output a \"D \" entry for each deleted property */\n+    for (this = apr_hash_first(pool, properties); this;\n+         this = apr_hash_next(this)) {\n+      /* Get this key */\n+      apr_hash_this(this, &key, &keylen, NULL);\n+\n+      /* Output name length, then name */\n+      svn_stringbuf_appendcstr(*strbuf,\n+             apr_psprintf(pool, \"D %\" APR_SSIZE_T_FMT \"\\n\",\n+                    keylen));\n+\n+      svn_stringbuf_appendbytes(*strbuf, (const char *) key, keylen);\n+      svn_stringbuf_appendbytes(*strbuf, \"\\n\", 1);\n+    }\n+  }\n+}\n+\n+svn_error_t *\n+dump_props(struct dump_edit_baton *eb,\n+           svn_boolean_t *trigger_var,\n+           svn_boolean_t dump_data_too,\n+           apr_pool_t *pool)\n+{\n+  if (trigger_var && !*trigger_var)\n+    return SVN_NO_ERROR;\n+\n+  /* Build a propstring to print */\n+  svn_stringbuf_setempty(eb->propstring);\n+  write_hash_to_stringbuf(eb->properties,\n+        FALSE,\n+        &(eb->propstring), eb->pool);\n+  write_hash_to_stringbuf(eb->del_properties,\n+        TRUE,\n+        &(eb->propstring), eb->pool);\n+  svn_stringbuf_appendbytes(eb->propstring, \"PROPS-END\\n\", 10);\n+\n+  /* prop-delta header */\n+  SVN_ERR(svn_stream_printf(eb->stream, pool,\n+          SVN_REPOS_DUMPFILE_PROP_DELTA\n+          \": true\\n\"));\n+\n+  /* prop-content-length header */\n+  SVN_ERR(svn_stream_printf(eb->stream, pool,\n+          SVN_REPOS_DUMPFILE_PROP_CONTENT_LENGTH\n+          \": %\" APR_SIZE_T_FMT \"\\n\", eb->propstring->len));\n+\n+  if (dump_data_too) {\n+    /* content-length header */\n+    SVN_ERR(svn_stream_printf(eb->stream, pool,\n+            SVN_REPOS_DUMPFILE_CONTENT_LENGTH\n+            \": %\" APR_SIZE_T_FMT \"\\n\\n\",\n+            eb->propstring->len));\n+\n+    /* the properties themselves */\n+    SVN_ERR(svn_stream_write(eb->stream, eb->propstring->data,\n+           &(eb->propstring->len)));\n+\n+    /* Cleanup so that data is never dumped twice */\n+    apr_hash_clear(eb->properties);\n+    apr_hash_clear(eb->del_properties);\n+    if (trigger_var)\n+      *trigger_var = FALSE;\n+  }\n+  return SVN_NO_ERROR;\n+}\n"},{"id":"145456","messageId":"20100713201105.GN13310@ted.stsp.name","threadId":"24349","inReplyTo":"20100709142910.GB20383@debian","subject":"Re: [PATCH v2] Add svnrdump","fromName":"Stefan Sperling","fromEmail":"stsp@elego.de","sentAt":"2010-07-13T20:11:05Z","receivedAt":"2010-07-13T20:11:05Z","isPatch":true,"sender":{"key":"stsp@elego.de","avatar":"https://avatars.githubusercontent.com/u/9281333?v=4"},"body":"On Fri, Jul 09, 2010 at 04:29:10PM +0200, Ramkumar Ramachandra wrote:\n> Hi,\n> \n> Along with the changes suggested by Bert and Daniel, this new version\n> includes a few small bugfixes and feature additions contributed by\n> David and Will, among others. Unfortunately, a diff of the changes\n> made is not available due to whitespace/ style conversion issues:\n> please check the recent commits on my GitHub repository for a summary\n> of these changes: ra-svn branch of\n> http://github.com/artagnon/svn-dump-fast-export\n> \n> Thanks.\n\nReview below.\n\nThis diff is needed to build svnrdump as part of svn on Unix:\n\nIndex: build.conf\n===================================================================\n--- build.conf\t(revision 963733)\n+++ build.conf\t(working copy)\n@@ -167,6 +167,13 @@ libs = libsvn_wc libsvn_subr apriconv apr\n install = bin\n manpages = subversion/svnversion/svnversion.1\n \n+[svnrdump]\n+description = Subversion remote repository dumper\n+type = exe\n+path = subversion/svnrdump\n+libs = libsvn_client libsvn_ra libvsvn_delta libsvn_subr apr\n+install = bin\n+\n # Support for GNOME Keyring\n [libsvn_auth_gnome_keyring]\n description = Subversion GNOME Keyring Library\n\n\nCan you include the above bit in your diff, please, and create follow-up\ndiffs relative to the root of a Subversion trunk working copy? Thanks.\n\nPlease also add a man page similar to the one of svnsync.\nEven though I don't like the fact that our man pages simply refer to\nthe Subversion book rather than providing a small and useful subset of it,\nit's good to at least be consistent about it.\n\nOverall, this looks very good to me. Once your apache.org account has been\nactivated, you have my +1 to commit this and continue working on it in-tree.\nUntil then, feel free to post updated versions of this patch to dev@.\n\nI've started my review with dump_editor.h, so I've moved it to\nthe front of the diff.\n\nI've jumped around a bit during review, so if I've ended up contradicting\nmyself somewhere, please forgive me. Just ask if anything is unclear.\n\n> Index: svnrdump/dump_editor.h\n> ===================================================================\n> --- svnrdump/dump_editor.h\t(revision 0)\n> +++ svnrdump/dump_editor.h\t(working copy)\n> @@ -0,0 +1,104 @@\n> +/*\n> + * ====================================================================\n> + *    Licensed to the Apache Software Foundation (ASF) under one\n> + *    or more contributor license agreements.  See the NOTICE file\n> + *    distributed with this work for additional information\n> + *    regarding copyright ownership.  The ASF licenses this file\n> + *    to you under the Apache License, Version 2.0 (the\n> + *    \"License\"); you may not use this file except in compliance\n> + *    with the License.  You may obtain a copy of the License at\n> + *\n> + *      http://www.apache.org/licenses/LICENSE-2.0\n> + *\n> + *    Unless required by applicable law or agreed to in writing,\n> + *    software distributed under the License is distributed on an\n> + *    \"AS IS\" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY\n> + *    KIND, either express or implied.  See the License for the\n> + *    specific language governing permissions and limitations\n> + *    under the License.\n> + * ====================================================================\n> + */\n> +\n> +#ifndef DUMP_EDITOR_H_\n> +#define DUMP_EDITOR_H_\n> +\n> +struct dump_edit_baton {\n\nPlease add a comment here explaining what the stream is used for,\nfor instance /* The output stream we write the dumpfile to. */\n\n> +  svn_stream_t *stream;\n> +  svn_revnum_t current_rev;\n\nThis is only incremented by never used?\n\n> +\n> +  /* pool is for per-edit-session allocations */\n> +  apr_pool_t *pool;\n> +\n> +  /* Store the properties that changed */\n> +  apr_hash_t *properties;\n> +  apr_hash_t *del_properties; /* Value is always 0x1 */\n\nJust say \"value is undefined\". Or use an apr_array_header_t.\n\nA comment here saying what propstring is for would be nice.\n\n> +  svn_stringbuf_t *propstring;\n> +\n> +  /* Was a copy command issued? */\n> +  svn_boolean_t is_copy;\n\nCopy of what and when? This baton is global for the entire edit...\n\nGoing through the code, I see that you're using this to indicate to\ndump_node() whether an add_directory() or add_file() was in fact a copy.\nWhy not remove this field from the struct and add it as a parameter to\ndump_node instead?\n\n> +\n> +  /* Path of changed file */\n> +  const char *changed_path;\n\nThe changed_path field seems to be unused.\n\nAccording to comments in open_file() and add_file(), change_file_prop()\nand apply_textdelta() should be using this but they aren't.\n\n> +  /* Temporary file to write delta to along with its checksum */\n> +  char *temp_filepath;\n\nThat's a poor variable name. What about delta_abspath?\n\n> +  svn_checksum_t *checksum;\n\nAnd rename this to delta_checksum?\n\n> +\n> +  /* Flags to trigger dumping props and text */\n> +  svn_boolean_t must_dump_props;\n> +  svn_boolean_t must_dump_text;\n\nI'd call these dump_props and dump_text, but that's a matter of taste.\n\n> +  svn_boolean_t dump_props_pending;\n> +};\n> +\n> +struct dir_baton {\n> +  struct dump_edit_baton *eb;\n> +  struct dir_baton *parent_dir_baton;\n> +\n> +  /* is this directory a new addition to this revision? */\n> +  svn_boolean_t added;\n> +\n> +  /* has this directory been written to the output stream? */\n> +  svn_boolean_t written_out;\n> +\n> +  /* the absolute path to this directory */\n> +  const char *path;\n\nIn code written post-svn-1.6, we usually call absolute paths\nsomething_abspath. E.g. local_abspath is an absolute path in a\nfilesystem on the client (but not in the repository).\n\nElsewhere, this is called 'full_path' which would be fine name\nfor this field, too. (Though any full_path variable you see in svn\nmost likely pre-dates the *_abspath convention.)\n\n> +\n> +  /* the comparison path and revision of this directory.  if both of\n> +     these are valid, use them as a source against which to compare\n> +     the directory instead of the default comparison source of PATH in\n> +     the previous revision. */\n> +  const char *cmp_path;\n> +  svn_revnum_t cmp_rev;\n\nThese just seem to be used as regular copyfrom info, so let's name them\nas such: copyfrom_path and copyfrom_rev\nThen you can also shrink the comment above cause everyone knows what\ncopyfrom info is: /* Copyfrom info for the node, if any. */\n\n> +\n> +  /* hash of paths that need to be deleted, though some -might- be\n> +     replaced.  maps const char * paths to this dir_baton.  (they're\n> +     full paths, because that's what the editor driver gives us.  but\n> +     really, they're all within this directory.) */\n> +  apr_hash_t *deleted_entries;\n\nThis is very well commented and well named.\n\n> +\n> +  /* pool to be used for deleting the hash items */\n> +  apr_pool_t *pool;\n\nHmmm.. a pool does not delete anything. It provides storage.\nWhy do you need this?\n\n> +};\n> +\n> +struct handler_baton\n> +{\n> +  svn_txdelta_window_handler_t apply_handler;\n> +  void *apply_baton;\n> +  apr_pool_t *pool;\n\nYet another pool. What's it for?\n\n> +\n> +  /* Information about the path of the tempoarary file used */\n\ns/tempoarary/temporary/\n\n> +  char *temp_filepath;\n> +  apr_file_t *temp_file;\n> +  svn_stream_t *temp_filestream;\n\nWhat's the temporary file used for? You're writing a delta to it,\nso maybe name it accordingly?\n\nYou need the file name to stat it in close_file().\nYou need the stream in the baton to write to the file.\nBut you don't need the apr_file.\nOpen the file. Then wrap it in the stream with disown=FALSE, and pass\njust the stream to the window handler via the baton. When the stream\nis closed, the file will be closed as well.\n\n> +\n> +  /* To fill in the edit baton fields */\n> +  struct dump_edit_baton *eb;\n\nJust say /* Global edit baton. */ or even drop the comment.\n\n> +};\n> +\n\nNeeds a docstring.\n\n> +svn_error_t *\n> +get_dump_editor(const svn_delta_editor_t **editor,\n> +                void **edit_baton,\n> +                svn_revnum_t to_rev,\n> +                apr_pool_t *pool);\n> +\n> +#endif\n> Index: svnrdump/dump_editor.c\n> ===================================================================\n> --- svnrdump/dump_editor.c\t(revision 0)\n> +++ svnrdump/dump_editor.c\t(working copy)\n> @@ -0,0 +1,690 @@\n> +/*\n> + * ====================================================================\n> + *    Licensed to the Apache Software Foundation (ASF) under one\n> + *    or more contributor license agreements.  See the NOTICE file\n> + *    distributed with this work for additional information\n> + *    regarding copyright ownership.  The ASF licenses this file\n> + *    to you under the Apache License, Version 2.0 (the\n> + *    \"License\"); you may not use this file except in compliance\n> + *    with the License.  You may obtain a copy of the License at\n> + *\n> + *      http://www.apache.org/licenses/LICENSE-2.0\n> + *\n> + *    Unless required by applicable law or agreed to in writing,\n> + *    software distributed under the License is distributed on an\n> + *    \"AS IS\" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY\n> + *    KIND, either express or implied.  See the License for the\n> + *    specific language governing permissions and limitations\n> + *    under the License.\n> + * ====================================================================\n> + */\n> +\n> +#include \"svn_pools.h\"\n> +#include \"svn_repos.h\"\n> +#include \"svn_path.h\"\n> +#include \"svn_props.h\"\n> +#include \"svn_dirent_uri.h\"\n> +\n> +#include \"svnrdump.h\"\n> +#include \"dump_editor.h\"\n> +\n> +#define ARE_VALID_COPY_ARGS(p,r) ((p) && SVN_IS_VALID_REVNUM(r))\n> +\n> +/* Make a directory baton to represent the directory was path\n> +   (relative to EDIT_BATON's path) is PATH.\n\nThe above sentence doesn't parse.\n\n> +\n> +   CMP_PATH/CMP_REV are the path/revision against which this directory\n> +   should be compared for changes.  If either is omitted (NULL for the\n> +   path, SVN_INVALID_REVNUM for the rev), just compare this directory\n> +   PATH against itself in the previous revision.\n\ns/CMP/COPYFROM/ and tweak the docstring to say something like:\n  If the copyfrom information is valid, the directory will be compared\n  against its copy source. Else, it will be compared against itself in\n  the previous revision.\n\n> +   PARENT_DIR_BATON is the directory baton of this directory's parent,\n> +   or NULL if this is the top-level directory of the edit.  ADDED\n> +   indicated if this directory is newly added in this revision.\n\ns/indicated/indicates/\n\n> +   Perform all allocations in POOL.  */\n> +static struct dir_baton *\n> +make_dir_baton(const char *path,\n> +               const char *cmp_path,\n> +               svn_revnum_t cmp_rev,\n> +               void *edit_baton,\n> +               void *parent_dir_baton,\n> +               svn_boolean_t added,\n> +               apr_pool_t *pool)\n> +{\n> +  struct dump_edit_baton *eb = edit_baton;\n> +  struct dir_baton *pb = parent_dir_baton;\n> +  struct dir_baton *new_db = apr_pcalloc(pool, sizeof(*new_db));\n> +  const char *full_path;\n> +  apr_array_header_t *compose_path = apr_array_make(pool, 2,\n> +\t\t\t\t\t\t    sizeof(const char *));\n\nThe above line contains tabs, please replace with spaces.\nAnd make sure to align function arguments like this (not sure if\nthey appeared aligned in your editor or not):\n\napr_array_header_t *compose_path = apr_array_make(pool, 2,\n                                                  sizeof(const char *));\n\n> +\n> +  /* A path relative to nothing?  I don't think so. */\n> +  SVN_ERR_ASSERT_NO_RETURN(!path || pb);\n> +\n> +  /* Construct the full path of this node. */\n> +  if (pb) {\n\nI've told you this on IRC before, but just for sake of completeness:\nVirtually all if blocks and loops in this patch have a \"wrong\" style\nof indentation.\nSee\nhttp://subversion.apache.org/docs/community-guide/conventions.html#coding-style\n\n(I personally prefer the indentation style you're using,\nbut the project convention has been set looooong ago -- such is life.)\n\n> +    APR_ARRAY_PUSH(compose_path, const char *) = \"/\";\n> +    APR_ARRAY_PUSH(compose_path, const char *) = path;\n> +    full_path = svn_path_compose(compose_path, pool);\n\nSee svn_dirent_join_many().\n\n> +  }\n> +  else\n> +    full_path = apr_pstrdup(pool, \"/\");\n\nWhy allocate \"/\" in a pool? This can be static string unless you\nintend to write to it.\n\n> +\n> +  /* Remove leading slashes from copyfrom paths. */\n> +  if (cmp_path)\n> +    cmp_path = ((*cmp_path == '/') ? cmp_path + 1 : cmp_path);\n> +\n> +  new_db->eb = eb;\n> +  new_db->parent_dir_baton = pb;\n> +  new_db->path = full_path;\n> +  new_db->cmp_path = cmp_path ? apr_pstrdup(pool, cmp_path) : NULL;\n> +  new_db->cmp_rev = cmp_rev;\n> +  new_db->added = added;\n> +  new_db->written_out = FALSE;\n> +  new_db->deleted_entries = apr_hash_make(pool);\n> +  new_db->pool = pool;\n> +\n> +  return new_db;\n> +}\n> +/*\n> + * Write out a node record for PATH of type KIND under EB->FS_ROOT.\n> + * ACTION describes what is happening to the node (see enum svn_node_action).\n> + * Write record to writable EB->STREAM, using EB->BUFFER to write in chunks.\n> + *\n> + * If the node was itself copied, IS_COPY is TRUE and the\n> + * path/revision of the copy source are in CMP_PATH/CMP_REV.  If\n> + * IS_COPY is FALSE, yet CMP_PATH/CMP_REV are valid, this node is part\n> + * of a copied subtree.\n\nAgain, s/CMP/COPYFROM/\n\n> + */\n> +static svn_error_t *\n> +dump_node(struct dump_edit_baton *eb,\n> +          const char *path,    /* an absolute path. */\n> +          svn_node_kind_t kind,\n> +          enum svn_node_action action,\n> +          const char *cmp_path,\n> +          svn_revnum_t cmp_rev,\n> +          apr_pool_t *pool)\n> +{\n> +  /* Write out metadata headers for this file node. */\n\nThe node might as well be a directory, so the above comment is misleading.\n\n> +  SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +          SVN_REPOS_DUMPFILE_NODE_PATH \": %s\\n\",\n> +          (*path == '/') ? path + 1 : path));\n> +\n> +  if (kind == svn_node_file)\n> +    SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +                              SVN_REPOS_DUMPFILE_NODE_KIND \": file\\n\"));\n> +  else if (kind == svn_node_dir)\n> +    SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +                              SVN_REPOS_DUMPFILE_NODE_KIND \": dir\\n\"));\n> +\n> +  /* Remove leading slashes from copyfrom paths. */\n> +  if (cmp_path)\n> +    cmp_path = ((*cmp_path == '/') ? cmp_path + 1 : cmp_path);\n\nWhat if the copyfrom path is \"/\"?\n(If memory serves me right we've had a bug like this before somewhere...)\n\n> +\n> +  switch (action) {\n> +    /* Appropriately handle the four svn_node_action actions */\n\nNuke the above comment. Don't put numbers that may change some day\ninto comments.\n\n> +\n> +  case svn_node_action_change:\n> +    SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +                              SVN_REPOS_DUMPFILE_NODE_ACTION\n> +                              \": change\\n\"));\n> +    break;\n> +\n> +  case svn_node_action_replace:\n> +    if (!eb->is_copy) {\n> +      /* a simple delete+add, implied by a single 'replace' action. */\n> +      SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +                                SVN_REPOS_DUMPFILE_NODE_ACTION\n> +                                \": replace\\n\"));\n> +\n> +      eb->dump_props_pending = TRUE;\n> +      break;\n> +    }\n> +    /* More complex case: eb->is_copy is true, and\n> +       cmp_path/ cmp_rev are present: delete the original,\n> +       and then re-add it */\n> +\n> +    /* the path & kind headers have already been printed;  just\n> +       add a delete action, and end the current record.*/\n> +    SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +                              SVN_REPOS_DUMPFILE_NODE_ACTION\n> +                              \": delete\\n\\n\"));\n> +\n> +    /* recurse:  print an additional add-with-history record. */\n> +    SVN_ERR(dump_node(eb, path, kind, svn_node_action_add,\n> +                      cmp_path, cmp_rev, pool));\n> +\n> +    /* we can leave this routine quietly now, don't need to dump\n> +       any content;  that was already done in the second record. */\n> +    eb->must_dump_props = FALSE;\n> +    eb->is_copy = FALSE;\n> +    break;\n> +\n> +  case svn_node_action_delete:\n> +    SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +                              SVN_REPOS_DUMPFILE_NODE_ACTION\n> +                              \": delete\\n\"));\n> +\n> +    /* we can leave this routine quietly now, don't need to dump\n> +       any content. */\n> +    SVN_ERR(svn_stream_printf(eb->stream, pool, \"\\n\\n\"));\n> +    eb->must_dump_props = FALSE;\n> +    break;\n> +\n> +  case svn_node_action_add:\n> +    SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +                              SVN_REPOS_DUMPFILE_NODE_ACTION \": add\\n\"));\n> +\n> +    if (!eb->is_copy) {\n> +      /* eb->dump_props_pending for files is handled in\n> +         close_file which is called immediately.\n> +         However, directories are not closed until\n> +         all the work inside them have been done;\n\ns/have been/has been/\n\n> +         eb->dump_props_pending for directories is\n> +         handled in all the functions that can\n> +         possibly be called after add_directory:\n> +         add_directory, open_directory,\n> +         delete_entry, close_directory, add_file,\n> +         open_file and change_dir_prop;\n> +         change_dir_prop is a special case\n> +         ofcourse */\n\nPlease re-format the above using longer lines (up to column 78).\n\n> +\n> +      eb->dump_props_pending = TRUE;\n> +      break;\n> +    }\n> +\n> +    SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +                              SVN_REPOS_DUMPFILE_NODE_COPYFROM_REV\n> +                              \": %ld\\n\"\n> +                              SVN_REPOS_DUMPFILE_NODE_COPYFROM_PATH\n> +                              \": %s\\n\",\n> +                              cmp_rev, cmp_path));\n> +\n> +    /* Dump the text only if apply_textdelta sets\n> +       eb->must_dump_text */\n> +\n> +    /* UGLY hack: If a directory was copied from a\n> +       previous revision, nothing else can be done, and\n> +       close_file won't be called to write two blank\n> +       lines; write them here */\n> +    if (kind == svn_node_dir)\n> +      SVN_ERR(svn_stream_printf(eb->stream, pool, \"\\n\\n\"));\n> +\n> +    eb->is_copy = FALSE;\n> +\n> +    break;\n> +  }\n> +\n> +  /* Dump property headers */\n> +  SVN_ERR(dump_props(eb, &(eb->must_dump_props), FALSE, pool));\n> +\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *open_root(void *edit_baton,\n> +\t\t\t      svn_revnum_t base_revision,\n> +\t\t\t      apr_pool_t *pool,\n> +\t\t\t      void **root_baton)\n\ntabs in above 3 lines\n\n> +{\n> +  /* Allocate a special pool for the edit_baton to avoid pool\n> +     lifetime issues */\n\nI think you don't need this comment because this is already\nsort of documented in the docstring for open_root() in svn_delta.h.\n\n> +  struct dump_edit_baton *eb = edit_baton;\n> +  eb->pool = svn_pool_create(pool);\n> +  eb->properties = apr_hash_make(eb->pool);\n> +  eb->del_properties = apr_hash_make(eb->pool);\n> +  eb->propstring = svn_stringbuf_create(\"\", eb->pool);\n> +  eb->is_copy = FALSE;\n> +\n> +  *root_baton = make_dir_baton(NULL, NULL, SVN_INVALID_REVNUM,\n> +                               edit_baton, NULL, FALSE, pool);\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +delete_entry(const char *path,\n> +             svn_revnum_t revision,\n> +             void *parent_baton,\n> +             apr_pool_t *pool)\n> +{\n> +  struct dir_baton *pb = parent_baton;\n> +  const char *mypath = apr_pstrdup(pb->pool, path);\n> +\n> +  /* Some pending properties to dump? */\n> +  SVN_ERR(dump_props(pb->eb, &(pb->eb->dump_props_pending), TRUE, pool));\n> +\n> +  /* remember this path needs to be deleted */\n> +  apr_hash_set(pb->deleted_entries, mypath, APR_HASH_KEY_STRING, pb);\n> +\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +add_directory(const char *path,\n> +              void *parent_baton,\n> +              const char *copyfrom_path,\n> +              svn_revnum_t copyfrom_rev,\n> +              apr_pool_t *pool,\n> +              void **child_baton)\n> +{\n> +  struct dir_baton *pb = parent_baton;\n> +  void *val;\n> +  struct dir_baton *new_db\n> +    = make_dir_baton(path, copyfrom_path, copyfrom_rev, pb->eb, pb, TRUE, pool);\n> +\n> +  /* Some pending properties to dump? */\n> +  SVN_ERR(dump_props(pb->eb, &(pb->eb->dump_props_pending), TRUE, pool));\n> +\n> +  /* This might be a replacement -- is the path already deleted? */\n> +  val = apr_hash_get(pb->deleted_entries, path, APR_HASH_KEY_STRING);\n> +\n> +  /* Detect an add-with-history */\n> +  pb->eb->is_copy = ARE_VALID_COPY_ARGS(copyfrom_path, copyfrom_rev);\n> +\n> +  /* Dump the node */\n> +  SVN_ERR(dump_node(pb->eb, path,\n> +                    svn_node_dir,\n> +                    val ? svn_node_action_replace : svn_node_action_add,\n> +                    pb->eb->is_copy ? copyfrom_path : NULL,\n> +                    pb->eb->is_copy ? copyfrom_rev : SVN_INVALID_REVNUM,\n> +                    pool));\n> +\n> +  if (val)\n> +    /* Delete the path, it's now been dumped */\n> +    apr_hash_set(pb->deleted_entries, path, APR_HASH_KEY_STRING, NULL);\n\nYou don't need to set the value to NULL in the hash table.\nDoing so won't save any memory. I've say just remove the above 3 lines.\n\n> +\n> +  new_db->written_out = TRUE;\n> +\n> +  *child_baton = new_db;\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +open_directory(const char *path,\n> +               void *parent_baton,\n> +               svn_revnum_t base_revision,\n> +               apr_pool_t *pool,\n> +               void **child_baton)\n> +{\n> +  struct dir_baton *pb = parent_baton;\n> +  struct dir_baton *new_db;\n> +  const char *cmp_path = NULL;\n> +  svn_revnum_t cmp_rev = SVN_INVALID_REVNUM;\n> +  apr_array_header_t *compose_path = apr_array_make(pool, 2,\n> +\t\t\t\t\t\t    sizeof(const char *));\n\ntabs again in the above line\n\n> +\n> +  /* Some pending properties to dump? */\n> +  SVN_ERR(dump_props(pb->eb, &(pb->eb->dump_props_pending), TRUE, pool));\n> +\n> +  /* If the parent directory has explicit comparison path and rev,\n> +     record the same for this one. */\n> +  if (pb && ARE_VALID_COPY_ARGS(pb->cmp_path, pb->cmp_rev)) {\n> +    APR_ARRAY_PUSH(compose_path, const char *) = pb->cmp_path;\n> +    APR_ARRAY_PUSH(compose_path, const char *) =\n> +\t    svn_relpath_basename(path, pool);\n\nand here is another tab \n\n> +    cmp_path = svn_path_compose(compose_path, pool);\n\nAgain, see svn_dirent_join_many().\nIf you need a svn_relpath_join_many() for some reason please write one.\n\n> +    cmp_rev = pb->cmp_rev;\n> +  }\n> +\n> +  new_db = make_dir_baton(path, cmp_path, cmp_rev, pb->eb, pb, FALSE, pool);\n> +  *child_baton = new_db;\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +close_directory(void *dir_baton,\n> +                apr_pool_t *pool)\n> +{\n> +  struct dir_baton *db = dir_baton;\n> +  struct dump_edit_baton *eb = db->eb;\n> +  apr_hash_index_t *hi;\n> +  apr_pool_t *subpool = svn_pool_create(pool);\n\nPlease call this iterpool, not subpool.\nYou're using it in a loop (so we prefer \"iteration pool\").\n\n> +\n> +  /* Some pending properties to dump? */\n> +  SVN_ERR(dump_props(eb, &(eb->dump_props_pending), TRUE, pool));\n> +\n> +  /* Dump the directory entries */\n> +  for (hi = apr_hash_first(pool, db->deleted_entries); hi;\n> +       hi = apr_hash_next(hi)) {\n> +    const void *key;\n> +    const char *path;\n> +    apr_hash_this(hi, &key, NULL, NULL);\n> +    path = key;\n\nSee svn__apr_hash_index_key().\n\n> +\n> +    svn_pool_clear(subpool);\n> +\n> +    SVN_ERR(dump_node(db->eb, path,\n> +                      svn_node_unknown, svn_node_action_delete,\n> +                      NULL, SVN_INVALID_REVNUM, subpool));\n> +  }\n> +\n> +  svn_pool_destroy(subpool);\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +add_file(const char *path,\n> +         void *parent_baton,\n> +         const char *copyfrom_path,\n> +         svn_revnum_t copyfrom_rev,\n> +         apr_pool_t *pool,\n> +         void **file_baton)\n> +{\n> +  struct dir_baton *pb = parent_baton;\n> +  void *val;\n> +\n> +  /* Some pending properties to dump? */\n> +  SVN_ERR(dump_props(pb->eb, &(pb->eb->dump_props_pending), TRUE, pool));\n> +\n> +  /* This might be a replacement -- is the path already deleted? */\n> +  val = apr_hash_get(pb->deleted_entries, path, APR_HASH_KEY_STRING);\n> +\n> +  /* Detect add-with-history. */\n> +  pb->eb->is_copy = ARE_VALID_COPY_ARGS(copyfrom_path, copyfrom_rev);\n> +\n> +  /* Dump the node. */\n> +  SVN_ERR(dump_node(pb->eb, path,\n> +                    svn_node_file,\n> +                    val ? svn_node_action_replace : svn_node_action_add,\n> +                    pb->eb->is_copy ? copyfrom_path : NULL,\n> +                    pb->eb->is_copy ? copyfrom_rev : SVN_INVALID_REVNUM,\n> +                    pool));\n> +\n> +  if (val)\n> +    /* delete the path, it's now been dumped. */\n> +    apr_hash_set(pb->deleted_entries, path, APR_HASH_KEY_STRING, NULL);\n> +\n> +  /* Build a nice file baton to pass to change_file_prop and apply_textdelta */\n> +  pb->eb->changed_path = path;\n> +  *file_baton = pb->eb;\n> +\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +open_file(const char *path,\n> +          void *parent_baton,\n> +          svn_revnum_t ancestor_revision,\n> +          apr_pool_t *pool,\n> +          void **file_baton)\n> +{\n> +  struct dir_baton *pb = parent_baton;\n> +  const char *cmp_path = NULL;\n> +  svn_revnum_t cmp_rev = SVN_INVALID_REVNUM;\n> +\n> +  /* Some pending properties to dump? */\n> +  SVN_ERR(dump_props(pb->eb, &(pb->eb->dump_props_pending), TRUE, pool));\n> +\n> +  apr_array_header_t *compose_path = apr_array_make(pool, 2,\n> +\t\t\t\t\t\t    sizeof(const char *));\n\ntabs\n\n> +  /* If the parent directory has explicit comparison path and rev,\n\ns/comparison/copyfrom/\n\n> +     record the same for this one. */\n> +  if (pb && ARE_VALID_COPY_ARGS(pb->cmp_path, pb->cmp_rev)) {\n> +    APR_ARRAY_PUSH(compose_path, const char *) = pb->cmp_path;\n> +    APR_ARRAY_PUSH(compose_path, const char *) =\n> +\t    svn_relpath_basename(path, pool);\n\none more tab\n\n> +    cmp_path = svn_path_compose(compose_path, pool);\n> +    cmp_rev = pb->cmp_rev;\n> +  }\n> +\n> +  SVN_ERR(dump_node(pb->eb, path,\n> +                    svn_node_file, svn_node_action_change,\n> +                    cmp_path, cmp_rev, pool));\n> +\n> +  /* Build a nice file baton to pass to change_file_prop and apply_textdelta */\n> +  pb->eb->changed_path = path;\n> +  *file_baton = pb->eb;\n> +\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +change_dir_prop(void *parent_baton,\n> +                const char *name,\n> +                const svn_string_t *value,\n> +                apr_pool_t *pool)\n> +{\n> +  struct dir_baton *db = parent_baton;\n> +\n> +  if (svn_property_kind(NULL, name) != svn_prop_regular_kind)\n> +    return SVN_NO_ERROR;\n> +\n> +  value ? apr_hash_set(db->eb->properties, apr_pstrdup(pool, name),\n> +                       APR_HASH_KEY_STRING, svn_string_dup(value, pool)) :\n> +    apr_hash_set(db->eb->del_properties, apr_pstrdup(pool, name),\n> +                 APR_HASH_KEY_STRING, (void *)0x1);\n> +\n> +  /* This function is what distinguishes between a directory that is\n> +     opened to merely get somewhere, vs. one that is opened because it\n> +     actually changed by itself  */\n> +  if (! db->written_out) {\n> +    /* If eb->dump_props_pending was set, it means that the\n> +       node information corresponding to add_directory has already\n> +       been written; just don't unset it and dump_node will dump\n> +       the properties before doing anything else. If it wasn't\n> +       set, node information hasn't been written yet: so dump the\n> +       node itself before dumping the props */\n> +\n> +    SVN_ERR(dump_node(db->eb, db->path,\n> +                      svn_node_dir, svn_node_action_change,\n> +                      db->cmp_path, db->cmp_rev, pool));\n> +\n> +    SVN_ERR(dump_props(db->eb, NULL, TRUE, pool));\n> +    db->written_out = TRUE;\n> +  }\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +change_file_prop(void *file_baton,\n> +                 const char *name,\n> +                 const svn_string_t *value,\n> +                 apr_pool_t *pool)\n> +{\n> +  struct dump_edit_baton *eb = file_baton;\n> +\n> +  if (svn_property_kind(NULL, name) != svn_prop_regular_kind)\n> +    return SVN_NO_ERROR;\n> +\n> +  apr_hash_set(eb->properties, apr_pstrdup(pool, name),\n> +               APR_HASH_KEY_STRING, value ?\n> +               svn_string_dup(value, pool): (void *)0x1);\n> +  /* Dump the property headers and wait; close_file might need\n> +     to write text headers too depending on whether\n> +     apply_textdelta is called */\n> +  eb->dump_props_pending = TRUE;\n> +\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +window_handler(svn_txdelta_window_t *window, void *baton)\n> +{\n> +  struct handler_baton *hb = baton;\n> +  struct dump_edit_baton *eb = hb->eb;\n> +  static svn_error_t *err;\n> +\n> +  err = hb->apply_handler(window, hb->apply_baton);\n> +  if (window != NULL && !err)\n> +    return SVN_NO_ERROR;\n> +\n> +  if (err)\n> +    SVN_ERR(err);\n> +\n> +  /* Write information about the filepath to hb->eb */\n\ns/to hb->eb/from the handler baton to the edit baton/\n\n> +  eb->temp_filepath = apr_pstrdup(eb->pool,\n> +          hb->temp_filepath);\n> +\n> +  /* Cleanup */\n> +  SVN_ERR(svn_io_file_close(hb->temp_file, hb->pool));\n\nAs described above, you don't need to close the file,\nclosing the stream is enough.\n\n> +  SVN_ERR(svn_stream_close(hb->temp_filestream));\n> +  svn_pool_destroy(hb->pool);\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +apply_textdelta(void *file_baton, const char *base_checksum,\n> +                apr_pool_t *pool,\n> +                svn_txdelta_window_handler_t *handler,\n> +                void **handler_baton)\n> +{\n> +  struct dump_edit_baton *eb = file_baton;\n> +  apr_status_t apr_err;\n> +  const char *tempdir;\n> +\n> +  /* Custom handler_baton allocated in a separate pool */\n> +  apr_pool_t *handler_pool = svn_pool_create(pool);\n> +  struct handler_baton *hb = apr_pcalloc(handler_pool, sizeof(*hb));\n> +  hb->pool = handler_pool;\n\nIt sucks that the window handler does not get pool arguments, so\nyou have to stick a pool in the baton. But that isn't your fault.\n\n> +  hb->eb = eb;\n> +\n> +  /* Use a temporary file to measure the text-content-length */\n> +  SVN_ERR(svn_io_temp_dir(&tempdir, hb->pool));\n> +\n> +  hb->temp_filepath = svn_dirent_join(tempdir, \"XXXXXX\", hb->pool);\n> +  apr_err = apr_file_mktemp(&(hb->temp_file), hb->temp_filepath,\n> +          APR_CREATE | APR_READ | APR_WRITE | APR_EXCL,\n> +          hb->pool);\n> +  if (apr_err != APR_SUCCESS)\n> +    SVN_ERR(svn_error_wrap_apr(apr_err, NULL));\n\nYou can replace the above chunk with a simple call to\nsvn_io_open_unique_file3().\n\n> +\n> +  hb->temp_filestream = svn_stream_from_aprfile2(hb->temp_file, TRUE, hb->pool);\n> +\n> +  /* Prepare to write the delta to the temporary file */\n> +  svn_txdelta_to_svndiff2(&(hb->apply_handler), &(hb->apply_baton),\n> +                          hb->temp_filestream, 0, hb->pool);\n> +  eb->must_dump_text = TRUE;\n> +\n> +  /* The actual writing takes place when this function has finished */\n> +  /* Set the handler and handler_baton */\n> +  *handler = window_handler;\n> +  *handler_baton = hb;\n> +\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +close_file(void *file_baton,\n> +           const char *text_checksum,\n> +           apr_pool_t *pool)\n> +{\n> +  struct dump_edit_baton *eb = file_baton;\n> +  apr_file_t *temp_file;\n> +  svn_stream_t *temp_filestream;\n> +  apr_finfo_t *info = apr_pcalloc(pool, sizeof(apr_finfo_t));\n> +\n> +  /* We didn't write the property headers because we were\n> +     waiting for file_prop_change; write them now */\n> +  SVN_ERR(dump_props(eb, &(eb->dump_props_pending), FALSE, pool));\n> +\n> +  /* The prop headers have already been dumped in dump_node */\n> +  /* Dump the text headers */\n> +  if (eb->must_dump_text) {\n> +    /* text-delta header */\n> +    SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +            SVN_REPOS_DUMPFILE_TEXT_DELTA\n> +            \": true\\n\"));\n> +\n> +    /* Measure the length */\n> +    SVN_ERR(svn_io_stat(info, eb->temp_filepath, APR_FINFO_SIZE, pool));\n> +\n> +    /* text-content-length header */\n> +    SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +            SVN_REPOS_DUMPFILE_TEXT_CONTENT_LENGTH\n> +            \": %lu\\n\",\n> +            (unsigned long)info->size));\n> +    /* text-content-md5 header */\n> +    SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +            SVN_REPOS_DUMPFILE_TEXT_CONTENT_MD5\n> +            \": %s\\n\",\n> +            text_checksum));\n> +  }\n> +\n> +  /* content-length header: if both text and props are absent,\n> +     skip this block */\n> +  if (eb->must_dump_props || eb->dump_props_pending)\n> +    SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +            SVN_REPOS_DUMPFILE_CONTENT_LENGTH\n> +            \": %ld\\n\\n\",\n> +            (unsigned long)info->size + eb->propstring->len));\n> +  else if (eb->must_dump_text)\n> +    SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +            SVN_REPOS_DUMPFILE_CONTENT_LENGTH\n> +            \": %ld\\n\\n\",\n> +            (unsigned long)info->size));\n> +\n> +  /* Dump the props; the propstring should have already been\n> +     written in dump_node or above */\n> +  if (eb->must_dump_props || eb->dump_props_pending) {\n> +    SVN_ERR(svn_stream_write(eb->stream, eb->propstring->data,\n> +           &(eb->propstring->len)));\n> +\n> +    /* Cleanup */\n> +    eb->must_dump_props = eb->dump_props_pending = FALSE;\n> +    apr_hash_clear(eb->properties);\n> +    apr_hash_clear(eb->del_properties);\n> +  }\n> +\n> +  /* Dump the text */\n> +  if (eb->must_dump_text) {\n> +\n> +    /* Open the temporary file, map it to a stream, copy\n> +       the stream to eb->stream, close and delete the\n> +       file */\n> +    SVN_ERR(svn_io_file_open(&temp_file, eb->temp_filepath,APR_READ,\n> +\t\t\t     0600,pool));\n\ntabs again\n\n> +    temp_filestream = svn_stream_from_aprfile2(temp_file, TRUE, pool);\n> +    SVN_ERR(svn_stream_copy3(temp_filestream, eb->stream, NULL, NULL, pool));\n> +\n> +    /* Cleanup */\n> +    SVN_ERR(svn_io_file_close(temp_file, pool));\n> +    SVN_ERR(svn_stream_close(temp_filestream));\n> +    SVN_ERR(svn_io_remove_file2(eb->temp_filepath, TRUE, pool));\n> +    eb->must_dump_text = FALSE;\n> +  }\n> +\n> +  SVN_ERR(svn_stream_printf(eb->stream, pool, \"\\n\\n\"));\n> +\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +close_edit(void *edit_baton, apr_pool_t *pool)\n> +{\n> +  struct dump_edit_baton *eb = edit_baton;\n> +  svn_pool_destroy(eb->pool);\n> +  (eb->current_rev) ++;\n> +\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +svn_error_t *\n> +get_dump_editor(const svn_delta_editor_t **editor,\n> +                void **edit_baton,\n> +                svn_revnum_t from_rev,\n> +                apr_pool_t *pool)\n> +{\n> +  struct dump_edit_baton *eb = apr_pcalloc(pool,\n> +\t\t\t\t\t   sizeof(struct dump_edit_baton));\n\nmore tabs\n\n> +  eb->current_rev = from_rev;\n> +  SVN_ERR(svn_stream_for_stdout(&(eb->stream), pool));\n> +  svn_delta_editor_t *de = svn_delta_default_editor(pool);\n> +\n> +  de->open_root = open_root;\n> +  de->delete_entry = delete_entry;\n> +  de->add_directory = add_directory;\n> +  de->open_directory = open_directory;\n> +  de->close_directory = close_directory;\n> +  de->change_dir_prop = change_dir_prop;\n> +  de->change_file_prop = change_file_prop;\n> +  de->apply_textdelta = apply_textdelta;\n> +  de->add_file = add_file;\n> +  de->open_file = open_file;\n> +  de->close_file = close_file;\n> +  de->close_edit = close_edit;\n> +\n> +  /* Set the edit_baton and editor */\n> +  *edit_baton = eb;\n> +  *editor = de;\n> +\n> +  return SVN_NO_ERROR;\n> +}\n> Index: svnrdump/svnrdump.c\n> ===================================================================\n> --- svnrdump/svnrdump.c\t(revision 0)\n> +++ svnrdump/svnrdump.c\t(working copy)\n> @@ -0,0 +1,204 @@\n> +/*\n> + * ====================================================================\n> + *    Licensed to the Apache Software Foundation (ASF) under one\n> + *    or more contributor license agreements.  See the NOTICE file\n> + *    distributed with this work for additional information\n> + *    regarding copyright ownership.  The ASF licenses this file\n> + *    to you under the Apache License, Version 2.0 (the\n> + *    \"License\"); you may not use this file except in compliance\n> + *    with the License.  You may obtain a copy of the License at\n> + *\n> + *      http://www.apache.org/licenses/LICENSE-2.0\n> + *\n> + *    Unless required by applicable law or agreed to in writing,\n> + *    software distributed under the License is distributed on an\n> + *    \"AS IS\" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY\n> + *    KIND, either express or implied.  See the License for the\n> + *    specific language governing permissions and limitations\n> + *    under the License.\n> + * ====================================================================\n> + */\n> +\n> +#include \"svn_pools.h\"\n> +#include \"svn_cmdline.h\"\n> +#include \"svn_client.h\"\n> +#include \"svn_ra.h\"\n> +#include \"svn_repos.h\"\n> +#include \"svn_path.h\"\n> +\n> +#include \"svnrdump.h\"\n> +#include \"dump_editor.h\"\n> +\n> +static int verbose = 0;\n> +static apr_pool_t *pool = NULL;\n> +static svn_client_ctx_t *ctx = NULL;\n\nYou're only using the client context in open_connection.\nMake it a local variable there?\n\n> +static svn_ra_session_t *session = NULL;\n> +\n> +static svn_error_t *\n> +replay_revstart(svn_revnum_t revision,\n> +                void *replay_baton,\n> +                const svn_delta_editor_t **editor,\n> +                void **edit_baton,\n> +                apr_hash_t *rev_props,\n> +                apr_pool_t *pool)\n> +{\n> +  /* Editing this revision has just started; dump the revprops\n> +     before invoking the editor callbacks */\n> +  svn_stringbuf_t *propstring = svn_stringbuf_create(\"\", pool);\n> +  svn_stream_t *stdout_stream;\n> +\n> +  /* Create an stdout stream */\n> +  svn_stream_for_stdout(&stdout_stream, pool);\n> +\n> +        /* Print revision number and prepare the propstring */\n> +  SVN_ERR(svn_stream_printf(stdout_stream, pool,\n> +          SVN_REPOS_DUMPFILE_REVISION_NUMBER\n> +          \": %ld\\n\", revision));\n> +  write_hash_to_stringbuf(rev_props, FALSE, &propstring, pool);\n> +  svn_stringbuf_appendbytes(propstring, \"PROPS-END\\n\", 10);\n> +\n> +  /* prop-content-length header */\n> +  SVN_ERR(svn_stream_printf(stdout_stream, pool,\n> +          SVN_REPOS_DUMPFILE_PROP_CONTENT_LENGTH\n> +          \": %\" APR_SIZE_T_FMT \"\\n\", propstring->len));\n> +\n> +  /* content-length header */\n> +  SVN_ERR(svn_stream_printf(stdout_stream, pool,\n> +          SVN_REPOS_DUMPFILE_CONTENT_LENGTH\n> +          \": %\" APR_SIZE_T_FMT \"\\n\\n\", propstring->len));\n> +\n> +  /* Print the revprops now */\n> +  SVN_ERR(svn_stream_write(stdout_stream, propstring->data,\n> +         &(propstring->len)));\n> +\n> +  svn_stream_close(stdout_stream);\n> +\n> +  /* Extract editor and editor_baton from the replay_baton and\n> +     set them so that the editor callbacks can use them */\n> +  struct replay_baton *rb = replay_baton;\n> +  *editor = rb->editor;\n> +  *edit_baton = rb->edit_baton;\n> +\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +replay_revend(svn_revnum_t revision,\n> +              void *replay_baton,\n> +              const svn_delta_editor_t *editor,\n> +              void *edit_baton,\n> +              apr_hash_t *rev_props,\n> +              apr_pool_t *pool)\n> +{\n> +  /* Editor has finished for this revision and close_edit has\n> +     been called; do nothing: just continue to the next\n> +     revision */\n> +  if (verbose)\n> +    fprintf(stderr, \"* Dumped revision %lu\\n\", revision);\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +open_connection(const char *url)\n> +{\n> +  SVN_ERR(svn_config_ensure (NULL, pool));\n> +  SVN_ERR(svn_client_create_context (&ctx, pool));\n> +  SVN_ERR(svn_ra_initialize(pool));\n> +\n> +  SVN_ERR(svn_config_get_config(&(ctx->config), NULL, pool));\n> +\n> +  /* Default authentication providers for non-interactive use */\n> +  SVN_ERR(svn_cmdline_create_auth_baton(&(ctx->auth_baton), TRUE,\n> +                NULL, NULL, NULL, FALSE,\n> +                FALSE, NULL, NULL, NULL,\n> +                pool));\n> +  SVN_ERR(svn_client_open_ra_session(&session, url, ctx, pool));\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +replay_range(svn_revnum_t start_revision, svn_revnum_t end_revision)\n> +{\n> +  const svn_delta_editor_t *dump_editor;\n> +  void *dump_baton;\n> +\n> +  SVN_ERR(get_dump_editor(&dump_editor,\n> +                          &dump_baton, start_revision, pool));\n> +\n> +  struct replay_baton *replay_baton = apr_palloc(pool,\n> +\t\t\t\t\t\t sizeof(struct replay_baton));\n\nmore tabs\n\n> +  replay_baton->editor = dump_editor;\n> +  replay_baton->edit_baton = dump_baton;\n> +  SVN_ERR(svn_cmdline_printf(pool, SVN_REPOS_DUMPFILE_MAGIC_HEADER \": %d\\n\",\n> +           SVN_REPOS_DUMPFILE_FORMAT_VERSION));\n> +  SVN_ERR(svn_ra_replay_range(session, start_revision, end_revision,\n> +                              0, TRUE, replay_revstart, replay_revend,\n> +                              replay_baton, pool));\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +static svn_error_t *\n> +usage(FILE *out_stream)\n> +{\n> +  fprintf(out_stream,\n\nUse svn_cmdline_fprintf()\n\n> +    \"usage: svnrdump URL [-r LOWER[:UPPER]]\\n\\n\"\n\nThis string needs to be marked for localisation like this: _(\"my string\")\n\n> +    \"Dump the contents of repository at remote URL to stdout in a 'dumpfile'\\n\"\n> +    \"v3 portable format.  Dump revisions LOWER rev through UPPER rev.\\n\"\n\nYou don't need to mention the dumpfile format version in the help\nstring.\n\n> +    \"LOWER defaults to 1 and UPPER defaults to the highest possible revision\\n\"\n> +    \"if omitted.\\n\");\n> +  return SVN_NO_ERROR;\n> +}\n> +\n> +int\n> +main(int argc, const char **argv)\n> +{\n> +  int i;\n> +  const char *url = NULL;\n> +  char *revision_cut = NULL;\n> +  svn_revnum_t start_revision = svn_opt_revision_unspecified;\n> +  svn_revnum_t end_revision = svn_opt_revision_unspecified;\n> +\n> +  if (svn_cmdline_init (\"svnrdump\", stderr) != EXIT_SUCCESS)\n> +    return EXIT_FAILURE;\n> +\n> +  pool = svn_pool_create(NULL);\n> +\n> +  for (i = 1; i < argc; i++) {\n\nPlease use svn_cmdline__getopt_init() and apr_getopt_long().\nSee svnsync for an example.\n\n> +    if (!strncmp(\"-r\", argv[i], 2)) {\n> +      revision_cut = strchr(argv[i] + 2, ':');\n> +      if (revision_cut) {\n> +        start_revision = (svn_revnum_t) strtoul(argv[i] + 2, &revision_cut, 10);\n> +        end_revision = (svn_revnum_t) strtoul(revision_cut + 1, NULL, 10);\n> +      }\n> +      else\n> +        start_revision = (svn_revnum_t) strtoul(argv[i] + 2, NULL, 10);\n> +    } else if (!strcmp(\"-v\", argv[i]) || !strcmp(\"--verbose\", argv[i])) {\n> +      verbose = 1;\n> +    } else if (!strcmp(\"help\", argv[i]) || !strcmp(\"--help\", argv[i])) {\n> +      SVN_INT_ERR(usage(stdout));\n> +      return EXIT_SUCCESS;\n> +    } else if (*argv[i] == '-' || url) {\n> +      SVN_INT_ERR(usage(stderr));\n> +      return EXIT_FAILURE;\n> +    } else\n> +      url = argv[i];\n> +  }\n> +\n> +  if (!url || !svn_path_is_url(url)) {\n> +    usage(stderr);\n> +    return EXIT_FAILURE;\n> +  }\n> +  SVN_INT_ERR(open_connection(url));\n> +\n> +  /* Have sane start_revision and end_revision defaults if unspecified */\n> +  if (start_revision == svn_opt_revision_unspecified)\n> +    start_revision = 1;\n> +  if (end_revision == svn_opt_revision_unspecified)\n> +    SVN_INT_ERR(svn_ra_get_latest_revnum(session, &end_revision, pool));\n> +\n> +  SVN_INT_ERR(replay_range(start_revision, end_revision));\n> +\n> +  svn_pool_destroy(pool);\n> +\n> +  return 0;\n> +}\n> Index: svnrdump/svnrdump.h\n> ===================================================================\n> --- svnrdump/svnrdump.h\t(revision 0)\n> +++ svnrdump/svnrdump.h\t(working copy)\n> @@ -0,0 +1,44 @@\n> +/*\n> + * ====================================================================\n> + *    Licensed to the Apache Software Foundation (ASF) under one\n> + *    or more contributor license agreements.  See the NOTICE file\n> + *    distributed with this work for additional information\n> + *    regarding copyright ownership.  The ASF licenses this file\n> + *    to you under the Apache License, Version 2.0 (the\n> + *    \"License\"); you may not use this file except in compliance\n> + *    with the License.  You may obtain a copy of the License at\n> + *\n> + *      http://www.apache.org/licenses/LICENSE-2.0\n> + *\n> + *    Unless required by applicable law or agreed to in writing,\n> + *    software distributed under the License is distributed on an\n> + *    \"AS IS\" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY\n> + *    KIND, either express or implied.  See the License for the\n> + *    specific language governing permissions and limitations\n> + *    under the License.\n> + * ====================================================================\n> + */\n> +\n> +#ifndef SVNRDUMP_H_\n> +#define SVNRDUMP_H_\n> +\n> +#include \"dump_editor.h\"\n> +\n> +struct replay_baton {\n> +  const svn_delta_editor_t *editor;\n> +  void *edit_baton;\n> +};\n> +\n\nPlease add a docstring.\n\n> +void\n> +write_hash_to_stringbuf(apr_hash_t *properties,\n> +                        svn_boolean_t deleted,\n> +                        svn_stringbuf_t **strbuf,\n> +                        apr_pool_t *pool);\n> +\n\nPlease add a docstring.\n\n> +svn_error_t *\n> +dump_props(struct dump_edit_baton *eb,\n> +           svn_boolean_t *trigger_var,\n> +           svn_boolean_t dump_data_too,\n> +           apr_pool_t *pool);\n> +\n> +#endif\n> Index: svnrdump/util.c\n> ===================================================================\n> --- svnrdump/util.c\t(revision 0)\n> +++ svnrdump/util.c\t(working copy)\n> @@ -0,0 +1,131 @@\n> +/*\n> + * ====================================================================\n> + *    Licensed to the Apache Software Foundation (ASF) under one\n> + *    or more contributor license agreements.  See the NOTICE file\n> + *    distributed with this work for additional information\n> + *    regarding copyright ownership.  The ASF licenses this file\n> + *    to you under the Apache License, Version 2.0 (the\n> + *    \"License\"); you may not use this file except in compliance\n> + *    with the License.  You may obtain a copy of the License at\n> + *\n> + *      http://www.apache.org/licenses/LICENSE-2.0\n> + *\n> + *    Unless required by applicable law or agreed to in writing,\n> + *    software distributed under the License is distributed on an\n> + *    \"AS IS\" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY\n> + *    KIND, either express or implied.  See the License for the\n> + *    specific language governing permissions and limitations\n> + *    under the License.\n> + * ====================================================================\n> + */\n> +\n> +#include \"svn_pools.h\"\n> +#include \"svn_cmdline.h\"\n> +#include \"svn_client.h\"\n> +#include \"svn_ra.h\"\n> +#include \"svn_repos.h\"\n\nAre all these includes really needed?\n\n> +\n> +#include \"svnrdump.h\"\n> +\n> +void\n> +write_hash_to_stringbuf(apr_hash_t *properties,\n> +                        svn_boolean_t deleted,\n> +                        svn_stringbuf_t **strbuf,\n> +                        apr_pool_t *pool)\n> +{\n\nThis function needs a docstring, too.\n\nAnd is there no function that already does this somewhere in the svn\nlibraries or in APR?\n\n> +  apr_hash_index_t *this;\n> +  const void *key;\n> +  void *val;\n> +  apr_ssize_t keylen;\n> +  svn_string_t *value;\n> +\n> +  if (!deleted) {\n> +    for (this = apr_hash_first(pool, properties); this;\n> +         this = apr_hash_next(this)) {\n> +      /* Get this key and val. */\n> +      apr_hash_this(this, &key, &keylen, &val);\n> +      value = val;\n> +\n> +      /* Output name length, then name. */\n> +      svn_stringbuf_appendcstr(*strbuf,\n> +             apr_psprintf(pool, \"K %\" APR_SSIZE_T_FMT \"\\n\",\n> +                    keylen));\n> +\n> +      svn_stringbuf_appendbytes(*strbuf, (const char *) key, keylen);\n> +      svn_stringbuf_appendbytes(*strbuf, \"\\n\", 1);\n> +\n> +      /* Output value length, then value. */\n> +      svn_stringbuf_appendcstr(*strbuf,\n> +             apr_psprintf(pool, \"V %\" APR_SIZE_T_FMT \"\\n\",\n> +                    value->len));\n> +\n> +      svn_stringbuf_appendbytes(*strbuf, value->data, value->len);\n> +      svn_stringbuf_appendbytes(*strbuf, \"\\n\", 1);\n> +    }\n> +  }\n> +  else {\n> +    /* Output a \"D \" entry for each deleted property */\n> +    for (this = apr_hash_first(pool, properties); this;\n> +         this = apr_hash_next(this)) {\n> +      /* Get this key */\n> +      apr_hash_this(this, &key, &keylen, NULL);\n> +\n> +      /* Output name length, then name */\n> +      svn_stringbuf_appendcstr(*strbuf,\n> +             apr_psprintf(pool, \"D %\" APR_SSIZE_T_FMT \"\\n\",\n> +                    keylen));\n> +\n> +      svn_stringbuf_appendbytes(*strbuf, (const char *) key, keylen);\n> +      svn_stringbuf_appendbytes(*strbuf, \"\\n\", 1);\n> +    }\n> +  }\n> +}\n> +\n> +svn_error_t *\n> +dump_props(struct dump_edit_baton *eb,\n> +           svn_boolean_t *trigger_var,\n> +           svn_boolean_t dump_data_too,\n> +           apr_pool_t *pool)\n> +{\n> +  if (trigger_var && !*trigger_var)\n> +    return SVN_NO_ERROR;\n> +\n> +  /* Build a propstring to print */\n> +  svn_stringbuf_setempty(eb->propstring);\n> +  write_hash_to_stringbuf(eb->properties,\n> +        FALSE,\n> +        &(eb->propstring), eb->pool);\n> +  write_hash_to_stringbuf(eb->del_properties,\n> +        TRUE,\n> +        &(eb->propstring), eb->pool);\n> +  svn_stringbuf_appendbytes(eb->propstring, \"PROPS-END\\n\", 10);\n> +\n> +  /* prop-delta header */\n> +  SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +          SVN_REPOS_DUMPFILE_PROP_DELTA\n> +          \": true\\n\"));\n> +\n> +  /* prop-content-length header */\n> +  SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +          SVN_REPOS_DUMPFILE_PROP_CONTENT_LENGTH\n> +          \": %\" APR_SIZE_T_FMT \"\\n\", eb->propstring->len));\n> +\n> +  if (dump_data_too) {\n> +    /* content-length header */\n> +    SVN_ERR(svn_stream_printf(eb->stream, pool,\n> +            SVN_REPOS_DUMPFILE_CONTENT_LENGTH\n> +            \": %\" APR_SIZE_T_FMT \"\\n\\n\",\n> +            eb->propstring->len));\n> +\n> +    /* the properties themselves */\n> +    SVN_ERR(svn_stream_write(eb->stream, eb->propstring->data,\n> +           &(eb->propstring->len)));\n> +\n> +    /* Cleanup so that data is never dumped twice */\n> +    apr_hash_clear(eb->properties);\n> +    apr_hash_clear(eb->del_properties);\n> +    if (trigger_var)\n> +      *trigger_var = FALSE;\n> +  }\n> +  return SVN_NO_ERROR;\n> +}\n"},{"id":"145545","messageId":"20100714153206.GH25630@jack.stsp.name","threadId":"24349","inReplyTo":"20100713201105.GN13310@ted.stsp.name","subject":"Re: [PATCH v2] Add svnrdump","fromName":"Stefan Sperling","fromEmail":"stsp@elego.de","sentAt":"2010-07-14T15:32:06Z","receivedAt":"2010-07-14T15:32:06Z","isPatch":true,"sender":{"key":"stsp@elego.de","avatar":"https://avatars.githubusercontent.com/u/9281333?v=4"},"body":"On Tue, Jul 13, 2010 at 10:11:05PM +0200, Stefan Sperling wrote:\n> Review below.\n\nA couple of additional remarks:\n\nPlaying with svnrdump and comparing its output to the output of\nsvnadmin dump --deltas, I noticed that:\n\n - svnrdump doesn't dump revision 0.\n   It should dump revision 0, because that revision can contain important\n   revprops such as metadata for svnsync (svn:sync-last-merge-rev etc.)\n - You're missing a couple of fields:\n   The UUID of the repository.\n   Text-content-sha1\n   Text-delta-base-md5\n   Text-delta-base-sha1\n - I've seen a \"Prop-delta: true\" line which svnadmin dump does not print.\n - You're missing some newlines that svnadmin dump prints (cosmetic,\n   but it would be nice if both produced matching output).\n\nHow to reproduce what I'm seeing:\n  Use svnsync to get a copy of the numptyphysics repository at\n    https://vcs.maemo.org/svn/numptyphysics (I had a dump of that lying\n    around... other repositories might do the job just as well, of course)\n  Dump the repository using svnadmin dump --deltas.\n  Dump the repository using svnrdump.\n  Compare output with diff -u.\n\nPlease get rid of all global variables in svnrdump.c:\nsubversion/svnrdump/svnrdump.c:43: warning: declaration of `pool' shadows a glob\nal declaration\nsubversion/svnrdump/svnrdump.c:33: warning: shadowed declaration is here\nsubversion/svnrdump/svnrdump.c:91: warning: declaration of `pool' shadows a glob\nal declaration\nsubversion/svnrdump/svnrdump.c:33: warning: shadowed declaration is here\n\nWhen adding unit tests for svnrdump, please make each and every one of\nthose tests compare with output of svnadmin dump --deltas, so that we\nwill keep them in sync.\n\nThanks,\nStefan\n"},{"id":"145549","messageId":"20100714160149.GA7561@debian","threadId":"24349","inReplyTo":"20100714153206.GH25630@jack.stsp.name","subject":"Re: [PATCH v2] Add svnrdump","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2010-07-14T16:01:49Z","receivedAt":"2010-07-14T16:01:49Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Stefan,\n\nStefan Sperling writes:\n> Playing with svnrdump and comparing its output to the output of\n> svnadmin dump --deltas, I noticed that:\n\nThanks for testing!\n\n>  - svnrdump doesn't dump revision 0.\n>    It should dump revision 0, because that revision can contain important\n>    revprops such as metadata for svnsync (svn:sync-last-merge-rev etc.)\n\nYeah, I forgot to ask about this: passing 0 as an argument to the\nreplay API doesn't seem to work. Why? How do I dump revision 0 then?\n\n>  - You're missing a couple of fields:\n>    The UUID of the repository.\n>    Text-content-sha1\n>    Text-delta-base-md5\n>    Text-delta-base-sha1\n\nYes, I'm aware. Since these fields aren't strictly necessary, I\ndecided not to take the extra effort to print them out: you'll notice\nthat I'm printing the md5 sum that the server gives me instead of\ncalculating anything. SHA1 sum would require /some/ calculation. UUID\nand text-delta-base-md5 aren't a big deal though: I'll fix these\nlater.\n\n>  - I've seen a \"Prop-delta: true\" line which svnadmin dump does not print.\n\nCorrect. `svnadmin dump` has a logic for determining when the prop is\nreally a delta (as opposed to a delta against /dev/null). Since\nthere's no harm printing extra Prop-delta headers, I decided not to\nimplement this logic.\n\n>  - You're missing some newlines that svnadmin dump prints (cosmetic,\n>    but it would be nice if both produced matching output).\n\nThis isn't in the dump-load-format spec document (atleast afaik), and\nit's very hard to get this right (yes, I tried). Moreover, it's very\nungratifying to have a few extra newlines (reverse engineered from\n`svnadmin dump`) printed at the end of 10+ hrs of work; yes, that's\nwhat I estimate it'll take to fix this.\n\n> How to reproduce what I'm seeing:\n>   Use svnsync to get a copy of the numptyphysics repository at\n>     https://vcs.maemo.org/svn/numptyphysics (I had a dump of that lying\n>     around... other repositories might do the job just as well, of course)\n>   Dump the repository using svnadmin dump --deltas.\n>   Dump the repository using svnrdump.\n>   Compare output with diff -u.\n\nRight. My validation script (validate.sh in the original repository)\nruns the following filter on the diff and validates if nothing seeps\nthrough. In other words, I know that these differences exist, and have\ndetermined that they're safe.\n\ngawk '$0 !~ \"Prop-delta: true|Text-delta-base-|sha1|Text-copy-source-|^-$\" && $0 ~ \"^+|^-\" { print; }'\n\n> Please get rid of all global variables in svnrdump.c:\n> subversion/svnrdump/svnrdump.c:43: warning: declaration of `pool' shadows a glob\n> al declaration\n> subversion/svnrdump/svnrdump.c:33: warning: shadowed declaration is here\n> subversion/svnrdump/svnrdump.c:91: warning: declaration of `pool' shadows a glob\n> al declaration\n> subversion/svnrdump/svnrdump.c:33: warning: shadowed declaration is here\n\nWill do. I'm waiting for commit access, because I don't want to make\nun-versioned edits to the file that I cannot track or revert in\nfuture.\n\n> When adding unit tests for svnrdump, please make each and every one of\n> those tests compare with output of svnadmin dump --deltas, so that we\n> will keep them in sync.\n\nRight. Please see the current `validate.sh` for an example of the\nfunctionality I'll write into the unit tests.\n\n-- Ram\n"},{"id":"145557","messageId":"4C3DEA4B.9090507@collab.net","threadId":"24349","inReplyTo":"20100714160149.GA7561@debian","subject":"Re: [PATCH v2] Add svnrdump","fromName":"C. Michael Pilato","fromEmail":"cmpilato@collab.net","sentAt":"2010-07-14T16:48:11Z","receivedAt":"2010-07-14T16:48:11Z","isPatch":true,"sender":{"key":"cmpilato@collab.net","avatar":"https://gravatar.com/avatar/c446855973de5b2ecf4677cd8c8f5a87901eb4bb7e0080350ce28d9558ad9ceb?d=mp&s=160"},"body":"On 07/14/2010 12:01 PM, Ramkumar Ramachandra wrote:\n> Hi Stefan,\n> \n> Stefan Sperling writes:\n>> Playing with svnrdump and comparing its output to the output of\n>> svnadmin dump --deltas, I noticed that:\n> \n> Thanks for testing!\n> \n>>  - svnrdump doesn't dump revision 0.\n>>    It should dump revision 0, because that revision can contain important\n>>    revprops such as metadata for svnsync (svn:sync-last-merge-rev etc.)\n> \n> Yeah, I forgot to ask about this: passing 0 as an argument to the\n> replay API doesn't seem to work. Why? How do I dump revision 0 then?\n\nYou fake it, just like the code behind 'svnadmin dump' does.  :-)\nSeriously, Revision 0 is nothing but a revision header and revision\nproperties.  There is no node data to transmit.\n\n-- \nC. Michael Pilato <cmpilato@collab.net>\nCollabNet   <>   www.collab.net   <>   Distributed Development On Demand\n"},{"id":"145566","messageId":"20100714172429.GC25861@ted.stsp.name","threadId":"24349","inReplyTo":"20100714160149.GA7561@debian","subject":"Re: [PATCH v2] Add svnrdump","fromName":"Stefan Sperling","fromEmail":"stsp@elego.de","sentAt":"2010-07-14T17:24:29Z","receivedAt":"2010-07-14T17:24:29Z","isPatch":true,"sender":{"key":"stsp@elego.de","avatar":"https://avatars.githubusercontent.com/u/9281333?v=4"},"body":"On Wed, Jul 14, 2010 at 06:01:49PM +0200, Ramkumar Ramachandra wrote:\n> Yeah, I forgot to ask about this: passing 0 as an argument to the\n> replay API doesn't seem to work. Why? How do I dump revision 0 then?\n\nIndeed. This seems to be a problem in the replay API.\nThis is not a problem for svnsync itself because svnsync manually\nsets the revision properties while doing a sync.\nWe can fix the replay API to allow svnrdump to get revprops for r0.\n \n> >  - You're missing a couple of fields:\n> >    The UUID of the repository.\n> >    Text-content-sha1\n> >    Text-delta-base-md5\n> >    Text-delta-base-sha1\n> \n> Yes, I'm aware.\n\nOK.\n \n> >  - I've seen a \"Prop-delta: true\" line which svnadmin dump does not print.\n> \n> Correct. `svnadmin dump` has a logic for determining when the prop is\n> really a delta (as opposed to a delta against /dev/null). Since\n> there's no harm printing extra Prop-delta headers, I decided not to\n> implement this logic.\n\nWe can fix this later.\n\n> >  - You're missing some newlines that svnadmin dump prints (cosmetic,\n> >    but it would be nice if both produced matching output).\n> \n> This isn't in the dump-load-format spec document (atleast afaik), and\n> it's very hard to get this right (yes, I tried). Moreover, it's very\n> ungratifying to have a few extra newlines (reverse engineered from\n> `svnadmin dump`) printed at the end of 10+ hrs of work; yes, that's\n> what I estimate it'll take to fix this.\n\nWell, it would be really nice to have.\nDetails like this are time sinks, I know. But it pays off.\nYou don't have to do it right away. We can file an issue so we don't\nforget about fixing it before 1.7 release.\nIf necessary, feel free to adjust the output of svnadmin dump a little\nif that makes it easier for svnrdump to produce matching output.\n\n> gawk '$0 !~ \"Prop-delta: true|Text-delta-base-|sha1|Text-copy-source-|^-$\" && $0 ~ \"^+|^-\" { print; }'\n\nFine for testing. But I still think the end-result should look just\nlike svnadmin dump, if possible. That would make testing even easier.\n\n> > Please get rid of all global variables in svnrdump.c:\n> Will do. I'm waiting for commit access, because I don't want to make\n> un-versioned edits to the file that I cannot track or revert in\n> future.\n\nWhat about using git until then? It does not matter which state you\ninitially import into the Subversion repository. But well, whatever\nworks for you is best.\n \n> Please see the current `validate.sh` for an example of the\n> functionality I'll write into the unit tests.\n\nThanks, I'll take a look.\n\nStefan\n"},{"id":"145567","messageId":"4C3DF456.20803@collab.net","threadId":"24349","inReplyTo":"20100714172429.GC25861@ted.stsp.name","subject":"Re: [PATCH v2] Add svnrdump","fromName":"C. Michael Pilato","fromEmail":"cmpilato@collab.net","sentAt":"2010-07-14T17:31:02Z","receivedAt":"2010-07-14T17:31:02Z","isPatch":true,"sender":{"key":"cmpilato@collab.net","avatar":"https://gravatar.com/avatar/c446855973de5b2ecf4677cd8c8f5a87901eb4bb7e0080350ce28d9558ad9ceb?d=mp&s=160"},"body":"On 07/14/2010 01:24 PM, Stefan Sperling wrote:\n> On Wed, Jul 14, 2010 at 06:01:49PM +0200, Ramkumar Ramachandra wrote:\n>> Yeah, I forgot to ask about this: passing 0 as an argument to the\n>> replay API doesn't seem to work. Why? How do I dump revision 0 then?\n> \n> Indeed. This seems to be a problem in the replay API.\n\nHow so?  The replay API is for driving an editor with tree changes.  There\nare by definition no tree changes in revision 0.  Therefore, it makes no\nsense to accept revision 0 as valid.\n\nRevprops aren't handled by the replay API for any revision.\n\n-- \nC. Michael Pilato <cmpilato@collab.net>\nCollabNet   <>   www.collab.net   <>   Distributed Development On Demand\n"},{"id":"145568","messageId":"20100714173409.GD25861@ted.stsp.name","threadId":"24349","inReplyTo":"4C3DF456.20803@collab.net","subject":"Re: [PATCH v2] Add svnrdump","fromName":"Stefan Sperling","fromEmail":"stsp@elego.de","sentAt":"2010-07-14T17:34:09Z","receivedAt":"2010-07-14T17:34:09Z","isPatch":true,"sender":{"key":"stsp@elego.de","avatar":"https://avatars.githubusercontent.com/u/9281333?v=4"},"body":"On Wed, Jul 14, 2010 at 01:31:02PM -0400, C. Michael Pilato wrote:\n> Revprops aren't handled by the replay API for any revision.\n\nAh, I didn't know that.\nI was assuming they were transmitted via the replay API but I didn't check.\n\nThanks,\nStefan\n"},{"id":"145570","messageId":"20100714174716.GB2866@burratino","threadId":"24349","inReplyTo":"4C3DF456.20803@collab.net","subject":"Re: [PATCH v2] Add svnrdump","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-14T17:47:16Z","receivedAt":"2010-07-14T17:47:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"C. Michael Pilato wrote:\n\n> Revprops aren't handled by the replay API for any revision.\n\nHmm?  What is the rev_props argument to the\n\ntypedef svn_error_t (*svn_ra_replay_revstart_callback_t)(\n\t\t\t\tsvn_revnum_t revision,\n\t\t\t\tvoid *replay_baton,\n\t\t\t\tconst svn_delta_editor_t **editor,\n\t\t\t\tvoid **edit_baton,\n\t\t\t\tapr_hash_t *rev_props,\n\t\t\t\tapr_pool_t *pool)\n\ncallback for, then?\n\nUsing svn_ra_rev_prop() for rev 0 does seem simple enough, though.\n"},{"id":"145572","messageId":"4C3DFA3E.7000809@collab.net","threadId":"24349","inReplyTo":"20100714174716.GB2866@burratino","subject":"Re: [PATCH v2] Add svnrdump","fromName":"C. Michael Pilato","fromEmail":"cmpilato@collab.net","sentAt":"2010-07-14T17:56:14Z","receivedAt":"2010-07-14T17:56:14Z","isPatch":true,"sender":{"key":"cmpilato@collab.net","avatar":"https://gravatar.com/avatar/c446855973de5b2ecf4677cd8c8f5a87901eb4bb7e0080350ce28d9558ad9ceb?d=mp&s=160"},"body":"On 07/14/2010 01:47 PM, Jonathan Nieder wrote:\n> C. Michael Pilato wrote:\n> \n>> Revprops aren't handled by the replay API for any revision.\n> \n> Hmm?  What is the rev_props argument to the\n> \n> typedef svn_error_t (*svn_ra_replay_revstart_callback_t)(\n> \t\t\t\tsvn_revnum_t revision,\n> \t\t\t\tvoid *replay_baton,\n> \t\t\t\tconst svn_delta_editor_t **editor,\n> \t\t\t\tvoid **edit_baton,\n> \t\t\t\tapr_hash_t *rev_props,\n> \t\t\t\tapr_pool_t *pool)\n> \n> callback for, then?\n> \n> Using svn_ra_rev_prop() for rev 0 does seem simple enough, though.\n\nAh, I was talking about the svn_repos_replay() API (which is used by the\nsvn_ra_replay() API).  It definitely makes sense to me that the RA's replay\nAPI be able to report the revprops for revision 0.\n\nSorry for the confusion.\n\n-- \nC. Michael Pilato <cmpilato@collab.net>\nCollabNet   <>   www.collab.net   <>   Distributed Development On Demand\n"},{"id":"145577","messageId":"000101cb238a$5b2bfea0$1183fbe0$@collab.net","threadId":"24349","inReplyTo":"20100714160149.GA7561@debian","subject":"RE: [PATCH v2] Add svnrdump","fromName":"Bert Huijben","fromEmail":"rhuijben@collab.net","sentAt":"2010-07-14T19:25:39Z","receivedAt":"2010-07-14T19:25:39Z","isPatch":true,"sender":{"key":"rhuijben@collab.net","avatar":null},"body":"\n\n> -----Original Message-----\n> From: Ramkumar Ramachandra [mailto:artagnon@gmail.com]\n> Sent: woensdag 14 juli 2010 18:02\n> To: Stefan Sperling\n> Cc: dev@subversion.apache.org; Bert Huijben; Daniel Shahaf; Will Palmer;\n> David Michael Barr; Jonathan Nieder; Sverre Rabbelier; Git Mailing List\n> Subject: Re: [PATCH v2] Add svnrdump\n> \n> Hi Stefan,\n> \n> Stefan Sperling writes:\n> > Playing with svnrdump and comparing its output to the output of\n> > svnadmin dump --deltas, I noticed that:\n> \n> Thanks for testing!\n> \n> >  - svnrdump doesn't dump revision 0.\n> >    It should dump revision 0, because that revision can contain\nimportant\n> >    revprops such as metadata for svnsync (svn:sync-last-merge-rev etc.)\n> \n> Yeah, I forgot to ask about this: passing 0 as an argument to the\n> replay API doesn't seem to work. Why? How do I dump revision 0 then?\n> \n> >  - You're missing a couple of fields:\n> >    The UUID of the repository.\n> >    Text-content-sha1\n> >    Text-delta-base-md5\n> >    Text-delta-base-sha1\n> \n> Yes, I'm aware. Since these fields aren't strictly necessary, I\n> decided not to take the extra effort to print them out: you'll notice\n> that I'm printing the md5 sum that the server gives me instead of\n> calculating anything. SHA1 sum would require /some/ calculation. UUID\n> and text-delta-base-md5 aren't a big deal though: I'll fix these\n> later.\n\nWe added the sha1 field to the format in 1.6, as we have it available in the\nfs layer anyway and we might want to prefer it over md5 in later version\n(for future proofing the dump format). It's not used by our tools yet and we\ndon't send the value over the ra layer yet. Without revving the editor layer\nit will be pretty hard to calculate it remotely even from the most recent\nrepositories.\n\nI assume you don't get the SHA1 values when you use a recent svnadmin dump\non an older repository. (Untested statement)\n\n \n> >  - I've seen a \"Prop-delta: true\" line which svnadmin dump does not\nprint.\n> \n> Correct. `svnadmin dump` has a logic for determining when the prop is\n> really a delta (as opposed to a delta against /dev/null). Since\n> there's no harm printing extra Prop-delta headers, I decided not to\n> implement this logic.\n\nDo you know if this is this something as simple as: 'Is this a new node?' or\nif this is some advanced scheme?\n> \n> >  - You're missing some newlines that svnadmin dump prints (cosmetic,\n> >    but it would be nice if both produced matching output).\n> \n> This isn't in the dump-load-format spec document (atleast afaik), and\n> it's very hard to get this right (yes, I tried). Moreover, it's very\n> ungratifying to have a few extra newlines (reverse engineered from\n> `svnadmin dump`) printed at the end of 10+ hrs of work; yes, that's\n> what I estimate it'll take to fix this.\n> \n> > How to reproduce what I'm seeing:\n> >   Use svnsync to get a copy of the numptyphysics repository at\n> >     https://vcs.maemo.org/svn/numptyphysics (I had a dump of that lying\n> >     around... other repositories might do the job just as well, of\ncourse)\n> >   Dump the repository using svnadmin dump --deltas.\n> >   Dump the repository using svnrdump.\n> >   Compare output with diff -u.\n> \n> Right. My validation script (validate.sh in the original repository)\n> runs the following filter on the diff and validates if nothing seeps\n> through. In other words, I know that these differences exist, and have\n> determined that they're safe.\n> \n> gawk '$0 !~ \"Prop-delta: true|Text-delta-base-|sha1|Text-copy-source-|^-\n> $\" && $0 ~ \"^+|^-\" { print; }'\n\nYour mail explains Prop-delta, sha1, but what about these Text-delta-base\nand Text-copy-source lines?\n\n\tBert\n"},{"id":"145617","messageId":"20100715102833.GB22574@debian","threadId":"24349","inReplyTo":"4C3DEA4B.9090507@collab.net","subject":"Re: [PATCH v2] Add svnrdump","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2010-07-15T10:28:33Z","receivedAt":"2010-07-15T10:28:33Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Michael,\n\nC. Michael Pilato writes:\n> You fake it, just like the code behind 'svnadmin dump' does.  :-)\n> Seriously, Revision 0 is nothing but a revision header and revision\n> properties.  There is no node data to transmit.\n\nRight, thanks. Will look at the code behind `svnadmin dump` :)\n\n-- Ram\n"},{"id":"145621","messageId":"20100715120143.GE22574@debian","threadId":"24349","inReplyTo":"20100714172429.GC25861@ted.stsp.name","subject":"Re: [PATCH v2] Add svnrdump","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2010-07-15T12:01:43Z","receivedAt":"2010-07-15T12:01:43Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Stefan,\n\nStefan Sperling writes:\n> > This isn't in the dump-load-format spec document (atleast afaik), and\n> > it's very hard to get this right (yes, I tried). Moreover, it's very\n> > ungratifying to have a few extra newlines (reverse engineered from\n> > `svnadmin dump`) printed at the end of 10+ hrs of work; yes, that's\n> > what I estimate it'll take to fix this.\n> \n> Well, it would be really nice to have.\n> Details like this are time sinks, I know. But it pays off.\n> You don't have to do it right away. We can file an issue so we don't\n> forget about fixing it before 1.7 release.\n> If necessary, feel free to adjust the output of svnadmin dump a little\n> if that makes it easier for svnrdump to produce matching output.\n\nI think the latter is certainly an option. We definitely need to fix\nthe dump-load-format spec to show everything.\n\n> > gawk '$0 !~ \"Prop-delta: true|Text-delta-base-|sha1|Text-copy-source-|^-$\" && $0 ~ \"^+|^-\" { print; }'\n> \n> Fine for testing. But I still think the end-result should look just\n> like svnadmin dump, if possible. That would make testing even easier.\n\nRight. We can use the same test suite and maintenance would become\ninfinitely easier.\n\n> > > Please get rid of all global variables in svnrdump.c:\n> > Will do. I'm waiting for commit access, because I don't want to make\n> > un-versioned edits to the file that I cannot track or revert in\n> > future.\n> \n> What about using git until then? It does not matter which state you\n> initially import into the Subversion repository. But well, whatever\n> works for you is best.\n\nOh, I didn't think it would take this long for my account to get\nactivated. I'll consider using Git to stage for now because I don't\nwant to delay the response to your review.\n\n-- Ram\n"},{"id":"145622","messageId":"20100715120732.GF22574@debian","threadId":"24349","inReplyTo":"000101cb238a$5b2bfea0$1183fbe0$@collab.net","subject":"Re: [PATCH v2] Add svnrdump","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2010-07-15T12:07:32Z","receivedAt":"2010-07-15T12:07:32Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Bert,\n\nBert Huijben writes:\n> > >  - I've seen a \"Prop-delta: true\" line which svnadmin dump does not\n> print.\n> > \n> > Correct. `svnadmin dump` has a logic for determining when the prop is\n> > really a delta (as opposed to a delta against /dev/null). Since\n> > there's no harm printing extra Prop-delta headers, I decided not to\n> > implement this logic.\n> \n> Do you know if this is this something as simple as: 'Is this a new node?' or\n> if this is some advanced scheme?\n\nThe former actually; although the task looks deceptively simple, it's\na little more involved than that: I'm a little worried about messing\nup the node-action-handling logic, as it might break something. Once\nwe have a large server constantly running validations against my\nlatest changes, I can change stuff more confidently.\n\n> > gawk '$0 !~ \"Prop-delta: true|Text-delta-base-|sha1|Text-copy-source-|^-\n> > $\" && $0 ~ \"^+|^-\" { print; }'\n> \n> Your mail explains Prop-delta, sha1, but what about these Text-delta-base\n> and Text-copy-source lines?\n\nThese headers are also not strictly necessary, and I haven't found out\nwhere this information is hidden. I'll dig through the API and find\nout where this information and print it later.\n\n-- Ram\n"},{"id":"145650","messageId":"20100715190220.GI22574@debian","threadId":"24349","inReplyTo":"20100713201105.GN13310@ted.stsp.name","subject":"Re: [PATCH v2] Add svnrdump","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2010-07-15T19:02:20Z","receivedAt":"2010-07-15T19:02:20Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Stefan,\n\nStefan Sperling writes:\n> Review below.\n\nFirst, thanks for the detailed review! I'll be travelling over the\nnext few days starting tomorrow, and didn't want to delay the response\nto your review: I've marked some items \"TODO\" so that I can grep for\nthem when I'm back in India and fix them.\n\n> This diff is needed to build svnrdump as part of svn on Unix:\n> \n> Index: build.conf\n> ===================================================================\n> --- build.conf\t(revision 963733)\n> +++ build.conf\t(working copy)\n> @@ -167,6 +167,13 @@ libs = libsvn_wc libsvn_subr apriconv apr\n>  install = bin\n>  manpages = subversion/svnversion/svnversion.1\n>  \n> +[svnrdump]\n> +description = Subversion remote repository dumper\n> +type = exe\n> +path = subversion/svnrdump\n> +libs = libsvn_client libsvn_ra libvsvn_delta libsvn_subr apr\n> +install = bin\n> +\n>  # Support for GNOME Keyring\n>  [libsvn_auth_gnome_keyring]\n>  description = Subversion GNOME Keyring Library\n\nThanks! Now included.\n\n> Can you include the above bit in your diff, please, and create follow-up\n> diffs relative to the root of a Subversion trunk working copy? Thanks.\n\nAh, I wasn't paying attention. `git diff` always produces diffs\nrelative to the root.\n\n> Please also add a man page similar to the one of svnsync.\n> Even though I don't like the fact that our man pages simply refer to\n> the Subversion book rather than providing a small and useful subset of it,\n> it's good to at least be consistent about it.\n\nFixed.\n\n> > +struct dump_edit_baton {\n> \n> Please add a comment here explaining what the stream is used for,\n> for instance /* The output stream we write the dumpfile to. */\n> \n> > +  svn_stream_t *stream;\n\nFixed.\n\n> > +  svn_revnum_t current_rev;\n> \n> This is only incremented by never used?\n\nFixed.\n\n> > +\n> > +  /* pool is for per-edit-session allocations */\n> > +  apr_pool_t *pool;\n> > +\n> > +  /* Store the properties that changed */\n> > +  apr_hash_t *properties;\n> > +  apr_hash_t *del_properties; /* Value is always 0x1 */\n> \n> Just say \"value is undefined\". Or use an apr_array_header_t.\n\nFixed.\n\n> A comment here saying what propstring is for would be nice.\n> \n> > +  svn_stringbuf_t *propstring;\n\nFixed.\n\n> > +\n> > +  /* Was a copy command issued? */\n> > +  svn_boolean_t is_copy;\n> \n> Copy of what and when? This baton is global for the entire edit...\n> \n> Going through the code, I see that you're using this to indicate to\n> dump_node() whether an add_directory() or add_file() was in fact a copy.\n> Why not remove this field from the struct and add it as a parameter to\n> dump_node instead?\n\nTODO.\n\n> > +\n> > +  /* Path of changed file */\n> > +  const char *changed_path;\n> \n> The changed_path field seems to be unused.\n> \n> According to comments in open_file() and add_file(), change_file_prop()\n> and apply_textdelta() should be using this but they aren't.\n\nTODO.\n\n> > +  /* Temporary file to write delta to along with its checksum */\n> > +  char *temp_filepath;\n> \n> That's a poor variable name. What about delta_abspath?\n\nFixed.\n\n> > +  svn_checksum_t *checksum;\n> \n> And rename this to delta_checksum?\n\nActually, I figured this wasn't used and removed it.\n\n> > +\n> > +  /* Flags to trigger dumping props and text */\n> > +  svn_boolean_t must_dump_props;\n> > +  svn_boolean_t must_dump_text;\n> \n> I'd call these dump_props and dump_text, but that's a matter of taste.\n\nFixed.\n\n> > +  svn_boolean_t dump_props_pending;\n> > +};\n> > +\n> > +struct dir_baton {\n> > +  struct dump_edit_baton *eb;\n> > +  struct dir_baton *parent_dir_baton;\n> > +\n> > +  /* is this directory a new addition to this revision? */\n> > +  svn_boolean_t added;\n> > +\n> > +  /* has this directory been written to the output stream? */\n> > +  svn_boolean_t written_out;\n> > +\n> > +  /* the absolute path to this directory */\n> > +  const char *path;\n> \n> In code written post-svn-1.6, we usually call absolute paths\n> something_abspath. E.g. local_abspath is an absolute path in a\n> filesystem on the client (but not in the repository).\n> \n> Elsewhere, this is called 'full_path' which would be fine name\n> for this field, too. (Though any full_path variable you see in svn\n> most likely pre-dates the *_abspath convention.)\n\nRenamed path to abspath.\n\n> > +\n> > +  /* the comparison path and revision of this directory.  if both of\n> > +     these are valid, use them as a source against which to compare\n> > +     the directory instead of the default comparison source of PATH in\n> > +     the previous revision. */\n> > +  const char *cmp_path;\n> > +  svn_revnum_t cmp_rev;\n> \n> These just seem to be used as regular copyfrom info, so let's name them\n> as such: copyfrom_path and copyfrom_rev\n> Then you can also shrink the comment above cause everyone knows what\n> copyfrom info is: /* Copyfrom info for the node, if any. */\n\nFixed.\n\n> > +\n> > +  /* hash of paths that need to be deleted, though some -might- be\n> > +     replaced.  maps const char * paths to this dir_baton.  (they're\n> > +     full paths, because that's what the editor driver gives us.  but\n> > +     really, they're all within this directory.) */\n> > +  apr_hash_t *deleted_entries;\n> \n> This is very well commented and well named.\n> \n> > +\n> > +  /* pool to be used for deleting the hash items */\n> > +  apr_pool_t *pool;\n> \n> Hmmm.. a pool does not delete anything. It provides storage.\n> Why do you need this?\n\nTODO.\n\n> > +};\n> > +\n> > +struct handler_baton\n> > +{\n> > +  svn_txdelta_window_handler_t apply_handler;\n> > +  void *apply_baton;\n> > +  apr_pool_t *pool;\n> \n> Yet another pool. What's it for?\n\nSee window_handler below :)\n\n> > +\n> > +  /* Information about the path of the tempoarary file used */\n> \n> s/tempoarary/temporary/\n\nFixed.\n\n> > +  char *temp_filepath;\n> > +  apr_file_t *temp_file;\n> > +  svn_stream_t *temp_filestream;\n> \n> What's the temporary file used for? You're writing a delta to it,\n> so maybe name it accordingly?\n\nFixed.\n\n> You need the file name to stat it in close_file().\n> You need the stream in the baton to write to the file.\n> But you don't need the apr_file.\n> Open the file. Then wrap it in the stream with disown=FALSE, and pass\n> just the stream to the window handler via the baton. When the stream\n> is closed, the file will be closed as well.\n\nTODO. I want to validate and make sure that I don't break anything\nelse or leak memory before performing this change.\n\n> > +\n> > +  /* To fill in the edit baton fields */\n> > +  struct dump_edit_baton *eb;\n> \n> Just say /* Global edit baton. */ or even drop the comment.\n\nFixed.\n\n> > +};\n> > +\n> \n> Needs a docstring.\n\nFixed. I noticed one more mistake: to_rev is unused. Looks like\nthere's a LOT of historical crufts that I didn't clean up. I wonder\nwhy the compiler doesn't tell me about all this.\n\n> > +/* Make a directory baton to represent the directory was path\n> > +   (relative to EDIT_BATON's path) is PATH.\n> \n> The above sentence doesn't parse.\n> \n> > +\n> > +   CMP_PATH/CMP_REV are the path/revision against which this directory\n> > +   should be compared for changes.  If either is omitted (NULL for the\n> > +   path, SVN_INVALID_REVNUM for the rev), just compare this directory\n> > +   PATH against itself in the previous revision.\n> \n> s/CMP/COPYFROM/ and tweak the docstring to say something like:\n>   If the copyfrom information is valid, the directory will be compared\n>   against its copy source. Else, it will be compared against itself in\n>   the previous revision.\n\nAnother historical cruft: since svnrdump doesn't support non-deltified\ndumps and doesn't have fs backing, I can't and don't ever (need to)\ncompare it against itself in the previous revision.\n\n> > +   PARENT_DIR_BATON is the directory baton of this directory's parent,\n> > +   or NULL if this is the top-level directory of the edit.  ADDED\n> > +   indicated if this directory is newly added in this revision.\n> \n> s/indicated/indicates/\n\nFixed.\n\n> > +  apr_array_header_t *compose_path = apr_array_make(pool, 2,\n> > +\t\t\t\t\t\t    sizeof(const char *));\n> \n> The above line contains tabs, please replace with spaces.\n> And make sure to align function arguments like this (not sure if\n> they appeared aligned in your editor or not):\n\nFixed.\n\n> > +  if (pb) {\n> \n> I've told you this on IRC before, but just for sake of completeness:\n> Virtually all if blocks and loops in this patch have a \"wrong\" style\n> of indentation.\n> See\n> http://subversion.apache.org/docs/community-guide/conventions.html#coding-style\n> \n> (I personally prefer the indentation style you're using,\n> but the project convention has been set looooong ago -- such is life.)\n\nAh, I missed this earlier- you have an svn-dev.el: I'll use it to fix\neverything before re-submitting.\n\n> > +    APR_ARRAY_PUSH(compose_path, const char *) = \"/\";\n> > +    APR_ARRAY_PUSH(compose_path, const char *) = path;\n> > +    full_path = svn_path_compose(compose_path, pool);\n> \n> See svn_dirent_join_many().\n\nFixed.\n\n> > +  }\n> > +  else\n> > +    full_path = apr_pstrdup(pool, \"/\");\n> \n> Why allocate \"/\" in a pool? This can be static string unless you\n> intend to write to it.\n\nFrankly, working with APR pools was quite a nightmare for me- after\nobserving many cases of leaks and crashes, I jotted down some notes\nabout using them and I made it a point to follow them strictly. This\nalloc adheres to those notes. I'll submit those notes to dev@ once\nI've polished them- new devs will probably find it useful.\n\n> > +/*\n> > + * Write out a node record for PATH of type KIND under EB->FS_ROOT.\n> > + * ACTION describes what is happening to the node (see enum svn_node_action).\n> > + * Write record to writable EB->STREAM, using EB->BUFFER to write in chunks.\n> > + *\n> > + * If the node was itself copied, IS_COPY is TRUE and the\n> > + * path/revision of the copy source are in CMP_PATH/CMP_REV.  If\n> > + * IS_COPY is FALSE, yet CMP_PATH/CMP_REV are valid, this node is part\n> > + * of a copied subtree.\n> \n> Again, s/CMP/COPYFROM/\n\nFixed.\n\n> > + */\n> > +static svn_error_t *\n> > +dump_node(struct dump_edit_baton *eb,\n> > +          const char *path,    /* an absolute path. */\n> > +          svn_node_kind_t kind,\n> > +          enum svn_node_action action,\n> > +          const char *cmp_path,\n> > +          svn_revnum_t cmp_rev,\n> > +          apr_pool_t *pool)\n> > +{\n> > +  /* Write out metadata headers for this file node. */\n> \n> The node might as well be a directory, so the above comment is misleading.\n\nFixed.\n\n> > +  /* Remove leading slashes from copyfrom paths. */\n> > +  if (cmp_path)\n> > +    cmp_path = ((*cmp_path == '/') ? cmp_path + 1 : cmp_path);\n> \n> What if the copyfrom path is \"/\"?\n> (If memory serves me right we've had a bug like this before somewhere...)\n\nRight, I copied this out from somewhere I think. Fixed with:\nif (copyfrom_path && strcmp(copyfrom_path, \"/\"))\n\n> > +\n> > +  switch (action) {\n> > +    /* Appropriately handle the four svn_node_action actions */\n> \n> Nuke the above comment. Don't put numbers that may change some day\n> into comments.\n\nOkay. Fixed.\n\n> > +    if (!eb->is_copy) {\n> > +      /* eb->dump_props_pending for files is handled in\n> > +         close_file which is called immediately.\n> > +         However, directories are not closed until\n> > +         all the work inside them have been done;\n> \n> s/have been/has been/\n\nFixed.\n\n> > +         eb->dump_props_pending for directories is\n> > +         handled in all the functions that can\n> > +         possibly be called after add_directory:\n> > +         add_directory, open_directory,\n> > +         delete_entry, close_directory, add_file,\n> > +         open_file and change_dir_prop;\n> > +         change_dir_prop is a special case\n> > +         ofcourse */\n> \n> Please re-format the above using longer lines (up to column 78).\n\nFixed.\n\n> > +static svn_error_t *open_root(void *edit_baton,\n> > +\t\t\t      svn_revnum_t base_revision,\n> > +\t\t\t      apr_pool_t *pool,\n> > +\t\t\t      void **root_baton)\n> \n> tabs in above 3 lines\n\nFixed.\n\n> > +{\n> > +  /* Allocate a special pool for the edit_baton to avoid pool\n> > +     lifetime issues */\n> \n> I think you don't need this comment because this is already\n> sort of documented in the docstring for open_root() in svn_delta.h.\n\nIt took me a while to grasp this even after reading the documentation\nof open_root, and I think others will find it useful. Didn't remove\ncomment.\n\n> > +  if (val)\n> > +    /* Delete the path, it's now been dumped */\n> > +    apr_hash_set(pb->deleted_entries, path, APR_HASH_KEY_STRING, NULL);\n> \n> You don't need to set the value to NULL in the hash table.\n> Doing so won't save any memory. I've say just remove the above 3 lines.\n\nOh, I'm not doing it to save memory. Although I'm not sure if I still\nneed it in my logic, this definitely makes debugging nicer.\n\n> tabs again in the above line\n\nFixed.\n\n> > +    APR_ARRAY_PUSH(compose_path, const char *) =\n> > +\t    svn_relpath_basename(path, pool);\n> \n> and here is another tab \n\nFixed.\n\n> > +    cmp_path = svn_path_compose(compose_path, pool);\n> \n> Again, see svn_dirent_join_many().\n> If you need a svn_relpath_join_many() for some reason please write one.\n\nFixed. Yes, svn_relpath_join_many sounds like a good idea: I'll mark\nthat as a TODO for now.\n\n> > +static svn_error_t *\n> > +close_directory(void *dir_baton,\n> > +                apr_pool_t *pool)\n> > +{\n> > +  struct dir_baton *db = dir_baton;\n> > +  struct dump_edit_baton *eb = db->eb;\n> > +  apr_hash_index_t *hi;\n> > +  apr_pool_t *subpool = svn_pool_create(pool);\n> \n> Please call this iterpool, not subpool.\n> You're using it in a loop (so we prefer \"iteration pool\").\n\nRight. Fixed.\n\n> > +\n> > +  /* Some pending properties to dump? */\n> > +  SVN_ERR(dump_props(eb, &(eb->dump_props_pending), TRUE, pool));\n> > +\n> > +  /* Dump the directory entries */\n> > +  for (hi = apr_hash_first(pool, db->deleted_entries); hi;\n> > +       hi = apr_hash_next(hi)) {\n> > +    const void *key;\n> > +    const char *path;\n> > +    apr_hash_this(hi, &key, NULL, NULL);\n> > +    path = key;\n> \n> See svn__apr_hash_index_key().\n\nTODO.\n\n> > +  apr_array_header_t *compose_path = apr_array_make(pool, 2,\n> > +\t\t\t\t\t\t    sizeof(const char *));\n> \n> tabs\n\nFixed.\n\n> > +  /* If the parent directory has explicit comparison path and rev,\n> \n> s/comparison/copyfrom/\n\nFixed.\n\n> > +     record the same for this one. */\n> > +  if (pb && ARE_VALID_COPY_ARGS(pb->cmp_path, pb->cmp_rev)) {\n> > +    APR_ARRAY_PUSH(compose_path, const char *) = pb->cmp_path;\n> > +    APR_ARRAY_PUSH(compose_path, const char *) =\n> > +\t    svn_relpath_basename(path, pool);\n> \n> one more tab\n\nFixed.\n\n> > +  /* Write information about the filepath to hb->eb */\n> \n> s/to hb->eb/from the handler baton to the edit baton/\n\nEr, I did mean `hb->eb` literally (the editor baton in the handler\nbaton).\n\n> > +  /* Cleanup */\n> > +  SVN_ERR(svn_io_file_close(hb->temp_file, hb->pool));\n> \n> As described above, you don't need to close the file,\n> closing the stream is enough.\n\nTODO.\n\n> > +  /* Custom handler_baton allocated in a separate pool */\n> > +  apr_pool_t *handler_pool = svn_pool_create(pool);\n> > +  struct handler_baton *hb = apr_pcalloc(handler_pool, sizeof(*hb));\n> > +  hb->pool = handler_pool;\n> \n> It sucks that the window handler does not get pool arguments, so\n> you have to stick a pool in the baton. But that isn't your fault.\n\nExactly.\n\n> > +  hb->eb = eb;\n> > +\n> > +  /* Use a temporary file to measure the text-content-length */\n> > +  SVN_ERR(svn_io_temp_dir(&tempdir, hb->pool));\n> > +\n> > +  hb->temp_filepath = svn_dirent_join(tempdir, \"XXXXXX\", hb->pool);\n> > +  apr_err = apr_file_mktemp(&(hb->temp_file), hb->temp_filepath,\n> > +          APR_CREATE | APR_READ | APR_WRITE | APR_EXCL,\n> > +          hb->pool);\n> > +  if (apr_err != APR_SUCCESS)\n> > +    SVN_ERR(svn_error_wrap_apr(apr_err, NULL));\n> \n> You can replace the above chunk with a simple call to\n> svn_io_open_unique_file3().\n\nTODO. If I recall correctly, someone else also suggested this on IRC,\nbut there seems to be some issue with it: I'll check this later.\n\n> > +    SVN_ERR(svn_io_file_open(&temp_file, eb->temp_filepath,APR_READ,\n> > +\t\t\t     0600,pool));\n> \n> tabs again\n\nFixed.\n\n> > +  struct dump_edit_baton *eb = apr_pcalloc(pool,\n> > +\t\t\t\t\t   sizeof(struct dump_edit_baton));\n> \n> more tabs\n\nFixed.\n\n> > +static int verbose = 0;\n> > +static apr_pool_t *pool = NULL;\n> > +static svn_client_ctx_t *ctx = NULL;\n> \n> You're only using the client context in open_connection.\n> Make it a local variable there?\n\nI was actually worried about lifetime issues. If ctx won't be read/\nwritten after open_connection, this is okay. Otherwise, not. TODO.\n\n> > +  struct replay_baton *replay_baton = apr_palloc(pool,\n> > +\t\t\t\t\t\t sizeof(struct replay_baton));\n> \n> more tabs\n\nFixed.\n\n> > +  fprintf(out_stream,\n> \n> Use svn_cmdline_fprintf()\n\nFixed.\n\n> > +    \"usage: svnrdump URL [-r LOWER[:UPPER]]\\n\\n\"\n> \n> This string needs to be marked for localisation like this: _(\"my string\")\n\nTODO. I'm missing some header: _ is undefined.\n\n> > +    \"Dump the contents of repository at remote URL to stdout in a 'dumpfile'\\n\"\n> > +    \"v3 portable format.  Dump revisions LOWER rev through UPPER rev.\\n\"\n> \n> You don't need to mention the dumpfile format version in the help\n> string.\n\nOkay. I need to mention somewhere that svnrdump doesn't support\nundeltified dumps though, don't I?\n\n> > +    \"LOWER defaults to 1 and UPPER defaults to the highest possible revision\\n\"\n> > +    \"if omitted.\\n\");\n> > +  for (i = 1; i < argc; i++) {\n> \n> Please use svn_cmdline__getopt_init() and apr_getopt_long().\n> See svnsync for an example.\n\nOuch. Don't you think it's an overkill for the current svnrdump? There\nare no subcommands and just a few command-line arguments.\n\n> Please add a docstring.\n> \n> > +void\n> > +write_hash_to_stringbuf(apr_hash_t *properties,\n> > +                        svn_boolean_t deleted,\n> > +                        svn_stringbuf_t **strbuf,\n> > +                        apr_pool_t *pool);\n> > +\n\nFixed.\n\n> Please add a docstring.\n> \n> > +svn_error_t *\n> > +dump_props(struct dump_edit_baton *eb,\n> > +           svn_boolean_t *trigger_var,\n> > +           svn_boolean_t dump_data_too,\n> > +           apr_pool_t *pool);\n> > +\n> > +#endif\n\nFixed. Doxygen-friendly docstrings are a TODO.\n\n> > +#include \"svn_pools.h\"\n> > +#include \"svn_cmdline.h\"\n> > +#include \"svn_client.h\"\n> > +#include \"svn_ra.h\"\n> > +#include \"svn_repos.h\"\n> \n> Are all these includes really needed?\n\nIt seems only svn_repos.h is needed. Fixed.\n\n> > +void\n> > +write_hash_to_stringbuf(apr_hash_t *properties,\n> > +                        svn_boolean_t deleted,\n> > +                        svn_stringbuf_t **strbuf,\n> > +                        apr_pool_t *pool)\n> > +{\n> \n> This function needs a docstring, too.\n\nWait. I just need to write the docstrings once, right? In the header?\n\n> And is there no function that already does this somewhere in the svn\n> libraries or in APR?\n\nNo. I copied this out from subversion/svnrdump/svnrdump.c and\nrefactored it a little bit.\n\nYou can see the changes I made after your review in the most recent\ncouple of commits on my GitHub [1].\n\n[1]: http://github.com/artagnon/svn-dump-fast-export/commits/svn-merge\n\n-- Ram\n"},{"id":"145657","messageId":"20100715192321.GA722@ted.stsp.name","threadId":"24349","inReplyTo":"20100715190220.GI22574@debian","subject":"Re: [PATCH v2] Add svnrdump","fromName":"Stefan Sperling","fromEmail":"stsp@elego.de","sentAt":"2010-07-15T19:23:21Z","receivedAt":"2010-07-15T19:23:21Z","isPatch":true,"sender":{"key":"stsp@elego.de","avatar":"https://avatars.githubusercontent.com/u/9281333?v=4"},"body":"On Thu, Jul 15, 2010 at 09:02:20PM +0200, Ramkumar Ramachandra wrote:\n> Stefan Sperling writes:\n> > > +};\n> > > +\n> > > +struct handler_baton\n> > > +{\n> > > +  svn_txdelta_window_handler_t apply_handler;\n> > > +  void *apply_baton;\n> > > +  apr_pool_t *pool;\n> > \n> > Yet another pool. What's it for?\n> \n> See window_handler below :)\n\nOops, I meant to imply that you should add a docstring :)\n\n> > > +  }\n> > > +  else\n> > > +    full_path = apr_pstrdup(pool, \"/\");\n> > \n> > Why allocate \"/\" in a pool? This can be static string unless you\n> > intend to write to it.\n> \n> Frankly, working with APR pools was quite a nightmare for me- after\n> observing many cases of leaks and crashes, I jotted down some notes\n> about using them and I made it a point to follow them strictly. This\n> alloc adheres to those notes. I'll submit those notes to dev@ once\n> I've polished them- new devs will probably find it useful.\n\nIt's not that hard once you get used to the concept.\nWhen you send your notes, we can comment on them in case there's\nanything you misunderstood.\n\n> > > +  if (val)\n> > > +    /* Delete the path, it's now been dumped */\n> > > +    apr_hash_set(pb->deleted_entries, path, APR_HASH_KEY_STRING, NULL);\n> > \n> > You don't need to set the value to NULL in the hash table.\n> > Doing so won't save any memory. I've say just remove the above 3 lines.\n> \n> Oh, I'm not doing it to save memory. Although I'm not sure if I still\n> need it in my logic, this definitely makes debugging nicer.\n\nThen please say so in the comment:\n\n /* Small debugging aid: set path to NULL so we crash if we use it again. */\n\n> > > +  /* Write information about the filepath to hb->eb */\n> > \n> > s/to hb->eb/from the handler baton to the edit baton/\n> \n> Er, I did mean `hb->eb` literally (the editor baton in the handler\n> baton).\n\nAh, right. Though maybe saying \"edit baton\" is just as clear?\n\n> > > +static int verbose = 0;\n> > > +static apr_pool_t *pool = NULL;\n> > > +static svn_client_ctx_t *ctx = NULL;\n> > \n> > You're only using the client context in open_connection.\n> > Make it a local variable there?\n> \n> I was actually worried about lifetime issues. If ctx won't be read/\n> written after open_connection, this is okay. Otherwise, not. TODO.\n\nThe global variables are still wrong.\nJust pass the root pool you create in main() down to open_connection()\nand use it when creating the client context. There won't be a lifetime\nproblem.\n\n> > > +    \"usage: svnrdump URL [-r LOWER[:UPPER]]\\n\\n\"\n> > \n> > This string needs to be marked for localisation like this: _(\"my string\")\n> \n> TODO. I'm missing some header: _ is undefined.\n\n#include \"svn_private_config.h\"\n\n> > > +    \"Dump the contents of repository at remote URL to stdout in a 'dumpfile'\\n\"\n> > > +    \"v3 portable format.  Dump revisions LOWER rev through UPPER rev.\\n\"\n> > \n> > You don't need to mention the dumpfile format version in the help\n> > string.\n> \n> Okay. I need to mention somewhere that svnrdump doesn't support\n> undeltified dumps though, don't I?\n\nNot yet. My plan is to ask people why we're not using the v3 format\nby default. Unless there is a good reason not to do so I'd like to\nmake v3 the default format for svnadmin dump in 1.7.\n\n> > > +    \"LOWER defaults to 1 and UPPER defaults to the highest possible revision\\n\"\n> > > +    \"if omitted.\\n\");\n> > > +  for (i = 1; i < argc; i++) {\n> > \n> > Please use svn_cmdline__getopt_init() and apr_getopt_long().\n> > See svnsync for an example.\n> \n> Ouch. Don't you think it's an overkill for the current svnrdump? There\n> are no subcommands and just a few command-line arguments.\n\nThe point is to have consistent code.\n\n> > Please add a docstring.\n> > \n> > > +svn_error_t *\n> > > +dump_props(struct dump_edit_baton *eb,\n> > > +           svn_boolean_t *trigger_var,\n> > > +           svn_boolean_t dump_data_too,\n> > > +           apr_pool_t *pool);\n> > > +\n> > > +#endif\n> \n> Fixed. Doxygen-friendly docstrings are a TODO.\n\nYou only need to be doxygen-friendly in the public headers,\nwhich are the ones in subversion/include.\n\n> > > +void\n> > > +write_hash_to_stringbuf(apr_hash_t *properties,\n> > > +                        svn_boolean_t deleted,\n> > > +                        svn_stringbuf_t **strbuf,\n> > > +                        apr_pool_t *pool)\n> > > +{\n> > \n> > This function needs a docstring, too.\n> \n> Wait. I just need to write the docstrings once, right? In the header?\n\nRight. It goes in the header, unless the function is static. My bad.\n\n> You can see the changes I made after your review in the most recent\n> couple of commits on my GitHub [1].\n> \n> [1]: http://github.com/artagnon/svn-dump-fast-export/commits/svn-merge\n\nThanks!\n\nStefan\n"},{"id":"145932","messageId":"20100721114610.GC15903@kytes","threadId":"24349","inReplyTo":"20100715192321.GA722@ted.stsp.name","subject":"Re: [PATCH v2] Add svnrdump","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2010-07-21T11:46:10Z","receivedAt":"2010-07-21T11:46:10Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Stefan,\n\nIt's been a while, and I've fixed most of the issues that you pointed\nout intermittently over the last few days. I'm still writing a\nunittest in Python, although I'm still trying to figure out how to\ntest it without using that ugly awk query to compare it against the\n`svnadmin dump` output. With the help of the Subversion community, I\nthink I should be able to beat it into shape in a month or so :)\n\n\n-- Ram\n"},{"id":"145934","messageId":"20100721132929.GA508@daniel3.local","threadId":"24349","inReplyTo":"20100721114610.GC15903@kytes","subject":"Re: [PATCH v2] Add svnrdump","fromName":"Daniel Shahaf","fromEmail":"d.s@daniel.shahaf.name","sentAt":"2010-07-21T13:29:29Z","receivedAt":"2010-07-21T13:29:29Z","isPatch":true,"sender":{"key":"d.s@daniel.shahaf.name","avatar":null},"body":"Ramkumar Ramachandra wrote on Wed, Jul 21, 2010 at 17:16:10 +0530:\n> I'm still writing a unittest in Python, although I'm still trying to figure\n> out how to test it without using that ugly awk query to compare it against\n> the `svnadmin dump` output.\n\nHave a look at svnsync_tests.py and svntest/verify.py.\n"},{"id":"145949","messageId":"20100721190324.GC23839@kytes","threadId":"24349","inReplyTo":"20100713201105.GN13310@ted.stsp.name","subject":"Re: [PATCH v2] Add svnrdump","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2010-07-21T19:03:24Z","receivedAt":"2010-07-21T19:03:24Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Stefan,\n\nStefan Sperling writes:\n> Copy of what and when? This baton is global for the entire edit...\n> \n> Going through the code, I see that you're using this to indicate to\n> dump_node() whether an add_directory() or add_file() was in fact a copy.\n> Why not remove this field from the struct and add it as a parameter to\n> dump_node instead?\n\nThis is an excellent catch! It's in the editor baton for historical\nreasons- cleaned up with my latest commit.\n\nThanks :)\n\n-- Ram\n"}]}