Summary: | prepare-ChangeLog should work with git | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Hausmann <hausmann> | ||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aroben, ddkilzer | ||||||
Priority: | P2 | ||||||||
Version: | 523.x (Safari 3) | ||||||||
Hardware: | Other | ||||||||
OS: | OS X 10.4 | ||||||||
Attachments: |
|
Description
Simon Hausmann
2007-05-15 09:19:46 PDT
Created attachment 14567 [details]
patch to make prepare-ChangeLog work with git-svn
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.
Created attachment 14568 [details]
More complete git support for prepare-ChangeLog
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 :) 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().
(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! *** Bug 13884 has been marked as a duplicate of this bug. *** |