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

[PATCH] Make 'svn commit --depth=foo' work.

From: Karl Fogel <kfogel_at_red-bean.com>
Date: 2007-07-11 09:40:27 CEST

This email contains:

  1. A log message (surrounded by "[[[ ... ]]]")
  2. A patch (also surrounded by "[[[ ... ]]]")
  3. A shell script for testing the patch (also surrounded by "[[[ ... ]]]")

I'd love some review on this. It needs regression tests: what I plan
to do is convert that shell script over to our test harness. But for
getting this coded, it was easier to have a test script I could tweak
quickly.

This patch is in 'make check' right now. It passed basic_tests, but
got one failure in commit_tests:

   commit_tests.py 26: commit named targets with -N (issues #1195, #1239)

(It's currently in prop_tests, but has encountered only that single
failure so far.)

I haven't yet had a chance to look at how serious the commit_tests #26
failure is. It might just be a case of "-N" behavior changing
slightly (i.e., we should change the test, not the code), or perhaps
it's a real bug in the code. I'll take a look at it later; right now
it's time for bed :-)

In the meantime, I wanted to send this so the change could get some
review.

-Karl

[[[
Make 'commit --depth=foo' work.

     ################################################################
     ### ###
     ### PATCH IN PROGRESS -- DO NOT COMMIT ###
     ### ###
     ### This seems to work, but I haven't finished running the ###
     ### regression tests yet, nor have I written new tests to ###
     ### cover this new functionality. Please don't commit it. ###
     ### ###
     ################################################################

* subversion/include/svn_client.h, subversion/libsvn_client/commit.c
  (svn_client_commit4): Change recurse parameter to depth.

* subversion/libsvn_client/client.h
  (svn_client__harvest_committables): Take depth instead of nonrecursive.

* subversion/libsvn_client/commit_util.c
  (svn_client__harvest_committables): Take depth instead of nonrecursive.
  (harvest_committables): Same, and remove a longstanding TODO with a
    satisfying "*thwack*!".
  (svn_client__get_copy_committables): Pass svn_depth_infinity,
    not nonrecursive=FALSE, to harvest_committables.

* subversion/svn/commit-cmd.c
  (svn_cl__commit): Pass opt_state->depth directly.

* notes/sparse-directories.txt: Remove item about svn_client_commit4.
]]]

[[[
Index: notes/sparse-directories.txt
===================================================================
--- notes/sparse-directories.txt (revision 25714)
+++ notes/sparse-directories.txt (working copy)
@@ -460,7 +460,6 @@
 
            svn_client_import2() # Note: takes 'nonrecursive' right now
            svn_client_revert() # Any point taking depth?
- svn_client_commit4() # Is manual control of depth needed here?
            svn_client_propset3() # Is manual control of depth needed here?
            svn_client_propget3() # Is manual control of depth needed here?
            svn_client_resolved() # Is manual control of depth needed here?
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 25714)
+++ subversion/include/svn_client.h (working copy)
@@ -1512,15 +1512,17 @@
  * @c svn_wc_notify_commit_deleted, @c svn_wc_notify_commit_replaced,
  * @c svn_wc_notify_commit_postfix_txdelta.
  *
- * ### TODO(sd): For consistency, this should probably take svn_depth_t
- * ### depth instead of svn_boolean_t recurse. But it's not needed
- * ### for the sparse-directories work right now, so leaving it alone
- * ### for now, although note that this is a 1.5 API so revving it
- * ### would be fairly painless.
+ * If @a depth is @c svn_depth_infinity, commit all changes to and
+ * below named targets. If @a depth is @c svn_depth_empty, commit
+ * only named targets (that is, only property changes on named
+ * directory targets, and property and content changes for named file
+ * targets). If @a depth is @c svn_depth_files, behave as above for
+ * named file targets, and for named directory targets, commit
+ * property changes on a named directory and all changes to files
+ * directly inside that directory. If @c svn_depth_immediates, behave
+ * as for @c svn_depth_files, and for subdirectories of any named
+ * directory target commit as though for @c svn_depth_empty.
  *
- * If @a recurse is false, subdirectories of directories in @a targets
- * will be ignored.
- *
  * Unlock paths in the repository, unless @a keep_locks is true.
  *
  * If @a changelist_name is non-NULL, then use it as a restrictive filter
@@ -1540,7 +1542,7 @@
 svn_error_t *
 svn_client_commit4(svn_commit_info_t **commit_info_p,
                    const apr_array_header_t *targets,
- svn_boolean_t recurse,
+ svn_depth_t depth,
                    svn_boolean_t keep_locks,
                    svn_boolean_t keep_changelist,
                    const char *changelist_name,
Index: subversion/libsvn_client/client.h
===================================================================
--- subversion/libsvn_client/client.h (revision 25714)
+++ subversion/libsvn_client/client.h (working copy)
@@ -697,8 +697,9 @@
    locked in the process of this crawl. These will need to be
    unlocked again post-commit.
 
- If NONRECURSIVE is specified, subdirectories of directory targets
- found in TARGETS will not be crawled for modifications.
+ If DEPTH is specified, descend (or not) into each target in TARGETS
+ as specified by DEPTH; the behavior is the same as that described
+ for svn_client_commit4().
 
    If JUST_LOCKED is TRUE, treat unmodified items with lock tokens as
    commit candidates.
@@ -715,7 +716,7 @@
                                  apr_hash_t **lock_tokens,
                                  svn_wc_adm_access_t *parent_dir,
                                  apr_array_header_t *targets,
- svn_boolean_t nonrecursive,
+ svn_depth_t depth,
                                  svn_boolean_t just_locked,
                                  const char *changelist_name,
                                  svn_client_ctx_t *ctx,
Index: subversion/libsvn_client/commit_util.c
===================================================================
--- subversion/libsvn_client/commit_util.c (revision 25714)
+++ subversion/libsvn_client/commit_util.c (working copy)
@@ -186,9 +186,13 @@
    entry ENTRY and ancestry URL), and add those candidates to
    COMMITTABLES. If in ADDS_ONLY modes, only new additions are
    recognized. COPYFROM_URL is the default copyfrom-url for children
- of copied directories. NONRECURSIVE indicates that this function
- will not recurse into subdirectories of PATH when PATH is itself a
- directory. Lock tokens of candidates will be added to LOCK_TOKENS, if
+ of copied directories.
+
+ DEPTH indicates how to treat files and subdirectories of PATH when
+ PATH is itself a directory; see svn_client__harvest_committables()
+ for its behavior.
+
+ Lock tokens of candidates will be added to LOCK_TOKENS, if
    non-NULL. JUST_LOCKED indicates whether to treat non-modified items with
    lock tokens as commit candidates.
 
@@ -216,7 +220,7 @@
                      const svn_wc_entry_t *parent_entry,
                      svn_boolean_t adds_only,
                      svn_boolean_t copy_mode,
- svn_boolean_t nonrecursive,
+ svn_depth_t depth,
                      svn_boolean_t just_locked,
                      const char *changelist_name,
                      svn_client_ctx_t *ctx,
@@ -505,13 +509,10 @@
         }
     }
 
- /* For directories, recursively handle each of their entries (except
- when the directory is being deleted, unless the deletion is part
- of a replacement ... how confusing). Oh, and don't recurse at
- all if this is a nonrecursive commit. ### We'll probably make
- the whole 'nonrecursive' concept go away soon and be replaced
- with the more sophisticated Depth0|Depth1|DepthInfinity. */
- if (entries && (! nonrecursive)
+ /* For directories, recursively handle each entry according to depth
+ (except when the directory is being deleted, unless the deletion
+ is part of a replacement ... how confusing). */
+ if (entries && (depth > svn_depth_empty)
       && ((! (state_flags & SVN_CLIENT_COMMIT_ITEM_DELETE))
           || (state_flags & SVN_CLIENT_COMMIT_ITEM_ADD)))
     {
@@ -561,64 +562,94 @@
           /* Recurse. */
           if (this_entry->kind == svn_node_dir)
             {
- svn_error_t *lockerr;
- lockerr = svn_wc_adm_retrieve(&dir_access, adm_access,
- full_path, loop_pool);
-
- if (lockerr)
+ if (depth <= svn_depth_files)
                 {
- if (lockerr->apr_err == SVN_ERR_WC_NOT_LOCKED)
+ /* Don't bother with any of this if it's a directory
+ and depth says not to go there. */
+ continue;
+ }
+ else
+ {
+ svn_error_t *lockerr;
+ lockerr = svn_wc_adm_retrieve(&dir_access, adm_access,
+ full_path, loop_pool);
+
+ if (lockerr)
                     {
- /* A missing, schedule-delete child dir is
- allowable. Just don't try to recurse. */
- svn_node_kind_t childkind;
- svn_error_t *err = svn_io_check_path(full_path,
- &childkind,
- loop_pool);
- if (! err
- && childkind == svn_node_none
- && this_entry->schedule == svn_wc_schedule_delete)
+ if (lockerr->apr_err == SVN_ERR_WC_NOT_LOCKED)
                         {
- if ((changelist_name == NULL)
- || (entry->changelist
- && (strcmp(changelist_name,
- entry->changelist) == 0)))
+ /* A missing, schedule-delete child dir is
+ allowable. Just don't try to recurse. */
+ svn_node_kind_t childkind;
+ svn_error_t *err = svn_io_check_path(full_path,
+ &childkind,
+ loop_pool);
+ if (! err
+ && (childkind == svn_node_none)
+ && (this_entry->schedule
+ == svn_wc_schedule_delete))
                             {
- add_committable(committables, full_path,
- this_entry->kind, used_url,
- SVN_INVALID_REVNUM,
- NULL,
- SVN_INVALID_REVNUM,
- SVN_CLIENT_COMMIT_ITEM_DELETE);
- svn_error_clear(lockerr);
- continue; /* don't recurse! */
+ if ((changelist_name == NULL)
+ || (entry->changelist
+ && (strcmp(changelist_name,
+ entry->changelist) == 0)))
+ {
+ add_committable(
+ committables, full_path,
+ this_entry->kind, used_url,
+ SVN_INVALID_REVNUM,
+ NULL,
+ SVN_INVALID_REVNUM,
+ SVN_CLIENT_COMMIT_ITEM_DELETE);
+ svn_error_clear(lockerr);
+ continue; /* don't recurse! */
+ }
                             }
+ else
+ {
+ svn_error_clear(err);
+ return lockerr;
+ }
                         }
                       else
- {
- svn_error_clear(err);
- return lockerr;
- }
+ return lockerr;
                     }
- else
- return lockerr;
                 }
             }
           else
- dir_access = adm_access;
+ {
+ dir_access = adm_access;
+ }
 
- SVN_ERR(harvest_committables
- (committables, lock_tokens, full_path, dir_access,
- used_url ? used_url : this_entry->url,
- this_cf_url,
- this_entry,
- entry,
- adds_only,
- copy_mode,
- FALSE, just_locked,
- changelist_name,
- ctx,
- loop_pool));
+ {
+ svn_depth_t depth_below_here = depth;
+
+ /* If depth is svn_depth_files, then we must be recursing
+ into a file, or else we wouldn't be here -- either way,
+ svn_depth_empty is the right depth to use. On the
+ other hand, if depth is svn_depth_immediates, then we
+ could be recursing into a directory or a file -- in
+ which case svn_depth_empty is *still* the right depth
+ to use. I know that sounds bizarre (one normally
+ expects to just do depth - 1) but it's correct. */
+ if (depth == svn_depth_immediates
+ || depth == svn_depth_files)
+ depth_below_here = svn_depth_empty;
+
+ SVN_ERR(harvest_committables
+ (committables, lock_tokens, full_path, dir_access,
+ used_url ? used_url : this_entry->url,
+ this_cf_url,
+ this_entry,
+ entry,
+ adds_only,
+ copy_mode,
+ depth_below_here,
+ just_locked,
+ changelist_name,
+ ctx,
+ loop_pool));
+ }
         }
 
       svn_pool_destroy(loop_pool);
@@ -642,7 +673,7 @@
                                  apr_hash_t **lock_tokens,
                                  svn_wc_adm_access_t *parent_dir,
                                  apr_array_header_t *targets,
- svn_boolean_t nonrecursive,
+ svn_depth_t depth,
                                  svn_boolean_t just_locked,
                                  const char *changelist_name,
                                  svn_client_ctx_t *ctx,
@@ -772,7 +803,7 @@
                                   subpool));
       SVN_ERR(harvest_committables(*committables, *lock_tokens, target,
                                    dir_access, entry->url, NULL,
- entry, NULL, FALSE, FALSE, nonrecursive,
+ entry, NULL, FALSE, FALSE, depth,
                                    just_locked, changelist_name,
                                    ctx, subpool));
 
@@ -857,8 +888,8 @@
          allocate the new commit_item, we can safely use the iterpool here. */
       SVN_ERR(harvest_committables(*committables, NULL, pair->src,
                                    dir_access, pair->dst, entry->url, entry,
- NULL, FALSE, TRUE, FALSE, FALSE,
- NULL, ctx, iterpool));
+ NULL, FALSE, TRUE, svn_depth_infinity,
+ FALSE, NULL, ctx, iterpool));
     }
 
   svn_pool_destroy(iterpool);
Index: subversion/libsvn_client/commit.c
===================================================================
--- subversion/libsvn_client/commit.c (revision 25714)
+++ subversion/libsvn_client/commit.c (working copy)
@@ -1156,7 +1156,7 @@
 svn_error_t *
 svn_client_commit4(svn_commit_info_t **commit_info_p,
                    const apr_array_header_t *targets,
- svn_boolean_t recurse,
+ svn_depth_t depth,
                    svn_boolean_t keep_locks,
                    svn_boolean_t keep_changelist,
                    const char *changelist_name,
@@ -1193,9 +1193,23 @@
            _("'%s' is a URL, but URLs cannot be commit targets"), target);
     }
 
- /* Condense the target list. */
+ /* Condense the target list.
+
+ ### TODO(sd): Back when svn_path_condense_targets() was written,
+ ### there was no depth, only a recurse boolean, so condensation
+ ### was a yes-no proposition.
+ ###
+ ### Nowadays things are a little more complex. For example, if
+ ### depth is svn_depth_files, and two targets are "foo" and
+ ### "foo/bar", we should ideally condense out the latter if
+ ### "foo/bar" is a file but not if it is a directory.
+ ###
+ ### However, the code isn't that smart yet, which might mean
+ ### we'll grab more/deeper recursive adm locks than we really
+ ### need.
+ */
   SVN_ERR(svn_path_condense_targets(&base_dir, &rel_targets, targets,
- recurse, pool));
+ depth == svn_depth_infinity, pool));
 
   /* No targets means nothing to commit, so just return. */
   if (! base_dir)
@@ -1235,7 +1249,7 @@
           /* If the final target is a dir, we want to recursively lock it */
           if (kind == svn_node_dir)
             {
- if (recurse)
+ if (depth == svn_depth_infinity || depth == svn_depth_immediates)
                 APR_ARRAY_PUSH(dirs_to_lock_recursive, const char *) = target;
               else
                 APR_ARRAY_PUSH(dirs_to_lock, const char *) = target;
@@ -1271,7 +1285,7 @@
           /* If the final target is a dir, we want to lock it */
           if (kind == svn_node_dir)
             {
- if (recurse)
+ if (depth == svn_depth_infinity || depth == svn_depth_immediates)
                 APR_ARRAY_PUSH(dirs_to_lock_recursive,
                                const char *) = apr_pstrdup(pool, target);
               else
@@ -1391,7 +1405,25 @@
                                           target, pool),
                 _("Are all the targets part of the same working copy?"));
 
- if (!recurse)
+ /* ### TODO(sd): This check is slightly too strict. It should be
+ ### possible to:
+ ###
+ ### * delete an empty directory when depth==svn_depth_empty;
+ ###
+ ### * delete a directory containing only files when
+ ### depth==svn_depth_files;
+ ###
+ ### * delete a directory containing only files and empty
+ ### subdirs when depth==svn_depth_immediates.
+ ###
+ ### But for now, we insist on svn_depth_infinity if you're
+ ### going to delete a directory, because we're lazy and
+ ### trying to get depthy commits working in the first place.
+ ###
+ ### This would be fairly easy to fix, though: just, well,
+ ### check the above conditions!
+ */
+ if (depth != svn_depth_infinity)
         {
           svn_wc_status2_t *status;
           svn_node_kind_t kind;
@@ -1415,7 +1447,7 @@
                                                   &lock_tokens,
                                                   base_dir_access,
                                                   rel_targets,
- recurse ? FALSE : TRUE,
+ depth,
                                                   ! keep_locks,
                                                   changelist_name,
                                                   ctx,
Index: subversion/svn/commit-cmd.c
===================================================================
--- subversion/svn/commit-cmd.c (revision 25714)
+++ subversion/svn/commit-cmd.c (working copy)
@@ -100,7 +100,7 @@
           (ctx->log_msg_baton3,
            svn_client_commit4(&commit_info,
                               targets,
- SVN_DEPTH_TO_RECURSE(opt_state->depth),
+ opt_state->depth,
                               no_unlock,
                               opt_state->keep_changelist,
                               opt_state->changelist,
]]]

[[[
#!/bin/sh

# Put this script in its own directory and run it. If you set the
# line below correctly, everything else should work automatically.

# Change this to wherever your Subversion source build lives:
BASE_DIR=/home/kfogel/src/subversion

# You shouldn't have to change anything below this line, but if you
# want to test over http:// instead of svn://, you could choose a
# different URL here.
URL=svn://localhost/repos
# URL=http://localhost/SOMETHING/repos
# URL=file:///`pwd`/repos

SVN=${BASE_DIR}/subversion/svn/svn
SVNSERVE=${BASE_DIR}/subversion/svnserve/svnserve
SVNADMIN=${BASE_DIR}/subversion/svnadmin/svnadmin

rm -rf import-me repos wc

${SVNADMIN} create repos
echo "[general]" > repos/conf/svnserve.conf
echo "anon-access = write" >> repos/conf/svnserve.conf
echo "auth-access = write" >> repos/conf/svnserve.conf

${SVNSERVE} --pid-file svnserve-pid -d -r `pwd`

echo "### Making a Greek Tree for import..."
mkdir import-me
mkdir import-me/A
mkdir import-me/A/B/
mkdir import-me/A/C/
mkdir import-me/A/D/
mkdir import-me/A/B/E/
mkdir import-me/A/B/F/
mkdir import-me/A/D/G/
mkdir import-me/A/D/H/
echo "This is the file 'iota'." > import-me/iota
echo "This is the file 'A/mu'." > import-me/A/mu
echo "This is the file 'A/B/lambda'." > import-me/A/B/lambda
echo "This is the file 'A/B/E/alpha'." > import-me/A/B/E/alpha
echo "This is the file 'A/B/E/beta'." > import-me/A/B/E/beta
echo "This is the file 'A/D/gamma'." > import-me/A/D/gamma
echo "This is the file 'A/D/G/pi'." > import-me/A/D/G/pi
echo "This is the file 'A/D/G/rho'." > import-me/A/D/G/rho
echo "This is the file 'A/D/G/tau'." > import-me/A/D/G/tau
echo "This is the file 'A/D/H/chi'." > import-me/A/D/H/chi
echo "This is the file 'A/D/H/omega'." > import-me/A/D/H/omega
echo "This is the file 'A/D/H/psi'." > import-me/A/D/H/psi
echo "### Done."
echo ""

echo "### Importing it..."
(cd import-me; ${SVN} import -q -m "Initial import." ${URL})
echo "### Done."
echo ""

echo "### Test that depth-upgrading a working copy works as expected. ###"
echo ""
echo "### Checking out a depth-files working copy..."
${SVN} co --depth=files -q ${URL}/ wc
echo "### Done."
echo ""
echo "### Listing the working copy:"
ls -R wc
echo ""
echo "### Now updating it to depth-infinity..."
(cd wc; ${SVN} up -q --depth=infinity)
echo "### Done."
echo ""
echo "### Listing the working copy:"
ls -R wc
echo ""
echo "### If you got just 'iota' in the first listing above, and a"
echo "### full Greek Tree in the second listing, then upgrading from"
echo "### depth-files to depth-infinity worked as expected."
echo ""

# Set up some changes.
${SVN} propset -q foo bar wc/A/D
${SVN} propset -q color blue wc/A/D/H
echo "This is a change to A/D/gamma." >> wc/A/D/gamma
echo "This is a change to A/D/G/pi." >> wc/A/D/G/pi
echo "This is a change to A/D/H/omega." >> wc/A/D/H/omega

echo "### All the commit tests will take place inside wc/A/D/. ###"
echo ""

# Commit with --depth=empty.
cd wc
echo "### 'svn status -q' in the working copy before any commits:"
${SVN} st -q
echo ""
echo "### There should be three modified files and two propchanges above."
echo ""

# Commit with --depth=empty.
echo "### Test that committing --depth=empty works as expected. ###"
echo ""
cd A/D
echo "### Committing with --depth=empty from inside A/D/."
${SVN} commit -m 'Commit just the propchange to D.' --depth=empty
echo "### Done."
echo ""
cd ../..

# Test that --depth=empty worked as expected.
echo "### 'svn status -q' in the working copy after the commit:"
${SVN} st -q
echo ""
echo "### There should be three modified files and one propchange above,"
echo "### else 'svn commit --depth=empty' isn't working right."
echo ""

# Make there be a propchange on D again, since it should be committed
# with every deeper depth as well.
${SVN} propset -q foo baz A/D

# Commit with --depth=files.
echo "### Test that committing --depth=files works as expected. ###"
echo ""
cd A/D
echo "### Committing with --depth=files from inside A/D/."
${SVN} commit -m 'Commit one propchange and gamma.' --depth=files
echo "### Done."
echo ""
cd ../..

# Test that --depth=files worked as expected.
echo "### 'svn status -q' in the working copy after the commit:"
${SVN} st -q
echo ""
echo "### There should be two modified files and one propchange above,"
echo "### else 'svn commit --depth=files' isn't working right."
echo ""

# Restore propchange again and file mod.
${SVN} propset -q foo qux A/D
echo "This is another change to A/D/gamma." >> A/D/gamma

# Commit with --depth=immediates.
echo "### Test that committing --depth=immediates works as expected. ###"
echo ""
cd A/D
echo "### Committing with --depth=immediates from inside A/D/."
${SVN} commit -m 'Commit two propchanges and gamma.' --depth=immediates
echo "### Done."
echo ""
cd ../..

# Test that --depth=immediates worked as expected.
echo "### 'svn status -q' in the working copy after the commit:"
${SVN} st -q
echo ""
echo "### There should be two modified files (in G and H) above, else"
echo "### 'svn commit --depth=immediates' isn't working right."
echo ""

# Restore propchanges and file mod.
${SVN} propset -q foo quux A/D
${SVN} propset -q color green A/D/H
echo "This is another change to A/D/gamma." >> A/D/gamma

# Commit with --depth=infinity.
echo "### Test that committing --depth=infinity works as expected. ###"
echo "### "
cd A/D
echo "Committing with --depth=infinity from inside A/D/."
${SVN} commit -m 'Commit two propchanges and three files.' --depth=infinity
echo "### Done."
echo ""
cd ../..

# Test that --depth=infinity worked as expected.
echo "### 'svn status -q' in the working copy after the commit:"
${SVN} st -q
echo ""
echo "### There should be no modifications above, else"
echo "### 'svn commit --depth=infinity' isn't working right."
echo ""

# Return to parent of wc.
cd ..

# Shut down the server.
kill -9 `cat svnserve-pid`
]]]

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jul 11 09:40:01 2007

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