Re: [PATCH] svn_wc_add3(): handles depth on add

On Tue, May 06, 2008 at 02:06:50AM -0400, Karl Fogel wrote:
> > * subversion/include/svn_wc.h
> > (svn_wc_add3): New, accept depth parameters and ignore it for files.
> > (svn_wc_add2): Now deprecated.
>
> "parameter"
>
> Also, you don't have to document the function here -- you're going to do
> that in the .h file anyway. So just say:
>
> * subversion/include/svn_wc.h
> (svn_wc_add3): New function.
> (svn_wc_add2): Deprecate.
>
> Or if you want to emphasize the purpose of this change, you could say:
>
> * subversion/include/svn_wc.h
> (svn_wc_add3): New function, taking --depth parameter.
> (svn_wc_add2): Deprecate.
>
> But I think even the first way is enough.
>
> > * subversion/libsvn_wc/adm_ops.c
> > (svn_wc_add3): New, accept depth parameters and ignore it for files.
> > Obsolete comment deleted.
>
> "parameter"

Karl, thank you very much for your careful review and detailed direction. This
the first time that I submit a patch consist of a change set covering many
source files. I think I can do it better next time under your kind direction.
:-)

And about the grammar pitfall, the problem of mismatching singular and plural
is hard to catch my attention even when I'm checking for them, shy. This
problem seems common to Chinese English speakers since we do not distinguish
them in our mother tongue.

> The purpose of a log message is to give an overview, and to prepare the
> reader's mind for the actual diff. The log message should supplement
> the diff, not duplicate it.

Great! Here is the core idea. I should keep it in my mind.

> > --- subversion/include/svn_wc.h (revision 31014)
> > +++ subversion/include/svn_wc.h (working copy)
> > @@ -2751,6 +2751,9 @@
> > *
> > * If @a path does not exist, return @c SVN_ERR_WC_PATH_NOT_FOUND.
> > *
> > + * The @a path is added at @a depth if it is a directory. The @a depth is
> > + * ignored for files.
> > + *
>
> Good! We generally try to write in the "active voice", that is, the
> mode of English in which a subject directly drives each verb. So:

Forgive me, I'm used to academic English too much than daily English. :-)

>
> Other than that stuff, the patch looks very good. I applied it, made
> tweaks as outlined above, and ran 'make check'. These tests failed:
>
> switch_tests.py 26: switch to dir_at_peg where dir doesn't exist in HEAD
> switch_tests.py 28: switch to old rev of now renamed branch
> log_tests.py 16: test 'svn log -g' on a single revision
> log_tests.py 19: test 'svn log -g' a path added before merge
> merge_tests.py 19: merge should skip over unversioned obstructions
> blame_tests.py 10: test 'svn blame -g'
> blame_tests.py 11: don't look for merged files out of range
>
> So I reverted the patch and tried with a clean trunk_at_r31039. This time,
> no failures. Could you look into this, please? Here's the adjusted
> version of the patch that I used. It should be functionally the same as
> yours, I only tweaked comments and formatting.

I should say sorry, Karl, for wasting your time on this. I admit that I did
not run a full check (too slow, don't you think?) before I submit the patch,
or I would have discovered this by myself.

The problem is that the "svn_client__make_local_parents()" function, which is
called by mkdir & copy logic, used a hard-coded svn_depth_empty when calling
svn_client_add4() to add it to the repository. This is OK before the patch is
applied, however, problem arise now since the depth of new directory is
unexpectedly set to empty. Changing the constant to svn_depth_infinity fixes
all the above failure.

When digging into this problem, I found that 'svn merge' will still incur
local modification (to its children)even when --depth is empty, is this okay?

Rui, Guo

svn_add4.diff
[[[
Introduce svn_wc_add3(), which takes a depth parameter. This makes
'svn add --depth' work the same way as commit, update, etc.

* subversion/include/svn_wc.h, subversion/libsvn_wc/adm_ops.c
  (svn_wc_add3): New function.
  (svn_wc_add2): Deprecate.

* subversion/libsvn_wc/copy.c
  (copy_added_file_administratively, copy_added_dir_administratively,
   copy_dir_administratively): Call svn_wc_add3 now.

* subversion/libsvn_client/merge.c
  (merge_dir_added): Call svn_wc_add3 now.

* subversion/libsvn_client/copy.c
  (repos_to_wc_copy_single): Call svn_wc_add3 now.

* subversion/libsvn_client/add.c
  (add_file, add_dir_recursive, add_parent_dirs): Call svn_wc_add3 now.
  (add): Same, and always call add_dir_recursive for directories.
    Add braces to conditional chain, for readability.
  (svn_cl__make_local_parents): Call svn_client_add4 with svn_depth_infinity.

* subversion/tests/cmdline/depth_tests.py
  (add_tree_with_depth): New name for add_tree_with_depth_files.
    Verify the depth, and test --force and --parents.
  (test_list): Adjust accordingly.
]]]

svn_add4_cleanup.diff
[[[
Some cleanups:
* subversion/include/svn_client.h, subversion/libsvn_client/add.c
  (svn_client_add4): Fold up duplicated code in both path of a branch
  (svn_client_add3): Map the -N to svn_depth_empty, make it consistent with
    the mapping in svn/main.c
]]]

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org