Re: Access Violation with 'svn status -u'

Philip Martin <philip.martin_at_wandisco.com> writes:

> Philip Martin <philip.martin_at_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