There are two short answers to your question:
1) we don't have a good handle on what to do with file externals in
wc-ng (we temporarily punted)
2) per the doc in BASE_NODE, file externals are just a copy of an
existing property, so we shouldn't need to duplicate the values into
another column
I'd be interested in asking why a separate column is needed. Can the
file externals code be rearranged to more directly interpret
svn:externals as needed, rather than referring to custom metadata?
Cheers,
-g
On Thu, Mar 18, 2010 at 16:52, C. Michael Pilato <cmpilato_at_collab.net> wrote:
> I started yesterday trying to convert a bit of svn_wc_entry_t-wielding code
> related to file externals into WC-NGness. I thought I had identified an
> easy abstraction of some related code, so I made that abstraction and found
> that it was a miserable failure.
>
> I'm attaching two patches. The first (file-externals-abstraction-patch.txt)
> just carves some code out of the entry-writing logic and drops it into
> wc_db.[ch]. This patch works fine.
>
> The second (file-externals-definition-patch.txt) was where I thought, "Hey,
> I can re-use the code that sets the file_external_* bits of the entry here,
> too!" I was wrong. And though it took me far too long to do so, I think I
> finally realize why I was wrong.
>
> See, in subversion/libsvn_client/externals.c:switch_file_external(), there
> is logic around file external handling that basically looks like this:
>
> - create a file scheduled for addition (using svn_wc_add4)
> - set the magic file_external_* bits on that file's entry
> (svn_wc__set_file_external_location)
> - switch the file to the external location (svn_client__switch_internal)
>
> In the current codebase, that works fine because that second step is using
> svn_wc__modify_entry2() which actually creates a BASE_NODE row for the new
> file with the file_external_* bits set. But I think this is a bit of a
> stretch in terms of legitimacy. Until the switch happens, this is just a
> new file scheduled for addition, right? There shouldn't be a base node for
> that object at all.
>
> My patched code doesn't work because there's not yet a base node for the new
> file, so attempts to update the file_external_* data on that base node
> silently fail.
>
> Clearly the code wants to hang this file external information somewhere.
> The WORKING tree lacks schema support for it. The switch logic needs it to
> make decisions about what it's doing, but it seems wrong to pass this bit of
> hackery through the public update/switch API. I hesitate to hack in yet
> another "private" workaround API.
>
> Anybody got any bright ideas or precedent that I can refer to?
>
> --
> C. Michael Pilato <cmpilato_at_collab.net>
> CollabNet <> www.collab.net <> Distributed Development On Demand
>
> Index: subversion/libsvn_wc/entries.c
> ===================================================================
> --- subversion/libsvn_wc/entries.c (revision 924936)
> +++ subversion/libsvn_wc/entries.c (working copy)
> @@ -2103,22 +2103,11 @@
> ### This is a hack! */
> if (entry->file_external_path)
> {
> - svn_sqlite__stmt_t *stmt;
> - const char *str;
> -
> - SVN_ERR(svn_wc__serialize_file_external(&str,
> + SVN_ERR(svn_wc__db_op_set_file_external(db, entry_abspath,
> entry->file_external_path,
> &entry->file_external_peg_rev,
> &entry->file_external_rev,
> scratch_pool));
> -
> - SVN_ERR(svn_sqlite__get_statement(&stmt, sdb,
> - STMT_UPDATE_FILE_EXTERNAL));
> - SVN_ERR(svn_sqlite__bindf(stmt, "iss",
> - (apr_uint64_t)1 /* wc_id */,
> - entry->name,
> - str));
> - SVN_ERR(svn_sqlite__step_done(stmt));
> }
> }
>
> Index: subversion/libsvn_wc/wc_db.c
> ===================================================================
> --- subversion/libsvn_wc/wc_db.c (revision 924936)
> +++ subversion/libsvn_wc/wc_db.c (working copy)
> @@ -2957,6 +2957,43 @@
>
>
> svn_error_t *
> +svn_wc__db_op_set_file_external(svn_wc__db_t *db,
> + const char *local_abspath,
> + const char *external_path,
> + const svn_opt_revision_t *external_peg_rev,
> + const svn_opt_revision_t *external_rev,
> + apr_pool_t *scratch_pool)
> +{
> + svn_wc__db_pdh_t *pdh;
> + const char *local_relpath;
> + svn_sqlite__stmt_t *stmt;
> + const char *str;
> +
> + SVN_ERR_ASSERT(svn_dirent_is_absolute(local_abspath));
> + SVN_ERR(parse_local_abspath(&pdh, &local_relpath, db, local_abspath,
> + svn_sqlite__mode_readwrite,
> + scratch_pool, scratch_pool));
> + VERIFY_USABLE_PDH(pdh);
> +
> + SVN_ERR(svn_wc__serialize_file_external(&str, external_path,
> + external_peg_rev, external_rev,
> + scratch_pool));
> +
> + SVN_ERR(svn_sqlite__get_statement(&stmt, pdh->wcroot->sdb,
> + STMT_UPDATE_FILE_EXTERNAL));
> + SVN_ERR(svn_sqlite__bindf(stmt, "iss",
> + pdh->wcroot->wc_id,
> + svn_relpath_basename(local_relpath, scratch_pool),
> + str));
> + SVN_ERR(svn_sqlite__step_done(stmt));
> +
> + flush_entries(pdh);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> +svn_error_t *
> svn_wc__db_op_revert(svn_wc__db_t *db,
> const char *local_abspath,
> svn_depth_t depth,
> Index: subversion/libsvn_wc/wc_db.h
> ===================================================================
> --- subversion/libsvn_wc/wc_db.h (revision 924936)
> +++ subversion/libsvn_wc/wc_db.h (working copy)
> @@ -1073,6 +1073,20 @@
> apr_pool_t *scratch_pool);
>
>
> +/** Set the file external definition associated with LOCAL_ABSPATH in
> + * DB. The definition consists of the EXTERNAL_PATH, EXTERNAL_PEG_REV
> + * revision, and EXTERNAL_REV working revision specification.
> + *
> + * Use SCRATCH_POOL for any temporary allocations.
> + */
> +svn_error_t *
> +svn_wc__db_op_set_file_external(svn_wc__db_t *db,
> + const char *local_abspath,
> + const char *external_path,
> + const svn_opt_revision_t *external_peg_rev,
> + const svn_opt_revision_t *external_rev,
> + apr_pool_t *scratch_pool);
> +
> /* ### status */
>
>
>
> Index: subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- subversion/libsvn_wc/adm_ops.c (revision 924379)
> +++ subversion/libsvn_wc/adm_ops.c (working copy)
> @@ -2655,29 +2655,28 @@
> const char *repos_root_url,
> apr_pool_t *scratch_pool)
> {
> - svn_wc_entry_t entry = { 0 };
> + const char *external_path;
> + svn_opt_revision_t external_peg_rev, external_rev;
>
> if (url)
> {
> /* A repository root relative path is stored in the entry. */
> SVN_ERR_ASSERT(peg_rev);
> SVN_ERR_ASSERT(rev);
> - entry.file_external_path = url + strlen(repos_root_url);
> - entry.file_external_peg_rev = *peg_rev;
> - entry.file_external_rev = *rev;
> + external_path = url + strlen(repos_root_url);
> + external_peg_rev = *peg_rev;
> + external_rev = *rev;
> }
> else
> {
> - entry.file_external_path = NULL;
> - entry.file_external_peg_rev.kind = svn_opt_revision_unspecified;
> - entry.file_external_rev.kind = svn_opt_revision_unspecified;
> + external_path = NULL;
> + external_peg_rev.kind = svn_opt_revision_unspecified;
> + external_rev.kind = svn_opt_revision_unspecified;
> }
>
> - SVN_ERR(svn_wc__entry_modify2(wc_ctx->db, local_abspath,
> - svn_node_unknown, FALSE,
> - &entry, SVN_WC__ENTRY_MODIFY_FILE_EXTERNAL,
> - scratch_pool));
> -
> + SVN_ERR(svn_wc__db_op_set_file_external(wc_ctx->db, local_abspath,
> + external_path, &external_peg_rev,
> + &external_rev, scratch_pool));
> return SVN_NO_ERROR;
> }
>
>
>
Received on 2010-03-18 22:16:13 CET