| Message ID | 1357768902-16190-1-git-send-email-vbendeb@chromium.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, Jan 9, 2013 at 2:01 PM, Vadim Bendebury <vbendeb@chromium.org> wrote: > To make it usable in git trees not providing a patch checker > implementation, add a command line option, allowing to suppress patch > check. While we are at it, sort debug options alphabetically. > > . unit test passes: > $ ./patman -t > <unittest.result.TestResult run=7 errors=0 failures=0> > . successfully used patman in the autotest tree to generate a patch > email (with --no-check option) > . successfully used patman in the u-boot tree to generate a patch > email > . `patman --help' now shows command line options ordered > alphabetically > > Signed-off-by: Vadim Bendebury <vbendeb@chromium.org> > --- > tools/patman/patman.py | 14 ++++++++++---- > 1 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/tools/patman/patman.py b/tools/patman/patman.py > index e56dd01..6620a48 100755 > --- a/tools/patman/patman.py > +++ b/tools/patman/patman.py > @@ -50,6 +50,9 @@ parser.add_option('-i', '--ignore-errors', action='store_true', > help='Send patches email even if patch errors are found') > parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run', > default=False, help="Do a try run (create but don't email patches)") > +parser.add_option('-p', '--project', default=project.DetectProject(), > + help="Project name; affects default option values and " > + "aliases [default: %default]") > parser.add_option('-s', '--start', dest='start', type='int', > default=0, help='Commit to start creating patches from (0 = HEAD)') > parser.add_option('-t', '--test', action='store_true', dest='test', > @@ -58,11 +61,11 @@ parser.add_option('-v', '--verbose', action='store_true', dest='verbose', > default=False, help='Verbose output of errors and warnings') > parser.add_option('--cc-cmd', dest='cc_cmd', type='string', action='store', > default=None, help='Output cc list for patch file (used by git)') > +parser.add_option('--no-check', action='store_true', dest='no_check', > + default=False, > + help="Don't check for patch compliance") > parser.add_option('--no-tags', action='store_false', dest='process_tags', > default=True, help="Don't process subject tags as aliaes") > -parser.add_option('-p', '--project', default=project.DetectProject(), > - help="Project name; affects default option values and " > - "aliases [default: %default]") > > parser.usage = """patman [options] > > @@ -146,7 +149,10 @@ else: > series.DoChecks() > > # Check the patches, and run them through 'git am' just to be sure > - ok = checkpatch.CheckPatches(options.verbose, args) > + if options.no_check: > + ok = True > + else: > + ok = checkpatch.CheckPatches(options.verbose, args) > if not gitutil.ApplyPatches(options.verbose, args, > options.count + options.start): > ok = False > -- > 1.7.7.3 > Doug, thank you for a prompt review, copying your response here, please see below: On Wed, Jan 9, 2013 at 1:48 PM, Doug Anderson <dianders@chromium.org> wrote: > Vadim, > > Thanks for the patch! Looks good in general, though please add the > "patman" prefix to the first line of your commit message. > done > > On Wed, Jan 9, 2013 at 1:13 PM, Vadim Bendebury <vbendeb@chromium.org> wrote: >> To make it usable in git trees not providing a patch checker >> implementation, add a command line option, allowing to suippress patch > > s/suippress/suppress > done >> +parser.add_option('--no-check', action='store_true', dest='no_check', >> + default=False, >> + help="Don't check for patch compliance") > > IMHO It would be slightly better to use action='store_false', > dest='check', and default=True (just to avoid so many > double-negatives). I don't quite agree with this part - I think it's perfectly reasonable to use 'no-check' to suppress the check, just as well as to use 'no-tags' to suppress interpreting tags. `--no' communicates that by default the respective feature is enabled, and to disable it one needs to add a command line option with no parameter. cheers, /vb
Vadim, On Wed, Jan 9, 2013 at 2:07 PM, Vadim Bendebury <vbendeb@chromium.org> wrote: >>> +parser.add_option('--no-check', action='store_true', dest='no_check', >>> + default=False, >>> + help="Don't check for patch compliance") >> >> IMHO It would be slightly better to use action='store_false', >> dest='check', and default=True (just to avoid so many >> double-negatives). > > I don't quite agree with this part - I think it's perfectly reasonable > to use 'no-check' to suppress the check, just as well as to use > 'no-tags' to suppress interpreting tags. > > `--no' communicates that by default the respective feature is enabled, > and to disable it one needs to add a command line option with no > parameter. Sorry--should have been more explicit. Was still expecting the option to be --no-check. Just asking for a change to the way it's stored. Like this in the python dev guide: parser.add_option("--clobber", action="store_true", dest="clobber") parser.add_option("--no-clobber", action="store_false", dest="clobber") In your case, I don't think you need to add the "check" option too, but just store to the "check" option: parser.add_option('--no-check', action='store_false', dest='check', default=True, help="Don't check for patch compliance")
On Wed, Jan 9, 2013 at 2:57 PM, Doug Anderson <dianders@chromium.org> wrote: > Vadim, > > On Wed, Jan 9, 2013 at 2:07 PM, Vadim Bendebury <vbendeb@chromium.org> wrote: >>>> +parser.add_option('--no-check', action='store_true', dest='no_check', >>>> + default=False, >>>> + help="Don't check for patch compliance") >>> >>> IMHO It would be slightly better to use action='store_false', >>> dest='check', and default=True (just to avoid so many >>> double-negatives). >> >> I don't quite agree with this part - I think it's perfectly reasonable >> to use 'no-check' to suppress the check, just as well as to use >> 'no-tags' to suppress interpreting tags. >> >> `--no' communicates that by default the respective feature is enabled, >> and to disable it one needs to add a command line option with no >> parameter. > > Sorry--should have been more explicit. Was still expecting the option > to be --no-check. Just asking for a change to the way it's stored. > Like this in the python dev guide: > > parser.add_option("--clobber", action="store_true", dest="clobber") > parser.add_option("--no-clobber", action="store_false", dest="clobber") > > In your case, I don't think you need to add the "check" option too, > but just store to the "check" option: > > parser.add_option('--no-check', action='store_false', dest='check', > default=True, > help="Don't check for patch compliance") ah, a good idea, changed. Also, modified the code in checkpatch.py not to throw an exception, but to print an error message and return a nonzero status.
diff --git a/tools/patman/patman.py b/tools/patman/patman.py index e56dd01..6620a48 100755 --- a/tools/patman/patman.py +++ b/tools/patman/patman.py @@ -50,6 +50,9 @@ parser.add_option('-i', '--ignore-errors', action='store_true', help='Send patches email even if patch errors are found') parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run', default=False, help="Do a try run (create but don't email patches)") +parser.add_option('-p', '--project', default=project.DetectProject(), + help="Project name; affects default option values and " + "aliases [default: %default]") parser.add_option('-s', '--start', dest='start', type='int', default=0, help='Commit to start creating patches from (0 = HEAD)') parser.add_option('-t', '--test', action='store_true', dest='test', @@ -58,11 +61,11 @@ parser.add_option('-v', '--verbose', action='store_true', dest='verbose', default=False, help='Verbose output of errors and warnings') parser.add_option('--cc-cmd', dest='cc_cmd', type='string', action='store', default=None, help='Output cc list for patch file (used by git)') +parser.add_option('--no-check', action='store_true', dest='no_check', + default=False, + help="Don't check for patch compliance") parser.add_option('--no-tags', action='store_false', dest='process_tags', default=True, help="Don't process subject tags as aliaes") -parser.add_option('-p', '--project', default=project.DetectProject(), - help="Project name; affects default option values and " - "aliases [default: %default]") parser.usage = """patman [options] @@ -146,7 +149,10 @@ else: series.DoChecks() # Check the patches, and run them through 'git am' just to be sure - ok = checkpatch.CheckPatches(options.verbose, args) + if options.no_check: + ok = True + else: + ok = checkpatch.CheckPatches(options.verbose, args) if not gitutil.ApplyPatches(options.verbose, args, options.count + options.start): ok = False
To make it usable in git trees not providing a patch checker implementation, add a command line option, allowing to suppress patch check. While we are at it, sort debug options alphabetically. . unit test passes: $ ./patman -t <unittest.result.TestResult run=7 errors=0 failures=0> . successfully used patman in the autotest tree to generate a patch email (with --no-check option) . successfully used patman in the u-boot tree to generate a patch email . `patman --help' now shows command line options ordered alphabetically Signed-off-by: Vadim Bendebury <vbendeb@chromium.org> --- tools/patman/patman.py | 14 ++++++++++---- 1 files changed, 10 insertions(+), 4 deletions(-)