Re: svn commit: r1815799 - in /subversion/trunk/subversion: libsvn_ra_serf/commit.c tests/libsvn_ra/ra-test.c

Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com> writes:

> (2) I am going to tweak the new test so that it would properly open the
> parent directory and commit to a locked file, to have this case
> covered with a native test.

Committed in r1816060.

Julian Foad <julianfoad_at_apache.org> writes:

>> (1) Keep the new simpler check in maybe_set_lock_token_header() — as,
>> unless I missing something, there should be no reason to explicitly
>> filter empty relpaths for the lock tokens since they are invalid.
>
> I agree it is good to remove the '*relpath &&' condition if there is no
> reason why it needs to be there.
>
> Don't replace it with 'relpath &&' instead, however. If relpath is null then
> I think the next line (svn_hash_gets(..., relpath)) would immediately crash
> anyway, so allowing it here is useless and therefore confusing. Remove that
> condition entirely. That's my suggestion.

The condition is inverted in the sense that it checks for a null relpath
and returns if so — in other words, the svn_hash_gets() won't get called
if the relpath is null.

However, as it turns out, this condition can indeed be simplified by not
checking for null, as the only calling site where the relpath might be
null, setup_proppatch_headers(), already checks for it. (Among the other
two calling sites, the relpath cannot be null as it would have segfaulted
earlier).

I committed this simplification in r1816061, thanks!

Regards,
Evgeny Kotkov
Received on 2017-11-22 16:23:34 CET