You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Rui, Guo" <ti...@mail.ustc.edu.cn> on 2008/04/25 16:19:15 UTC
[PATCH] Fix --depth behavior for svn add (Re: Semantics of --depth: should define WC-depth for omitted-items?)
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.
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. 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'.
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?
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" <ti...@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@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
Re: [PATCH] Fix --depth behavior for svn add (Re: Semantics of --depth: should define WC-depth for omitted-items?)
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Tue, Apr 29, 2008 at 10:41:54AM -0700, David Glasser wrote:
> On Tue, Apr 29, 2008 at 2:36 AM, Rui, Guo <ti...@mail.ustc.edu.cn> wrote:
> > Take the greek tree as an example. Consider a wc with directory A at
> > immediates and revision 1. And somebody introduced items A/D/new1 and
> > A/B/E/new_tree at revision 2. When we update our wc from revision 1 to 2, we
> > will notice the skipped new item A/D/new1 but not the A/B/E/new_tree. The idea
> > is that, if we include a directory in wc (a leaf directory in wc), we should
> > be able to see what happens (piratical) to it and probably would like to. If
> > we didn't bother to include the directory in wc, we are probably not
> > interested at all.
>
> It sounds like what you're describing is a variant of the wc crawler
> that goes one level deeper than the ambient depth.
It's possible to handle this in a customized wc crawler, but not in a way that
simply goes one level deeper, since only depth empty/files need special
attention. Moreover, handle this in client side only may result in unnecessary
data transfer, since we only need new items (after last update) in this
situation.
Rui, Guo
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix --depth behavior for svn add (Re: Semantics of --depth: should define WC-depth for omitted-items?)
Posted by David Glasser <gl...@davidglasser.net>.
On Tue, Apr 29, 2008 at 2:36 AM, Rui, Guo <ti...@mail.ustc.edu.cn> wrote:
> Take the greek tree as an example. Consider a wc with directory A at
> immediates and revision 1. And somebody introduced items A/D/new1 and
> A/B/E/new_tree at revision 2. When we update our wc from revision 1 to 2, we
> will notice the skipped new item A/D/new1 but not the A/B/E/new_tree. The idea
> is that, if we include a directory in wc (a leaf directory in wc), we should
> be able to see what happens (piratical) to it and probably would like to. If
> we didn't bother to include the directory in wc, we are probably not
> interested at all.
It sounds like what you're describing is a variant of the wc crawler
that goes one level deeper than the ambient depth.
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn_wc_add3(): handles depth on add (was Re: [PATCH] Fix --depth behavior for svn add)
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Mon, May 05, 2008 at 11:55:55AM +0200, Stefan Sperling wrote:
> >
> > + /* default to depth_infinity if the value is invalid */
> > + if (!(depth >= svn_depth_empty
> > + && depth <= svn_depth_infinity))
> > + depth = svn_depth_infinity;
> > +
> > /* Get the original entry for this path if one exists (perhaps
> > this is actually a replacement of a previously deleted thing).
>
> Shouldn't this throw an error instead of silently overriding whatever
> the caller passed in? Silently overriding the caller's value may produce
> subtle bugs in client software that are difficult to track down for our
> API consumers. I think it's better to let people know right away when they
> did something wrong, by throwing an error at them. You could create
> a new error code if no suitable one already eixsts, for example
> SVN_ERR_WC_INVALID_DEPTH, or something like that.
Well, I'll fix this. However, does this error fit in the wc category? After
all, it's not the wc itself but the parameter that has problem.
Rui, Guo
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] svn_wc_add3(): handles depth on add (was Re: [PATCH] Fix --depth behavior for svn add)
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Mon, Apr 28, 2008 at 05:57:52PM -0400, Karl Fogel wrote:
>
> Basically, I think just adding depth to the regular add API is okay.
>
Here comes the patch that introduce a new svn_wc_add3() to handle depth on add
correctly.
svn_add3.diff:
[[[
Introduced a new svn_wc_add3() to handle depth correctly. This makes the
--depth option in 'svn add' works in the same way as ci, up etc.
* subversion/include/svn_wc.h
(svn_wc_add3): New, accept depth parameters and ignore it for files.
(svn_wc_add2): Now deprecated.
* subversion/libsvn_wc/adm_ops.c
(svn_wc_add3): New, accept depth parameters and ignore it for files.
svn_depth_infinity will be used upon invalid depth value. Obsolete comment
deleted.
(svn_wc_add2): Deprecated. Now calls svn_wc_add3 with a default
svn_depth_infinity.
* subversion/libsvn_wc/copy.c
(copy_added_file_administratively,
copy_added_dir_administratively,
copy_dir_administratively): Switch to svn_wc_add3, with a default
svn_depth_infinity depth.
* subversion/libsvn_client/merge.c
(merge_dir_added): Switch to svn_wc_add3, with a default svn_depth_infinity
depth.
* subversion/libsvn_client/copy.c
(repos_to_wc_copy_single): Switch to svn_wc_add3, with a default
svn_depth_infinity depth.
* subversion/libsvn_client/add.c
(add_file, add_dir_recursive,
add, add_parent_dirs): Switch to svn_wc_add3, with a default
svn_depth_infinity depth.
(add): Pass all directory target to add_dir_recursive, otherwise adjustment
will also be needed here.
* subversion/tests/cmdline/depth_tests.py
(add_tree_with_depth_files): Renamed to add_tree_with_depth to reflect the
new content: depth verified & new tests added for the --force & --parents
situation
(test_list): update according to the rename.
]]]
svn_add3_cleanup.diff
[[[
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
]]]
Some questions about the svn_wc__do_update_cleanup() call in the original
svn_wc_add2(). This function is called only when add-with-history and will
recursively tweak the children entries. However, the svn_wc_add2() adds only
the single item specified in the parameter, rather than the whole tree. So, to
put a sub-tree under version control, we have to crawl the tree and call
svn_wc_add2() many times. If we call svn_wc_add2() in a root-first favor, the
svn_wc__do_update_cleanup() call does nothing more than tweak 'this_dir'; if
we call svn_wc_add2() in a root_last favor (I really doubt this), the
svn_wc__do_update_cleanup() call will tweak each children many times. Why do
we have to call this function?
Rui, Guo
Re: [PATCH] svn_wc_add3(): handles depth on add
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Tue, May 06, 2008 at 03:03:04PM -0400, Karl Fogel wrote:
> "Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
> > 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.
>
> 我知道,没问题 :-).
Wow~~ Incredible! You can speak Chinese! Where did you learn it from,huh? I'm
really curious about it. ^_^
>
> >> So I reverted the patch and tried with a clean trunk@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.
>
> Oh, actually, it's okay to post a patch for review without first running
> 'make check'. Just make sure you say that in your mail, so people know
> what to expect.
Okay, I'll mention it next time if I only run a partial check.
>
> > 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.
>
> Ah, okay.
And the new patch was also included in the last reply, did you neglect it?
>
> > 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?
>
> I didn't quite understand. Can you explain that in more words?
Well, I observed something that I can't tell whether it is a Okay. I'll
show the detail this time.
In log_tests.py, there is a function 'merge_history_repos' used to prepare a
repository with complicated merge history. And it was in the function that I
found where the source of misc test failure lies in.
Here is a modified code snatch in that function. The original code used 'svn
mkdir' instead and caused the test failure. I explicitly add these directories
in depth empty here.
# Create trunk/tags/branches - r1
os.mkdir('trunk')
os.mkdir('tags')
os.mkdir('branches')
svntest.main.run_svn(None, 'add', '--depth=empty', 'trunk')
svntest.main.run_svn(None, 'add', '--depth=empty', 'tags')
svntest.main.run_svn(None, 'add', '--depth=empty', 'branches')
svntest.main.run_svn(None, 'ci', '-m',
'Add trunk/tags/branches structure.')
There is a merge later, as quoted here:
# Do some mergeing - r6
#
# Mergeinfo changes on /trunk:
# Merged /trunk:r2
# Merged /branches/a:r3-5
os.chdir('trunk')
svntest.main.run_svn(None, 'merge', os.path.join('..', branch_a) + '@HEAD')
The command 'svn merge ../branches/a@HEAD' will derive its depth from the depth
of trunk -- svn_depth_empty. After this merge, I checked the state of wc:
$ svndev st
M trunk
M trunk/A
M trunk/iota
$ svndev diff
Property changes on: trunk
___________________________________________________________________
Added: svn:mergeinfo
Merged /trunk:r2*
Merged /branches/a:r3-5*
Property changes on: trunk/A
___________________________________________________________________
Added: svn:mergeinfo
Merged /branches/a/A:r3-5*
Property changes on: trunk/iota
___________________________________________________________________
Added: svn:mergeinfo
Merged /branches/a/iota:r3-5
It seems that the merge-info has recorded this merge. However, no merge on
file content actually happened.
The diff of branch a is as follows:
$ svndev diff -r3:5 branches/a
Index: branches/a/A/mu
===================================================================
--- branches/a/A/mu (revision 3)
+++ branches/a/A/mu (revision 5)
@@ -1 +1,2 @@
This is the file 'mu'.
+Don't forget to look at 'upsilon', too.
\ No newline at end of file
Index: branches/a/A/upsilon
===================================================================
--- branches/a/A/upsilon (revision 0)
+++ branches/a/A/upsilon (revision 5)
@@ -0,0 +1 @@
+This is the file 'upsilon'.
Index: branches/a/iota
===================================================================
--- branches/a/iota (revision 3)
+++ branches/a/iota (revision 5)
@@ -1 +1,2 @@
This is the file 'iota'.
+'A' has changed a bit.
That's all. It's up to you to judge whether it is okay.
Rui, Guo
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix --depth behavior for svn add (Re: Semantics of --depth: should define WC-depth for omitted-items?)
Posted by Karl Fogel <kf...@red-bean.com>.
"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
>> I'm not sure exactly what you meant. Could you give an example?
>
> Take the greek tree as an example. Consider a wc with directory A at
> immediates and revision 1. And somebody introduced items A/D/new1 and
> A/B/E/new_tree at revision 2. When we update our wc from revision 1 to 2, we
> will notice the skipped new item A/D/new1 but not the A/B/E/new_tree. The idea
> is that, if we include a directory in wc (a leaf directory in wc), we should
> be able to see what happens (piratical) to it and probably would like to. If
> we didn't bother to include the directory in wc, we are probably not
> interested at all.
>
> In such a way, the server do not need to crawl into unrelated path and thus
> maintains the current way it works. The only extension is that the server will
> have to feedback a little more when handling leaf directories in wc. However,
> this behavior should be explicitly requested by client and should be still
> considered 'finely-tuned directory delta'.
>
> Did I make it clear enough?
Hmmm. Almost -- I understand more now.
A good way to describe a change like this is to simply write the new
documentation string(s) for any functions that will be affected. That
is the final specification anyway, and if the reviewers can't understand
the new doc strings, then future readers won't either :-). So if you
write them before making the code change, then we can tell exactly what
will happen.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix --depth behavior for svn add (Re: Semantics of --depth: should define WC-depth for omitted-items?)
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Mon, Apr 28, 2008 at 05:57:52PM -0400, Karl Fogel wrote:
> "Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
> >
> > 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.
>
> I think I made my earlier decision about svn_wc_add2() without all the
> information we have now. If it just took a 'depth' argument, we could
> ignore it for files (or we could require that files be called with
> 'svn_depth_unknown' and check for it, to sanity-check the caller).
>
> Possibly other uses (outside of 'svn add') might also find the 'depth'
> argument useful. In any case, once we admit that addition of a
> directory is an operation involving depth questions, we might as well
> reflect that in the API. If many callers have to pass
> svn_depth_infinity, at least that's readable.
Ok, I'll introduce a new svn_wc_add2() and deprecate the current version. I
would just ignore the depth for files. (If I have to pay special attention to
files and prepare a 'svn_depth_unknown', I would like to suggest splitting the
API instead.) You are right in the sense that the API should be reflective and
may be useful in the future.
Personally, I do not like the idea of passing parameters which are just to be
ignored. However, this is an underlying design issue and does not concern me.
If you guys are really going to re-design the wc library, I think splitting
the API may be desirable, since the current one looks too heavy to me. Anyway,
that would be another story. :-)
>
> > 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.
>
> Well, we'd want the depth as we're crawling down and doing the add, so
> we can avoid doing more work than necessary.
The API does not do the crawling; it just add the specified target. The
crawling is handled by the caller instead.
>
> Basically, I think just adding depth to the regular add API is okay.
Yes, basically I'm agree with you at this point.
>
> >
> > 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?
>
> Either empty or infinity.
>
> Actually, I think infinity may be better. Imagine that you
>
> svn add --depth=files --parents NEW_DIR_1/NEW_DIR_2/FINAL_DIR
> svn commit -m "...blah blah blah..."
>
> There might be a lot of subdirectories under FINAL_DIR, but they won't
> be added, only FINAL_DIR itself and its file children. Great.
>
> But later, after you do that, someone else adds NEW_DIR_1/FISH/ to the
> repository. What should happen if you run 'svn up' now?
>
> I think you should receive NEW_DIR_1/FISH. The alternative is that you
> don't find out that someone has been adding things in the directory you
> created -- which would be fine if you'd *requested* that directory with
> a limiting depth, but you didn't. The '--depth=files' in the original
> 'svn add' really referred to the final target, not to the intermediate
> stuff -- the intermediate dirs are just things you had to add in order
> to position FINAL_DIR correctly. If stuff appears in those intermediate
> directories later, you'd probably like to know. Not definitely, but
> probably.
I'm agree with you again, Karl. It's better to leave them infinity at current
time, even though the directories are logically added at empty.
This is just once again the same argue that whether we should apply the depth
to the new added directory. That's why I think it's handy to have a
notification mechanism. The difference this time is that the user does not
explicitly specify the depth for parent directories. (In fact, they do not
have a way to specify this, expect to use the traditional step by step way)
Since the user does not explicitly request it, and we do not have a mechanism
to notify them yet, it's better to leave it untouched.
>
> >
> > 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. :-)
>
> I just think we have more important things to do right now. This might
> be a nice feature, but it is unimportant compared to, say, a depth
> deselection interface :-).
Well, this idea just come to my mind when I was thinking about the semantics
of the --depth option. And I think it's valuable to share it with you. I'm not
saying that I would like to do it right now, even though I'm indeed
interested. The de-selection interface will always be my first priority until
it's settled.
>
> > 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.
>
> I'm not sure exactly what you meant. Could you give an example?
Take the greek tree as an example. Consider a wc with directory A at
immediates and revision 1. And somebody introduced items A/D/new1 and
A/B/E/new_tree at revision 2. When we update our wc from revision 1 to 2, we
will notice the skipped new item A/D/new1 but not the A/B/E/new_tree. The idea
is that, if we include a directory in wc (a leaf directory in wc), we should
be able to see what happens (piratical) to it and probably would like to. If
we didn't bother to include the directory in wc, we are probably not
interested at all.
In such a way, the server do not need to crawl into unrelated path and thus
maintains the current way it works. The only extension is that the server will
have to feedback a little more when handling leaf directories in wc. However,
this behavior should be explicitly requested by client and should be still
considered 'finely-tuned directory delta'.
Did I make it clear enough?
Thanks,
Rui, Guo
PS: The new patch to 'svn add' will be delayed to this weekend. Because I have
a three-day vacation, the May Day in China, and would like to have a short trip.
Hope that this delay is not too much.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn_wc_add3(): handles depth on add
Posted by Karl Fogel <kf...@red-bean.com>.
Karl Fogel <kf...@red-bean.com> writes:
> For API compatibility reasons, we have to be careful not to change the
> meaning of svn_client_add3(). The question is, did r26607 already
> change its meaning, and now you're just correcting it back, or was
> r26607 already as correct as we could be?
>
> Consistency with svn/main.c isn't the issue, I think. Consistency with
> the past meaning of the API is what's important.
By the way, I traced svn_client_add3() in 1.4.x and confirmed that it
did treat recursive=false as svn_depth_empty. So I applied your patch
in r31131 and nominated it for backport (with a note that it's only a
nice-to-have, not a must-have, for release -- if this fix came out in
1.5.1 that would be okay too).
Thanks!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn_wc_add3(): handles depth on add
Posted by Karl Fogel <kf...@red-bean.com>.
"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
> 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
> ]]]
As you can see, I applied the svn_client_add4() part of this in r31102,
but not the svn_client_add3() part.
For API compatibility reasons, we have to be careful not to change the
meaning of svn_client_add3(). The question is, did r26607 already
change its meaning, and now you're just correcting it back, or was
r26607 already as correct as we could be?
Consistency with svn/main.c isn't the issue, I think. Consistency with
the past meaning of the API is what's important.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn_wc_add3(): handles depth on add
Posted by Karl Fogel <kf...@red-bean.com>.
"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
> 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.
我知道,没问题 :-).
>> So I reverted the patch and tried with a clean trunk@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.
Oh, actually, it's okay to post a patch for review without first running
'make check'. Just make sure you say that in your mail, so people know
what to expect.
> 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.
Ah, okay.
> 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?
I didn't quite understand. Can you explain that in more words?
Thanks,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn_wc_add3(): handles depth on add
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
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@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@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
]]]
Re: [PATCH] svn_wc_add3(): handles depth on add
Posted by Karl Fogel <kf...@red-bean.com>.
"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
> I changed my mind. It should be OK to not check the depth value as other lib
> functions do. Here is the updated svn_wc_add3 patch, the cleanup patch just
> state the same.
>
> [[[
> Introduced a new svn_wc_add3() to handle depth correctly. This makes the
> --depth option in 'svn add' works in the same way as ci, up etc.
"work" :-)
(Your English is very good, by the way. I'm correcting these little
problems just because I'm reviewing anyway. Minor grammar problems
don't interfere with comprehension.)
> * 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"
Again, no need to document this here (and also there would be no need to
repeat what you said earlier in the same log message).
It is impossible to delete an obsolete comment from a function that's
new in this commit :-). Seriously: you don't need to mention that in
the log message. It's just a natural part of the change -- anyone
reading the diff would easily understand why that comment went away,
therefore there is no need to make special mention of it in the log
message.
> (svn_wc_add2): Deprecated. Now calls svn_wc_add3 with a default
> svn_depth_infinity.
Again, don't describe the details, just give an overview:
(svn_wc_add2): Just wrap svn_wc_add3.
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.
Therefore, we can combine the two files' log entries like this:
* subversion/include/svn_wc.h, subversion/libsvn_wc/adm_ops.c
(svn_wc_add3): New function.
(svn_wc_add2): Deprecate.
That's all you need to say. The reader will know what it means, from
the introductory part of the log message and from the diff.
> * subversion/libsvn_wc/copy.c
> (copy_added_file_administratively,
> copy_added_dir_administratively,
> copy_dir_administratively): Switch to svn_wc_add3, with a default
> svn_depth_infinity depth.
Try:
* 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): Switch to svn_wc_add3, with a default svn_depth_infinity
> depth.
Try:
* subversion/libsvn_client/merge.c
(merge_dir_added): Call svn_wc_add3 now.
> * subversion/libsvn_client/copy.c
> (repos_to_wc_copy_single): Switch to svn_wc_add3, with a default
> svn_depth_infinity depth.
...you know what I'm going to say here...
> * subversion/libsvn_client/add.c
> (add_file, add_dir_recursive,
> add, add_parent_dirs): Switch to svn_wc_add3, with a default
> svn_depth_infinity depth.
...and here :-).
> (add): Pass all directory target to add_dir_recursive, otherwise
> adjustment will also be needed here.
"targets", but see below.
Okay, there are several independent problems here. Let's sort them out:
First, the 'add' function is listed twice, once in the "Switch to
svn_wc_add3..." part, and then again right below it. nGenerally,
affected symbol should be listed exactly once in the log message.
Second, I had trouble understanding the comment. I think you meant
something like this:
Unconditionally call add_dir_recursive on directory targets,
regardless of depth parameter, so the target's depth will be set
correctly.
...right? So, here's a new log message for that file, addressing both
of the above problems:
* subversion/libsvn_client/add.c
(add_file, add_dir_recursive, add_parent_dirs): Call svn_wc_add3 now.
(add): Same. Also, call add_dir_recursive on all directory targets,
regardless of depth parameter, so the target's depth will be set
correctly.
But there's still one more problem:
The note about why we invoke add_dir_recursive should really be a
comment in the code, not in the log message! It would be important for
a reader of the code to understand it. The log message can just say:
* 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.
...then in the code, put a comment explaining *why* it's being called.
> * subversion/tests/cmdline/depth_tests.py
> (add_tree_with_depth_files): Renamed to add_tree_with_depth to reflect the
> new content: depth verified & new tests added for the --force & --parents
> situation
> (test_list): update according to the rename.
In a rename, the new symbol should always appear in the symbol position
in the log message. So:
* 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.
Okay, on to the code...
> --- 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:
+ * If @a path is a directory, add it at @a depth; otherwise,
+ * ignore @a depth.
> @@ -2794,9 +2797,26 @@
> * ### Update: see "###" comment in svn_wc_add_repos_file()'s doc
> * string about this.
> *
> - * @since New in 1.2.
> + * @since New in 1.5.
> */
> svn_error_t *
> +svn_wc_add3(const char *path,
> + svn_wc_adm_access_t *parent_access,
> + svn_depth_t depth,
> + const char *copyfrom_url,
> + svn_revnum_t copyfrom_rev,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + svn_wc_notify_func2_t notify_func,
> + void *notify_baton,
> + apr_pool_t *pool);
> +
> +/**
> + * Similar to svn_wc_add3(), but takes an @c svn_depth_infinity instead.
> + *
> + * @deprecated Provided for backward compatibility with the 1.2 API.
> + */
> +svn_error_t *
> svn_wc_add2(const char *path,
> svn_wc_adm_access_t *parent_access,
> const char *copyfrom_url,
Good, but it doesn't "take" a depth parameter. (I know what you were
trying to say, of course.) Try this:
+/**
+ * Similar to svn_wc_add3(), but with the @a depth parameter always
+ * @c svn_depth_infinity.
+ *
+ * @deprecated Provided for backward compatibility with the 1.2 API.
+ */
That's how we usually express this situation (see other deprecations in
the .h files for examples).
> --- subversion/libsvn_client/add.c (revision 31014)
> +++ subversion/libsvn_client/add.c (working copy)
> @@ -430,13 +429,13 @@
> svn_error_t *err;
>
> SVN_ERR(svn_io_check_path(path, &kind, pool));
> - if (kind == svn_node_dir && depth >= svn_depth_files)
> + if (kind == svn_node_dir && depth >= svn_depth_empty)
> err = add_dir_recursive(path, adm_access, depth,
> force, no_ignore, ctx, pool);
Yup, this is where the comment would help.
But why are we checking depth at all, if we accept anything from
empty<->infinity? Just check "(kind == svn_node_dir)", in that case. I
mean, if depth is 'svn_depth_unknown' or some other strange value, your
code would just skip the call to add_dir_recursive() entirely. Surely
that's no better (nor worse) than calling add_dir_recursive() with a bad
depth :-) ?
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@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@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.
[[[
Introduce svn_wc_add3(), which takes a depth parameter. This makes
'svn add --depth' work the same way as commit, update, etc.
Patch by: Guo Rui <ti...@mail.ustc.edu.cn>
(Minor tweaks by me.)
* 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.
* 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.
]]]
Index: subversion/include/svn_wc.h
===================================================================
--- subversion/include/svn_wc.h (revision 31039)
+++ subversion/include/svn_wc.h (working copy)
@@ -2756,6 +2756,9 @@
*
* If @a path does not exist, return @c SVN_ERR_WC_PATH_NOT_FOUND.
*
+ * If @a path is a directory, add it at @a depth; otherwise, ignore
+ * @a depth.
+ *
* If @a copyfrom_url is non-NULL, it and @a copyfrom_rev are used as
* `copyfrom' args. This is for copy operations, where one wants
* to schedule @a path for addition with a particular history.
@@ -2799,9 +2802,27 @@
* ### Update: see "###" comment in svn_wc_add_repos_file()'s doc
* string about this.
*
- * @since New in 1.2.
+ * @since New in 1.5.
*/
svn_error_t *
+svn_wc_add3(const char *path,
+ svn_wc_adm_access_t *parent_access,
+ svn_depth_t depth,
+ const char *copyfrom_url,
+ svn_revnum_t copyfrom_rev,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ svn_wc_notify_func2_t notify_func,
+ void *notify_baton,
+ apr_pool_t *pool);
+
+/**
+ * Similar to svn_wc_add3(), but with the @a depth parameter always
+ * @c svn_depth_infinity.
+ *
+ * @deprecated Provided for backward compatibility with the 1.2 API.
+ */
+svn_error_t *
svn_wc_add2(const char *path,
svn_wc_adm_access_t *parent_access,
const char *copyfrom_url,
Index: subversion/libsvn_wc/copy.c
===================================================================
--- subversion/libsvn_wc/copy.c (revision 31039)
+++ subversion/libsvn_wc/copy.c (working copy)
@@ -75,8 +75,8 @@
if (src_is_added)
{
- SVN_ERR(svn_wc_add2(dst_path, dst_parent_access, NULL,
- SVN_INVALID_REVNUM, cancel_func,
+ SVN_ERR(svn_wc_add3(dst_path, dst_parent_access, svn_depth_infinity,
+ NULL, SVN_INVALID_REVNUM, cancel_func,
cancel_baton, notify_func,
notify_baton, pool));
}
@@ -150,7 +150,7 @@
/* Add the directory, adding locking access for dst_path
to dst_parent_access at the same time. */
- SVN_ERR(svn_wc_add2(dst_path, dst_parent_access, NULL,
+ SVN_ERR(svn_wc_add3(dst_path, dst_parent_access, svn_depth_infinity, NULL,
SVN_INVALID_REVNUM, cancel_func, cancel_baton,
notify_func, notify_baton, pool));
@@ -755,7 +755,7 @@
SVN_ERR(svn_wc_adm_close(adm_access));
- SVN_ERR(svn_wc_add2(dst_path, dst_parent,
+ SVN_ERR(svn_wc_add3(dst_path, dst_parent, svn_depth_infinity,
copyfrom_url, copyfrom_rev,
cancel_func, cancel_baton,
notify_copied, notify_baton, pool));
Index: subversion/libsvn_wc/adm_ops.c
===================================================================
--- subversion/libsvn_wc/adm_ops.c (revision 31039)
+++ subversion/libsvn_wc/adm_ops.c (working copy)
@@ -1377,8 +1377,9 @@
svn_error_t *
-svn_wc_add2(const char *path,
+svn_wc_add3(const char *path,
svn_wc_adm_access_t *parent_access,
+ svn_depth_t depth,
const char *copyfrom_url,
svn_revnum_t copyfrom_rev,
svn_cancel_func_t cancel_func,
@@ -1526,13 +1527,6 @@
if (kind == svn_node_dir) /* scheduling a directory for addition */
{
- /* Note that both calls to svn_wc_ensure_adm3() below pass
- svn_depth_infinity. Even if 'svn add' were invoked with some
- other depth, we'd want to create the adm area with
- svn_depth_infinity, because when the user passes add a depth,
- that's just a way of telling Subversion what items to add,
- not a way of telling Subversion what depth the resultant
- newly-versioned directory should have. */
if (! copyfrom_url)
{
@@ -1550,7 +1544,7 @@
/* Make sure this new directory has an admistrative subdirectory
created inside of it */
SVN_ERR(svn_wc_ensure_adm3(path, NULL, new_url, p_entry->repos,
- 0, svn_depth_infinity, pool));
+ 0, depth, pool));
}
else
{
@@ -1560,7 +1554,7 @@
copyfrom arguments to the ensure call. */
SVN_ERR(svn_wc_ensure_adm3(path, NULL, copyfrom_url,
parent_entry->repos, copyfrom_rev,
- svn_depth_infinity, pool));
+ depth, pool));
}
/* We want the locks to persist, so use the access baton's pool */
@@ -1604,7 +1598,7 @@
/* Change the entry urls recursively (but not the working rev). */
SVN_ERR(svn_wc__do_update_cleanup(path, adm_access,
- svn_depth_infinity, new_url,
+ depth, new_url,
parent_entry->repos,
SVN_INVALID_REVNUM, NULL,
NULL, FALSE, apr_hash_make(pool),
@@ -1636,6 +1630,22 @@
return SVN_NO_ERROR;
}
+svn_error_t *
+svn_wc_add2(const char *path,
+ svn_wc_adm_access_t *parent_access,
+ const char *copyfrom_url,
+ svn_revnum_t copyfrom_rev,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ svn_wc_notify_func2_t notify_func,
+ void *notify_baton,
+ apr_pool_t *pool)
+{
+ return svn_wc_add3(path, parent_access, svn_depth_infinity,
+ copyfrom_url, copyfrom_rev,
+ cancel_func, cancel_baton,
+ notify_func, notify_baton, pool);
+}
svn_error_t *
svn_wc_add(const char *path,
@@ -1658,7 +1668,6 @@
svn_wc__compat_call_notify_func, &nb, pool);
}
-
/* Thoughts on Reversion.
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 31039)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -1100,7 +1100,7 @@
else
{
SVN_ERR(svn_io_make_dir_recursively(path, subpool));
- SVN_ERR(svn_wc_add2(path, adm_access,
+ SVN_ERR(svn_wc_add3(path, adm_access, svn_depth_infinity,
copyfrom_url, copyfrom_rev,
merge_b->ctx->cancel_func,
merge_b->ctx->cancel_baton,
@@ -1117,7 +1117,7 @@
if (! entry || entry->schedule == svn_wc_schedule_delete)
{
if (!merge_b->dry_run)
- SVN_ERR(svn_wc_add2(path, adm_access,
+ SVN_ERR(svn_wc_add3(path, adm_access, svn_depth_infinity,
copyfrom_url, copyfrom_rev,
merge_b->ctx->cancel_func,
merge_b->ctx->cancel_baton,
Index: subversion/libsvn_client/copy.c
===================================================================
--- subversion/libsvn_client/copy.c (revision 31039)
+++ subversion/libsvn_client/copy.c (working copy)
@@ -1389,8 +1389,8 @@
/* Schedule dst_path for addition in parent, with copy history.
(This function also recursively puts a 'copied' flag on every
entry). */
- SVN_ERR(svn_wc_add2(pair->dst, adm_access, pair->src,
- src_revnum,
+ SVN_ERR(svn_wc_add3(pair->dst, adm_access, svn_depth_infinity,
+ pair->src, src_revnum,
ctx->cancel_func, ctx->cancel_baton,
ctx->notify_func2, ctx->notify_baton2, pool));
Index: subversion/libsvn_client/add.c
===================================================================
--- subversion/libsvn_client/add.c (revision 31039)
+++ subversion/libsvn_client/add.c (working copy)
@@ -232,8 +232,8 @@
pool));
/* Add the file */
- SVN_ERR(svn_wc_add2(path, adm_access, NULL, SVN_INVALID_REVNUM,
- ctx->cancel_func, ctx->cancel_baton,
+ SVN_ERR(svn_wc_add3(path, adm_access, svn_depth_infinity, NULL,
+ SVN_INVALID_REVNUM, ctx->cancel_func, ctx->cancel_baton,
NULL, NULL, pool));
if (is_special)
@@ -308,9 +308,8 @@
SVN_ERR(ctx->cancel_func(ctx->cancel_baton));
/* Add this directory to revision control. */
- err = svn_wc_add2(dirname, adm_access,
- NULL, SVN_INVALID_REVNUM,
- ctx->cancel_func, ctx->cancel_baton,
+ err = svn_wc_add3(dirname, adm_access, depth, NULL, SVN_INVALID_REVNUM,
+ ctx->cancel_func, ctx->cancel_baton,
ctx->notify_func2, ctx->notify_baton2, pool);
if (err && err->apr_err == SVN_ERR_ENTRY_EXISTS && force)
svn_error_clear(err);
@@ -430,15 +429,25 @@
svn_error_t *err;
SVN_ERR(svn_io_check_path(path, &kind, pool));
- if (kind == svn_node_dir && depth >= svn_depth_files)
- err = add_dir_recursive(path, adm_access, depth,
- force, no_ignore, ctx, pool);
+ if (kind == svn_node_dir)
+ {
+ /* We use add_dir_recursive for all directory targets,
+ and pass depth along no matter what it is, so that the
+ target's depth will be set correctly. */
+ err = add_dir_recursive(path, adm_access, depth,
+ force, no_ignore, ctx, pool);
+
+ }
else if (kind == svn_node_file)
- err = add_file(path, ctx, adm_access, pool);
+ {
+ err = add_file(path, ctx, adm_access, pool);
+ }
else
- err = svn_wc_add2(path, adm_access, NULL, SVN_INVALID_REVNUM,
- ctx->cancel_func, ctx->cancel_baton,
- ctx->notify_func2, ctx->notify_baton2, pool);
+ {
+ err = svn_wc_add3(path, adm_access, depth, NULL, SVN_INVALID_REVNUM,
+ ctx->cancel_func, ctx->cancel_baton,
+ ctx->notify_func2, ctx->notify_baton2, pool);
+ }
/* Ignore SVN_ERR_ENTRY_EXISTS when FORCE is set. */
if (err && err->apr_err == SVN_ERR_ENTRY_EXISTS && force)
@@ -482,7 +491,8 @@
SVN_ERR(add_parent_dirs(parent_path, &adm_access, ctx, pool));
SVN_ERR(svn_wc_adm_retrieve(&adm_access, adm_access, parent_path,
pool));
- SVN_ERR(svn_wc_add2(path, adm_access, NULL, SVN_INVALID_REVNUM,
+ SVN_ERR(svn_wc_add3(path, adm_access, svn_depth_infinity,
+ NULL, SVN_INVALID_REVNUM,
ctx->cancel_func, ctx->cancel_baton,
ctx->notify_func2, ctx->notify_baton2, pool));
}
Index: subversion/tests/cmdline/depth_tests.py
===================================================================
--- subversion/tests/cmdline/depth_tests.py (revision 31039)
+++ subversion/tests/cmdline/depth_tests.py (working copy)
@@ -1210,17 +1210,38 @@
# Check that the new directory was added at depth=empty.
verify_depth(None, "empty", other_I_path)
-def add_tree_with_depth_files(sbox):
- "add multi-subdir tree with --depth=files" # For issue #2931
+def add_tree_with_depth(sbox):
+ "add multi-subdir tree with --depth options" # For issue #2931
sbox.build()
wc_dir = sbox.wc_dir
new1_path = os.path.join(wc_dir, 'new1')
new2_path = os.path.join(new1_path, 'new2')
+ new3_path = os.path.join(new2_path, 'new3')
+ new4_path = os.path.join(new3_path, 'new4')
os.mkdir(new1_path)
os.mkdir(new2_path)
+ os.mkdir(new3_path)
+ os.mkdir(new4_path)
+ # Simple case, add new1 only, set depth to files
svntest.actions.run_and_verify_svn(None, None, [],
"add", "--depth", "files", new1_path)
+ verify_depth(None, "files", new1_path)
+ # Force add new1 at new1 again, should include new2 at empty, the depth of
+ # new1 should not change
+ svntest.actions.run_and_verify_svn(None, None, [],
+ "add", "--depth", "immediates",
+ "--force", new1_path)
+ verify_depth(None, "files", new1_path)
+ verify_depth(None, "empty", new2_path)
+
+ # add new4 with intermediate path, the intermediate path is added at empty
+ svntest.actions.run_and_verify_svn(None, None, [],
+ "add", "--depth", "immediates",
+ "--parents", new4_path)
+ verify_depth(None, "infinity", new3_path)
+ verify_depth(None, "immediates", new4_path)
+
def upgrade_from_above(sbox):
"upgrade a depth=empty wc from above"
@@ -2016,7 +2037,7 @@
diff_in_depthy_wc,
commit_depth_immediates,
depth_immediates_receive_new_dir,
- add_tree_with_depth_files,
+ add_tree_with_depth,
upgrade_from_above,
status_in_depthy_wc,
depthy_update_above_dir_to_be_deleted,
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn_wc_add3(): handles depth on add (was Re: [PATCH] Fix --depth behavior for svn add)
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Mon, May 05, 2008 at 08:48:43PM +0800, Rui, Guo wrote:
> On Mon, May 05, 2008 at 11:55:55AM +0200, Stefan Sperling wrote:
> > >
> > > + /* default to depth_infinity if the value is invalid */ + if
> > > (!(depth >= svn_depth_empty + && depth <= svn_depth_infinity)) +
> > > depth = svn_depth_infinity; + /* Get the original entry for this path if
> > > one exists (perhaps this is actually a replacement of a previously
> > > deleted thing).
> >
> > Shouldn't this throw an error instead of silently overriding whatever the
> > caller passed in? Silently overriding the caller's value may produce
> > subtle bugs in client software that are difficult to track down for our
> > API consumers. I think it's better to let people know right away when they
> > did something wrong, by throwing an error at them. You could create a new
> > error code if no suitable one already eixsts, for example
> > SVN_ERR_WC_INVALID_DEPTH, or something like that.
>
> Well, I'll fix this. However, does this error fit in the wc category? After
> all, it's not the wc itself but the parameter that has problem.
>
I changed my mind. It should be OK to not check the depth value as other lib
functions do. Here is the updated svn_wc_add3 patch, the cleanup patch just
state the same.
Rui, Guo
[[[
Introduced a new svn_wc_add3() to handle depth correctly. This makes the
--depth option in 'svn add' works in the same way as ci, up etc.
* subversion/include/svn_wc.h
(svn_wc_add3): New, accept depth parameters and ignore it for files.
(svn_wc_add2): Now deprecated.
* subversion/libsvn_wc/adm_ops.c
(svn_wc_add3): New, accept depth parameters and ignore it for files.
Obsolete comment deleted.
(svn_wc_add2): Deprecated. Now calls svn_wc_add3 with a default
svn_depth_infinity.
* subversion/libsvn_wc/copy.c
(copy_added_file_administratively,
copy_added_dir_administratively,
copy_dir_administratively): Switch to svn_wc_add3, with a default
svn_depth_infinity depth.
* subversion/libsvn_client/merge.c
(merge_dir_added): Switch to svn_wc_add3, with a default svn_depth_infinity
depth.
* subversion/libsvn_client/copy.c
(repos_to_wc_copy_single): Switch to svn_wc_add3, with a default
svn_depth_infinity depth.
* subversion/libsvn_client/add.c
(add_file, add_dir_recursive,
add, add_parent_dirs): Switch to svn_wc_add3, with a default
svn_depth_infinity depth.
(add): Pass all directory target to add_dir_recursive, otherwise adjustment
will also be needed here.
* subversion/tests/cmdline/depth_tests.py
(add_tree_with_depth_files): Renamed to add_tree_with_depth to reflect the
new content: depth verified & new tests added for the --force & --parents
situation
(test_list): update according to the rename.
]]]
Re: [PATCH] svn_wc_add3(): handles depth on add (was Re: [PATCH]
Fix --depth behavior for svn add)
Posted by Stefan Sperling <st...@elego.de>.
On Sun, May 04, 2008 at 09:26:33PM +0800, Rui, Guo wrote:
> On Mon, Apr 28, 2008 at 05:57:52PM -0400, Karl Fogel wrote:
> >
> > Basically, I think just adding depth to the regular add API is okay.
> >
> Here comes the patch that introduce a new svn_wc_add3() to handle depth on add
> correctly.
Hey Rui,
I spotted on thing:
> Index: subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- subversion/libsvn_wc/adm_ops.c (revision 31014)
> +++ subversion/libsvn_wc/adm_ops.c (working copy)
> @@ -1377,8 +1377,9 @@
>
>
> svn_error_t *
> -svn_wc_add2(const char *path,
> +svn_wc_add3(const char *path,
> svn_wc_adm_access_t *parent_access,
> + svn_depth_t depth,
> const char *copyfrom_url,
> svn_revnum_t copyfrom_rev,
> svn_cancel_func_t cancel_func,
> @@ -1408,6 +1409,11 @@
> _("Unsupported node kind for path '%s'"),
> svn_path_local_style(path, pool));
>
> + /* default to depth_infinity if the value is invalid */
> + if (!(depth >= svn_depth_empty
> + && depth <= svn_depth_infinity))
> + depth = svn_depth_infinity;
> +
> /* Get the original entry for this path if one exists (perhaps
> this is actually a replacement of a previously deleted thing).
Shouldn't this throw an error instead of silently overriding whatever
the caller passed in? Silently overriding the caller's value may produce
subtle bugs in client software that are difficult to track down for our
API consumers. I think it's better to let people know right away when they
did something wrong, by throwing an error at them. You could create
a new error code if no suitable one already eixsts, for example
SVN_ERR_WC_INVALID_DEPTH, or something like that.
--
Stefan Sperling <st...@elego.de> Software Monkey
German law requires the following banner :(
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de CEO: Olaf Wagner
Store password unencrypted (yes/no)? No
Re: [PATCH] Fix --depth behavior for svn add (Re: Semantics of --depth: should define WC-depth for omitted-items?)
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Tue, Apr 29, 2008 at 05:36:52PM +0800, Rui, Guo wrote:
> Ok, I'll introduce a new svn_wc_add2() and deprecate the current version. I
A silly typo mistake, this should be svn_wc_add3() of couse.
Rui, Guo
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix --depth behavior for svn add (Re: Semantics of --depth: should define WC-depth for omitted-items?)
Posted by Karl Fogel <kf...@red-bean.com>.
"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
> On Fri, Apr 25, 2008 at 03:46:31PM -0700, David Glasser wrote:
>> 2008/4/25 Rui, Guo <ti...@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.
I think I made my earlier decision about svn_wc_add2() without all the
information we have now. If it just took a 'depth' argument, we could
ignore it for files (or we could require that files be called with
'svn_depth_unknown' and check for it, to sanity-check the caller).
Possibly other uses (outside of 'svn add') might also find the 'depth'
argument useful. In any case, once we admit that addition of a
directory is an operation involving depth questions, we might as well
reflect that in the API. If many callers have to pass
svn_depth_infinity, at least that's readable.
> 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.
Well, we'd want the depth as we're crawling down and doing the add, so
we can avoid doing more work than necessary.
Basically, I think just adding depth to the regular add API is okay.
>> > 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?
Either empty or infinity.
Actually, I think infinity may be better. Imagine that you
svn add --depth=files --parents NEW_DIR_1/NEW_DIR_2/FINAL_DIR
svn commit -m "...blah blah blah..."
There might be a lot of subdirectories under FINAL_DIR, but they won't
be added, only FINAL_DIR itself and its file children. Great.
But later, after you do that, someone else adds NEW_DIR_1/FISH/ to the
repository. What should happen if you run 'svn up' now?
I think you should receive NEW_DIR_1/FISH. The alternative is that you
don't find out that someone has been adding things in the directory you
created -- which would be fine if you'd *requested* that directory with
a limiting depth, but you didn't. The '--depth=files' in the original
'svn add' really referred to the final target, not to the intermediate
stuff -- the intermediate dirs are just things you had to add in order
to position FINAL_DIR correctly. If stuff appears in those intermediate
directories later, you'd probably like to know. Not definitely, but
probably.
>> > 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. :-)
I just think we have more important things to do right now. This might
be a nice feature, but it is unimportant compared to, say, a depth
deselection interface :-).
> 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.
I'm not sure exactly what you meant. Could you give an example?
Thanks,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix --depth behavior for svn add (Re: Semantics of --depth: should define WC-depth for omitted-items?)
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Fri, Apr 25, 2008 at 03:46:31PM -0700, David Glasser wrote:
> 2008/4/25 Rui, Guo <ti...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] updated version (Re: [PATCH] Fix --depth behavior for svn add)
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Sat, Apr 26, 2008 at 02:02:46PM +0800, Rui, Guo wrote:
> > 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.
Does it looks better now? See these:
svn_add2.diff:
[[[
Make the --depth option in svn add works in the same way as svn ci, up etc.
* subversion/include/svn_wc.h
(svn_wc_add_depth_dir): Declare the new function that specialized for adding
directory with depth.
* 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_depth_dir): New specialized function that add directory with
correct depth.
* subversion/libsvn_client/add.c
(add_dir_recursive, add_parent_dirs): Call svn_wc_add_depth_dir() instead
for directory.
(add): Pass all directory target to add_dir_recursive, otherwise adjustment
will also be needed here.
* subversion/tests/cmdline/depth_tests.py
(add_tree_with_depth_files): verify depth & added new tests for the --force
& --parents situation
]]]
svn_add2_cleanup.diff: (not modified, except for line numbers)
[[[
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
]]]
Rui, Guo
Re: [PATCH] Fix --depth behavior for svn add (Re: Semantics of --depth: should define WC-depth for omitted-items?)
Posted by David Glasser <gl...@davidglasser.net>.
2008/4/25 Rui, Guo <ti...@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" <ti...@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@subversion.tigris.org
> > For additional commands, e-mail: dev-help@subversion.tigris.org
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix --depth behavior for svn add (Re: Semantics of
--depth: should define WC-depth for omitted-items?)
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Rui, Guo wrote on Sat, 26 Apr 2008 at 01:35 +0800:
> On Fri, Apr 25, 2008 at 07:50:09PM +0300, Daniel Shahaf wrote:
> > Rui, Guo wrote on Sat, 26 Apr 2008 at 00:19 +0800:
> > > 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.
> > >
> >
> > You could use the (new in 1.5) changelists feature, and do 'svn diff
> > --cl CLNAME' to get the diff for changelist CLNAME.
> >
> > Is there an easy way, however, to get 'svn diff' to output files' diffs
> > a particular order?
> What if a file falls into both of the logical independent change sets? Just as
> the situation here. I think this is where the headache comes from.
I didn't notice that. There are tools that can handle overlapping or
interdependent patches; unfortunately I don't have much experience with
any of them, so I cannot make a specific recommendation.
> >
> > > Here are change logs for the patch:
> > > [[[
> > > Make the --depth option in svn add works in the same way with svn ci, up etc.
> > > ]]]
> > >
> > > And I also made some cleanups for the code:
> > > [[[
> > > Some cleanups:
> > > ]]]
> > >
> >
> > (patch manager hat on)
> >
> > Rui, it would be easier to review your patches if you said which log
> > message goes with which patch (although, this time, I could guess it from
> > the filenames). You might also include the log message at the top of the
> > patch file (patch(1) would ignore it).
> Shy, my fault. I should be more considerate. Thanks for reminding.
>
No problem. It's not a big issue.
> PS: I have to admit that I never manipulated patches directly,
This is another prescription for headaches. :)
> thanks to svn. :-)
>
> Rui, Guo
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix --depth behavior for svn add (Re: Semantics of --depth: should define WC-depth for omitted-items?)
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Fri, Apr 25, 2008 at 07:50:09PM +0300, Daniel Shahaf wrote:
> Rui, Guo wrote on Sat, 26 Apr 2008 at 00:19 +0800:
> > 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.
> >
>
> You could use the (new in 1.5) changelists feature, and do 'svn diff
> --cl CLNAME' to get the diff for changelist CLNAME.
>
> Is there an easy way, however, to get 'svn diff' to output files' diffs
> a particular order?
What if a file falls into both of the logical independent change sets? Just as
the situation here. I think this is where the headache comes from.
>
> > Here are change logs for the patch:
> > [[[
> > Make the --depth option in svn add works in the same way with svn ci, up etc.
> > ]]]
> >
> > And I also made some cleanups for the code:
> > [[[
> > Some cleanups:
> > ]]]
> >
>
> (patch manager hat on)
>
> Rui, it would be easier to review your patches if you said which log
> message goes with which patch (although, this time, I could guess it from
> the filenames). You might also include the log message at the top of the
> patch file (patch(1) would ignore it).
Shy, my fault. I should be more considerate. Thanks for reminding.
PS: I have to admit that I never manipulated patches directly,
thanks to svn. :-)
Rui, Guo
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix --depth behavior for svn add (Re: Semantics of
--depth: should define WC-depth for omitted-items?)
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Rui, Guo wrote on Sat, 26 Apr 2008 at 00:19 +0800:
> 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.
>
You could use the (new in 1.5) changelists feature, and do 'svn diff
--cl CLNAME' to get the diff for changelist CLNAME.
Is there an easy way, however, to get 'svn diff' to output files' diffs
a particular order?
> Here are change logs for the patch:
> [[[
> Make the --depth option in svn add works in the same way with svn ci, up etc.
> ]]]
>
> And I also made some cleanups for the code:
> [[[
> Some cleanups:
> ]]]
>
(patch manager hat on)
Rui, it would be easier to review your patches if you said which log
message goes with which patch (although, this time, I could guess it from
the filenames). You might also include the log message at the top of the
patch file (patch(1) would ignore it).
Daniel
> Rui, Guo
> On Tue, Apr 22, 2008 at 11:10:47AM -0400, Karl Fogel wrote:
> > "Rui, Guo" <ti...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org