Bug 16052 – prepare-ChangeLog doesn't report deleted files

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kevin Ollivier]

prepare-ChangeLog currently only reports added or modified files, so any deleted files won't be in the ChangeLog unless manually added.

[Kevin Ollivier]

Created attachment 17402 [details]
Patch to get prepare-ChangeLog to report deleted files

[David Kilzer (:ddkilzer)]

Comment on attachment 17402 [details]
Patch to get prepare-ChangeLog to report deleted files

Marking r- since this is the wrong fix.

The prepare-ChangeLog script already reports removed files for svn.

I just realized that the actual bug is that prepare-ChangeLog ignores changes staged to the index for git.  For example, if you use "git rm path/to/file" instead of "rm path/to/file", the prepare-ChangeLog script ignores this change since it doesn't use the --cached switch on git-diff.

Also, if you git-add changed files before running prepare-ChangeLog, those changes won't be seen, either.

Should we be running git-diff with and without "--cached", or should we be looking at the output of git-status and parsing both of its sections?

Kevin, are you using svn or git?  Does the above explain the issue you're seeing?

[David Kilzer (:ddkilzer)]

Confirming issue with git and changes staged to index.

[Kevin Ollivier]

(In reply to comment #2)
> (From update of attachment 17402 [details] [edit])
> Marking r- since this is the wrong fix.
> 
> The prepare-ChangeLog script already reports removed files for svn.
> 
> I just realized that the actual bug is that prepare-ChangeLog ignores changes
> staged to the index for git.  For example, if you use "git rm path/to/file"
> instead of "rm path/to/file", the prepare-ChangeLog script ignores this change
> since it doesn't use the --cached switch on git-diff.
> 
> Also, if you git-add changed files before running prepare-ChangeLog, those
> changes won't be seen, either.
> 
> Should we be running git-diff with and without "--cached", or should we be
> looking at the output of git-status and parsing both of its sections?
> 
> Kevin, are you using svn or git?  Does the above explain the issue you're
> seeing?
> 

I'm using svn. I don't have git on my machine, and I removed the files with svn remove path/to/file, so I'm not sure how it could be an issue with git in my case. Are you sure it handles files removed in svn correctly? Because neither the isAdded or isModified functions check for the "D" status, and if those functions don't return true, then how is the file added to @changedFiles?

[Adam Roben (:aroben)]

(In reply to comment #2)
> Should we be running git-diff with and without "--cached", or should we be
> looking at the output of git-status and parsing both of its sections?

I think we want to stay away from git-status since it operates over the whole repository. I'm surprised there isn't a way to get git-diff to do what we want in a single invocation.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> I'm using svn. I don't have git on my machine, and I removed the files with svn
> remove path/to/file, so I'm not sure how it could be an issue with git in my
> case. Are you sure it handles files removed in svn correctly? Because neither
> the isAdded or isModified functions check for the "D" status, and if those
> functions don't return true, then how is the file added to @changedFiles?

I see the issue now.  (I'll file a separate bug on the git issue.)

If you only have deleted files in the tree, then generateFileList() returns no changes and the script exits.

1. svn rm WebKitTools/mangleme/README
2. ./WebKitTools/Scripts/prepare-ChangeLog WebKitTools

To undo these changes (for any new svn users reading this):
svn revert WebKitTools/ChangeLog  WebKitTools/mangleme/README

However, we don't want to add removed (deleted) files to @changed_files since this is used to create a list of added/changed method names for source files.

After playing with the script for a while, I think the "No changes found." test is broken.  It currently reads:

if (!@changed_files && !@conflict_files || !%function_lists) {

I think it should read:

if (!@changed_files && !@conflict_files && !scalar keys %function_lists) {

I believe that should fix the issue.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> I think we want to stay away from git-status since it operates over the whole
> repository. I'm surprised there isn't a way to get git-diff to do what we want
> in a single invocation.

(In reply to comment #6)
> I see the issue now.  (I'll file a separate bug on the git issue.)

Bug 16060

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> if (!@changed_files && !@conflict_files && !scalar keys %function_lists) {

Or just:

> if (!@changed_files && !@conflict_files && !keys %function_lists) {

[David Kilzer (:ddkilzer)]

Created attachment 17523 [details]
Patch v2

Simpler fix.

If there were only removed files, the prepare-ChangeLog script would always report no changes because the check for !%function_lists did not work as expected.  We now check for !keys %function_lists instead, and make sure there are no changed files (added, modified), no conflicted files and no removed files.

[David Kilzer (:ddkilzer)]

Committed revision 28037.