[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 #61 from Chris Jerdonek <cjerdonek at webkit.org>  2010-04-27 01:04:22 PST ---
Thanks for the patch, Ojan.  I noticed a couple things in it, though, that I
was hoping you could address in a subsequent patch.

> +        # Set logging level to avoid rietveld's logging spew.
> +        old_level_name = logging.getLogger().getEffectiveLevel()
> +        logging.getLogger().setLevel(logging.ERROR)
> +
> +        # Use RealMain instead of calling upload from the commandline so that
> +        # we can pass in the diff ourselves. Otherwise, upload will just use
> +        # git diff for git checkouts, which doesn't respect --squash and --git-commit.
> +        issue, patchset = upload.RealMain(args[1:], data=diff)
> +
> +        # Reset logging level to the original value.
> +        logging.getLogger().setLevel(old_level_name)

It is probably better to throttle the rietveld logging from the calling code
rather than by temporarily changing it from within.  Otherwise, it doesn't seem
like there is a way for the caller to, say, temporarily re-enable this logging.

You may wish to look at the pattern we're using to disable autoinstall logging
from the calling code while running test-webkitpy:

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/test-webkitpy?rev=56981#L87


> -        # FIXME: Add unit tests.
> -        if git_commit and '..' in git_commit:
> -            # FIXME: If the range is a "...", the code should find the common
> -            #        ancestor and start there.  See git diff --help for how
> -            #        "..." usually works.
> -            self._parse_error('invalid --git-commit option: option does '
> -                              'not support ranges "..": %s' % git_commit)

Above you deleted the FIXME to add more unit tests, but below you removed some
unit tests without adding any new ones.  It seems like you should at least
preserve the FIXME above to add more unit tests.

> +++ b/WebKitTools/Scripts/webkitpy/style/optparser_unittest.py
> @@ -114,10 +114,6 @@ class ArgumentParserTest(LoggingTestCase):

> -        self.assertRaises(SystemExit, parse, ['--git-diff=aa..bb'])
> -        self.assertLog(['ERROR: invalid --git-commit option: '
> -                        'option does not support ranges "..": aa..bb\n'])
> -

> -        (files, options) = parse(['--git-since=commit'])
> -        self.assertEquals(options.git_commit, 'commit')

Would it be possible for you to create optparser unit tests for the new options
you are adding (and to update the existing tests to take into account the new
options)?  Up to this point, we have maintained pretty good code coverage of
the option parsing code.  Thanks.

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