You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@red-bean.com> on 2008/06/15 00:57:47 UTC
Re: svn commit: r31704 - in branches/issue-2843-dev/subversion: libsvn_client libsvn_wc
firemeteor@tigris.org writes:
> Log:
> On issue-2843-dev branch.
>
> Handle svn_depth_exclude in the cropping logic. However, other part of the
> libsvn_wc code still don't honor the svn_depth_exclude flag.
>
> [in subversion/libsvn_wc]
>
> * entries.c
> (write_entry): Store svn_depth_exclude for subdir entries.
>
> * README
> (depth): Document the change in entries.c.
>
> * lock.c
> (do_open): Skip entry with svn_depth_exclude depth.
>
> * adm_ops.c
> (svn_wc_remove_from_revision): Skip excluded subdir entries and preserve
> excluded entry in parent dir.
>
> * crop.c
> (svn_wc_crop_tree): Handle svn_depth_exclude properly, mark target as
> svn_depth_exclude if the parent expects it.
> (crop_children): Same. Also fold the notify branches.
>
> [in subversion/libsvn_client]
>
> * update.c
> (svn_client__update_internal): Skip update totally on svn_depth_exclude.
We usually give the full path (from source tree root) of each file.
It's not a big deal, just fix up the log message when you get a chance.
> --- branches/issue-2843-dev/subversion/libsvn_client/update.c (r31703)
> +++ branches/issue-2843-dev/subversion/libsvn_client/update.c (r31704)
> @@ -160,10 +160,18 @@ svn_client__update_internal(svn_revnum_t
>
> /* We may need to crop the tree if the depth is sticky */
> if (depth_is_sticky && depth < svn_depth_infinity)
> - SVN_ERR(svn_wc_crop_tree(adm_access, target, depth,
> - ctx->notify_func2, ctx->notify_baton2,
> - ctx->cancel_func, ctx->cancel_baton,
> - pool));
> + {
> + SVN_ERR(svn_wc_crop_tree(adm_access, target, depth,
> + ctx->notify_func2, ctx->notify_baton2,
> + ctx->cancel_func, ctx->cancel_baton,
> + pool));
> + /* If we are asked to exclude a target, we can just stop now. */
> + if (depth == svn_depth_exclude)
> + {
> + SVN_ERR(svn_wc_adm_close(adm_access));
> + return SVN_NO_ERROR;
> + }
> + }
What about in the svn_depth_empty case? I would think we'd also "just
stop now" in that case, but maybe I'm missing something...?
> --- branches/issue-2843-dev/subversion/libsvn_wc/README (r31703)
> +++ branches/issue-2843-dev/subversion/libsvn_wc/README (r31704)
> @@ -503,10 +503,18 @@ depth:
> entries and pulls new entries with depth infinity as well.
> Default is infinite (`').
>
> + Apart from above normal values, there is still a special `exclude',
> + which means the directory is excluded from wc even if the depth of
> + parent directory is infinity or immediates. The directory in
> + question will not physically exist on the disk. In other words, we
> + can only store the `exclude' value in the corresponding entry of
> + the entries file in parent directory.
> +
*nod* Thanks for remembering to update README :-).
> The only fields allowed in an entry for a directory (other than the
> this_dir entry) are name, absent, deleted, schedule, copied,
> -copyfrom-url, copyfrom-rev and kind. The other fields are only stored
> -in the this_dir entry for the directory in question.
> +copyfrom-url, copyfrom-rev, kind and depth (only when the value says
> +exclude). The other fields are only stored in the this_dir entry for
> +the directory in question.
We may need a few wording and formatting changes here, but this is good.
> --- branches/issue-2843-dev/subversion/libsvn_wc/adm_ops.c (r31703)
> +++ branches/issue-2843-dev/subversion/libsvn_wc/adm_ops.c (r31704)
> @@ -2500,11 +2500,12 @@ svn_wc_remove_from_revision_control(svn_
> current_entry_name,
> subpool);
>
> - if (svn_wc__adm_missing(adm_access, entrypath))
> + if (svn_wc__adm_missing(adm_access, entrypath)
> + || current_entry->depth == svn_depth_exclude)
> {
> - /* The directory is already missing, so don't try to
> - recurse, just delete the entry in the parent
> - directory. */
> + /* The directory is either missing or excluded,
> + so don't try to recurse, just delete the
> + entry in the parent directory. */
> svn_wc__entry_remove(entries, current_entry_name);
> }
> else
That all makes sense, yup. It might be even clearer to say
"subdirectory" instead of "directory", but it's comprehensible this way
too (I realize the original wording is not yours).
> @@ -2550,6 +2551,7 @@ svn_wc_remove_from_revision_control(svn_
> full_path. We need to remove that entry: */
> if (! is_root)
> {
> + svn_wc_entry_t *dir_entry;
> svn_wc_adm_access_t *parent_access;
>
> svn_path_split(full_path, &parent_dir, &base_name, pool);
> @@ -2558,8 +2560,19 @@ svn_wc_remove_from_revision_control(svn_
> parent_dir, pool));
> SVN_ERR(svn_wc_entries_read(&entries, parent_access, TRUE,
> pool));
> - svn_wc__entry_remove(entries, base_name);
> - SVN_ERR(svn_wc__entries_write(entries, parent_access, pool));
> +
> + /* An exception: When the path is at svn_depth_exclude,
> + the entry in the parent directory should be preserved
> + for bookkeeping purpose. This only happens when the
> + function is called by svn_wc_crop_tree(). */
> + dir_entry = apr_hash_get(entries, base_name,
> + APR_HASH_KEY_STRING);
> + if (dir_entry->depth != svn_depth_exclude)
> + {
> + svn_wc__entry_remove(entries, base_name);
> + SVN_ERR(svn_wc__entries_write(entries, parent_access, pool));
> +
> + }
> }
> }
I realize it's not your code, but I find the reuse of "entries"
confusing here, and wish there were a separate, block-local variable
named "parent_entries" or something. Do you agree?
> --- branches/issue-2843-dev/subversion/libsvn_wc/crop.c (r31703)
> +++ branches/issue-2843-dev/subversion/libsvn_wc/crop.c (r31704)
> @@ -96,29 +96,36 @@ crop_children(svn_wc_adm_access_t *adm_a
> current_entry = val;
> this_path = svn_path_join(dir_path, current_entry->name, iterpool);
>
> - if (current_entry->kind == svn_node_file && depth == svn_depth_empty)
> + if (current_entry->kind == svn_node_file)
> {
> - SVN_ERR_IGNORE_LOCAL_MOD
> - (svn_wc_remove_from_revision_control(dir_access,
> - current_entry->name,
> - TRUE, /* destroy */
> - FALSE, /* instant error */
> - cancel_func,
> - cancel_baton,
> - iterpool));
> + if (depth == svn_depth_empty)
> + SVN_ERR_IGNORE_LOCAL_MOD
> + (svn_wc_remove_from_revision_control(dir_access,
> + current_entry->name,
> + TRUE, /* destroy */
> + FALSE, /* instant error */
> + cancel_func,
> + cancel_baton,
> + iterpool));
> + else
> + continue;
Hmm. Okay, but could depth ever be svn_depth_exclude for a file? (If
so, we should handle it; if not, we should explain why not.)
> - if (notify_func)
> - {
> - svn_wc_notify_t *notify;
> - notify = svn_wc_create_notify(this_path,
> - svn_wc_notify_delete,
> - iterpool);
> - (*notify_func)(notify_baton, notify, iterpool);
> - }
> }
> else if (current_entry->kind == svn_node_dir)
> {
> - if (depth < svn_depth_immediates)
> + if (current_entry->depth == svn_depth_exclude)
> + {
> + /* Preserve the excluded entry if the parent need it.
> + Anyway, don't report on excluded subdir, since they are
> + logically not exist. */
> + if (depth < svn_depth_immediates)
> + {
> + svn_wc__entry_remove(entries, current_entry->name);
> + SVN_ERR(svn_wc__entries_write(entries, dir_access, iterpool));
> + }
> + continue;
> + }
> + else if (depth < svn_depth_immediates)
> {
> svn_wc_adm_access_t *child_access;
> SVN_ERR(svn_wc_adm_retrieve(&child_access, dir_access,
> @@ -132,14 +139,6 @@ crop_children(svn_wc_adm_access_t *adm_a
> cancel_func,
> cancel_baton,
> iterpool));
> - if (notify_func)
> - {
> - svn_wc_notify_t *notify;
> - notify = svn_wc_create_notify(this_path,
> - svn_wc_notify_delete,
> - iterpool);
> - (*notify_func)(notify_baton, notify, iterpool);
> - }
> }
> else
> {
> @@ -151,10 +150,23 @@ crop_children(svn_wc_adm_access_t *adm_a
> cancel_func,
> cancel_baton,
> iterpool));
> + continue;
> }
By 'continue'ing here, we skip the notification block down below.
That's okay because the recursive call will do notification for
this_path, right?
> }
> - /* XXX: What about svn_node_none & svn_node_unkown? Currently assume
> - svn_node_dir*/
> + else
> + {
> + /* XXX: What about svn_node_none & svn_node_unkown? Currently assume
> + svn_node_dir*/
> + }
As always, the second sentence of that comment seems inaccurate to me :-).
> @@ -176,9 +188,8 @@ svn_wc_crop_tree(svn_wc_adm_access_t *an
> const char *full_path;
> svn_wc_adm_access_t *dir_access;
>
> - /* Only makes sense when the depth is restrictive.
> - Currently does not support svn_depth_exclude. */
> - if (!(depth > svn_depth_exclude && depth < svn_depth_infinity))
> + /* Only makes sense when the depth is restrictive. */
> + if (!(depth >= svn_depth_exclude && depth < svn_depth_infinity))
> return SVN_NO_ERROR;
>
> /* Only makes sense to crop a dir target. */
> @@ -187,23 +198,40 @@ svn_wc_crop_tree(svn_wc_adm_access_t *an
> if (!entry || entry->kind != svn_node_dir)
> return SVN_NO_ERROR;
>
> - /* Crop the target itself if we are requested to. */
> + /* Defensive test against excluded target. We may need this until we filter
> + them out in svn_wc_entry(). */
> + if (entry->depth == svn_depth_exclude)
> + return SVN_NO_ERROR;
Are we planning to filter them out in svn_wc_entry()? (Might be clearer
to say "already-exluded" target, by the way.)
Rest of this file looks good.
> --- branches/issue-2843-dev/subversion/libsvn_wc/entries.c (r31703)
> +++ branches/issue-2843-dev/subversion/libsvn_wc/entries.c (r31704)
> @@ -1654,7 +1654,9 @@ write_entry(svn_stringbuf_t *buf,
> }
>
> /* Depth. */
> - if (is_subdir || entry->depth == svn_depth_infinity)
> + /* Accept `exclude' for subdir entry. */
> + if ((is_subdir && entry->depth != svn_depth_exclude)
> + || entry->depth == svn_depth_infinity)
> {
> write_val(buf, NULL, 0);
> }
I had to read that five times, for some reason, but now I see that it
does the right thing.
> --- branches/issue-2843-dev/subversion/libsvn_wc/lock.c (r31703)
> +++ branches/issue-2843-dev/subversion/libsvn_wc/lock.c (r31704)
> @@ -673,6 +673,11 @@ do_open(svn_wc_adm_access_t **adm_access
> if (entry->kind != svn_node_dir
> || ! strcmp(entry->name, SVN_WC_ENTRY_THIS_DIR))
> continue;
> +
> + /* Also skip the excluded subdir. */
> + if (entry->depth == svn_depth_exclude)
> + continue;
> +
> entry_path = svn_path_join(lock->path, entry->name, subpool);
>
> /* Don't use the subpool pool here, the lock needs to persist */
Looks good. Any particular reason you didn't group that condition with
the other "skip" conditions immediately above it?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31704 - in branches/issue-2843-dev/subversion: libsvn_client libsvn_wc
Posted by Karl Fogel <kf...@red-bean.com>.
"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
>> Oh -- please do that on trunk (+1 from me), and just have it come over
>> to the branch the next time you merge.
>
> Err.. My fault. I've done this on my branch after I checked all your reviews,
> along with your other suggestions. So, to fix this problem, should I merge it
> to trunk or submit a patch? Or even revert that commit and redo it in trunk?
Nah, it's okay, just leave it on the branch. I think the slight extra
cleanliness of redoing it would not be worth the noise. Just try to
remember for the future that changes that aren't specific to the branch
can enter on trunk first (and usually someone will be quick to approve,
when they're like this one).
>> > In fact, I can also simply call svn_wc_remove_from_revision_control()
>> > for excluded subdirectory (even though my code may be a bit faster?)
>> > -- the document of svn_wc_remove_from_revision_control() seems a bit
>> > misleading. If I understand correctly, it can handle any target rather
>> > than just 'file and this_dir'.
>>
>> The documentation claims it can only do file and this_dir, but I'm not
>> sure why it has that requirement...
>
> I double checked the implementation and found the document correct. I was
> confused by the check that setup is_file flag when the target name is not
> SVN_WC_ENTRY_THIS_DIR. In fact, if you pass the subdir entry to that function,
> that entry itself will be removed, leaving the entire subdirectory intact.
Yep -- that's what I found when I looked in there too. I'm still not
sure why we arranged things this way, that's all :-).
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31704 - in branches/issue-2843-dev/subversion: libsvn_client libsvn_wc
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Sun, Jun 15, 2008 at 10:31:05AM -0400, Karl Fogel wrote:
> "Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
> >> We usually give the full path (from source tree root) of each file.
> >> It's not a big deal, just fix up the log message when you get a chance.
> >
> > Well, I just followed the hacking guide.
>
> Indeed -- I never noticed that recommendation! Sorry. It's fine either
> way, then. I have to admit I prefer having the full path, since we
> often have files with the same name (e.g., "lock.c") in different places
> in the tree, and if the full path is given I don't have to glance
> backwards to see which file we're dealing with.
No problem. I can do it either way.
> >>
> >> I realize it's not your code, but I find the reuse of "entries"
> >> confusing here, and wish there were a separate, block-local variable
> >> named "parent_entries" or something. Do you agree?
> >
> > Yep! Done, waiting for commit.
>
> Oh -- please do that on trunk (+1 from me), and just have it come over
> to the branch the next time you merge.
Err.. My fault. I've done this on my branch after I checked all your reviews,
along with your other suggestions. So, to fix this problem, should I merge it
to trunk or submit a patch? Or even revert that commit and redo it in trunk?
>
> > In fact, I can also simply call svn_wc_remove_from_revision_control()
> > for excluded subdirectory (even though my code may be a bit faster?)
> > -- the document of svn_wc_remove_from_revision_control() seems a bit
> > misleading. If I understand correctly, it can handle any target rather
> > than just 'file and this_dir'.
>
> The documentation claims it can only do file and this_dir, but I'm not
> sure why it has that requirement...
I double checked the implementation and found the document correct. I was
confused by the check that setup is_file flag when the target name is not
SVN_WC_ENTRY_THIS_DIR. In fact, if you pass the subdir entry to that function,
that entry itself will be removed, leaving the entire subdirectory intact.
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31704 - in branches/issue-2843-dev/subversion:
libsvn_client libsvn_wc
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Tue, Jun 17, 2008 at 11:04:11PM -0400, Karl Fogel wrote:
> "Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
> > I reverted this portion of the commit in my branch and redone it in the trunk.
> > I think this fixes my mistake. And I would like to ask a question about
> > mergeing: How often do you do it when you are working on your branch? I heard
> > that one may have problem next time if he does't do it frequent enough.
>
> Okay. (Even though I just sent a reply saying it's not necessary, it's
> fine that you redid it. You're moving so fast I can't keep up! :-) )
I was just trying to push ahead with the progress a bit, since I have to
suspend my work for about 10 days (to attend a research conference). This is a
relatively long break and there are many miscellaneous modifications await.
I'm just a bit worried.
I believe that 'svn update' has come to a satisfactory status. However, I have
to make sure that all other commands behave correctly with svn_depth_exclude.
The first step would be take care of those passing TRUE to show_hidden
parameter of svn_wc_entry() or svn_wc_entries_read(). I'm not sure whether it
is enough, maybe new test cases can tell (I may need help on this, since I
never used all these sub-commands).
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31704 - in branches/issue-2843-dev/subversion: libsvn_client libsvn_wc
Posted by Karl Fogel <kf...@red-bean.com>.
"Paul Burba" <pt...@gmail.com> writes:
> To demonstrate the problem Blair is talking about:
>
> C:\SVN>svn co http://svn.collab.net/repos/svn/branches/file-externals
> file-externals-2
>
> C:\SVN>cd file-externals-2
>
>>C:\SVN\file-externals-2>svn merge http://svn.collab.net/repos/svn/trunk .
> ..\..\..\subversion\libsvn_ra_neon\util.c:711: (apr_err=160013)
> svn: Working copy path 'www/tasks.html' does not exist in repository
>
> The problem here is issue #3067 'subtrees that don't exist at the
> start or end of a merge range shouldn't break the merge'.
>
> The latest patch for this issue fixes this problem:
> http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=140399
> Kamesh is reviewing this patch, but if anyone else has time to look
> at issue #3067 in any capacity...that would be quite fantastic.
Thanks, Paul.
I'd like to -- but first I'm busy reviewing your "r31059, r31060,
r31061, r31075, r31151, r31398, and r31482" group in 1.5.x/STATUS!
:-)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31704 - in branches/issue-2843-dev/subversion: libsvn_client libsvn_wc
Posted by Paul Burba <pt...@gmail.com>.
On Mon, Jun 23, 2008 at 11:39 AM, Blair Zajac <bl...@orcaware.com> wrote:
> Karl Fogel wrote:
>>
>> Blair Zajac <bl...@orcaware.com> writes:
>>>
>>> Karl Fogel wrote:
>>>>
>>>> Blair Zajac <bl...@orcaware.com> writes:
>>>>>
>>>>> Karl Fogel wrote:
>>>>>>
>>>>>> I thinkd say you should merge trunk to your branch quite frequently.
>>>>>> At
>>>>>> least every few days, though of course it really depends on the rate &
>>>>>> kind of changes that happen on trunk. Don't let your branch get too
>>>>>> far
>>>>>> out of sync...
>>>>>
>>>>> I wish that were possible with the current merging code :(
>>>>
>>>> You wish it were possible to get too far out of sync, or you wish it
>>>> were possible to merge very often?
>>>>
>>>> (I'm assuming the former, but just want to make sure there isn't
>>>> something I didn't know about :-) ).
>>>
>>> To merge often.
>>>
>>> Do a checkout of the file-externals branch and try to merge trunk into
>>> it.
>>
>> I haven't had time to perform that experiment, but it sounds like you're
>> saying something very surprising: that if you do merges fairly
>> frequently, they fail, but if you do them less frequently, they succeed.
>>
>> I mean, you should be pulling in the same changes, it's just a question
>> of how you're batching them, right?
>
> No, I'm not saying that. It has nothing to do with frequency. Just that
> the current merge code doesn't handle when the issue-3000 commit into trunk
> was done then you want to merge that to another branch.
To demonstrate the problem Blair is talking about:
C:\SVN>svn co http://svn.collab.net/repos/svn/branches/file-externals
file-externals-2
C:\SVN>cd file-externals-2
>C:\SVN\file-externals-2>svn merge http://svn.collab.net/repos/svn/trunk .
..\..\..\subversion\libsvn_ra_neon\util.c:711: (apr_err=160013)
svn: Working copy path 'www/tasks.html' does not exist in repository
The problem here is issue #3067 'subtrees that don't exist at the
start or end of a merge range shouldn't break the merge'.
The latest patch for this issue fixes this problem:
http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=140399
Kamesh is reviewing this patch, but if anyone else has time to look
at issue #3067 in any capacity...that would be quite fantastic.
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31704 - in branches/issue-2843-dev/subversion: libsvn_client
libsvn_wc
Posted by Blair Zajac <bl...@orcaware.com>.
Karl Fogel wrote:
> Blair Zajac <bl...@orcaware.com> writes:
>> Karl Fogel wrote:
>>> Blair Zajac <bl...@orcaware.com> writes:
>>>> Karl Fogel wrote:
>>>>> I thinkd say you should merge trunk to your branch quite frequently. At
>>>>> least every few days, though of course it really depends on the rate &
>>>>> kind of changes that happen on trunk. Don't let your branch get too far
>>>>> out of sync...
>>>> I wish that were possible with the current merging code :(
>>> You wish it were possible to get too far out of sync, or you wish it
>>> were possible to merge very often?
>>>
>>> (I'm assuming the former, but just want to make sure there isn't
>>> something I didn't know about :-) ).
>> To merge often.
>>
>> Do a checkout of the file-externals branch and try to merge trunk into it.
>
> I haven't had time to perform that experiment, but it sounds like you're
> saying something very surprising: that if you do merges fairly
> frequently, they fail, but if you do them less frequently, they succeed.
>
> I mean, you should be pulling in the same changes, it's just a question
> of how you're batching them, right?
No, I'm not saying that. It has nothing to do with frequency. Just that the
current merge code doesn't handle when the issue-3000 commit into trunk was done
then you want to merge that to another branch.
Blair
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31704 - in branches/issue-2843-dev/subversion: libsvn_client libsvn_wc
Posted by Karl Fogel <kf...@red-bean.com>.
Blair Zajac <bl...@orcaware.com> writes:
> Karl Fogel wrote:
>> Blair Zajac <bl...@orcaware.com> writes:
>>> Karl Fogel wrote:
>>>> I thinkd say you should merge trunk to your branch quite frequently. At
>>>> least every few days, though of course it really depends on the rate &
>>>> kind of changes that happen on trunk. Don't let your branch get too far
>>>> out of sync...
>>> I wish that were possible with the current merging code :(
>>
>> You wish it were possible to get too far out of sync, or you wish it
>> were possible to merge very often?
>>
>> (I'm assuming the former, but just want to make sure there isn't
>> something I didn't know about :-) ).
>
> To merge often.
>
> Do a checkout of the file-externals branch and try to merge trunk into it.
I haven't had time to perform that experiment, but it sounds like you're
saying something very surprising: that if you do merges fairly
frequently, they fail, but if you do them less frequently, they succeed.
I mean, you should be pulling in the same changes, it's just a question
of how you're batching them, right?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31704 - in branches/issue-2843-dev/subversion: libsvn_client
libsvn_wc
Posted by Blair Zajac <bl...@orcaware.com>.
Karl Fogel wrote:
> Blair Zajac <bl...@orcaware.com> writes:
>> Karl Fogel wrote:
>>> I thinkd say you should merge trunk to your branch quite frequently. At
>>> least every few days, though of course it really depends on the rate &
>>> kind of changes that happen on trunk. Don't let your branch get too far
>>> out of sync...
>> I wish that were possible with the current merging code :(
>
> You wish it were possible to get too far out of sync, or you wish it
> were possible to merge very often?
>
> (I'm assuming the former, but just want to make sure there isn't
> something I didn't know about :-) ).
To merge often.
Do a checkout of the file-externals branch and try to merge trunk into it.
Blair
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31704 - in branches/issue-2843-dev/subversion: libsvn_client libsvn_wc
Posted by Karl Fogel <kf...@red-bean.com>.
Blair Zajac <bl...@orcaware.com> writes:
> Karl Fogel wrote:
>> I thinkd say you should merge trunk to your branch quite frequently. At
>> least every few days, though of course it really depends on the rate &
>> kind of changes that happen on trunk. Don't let your branch get too far
>> out of sync...
>
> I wish that were possible with the current merging code :(
You wish it were possible to get too far out of sync, or you wish it
were possible to merge very often?
(I'm assuming the former, but just want to make sure there isn't
something I didn't know about :-) ).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31704 - in branches/issue-2843-dev/subversion: libsvn_client
libsvn_wc
Posted by Blair Zajac <bl...@orcaware.com>.
Karl Fogel wrote:
> I thinkd say you should merge trunk to your branch quite frequently. At
> least every few days, though of course it really depends on the rate &
> kind of changes that happen on trunk. Don't let your branch get too far
> out of sync...
I wish that were possible with the current merging code :(
Eagerly waiting Paul's work on merges :)
Blair
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31704 - in branches/issue-2843-dev/subversion: libsvn_client libsvn_wc
Posted by Karl Fogel <kf...@red-bean.com>.
"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
> I reverted this portion of the commit in my branch and redone it in the trunk.
> I think this fixes my mistake. And I would like to ask a question about
> mergeing: How often do you do it when you are working on your branch? I heard
> that one may have problem next time if he does't do it frequent enough.
Okay. (Even though I just sent a reply saying it's not necessary, it's
fine that you redid it. You're moving so fast I can't keep up! :-) )
I thinkd say you should merge trunk to your branch quite frequently. At
least every few days, though of course it really depends on the rate &
kind of changes that happen on trunk. Don't let your branch get too far
out of sync...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31704 - in branches/issue-2843-dev/subversion:
libsvn_client libsvn_wc
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Mon, Jun 16, 2008 at 01:40:21PM +0800, Rui, Guo wrote:
> > >>
> > >> I realize it's not your code, but I find the reuse of "entries"
> > >> confusing here, and wish there were a separate, block-local variable
> > >> named "parent_entries" or something. Do you agree?
> > >
> > > Yep! Done, waiting for commit.
> >
> > Oh -- please do that on trunk (+1 from me), and just have it come over
> > to the branch the next time you merge.
>
> Err.. My fault. I've done this on my branch after I checked all your reviews,
> along with your other suggestions. So, to fix this problem, should I merge it
> to trunk or submit a patch? Or even revert that commit and redo it in trunk?
>
I reverted this portion of the commit in my branch and redone it in the trunk.
I think this fixes my mistake. And I would like to ask a question about
mergeing: How often do you do it when you are working on your branch? I heard
that one may have problem next time if he does't do it frequent enough.
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31704 - in branches/issue-2843-dev/subversion: libsvn_client libsvn_wc
Posted by Karl Fogel <kf...@red-bean.com>.
"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
>> We usually give the full path (from source tree root) of each file.
>> It's not a big deal, just fix up the log message when you get a chance.
>
> Well, I just followed the hacking guide.
Indeed -- I never noticed that recommendation! Sorry. It's fine either
way, then. I have to admit I prefer having the full path, since we
often have files with the same name (e.g., "lock.c") in different places
in the tree, and if the full path is given I don't have to glance
backwards to see which file we're dealing with.
>> > @@ -2558,8 +2560,19 @@ svn_wc_remove_from_revision_control(svn_
>> > parent_dir, pool));
>> > SVN_ERR(svn_wc_entries_read(&entries, parent_access, TRUE,
>> > pool));
>> > - svn_wc__entry_remove(entries, base_name);
>> > - SVN_ERR(svn_wc__entries_write(entries, parent_access, pool));
>> > +
>> > + /* An exception: When the path is at svn_depth_exclude,
>> > + the entry in the parent directory should be preserved
>> > + for bookkeeping purpose. This only happens when the
>> > + function is called by svn_wc_crop_tree(). */
>> > + dir_entry = apr_hash_get(entries, base_name,
>> > + APR_HASH_KEY_STRING);
>> > + if (dir_entry->depth != svn_depth_exclude)
>> > + {
>> > + svn_wc__entry_remove(entries, base_name);
>> > + SVN_ERR(svn_wc__entries_write(entries, parent_access, pool));
>> > +
>> > + }
>> > }
>> > }
>>
>> I realize it's not your code, but I find the reuse of "entries"
>> confusing here, and wish there were a separate, block-local variable
>> named "parent_entries" or something. Do you agree?
>
> Yep! Done, waiting for commit.
Oh -- please do that on trunk (+1 from me), and just have it come over
to the branch the next time you merge.
> In fact, I can also simply call svn_wc_remove_from_revision_control()
> for excluded subdirectory (even though my code may be a bit faster?)
> -- the document of svn_wc_remove_from_revision_control() seems a bit
> misleading. If I understand correctly, it can handle any target rather
> than just 'file and this_dir'.
The documentation claims it can only do file and this_dir, but I'm not
sure why it has that requirement...
>> > }
>> > - /* XXX: What about svn_node_none & svn_node_unkown? Currently assume
>> > - svn_node_dir*/
>> > + else
>> > + {
>> > + /* XXX: What about svn_node_none & svn_node_unkown? Currently assume
>> > + svn_node_dir*/
>> > + }
>>
>> As always, the second sentence of that comment seems inaccurate to me :-).
>
> Shy. It dates back to the time that I created the file. I just have no idea if
> I need to shrow an exception here.
I think you should. It's better to be strict at first; we can decide to
loosen up later if it's a problem.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31704 - in branches/issue-2843-dev/subversion: libsvn_client libsvn_wc
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Sat, Jun 14, 2008 at 08:57:47PM -0400, Karl Fogel wrote:
> firemeteor@tigris.org writes:
> > Log:
> > On issue-2843-dev branch.
> >
> > Handle svn_depth_exclude in the cropping logic. However, other part of the
> > libsvn_wc code still don't honor the svn_depth_exclude flag.
> >
> > [in subversion/libsvn_wc]
> >
> > * entries.c
> > (write_entry): Store svn_depth_exclude for subdir entries.
> >
> > * README
> > (depth): Document the change in entries.c.
> >
> > * lock.c
> > (do_open): Skip entry with svn_depth_exclude depth.
> >
> > * adm_ops.c
> > (svn_wc_remove_from_revision): Skip excluded subdir entries and preserve
> > excluded entry in parent dir.
> >
> > * crop.c
> > (svn_wc_crop_tree): Handle svn_depth_exclude properly, mark target as
> > svn_depth_exclude if the parent expects it.
> > (crop_children): Same. Also fold the notify branches.
> >
> > [in subversion/libsvn_client]
> >
> > * update.c
> > (svn_client__update_internal): Skip update totally on svn_depth_exclude.
>
> We usually give the full path (from source tree root) of each file.
> It's not a big deal, just fix up the log message when you get a chance.
Well, I just followed the hacking guide.
>
> > --- branches/issue-2843-dev/subversion/libsvn_client/update.c (r31703)
> > +++ branches/issue-2843-dev/subversion/libsvn_client/update.c (r31704)
> > @@ -160,10 +160,18 @@ svn_client__update_internal(svn_revnum_t
> >
> > /* We may need to crop the tree if the depth is sticky */
> > if (depth_is_sticky && depth < svn_depth_infinity)
> > - SVN_ERR(svn_wc_crop_tree(adm_access, target, depth,
> > - ctx->notify_func2, ctx->notify_baton2,
> > - ctx->cancel_func, ctx->cancel_baton,
> > - pool));
> > + {
> > + SVN_ERR(svn_wc_crop_tree(adm_access, target, depth,
> > + ctx->notify_func2, ctx->notify_baton2,
> > + ctx->cancel_func, ctx->cancel_baton,
> > + pool));
> > + /* If we are asked to exclude a target, we can just stop now. */
> > + if (depth == svn_depth_exclude)
> > + {
> > + SVN_ERR(svn_wc_adm_close(adm_access));
> > + return SVN_NO_ERROR;
> > + }
> > + }
>
> What about in the svn_depth_empty case? I would think we'd also "just
> stop now" in that case, but maybe I'm missing something...?
What if we are asked to pull in the not-existing-yet target?
> > @@ -2550,6 +2551,7 @@ svn_wc_remove_from_revision_control(svn_
> > full_path. We need to remove that entry: */
> > if (! is_root)
> > {
> > + svn_wc_entry_t *dir_entry;
> > svn_wc_adm_access_t *parent_access;
> >
> > svn_path_split(full_path, &parent_dir, &base_name, pool);
> > @@ -2558,8 +2560,19 @@ svn_wc_remove_from_revision_control(svn_
> > parent_dir, pool));
> > SVN_ERR(svn_wc_entries_read(&entries, parent_access, TRUE,
> > pool));
> > - svn_wc__entry_remove(entries, base_name);
> > - SVN_ERR(svn_wc__entries_write(entries, parent_access, pool));
> > +
> > + /* An exception: When the path is at svn_depth_exclude,
> > + the entry in the parent directory should be preserved
> > + for bookkeeping purpose. This only happens when the
> > + function is called by svn_wc_crop_tree(). */
> > + dir_entry = apr_hash_get(entries, base_name,
> > + APR_HASH_KEY_STRING);
> > + if (dir_entry->depth != svn_depth_exclude)
> > + {
> > + svn_wc__entry_remove(entries, base_name);
> > + SVN_ERR(svn_wc__entries_write(entries, parent_access, pool));
> > +
> > + }
> > }
> > }
>
> I realize it's not your code, but I find the reuse of "entries"
> confusing here, and wish there were a separate, block-local variable
> named "parent_entries" or something. Do you agree?
Yep! Done, waiting for commit.
>
> > --- branches/issue-2843-dev/subversion/libsvn_wc/crop.c (r31703)
> > +++ branches/issue-2843-dev/subversion/libsvn_wc/crop.c (r31704)
> > @@ -96,29 +96,36 @@ crop_children(svn_wc_adm_access_t *adm_a
> > current_entry = val;
> > this_path = svn_path_join(dir_path, current_entry->name, iterpool);
> >
> > - if (current_entry->kind == svn_node_file && depth == svn_depth_empty)
> > + if (current_entry->kind == svn_node_file)
> > {
> > - SVN_ERR_IGNORE_LOCAL_MOD
> > - (svn_wc_remove_from_revision_control(dir_access,
> > - current_entry->name,
> > - TRUE, /* destroy */
> > - FALSE, /* instant error */
> > - cancel_func,
> > - cancel_baton,
> > - iterpool));
> > + if (depth == svn_depth_empty)
> > + SVN_ERR_IGNORE_LOCAL_MOD
> > + (svn_wc_remove_from_revision_control(dir_access,
> > + current_entry->name,
> > + TRUE, /* destroy */
> > + FALSE, /* instant error */
> > + cancel_func,
> > + cancel_baton,
> > + iterpool));
> > + else
> > + continue;
>
> Hmm. Okay, but could depth ever be svn_depth_exclude for a file? (If
> so, we should handle it; if not, we should explain why not.)
I currently choose not to crop the tree on a file basis. But even if I do crop
a single file, it will not happen here. The svn_wc_crop_tree() should handle
that situation. (I may also need to skip the notification in that situation.
I'll document this.)
In fact, I can also simply call svn_wc_remove_from_revision_control() for
excluded subdirectory (even though my code may be a bit faster?) -- the document
of svn_wc_remove_from_revision_control() seems a bit misleading. If I
understand correctly, it can handle any target rather than just 'file and
this_dir'.
> > @@ -132,14 +139,6 @@ crop_children(svn_wc_adm_access_t *adm_a
> > cancel_func,
> > cancel_baton,
> > iterpool));
> > - if (notify_func)
> > - {
> > - svn_wc_notify_t *notify;
> > - notify = svn_wc_create_notify(this_path,
> > - svn_wc_notify_delete,
> > - iterpool);
> > - (*notify_func)(notify_baton, notify, iterpool);
> > - }
> > }
> > else
> > {
> > @@ -151,10 +150,23 @@ crop_children(svn_wc_adm_access_t *adm_a
> > cancel_func,
> > cancel_baton,
> > iterpool));
> > + continue;
> > }
>
> By 'continue'ing here, we skip the notification block down below.
> That's okay because the recursive call will do notification for
> this_path, right?
Yep!
>
> > }
> > - /* XXX: What about svn_node_none & svn_node_unkown? Currently assume
> > - svn_node_dir*/
> > + else
> > + {
> > + /* XXX: What about svn_node_none & svn_node_unkown? Currently assume
> > + svn_node_dir*/
> > + }
>
> As always, the second sentence of that comment seems inaccurate to me :-).
Shy. It dates back to the time that I created the file. I just have no idea if
I need to shrow an exception here.
> > --- branches/issue-2843-dev/subversion/libsvn_wc/lock.c (r31703)
> > +++ branches/issue-2843-dev/subversion/libsvn_wc/lock.c (r31704)
> > @@ -673,6 +673,11 @@ do_open(svn_wc_adm_access_t **adm_access
> > if (entry->kind != svn_node_dir
> > || ! strcmp(entry->name, SVN_WC_ENTRY_THIS_DIR))
> > continue;
> > +
> > + /* Also skip the excluded subdir. */
> > + if (entry->depth == svn_depth_exclude)
> > + continue;
> > +
> > entry_path = svn_path_join(lock->path, entry->name, subpool);
> >
> > /* Don't use the subpool pool here, the lock needs to persist */
>
> Looks good. Any particular reason you didn't group that condition with
> the other "skip" conditions immediately above it?
Nope. Just to make my comment more specific. :-)
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org