Bug 16052 - prepare-ChangeLog doesn't report deleted files
Summary: prepare-ChangeLog doesn't report deleted files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-11-19 10:30 PST by Kevin Ollivier
Modified: 2007-11-25 23:07 PST (History)
2 users (show)

See Also:


Attachments
Patch to get prepare-ChangeLog to report deleted files (1.33 KB, patch)
2007-11-19 10:30 PST, Kevin Ollivier
ddkilzer: review-
Details | Formatted Diff | Diff
Patch v2 (1.14 KB, patch)
2007-11-25 22:36 PST, David Kilzer (:ddkilzer)
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Ollivier 2007-11-19 10:30:09 PST
prepare-ChangeLog currently only reports added or modified files, so any deleted files won't be in the ChangeLog unless manually added.
Comment 1 Kevin Ollivier 2007-11-19 10:30:43 PST
Created attachment 17402 [details]
Patch to get prepare-ChangeLog to report deleted files
Comment 2 David Kilzer (:ddkilzer) 2007-11-19 22:43:30 PST
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?
Comment 3 David Kilzer (:ddkilzer) 2007-11-19 22:44:12 PST
Confirming issue with git and changes staged to index.

Comment 4 Kevin Ollivier 2007-11-19 23:01:25 PST
(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?
Comment 5 Adam Roben (:aroben) 2007-11-19 23:12:46 PST
(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.
Comment 6 David Kilzer (:ddkilzer) 2007-11-19 23:33:56 PST
(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.
Comment 7 David Kilzer (:ddkilzer) 2007-11-19 23:38:25 PST
(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

Comment 8 David Kilzer (:ddkilzer) 2007-11-19 23:39:56 PST
(In reply to comment #6)
> if (!@changed_files && !@conflict_files && !scalar keys %function_lists) {

Or just:

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

Comment 9 David Kilzer (:ddkilzer) 2007-11-25 22:36:45 PST
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.
Comment 10 David Kilzer (:ddkilzer) 2007-11-25 23:07:11 PST
Committed revision 28037.