git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
SZEDER Gábor <szeder.dev@gmail.com>
Date
Jul 18, 2019, 15:22 UTC
Message-ID
<20190718152234.GI20404@szeder.dev>
In-Reply-To
<xmqq8ssx53a0.fsf@gitster-ct.c.googlers.com>
On Tue, Jul 16, 2019 at 09:53:59AM -0700, Junio C Hamano wrote:
> Carlo Arenas <carenas@gmail.com> writes:
Show 5 quoted lines
> >> +                       for (struct ref *it = remote_refs; it; it = it->next)
> >
> > moving "struct ref it" out of the loop, allows for building with ancient
> > compilers that don't support C90 (even if only by default) as I found
> > out while building pu in a Centos 6 box
Show 5 quoted lines
> but I think we still reject variable definition in
> for loop control (we saw and rewrote another patch that tried to use
> it late last year).
> 
> Apparently, this one slipped our review process.

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.

  --- >8 ---
Subject: [PATCH] travis-ci: build with GCC 4.8 as well

C99 'for' loop initial declaration, i.e. 'for (int i = 0; i < n; i++)', is not allowed in Git's codebase yet, to maintain compatibility with some older compilers.

Our Travis CI builds used to catch 'for' loop initial declarations, because the GETTEXT_POISON job has always built Git with the default 'cc', which in Travis CI's previous default Linux image (based on Ubuntu 14.04 Trusty) is GCC 4.8, and that GCC version errors out on this construct (not only with DEVELOPER=1, but with our default CFLAGS as well). Alas, that's not the case anymore, becase after 14.04's EOL Travis CI's current default Linux image is based on Ubuntu 16.04 Xenial [1] and its default 'cc' is now GCC 5.4, which, just like all later GCC and Clang versions, simply accepts this construct, even if we don't explicitly specify '-std=c99'.

Ideally we would adjust our CFLAGS used with DEVELOPER=1 to catch this undesired construct already when contributors build Git on their own machines. Unfortunately, however, there seems to be no compiler option that would catch only this particular construct without choking on many other things, e.g. while a later compiler with '-std=c90' and/or '-ansi' does catch this construct, it can't build Git because of several screenfulls of other errors.

Add the 'linux-gcc-4.8' job to Travis CI, in order to build Git with GCC 4.8, and thus to timely catch any 'for' loop initial declarations. To catch those it's sufficient to only build Git with GCC 4.8, so don't run the test suite in this job, because 'make test' takes rather long [2], and it's already run five times in other jobs, so we wouldn't get our time's worth.

[1] The Azure Pipelines builds have been using Ubuntu 16.04 images
    from the start, so I belive they never caught 'for' loop initial
    declarations.
[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.
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(-)
diff --git a/.travis.yml b/.travis.yml
index ffb1bc46f2..fc5730b085 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -21,6 +21,10 @@ matrix:
       compiler:
       addons:
       before_install:
+    - env: jobname=linux-gcc-4.8
+      os: linux
+      dist: trusty
+      compiler:
     - env: jobname=Linux32
       os: linux
       compiler:
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index cdd2913440..ff0ef7f08e 100755
--- 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
 
 check_unignored_build_artifacts
 
-- 
2.22.0.810.g50207c7d84
Previous: Jonathan NiederNext: Junio C Hamano
Message 37 of 45 in “transport-helper: enforce atomic in push_refs_with_push”
  1. transport-helper: enforce atomic in push_refs_with_pushEmily Shaffer, Jul 2, 2019
  2. Johannes SchindelinJul 2, 2019
  3. Junio C HamanoJul 2, 2019
  4. Emily ShafferJul 3, 2019
  5. Emily ShafferJul 3, 2019
  6. Johannes SchindelinJul 3, 2019
  7. Emily ShafferJul 3, 2019
  8. Johannes SchindelinJul 4, 2019
  9. Emily ShafferJul 9, 2019
  10. Junio C HamanoJul 2, 2019
  11. Junio C HamanoJul 2, 2019
  12. Emily ShafferJul 3, 2019
  13. Junio C HamanoJul 2, 2019
  14. Emily ShafferJul 3, 2019
  15. SZEDER GáborJul 3, 2019
  16. Junio C HamanoJul 3, 2019
  17. Emily ShafferJul 3, 2019
  18. transport-helper: enforce atomic in push_refs_with_pushEmily Shaffer, Jul 9, 2019
  19. Junio C HamanoJul 10, 2019
  20. Junio C HamanoJul 10, 2019
  21. Emily ShafferJul 11, 2019
  22. Emily ShafferJul 11, 2019
  23. Junio C HamanoJul 11, 2019
  24. transport-helper: enforce atomic in push_refs_with_pushEmily Shaffer, Jul 11, 2019
  25. Junio C HamanoJul 12, 2019
  26. Carlo ArenasJul 16, 2019
  27. Junio C HamanoJul 16, 2019
  28. CodingGuidelines: spell out post-C89 rulesJunio C Hamano, Jul 16, 2019
  29. Jonathan NiederJul 17, 2019
  30. Junio C HamanoJul 17, 2019
  31. Jonathan NiederJul 19, 2019
  32. Bryan TurnerJul 17, 2019
  33. Junio C HamanoJul 17, 2019
  34. Carlo ArenasJul 16, 2019
  35. transport-helper: avoid var decl in for () loop controlJunio C Hamano, Jul 16, 2019
  36. Jonathan NiederJul 17, 2019
  37. SZEDER GáborJul 18, 2019
  38. Junio C HamanoJul 18, 2019
  39. SZEDER GáborJul 18, 2019
  40. Eric SunshineJul 18, 2019
  41. Jonathan NiederJul 19, 2019
  42. Carlo ArenasJul 19, 2019
  43. Junio C HamanoJul 19, 2019
  44. SZEDER GáborJul 27, 2019
  45. Junio C HamanoJul 27, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.