[webkit-reviews] review denied: [Bug 28675] Make prepare-ChangeLog notice property changes : [Attachment 38585] Make prepare-ChangeLog work as described

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Cameron McCormack
<cam at mcc.id.au>'s request for review:
Bug 28675: Make prepare-ChangeLog notice property changes
https://bugs.webkit.org/show_bug.cgi?id=28675

Attachment 38585: Make prepare-ChangeLog work as described
https://bugs.webkit.org/attachment.cgi?id=38585&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
Thanks for taking this on!  This has been a long time in coming.

> +    open INFO, "$SVN diff '$file' |" or die;

Please use "DIFF" for the file handle name instead of "INFO" here.

> -	   next unless $status;
> +	   next unless $status && !(isUnmodifiedStatus($status) &&
isUnmodifiedStatus($propertyStatus));

This logic is too negative.  Can we make it more positive?  Also, I think you
need to check $propertyStatus since there are two cases (one for svn and one
for git) where it may not be set.

    next if (!$status || isUnmodifiedStatus($status)) && (!$propertyStatus ||
isUnmodifiedStatus($propertyStatus));

This may also be fixed by setting $propertyStatus to a default of " ".	That
may work better for the code following this statement.

> +sub propertyChangeDescription($)
> +{
> +    my ($propertyChanges) = @_;
> +
> +    my @properties = keys(%$propertyChanges);
> +    my $property;
> +    my $formatString;
> +
> +    if (@properties == 1) {
> +	   my %operation = (
> +	       "A" => " Added %s property.",
> +	       "M" => " Modified %s property.",
> +	       "D" => " Removed %s property.",
> +	   );
> +
> +	   ($property) = @properties;
> +	   my $propertyChange = $propertyChanges->{$property};
> +	   $formatString = $operation{$propertyChange};
> +    } else {
> +	   $formatString = " Changed properties.";
> +    }
> +
> +    return sprintf($formatString, $property);
> +}

I would rather see all property changes listed if there are more than one. 
Please update this method to list all the property changes and just append the
formatted strings to a $result string.

r- to make the above changes.

Thanks!