Make prepare-ChangeLog notice property changes

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Cameron McCormack (:heycam)]

It would be good if prepare-ChangeLog would not bail if only properties have been modified.  It would be handy too if the property changes are noticed and included in the ChangeLog entry.

[Cameron McCormack (:heycam)]

Created attachment 38472 [details]
Make prepare-ChangeLog work as described

This causes prepare-ChangeLog to add ChangeLog entries like this:

  * path/to/file: Added xxx property.
  * path/to/file: Modified xxx property.
  * path/to/file: Removed xxx property.

if one property was changed on the file, and:

  * path/to/file: Changed properties.

if more than one property was changed.  These descriptions are only inserted if there is no change to the file contents itself.

Also, now if the only changes to the repository are property changes, a ChangeLog entry will still be prepared, whereas currently it will bail and say "No changes found".

[Cameron McCormack (:heycam)]

BTW I haven't tested this on a git checkout, where there should be no change in behaviour, since git doesn't have properties.

A small additional note: older versions of svn have output like this:

  Property changes on: blah
  -----------------------------
  Name: svn:executable
    + *

where it doesn't explicitly note if the property was added/modified/deleted.  The patch here falls back in this case to the description "Changed properties".

[Cameron McCormack (:heycam)]

Created attachment 38585 [details]
Make prepare-ChangeLog work as described

Sorry, just noticed there was some additional cruft left in the patch from a different bug.

[David Kilzer (:ddkilzer)]

Comment on attachment 38585 [details]
Make prepare-ChangeLog work as described

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!

[Cameron McCormack (:heycam)]

Created attachment 38604 [details]
Make prepare-ChangeLog work as described, v2

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

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> > > +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.

Hmm...I think it would be nice to list all property changes regardless of other changes to the file.  When a new file is added, I think the main entry for the file says "Added.", but all the new methods are also listed.  We could do something similar for properties.

Ultimately, the patch author has the responsibility to update the ChangeLog entry, so I think it's best to give them all the information possible (as long as the default behavior can also be used without modification).

[Cameron McCormack (:heycam)]

Created attachment 38649 [details]
Make prepare-ChangeLog work as described, v3

(In reply to comment #6)
> Hmm...I think it would be nice to list all property changes regardless of other
> changes to the file.  When a new file is added, I think the main entry for the
> file says "Added.", but all the new methods are also listed.  We could do
> something similar for properties.

I've made it always list the property changes if that information is available.  I don't think adding a separate line for each property changed is really necessary, since often it will be clear what the property change is doing.  (For example adding svn:executable really only means one thing.)

I've added some code to notice which properties have been added/deleted when doing an 'svn mv' now, too.

For example, after doing:

  $ svn mv ChangeLog-2009-06-16 ChangeLog-Old
  $ svn propset svn:executable . ChangeLog-Old
  $ touch NewFile
  $ svn add NewFile
  $ svn propset blah . NewFile
  $ svn propset myprop . Scripts/build-webkit
  $ svn propdel svn:executable Scripts/build-webkit

the ChangeLog output is:

        * ChangeLog-2009-06-16: Removed.
        * ChangeLog-Old: Copied from ChangeLog-2009-06-16. Added property svn:executable.
        * NewFile: Added with property blah.
        * Scripts/build-webkit: Added property myprop. Removed property svn:executable.

> Ultimately, the patch author has the responsibility to update the ChangeLog
> entry, so I think it's best to give them all the information possible (as long
> as the default behavior can also be used without modification).

OK.  Is there a need for me to wrap the generated lines?

[David Kilzer (:ddkilzer)]

Comment on attachment 38649 [details]
Make prepare-ChangeLog work as described, v3

Wow!  r=me

[Cameron McCormack (:heycam)]

Thanks!  I'd advise someone to check that it doesn't do anything stupid when running in a git tree, though.

[Eric Seidel (no email)]

Comment on attachment 38649 [details]
Make prepare-ChangeLog work as described, v3

Clearing flags on attachment: 38649

Committed r47810: <http://trac.webkit.org/changeset/47810>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.