Re: [PATCH v1] travis-ci: build documentation

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 22, 2016, 18:14 UTC
Message-ID: <xmqqinz9pl3l.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <vpq37qeovu4.fsf@anie.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Show 8 quoted lines

> larsxschneider@gmail.com writes:
>
>> +      if [[ "$TRAVIS_OS_NAME" = linux ]] && [[ "$CC" = gcc ]];
>
> [[ is a bashism, and doesn't bring anything here compared to the POSIX
> [ ... ], or "test" which is prefered in Git's source code.
>
> The ; or the newline is not needed either.

Honestly, I didn't know that we were even trying to be pure POSIX, avoid bashism or GNUism, or in general to follow our shell scripting style in the scriptlet in the .travis.yml file.

While I feel fairly strongly about keeping the generic part generic, I am actually OK with things that are known to be used in a specific environment to be specific to that environment.

Having said all that, if we are not benefiting from using features beyond POSIX, then by all means we should strive to be writing our stuff in a portable way, as we do not have firm control over from where and to where people cut and paste code snippets.

And I do think bashism [[ ... ]] is *NOT* buying anything in this particular case, so I do agree with you that

	if test "$TRAVIS_OS_NAME" = linux && test "$CC" = gcc
        then
        	...

or even

	case "$TRAVIS_OS_NAME,$CC" in
        linux,gcc)
        	...

is what I would have written instead if I were writing this conditional.

If we were to shoot for "be POSIX unless we can clearly benefit from being bash/gnu/linux specific in bash/gnu/linux specific parts", the existing scriptlets in .travis.yml file has a few things that may need to be cleaned up already. There are "mkdir --parents" (POSIX spells it "-p"), "pushd/popd" and invocation of "tar" is very GNU specific in the part that appears in the case arm for "linux".

There also are existing instances of "useless ;" that would want to be cleaned up regardless of portability issues.

Show 7 quoted lines

>> +          then
>> +          echo ""
>> +          echo "------------------------------------------------------------------------" &&
>
> I usualy avoid "echo <something-starting-with-dash>" as I'm not sure how
> portable it is across variants of "echo". Maybe this one is portable
> enough, I don't know. Perhaps printf, or cat << EOF ...?

Do you even need a long divider there?

Show 6 quoted lines

> I think it makes sense to do some lightweight checks after "make doc",
> rather than just check the return code. For example, check that a few
> generated files exist and are non-empty, like
>
> test -s Documentation/git.html &&
> test -s Documentation/git.1

Yup, or the formatter does not give new/unknown warnings.

Thanks.