Bug in svn_wc__entries_write?

[Philip Martin <ph...@codematters.co.uk>]

Hello

While implementing the access baton stuff I came across this in
libsvn_wc/entries.c:svn_wc__entries_write (I added the BUG comment in
r3206)

  svn_xml_make_close_tag (&bigstr, pool, SVN_WC__ENTRIES_TOPLEVEL);

  apr_err = apr_file_write_full (outfile, bigstr->data, bigstr->len, NULL);
  if (apr_err)
    err = svn_error_createf (apr_err, 0, NULL, pool,
                             "svn_wc__entries_write: %s",
                             svn_wc_adm_access_path (adm_access));
      
  /* Close & sync. ### BUG? Why do we sync if the write failed? */
  err2 = svn_wc__close_adm_file (outfile, svn_wc_adm_access_path (adm_access),
                                 SVN_WC__ADM_ENTRIES, 1, pool);


I do not understand why we would want to sync the entries file if the
write fails, doing so would seem to produce an incomplete, or corrupt,
entries file.  Is this a bug, or am I missing something?

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Bug in svn_wc__entries_write?

[Karl Fogel <kf...@newton.ch.collab.net>]

Ben Collins-Sussman <su...@collab.net> writes:
> Philip Martin <ph...@codematters.co.uk> writes:
> 
> >   /* Close & sync. ### BUG? Why do we sync if the write failed? */
> >   err2 = svn_wc__close_adm_file (outfile, svn_wc_adm_access_path (adm_access),
> >                                  SVN_WC__ADM_ENTRIES, 1, pool);
> > 
> > 
> > I do not understand why we would want to sync the entries file if the
> > write fails, doing so would seem to produce an incomplete, or corrupt,
> > entries file.  Is this a bug, or am I missing something?
> 
> Sure looks like a bug to me.  We should call close_adm_file() without
> syncing, and if the operation succeeds, use that other wc function to
> do the sync manually.

I think what Philip is saying is that if the previous write (not
quoted above) failed, then we shouldn't rename the tmp entries file to
the real entries file -- i.e., what close_adm_file() with the sync
flag does.  So the issue isn't the return value of close_adm_file(),
but that of the apr_file_write_full() call immediately before.

I'll patch it up now; Philip deserves a rest :-).

-K

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Bug in svn_wc__entries_write?

[Ben Collins-Sussman <su...@collab.net>]

Philip Martin <ph...@codematters.co.uk> writes:

>   /* Close & sync. ### BUG? Why do we sync if the write failed? */
>   err2 = svn_wc__close_adm_file (outfile, svn_wc_adm_access_path (adm_access),
>                                  SVN_WC__ADM_ENTRIES, 1, pool);
> 
> 
> I do not understand why we would want to sync the entries file if the
> write fails, doing so would seem to produce an incomplete, or corrupt,
> entries file.  Is this a bug, or am I missing something?

Sure looks like a bug to me.  We should call close_adm_file() without
syncing, and if the operation succeeds, use that other wc function to
do the sync manually.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org