Bug 13732 – prepare-ChangeLog should work with git

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Simon Hausmann]

When using git-svn to check out WebKit the prepare-ChangeLog script does not work anymore. The attached patch tries to distinguish the presence of a svn checkout or a git repository and calls git diff in the latter case.

[Simon Hausmann]

Created attachment 14567 [details]
patch to make prepare-ChangeLog work with git-svn

[Adam Roben (:aroben)]

Comment on attachment 14567 [details]
patch to make prepare-ChangeLog work with git-svn

I've been working on this as well. I think this patch will have problems when running prepare-ChangeLog from within a subdirectory. I'm going to upload my patch that does what this one does and more.

[Adam Roben (:aroben)]

Created attachment 14568 [details]
More complete git support for prepare-ChangeLog

[Simon Hausmann]

Great, your patch does indeed more and looks a lot cleaner. I'm all for it, anything that makes working with git and webkit easier :)

[David Kilzer (:ddkilzer)]

Comment on attachment 14568 [details]
More complete git support for prepare-ChangeLog

Overall this patch looks great!  I assume you've been living on it for a while, Adam?

A couple comments:

- Shouldn't %svn in isModifiedOrAddedStatus() also contain "R" for replaced files?

- Using diffHeaderFormat() within loops (when it returns the same value each time) and redefining local data structures for each method call (like in isModifiedOrAddedStatus() and isConflictStatus() and statusDescription()) makes me cringe, but unless there is an apparent slow down, it's nice not to have to declare global variables that are farther away from the subroutines.

- I understand the reason for declaring $isSVN, $isGit and $gitRoot next to their methods, but I'm torn about not having them at the top of the script, too.  They're okay, though.

r=me if you address "R" in %svn in isModifiedOrAddedStatus().

[Adam Roben (:aroben)]

(In reply to comment #5)
> (From update of attachment 14568 [details] [edit])
> Overall this patch looks great!  I assume you've been living on it for a while,
> Adam?

   Yep, and it seems to be working nicely, modulo a few FIXMEs in the script.

> A couple comments:
> 
> - Shouldn't %svn in isModifiedOrAddedStatus() also contain "R" for replaced
> files?

   Yes. Fixed.

> - Using diffHeaderFormat() within loops (when it returns the same value each
> time) and redefining local data structures for each method call (like in
> isModifiedOrAddedStatus() and isConflictStatus() and statusDescription()) makes
> me cringe, but unless there is an apparent slow down, it's nice not to have to
> declare global variables that are farther away from the subroutines.

   I'd guess that, as you said, these won't slow things down much.

   Thanks for the review!

[Adam Roben (:aroben)]

Committed r21501

[David Kilzer (:ddkilzer)]

*** Bug 13884 has been marked as a duplicate of this bug. ***

[Simon Hausmann]

Why is 13884 a duplicate of this bug? This bug report was about the initial git support for prepare-ChangeLog, bug 13884 is about extending it to operate on a given git commit instead of just changes in the working directory.