Re: [PATCH 1/3] test-lib: Document short options in t/README

From: Ilya Bobyr <ilya.bobyr@gmail.com>
Date: Mar 27, 2014, 09:39 UTC
Message-ID: <5333F1E6.5060009@gmail.com>
In-Reply-To: <xmqqior2mbtx.fsf@gitster.dls.corp.google.com>

On 3/25/2014 10:23 AM, Junio C Hamano wrote:

Show 49 quoted lines

> Ilya Bobyr <ilya.bobyr@gmail.com> writes:
>
>> On 3/24/2014 4:39 AM, Ramsay Jones wrote:
>>> On 24/03/14 08:49, Ilya Bobyr wrote:
>>> [...]
>>>> [...]
>>>>  
>>>> ---valgrind=<tool>::
>>>> +-v,--valgrind=<tool>::
>>> The -v short option is taken, above ... :-P
>> Right %)
>> Thanks :)
>> This one starts only with "--va", will fix it.
> Please don't.
>
> In general, when option names can be shortened by taking a unique
> prefix, it is better not to give short form in the documentation at
> all.  There is no guarantee that the short form you happen to pick
> when you document it will continue to be unique forever.  When we
> add another --vasomething option, --va will become ambiguous and one
> of these two things must happen:
>
>  (1) --valgrind and --vasomething are equally useful and often used.
>      Neither will get --va and either --val or --vas needs to be
>      given.
>
>  (2) Because we documented --va as --valgrind, people feel that they
>      are entitled to expect --va will stay forever to be a shorthand
>      for --valgrind and nothing else.  The shortened forms will be
>      between --va (or longer prefix of --valgrind) and --vas (or
>      longer prefix of --vasomething).
>
> We would rather want to see (1), as people new to the system do not
> have to learn that --valgrind can be spelled --va merely by being
> the first to appear, and --vasomething must be spelled --vas because
> it happened to come later.  Longer term, nobody should care how the
> system evolved into the current shape, but (2) will require that to
> understand and remember why one is --va and the other has to be --vas.
>
> We already have this suboptimal (2) situation between "--valgrind"
> and "--verbose" options, but a shorter form "v" that is used for
> "verbose" is so widely understood and used that I think it is an
> acceptable exception.  So
>
>          --verbose::
>         +-v::
>                 Give verbose output from the test
>
> is OK, but "--valgrind can be shortened to --va" is not.

Sure, this is exactly what I meant, but I guess, I was too short so it created ambiguity =) I was going to just remove the '-v' from '--valgrind'.

Shortening is a separate issue. I did not look at it. I can see that it is also not documented. At the same time shortening is entirely consistent at the moment, and does not work for options that take arguments.

My main intent was to document '-r' :) As no other short form were documented, I had to fix that issue first.

If there is decision on how shortening should work for all the options, maybe I could add a paragraph on that and make existing options more consistent.

I guess the questions would be, should it possible to use short forms for options that take arguments?

If so, '--valgrind' becomes impossible to shorten because there is '--valgrind-only' that is a separate option. Same for '--verbose' and '--verbose-only'.

For convenience here is the relevant switch in the way it is right now:

    case "$1" in
    -d|--d|--de|--deb|--debu|--debug)
        debug=t; shift ;;
   
-i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)
        immediate=t; shift ;;
   
-l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests)
        GIT_TEST_LONG=t; export GIT_TEST_LONG; shift ;;
    -r)
        shift; test "$#" -ne 0 || {
            echo 'error: -r requires an argument' >&2;
            exit 1;
        }
        run_list=$1; shift ;;
    --run=*)
        run_list=$(expr "z$1" : 'z[^=]*=\(.*\)'); shift ;;
    -h|--h|--he|--hel|--help)
        help=t; shift ;;
    -v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
        verbose=t; shift ;;
    --verbose-only=*)
        verbose_only=$(expr "z$1" : 'z[^=]*=\(.*\)')
        shift ;;
    -q|--q|--qu|--qui|--quie|--quiet)
        # Ignore --quiet under a TAP::Harness. Saying how many tests
        # passed without the ok/not ok details is always an error.
        test -z "$HARNESS_ACTIVE" && quiet=t; shift ;;
    --with-dashes)
        with_dashes=t; shift ;;
    --no-color)
        color=; shift ;;
    --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
        valgrind=memcheck
        shift ;;
    --valgrind=*)
        valgrind=$(expr "z$1" : 'z[^=]*=\(.*\)')
        shift ;;
    --valgrind-only=*)
        valgrind_only=$(expr "z$1" : 'z[^=]*=\(.*\)')
        shift ;;
    --tee)
        shift ;; # was handled already
    --root=*)
        root=$(expr "z$1" : 'z[^=]*=\(.*\)')
        shift ;;
    *)
        echo "error: unknown test option '$1'" >&2; exit 1 ;;
    esac

P.S. Sorry it takes me this long to reply. I will try to be more responsive, should there will be a discussion :)