On 09.05.2017 15:25, Stefan Sperling wrote:
On Tue, May 09, 2017 at 01:39:49PM +0200, Stefan Sperling wrote:
On Tue, May 09, 2017 at 11:38:51AM +0200, Stefan Sperling wrote:
On Tue, Apr 18, 2017 at 12:54:20AM +0000, Daniel Shahaf wrote:
% svnadmin load r2 < dump
<<< Started new transaction, based on original revision 1
* editing path : shattered-1.pdf ... done.
* editing path : shattered-2.pdf ...svnadmin: E200014: Checksum mismatch
for '/shattered-2.pdf':
expected: 5bd9d8cabc46041579a311230539b8d1
actual: ee4aa52b139d925f8d8884402b0a750c
zsh: exit 1 svnadmin load r2 < dump
%
]]]
That's with 1.9.5. Is it fixed on trunk now? I'm not sure whether
r1785754 addresses that.
Yes, this is fixed on trunk :-)
I have now voted for and backported whatever I could to 1.9.x.
Please add your votes to the STATUS file as well so we can finally
release these fixes (or improve them, if there are still concerns).
On IRC, Branko and Johan raised concerns about the proposed backport.
The proposed backport allows files with SHA1 collisions into the repository
and avoids de-duplication of such content by the rep-cache. It fixes the
integrity problem with the rep-cache but other problems remain.
Clients will still suffer from the consequences of having such files in
the repository. The RA layer cannot de-duplicate content, the working
copy's pristine store is broken by such content, etc.
So the question is whether we want to even store such content before all
parts of the system are ready to handle it. Some users will likely keep
asking for a way to reject this content because of the ripple effects
it can cause. Many users may perceive a risk even if "only" working copies
can be rendered unusable by committing such content, because dealing with
fallout can be highly disruptive to the actual work people have to get done.
We can block such content from entering the repository with a patch such
as the one below. This check will only be done if rep-sharing is active.
But since rep-sharing is active by default this patch will work for most
of our users.
This could be further extended by the config knob to give users a choice.
I don't see a good way of adding such a knob in a patch release, though.
Perhaps that could be done for 1.10.
[Hmpf. After answering on two other threads, I now found
this one and I can toss away those other posts ...]
Yeah, I think the patch gives us the most consistent behavior
with the least added effort. And it keeps the door open for
allowing checksum collisions in the future.
So, +1 from my side. And then let's continue with the out-
standing releases.
Index: subversion/libsvn_fs_fs/transaction.c
===================================================================
--- subversion/libsvn_fs_fs/transaction.c (revision 1794541)
+++ subversion/libsvn_fs_fs/transaction.c (working copy)
@@ -2430,12 +2430,10 @@ get_shared_rep(representation_t **old_rep,
scratch_pool);
/* A mismatch should be extremely rare.
- * If it does happen, log the event and don't share the rep. */
+ * If it does happen, reject the commit. */
if (!same || err)
{
- /* SHA1 collision or worse.
- We can continue without rep-sharing, but warn.
- */
+ /* SHA1 collision or worse. */
svn_stringbuf_t *old_rep_str
= svn_fs_fs__unparse_representation(*old_rep,
ffd->format, FALSE,
@@ -2449,15 +2447,11 @@ get_shared_rep(representation_t **old_rep,
const char *checksum__str
= svn_checksum_to_cstring_display(&checksum, scratch_pool);
- err = svn_error_createf(SVN_ERR_FS_AMBIUGOUS_CHECKSUM_REP,
- err, "SHA1 of reps '%s' and '%s' "
- "matches (%s) but contents differ",
- old_rep_str->data, rep_str->data,
- checksum__str);
-
- (fs->warning)(fs->warning_baton, err);
- svn_error_clear(err);
- *old_rep = NULL;
+ return svn_error_createf(SVN_ERR_FS_AMBIUGOUS_CHECKSUM_REP,
+ err, "SHA1 of reps '%s' and '%s' "
+ "matches (%s) but contents differ",
+ old_rep_str->data, rep_str->data,
+ checksum__str);
}
/* Restore FILE's read / write position. */
I have not tested it, but this looks fine,
-- Stefan^2.
[on my way to ApacheCON]
