Re: [PATCH] Add svnrdump
Michael J Gruber writes: > Blair Zajac venit, vidit, dixit 09.07.2010 02:42: >> On 07/08/2010 01:17 AM, Daniel Shahaf wrote: >>> @Bert: could you please trim quoted patches to only the relevant parts? >>> Scrolling is tedious when I don't have have line folding available... >> >> +1 on this, in Thunderbird, it took a while to scan through the whole >> email to see the comments. > > > That is what the QuoteCollapse extension to Thunderbird was invented for. ;) > > > In fact, on the git list we tend to trim as little as possible. What??? Perhaps we are on different git lists?
Re: [PATCH] Add svnrdump
Hi, Ramkumar Ramachandra writes: > Where is svn_dirent_basename defined? I can't seem to find it in the > codebase at all. np, I found it. It's svn_relpath_basename. -- Ram
Re: [PATCH] Add svnrdump
Hi, Bert Huijben writes: > > + /* Cleanup */ > > + SVN_ERR(svn_io_file_close(hb->temp_file, hb->pool)); > > + SVN_ERR(svn_stream_close(hb->temp_filestream)); > > The standard handler already closes the stream for you and if you don't > disown the file on mapping, this also closes the file. > > > + svn_pool_destroy(hb->pool); > > And as you clear the pool that contains the file and stream here, closing > yourself is not necessary. I realize this, but I closed the stream anyway to make debugging easier. The pool is only destroyed much later. Do you think this is bad policy? -- Ram
Re: [PATCH] Add svnrdump
Hi Daniel, Daniel Shahaf writes: > > > + /* Use a temporary file to measure the text-content-length */ > > > + apr_err = apr_temp_dir_get(&tempdir, hb->pool); > > svn_io_temp_dir() Fixed. > > > + if (apr_err != APR_SUCCESS) > > > +SVN_ERR(svn_error_wrap_apr(apr_err, NULL)); > > > + > > > + hb->temp_filepath = apr_psprintf(eb->pool, "%s/svn-fe-XX", > > > tempdir); > > > > os.path.join() > > Err, I mean, svn_dirent_join(). Fixed. Thanks :) -- Ram
Re: [PATCH] Add svnrdump
Hi Bert,
Thank you for the review.
Bert Huijben writes:
> > +svn_error_t *open_root(void *edit_baton,
> > + svn_revnum_t base_revision,
> > + apr_pool_t *pool,
> > + void **root_baton)
>
> Static and the return type on its own line.
Fixed. Sorry about the sloppy error.
> This looks like more than 80 characters to me.
I didn't realize that it was a strict requirement. Fixed now.
> > + if (pb && ARE_VALID_COPY_ARGS(pb->cmp_path, pb->cmp_rev)) {
> > +APR_ARRAY_PUSH(compose_path, const char *) = pb->cmp_path;
> > +APR_ARRAY_PUSH(compose_path, const char *) =
> > svn_dirent_basename(path, pool);
>
> Assuming that the path doesn't start with a '/' here, this should be
> svn_relent_basename() to avoid platform specific path rules.
Where is svn_dirent_basename defined? I can't seem to find it in the
codebase at all.
> > + hb->temp_filepath = apr_psprintf(eb->pool, "%s/svn-fe-XX",
> > tempdir);
>
> Why store this path in the editor pool? Do you really need this path to
> live that long?
Excellent catch! :) Fixed now.
> > +svn_error_t *
> > +get_dump_editor(const svn_delta_editor_t **editor,
> > +void **edit_baton,
> > +svn_revnum_t to_rev,
> > +apr_pool_t *pool);
>
> These structs and this function don't follow our naming guidelines for
> libraries. But these functions are no reusable library (yet).
Right. Is it alright then? Can I re-submit the patch now? (Also fixed
a couple of things Daniel pointed out).
-- Ram
Re: [PATCH] Add svnrdump
Heya, On Fri, Jul 9, 2010 at 03:16, Michael J Gruber wrote: > In fact, on the git list we tend to trim as little as possible. Huh? Quite the opposite I thought... -- Cheers, Sverre Rabbelier
Re: [PATCH] Add svnrdump
Blair Zajac venit, vidit, dixit 09.07.2010 02:42: > On 07/08/2010 01:17 AM, Daniel Shahaf wrote: >> @Bert: could you please trim quoted patches to only the relevant parts? >> Scrolling is tedious when I don't have have line folding available... > > +1 on this, in Thunderbird, it took a while to scan through the whole > email to see the comments. That is what the QuoteCollapse extension to Thunderbird was invented for. ;) In fact, on the git list we tend to trim as little as possible. Michael
Re: [PATCH] Add svnrdump
On 07/08/2010 01:17 AM, Daniel Shahaf wrote: @Bert: could you please trim quoted patches to only the relevant parts? Scrolling is tedious when I don't have have line folding available... +1 on this, in Thunderbird, it took a while to scan through the whole email to see the comments. Regards, Blair
RE: [PATCH] Add svnrdump
@Bert: could you please trim quoted patches to only the relevant parts? Scrolling is tedious when I don't have have line folding available... Bert Huijben wrote on Thu, 8 Jul 2010 at 12:28 -: > > -Original Message- > > From: Ramkumar Ramachandra [mailto:artag...@gmail.com] > > Sent: donderdag 8 juli 2010 10:35 > > To: dev@subversion.apache.org > > Cc: Jonathan Nieder; Sverre Rabbelier; Git Mailing List > > Subject: [PATCH] Add svnrdump > > > > + /* Use a temporary file to measure the text-content-length */ > > + apr_err = apr_temp_dir_get(&tempdir, hb->pool); svn_io_temp_dir() > > + if (apr_err != APR_SUCCESS) > > +SVN_ERR(svn_error_wrap_apr(apr_err, NULL)); > > + > > + hb->temp_filepath = apr_psprintf(eb->pool, "%s/svn-fe-XX", > > tempdir); > os.path.join() Err, I mean, svn_dirent_join().
RE: [PATCH] Add svnrdump
> -Original Message-
> From: Ramkumar Ramachandra [mailto:artag...@gmail.com]
> Sent: donderdag 8 juli 2010 10:35
> To: dev@subversion.apache.org
> Cc: Jonathan Nieder; Sverre Rabbelier; Git Mailing List
> Subject: [PATCH] Add svnrdump
>
> Hi,
>
> Kindly find the diff for svnrdump in the later part of this email. I
> haven't written a build.conf rule or manpage, but I suppose I can do
> them in another patch.
>
> Thanks.
>
> -- Ram
>
> 8<--->8-
>
> Index: subversion/svnrdump/dump_editor.c
> ===
> --- subversion/svnrdump/dump_editor.c (revision 0)
> +++ subversion/svnrdump/dump_editor.c (working copy)
> @@ -0,0 +1,689 @@
> +/*
> + *
>
> + *Licensed to the Apache Software Foundation (ASF) under one
> + *or more contributor license agreements. See the NOTICE file
> + *distributed with this work for additional information
> + *regarding copyright ownership. The ASF licenses this file
> + *to you under the Apache License, Version 2.0 (the
> + *"License"); you may not use this file except in compliance
> + *with the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + *Unless required by applicable law or agreed to in writing,
> + *software distributed under the License is distributed on an
> + *"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
> + *KIND, either express or implied. See the License for the
> + *specific language governing permissions and limitations
> + *under the License.
> + *
>
> + */
> +
> +#include "svn_pools.h"
> +#include "svn_repos.h"
> +#include "svn_path.h"
> +#include "svn_props.h"
> +
> +#include "svnrdump.h"
> +#include "dump_editor.h"
> +
> +#define ARE_VALID_COPY_ARGS(p,r) ((p) && SVN_IS_VALID_REVNUM(r))
> +
> +svn_boolean_t must_dump_props = FALSE, must_dump_text = FALSE,
> + dump_props_pending = FALSE;
You should use the editor baton for this to avoid global variables.
> +
> +/* Make a directory baton to represent the directory was path
> + (relative to EDIT_BATON's path) is PATH.
> +
> + CMP_PATH/CMP_REV are the path/revision against which this directory
> + should be compared for changes. If either is omitted (NULL for the
> + path, SVN_INVALID_REVNUM for the rev), just compare this directory
> + PATH against itself in the previous revision.
> +
> + PARENT_DIR_BATON is the directory baton of this directory's parent,
> + or NULL if this is the top-level directory of the edit. ADDED
> + indicated if this directory is newly added in this revision.
> + Perform all allocations in POOL. */
> +static struct dir_baton *
> +make_dir_baton(const char *path,
> + const char *cmp_path,
> + svn_revnum_t cmp_rev,
> + void *edit_baton,
> + void *parent_dir_baton,
> + svn_boolean_t added,
> + apr_pool_t *pool)
> +{
> + struct dump_edit_baton *eb = edit_baton;
> + struct dir_baton *pb = parent_dir_baton;
> + struct dir_baton *new_db = apr_pcalloc(pool, sizeof(*new_db));
> + const char *full_path;
> + apr_array_header_t *compose_path = apr_array_make(pool, 2,
> sizeof(const char *));
> +
> + /* A path relative to nothing? I don't think so. */
> + SVN_ERR_ASSERT_NO_RETURN(!path || pb);
> +
> + /* Construct the full path of this node. */
> + if (pb) {
> +APR_ARRAY_PUSH(compose_path, const char *) = "/";
> +APR_ARRAY_PUSH(compose_path, const char *) = path;
> +full_path = svn_path_compose(compose_path, pool);
> + }
> + else
> +full_path = apr_pstrdup(pool, "/");
> +
> + /* Remove leading slashes from copyfrom paths. */
> + if (cmp_path)
> +cmp_path = ((*cmp_path == '/') ? cmp_path + 1 : cmp_path);
> +
> + new_db->eb = eb;
> + new_db->parent_dir_baton = pb;
> + new_db->path = full_path;
> + new_db->cmp_path = cmp_path ? apr_pstrdup(pool, cmp_path) : NULL;
> + new_db->cmp_rev = cmp_rev;
> + new_db->added = added;
> + new_db->written_out = FALSE;
> + new_db->deleted_entries = apr_hash_make(pool);
> + new_db->pool = pool;
> +
> + return new_db;
> +}
> +/*
> + * Write out a node record for PATH of type KIND under EB->FS_ROOT
[PATCH] Add svnrdump
Hi,
Kindly find the diff for svnrdump in the later part of this email. I
haven't written a build.conf rule or manpage, but I suppose I can do
them in another patch.
Thanks.
-- Ram
8<--->8-
Index: subversion/svnrdump/dump_editor.c
===
--- subversion/svnrdump/dump_editor.c (revision 0)
+++ subversion/svnrdump/dump_editor.c (working copy)
@@ -0,0 +1,689 @@
+/*
+ *
+ *Licensed to the Apache Software Foundation (ASF) under one
+ *or more contributor license agreements. See the NOTICE file
+ *distributed with this work for additional information
+ *regarding copyright ownership. The ASF licenses this file
+ *to you under the Apache License, Version 2.0 (the
+ *"License"); you may not use this file except in compliance
+ *with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *Unless required by applicable law or agreed to in writing,
+ *software distributed under the License is distributed on an
+ *"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *KIND, either express or implied. See the License for the
+ *specific language governing permissions and limitations
+ *under the License.
+ *
+ */
+
+#include "svn_pools.h"
+#include "svn_repos.h"
+#include "svn_path.h"
+#include "svn_props.h"
+
+#include "svnrdump.h"
+#include "dump_editor.h"
+
+#define ARE_VALID_COPY_ARGS(p,r) ((p) && SVN_IS_VALID_REVNUM(r))
+
+svn_boolean_t must_dump_props = FALSE, must_dump_text = FALSE,
+ dump_props_pending = FALSE;
+
+/* Make a directory baton to represent the directory was path
+ (relative to EDIT_BATON's path) is PATH.
+
+ CMP_PATH/CMP_REV are the path/revision against which this directory
+ should be compared for changes. If either is omitted (NULL for the
+ path, SVN_INVALID_REVNUM for the rev), just compare this directory
+ PATH against itself in the previous revision.
+
+ PARENT_DIR_BATON is the directory baton of this directory's parent,
+ or NULL if this is the top-level directory of the edit. ADDED
+ indicated if this directory is newly added in this revision.
+ Perform all allocations in POOL. */
+static struct dir_baton *
+make_dir_baton(const char *path,
+ const char *cmp_path,
+ svn_revnum_t cmp_rev,
+ void *edit_baton,
+ void *parent_dir_baton,
+ svn_boolean_t added,
+ apr_pool_t *pool)
+{
+ struct dump_edit_baton *eb = edit_baton;
+ struct dir_baton *pb = parent_dir_baton;
+ struct dir_baton *new_db = apr_pcalloc(pool, sizeof(*new_db));
+ const char *full_path;
+ apr_array_header_t *compose_path = apr_array_make(pool, 2, sizeof(const char
*));
+
+ /* A path relative to nothing? I don't think so. */
+ SVN_ERR_ASSERT_NO_RETURN(!path || pb);
+
+ /* Construct the full path of this node. */
+ if (pb) {
+APR_ARRAY_PUSH(compose_path, const char *) = "/";
+APR_ARRAY_PUSH(compose_path, const char *) = path;
+full_path = svn_path_compose(compose_path, pool);
+ }
+ else
+full_path = apr_pstrdup(pool, "/");
+
+ /* Remove leading slashes from copyfrom paths. */
+ if (cmp_path)
+cmp_path = ((*cmp_path == '/') ? cmp_path + 1 : cmp_path);
+
+ new_db->eb = eb;
+ new_db->parent_dir_baton = pb;
+ new_db->path = full_path;
+ new_db->cmp_path = cmp_path ? apr_pstrdup(pool, cmp_path) : NULL;
+ new_db->cmp_rev = cmp_rev;
+ new_db->added = added;
+ new_db->written_out = FALSE;
+ new_db->deleted_entries = apr_hash_make(pool);
+ new_db->pool = pool;
+
+ return new_db;
+}
+/*
+ * Write out a node record for PATH of type KIND under EB->FS_ROOT.
+ * ACTION describes what is happening to the node (see enum svn_node_action).
+ * Write record to writable EB->STREAM, using EB->BUFFER to write in chunks.
+ *
+ * If the node was itself copied, IS_COPY is TRUE and the
+ * path/revision of the copy source are in CMP_PATH/CMP_REV. If
+ * IS_COPY is FALSE, yet CMP_PATH/CMP_REV are valid, this node is part
+ * of a copied subtree.
+ */
+static svn_error_t *
+dump_node(struct dump_edit_baton *eb,
+ const char *path,/* an absolute path. */
+ svn_node_kind_t kind,
+ enum svn_node_action action,
+ const char *cmp_path,
+ svn_revnum_t cmp_rev,
+ apr_pool_t *pool)
+{
+ /* Write out metadata headers for this file node. */
+ SVN_ERR(svn_stream_printf(eb->stream, pool,
+ SVN_REPOS_DUMPFILE_NODE_PATH ": %s\n",
+ (*path == '/') ? path + 1 : path));
+
+ if (kind == svn_node_file)
+SVN_ERR(svn_stream_printf(eb->stream, pool,
+ SVN_REPOS_DUMPFILE_NODE_KIND ": file\n"));
+ else if (kind == svn_node_dir)
+SVN_ERR(
