RE: svn commit: r884250 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c

> -----Original Message-----
> From: Julian Foad [mailto:julian.foad_at_wandisco.com]
> Sent: maandag 21 december 2009 16:05
> To: Stefan Sperling
> Cc: Philip Martin; Mark Phippard; dev_at_subversion.apache.org
> Subject: Re: svn commit: r884250 -
> /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>
> Stefan Sperling wrote:
> > - assert(strlen(txn_id) < sizeof(txn->txn_id));
> > - strncpy(txn->txn_id, txn_id, sizeof(txn->txn_id) - 1);
> > - txn->txn_id[sizeof(txn->txn_id) - 1] = '\0';
> > + end = apr_cpystrn(txn->txn_id, txn_id, sizeof(txn->txn_id));
> > + if (end < txn->txn_id + strlen(txn_id))
> > + return svn_error_createf(SVN_ERR_FS_TXN_NAME_TOO_LONG,
> NULL,
> > + _("Transaction name '%s' was truncated"), txn_id);
>
> The "was truncated" error message is not helpful. (That sounds like the
> server is saying, "I have renamed one of your transactions".) Any reason
> not to use SVN_ERR_ASSERT?
>
> The logic looks correct.

Assertions are not nice to library users. (Not everybody uses 'svn' as his/her subversion client). An assertion/abort in release code will most likely make my users loose at least some of their work.

I don't know if this case is user and or network triggerable, but littering our code with assertions makes our code slow and easier to crash, while the error return method was designed to be more graceful.

I guess this code is server side. For this case errors are transferred to the client, while a server assertion will only be visible as 'connection closed unexpectedly' or something similar.

Maybe assertions and aborts are easier for Subversion developers, but they are not for our users.

        Bert
Received on 2009-12-21 16:24:30 CET