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.
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.
Created attachment 56939 [details] Patch
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.
(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.
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.
Created attachment 56956 [details] Patch
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.
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.
(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.
Created attachment 57042 [details] Patch
Comment on attachment 57042 [details] Patch Went ahead and added support for webkit-patch.squash=true bypassing the prompt.
(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.
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.
ping
(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
(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 *.
Created attachment 57363 [details] Patch
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.
(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. :)
Created attachment 57833 [details] Patch
Comment on attachment 57833 [details] Patch Just merging this patch to trunk.
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.
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
(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.
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... :(
Created attachment 58688 [details] Patch
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.
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.
Comment on attachment 58688 [details] Patch I use --no-squash all the time. I don't think we should remove it.
(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?
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.
Created attachment 60408 [details] Support --git-commit=HEAD..
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.
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..
(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.
Created attachment 61074 [details] Patch
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.
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.
Committed r63004: <http://trac.webkit.org/changeset/63004>
http://trac.webkit.org/changeset/63004 might have broken Leopard Intel Release (Tests)
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.
> 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.