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 K, feel free to commit.
TIA,
//Peter
To unsubscribe, e-mail: dev-unsubscribe (AT) subversion (DOT) tigris.org
For additional commands, e-mail: dev-help (AT) subversion (DOT) tigris.org

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.)

*
(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:

(revision 17295)
(arbetskopia)
@@ -1701,17 +1701,153 @@
[]
+/* Write log commands to merge PRP_CHANGES into the existing properties of
+ FILE_PATH. PRP_CHANGES can contain regular properties as well as
+ entryprops and wcprops. Update *PRP_STATE (which may be NULL)

You don't update NULL, so say: "Update *PRP_STATE (unless PRP_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_REVISIN 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:
+ * PRP_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 LG_ACCUM, commands to install properties for an added DST_PATH.
+ NEW_BASE_PRPS and NEW_PRPS are base and working properties, respectively.
+ BASE_PRPS can contain entryprops and wcprops as well.
+ Use @a PL 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 (AT) subversion (DOT) tigris.org
For additional commands, e-mail: dev-help (AT) subversion (DOT) tigris.org

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 (AT) subversion (DOT) tigris.org
For additional commands, e-mail: dev-help (AT) subversion (DOT) tigris.org