Edmund Horner writes:
> Ok, here's a new patch...
Committed in r18650.
Thanks for the patch,
//Peter
> I have removed create_unique_tmpdir but kept
> the tmpdir argument to print_diff_tree; it seems more efficient to call
> svn_io_temp_dir just once. The difference is that tmpdir is now simply
> the official temp dir instead of a special unique svnlook temp dir.
>
> The comments in print_diff_tree have been trimmed down a bit more. The
> code continues to create the "second" temp file first (which it did in
> case of name conflicts), which isn't necessary now.
>
> I've tested it on the first 10000 svn revisions (yeah, my machine is
> slow) and it produces identical diffs in all cases that the original can
> handle and diffs for those cases the original can't (1398, 1399, 1795,
> 1805). It also passes svnlook-tests.
>
> Edmund.
>
>
>
> [[[
> Use simple and safe temporary file names for svnlook diff, rather than
> building out the tree as it exists in the repository -- this avoids
> problems with paths that can't be used in the temporary directory.
>
> Patch by: Edmund Horner <ejrh00@gmail.com>
> Suggested by: Max Bowsher <maxb@ukf.net>
> Review by: Peter N. Lundblad <peter@famlundblad.se>
>
> * subversion/svnlook/main.c
> (prepare_tmpfiles): Create a normal unique file in the temporary
> directory.
> (print_diff_tree): Update comments to omit mention of the built-out
> temporary directories, which don't get created any more.
> (create_unique_tmpdir): Remove function.
> (do_diff): Use the system temporary directory instead of creating
> a special svnlook one; unlike the old one, this temporary directory
> doesn't need to be removed after use, so don't try to delete it.
> ]]]
> Index: main.c
> ===================================================================
> --- main.c (revision 18619)
> +++ main.c (working copy)
> @@ -692,10 +692,9 @@
> }
>
> /* Now, prepare the two temporary files, each of which will either
> - be empty, or will have real contents. The first file we will
> - make in our temporary directory. */
> - *tmpfile2 = svn_path_join(tmpdir, path2, pool);
> - SVN_ERR(open_writable_binary_file(&fh, *tmpfile2, pool));
> + be empty, or will have real contents. */
> + SVN_ERR(svn_io_open_unique_file2(&fh, tmpfile2, apr_psprintf(pool, "%s/diff", tmpdir),
> + ".tmp", svn_io_file_del_none, pool));
> if (root2)
> SVN_ERR(dump_contents(fh, root2, path2, pool));
> apr_file_close(fh);
> @@ -897,24 +896,15 @@
> diff.
>
> - Second, dump the contents of the new version of the file
> - into the svnlook temporary directory, building out the
> - actual directories that need to be created in order to
> - fully represent the filesystem path inside the tmp
> - directory.
> + into the temporary directory.
>
> - Then, dump the contents of the old version of the file into
> - the svnlook temporary directory, also building out intermediate
> - directories as needed, using a unique temporary file name (we
> - do this *after* putting the new version of the file
> - there in case something actually versioned has a name
> - that looks like one of our unique identifiers).
> + the temporary directory.
>
> - Next, we run 'diff', passing the repository paths as the
> labels.
>
> - - Finally, we delete the temporary files (but leave the
> - built-out directories in place until after all diff
> - handling has been finished). */
> + - Finally, we delete the temporary files. */
> if (node->action == 'R' && node->text_mod)
> {
> do_diff = TRUE;
> @@ -1347,48 +1337,7 @@
> return SVN_NO_ERROR;
> }
>
> -/* Create a new temporary directory with an 'svnlook' prefix. */
> -static svn_error_t *
> -create_unique_tmpdir(const char **name, apr_pool_t *pool)
> -{
> - const char *unique_name;
> - const char *sys_tmp_dir;
> - const char *base;
> - unsigned int i;
>
> - SVN_ERR(svn_io_temp_dir(&sys_tmp_dir, pool));
> - base = svn_path_join(sys_tmp_dir, "svnlook", pool);
> -
> - for (i = 1; i <= 99999; i++)
> - {
> - svn_error_t *err;
> -
> - unique_name = apr_psprintf(pool, "%s.%u", base, i);
> - /* The directory has a predictable name so it is made writeable for
> - the owner only (without relying on the umask) to inhibit symlink
> - attacks on the filenames; the filenames are also, to a certain
> - extent, predictable. */
> - err = svn_io_dir_make(unique_name,
> - APR_UREAD | APR_UWRITE | APR_UEXECUTE,
> - pool);
> -
> - if (!err)
> - {
> - *name = unique_name;
> - return SVN_NO_ERROR;
> - }
> -
> - if (! APR_STATUS_IS_EEXIST(err->apr_err))
> - return err;
> -
> - svn_error_clear(err);
> - }
> -
> - *name = NULL;
> - return svn_error_createf(SVN_ERR_IO_UNIQUE_NAMES_EXHAUSTED,
> - NULL, _("Can't create temporary directory"));
> -}
> -
> /* Print some diff-y stuff in a TBD way. :-) */
> static svn_error_t *
> do_diff(svnlook_ctxt_t *c, apr_pool_t *pool)
> @@ -1414,18 +1363,12 @@
> if (tree)
> {
> const char *tmpdir;
> - svn_error_t *err;
>
> SVN_ERR(svn_fs_revision_root(&base_root, c->fs, base_rev_id, pool));
> - SVN_ERR(create_unique_tmpdir(&tmpdir, pool));
> - err = print_diff_tree(root, base_root, tree, "", "",
> - c, tmpdir, pool);
> - if (err)
> - {
> - svn_error_clear(svn_io_remove_dir(tmpdir, pool));
> - return err;
> - }
> - SVN_ERR(svn_io_remove_dir(tmpdir, pool));
> + SVN_ERR(svn_io_temp_dir(&tmpdir, pool));
> +
> + SVN_ERR(print_diff_tree(root, base_root, tree, "", "",
> + c, tmpdir, pool));
> }
> return SVN_NO_ERROR;
> }
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Feb 28 21:06:49 2006