subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <danie...@apache.org>
Subject Re: [PATCH] A test for "Can't get entries" error
Date Tue, 20 Nov 2018 07:29:05 GMT
Dmitry Pavlenko wrote on Mon, Nov 19, 2018 at 17:05:16 +0300:
> Hello Subversion community!
> I've run into an error: when performing 2 specially constructed updates one after another
> within the same session, SVN fails with 
> 
>   $ ./ra-test 15
>   svn_tests: E160016: Can't get entries of non-directory
>   XFAIL: ra-test 15: check that there's no "Can't get entries" error
> 

That error code is SVN_ERR_FS_NOT_DIRECTORY.

> error. I believe these updates constructed that way are valid, so the problem is
> somewhere in FSFS code. It's also interesting that if these updates are run
> separately (e.g. by adding "if (FALSE)" to one or another), they succeed.
> 

Could you please clarify whether the bug reproduces under other backends (FSX and BDB)?

> +++ subversion/tests/libsvn_ra/ra-test.c	(working copy)
> @@ -1784,7 +1784,113 @@ commit_locked_file(const svn_test_opts_t *opts, ap
> +cant_get_entries_of_non_directory(const svn_test_opts_t *opts, apr_pool_t *pool)
> +{
> +  svn_ra_session_t *session;
> +  const svn_delta_editor_t *editor;
> +  void *edit_baton;
> +  const svn_ra_reporter3_t *reporter;
> +  void *report_baton;
>  
> +  SVN_ERR(make_and_open_repos(&session,
> +                              "cant_get_entries_of_non_directory", opts,
> +                              pool));
> +  
> +  {
> +    SVN_ERR(svn_ra_get_commit_editor3(session, &editor, &edit_baton,
> +                                      apr_hash_make(pool), NULL,
> +                                      NULL, NULL, FALSE, pool));
> +
> +    SVN_ERR(editor->open_root(edit_baton, 0, pool, &root_baton));
> +  }

You make all commits using the same EDITOR.  Is that allowed?  Should
you make the 'editor' variable block-scoped and call svn_ra_get_commit_editor3()
anew in each block?

> +  {
> +    SVN_ERR(editor->open_root(edit_baton, 1, pool, &root_baton));
⋮
> +  }

> +  {
> +    SVN_ERR(svn_ra_do_update3(session, &reporter, &report_baton,
> +                            3, "", svn_depth_infinity, TRUE, FALSE,
> +                            svn_delta_default_editor(pool), NULL,
> +                            pool, pool));
> +    SVN_ERR(reporter->set_path(report_baton, "", 3, svn_depth_infinity, FALSE,
> +                               NULL, pool));
> +    SVN_ERR(reporter->set_path(report_baton, "B", 2, svn_depth_infinity, FALSE,
> +                               NULL, pool));
> +    SVN_ERR(reporter->finish_report(report_baton, pool));
> +    
> +    
> +  }
> +  {
> +    SVN_ERR(svn_ra_do_update3(session, &reporter, &report_baton,
> +                            4, "", svn_depth_infinity, TRUE, FALSE,
> +                            svn_delta_default_editor(pool), NULL,
> +                            pool, pool));
> +    SVN_ERR(reporter->set_path(report_baton, "", 4, svn_depth_infinity, FALSE,
> +                               NULL, pool));
> +    SVN_ERR(reporter->set_path(report_baton, "B", 3, svn_depth_infinity, FALSE,
> +                               NULL, pool));
> +    SVN_ERR(reporter->finish_report(report_baton, pool));
> +  }

I agree that this should work, and therefore that it's a bug.

Suggestions for further debugging:

- Further minimise the test.  Try to have fewer add_file calls, fewer
  set_path calls, etc..  (I realise you must have minimised it a lot
  already, from whatever the original instance was, but the more the
  better.)

- Try doing the two updates in the opposite order (exchange the order of
  the two blocks).

- See if the bug happens under FSX/BDB.  That would tell us where to
  look further (in libsvn_fs_fs, or in the reporter logic in
  libsvn_repos).

> +  return SVN_NO_ERROR;
> +}

Style nits:

- Per-block comments would be helpful.  They don't need to be detailed,
  but something like /* r1: Create 'A' and 'A/mu' */ would help skim the
  function quickly.

- The test name isn't very descriptive.  I think it would be better to
  name the test after what it expects to work: doing two updates in a
  single session after a file replacement by directory.

- One line exceeds 80 columns.

Cheers,

Daniel

Mime
View raw message