[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]

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?