You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Peter N. Lundblad" <pe...@famlundblad.se> on 2005/11/10 23:37:12 UTC
[PATCH] Refactor install_file
Hi,
I've spent the last few days trying to make install_file in
libsvn_wc/update_editor.c a bit more managable. This is a complex piece
of software in itself. Here is a patch that does som refactoring. It
passes tests (over DAV to test wcprops), of course, but I really would
appreciate some other people's review.
This could probably be refactored even more, and we know there are bugs in
install_file. I'm not trying to fix bugs in this round.
I'm not going to commit this until earliest Sunday evening (UTC+2). If
anyone wants to work in this area during the weekend and thinks the patch
is OK, feel free to commit.
TIA,
//Peter
Re: [PATCH] Refactor install_file
Posted by Ivan Zhakov <ch...@gmail.com>.
On 11/11/05, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> Hi,
>
> I've spent the last few days trying to make install_file in
> libsvn_wc/update_editor.c a bit more managable. This is a complex piece
> of software in itself. Here is a patch that does som refactoring. It
> passes tests (over DAV to test wcprops), of course, but I really would
> appreciate some other people's review.
>
> This could probably be refactored even more, and we know there are bugs in
> install_file. I'm not trying to fix bugs in this round.
>
> I'm not going to commit this until earliest Sunday evening (UTC+2). If
> anyone wants to work in this area during the weekend and thinks the patch
> is OK, feel free to commit.
I have looked to your patch and didn't find any errors. Patch looks
very good for me. Also it passes all test on Windows (only local
fsfs).
I have only one thought: what about rename install_file to merge_file?
It looks more realistic.
--
Ivan Zhakov
Re: [PATCH] Refactor install_file
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 14 Nov 2005, Julian Foad wrote:
> I'm sorry that I don't currently have the brain power to review the
> implementation, but I've commented on the log message, doc strings and function
> signatures. Style nits are in parentheses.
>
Thanks, Julain for these nits:-)
I've committed a follow-up fixing those.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Refactor install_file
Posted by Julian Foad <ju...@btopenworld.com>.
I'm sorry that I don't currently have the brain power to review the
implementation, but I've commented on the log message, doc strings and function
signatures. Style nits are in parentheses.
Peter N. Lundblad wrote:
> ------------------------------------------------------------------------
>
> Refactor that big baby of sussman (aka install_file in libsvn_wc)
> into some (hopefully) more managable functions and don't use this
> beast when adding a file with svn_wc_add_repos_file2.
>
> * subversion/libsvn_wc/props.h
> * subversion/libsvn_wc/props.c
> (svn_wc__has_magic_property): Const qualify
> an argument.
That change looks fine. (Strange early line wrapping here in the log, though.)
> * subversion/libsvn_wc/update_editor.c
> (merge_props, tweak_entry): New function, factored out of install_file.
> (install_file): Don't handle scheduleing a file for addition. Move some
Yay! A very nice improvement. (Sp.: "scheduling".)
> parts to separate functions. Dedoxygenify docstring while updating it.
> Expect new text to be in .svn/tmp and don't move it in place if it isn't.
> Don't support a full property list as input.
> (close_file): Adjust call to install_file().
> (install_added_props): New function.
> (svn_wc_add_repos_file2): Don't use install_file to install our base files
> (and do other things), because that tries to merge the working props
> and text which is both wrong and confusing. Instead, handle our own
> bussiness.
(Sp.: "business". A bit of indentation on the continuation lines would make it
easier to see which functions have been changed.)
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 17295)
> +++ subversion/libsvn_wc/update_editor.c (arbetskopia)
> @@ -1701,17 +1701,153 @@
[...]
> +/* Write log commands to merge PROP_CHANGES into the existing properties of
> + FILE_PATH. PROP_CHANGES can contain regular properties as well as
> + entryprops and wcprops. Update *PROP_STATE (which may be NULL)
You don't update NULL, so say: "Update *PROP_STATE (unless PROP_STATE is NULL)".
(The rest of this doc string and function signature is fine.)
> +tweak_entry (svn_stringbuf_t *log_accum,
(This doc string and function signature is fine.)
> /* This is the small planet. It has the complex responsibility of
> * "integrating" a new revision of a file into a working copy.
> *
> - * Given a @a file_path either already under version control, or
> - * prepared (see below) to join revision control, fully install a @a
> - * new_revision of the file; @a adm_access is an access
> - * baton with a write lock for the directory containing @a file_path.
> + * Given a FILE_PATH either already under version control, or
> + * prepared (see below) to join revision control, fully install a
(You might as well change "revision control" to "version control" to match the
previous line, while you're editing this.)
> + * NEW_REVISION of the file; ADM_ACCESS is an access baton with a
> + * write lock for the directory containing FILE_PATH.
[...]
> @@ -1720,89 +1856,70 @@
[...]
> * The caller also provides the new properties for the file in the
Careful: does it provide the "new properties" or does it provide the changes?
This line says "new", ...
> - * @a props array; if there are no new props, then caller must pass
> - * @c NULL instead. This argument is an array of @c svn_prop_t structures,
> - * and can be interpreted in one of two ways:
> + * PROP_CHANGES array; if there are no new props, then caller must pass
... this line says "CHANGES" but then says "new props" ...
> + * NULL instead. This argument is an array of svn_prop_t structures,
> + * representing differences against the files existing base properties.
... and this says "differences". Some parts of this description must be wrong.
(Sp.: "file's")
> + * (A deletion is represented by setting an svn_prop_t's 'value'
> + * field to NULL.)
(The rest of this doc string and function signature is fine.)
> +/* Write, to LOG_ACCUM, commands to install properties for an added DST_PATH.
> + NEW_BASE_PROPS and NEW_PROPS are base and working properties, respectively.
> + BASE_PROPS can contain entryprops and wcprops as well.
> + Use @a POOL for temporary allocations. */
Mention ADM_ACCESS for completeness.
> +static svn_error_t *
> +install_added_props (svn_stringbuf_t *log_accum,
> + svn_wc_adm_access_t *adm_access,
> + const char *dst_path,
> + apr_hash_t *new_base_props,
> + apr_hash_t *new_props,
> + apr_pool_t *pool)
> +{
[...]
> +}
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org