[webkit-reviews] review granted: [Bug 38852] webkit-patch land --squash commits too much if branch is not up to date : [Attachment 56294] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri May 21 15:15:50 PDT 2010
Eric Seidel <eric at webkit.org> has granted Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 38852: webkit-patch land --squash commits too much if branch is not up to
date
https://bugs.webkit.org/show_bug.cgi?id=38852
Attachment 56294: Patch
https://bugs.webkit.org/attachment.cgi?id=56294&action=review
------- Additional Comments from Eric Seidel <eric at webkit.org>
I would have put these 3 lines in their own function:
596 config_squash = Git.read_git_config('webkit-patch.squash')
597 if (config_squash and config_squash is not ""):
598 squash = config_squash.lower() == "true"
Then the other code doesn't have to know about the empty value meaning false or
true or whatever.
603 raise
ScriptError(message=self._get_squash_error_message(num_local_commits))
ScriptErorr has historically meant "there was an error running the script".
Used by run_command. I think you want some other type of exception here.
Maybe not? I know our infrstrcture is set up to handle ScriptERrors well,
maybe tthat's what your'e going for here?
I still think distingushing false from None doesn't help us here:
595 if squash is None:
Unless "NOne" is supposed to mean true?
Is this really the way you want to do this?
def _svn_branch_has_extra_commits(self):
612 # When head contains all the commits on the svn branch,
show-branch --topics will have exactly 4 lines.
613 # One line for head, one for trunk, one for the "--" seprator and
one for the branch point on trunk.
614 trunk_commits = run_command(['git', 'show-branch', '--topics',
'head', self.svn_branch_name()])
615 return len(re.findall('\n', trunk_commits)) != 4
Seems like a strange hack. Maybe I'm mis-remembering evan's recomendation...
if he had one?
This is better than what we have, but It feels like a hack. Please consider
the above comments, especially the one about the "4 line" hack. Are we testing
the 4-line hack?
More information about the webkit-reviews
mailing list