On Wed, Jan 29, 2014 at 12:28 AM, Branko Čibej <brane_at_wandisco.com> wrote:
> Hi Stefan,
>
> I'd appreciate a review of my changes in r1562172 and the fixes in
> r1562210. Note that this is not a complete implementation, I still have to
> get (at least) dag.c and tree.c into shape; but I'd appreciate an expert
> opinion on whether the approach I chose is sane.
>
Here we go. It's all minor style / consistency findings.
Index: branches/fsfs-ucsnorm/subversion/libsvn_fs_fs/transaction.c
>
> Index: branches/fsfs-ucsnorm/subversion/libsvn_fs_fs/temp_serializer.c
> ===================================================================
> --- branches/fsfs-ucsnorm/subversion/libsvn_fs_fs/temp_serializer.c (revision
> 1562171)
> +++ branches/fsfs-ucsnorm/subversion/libsvn_fs_fs/temp_serializer.c (revision
> 1562172)
> @@ -785,16 +799,16 @@
> }
>
> /* check whether we actually found a match */
> - *found = FALSE;
> - if (lower < count)
> + if (lower >= count)
> + *found = FALSE;
>
> Since the else-case uses braces, there should be braces
in the if-case as well.
> + else
> {
> - const svn_fs_dirent_t *entry =
> - svn_temp_deserializer__ptr(entries, (const void *const
> *)&entries[lower]);
> - const char* entry_name =
> - svn_temp_deserializer__ptr(entry, (const void *const
> *)&entry->name);
> + const svn_fs_fs__dirent_t *entry =
> + svn_temp_deserializer__ptr(entries, (const void *const
> *)&entries[lower]);
> + const char* entry_key =
> + svn_temp_deserializer__ptr(entry, (const void *const
> *)&entry->key);
>
> - if (strcmp(entry_name, name) == 0)
> - *found = TRUE;
> + *found = (strcmp(entry_key, key) == 0);
> }
>
> return lower;
> Index: branches/fsfs-ucsnorm/subversion/libsvn_fs_fs/cached_data.c
> ===================================================================
> --- branches/fsfs-ucsnorm/subversion/libsvn_fs_fs/cached_data.c (revision
> 1562171)
> +++ branches/fsfs-ucsnorm/subversion/libsvn_fs_fs/cached_data.c (revision
> 1562172)
> @@ -1919,39 +1919,39 @@
> return SVN_NO_ERROR;
> }
>
> -/* Return TRUE when all svn_fs_dirent_t* in ENTRIES are already sorted
> +/* Return TRUE when all svn_fs_fs__dirent_t* in ENTRIES are already sorted
> by their respective name. */
>
> s/name/key/
> static svn_boolean_t
> sorted(apr_array_header_t *entries)
> {
> int i;
>
> - const svn_fs_dirent_t * const *dirents = (const void *)entries->elts;
> + const svn_fs_fs__dirent_t *const *dirents = (const void *)entries->elts;
> for (i = 0; i < entries->nelts-1; ++i)
> - if (strcmp(dirents[i]->name, dirents[i+1]->name) > 0)
> + if (strcmp(dirents[i]->key, dirents[i+1]->key) > 0)
> return FALSE;
>
> return TRUE;
> }
>
> -/* Compare the names of the two dirents given in **A and **B. */
> +/* Compare the keys of the two dirents given in **A and **B. */
> static int
> compare_dirents(const void *a, const void *b)
> {
> - const svn_fs_dirent_t *lhs = *((const svn_fs_dirent_t * const *) a);
> - const svn_fs_dirent_t *rhs = *((const svn_fs_dirent_t * const *) b);
> + const svn_fs_fs__dirent_t *lhs = *((const svn_fs_fs__dirent_t * const
> *) a);
> + const svn_fs_fs__dirent_t *rhs = *((const svn_fs_fs__dirent_t * const
> *) b);
>
> - return strcmp(lhs->name, rhs->name);
> + return strcmp(lhs->key, rhs->key);
> }
>
> -/* Compare the name of the dirents given in **A with the C string in *B.
> */
> +/* Compare the key of the dirents given in **A with the C string in *B. */
> static int
> compare_dirent_name(const void *a, const void *b)
>
> Should be renamed to compare_dirent_key.
> {
> - const svn_fs_dirent_t *lhs = *((const svn_fs_dirent_t * const *) a);
> + const svn_fs_fs__dirent_t *lhs = *((const svn_fs_fs__dirent_t * const
> *) a);
> const char *rhs = b;
>
> - return strcmp(lhs->name, rhs);
> + return strcmp(lhs->key, rhs);
> }
>
> /* Into ENTRIES, read all directories entries from the key-value text in
> @@ -2191,18 +2196,18 @@
> return SVN_NO_ERROR;
> }
>
> -svn_fs_dirent_t *
> +svn_fs_fs__dirent_t *
> svn_fs_fs__find_dir_entry(apr_array_header_t *entries,
> const char *name,
>
> s/name/key/ . But you already added the corresponding TODO
in the header file.
> int *hint)
> {
> - svn_fs_dirent_t **result
> + svn_fs_fs__dirent_t **result
> = svn_sort__array_lookup(entries, name, hint, compare_dirent_name);
> return result ? *result : NULL;
> }
>
> svn_error_t *
> -svn_fs_fs__rep_contents_dir_entry(svn_fs_dirent_t **dirent,
> +svn_fs_fs__rep_contents_dir_entry(svn_fs_fs__dirent_t **dirent,
> svn_fs_t *fs,
> node_revision_t *noderev,
> const char *name,
>
> Same here.
> Index: branches/fsfs-ucsnorm/subversion/libsvn_fs_fs/fs_fs.h
> ===================================================================
> --- branches/fsfs-ucsnorm/subversion/libsvn_fs_fs/fs_fs.h (revision
> 1562171)
> +++ branches/fsfs-ucsnorm/subversion/libsvn_fs_fs/fs_fs.h (revision
> 1562172)
> @@ -212,4 +212,8 @@
> void
> svn_fs_fs__reset_txn_caches(svn_fs_t *fs);
>
> -#endif
> +/* Set *NORMSTR to a normalized form of STR, allocated from POOL. */
> +svn_error_t *
> +svn_fs_fs__normalize(const char **normstr, const char *str, apr_pool_t
> *pool);
> +
> +#endif /* SVN_LIBSVN_FS__FS_FS_H */
>
> Since the implementation is in utils.c, consider moving this
declaration to utils.h.
-- Stefan^2.
Received on 2014-02-01 11:36:40 CET