(Daniel, please don't interpret any of the below as criticism: I realise
you neither wrote the original code, nor are in a position to test it.
I'm only copying you because you nominated the change for 1.3.x).
Ok, I've been trying to review the backport of r17333 for 1.3.x, and
I'm a little confused.
Firstly, justification for the backport reads:
Backport of a change to trunk which resolves buggy behavior on
(at least) Windows and Mac OS X, behavior which is inconsistent
with Linux.
Yet, as far as I can see, the new 'switch' test added on the 1.3.x-r17333
branch already passes on 1.3.x on Mac OS X (i.e., it does not require
the change to libsvn_wc/log.c that's also on the branch). So is this
actually Windows-only?
As far as the code is concerned: The problem that r17333 seems to be
fixing is to avoid calling svn_subst_copy_and_translate3() with the
same source and destination file - instead, going through a 'create temp
file / copy/translate source->temp / rename temp->source' dance to avoid
the problem.
Fair enough, but it's not at all clear how that change by itself is
intended to resolve the problem demonstrated in the regression test.
[or is it the call to svn_wc__prep_file_for_replacement() that's
important here? If so, the whole body of that function is #ifdef WIN32,
so it definitely won't have an impact on Mac OS X.]
And while we're on that function, r17333 initially added the following
lines to file_xfer_under_path():
if (translate_tgt != full_dest_path)
{
SVN_ERR (svn_wc__prep_file_for_replacement (full_dest_path, pool));
This was bogus (it didn't compile), and so immediately afterwards,
r17335 added the missing 'ignore_enoent' argument, as TRUE. Yet in the
backport branch, 'ignore_enoent' is set to FALSE. Now maybe it doesn't
make a difference here, but what was the reason for this change?
Finally, I'm not too happy with r17333 itself, in one respect (the code
has since been rewritten, so these comments do not apply to trunk): the
change creates a temporary file marked for deletion at pool cleanup time,
then renames it to a target file.
Two questions spring to mind: firstly, shouldn't we close the file
before we rename it? (doesn't this matter on Windows?); secondly, why
is the file registered for deletion when it is almost certainly going
to be renamed to something else?
The backport branch does this even more explicitly (though because it
can't register the temporary file for deletion, it won't remove the
temporary if a step fails in the meantime):
if (translate_tgt != full_dest_path)
{
SVN_ERR (svn_wc__prep_file_for_replacement (full_dest_path, FALSE,
pool));
SVN_ERR (svn_io_file_rename (translate_tgt, full_dest_path, pool));
SVN_ERR (svn_io_file_close (tmp_file, pool));
SVN_ERR (svn_io_remove_file (translate_tgt, pool));
}
... and I must be missing something, because I can't see how
svn_io_remove_file() isn't returning an error for attempting to remove
a non-existent file. (The pool cleanup function should also have done
so on trunk, but I wouldn't be surprised if we don't check that anywhere).
I'm happy to help review this, but only if someone can explain to me:
* What the operative part of this change is supposed to be. (the call
to svn_wc__prep_file_for_replacement(), clearing the read-only flag
on Windows?)
* Why the change for 'ignore_enoent' was made, and why it does/doesn't
matter.
* Why the order of calls listed above isn't simply
svn_wc__prep_file_for_replacement(target), svn_io_file_close(temp),
svn_io_file_rename(temp -> target), and why the current code doesn't
always error out on the remove.
Until then, I'm not happy that this actually works as intended.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jan 27 11:38:03 2006