Thanks.  Looks pretty good.  My review comments:

> +    for c in "${force[@]}"; do
> +        if [ "$c" = "$check" ]; then
> +            check_is_forced=true
> +            break
> +        fi
> +    done

It is possible, including here I think, to do something like this:

  case " ${force[*]} " in
  *" $check "*) check_is_forced=true ;;
  esac

If you make force be a list of ,-terminated items rather than an
array, the user can say --force=foo,bar and the case stunt I propose
above (with spaced replaced with appropriate commas) will still do
nicely.

Up to you.

> +        '--force')
> +            case "$2" in
> +                'suite'|'upstream-nonancestor'|'unreleased'|'dgit-view')
> +                    force+=("$2")                             ;;
> +                '')
> +                    force_all=true                            ;;
> +                *)
> +                    fail "invalid name of check to force: $2" ;;

This is not correct.  Unrecognised force options should be ignored, so
that if we introduce a new check you can force it in a way that works
with old git-debpush.

We should perhaps add
   --force=no-such-force-option
to an arbitrary one of the runes in tagupl.

> +    fail_check "unreleased" "UNRELEASED changelog"

I wouldn't quote the check name personally.  After all we are
certainly not going to introduce check names that contain shell
metacharacters...

Up to you.

> +if $failed_check; then
> +    # We don't mention the --force=check options here as those are
> +    # mainly for use by scripts, or when you already know what check
> +    # is going to fail before you invoke git-debpush.  Keep the
> +    # script's terminal output as simple as possible.  No "see the
> +    # manpage"!

AFAICT nothing prints the name of the check that failed.  Can we
please print it *somewhere* ?  Ideally in the error message printed by
fail_check.  Not printing it makes this new feature much less useable.

How about adding `[--force=THING]' to the per-failure error message
and doing this:
-     fail "some checks failed; you can override with --force"
+     fail "some check(s) failed; say just --force to override them all"
or something ?

Ian.

-- 
Ian Jackson <ijack...@chiark.greenend.org.uk>   These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.

Reply via email to