[Webkit-unassigned] [Bug 39624] --squash should go away and become the default

Mon Jun 14 12:32:52 PDT 2010

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

--- Comment #28 from Ojan Vafai <ojan at chromium.org>  2010-06-14 12:32:52 PST ---
Thanks for the feedback.

(In reply to comment #26)
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:101
>  +          self._user = User()
> Um... Do we really want SCM to be able to interact with the user?  I'm wondering if this is a layering violation.

Not sure. SVN already uses User.

The only way I can think to address this is to raise a custom Exception, e.g. AmbiguousCommitError, or something. Then the calling code can catch that and prompt. Then, the calling code can call commit_with_message again with an extra bool (e.g. force_commit) that will skip the error.

I made that change here and for the SVN. It is definitely cleaner in terms of separation of layers. I'm not a huge fan of using Exceptions for control flow though.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:580
>  +          return """There are %s local commits%s. Everything will be committed as a single commit. To avoid this prompt, set "git config webkit-patch.squash true". Continue?""" % (num_local_commits, working_directory_message)
> This is hard to read as one line. 

Fixed.
> 
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:583
>  +          squash = Git.read_git_config('webkit-patch.squash')
> Should this be webkit-patch.commit_should_always_squash instead?  Does .squash mean multiple things?  This commit behavior seems possibly destructive where as other meanings of .squash could be less so.

Changed it. squash just means one thing afaik.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:599
>  +          if not self.commit_should_always_squash():
> Seems this could be its own function. :)

Good point. Done.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:604
>  +                  raise ScriptError(message="Did not commit")
> Confused.

Added a comment explaining this.

> WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py:57
>  +          if self._tool.scm().support_local_commits():
> Does this need to check _options.git_commit first?

No. This returns the correct value for squashing if git_commit is None.

> Big change for my little brain... :(

Sorry, couldn't think of how to make it smaller. I could make it less than half the size by leaving in the squash arguments but making them noops, then removing them in a followup patch, but that seems silly.

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