Bug 39624 – --squash should go away and become the default

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ojan Vafai]

As per the conclusions of https://lists.webkit.org/pipermail/webkit-dev/2010-April/012651.html, --git-commit=* should replace --squash and --no-squash should go away entirely.

Leaving out --git-commit means that webkit-patch should operate only on working-copy changes and "webkit-patch land" should error out if there are any local commits rather than silently committing them.

I think having upload, check-style, etc., work even if there are local commits is fine.

[Eric Seidel (no email)]

You shell will eat "*", seems like a poor choice of argument.

I guess I should go back and read the thread as to why --git-commit=* is better than --squash.

[Ojan Vafai]

Created attachment 56939 [details]
Patch

[WebKit Review Bot]

Attachment 56939 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 2
Last 3072 characters of output:
 "whitespace/braces".

  Examples: --filter=-whitespace,+whitespace/braces
            --filter=-whitespace,-runtime/printf,+runtime/printf_format
            --filter=-,+build/include_what_you_use

Paths:
  Certain style-checking behavior depends on the paths relative to
  the WebKit source root of the files being checked.  For example,
  certain types of errors may be handled differently for files in
  WebKit/gtk/webkit/ (e.g. by suppressing "readability/naming" errors
  for files in this directory).

  Consequently, if the path relative to the source root cannot be
  determined for a file being checked, then style checking may not
  work correctly for that file.  This can occur, for example, if no
  WebKit checkout can be found, or if the source root can be detected,
  but one of the files being checked lies outside the source tree.

  If a WebKit checkout can be detected and all files being checked
  are in the source tree, then all paths will automatically be
  converted to paths relative to the source root prior to checking.
  This is also useful for display purposes.

  Currently, this command can detect the source root only if the
  command is run from within a WebKit checkout (i.e. if the current
  working directory is below the root of a checkout).  In particular,
  it is not recommended to run this script from a directory outside
  a checkout.

  Running this script from a top-level WebKit source directory and
  checking only files in the source tree will ensure that all style
  checking behaves correctly -- whether or not a checkout can be
  detected.  This is because all file paths will already be relative
  to the source root and so will not need to be converted.

Options:
  -h, --help            show this help message and exit
  -f RULES, --filter-rules=RULES
                        set a filter to control what categories of style errors to report.  Specify a
                        filter using a comma-delimited list of boolean filter rules, for example "--
                        filter -whitespace,+whitespace/braces".  To display all categories and which
                        are enabled by default, pass no value (e.g. '-f ""' or '--filter=').
  -g COMMIT, --git-diff=COMMIT, --git-commit=COMMIT
                        check all changes in the given commit. Use 'commit_id..' to check all changes
                        after commmit_id, or '*' to check all changes in the local branch including
                        local commits and staged/unstages working copy changes.
  -m INT, --min-confidence=INT
                        set the minimum confidence of style errors to report.  Can be an integer 1-5,
                        with 1 displaying all errors.  Defaults to 1.
  -o FORMAT, --output-format=FORMAT
                        set the output format, which can be "emacs" or "vs7" (for Visual Studio).
                        Defaults to "emacs".
  -v, --verbose         enable verbose logging.

This script can miss errors and does not substitute for code review.

ERROR: no such option: --no-squash


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ojan Vafai]

(In reply to comment #1)
> You shell will eat "*", seems like a poor choice of argument.

Hm. Yeah, this is kind of gross. "--git-commit=*" or "-g*" work, but "--git-commit *" and "-g *" don't. Any suggestions for alternatives? We want something simple that people can remember and that won't conflict with a commit/branch name.
 
> I guess I should go back and read the thread as to why --git-commit=* is better than --squash.

It makes things simpler both in terms of using webkit-patch and in terms of the underlying code. Much of this change is deleting code! In terms of webkit-patch use, it turns three mutually exclusive command-line flags into one. I think that will be much easier for people to wrap their heads around.

[Ojan Vafai]

Comment on attachment 56939 [details]
Patch

As per discussion on #webkit and webkit-dev, I'm changing this patch to squash by default and prompt instead of erroring out in cases where squashing will actually squash multiple commits.

[Ojan Vafai]

Created attachment 56956 [details]
Patch

[Ojan Vafai]

Comment on attachment 56956 [details]
Patch

This patch lacks the git config preference to avoid prompting, but I'll add that in a followup patch.

[WebKit Review Bot]

Attachment 56956 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 2
Last 3072 characters of output:
 "whitespace/braces".

  Examples: --filter=-whitespace,+whitespace/braces
            --filter=-whitespace,-runtime/printf,+runtime/printf_format
            --filter=-,+build/include_what_you_use

Paths:
  Certain style-checking behavior depends on the paths relative to
  the WebKit source root of the files being checked.  For example,
  certain types of errors may be handled differently for files in
  WebKit/gtk/webkit/ (e.g. by suppressing "readability/naming" errors
  for files in this directory).

  Consequently, if the path relative to the source root cannot be
  determined for a file being checked, then style checking may not
  work correctly for that file.  This can occur, for example, if no
  WebKit checkout can be found, or if the source root can be detected,
  but one of the files being checked lies outside the source tree.

  If a WebKit checkout can be detected and all files being checked
  are in the source tree, then all paths will automatically be
  converted to paths relative to the source root prior to checking.
  This is also useful for display purposes.

  Currently, this command can detect the source root only if the
  command is run from within a WebKit checkout (i.e. if the current
  working directory is below the root of a checkout).  In particular,
  it is not recommended to run this script from a directory outside
  a checkout.

  Running this script from a top-level WebKit source directory and
  checking only files in the source tree will ensure that all style
  checking behaves correctly -- whether or not a checkout can be
  detected.  This is because all file paths will already be relative
  to the source root and so will not need to be converted.

Options:
  -h, --help            show this help message and exit
  -f RULES, --filter-rules=RULES
                        set a filter to control what categories of style errors to report.  Specify a
                        filter using a comma-delimited list of boolean filter rules, for example "--
                        filter -whitespace,+whitespace/braces".  To display all categories and which
                        are enabled by default, pass no value (e.g. '-f ""' or '--filter=').
  -g COMMIT, --git-diff=COMMIT, --git-commit=COMMIT
                        check all changes in the given commit. Use 'commit_id..' to check all changes
                        after commmit_id, or '*' to check all changes in the local branch including
                        local commits and staged/unstages working copy changes.
  -m INT, --min-confidence=INT
                        set the minimum confidence of style errors to report.  Can be an integer 1-5,
                        with 1 displaying all errors.  Defaults to 1.
  -o FORMAT, --output-format=FORMAT
                        set the output format, which can be "emacs" or "vs7" (for Visual Studio).
                        Defaults to "emacs".
  -v, --verbose         enable verbose logging.

This script can miss errors and does not substitute for code review.

ERROR: no such option: --no-squash


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Jerdonek]

(In reply to comment #8)
> Attachment 56956 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 2
> Last 3072 characters of output:
>  "whitespace/braces".
> 
>   Examples: --filter=-whitespace,+whitespace/braces
>             --filter=-whitespace,-runtime/printf,+runtime/printf_format
>             --filter=-,+build/include_what_you_use
> 
> Paths:
>   Certain style-checking behavior depends on the paths relative to
>   the WebKit source root of the files being checked.  For example,
> <snip>
> 
> ERROR: no such option: --no-squash
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

We probably want to do something about this extended error message -- maybe send the help string to stdout instead of stderr if that will do it.

[Ojan Vafai]

Created attachment 57042 [details]
Patch

[Ojan Vafai]

Comment on attachment 57042 [details]
Patch

Went ahead and added support for webkit-patch.squash=true bypassing the prompt.

[Ojan Vafai]

(In reply to comment #9)
> (In reply to comment #8)
> > If any of these errors are false positives, please file a bug against check-webkit-style.
> 
> We probably want to do something about this extended error message -- maybe send the help string to stdout instead of stderr if that will do it.

Yeah, we should not post such long error messages to bugzilla. This specific error is a bit of a unique case. My patch removes --no-squash. So the style-queue, which does not have my patch, calls check-webkit-style, which does have my patch, with --no-squash and it errors out.

[WebKit Review Bot]

Attachment 57042 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 2
Last 3072 characters of output:
 "whitespace/braces".

  Examples: --filter=-whitespace,+whitespace/braces
            --filter=-whitespace,-runtime/printf,+runtime/printf_format
            --filter=-,+build/include_what_you_use

Paths:
  Certain style-checking behavior depends on the paths relative to
  the WebKit source root of the files being checked.  For example,
  certain types of errors may be handled differently for files in
  WebKit/gtk/webkit/ (e.g. by suppressing "readability/naming" errors
  for files in this directory).

  Consequently, if the path relative to the source root cannot be
  determined for a file being checked, then style checking may not
  work correctly for that file.  This can occur, for example, if no
  WebKit checkout can be found, or if the source root can be detected,
  but one of the files being checked lies outside the source tree.

  If a WebKit checkout can be detected and all files being checked
  are in the source tree, then all paths will automatically be
  converted to paths relative to the source root prior to checking.
  This is also useful for display purposes.

  Currently, this command can detect the source root only if the
  command is run from within a WebKit checkout (i.e. if the current
  working directory is below the root of a checkout).  In particular,
  it is not recommended to run this script from a directory outside
  a checkout.

  Running this script from a top-level WebKit source directory and
  checking only files in the source tree will ensure that all style
  checking behaves correctly -- whether or not a checkout can be
  detected.  This is because all file paths will already be relative
  to the source root and so will not need to be converted.

Options:
  -h, --help            show this help message and exit
  -f RULES, --filter-rules=RULES
                        set a filter to control what categories of style errors to report.  Specify a
                        filter using a comma-delimited list of boolean filter rules, for example "--
                        filter -whitespace,+whitespace/braces".  To display all categories and which
                        are enabled by default, pass no value (e.g. '-f ""' or '--filter=').
  -g COMMIT, --git-diff=COMMIT, --git-commit=COMMIT
                        check all changes in the given commit. Use 'commit_id..' to check all changes
                        after commmit_id, or '*' to check all changes in the local branch including
                        local commits and staged/unstages working copy changes.
  -m INT, --min-confidence=INT
                        set the minimum confidence of style errors to report.  Can be an integer 1-5,
                        with 1 displaying all errors.  Defaults to 1.
  -o FORMAT, --output-format=FORMAT
                        set the output format, which can be "emacs" or "vs7" (for Visual Studio).
                        Defaults to "emacs".
  -v, --verbose         enable verbose logging.

This script can miss errors and does not substitute for code review.

ERROR: no such option: --no-squash


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ojan Vafai]

ping

[Chris Jerdonek]

(In reply to comment #12)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > If any of these errors are false positives, please file a bug against check-webkit-style.
> > 
> > We probably want to do something about this extended error message -- maybe send the help string to stdout instead of stderr if that will do it.
> 
> Yeah, we should not post such long error messages to bugzilla. This specific error is a bit of a unique case. My patch removes --no-squash. So the style-queue, which does not have my patch, calls check-webkit-style, which does have my patch, with --no-squash and it errors out.

I filed a bug for this behavior here:

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

[Chris Jerdonek]

(In reply to comment #5)
> (From update of attachment 56939 [details])
> As per discussion on #webkit and webkit-dev, I'm changing this patch to squash by default and prompt instead of erroring out in cases where squashing will actually squash multiple commits.

Can you provide a direct link to the webkit-dev e-mail saying what the final decision was?  I thought the decision was not to use "*" anymore, but the latest patch (patch #3) still has some references to *.

[Ojan Vafai]

Created attachment 57363 [details]
Patch

[WebKit Review Bot]

Attachment 57363 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 2
Last 3072 characters of output:
whitespace/braces" fails
  the category "whitespace/tab" and passes "whitespace/braces".

  Examples: --filter=-whitespace,+whitespace/braces
            --filter=-whitespace,-runtime/printf,+runtime/printf_format
            --filter=-,+build/include_what_you_use

Paths:
  Certain style-checking behavior depends on the paths relative to
  the WebKit source root of the files being checked.  For example,
  certain types of errors may be handled differently for files in
  WebKit/gtk/webkit/ (e.g. by suppressing "readability/naming" errors
  for files in this directory).

  Consequently, if the path relative to the source root cannot be
  determined for a file being checked, then style checking may not
  work correctly for that file.  This can occur, for example, if no
  WebKit checkout can be found, or if the source root can be detected,
  but one of the files being checked lies outside the source tree.

  If a WebKit checkout can be detected and all files being checked
  are in the source tree, then all paths will automatically be
  converted to paths relative to the source root prior to checking.
  This is also useful for display purposes.

  Currently, this command can detect the source root only if the
  command is run from within a WebKit checkout (i.e. if the current
  working directory is below the root of a checkout).  In particular,
  it is not recommended to run this script from a directory outside
  a checkout.

  Running this script from a top-level WebKit source directory and
  checking only files in the source tree will ensure that all style
  checking behaves correctly -- whether or not a checkout can be
  detected.  This is because all file paths will already be relative
  to the source root and so will not need to be converted.

Options:
  -h, --help            show this help message and exit
  -f RULES, --filter-rules=RULES
                        set a filter to control what categories of style
                        errors to report.  Specify a filter using a comma-
                        delimited list of boolean filter rules, for example "
                        --filter -whitespace,+whitespace/braces".  To display
                        all categories and which are enabled by default, pass
                        no value (e.g. '-f ""' or '--filter=').
  -g COMMIT, --git-diff=COMMIT, --git-commit=COMMIT
                        check all changes in the given commit. Use
                        'commit_id..' to check all changes after commmit_id
  -m INT, --min-confidence=INT
                        set the minimum confidence of style errors to report.
                        Can be an integer 1-5, with 1 displaying all errors.
                        Defaults to 1.
  -o FORMAT, --output-format=FORMAT
                        set the output format, which can be "emacs" or "vs7"
                        (for Visual Studio).  Defaults to "emacs".
  -v, --verbose         enable verbose logging.

This script can miss errors and does not substitute for code review.

ERROR: no such option: --no-squash


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ojan Vafai]

(In reply to comment #16)
> (In reply to comment #5)
> > (From update of attachment 56939 [details] [details])
> > As per discussion on #webkit and webkit-dev, I'm changing this patch to squash by default and prompt instead of erroring out in cases where squashing will actually squash multiple commits.
> 
> Can you provide a direct link to the webkit-dev e-mail saying what the final decision was?  I thought the decision was not to use "*" anymore, but the latest patch (patch #3) still has some references to *.

Ugh. I just missed a couple cases. Most of them were cases where we assertRaises and putting --git-commit=* or not both result in raising an error.

Sorry for the confusion. There are no more "*"s anywhere. :)

[Ojan Vafai]

Created attachment 57833 [details]
Patch

[Ojan Vafai]

Comment on attachment 57833 [details]
Patch

Just merging this patch to trunk.

[WebKit Review Bot]

Attachment 57833 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 2
Last 3072 characters of output:
whitespace/braces" fails
  the category "whitespace/tab" and passes "whitespace/braces".

  Examples: --filter=-whitespace,+whitespace/braces
            --filter=-whitespace,-runtime/printf,+runtime/printf_format
            --filter=-,+build/include_what_you_use

Paths:
  Certain style-checking behavior depends on the paths relative to
  the WebKit source root of the files being checked.  For example,
  certain types of errors may be handled differently for files in
  WebKit/gtk/webkit/ (e.g. by suppressing "readability/naming" errors
  for files in this directory).

  Consequently, if the path relative to the source root cannot be
  determined for a file being checked, then style checking may not
  work correctly for that file.  This can occur, for example, if no
  WebKit checkout can be found, or if the source root can be detected,
  but one of the files being checked lies outside the source tree.

  If a WebKit checkout can be detected and all files being checked
  are in the source tree, then all paths will automatically be
  converted to paths relative to the source root prior to checking.
  This is also useful for display purposes.

  Currently, this command can detect the source root only if the
  command is run from within a WebKit checkout (i.e. if the current
  working directory is below the root of a checkout).  In particular,
  it is not recommended to run this script from a directory outside
  a checkout.

  Running this script from a top-level WebKit source directory and
  checking only files in the source tree will ensure that all style
  checking behaves correctly -- whether or not a checkout can be
  detected.  This is because all file paths will already be relative
  to the source root and so will not need to be converted.

Options:
  -h, --help            show this help message and exit
  -f RULES, --filter-rules=RULES
                        set a filter to control what categories of style
                        errors to report.  Specify a filter using a comma-
                        delimited list of boolean filter rules, for example "
                        --filter -whitespace,+whitespace/braces".  To display
                        all categories and which are enabled by default, pass
                        no value (e.g. '-f ""' or '--filter=').
  -g COMMIT, --git-diff=COMMIT, --git-commit=COMMIT
                        check all changes in the given commit. Use
                        'commit_id..' to check all changes after commmit_id
  -m INT, --min-confidence=INT
                        set the minimum confidence of style errors to report.
                        Can be an integer 1-5, with 1 displaying all errors.
                        Defaults to 1.
  -o FORMAT, --output-format=FORMAT
                        set the output format, which can be "emacs" or "vs7"
                        (for Visual Studio).  Defaults to "emacs".
  -v, --verbose         enable verbose logging.

This script can miss errors and does not substitute for code review.

ERROR: no such option: --no-squash


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ojan Vafai]

Comment on attachment 57833 [details]
Patch

Could not upload patch 57833 to rietveld. Rietveld is down or there's a bug in the upload bot.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['/WebKit/WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'post-attachment-to-rietveld', '--force-clean', '--non-interactive', '--parent-command=rietveld-upload-queue', 57833]" exit_code: 1
Last 500 characters of output:
d.py", line 143, in _process_patch
    self._main_sequence.run_and_handle_errors(tool, options, state)
  File "/WebKit/WebKitTools/Scripts/webkitpy/tool/commands/stepsequence.py", line 86, in run_and_handle_errors
    command.handle_script_error(tool, state, e)
  File "/WebKit/WebKitTools/Scripts/webkitpy/tool/commands/queues.py", line 340, in handle_script_error
    self._reject_patch(patch, cls._error_message_for_bug(tool, status_id, script_error))
NameError: global name 'self' is not defined

[Ojan Vafai]

(In reply to comment #23)
> (From update of attachment 57833 [details])
> Could not upload patch 57833 to rietveld. Rietveld is down or there's a bug in the upload bot.

This is a bug in the rietveld upload bot. Sorry for the noise.

[Ojan Vafai]

ping

[Eric Seidel (no email)]

Comment on attachment 57833 [details]
Patch

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.

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. 

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.

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

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

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

Big change for my little brain... :(

[Ojan Vafai]

Created attachment 58688 [details]
Patch

[Ojan Vafai]

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.

[WebKit Review Bot]

Attachment 58688 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 2
Last 3072 characters of output:
For example, the
  rule "+foo" passes any category that starts with "foo", and "-foo" fails
  any such category.  The filter input "-whitespace,+whitespace/braces" fails
  the category "whitespace/tab" and passes "whitespace/braces".

  Examples: --filter=-whitespace,+whitespace/braces
            --filter=-whitespace,-runtime/printf,+runtime/printf_format
            --filter=-,+build/include_what_you_use

Paths:
  Certain style-checking behavior depends on the paths relative to
  the WebKit source root of the files being checked.  For example,
  certain types of errors may be handled differently for files in
  WebKit/gtk/webkit/ (e.g. by suppressing "readability/naming" errors
  for files in this directory).

  Consequently, if the path relative to the source root cannot be
  determined for a file being checked, then style checking may not
  work correctly for that file.  This can occur, for example, if no
  WebKit checkout can be found, or if the source root can be detected,
  but one of the files being checked lies outside the source tree.

  If a WebKit checkout can be detected and all files being checked
  are in the source tree, then all paths will automatically be
  converted to paths relative to the source root prior to checking.
  This is also useful for display purposes.

  Currently, this command can detect the source root only if the
  command is run from within a WebKit checkout (i.e. if the current
  working directory is below the root of a checkout).  In particular,
  it is not recommended to run this script from a directory outside
  a checkout.

  Running this script from a top-level WebKit source directory and
  checking only files in the source tree will ensure that all style
  checking behaves correctly -- whether or not a checkout can be
  detected.  This is because all file paths will already be relative
  to the source root and so will not need to be converted.

Options:
  -h, --help            show this help message and exit
  -f RULES, --filter-rules=RULES
                        set a filter to control what categories of style errors to report.  Specify a filter using a comma-delimited
                        list of boolean filter rules, for example "--filter -whitespace,+whitespace/braces".  To display all
                        categories and which are enabled by default, pass no value (e.g. '-f ""' or '--filter=').
  -g COMMIT, --git-diff=COMMIT, --git-commit=COMMIT
                        check all changes in the given commit. Use 'commit_id..' to check all changes after commmit_id
  -m INT, --min-confidence=INT
                        set the minimum confidence of style errors to report.  Can be an integer 1-5, with 1 displaying all errors.
                        Defaults to 1.
  -o FORMAT, --output-format=FORMAT
                        set the output format, which can be "emacs" or "vs7" (for Visual Studio).  Defaults to "emacs".
  -v, --verbose         enable verbose logging.

This script can miss errors and does not substitute for code review.

ERROR: no such option: --no-squash


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 58688 [details]
Patch

I use --no-squash all the time.  I don't think we should remove it.

[Ojan Vafai]

(In reply to comment #30)
> I use --no-squash all the time.  I don't think we should remove it.

In what cases do you use --no-squash? Are you sure they're not met by this patch?

[Ojan Vafai]

Adam and I talked this over in person. His use case is making a change in the working copy and uploading it when there are already local commits. He can't just commit locally and upload that due to bug 40552 (can't update the ChangeLog on a local commit).

I think the use-case is worth supporting. We should make --git-commit=head.. operate on the working copy.

I'll try and post a patch soon.

[Ojan Vafai]

Created attachment 60408 [details]
Support --git-commit=HEAD..

[WebKit Review Bot]

Attachment 60408 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 2
Last 3072 characters of output:
whitespace/braces" fails
  the category "whitespace/tab" and passes "whitespace/braces".

  Examples: --filter=-whitespace,+whitespace/braces
            --filter=-whitespace,-runtime/printf,+runtime/printf_format
            --filter=-,+build/include_what_you_use

Paths:
  Certain style-checking behavior depends on the paths relative to
  the WebKit source root of the files being checked.  For example,
  certain types of errors may be handled differently for files in
  WebKit/gtk/webkit/ (e.g. by suppressing "readability/naming" errors
  for files in this directory).

  Consequently, if the path relative to the source root cannot be
  determined for a file being checked, then style checking may not
  work correctly for that file.  This can occur, for example, if no
  WebKit checkout can be found, or if the source root can be detected,
  but one of the files being checked lies outside the source tree.

  If a WebKit checkout can be detected and all files being checked
  are in the source tree, then all paths will automatically be
  converted to paths relative to the source root prior to checking.
  This is also useful for display purposes.

  Currently, this command can detect the source root only if the
  command is run from within a WebKit checkout (i.e. if the current
  working directory is below the root of a checkout).  In particular,
  it is not recommended to run this script from a directory outside
  a checkout.

  Running this script from a top-level WebKit source directory and
  checking only files in the source tree will ensure that all style
  checking behaves correctly -- whether or not a checkout can be
  detected.  This is because all file paths will already be relative
  to the source root and so will not need to be converted.

Options:
  -h, --help            show this help message and exit
  -f RULES, --filter-rules=RULES
                        set a filter to control what categories of style
                        errors to report.  Specify a filter using a comma-
                        delimited list of boolean filter rules, for example "
                        --filter -whitespace,+whitespace/braces".  To display
                        all categories and which are enabled by default, pass
                        no value (e.g. '-f ""' or '--filter=').
  -g COMMIT, --git-diff=COMMIT, --git-commit=COMMIT
                        check all changes in the given commit. Use
                        'commit_id..' to check all changes after commmit_id
  -m INT, --min-confidence=INT
                        set the minimum confidence of style errors to report.
                        Can be an integer 1-5, with 1 displaying all errors.
                        Defaults to 1.
  -o FORMAT, --output-format=FORMAT
                        set the output format, which can be "emacs" or "vs7"
                        (for Visual Studio).  Defaults to "emacs".
  -v, --verbose         enable verbose logging.

This script can miss errors and does not substitute for code review.

ERROR: no such option: --no-squash


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 60408 [details]
Support --git-commit=HEAD..

Can we be more user friendly during the transition?  Maybe we should understand --no-squash and print a message that explains the new way is --git-commit=HEAD.. ?  Another option is to automatically re-write --no-squash into --git-commit=HEAD..  Some minor comments below.

WebKitTools/Scripts/webkitpy/tool/steps/checkstyle_unittest.py:38
 +      pass
Should we delete this file if there aren't any tests in it?

WebKitTools/Scripts/webkitpy/tool/steps/commit.py:68
 +              username = User.prompt("%s login: " % e.server_host, repeat=5)
You should use self._tool.user here.

WebKitTools/Scripts/webkitpy/tool/steps/commit.py:71
 +              self._commit(username=username)
What if this throws an AmbiguousCommitError?  I think you want to use some sort of loop with these things as state variables.  You can put a retry limit in the loop if you're worried about going infinite.

WebKitTools/ChangeLog:9
 +          need to be met by a wrapper script around webkit-patch.
This isn't really true anymore, right?  I can use --git-commit=HEAD..

[Ojan Vafai]

(In reply to comment #35)
> (From update of attachment 60408 [details])
> Can we be more user friendly during the transition?  Maybe we should understand --no-squash and print a message that explains the new way is --git-commit=HEAD.. ?  Another option is to automatically re-write --no-squash into --git-commit=HEAD..  Some minor comments below.

Good idea. I put in --squash and --no-squash. I made them error out with a descriptive message. I'd like to remove these in a couple weeks once people have adjusted.

> WebKitTools/Scripts/webkitpy/tool/steps/checkstyle_unittest.py:38
>  +      pass
> Should we delete this file if there aren't any tests in it?

Done.

> WebKitTools/Scripts/webkitpy/tool/steps/commit.py:68
>  +              username = User.prompt("%s login: " % e.server_host, repeat=5)
> You should use self._tool.user here.

Done.

> WebKitTools/Scripts/webkitpy/tool/steps/commit.py:71
>  +              self._commit(username=username)
> What if this throws an AmbiguousCommitError?  I think you want to use some sort of loop with these things as state variables.  You can put a retry limit in the loop if you're worried about going infinite.

This isn't possible right now. Only the SVN impl of scm fires AuthenticationError and only the git impl fires AmbiguousCommitError. The former could definitely change though. Added a try/except.

> WebKitTools/ChangeLog:9
>  +          need to be met by a wrapper script around webkit-patch.
> This isn't really true anymore, right?  I can use --git-commit=HEAD..

No, there are still people who want to do setup local commits and have the script act on all the commits at once (e.g. commit them all with a single command). Made the description a bit more accurate.

[Ojan Vafai]

Created attachment 61074 [details]
Patch

[WebKit Review Bot]

Attachment 61074 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 2
Last 3072 characters of output:
whitespace/braces" fails
  the category "whitespace/tab" and passes "whitespace/braces".

  Examples: --filter=-whitespace,+whitespace/braces
            --filter=-whitespace,-runtime/printf,+runtime/printf_format
            --filter=-,+build/include_what_you_use

Paths:
  Certain style-checking behavior depends on the paths relative to
  the WebKit source root of the files being checked.  For example,
  certain types of errors may be handled differently for files in
  WebKit/gtk/webkit/ (e.g. by suppressing "readability/naming" errors
  for files in this directory).

  Consequently, if the path relative to the source root cannot be
  determined for a file being checked, then style checking may not
  work correctly for that file.  This can occur, for example, if no
  WebKit checkout can be found, or if the source root can be detected,
  but one of the files being checked lies outside the source tree.

  If a WebKit checkout can be detected and all files being checked
  are in the source tree, then all paths will automatically be
  converted to paths relative to the source root prior to checking.
  This is also useful for display purposes.

  Currently, this command can detect the source root only if the
  command is run from within a WebKit checkout (i.e. if the current
  working directory is below the root of a checkout).  In particular,
  it is not recommended to run this script from a directory outside
  a checkout.

  Running this script from a top-level WebKit source directory and
  checking only files in the source tree will ensure that all style
  checking behaves correctly -- whether or not a checkout can be
  detected.  This is because all file paths will already be relative
  to the source root and so will not need to be converted.

Options:
  -h, --help            show this help message and exit
  -f RULES, --filter-rules=RULES
                        set a filter to control what categories of style
                        errors to report.  Specify a filter using a comma-
                        delimited list of boolean filter rules, for example "
                        --filter -whitespace,+whitespace/braces".  To display
                        all categories and which are enabled by default, pass
                        no value (e.g. '-f ""' or '--filter=').
  -g COMMIT, --git-diff=COMMIT, --git-commit=COMMIT
                        check all changes in the given commit. Use
                        'commit_id..' to check all changes after commmit_id
  -m INT, --min-confidence=INT
                        set the minimum confidence of style errors to report.
                        Can be an integer 1-5, with 1 displaying all errors.
                        Defaults to 1.
  -o FORMAT, --output-format=FORMAT
                        set the output format, which can be "emacs" or "vs7"
                        (for Visual Studio).  Defaults to "emacs".
  -v, --verbose         enable verbose logging.

This script can miss errors and does not substitute for code review.

ERROR: no such option: --no-squash


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 61074 [details]
Patch

I'm glad we're getting this done.  Please fix the retry look and coordinate with me and eric when you land this change since we'll need to reboot all the bots at the same time.

[Ojan Vafai]

Committed r63004: <http://trac.webkit.org/changeset/63004>

[WebKit Review Bot]

http://trac.webkit.org/changeset/63004 might have broken Leopard Intel Release (Tests)

[Martin Robinson]

My workflow is a bit different than others because I often have a very slow review cycle. I may upload patches that sit for days without being reviewed. In the meantime, I wish to continue working on further patches. In those cases I use them as local commits, but I never want to upload them.

Is there a way with this change to disable the squash behavior entirely? For me it is almost always the incorrect behavior.

[Adam Barth]

> Is there a way with this change to disable the squash behavior entirely? For me it is almost always the incorrect behavior.

There's a git config setting to control the behavior.  If the setting you want doesn't exist, feel free to make a new one.