772359 - LogActivityEntry shouldn't remove any characters from the field when splitting long lines; and delimiters need to be inserted when joining split entries

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[:glob ✱]

Attachment: patch v1

if LogActivityEntry has to split a long line, resulting in multiple rows inserted into bugs_activity for a single event, any leading commas or whitespace are removed via:

> $removed =~ s/^[,\s]+//; # remove any comma or space

now we are reassembling these split lines (bug 169752), it's clear that what we have in the table doesn't match what was actually changed.

Comment 1

[:glob ✱]

Comment on attachment 640529 [details] [diff] [review]
patch v1

while backporting bug 169752 to bmo/4.2, an issue was found with how the split items are joined.

i'll morph this bug to encompass a fix for that too.

Comment 2

[:glob ✱]

Attachment: patch v1

Comment 3

[Frédéric Buclin]

Wow, I just did a mass-change, adding several bugs at once to the dependency tree of another bug, and what I got in the bug history was:

208919641996221522472044

With this patch applied, I can now read:

2089, 1964, 1996, 2215, 2247, 2044

Much better! :) This also means that this is a blocker.

Comment 4

[Frédéric Buclin]

Comment on attachment 642592 [details] [diff] [review]
patch v1

>+    if ($current_change eq '') {
>+        return $new_change;
>+    }

Nit: could be written on a single line: return $new_change if $current_change eq '';


>+    if ($field eq 'dependson' || $field eq 'blocked' || $field eq 'see_also') {
>+        if (substr($new_change, 0, 1) eq ',') {

Now that leading commas and spaces are no longer removed, I would like the return point in find_wrap_point() to be right-shifted by one, i.e. return ++$wrappoint instead of $wrappoint (and the hack for hyphens be removed), because we all learnt at school that a comma or a hyphen remains at the end of the previous line and is never the first character of the next line. This may look unimportant, but it makes total sense to me (call me a purist if you want). Also, this change also has the advantage to fix a problem I will mention below.


>+    # All other fields get a space
>+    if (substr($new_change, 0, 1) eq ' ') {

This comment is wrong for at least bug summaries. If a bug summary is truncated on a comma or a hyphen, you don't want to add a whitespace right before the comma, respectively right after the hyphen. With the change I proposed above about the return point of find_wrap_point(), you should rather look at the last character of the previous line, and if it's a comma, append a whitespace, else append nothing (i.e. for a hyphen, a whitespace, or any another character). We cannot change the logic once this patch is committed, so we must make it right the first time.


Otherwise looks good.

Comment 5

[:glob ✱]

(In reply to Frédéric Buclin from comment #4)
> Now that leading commas and spaces are no longer removed, I would like the
> return point in find_wrap_point() to be right-shifted by one.

how do you feel about dropping find_wrap_point completely?  there's no real gain for splitting anywhere other than MAX_LINE_LENGTH as we never display the lines split anymore.

when combining the lines, if the length == MAX_LINE_LENGTH, don't add a delimiter, otherwise use the "try to figure it out" code from this patch (with the extra logic you detailed in comment 4).

Comment 6

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #5)
> how do you feel about dropping find_wrap_point completely?  there's no real
> gain for splitting anywhere other than MAX_LINE_LENGTH as we never display
> the lines split anymore.

I think blindly splitting strings at MAX_LINE_LENGTH could have some side effects, such as cutting a URL in the See Also field or a bug ID in the dependency tree into two pieces, making impossible to use them in queries anymore. Bugzilla::Search doesn't join splitted lines before looking at them, and so these lines in the bugs_activity table would never match anything anymore (think about "removed = ?", or "added = ?"), or would match when they shouldn't (e.g bug: 958762 which would be split as 958 762 if appearing near MAX_LINE_LENGTH). So I think it's still a good idea to try to split very long lines at an appropriate place if possible, such as commas or whitespaces (so when you query bugs_activity for some given word or bug ID, you are sure the word or ID is never cut).

Comment 7

[:glob ✱]

(In reply to Frédéric Buclin from comment #4)
> Now that leading commas and spaces are no longer removed, I would like the return point
> in find_wrap_point() to be right-shifted by one

i don't think changing how we split lines in this way is a good idea, as the joining code will then have to deal with lines split in two different ways.

> This comment is wrong for at least bug summaries. If a bug summary is
> truncated on a comma or a hyphen, you don't want to add a whitespace right
> before the comma, respectively right after the hyphen. With the change I
> proposed above about the return point of find_wrap_point(), you should
> rather look at the last character of the previous line, and if it's a comma,
> append a whitespace, else append nothing (i.e. for a hyphen, a whitespace,
> or any another character).

that logic will only work for items split after this fix is committed, however it won't work for any existing content which has already been split -- for lines split at whitespace, we'll be joining then again without a space.

> We cannot change the logic once this patch is committed, so we must make it right the
> first time.

this is only true for changes to the splitting code, not the joining.

Comment 8

[:glob ✱]

Attachment: patch v2

this revision fixes items being joined in the wrong order by added bugs_activity.id to the ORDER BY clause, and fixes an infinite loop in find_wrap_point when a comma/space/hyphen is at the start of a string.

Comment 9

[Frédéric Buclin]

Comment on attachment 675140 [details] [diff] [review]
patch v2

>=== modified file 'Bugzilla/Bug.pm'

>+sub _join_activity_entries {

>+    return if $current_change eq '';

Must be: return $new_change if ...



>=== modified file 'Bugzilla/Util.pm'

>     my $wrappoint = rindex($string, ",", $maxpos); # look for comma

In a follow-up bug, I think it would be a good idea to also split strings on semicolons (for long URLs).


r=LpSolit with the returned value fixed.

Comment 10

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified Bugzilla/Bug.pm
modified Bugzilla/Util.pm
Committed revision 8434.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Bug.pm
modified Bugzilla/Util.pm
Committed revision 8448.