Julian Foad <julianfoad@btopenworld.com> wrote on 06/27/2006 07:51:21 PM:
> Paul Burba wrote:
> > Julian Foad <julianfoad@btopenworld.com> wrote on 06/22/2006 07:12:16
PM:
> >>Paul Burba wrote:
> >
> >>>* subversion/libsvn_wc/questions.c
> >>> (svn_wc__text_modified_internal_p): New arg. Update call to
> >>> svn_wc__text_base_path.
> >>> (svn_wc_text_modified_p2): New. Update comment.
> >>> (svn_wc_text_modified_p): Reimplement as wrapper around
> >>> svn_wc_text_modified_p2
> >>
> >>I'm not keen on adding a boolean parameter to any API. Calls
withmultiple
> >>booleans quickly become unreadable. But it's not a big deal here
> >>compared with some of our other APIs.
> >>
> >>Thinking out loud... I do wonder whether providing operations on
> thetemporary
> >>text base is something we really want to be exposing in this way.
> >
> > Are you thinking a new function to handle comparisons with the temp
text
> > base is better or that operating on the temp text base is a
fundamentally
> > flawed design? If the latter, do you have any specific concerns?
>
> Actually I wasn't thinking that deeply at all - it was just a "first
> impression" that providing APIs to access a "temporary" object is
> either a bad
> idea our should be done thoroughly. I do see in this patch that the
vast
> majority of calls to this function don't want to access the temporary
copy,
> which would seem to suggest that keeping an API without that option
> is sensible.
>
> I'll hopefully think more about this later, and reply in another mail.
Ok, I leave it as is for now.
> > In a nutshell, I'm using svn_wc_text_modified_p2() this way because:
While
> > not absolutely necessary, it's beneficial to "fix" the entries file so
> > obstructing files with no actual differences have their text-time set
to
> > the file's last modified time. Presently the patch relies on
> > svn_wc_text_modified_p() to do this, but can only do so if it can
operate
> > on files about to be added (i.e. whose text base exists only in the
temp
> > area).
> >
> >>Are we providing the option to access the "tmp" text base in all other
> >>libsvn_wc APIs that access the text base? (I haven't looked.)
> >
> > No, this is something new.
>
> >>>checkout (co): Check out a working copy from a repository.
> >>>usage: checkout URL[@REV]... [PATH]
> >>
> >>[...]
> > I see your point, how does this sound?
> >
> > If --force is used, unversioned obstructing paths in the working
> > copy destination do not automatically cause the check out to fail.
> > If the obstructing path is the same type (file or directory) as the
> > corresponding path in the repository it becomes versioned but its
> > contents are left 'as-is' in the working copy. This means that an
> > obstructing directory's unversioned children may also obstruct and
> > become versioned. For files, any content differences between the
> > obstruction and the repository are treated like a local modification
> > to the working copy.
>
> That text seems good.
Added similar text to switch and update as well.
> [...]
> > Sorry bout that, the bit about "applicable auto-props" is a mistake on
my
> > part. The patch as it stands now does *not* add any auto-props, it
just
> > applies the props from the repository, just as sw, up, and co normally
do
> > when adding files.
>
> OK.
>
> >>I think it would make
> >>sense to give the working file the properties that "svn add" would
> have given
> >>it, and then any differences between that set and the pristine
> base set from
> >>the repository will show up as local modifications. To me that
> would be the
> >>equivalent for properties of what we do with the base text, and
> therefore the
> >>right thing to do.
> >
> > Regardless of the equivalency between treating props this way and how
text
> > is treated, I don't see it as desirable behavior. Do we really want
to
> > lose all the props from the repository? Is there a common use case
where
> > this is preferable?
>
> What I was suggesting is that the BASE props come from the repository
and the
> WORKING props come from the WC's automatic rules (including "auto-props"
and
> auto-detection of executable, mime-type, etc.), just as if the user
> had "added"
> or "imported" the file. I suggest that this mirrors the fact that the
BASE
> text comes from the repos and the WORKING text from the WC. Thus no
> properties
> are "lost"; it is up to the user to observe any differences and
> decide what to
> do about them, just like with the text. In some situations I feel this
would
> be the logical thing to do, but I'm unable to think of a good example.
Ah, that is what you said the first time...I misread it.
> The alternative, and I think this is what you are suggesting, is to set
the
> working props equal to the base props from the repository. I believe
this
> makes sense if the expected use case is when the obstructing files and
> directories have been obtained from the repository and perhaps modified
> somewhat, but not been replaced with different types of files
(executability,
> binaryness, etc.).
>
> Now that I think about it, I agree that the latter behaviour is much
more
> likely to be useful in most cases, and I'm happy with it.
Ok, we'll go with the existing approach.
> >>> For each item checked out a line will start with a character
> >>> reporting the action taken. For unforced checkouts this character
> >>> is always 'A' for 'Added'. Forced checkouts report obstructed
paths
> >>> with 'E' for 'Existed' and use 'A' otherwise.
> >>
> >>This paragraph is talking about files Added by the checkout, but
> doesn't say
> >>so, incorrectly implying that no other codes than 'E' and 'A' can
> >>result from a checkout.
> >
> > Talk about missing the forest for the trees, I got so caught up with
> > initial checkouts into unversioned dirs that I forgot about the
> > update-esque behavior of doing subsequent checkouts into an existing
WC.
>
> Heh, I can quite see how one could get into that situation with a
> patch of this
> size ... but isn't it me getting into a muddle this time? Isn't it
correct
> that a checkout can only add files? It's updates (and switches) that
can do
> other things.
>
> > Perhaps the best approach is to simply not mention the notification
codes
> > as you suggested with switch below? I've done just that for now,
unless
> > anyone feels strongly that 'E' warrants special mention in co's help.
>
> If I'm right that I was wrong, put it back! Sorry for the confusion. If
I'm
> wrong now and was right before, please enlighten me. :-)
Heh, I think your wrong now and was right before! My initial reaction
was, "svn co can only 'A'dd paths! Julian must be crazy!" :-) But
subsequent checkouts are essentially updates, so 'D', 'U', 'C', and 'G'
can all make an appearence:
Initial checkout:
svn co file:///home/BURBA/REPOS/CO_MANIA /home/BURBA/WCS/CO_MAINIA_WC
A \home\BURBA\WCS\CO_MAINIA_WC\Doc A.txt
A \home\BURBA\WCS\CO_MAINIA_WC\Doc B.txt
A \home\BURBA\WCS\CO_MAINIA_WC\Doc C.txt
A \home\BURBA\WCS\CO_MAINIA_WC\Doc D.txt
A \home\BURBA\WCS\CO_MAINIA_WC\Doc E.txt
Checked out revision 1.
Make some changes to repos via another WC:
svn ci -m "" "/home/BURBA/WCS/CO_MAINIA_WC1"
Deleting home\BURBA\WCS\CO_MAINIA_WC1\Doc A.txt
Sending home\BURBA\WCS\CO_MAINIA_WC1\Doc B.txt
Sending home\BURBA\WCS\CO_MAINIA_WC1\Doc C.txt
Sending home\BURBA\WCS\CO_MAINIA_WC1\Doc E.txt
Transmitting file data ...
Committed revision 2.
Checkout into first WC again:
svn co file:///home/BURBA/REPOS/CO_MANIA /home/BURBA/WCS/CO_MAINIA_WC
D \home\BURBA\WCS\CO_MAINIA_WC\Doc A.txt
C \home\BURBA\WCS\CO_MAINIA_WC\Doc B.txt
U \home\BURBA\WCS\CO_MAINIA_WC\Doc C.txt
G \home\BURBA\WCS\CO_MAINIA_WC\Doc E.txt
Checked out revision 2.
> I think we've got the concept sorted now, and all that remains is a few
> implementation details :-)
>
> There follows a code review of about half of the patch. I've run out of
time
> tonight and thought it would be best to post what I have written so far.
>
>
> > Index: subversion/include/svn_client.h
> > ===================================================================
> > --- subversion/include/svn_client.h (revision 20268)
> > +++ subversion/include/svn_client.h (working copy)
> > @@ -696,15 +696,40 @@
> > * just the directory represented by @a URL and its immediate
> > * non-directory children, but none of its child directories (if
any).
> > *
> > + * If @a force_checkout is @c TRUE then checkout tolerates an
> existing local
> > + * tree rooted at @a PATH even if some paths in the local tree
> are identical
>
> This comment should probably say "unversioned" somewhere, like the other
APIs
> and the command-line help do.
Done.
> "some paths ... are identical": The word "path" is sometimes used tomean
the
> item existing at a given path, so this could be misleading. I suggest
making
> the wording consistent with your doc strings for "update" and "switch"
which
> don't use the word "identical" and are clearer.
Agreed, that doc string used my original language from when the patch
handled only co. It also used the term "colliding" rather than
"obstructing", which is inconsistent with the rest of the patch. I
improved the doc string re the force argument and made it consistent
across svn_client_checkout3(), svn_client_update3(), and
svn_client_switch2().
> It would be really nice for the APIs to use a more descriptive parameter
name
> than "force" (and the "_checkout" suffix doesn't help). Think: it's
quite
> likely that we'll add a way to force some other aspect of its
> behaviour later.
> Maybe use "{allow,tolerate,integrate,take_over}_unver_{items,
> obstructions}" ?
You taunt me I think :-) Anything but the 'T' word! It exists only in
this thread's name...and I'd like to keep it that way, it really describes
something more elaborate than what I'm doing here.
> I don't think most of those combinations are too long given that
callers
> don't need to type it. Ah, I see in svn_wc_get_*_editor you've called
it
> "allow_obstructions". That's much better than "force", but only
> some kinds of
> obstructions are allowed. How about using "allow_unver_obstructions"?
> Whichever you choose, try to use the same name in all places except
> when there
> is a reason not to, especially in all the public APIs that are affected.
Easy enough, "allow_unver_obstructions" it is. I changed this everywhere,
I saw no reason not to.
> We should seriously consider adding a more specific command-line option
name
> too. It's pure chance that the three commands you wanted to add
> this behaviour
> to didn't already have "--force" options for some other purpose.
One man's pure chance is another's serendipity. Seriously, I have no
strong objections to a new option, but wouldn't this be a problem with
*any* new behavior associated with --force? Is it simply the ill-defined
nature of --force in general that concerns you?
http://svn.haxx.se/dev/archive-2006-05/0143.shtml
> Syle nit: Personally I hate reading "is @c FALSE" and prefer "is false",
and
> the same with "true" and "null", as these three are well-known concepts
that
> don't require references to the particular preprocessor macros that we
use to
> represent them. In Doxygen output, of course, the "@c" is
> suppressed and small
> capitals are used so I hardly notice it, but normally I'm reading
> the C header
> text. However, both styles are used extensively at present so it's your
> choice. (Apologies: I just can't stop myself from ranting on these
sorts of
> trivia!)
I never read the Doxygen output either so I'm quite happy to use the more
readable format you suggest. I was defaulting to what I thought was the
strict Doxygen standard and hadn't realized that "true", "false", etc.
were ok.
> > + * to those in @a URL. File paths that collide will effectively
treat the
> > + * local file as a user modification to the pristine checkout. If @a
force
> > + * is @c FALSE then the checkout will abort if there are any
> colliding paths.
>
> Maybe also say that working properties are set equal to the base
properties?
> That doesn't seem implicit to me.
Fixed there and added to the doc string for svn_client_update3() and
svn_client_switch2() too.
> > + *
> > * If @a URL refers to a file rather than a directory, return the
> > * error @c SVN_ERR_UNSUPPORTED_FEATURE. If @a URL does not exist,
> > * return the error @c SVN_ERR_RA_ILLEGAL_URL.
> > *
> > * Use @a pool for any temporary allocation.
> > *
> > - * @since New in 1.2.
> > + * @since New in 1.5.
> > */
> > svn_error_t *
> > +svn_client_checkout3(svn_revnum_t *result_rev,
> > + const char *URL,
> > + const char *path,
> > + const svn_opt_revision_t *peg_revision,
> > + const svn_opt_revision_t *revision,
> > + svn_boolean_t recurse,
> > + svn_boolean_t ignore_externals,
> > + svn_boolean_t force_checkout,
> > + svn_client_ctx_t *ctx,
> > + apr_pool_t *pool);
> > +
> > +
> > +/**
> > + * Similar to svn_client_checkout3() but with the force parameter
always
>
> Give the exact name of the parameter.
Fixed.
> > + * set to @c FALSE.
> > + *
> > + * @deprecated Provided for backward compatibility with the 1.4 API.
> > + */
> > +svn_error_t *
> > svn_client_checkout2(svn_revnum_t *result_rev,
> > const char *URL,
> > const char *path,
> [...]
>
> > Index: subversion/libsvn_client/externals.c
> > ===================================================================
>
> So externals won't yet support this mode. I guess that's reasonable...
I've
> never actually used them so have not been much interested in their
semantics.
I'm open to supporting it but haven't looked into any of the implications
yet (if there are any).
> > Index: subversion/libsvn_wc/update_editor.c
> > ===================================================================
> [...]
> > @@ -1057,15 +1066,65 @@
> > || ((! copyfrom_path) &&
(SVN_IS_VALID_REVNUM(copyfrom_revision))))
> > abort();
> >
> > - /* There should be nothing with this name. */
> > SVN_ERR(svn_io_check_path(db->path, &kind, db->pool));
> > - if (kind != svn_node_none)
> > - return svn_error_createf
> > - (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> > - _("Failed to add directory '%s': "
> > - "object of the same name already exists"),
> > - svn_path_local_style(db->path, pool));
> >
> > + /* Check for obstructions. */
> > + if (eb->allow_obstructions)
> > + {
> > + /* The path can exist, but it must be a directory... */
> > + if (kind == svn_node_file || kind == svn_node_unknown)
>
> "(kind != svn_node_dir)" would answer the question more directly.
At this point we only know that obstructions are allowed, there might
*not* be an obstruction though, so svn_node_none or svn_node_dir are both
acceptable.
> > + return svn_error_createf
> > + (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> > + _("Failed to add directory '%s': a non-directory object of
the "
> > + "same name already exists"),
> > + svn_path_local_style(db->path, pool));
> > + else if (kind == svn_node_dir)
>
> ... and this "else if" is redundant either way.
Fixed.
> > + {
> > + /* ...Ok, it's a directory but it can't be versioned. We
don't
> > + handle that, yet... */
> > + svn_wc_adm_access_t *adm_access;
> > + const char *obstructing_path = svn_path_join(
> > + pb->path, svn_path_basename(path, pool), pool);
>
> Why is obstructing_path not just db->path? That's the path that we
found of
> kind svn_node_dir.
Yes that works, my reasoning for recreating is lost in the haze of time.
> > +
> > + /* Test the obstructing dir to see if it's versioned. */
> > + svn_error_t *err = svn_wc_adm_open3(&adm_access, NULL,
> > + obstructing_path,
FALSE, 0,
> > + NULL, NULL, pool);
> > + if (err && err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)
> > + {
> > + /* Something quite unexepected has happened. */
> > + return err;
> > + }
> > + else if (err)
> > + {
> > + /* Obstructing dir is not versioned, just need to flag
it as
> > + existing then we are done here. */
> > + exists = TRUE;
> > + svn_error_clear(err);
> > + }
> > + else
> > + {
> > + /* Obstructing dir *is* versioned. */
> > + return svn_error_createf
> > + (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> > + _("Failed to forcibly add directory '%s': a
versioned "
> > + "directory of the same name already exists"),
>
> Minor nit: I don't think inclusion of the word "forcibly" helps,
> especially if
> the operation was invoked through a client interface other than "svn
> co --force".
Removed "forcibly".
> > + svn_path_local_style(db->path, pool));
> > + }
> [...]
> > @@ -1684,9 +1764,9 @@
> > applied. */
> > fb->prop_changed = 1;
> >
> > - /* Special case: if the file is added during a checkout, cache the
> > - last-changed-date propval for future use. */
> > - if (eb->use_commit_times
> > + /* Special case: if the file is added during a checkout or existed,
cache
> > + the last-changed-date propval for future use. */
> > + if ((eb->use_commit_times || fb->existed)
>
> Are you sure about this change? It looks odd to me, looks like the code
> doesn't match the comment, even before your change. Does "if
> (use_commit_times)" really mean "if (file is added during a checkout)"?
Ignoring my patch for a moment, that comment is not very enlightening.
use_commit_times really means the miscellaneous config variable
"use-commit-times" is "yes" - see r6983. The comment should probably be
something more like this:
- /* Special case: if the file is added during a checkout, cache the
- last-changed-date propval for future use. */
+ /* Special case: If use-commit-times config variable is set we
+ cache the last-changed-date propval so we can use it to set
+ the working file's timestamp. */
As for the patch, the reason I cache the last changed date is so that it's
available later in merge_file(). There it's used to set the text-time for
(differing) obstructing files. Otherwise the obstruction would have no
text-time entry. I suppose this isn't necessary(?) I need to think on
that more.
/* It is quite legitimate for modifications to the working copy to
produce a timestamp variation with no text variation. If it turns out
that there are no differences then we might be able to "repair" the
text-time in the entries file and so avoid the expensive file
contents
comparison in the future. */
> > && (strcmp(name, SVN_PROP_ENTRY_COMMITTED_DATE) == 0)
> > && value)
> > fb->last_changed_date = apr_pstrdup(fb->pool, value->data);
> [...]
>
> Right, that's all for tonight; I'll try to do some more but it probably
won't
> be tomorrow. Feel free to send an update in the meantime. Thanks
> for listening...
...Thanks for the review. Here is an updated log and patch reflecting
your latest suggestions.
FYI: I'm on vacation in the deep woods of New Hampshire from 6/30 - 7/10
and should have e-mail access via dial-up during that time. I'll be
checking my messages, but probably not every day. Barring any technical
difficulties, my Subversion activity level during that time will largely
be a function of how much it rains :-P
Paul B.
[[[
Support --force option with svn checkout, update, and switch.
With the force option, co, sw, and up now tolerate unversioned
obstructing paths when adding new paths of the same type, rather than
generating a SVN_ERR_WC_OBSTRUCTED_UPDATE error.
If the obstructing path is the same type (file or directory) as the
corresponding path in the repository it will be left 'as-is' in the
working copy. For directories this simply means the obstruction is
tolerated. For files, any content differences between the obstruction
and the repository are treated like a local modification to the
working copy.
This patch is an expansion of one originally posted by Jonathan Gilbert
<o2w9gs702@sneakemail.com> against the 1.2.0 tag. See the various
"Takeover" threads on the dev mailing list.
* build.conf
(test-scripts): Add checkout_tests.py.
* subversion/include/svn_client.h
(svn_client_checkout3, svn_client_update3,
svn_client_switch2): New.
(svn_client_checkout2, svn_client_update2,
svn_client_switch): Deprecate.
* subversion/include/svn_wc.h
(svn_wc_notify_action_t): New 'Exists' action.
(svn_wc_get_update_editor3, svn_wc_text_modified_p2,
svn_wc_get_switch_editor3): New.
(svn_wc_get_update_editor2, svn_wc_text_modified_p,
svn_wc_get_switch_editor2): Deprecate.
* subversion/libsvn_client/checkout.c
(svn_client__checkout_internal): New argument. Update calls to
svn_client__update_internal.
(svn_client_checkout3): New.
(svn_client_checkout2, svn_client_checkout): Update calls to
svn_client__checkout_internal.
* subversion/libsvn_client/client.h
(svn_client__update_internal, svn_client__checkout_internal,
svn_client__switch_internal): New bool argument to identify updates,
checkouts, and switches, respectively, made with --force option.
* subversion/libsvn_client/copy.c
(repos_to_wc_copy): Update call to svn_client__checkout_internal.
* subversion/libsvn_client/externals.c
(switch_external): Update calls to svn_client__checkout_internal,
svn_client__update_internal, and svn_client__switch_internal.
(handle_external_item_change): Update call to
svn_client__checkout_internal.
* subversion/libsvn_client/switch.c
(svn_client__switch_internal): New argument. Replace call to
svn_wc_get_switch_editor2 with svn_wc_get_switch_editor3.
(svn_client_switch2): New.
(svn_client_switch): Update call to svn_client__switch_internal.
* subversion/libsvn_client/update.c
(svn_client__update_internal): New argument. Replace call to
svn_wc_get_update_editor2 with svn_wc_get_update_editor3.
(svn_client_update3): New.
(svn_client_update2): Reimplement as wrapper around
svn_client_update3.
(svn_client_update): Update call to svn_client__update_internal.
* subversion/libsvn_wc/adm_ops.c
(revert_admin_things): Update call to
svn_wc__text_modified_internal_p.
* subversion/libsvn_client/commit_util.c (harvest_committables):
* subversion/libsvn_wc/diff.c (file_diff, close_file):
* subversion/libsvn_wc/log.c (svn_wc_cleanup2):
* subversion/libsvn_wc/status.c (assemble_status):
Replace calls to svn_wc_text_modified_p with svn_wc_text_modified_p2.
* subversion/libsvn_wc/props.c
(svn_wc_prop_set2): Update comment.
* subversion/libsvn_wc/questions.c
(svn_wc__text_modified_internal_p): New arg. Update call to
svn_wc__text_base_path.
(svn_wc_text_modified_p2): New. Update comment.
(svn_wc_text_modified_p): Reimplement as wrapper around
svn_wc_text_modified_p2
* subversion/libsvn_wc/update_editor.c
(struct edit_baton): New member allow_unver_obstructions.
(struct file_baton): New member existed.
(make_file_baton): Initialize new file_baton member.
(add_directory): Allow obstructing directories if edit baton
permits them. Support notification for adding directories which
already 'E'xist.
(add_or_open_file): Allow obstructing files if edit baton
permits them.
(change_file_prop): Cache the last-changed-date propval for
obstructing files.
(merge_file): New argument to identify when obstructions are
allowed. Handle the various obstruction scenarios.
(close_file): Update call to merge_file. Support notification for
adding files which already 'E'xist.
(make_editor): New argument to identify when obstructions are
allowed. Initialize new edit baton member.
(svn_wc_get_update_editor3) New.
(svn_wc_get_update_editor, svn_wc_get_update_editor2): Reimplement
as wrappers around svn_wc_get_update_editor3.
(svn_wc_get_switch_editor3): New.
(svn_wc_get_switch_editor, svn_wc_get_switch_editor2): Reimplement
as wrappers around svn_wc_get_switch_editor3.
* subversion/libsvn_wc/wc.h
(svn_wc__text_modified_internal_p): New arg to select comparison
against normal text base or temporary text base.
* subversion/svn/checkout-cmd.c
(svn_cl__checkout): Replace call to svn_wc_get_update_editor2 with
svn_wc_get_update_editor3.
* subversion/svn/main.c
(svn_cl__cmd_table): Enable --force option with svn co, up, and sw.
Add new help text for each.
* subversion/svn/notify.c
(notify): Handle new 'Existed' action.
* subversion/svn/switch-cmd.c
(svn_cl__switch): Replace call to svn_client_switch with
svn_client_switch2.
* subversion/svn/update-cmd.c
(svn_cl__update): Replace call to svn_client_update2 with
svn_client_update3.
* subversion/tests/cmdline/checkout_tests.py: New.
* subversion/tests/cmdline/getopt_tests_data/svn_help_log_switch_stdout
Update to reflect new svn switch help text.
* subversion/tests/cmdline/svntest/actions.py
(run_and_verify_checkout): Support optional arguments. Don't delete
WC target during forced checkouts.
* subversion/tests/cmdline/svntest/main.py
(run_and_verify_switch): Support optional arguments and regular
expression error checking.
* subversion/tests/cmdline/svntest/tree.py
(build_tree_from_checkout): Enable parsing of 'E' notification.
* subversion/tests/cmdline/switch_tests.py
(forced_switch, forced_switch_failures): New test definitions.
(test_list): Run new tests.
* subversion/tests/cmdline/update_tests.py
(forced_update, forced_update_failures): New test definitions.
(test_list): Run new tests.
]]]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jun 29 22:16:15 2006