Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 19, 2019, 01:31 UTC
Message-ID: <20190719013128.GB251102@google.com>
In-Reply-To: <20190718152234.GI20404@szeder.dev>

SZEDER Gábor wrote:

Show 12 quoted lines

> I expected that this will eventually happen after Travis CI's default
> Linux image recently changed from Ubuntu 14.04 to 16.04; explanation
> in the commit message below.
>
> With that patch issues like this could be caught earlier, while they
> are only in 'pu' but not yet in 'next'.  But do we really want to do
> that, is that the right tradeoff?  Dunno...  Adding a dedicated CI job
> just to check that there are no 'for' loop initial declarations seems
> kind of excessive, even if it only builds but doesn't run the test
> suite.  And I don't know whether there are any other undesired ("too
> new") constructs that GCC 4.8 would catch but later compilers quietly
> accept.

This makes sense to me. Not really for the 'for' loop declaration aspect: for that, I'd want some more specialized tool that allows turning on such a check specifically. But more because Ubuntu trusty is still a platform that some people use (though hopefully not for long), so it's helpful as a representative old platform to see if we break the build on it.

[...]

> [2] On Travis CI 'make test' alone would take about 9 minutes in this
>     new job (without running httpd, Subversion, and P4 tests).  For
>     comparison, starting the job and building Git with GCC 4.8 takes
>     only about 2 minutes.

Nice. In an ideal world there would be some kind of "make fasttest" that runs some fast subset of tests. But this seems pretty safe.

Show 5 quoted lines

> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  .travis.yml               |  4 ++++
>  ci/run-build-and-tests.sh | 17 +++++++++++++----
>  2 files changed, 17 insertions(+), 4 deletions(-)

Thanks.

[...]

Show 30 quoted lines

> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -11,9 +11,9 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";;
>  esac
>  
>  make
> -make test
> -if test "$jobname" = "linux-gcc"
> -then
> +case "$jobname" in
> +linux-gcc)
> +	make test
>  	export GIT_TEST_SPLIT_INDEX=yes
>  	export GIT_TEST_FULL_IN_PACK_ARRAY=true
>  	export GIT_TEST_OE_SIZE=10
> @@ -21,7 +21,16 @@ then
>  	export GIT_TEST_COMMIT_GRAPH=1
>  	export GIT_TEST_MULTI_PACK_INDEX=1
>  	make test
> -fi
> +	;;
> +linux-gcc-4.8)
> +	# Don't run the tests; we only care about whether Git can be
> +	# built with GCC 4.8, as it errors out on some undesired (C99)
> +	# constructs that newer compilers seem to quietly accept.
> +	;;
> +*)
> +	make test
> +	;;
> +esac

Does what it says on the tin.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>