[Webkit-unassigned] [Bug 36394] Include git commits in the diff for webkit-patch upload/land.

[bugzilla-daemon at webkit.org]

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





--- Comment #52 from Chris Jerdonek <cjerdonek at webkit.org>  2010-04-07 03:27:02 PST ---
(In reply to comment #46)
> (From update of attachment 52686 [details])

I'm starting to wonder if a --squash option is appropriate for checking
style.  Will running check-webkit-style with the squash option actually
change your local branch, or is it just a temporary calculation step needed
for the style check?  It doesn't seem like the act of checking style should
have as a side effect that it alters your branch.

If squashing is necessary for the webkit-patch workflow you envision, then
can the squashing simply be done before the check-webkit-style invocation?

-        if options.git_commit:
-            patch =
checkout.create_patch_since_local_commit(options.git_commit)
-        else:
-            patch = checkout.create_patch()
+        patch = checkout.create_patch(options.git_commit, options.squash)

It seems that more side effects and references to global objects are being
added to the method above that would make it harder to unit test than it
already is.  For example, the following is being added deep within the call to
create_patch()--

+            # FIXME: parameterize prompt so this can be tested.
+            response = User.prompt("There are %s local commits%s. Squash into
a single commit? [Y/n]: " % (
+                num_local_commits, working_directory_message), repeat=3)

The style code has been moving in the opposite direction by incorporating
more dependency injection to make testing easier (e.g. by adding a mock_stderr
parameter to methods where appropriate).

Would it be possible to take a flatter approach initially, say by either--

(1) requiring the user to know in advance if s/he wants to squash, and to raise
    an error if an incompatible choice is made, rather than prompting, or
(2) structuring the code so that any prompting can take place at one of
    the outer levels of the calling code?

Something else that makes this code harder to test is that there is a lot
of branching going on deep down around that prompt, which is one of the
reasons I suggest trying to flatten things out a bit:

+    def should_squash(self, squash):
+        if squash is not None:
+            # Squash is specified on the command-line.
+            return squash
+
+        if Git.squash_prompt is not None:
+            # The user already responded to the prompt below on a previous
should_squash call.
+            return Git.squash_prompt
+
+        config_squash = Git.read_git_config('webkit-patch.squash')
+        if (config_squash and config_squash is not ""):
+            return config_squash.lower() == "true"
+
+        # Only prompt if there are actually multiple commits to squash.
+        num_local_commits = len(self.local_commits())
+        if num_local_commits > 1 or num_local_commits > 0 and not
self.working_directory_is_clean():
+            working_directory_message = "" if
self.working_directory_is_clean() else " and working copy changes"
+            # FIXME: parameterize prompt so this can be tested.
+            response = User.prompt("There are %s local commits%s. Squash into
a single commit? [Y/n]: " % (
+                num_local_commits, working_directory_message), repeat=3)
+            if response == "Y":
+                Git.squash_prompt = True
+            elif response == 'n':
+                Git.squash_prompt = False
+            else:
+                raise "Must answer 'Y' or 'n'"
+
+            return Git.squash_prompt
+
+        return False

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