[Webkit-unassigned] [Bug 38852] webkit-patch land --squash commits too much if branch is not up to date

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38852


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #56294|review?                     |review+
               Flag|                            |




--- Comment #8 from Eric Seidel <eric at webkit.org>  2010-05-21 15:15:51 PST ---
(From update of attachment 56294)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.