[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Need help with "the WC-NG way" of dealing with file externals.

From: Greg Stein <gstein_at_gmail.com>
Date: Thu, 18 Mar 2010 17:15:44 -0400

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

This is an archived mail posted to the Subversion Dev mailing list.