Philip Martin <philip.mar...@wandisco.com> writes:
> Philip Martin <philip.mar...@wandisco.com> writes:
>
>> svnadmin create repo
>> svn import -mm repo/format file://$PWD/repo/A/f
>> svn co -r0 file://$PWD/repo wc
>> svn mkdir wc/A
>> svn st -u wc
>>
>> That's obviously a bug. It's crashing in make_file_baton:
>>
>> f->repos_relpath = svn_relpath_join(find_dir_repos_relpath(pb, pool),
>> f->name, pool);
>>
>> when find_dir_repos_relpath returns NULL. We could set f->repos_relpath
>> to NULL, which matched what happens for the directory baton for A, but
>> is that correct? find_dir_repos_relpath is returning NULL because the
>> "obstructing" A is not versioned, although there is an A in the
>> repository. I'm not sure what repos_relpath is supposed to represent
>> here. Is it the repository path associated with the name in the
>> repository, or the repository path associated with the node in wc.db?
>
> There is a commment in find_dir_repos_relpath:
>
> /* Note that status->repos_relpath could be NULL in the case of a
> missing
> * directory, which means we need to recurse up another level to get
> * a useful relpath. */
> if (status)
> return status->repos_relpath;
>
>
> but the code doesn't recurse. Should that if be changed to
>
> if (status && status->repos_relpath)
> return status->repos_relpath;
>
> If we do that does that mean that find_dir_repos_relpath can never
> return NULL as it always recurses up to some non-NULL path? Should we
> remove the "return NULL?
I don't know whether repos_relpath should be allowed to be NULL or not.
I can fix it either way and all the regression tests, including the
javahl tests, pass.
I could fix it like this, allowing repos_relpath to be NULL:
Index: subversion/tests/cmdline/stat_tests.py
===================================================================
--- subversion/tests/cmdline/stat_tests.py (revision 1229514)
+++ subversion/tests/cmdline/stat_tests.py (working copy)
@@ -117,6 +117,12 @@ def status_update_with_nested_adds(sbox):
svntest.actions.run_and_verify_unquiet_status(wc_backup,
expected_status)
+ # At one time an obstructing 'newdir' caused a SEGV on 'newdir/newfile'
+ os.makedirs(os.path.join(wc_backup, 'newdir'))
+ expected_status.tweak('newdir', status='? ')
+ svntest.actions.run_and_verify_unquiet_status(wc_backup,
+ expected_status)
+
#----------------------------------------------------------------------
# svn status -vN should include all entries in a directory
Index: subversion/libsvn_wc/status.c
===================================================================
--- subversion/libsvn_wc/status.c (revision 1229514)
+++ subversion/libsvn_wc/status.c (working copy)
@@ -1795,6 +1795,7 @@ make_file_baton(struct dir_baton *parent_dir_baton
struct dir_baton *pb = parent_dir_baton;
struct edit_baton *eb = pb->edit_baton;
struct file_baton *f = apr_pcalloc(pool, sizeof(*f));
+ const char *dir_repos_relpath = find_dir_repos_relpath(pb, pool);
/* Finish populating the baton members. */
f->local_abspath = svn_dirent_join(eb->anchor_abspath, path, pool);
@@ -1804,8 +1805,9 @@ make_file_baton(struct dir_baton *parent_dir_baton
f->edit_baton = eb;
f->ood_changed_rev = SVN_INVALID_REVNUM;
f->ood_changed_date = 0;
- f->repos_relpath = svn_relpath_join(find_dir_repos_relpath(pb, pool),
- f->name, pool);
+ f->repos_relpath = (dir_repos_relpath
+ ? svn_relpath_join(dir_repos_relpath, f->name, pool)
+ : NULL);
f->ood_kind = svn_node_file;
f->ood_changed_author = NULL;
return f;
or I could fix it like this, with repos_relpath never NULL:
# svn status -vN should include all entries in a directory
Index: subversion/libsvn_wc/status.c
===================================================================
--- subversion/libsvn_wc/status.c (revision 1229514)
+++ subversion/libsvn_wc/status.c (working copy)
@@ -1649,7 +1649,7 @@ tweak_statushash(void *baton,
return SVN_NO_ERROR;
}
-/* Returns the URL for DB, or NULL: */
+/* Returns the URL for DB */
static const char *
find_dir_repos_relpath(const struct dir_baton *db, apr_pool_t *pool)
{
@@ -1666,14 +1666,11 @@ find_dir_repos_relpath(const struct dir_baton *db,
/* Note that status->repos_relpath could be NULL in the case of a missing
* directory, which means we need to recurse up another level to get
* a useful relpath. */
- if (status)
+ if (status && status->repos_relpath)
return status->repos_relpath;
repos_relpath = find_dir_repos_relpath(pb, pool);
- if (repos_relpath)
- return svn_relpath_join(repos_relpath, db->name, pool);
- else
- return NULL;
+ return svn_relpath_join(repos_relpath, db->name, pool);
}
}
@@ -2369,19 +2366,15 @@ close_file(void *file_baton,
const char *dir_repos_relpath = find_dir_repos_relpath(fb->dir_baton,
pool);
- if (dir_repos_relpath)
- {
- /* repos_lock still uses the deprecated filesystem absolute path
- format */
+ /* repos_lock still uses the deprecated filesystem absolute path
+ format */
+ const char *repos_relpath = svn_relpath_join(dir_repos_relpath,
+ fb->name, pool);
- const char *repos_relpath = svn_relpath_join(dir_repos_relpath,
- fb->name, pool);
-
- repos_lock = apr_hash_get(fb->edit_baton->wb.repos_locks,
- svn_fspath__join("/", repos_relpath,
- pool),
- APR_HASH_KEY_STRING);
- }
+ repos_lock = apr_hash_get(fb->edit_baton->wb.repos_locks,
+ svn_fspath__join("/", repos_relpath,
+ pool),
+ APR_HASH_KEY_STRING);
}
}
else
--
Philip
