Bug 21261

Can you explain what problem or bug you are trying to solve with this patch?

(In reply to comment #1)
> Can you explain what problem or bug you are trying to solve with this patch?
> 
The "@todo" found above the function herself.
Are not "todo"s and "fixme"s eligible for patching?

> The "@todo" found above the function herself.
> Are not "todo"s and "fixme"s eligible for patching?

I see. Yes, this is good to fix, I just did not know what motivated the patch. Thanks.

Some comments:

* patch should remove the @todo (which will also document what the patch does)

* body of "if (len == replace_len)" should have braces around it even though it is only one line

* for the unit tests, I would suggest "starting over" rather than using the somewhat odd strings left over from the previous tests, to make them clearer.
For example start with "Hello World" and replace "Hello" with "Replacement Text" to get "Replacement Text World", or whatever. Avoiding things like "HWorldloHello Hello" where it's hard to understand.

* would recommend adding tests for start != 0, since an easy bug would be to fail to add start to len

* in the len > replace_len case, a _dbus_assert(len > replace_len) is as good at documenting as the comment, but also checks during testing, so can replace the comment with assert

> * would recommend adding tests for start != 0, since an easy bug would be to
> fail to add start to len
> 
I've missed this point: what means "test for start != 0" ?
If you refer on unit testing, the latest case added by the patch already starts from position 6 of the source string, while if you refer on addictional checks in the function I cannot understand why "start" could not be 0...

well there are three cases in the function (len == replace_len, len < replace_len, len > replace_len). I was just saying it makes sense to unit test both start==0, and start>0, for each case. Because a possible bug is to fail to offset by "start". Also useful I think is to be sure to test with the replace_at != start, to catch any confusion between those two.

Created attachment 24970 [details] [review]
Reworked patch for improved _dbus_string_replace_len()

Re-worked patch, as suggested

Looks great. Someone else will need to apply... it seems my ssh key doesn't work anymore. But I think the patch is ready.

I think this is a job for 1.5, when we've branched 1.4.

Applied to master, will be in dbus 1.5.0. Thanks!

In future it'd be useful if you could provide patches in "git format-patch" format (or as a git branch we can add as a remote), which makes them easier to merge.

Providing a commit message saying what you changed and why would also have avoided Havoc asking why you wanted this change, back in Comment #1 - see http://spheredev.org/wiki/Git_for_the_lazy#Writing_good_commit_messages for some good advice on commit messages.