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 with multiple
>>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.
> 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.
> Once we agree on the language I'll apply something similar to the help
> messages for sw and up.
[...]
> 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.
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.
>>> 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. :-)
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.
"some paths ... are identical": The word "path" is sometimes used to mean 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.
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}" ?
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.
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.
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!)
> + * 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.
> + *
> * 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.
> + * 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.
> 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.
> + 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.
> + {
> + /* ...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.
> +
> + /* 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".
> + 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)"?
> && (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...
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jun 28 01:51:38 2006