[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 #54 from Ojan Vafai <ojan at chromium.org>  2010-04-20 09:52:43 PST ---
(In reply to comment #52)
> (In reply to comment #46)
> > (From update of attachment 52686 [details] [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.

The only step that actually modifies your local branch is committing. So,
upload, check-style, etc. do not.

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

Raising an error is definitely doable. The more I think about it, the more I'm
OK with this. I'll try changing it and see how it looks.

> (2) structuring the code so that any prompting can take place at one of
>     the outer levels of the calling code?

I don't see a reasonable way to do this.

> 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:

Getting rid of the prompt should make this considerably simpler.

(In reply to comment #49)
> +        squash_help = ("All diffs from the remote branch are checked."
> +                       "If excluded, prompts whether to squash when there are
> multiple commits.")
> +        parser.add_option("-s", "--squash", action="store_true",
> dest="squash", help=squash_help)
> +
> +        squash_help = ("Only working copy diffs are checked."
> +                       "If excluded, prompts whether to squash when there are
> multiple commits.")
> +        parser.add_option("--no-squash", action="store_false", dest="squash",
> help=squash_help)
> +
> Can "--squash" and "--no-squash" perhaps be combined into a single
> argument-less flag?  They are writing to the same "squash" property.  The code
> for the "--verbose" option above might help which uses a default parameter.

The issue is that it's more like there are 3 states, False, True and None. If
neither squash nor no-squash is set, then we prompt the user (or raise an error
per the above comments). If one of them is set, then we avoid the prompt and
assume the user wants that behavior. Another option is to combine it into a
single flag with an argument. It could take true, false and confirm as values,
or something like that.

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