On May 5, 2009, at 11:41 AM, Greg Stein wrote:
> On Thu, Apr 30, 2009 at 17:01, Hyrum K. Wright
> <hyrum_at_hyrumwright.org> wrote:
>> ...
>> +++ trunk/subversion/libsvn_wc/upgrade.c Thu Apr 30 08:01:58
>> 2009 (r37513)
>> ...
>> +/* Helper for converting wcprops from the one-file-per-file model.
>> + This implements svn_io_walk_func_t(). */
>> +static svn_error_t *
>> +convert_props_walker(void *baton,
>> + const char *path,
>> + const apr_finfo_t *finfo,
>> + apr_pool_t *pool)
>> +{
>> + svn_wc__db_t *db = baton;
>> + apr_hash_t *proplist;
>> + apr_file_t *file;
>> + svn_stream_t *stream;
>> + const char *local_abspath;
>> + int len;
>> +
>> + /* Skip the directory. */
>> + if (finfo->filetype == APR_DIR)
>> + return SVN_NO_ERROR;
>> +
>> + proplist = apr_hash_make(pool);
>> + SVN_ERR(svn_io_file_open(&file, path, APR_READ | APR_BUFFERED,
>> + APR_OS_DEFAULT, pool));
>> + stream = svn_stream_from_aprfile2(file, FALSE, pool);
>> + SVN_ERR(svn_hash_read2(proplist, stream, SVN_HASH_TERMINATOR,
>> pool));
>> + SVN_ERR(svn_stream_close(stream));
>> +
>> + /* The filename will be something like foo.c.svn-work. From it,
>> determine
>> + the local_abspath of the node. The magic number of 9 below
>> is basically
>> + strlen(".svn-work"). */
>> + len = strlen(path);
>> + local_abspath = apr_pstrndup(pool, local_abspath, len - 9);
>
> This won't work. local_abspath is never set. And even then, you're
> talking about a resulting abspath that looks like
> "/some/path/.svn/foo.c" ... you really don't want the .svn in there.
>
> Might I also suggest adding a test case for this to upgrade_tests.py?
I'd love to, but building a 1.3.x-era client to generate this kind of
working copy turns out to be a bit non-trivial on my platform. Do you
have such a working copy laying about?
>> ...
>> +static svn_error_t *
>> +convert_wcprops(svn_wc_adm_access_t *adm_access,
>> + int old_format,
>> + apr_pool_t *scratch_pool)
>> +{
>> + svn_wc__db_t *db = svn_wc__adm_get_db(adm_access);
>> +
>> + if (old_format <= SVN_WC__WCPROPS_MANY_FILES_VERSION)
>> + {
>> + svn_stream_t *stream;
>> + svn_error_t *err;
>> + const char *dir_abspath;
>> + apr_hash_t *proplist;
>> +
>> + /* First, look at dir-wcprops. */
>> + SVN_ERR(svn_dirent_get_absolute(&dir_abspath,
>> +
>> svn_wc_adm_access_path(adm_access),
>> + scratch_pool));
>
> dir_abspath = svn_wc__adm_access_abspath(adm_access);
>
>> + err = svn_wc__open_adm_stream(&stream, dir_abspath,
>> + SVN_WC__ADM_DIR_WCPROPS,
>> + scratch_pool, scratch_pool);
>> + if (err)
>> + {
>> + /* If the file doesn't exist, it means no wcprops. */
>> + if (APR_STATUS_IS_ENOENT(err->apr_err))
>> + svn_error_clear(err);
>> + else
>> + return svn_error_return(err);
>> + }
>> + else
>> + {
>> + proplist = apr_hash_make(scratch_pool);
>> + SVN_ERR(svn_hash_read2(proplist, stream,
>> SVN_HASH_TERMINATOR,
>> + scratch_pool));
>> + SVN_ERR(svn_wc__db_base_set_dav_cache(db, dir_abspath,
>> proplist,
>> + scratch_pool));
>> + }
>> +
>> + /* Now walk the wcprops directory. */
>> +
>> SVN_ERR(svn_io_dir_walk(svn_wc__adm_child(svn_wc_adm_access_path(
>> +
>> adm_access),
>> + SVN_WC__ADM_WCPROPS,
>> + scratch_pool),
>> + 0 /* wanted */,
>> + convert_props_walker,
>> + db, scratch_pool));
>
> I understand this was the original/old code, but why walk rather than
> simply fetching the dirents?
Hmm, yes. Fixed.
>
>> + }
>> + else
>> + {
>> + apr_hash_t *allprops;
>> + apr_hash_index_t *hi;
>> + apr_pool_t *iterpool;
>> +
>> + /* Read the all-wcprops file. */
>> + SVN_ERR(read_wcprops(&allprops, db,
>> svn_wc_adm_access_path(adm_access),
>> + scratch_pool, scratch_pool));
>> +
>> + /* Iterate over all the wcprops, writing each one to the
>> wc_db. */
>> + iterpool = svn_pool_create(scratch_pool);
>> + for (hi = apr_hash_first(scratch_pool, allprops); hi;
>> + hi = apr_hash_next(hi))
>> + {
>> + const void *key;
>> + void *val;
>> + const char *name;
>> + apr_hash_t *proplist;
>> + const char *local_abspath;
>> +
>> + svn_pool_clear(iterpool);
>> +
>> + apr_hash_this(hi, &key, NULL, &val);
>> + name = key;
>> + proplist = val;
>> +
>> + SVN_ERR(svn_dirent_get_absolute(&local_abspath,
>> +
>> svn_wc_adm_access_path(adm_access),
>> + iterpool));
>
> I'd call this dir_abspath, locate it outside the loop, and use
> svn_wc__adm_access_abspath() to get it.
>
>> + local_abspath = svn_dirent_join(local_abspath, name,
>> iterpool);
>> + SVN_ERR(svn_wc__db_base_set_dav_cache(db, local_abspath,
>> proplist,
>> + iterpool));
>> + }
>> + svn_pool_destroy(iterpool);
>> + }
>> +
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +
>> +svn_error_t *
>> +svn_wc__upgrade_format(svn_wc_adm_access_t *adm_access,
>> + apr_pool_t *scratch_pool)
>> +{
>> + int wc_format;
>> + svn_boolean_t cleanup_required;
>> + svn_stringbuf_t *log_accum;
>> + const svn_wc_entry_t *this_dir;
>> +
>> + SVN_ERR(svn_wc__adm_wc_format(&wc_format, adm_access,
>> scratch_pool));
>> +
>> + if (wc_format > SVN_WC__VERSION)
>> + {
>> + return svn_error_createf
>> + (SVN_ERR_WC_UNSUPPORTED_FORMAT, NULL,
>> + _("This client is too old to work with working copy
>> '%s'. You need\n"
>> + "to get a newer Subversion client, or to downgrade this
>> working "
>> + "copy.\n"
>> + "See "
>> + "http://subversion.tigris.org/faq.html#working-copy-format-change
>> \n"
>> + "for details."
>> + ),
>> + svn_path_local_style(svn_wc_adm_access_path(adm_access),
>> + scratch_pool));
>> + }
>> +
>> + /* We can upgrade all formats that are accepted by
>> check_format(). */
>> + if (wc_format >= SVN_WC__VERSION)
>> + return SVN_NO_ERROR;
>
> This test makes no sense. You already checked for greater-than above.
> Further, the comment *totally* doesn't describe what is going on here.
>
>> +
>> + /* Don't try to mess with the WC if there are old log files
>> left. */
>> + SVN_ERR(svn_wc__adm_is_cleanup_required(&cleanup_required,
>> + adm_access,
>> scratch_pool));
>> +
>> + if (cleanup_required)
>> + return svn_error_create(SVN_ERR_WC_UNSUPPORTED_FORMAT, NULL,
>> + _("Cannot upgrade with existing logs;
>> please "
>> + "run 'svn cleanup' with Subversion
>> 1.6"));
>> +
>> + /* What's going on here?
>> + *
>> + * We're attempting to upgrade an older working copy to the new
>> wc-ng format.
>> + * The sematics and storage mechanisms between the two are
>> vastly different,
>> + * so it's going to be a bit painful. Here's a plan for the
>> operation:
>> + *
>> + * 1) The 'entries' file needs to be moved to the new format.
>> Ideally, we'd
>> + * read it using svn_wc__entries_read_old(), and then
>> translate the
>> + * current state of the file into a series of wc_db commands
>> to duplicate
>> + * that state in WC-NG. We're not quite there yet, so we
>> just use
>> + * the same loggy process as we always have, relying on the
>> lower layers
>> + * to take care of the translation, and remembering to remove
>> the old
>> + * entries file when were're done. ### This isn't a long-
>> term solution.
>> + *
>> + * 2) Convert wcprop to the wc-ng format
>> + *
>> + * ### (fill in other bits as they are implemented)
>> + */
>> +
>> + /***** ENTRIES *****/
>> + /* Create an empty sqlite database for this directory. */
>> + SVN_ERR(svn_wc_entry(&this_dir,
>> svn_wc_adm_access_path(adm_access),
>> + adm_access, FALSE, scratch_pool));
>> + SVN_ERR(svn_wc__entries_init(svn_wc_adm_access_path(adm_access),
>> + this_dir->uuid, this_dir->url,
>> + this_dir->repos, this_dir->revision,
>> + this_dir->depth, scratch_pool));
>> +
>> + /* Migrate the entries over to the new database.
>> + ### We need to thing about atomicity here. */
>> + SVN_ERR(svn_wc__entries_upgrade(adm_access, SVN_WC__VERSION,
>> scratch_pool));
>> +
>> svn_error_clear
>> (svn_io_remove_file(svn_wc__adm_child(svn_wc_adm_access_path(
>> +
>> adm_access),
>> +
>> SVN_WC__ADM_FORMAT,
>> +
>> scratch_pool),
>> + scratch_pool));
>> +
>> SVN_ERR(svn_io_remove_file(svn_wc__adm_child(svn_wc_adm_access_path(
>> +
>> adm_access),
>> + SVN_WC__ADM_ENTRIES,
>> + scratch_pool),
>> + scratch_pool));
>> +
>> + /* ### Note that lots of this content is cribbed from the old
>> format updater.
>> + ### The following code will change as the wc-ng format
>> changes and more
>> + ### stuff gets migrated to the sqlite format. */
>> +
>> + log_accum = svn_stringbuf_create("", scratch_pool);
>> +
>> + /***** WC PROPS *****/
>> + SVN_ERR(convert_wcprops(adm_access, wc_format, scratch_pool));
>> +
>> + SVN_ERR(svn_wc__write_log(adm_access, 0, log_accum,
>> scratch_pool));
>
> eh? what is this write_log doing here?
>
>> +
>> + if (wc_format <= SVN_WC__WCPROPS_MANY_FILES_VERSION)
>> + {
>> + /* Remove wcprops directory, dir-props, README.txt and empty-
>> file
>> + files.
>> + We just silently ignore errors, because keeping these
>> files is
>> + not catastrophic. */
>> +
>> + svn_error_clear(svn_io_remove_dir2(
>> + svn_wc__adm_child(svn_wc_adm_access_path(adm_access),
>> + SVN_WC__ADM_WCPROPS, scratch_pool),
>> + FALSE, NULL, NULL, scratch_pool));
>> + svn_error_clear(svn_io_remove_file(
>> + svn_wc__adm_child(svn_wc_adm_access_path(adm_access),
>> + SVN_WC__ADM_DIR_WCPROPS, scratch_pool),
>> + scratch_pool));
>> + svn_error_clear(svn_io_remove_file(
>> + svn_wc__adm_child(svn_wc_adm_access_path(adm_access),
>> + SVN_WC__ADM_EMPTY_FILE, scratch_pool),
>> + scratch_pool));
>> + svn_error_clear(svn_io_remove_file(
>> + svn_wc__adm_child(svn_wc_adm_access_path(adm_access),
>> + SVN_WC__ADM_README, scratch_pool),
>> + scratch_pool));
>> + }
>> + else
>> + {
>> + svn_error_clear(svn_io_remove_file(
>> + svn_wc__adm_child(svn_wc_adm_access_path(adm_access),
>> + SVN_WC__ADM_ALL_WCPROPS, scratch_pool),
>> + scratch_pool));
>> + }
>> +
>> + return svn_wc__run_log(adm_access, NULL, scratch_pool);
>
> and what is this? you didn't put anything into log_accum.
Removed.
Thanks for the review.
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2071798
Received on 2009-05-05 20:52:10 CEST