[webkit-reviews] review requested: [Bug 28675] Make prepare-ChangeLog notice property changes : [Attachment 38604] Make prepare-ChangeLog work as described, v2

[bugzilla-daemon at webkit.org]

Cameron McCormack <cam at mcc.id.au> has asked  for review:
Bug 28675: Make prepare-ChangeLog notice property changes
https://bugs.webkit.org/show_bug.cgi?id=28675

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

------- Additional Comments from Cameron McCormack <cam at mcc.id.au>
(In reply to comment #4)
> > +	 open INFO, "$SVN diff '$file' |" or die;
> 
> Please use "DIFF" for the file handle name instead of "INFO" here.

Done.
 
> > -	     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.

Solved this by ensuring that $propertyStatus is always set if $status is set,
and by changing it to a "next if" statement.

> > +sub propertyChangeDescription($)
> > ... 
> 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.

OK I've made it output all changed properties.

It still only describes the property changes if there are no changes to the
file contents, though.	Do you think that's the preferred thing to do?	Or
should it always list the property changes?  One thing that looks confusing is
if the file was modified and properties were changed.  Since the description
for a file modification is empty, the description of the property changes by
itself makes it look like that's the only change.