2008/4/25 Rui, Guo <timmyguo_at_mail.ustc.edu.cn>:
> I attach the patch in this mail to fix this in-consistency. This is my first
> code patch and takes me some time to go through the code.
>
> Function svn_wc_add2() is the one that is responsible to actually add items to
> wc and schedule an add at commit time. It does not take a depth parameter and
> is not appropriate to be tuned to accept one, since it handles both directory
> and file items. The function always use an infinity depth when adding new
> directory, since there is no way to get the proper depth value. (And there is
> a comment dated back to r26607 -- by you, Karl -- declaring that this is a
> proper behavior.) So, the proper depth should be adjusted by the caller of
> svn_wc_add2() when needed.
Hmm. I'm not sure I follow here. I agree that svn_wc_add2 doesn't
provide a way to specify the right depth, but I think it would be
perfectly reasonable to just add a depth argument to it, which is
ignored for file arguments.
I'm not sure that the "write something incorrect to disk, then write
the correct thing" is better than just "introduce arguments that let
you write the right thing in the first place".
> However, there is no way to change only the depth of an existing dir outside
> the libsvn_wc code base. I introduced a helper function for this purpose. And
> the problem remained is simply call it at proper places.
This helper seems somewhat dangerous. You generally can't just change
the depth in the absence of everything else (except for between
immediates and infinity). For example, if you have a directory at
depth-empty and base rev 15, with a few children in it, and you change
it to depth-infinity, then svn is going to assume that this means that
at rev 15 the directory contained exactly those children. (That's why
we currently only let you change depth with an update command.) Now,
sure, it's safe for a brand-new directory, or (I think) a schedule-add
directory... but exposing this helper publicly and documenting it as
generally useful seems like a bad idea.
> Currently, the
> following two behavior is maintained: 1) existing dir is never touched for
> 'svn add --force --depth=xxx existing_dir'; 2) intermediate parents is set to
> svn_depth_empty for 'svn add --parents target_deep_in_subtree'.
Good insight; I wouldn't have thought about the --parents case! I
haven't completely thought things through to be sure if the
parents-should-be-empty choice is correct, but the fact that it's
worth thinking about is a great catch.
> I think it would be handy to have an notification when there are items newly
> added (since last update) but just excluded by the depth setting of current
> wc. This should include a new file/dir immediately under a existing directory
> that is set to empty/files, but should not include a new item deeply under a
> totally excluded subtree. What do you think about it?
Hmm. In a sense that completely defeats the point of
depth-in-the-repos-reporter (on the API level), which is "tell the
repository to give me a very finely-tuned directory delta". It does
seem like it might be a useful optional feature for users, though.
--dave
> Here are change logs for the patch:
> [[[
> Make the --depth option in svn add works in the same way with svn ci, up etc.
>
> * subversion/include/svn_wc.h
> (svn_wc_add_adjust_depth): Declare the new helper function.
>
> * subversion/libsvn_wc/adm_ops.c
> (svn_wc_add2): Update the comment about depth to indicate that this function
> default to svn_depth_infinity
> (svn_wc_add_adjust_depth): New helper function to adjust depth.
>
> * subversion/libsvn_client/add.c
> (add_dir_recursive): Adjust depth after a directory is added, only when it
> is really a new directory.
> (add): Pass all directory target to add_dir_recursive, otherwise adjustment
> will also be needed here.
> (add_parent_dirs): Adjust intermediate parents to svn_depth_empty
>
> * subversion/tests/cmdline/depth_tests.py
> (add_tree_with_depth_files): verify depth & added new tests for the --force
> & --parents situation
> ]]]
>
> And I also made some cleanups for the code:
> [[[
> Some cleanups:
> * subversion/include/svn_client.h
> (svn_client_add3): Update the document that -N is mapped to svn_depth_empty
>
> * 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
> ]]]
>
> PS: I'm really not good at splitting a unified patch into logical independent
> changes sets. What's the best way of handling this? I just edit the patch file
> by hand this time and not sure about whether they are still valid.
>
> Rui, Guo
> On Tue, Apr 22, 2008 at 11:10:47AM -0400, Karl Fogel wrote:
> > "Rui, Guo" <timmyguo_at_mail.ustc.edu.cn> writes:
> > >> I think I do too -- that is, the operational depth becomes the "set"
> > >> depth for added trees. Would you like to try writing the patch?
> > >
> > > Certainly I would like to. However, it may take me some time to handle
> > > this, since this will be my first patch on the code. :)
> >
> > Well, since your Summer of Code project was accepted (congratulations),
> > this is a good place to start! :-)
> >
> > We're always here for questions, of course.
> >
> > -Karl
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
> > For additional commands, e-mail: dev-help_at_subversion.tigris.org
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
> For additional commands, e-mail: dev-help_at_subversion.tigris.org
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-04-26 00:46:46 CEST