On Fri, Apr 25, 2008 at 03:46:31PM -0700, David Glasser wrote:
> 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".
>
Well, it's a hard decision here and I'm not sure that I made the right
decision. The standpoint is that svn_wc_add2() is used in many situations:
add, merge and copy etc. The depth only makes sense in a small piece of
situations. That is, adding a directory (not files, which should be much more
than dir) in 'svn add' command. If such a parameter is introduced, most of the
time it only add the burden to the caller (to fill in a nonsense) and looks
ugly.
An alternative should be just introduce a specialized svn_wc_add_dir() only
for 'svn add' and don't make it deprecate svn_wc_add2() (as we generally do
when we introduce something like svn_wc_add3()). Does this seem to be a better
choice? In this way, the default_and_fix way will still have to be used, to
reuse the implementation in svn_wc_add2(). However, this dirty trick will not
expose to the client.
> 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.
Yes, the helper may be potentially dangerous. I just come up with a better
one, see above. But just to have a discussion. Does this really harmful? I
admit that I'm not yet understand the detail in the update logic. But since
the depths are allowed to mix up in wc, I don't think it will be really
harmful. A dir with depth_empty can also have sub-tree and explicitly pulled
in files. On the other hand, a dir with depth_files will also receive new
added items ('new added in repository' vs. 'already in repository but not in
wc', difference?). The depth setting is currently just no more than a value in
this_dir entry, which is more important is the logic that handles it.
Well, this is just a discussion and does not mean that I would like to stick
to the helper solution since I find a solution that is more beautiful. :-)
>
> > 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.
Well, this is a hard decision too, just as the decision to patch 'svn add'
itself. :) I think it makes sense to add the parent as empty. If the user
cares about something directly under the parent directory, why not add the
entire parent tree in the first place?
>
> > 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.
As you have mentioned previously in this thread. Whatever we choose to do in
the code, the user may complain in a oppose direction. Why not give them a
notice if we choose to preclude the update? This may relieve their anger. :-)
This feature will need to tune the API. However, since the notification only
happens at the boundary that is defined by the ambient depth of wc (in other
words, we don't need to dig into excluded sub-trees), it's possible to make it
available with only implement level modification. The "tell the repository to
give me a very finely-tuned directory delta" semantics should still be
maintained.
Rui, Guo
---------------------------------------------------------------------
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 08:03:27 CEST