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

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;
 }