[PATCH v2 0/4] Add Travis CI support

[PATCH v2 0/4] Add Travis CI support

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

diff to v1:
* improve readabilty of Travis "before_script" section
* make OS X Perforce brew robust against changing Perforce builds [1]
* add a prereq to detect OS X in tests
* fix t9815 git-p4-submit-fail test case on OS X
* disable t9819 git-p4-case-folding test on OS X

Thanks Junio for the feedback. You are right, I should have fixed the failing
tests in the first place. With this patch all tests pass on Linux and OS X:
https://travis-ci.org/larsxschneider/git/builds/83575208

In a subsequent patch I plan to:
* add cvs to run t94?? tests
* add svn to run t91?? tests
* add apache to run 5539, 5550, and 5561
* investigate if it is possible to run t1509 root worktree test
* investigate if it is possible to add jgit to run t5310

Plus I have the following questions:
* Can you explain to me how the t7006 page tests should be executed?
* Should we enable EXPENSIVE, CLONE_2GB, and USE_LIBPCRE flag?

Thanks,
Lars

[1] This is a workaround. I am in contact about the issue with the homebrew
maintainers and maybe we can make this easier soon:
https://github.com/Homebrew/homebrew-binary/pull/267#issuecomment-145317114

Lars Schneider (4):
  Add Travis CI support
  Add prereq to detect OS X
  git-p4: Fix t9815 git-p4-submit-fail test case on OS X
  git-p4: Disable t9819 git-p4-case-folding test on OS X

 .travis.yml                    | 31 +++++++++++++++++++++++++++++++
 t/t9815-git-p4-submit-fail.sh  |  8 ++++++--
 t/t9819-git-p4-case-folding.sh |  5 +++++
 t/test-lib.sh                  |  5 ++++-
 4 files changed, 46 insertions(+), 3 deletions(-)
 create mode 100644 .travis.yml

--
2.5.1

[PATCH v2 1/4] Add Travis CI support

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

The tests are executed on "Ubuntu 12.04 LTS Server Edition 64 bit" and
on "OS X Mavericks" using gcc and clang.

Perforce and Git-LFS are installed and therefore available for the
respective tests.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 .travis.yml | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 .travis.yml

diff --git a/.travis.yml b/.travis.yml
new file mode 100644
index 0000000..8a29dd6
--- /dev/null
+++ b/.travis.yml
@@ -0,0 +1,31 @@
+language: c
+
+os:
+  - linux
+  - osx
+
+compiler:
+  - clang
+  - gcc
+
+before_script:
+  - >
+    if [ ${TRAVIS_OS_NAME:-'linux'} = 'linux' ]; then
+      wget -q https://package.perforce.com/perforce.pubkey -O - | sudo apt-key add -
+      echo 'deb http://package.perforce.com/apt/ubuntu precise release' | sudo tee -a /etc/apt/sources.list
+      wget -q https://packagecloud.io/gpg.key -O - | sudo apt-key add -
+      echo 'deb https://packagecloud.io/github/git-lfs/debian/ jessie main' | sudo tee -a /etc/apt/sources.list
+      sudo apt-get update -qq
+      sudo apt-get install -y apt-transport-https
+      sudo apt-get install perforce-server git-lfs
+    elif [ ${TRAVIS_OS_NAME:-'linux'} = 'osx' ]; then
+      brew update
+      brew tap homebrew/binary
+      P4_SERVER_SHA=$(brew fetch --force perforce-server 2>&1 | grep ^SHA256: | cut -d ' ' -f 2)
+      P4_SHA=$(brew fetch --force perforce 2>&1 | grep ^SHA256: | cut -d ' ' -f 2)
+      sed -E -i.bak "s/sha256 \"[0-9a-f]{64}\"/sha256 \"$P4_SERVER_SHA\"/g" /usr/local/Library/Taps/homebrew/homebrew-binary/perforce-server.rb
+      sed -E -i.bak "s/sha256 \"[0-9a-f]{64}\"/sha256 \"$P4_SHA\"/g" /usr/local/Library/Taps/homebrew/homebrew-binary/perforce.rb
+      brew install git-lfs perforce-server perforce
+    fi
+
+install: make configure
-- 
2.5.1

Re: [PATCH v2 1/4] Add Travis CI support

[Johannes Schindelin]

Hi Lars,

On 2015-10-04 20:06, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
> 
> The tests are executed on "Ubuntu 12.04 LTS Server Edition 64 bit" and
> on "OS X Mavericks" using gcc and clang.

I think that the word "currently" is missing from this description, as it is not something this patch controls (but Travis CI's infrastructure).

> Perforce and Git-LFS are installed and therefore available for the
> respective tests.

This is *the* bonus of using Travis CI over BuildHive (which is enabled in Git for Windows' fork of git/git): you can install additional dependencies for more extensive testing. Oh, and it tests with clang and on MacOSX, too, which would reduce the number of fixer-upper patches when git/git introduced yet Another Change That Works Only On Linux.

> +before_script:
> +  - >
> +    if [ ${TRAVIS_OS_NAME:-'linux'} = 'linux' ]; then

Maybe for readability use

    case "${TRAVIS_OS_NAME:-linux}" in
    linux)
        ...

?

Thanks,
Dscho

[PATCH v2 2/4] Add prereq to detect OS X

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

Some Unix tools differ between Linux and Mac OS X. Add this prereq to
detect OS X and handle it appropriately.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 t/test-lib.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 16c4d7b..726af40 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -913,7 +913,7 @@ yes () {
 	done
 }
 
-# Fix some commands on Windows
+# Fix some commands on Windows and OS X
 case $(uname -s) in
 *MINGW*)
 	# Windows has its own (incompatible) sort and find
@@ -946,6 +946,9 @@ case $(uname -s) in
 	test_set_prereq SED_STRIPS_CR
 	test_set_prereq GREP_STRIPS_CR
 	;;
+*Darwin*)
+	test_set_prereq DARWIN
+	;;
 *)
 	test_set_prereq POSIXPERM
 	test_set_prereq BSLASHPSPEC
-- 
2.5.1

Re: [PATCH v2 2/4] Add prereq to detect OS X

[Junio C Hamano]

larsxschneider@gmail.com writes:

> From: Lars Schneider <larsxschneider@gmail.com>
>
> Some Unix tools differ between Linux and Mac OS X. Add this prereq to
> detect OS X and handle it appropriately.

If my comment on 4/4 turns out to be to the point, then we do not
need this, I presume?  In general, we really should think three
times before adding prerequisite based on the platform.  A prereq
based on a specific feature of the platform is often what we want
instead.

Re: [PATCH v2 2/4] Add prereq to detect OS X

[Lars Schneider]

On 04 Oct 2015, at 11:28, Junio C Hamano <gitster@pobox.com> wrote:

> larsxschneider@gmail.com writes:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Some Unix tools differ between Linux and Mac OS X. Add this prereq to
>> detect OS X and handle it appropriately.
> 
> If my comment on 4/4 turns out to be to the point, then we do not
> need this, I presume?  In general, we really should think three
> times before adding prerequisite based on the platform.  A prereq
> based on a specific feature of the platform is often what we want
> instead.
Agreed. I will remove this commit in the next roll.

Thanks,
Lars

[PATCH v2 3/4] git-p4: Fix t9815 git-p4-submit-fail test case on OS X

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

The stats command works differently on OS X compared to Linux. Detect
OS X and execute the appropriate assertions.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 t/t9815-git-p4-submit-fail.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/t/t9815-git-p4-submit-fail.sh b/t/t9815-git-p4-submit-fail.sh
index 4cff6a7..520b812 100755
--- a/t/t9815-git-p4-submit-fail.sh
+++ b/t/t9815-git-p4-submit-fail.sh
@@ -417,8 +417,12 @@ test_expect_success 'cleanup chmod after submit cancel' '
 		! p4 fstat -T action text &&
 		test_path_is_file text+x &&
 		! p4 fstat -T action text+x &&
-		if test_have_prereq !CYGWIN
-		then
+		if test_have_prereq CYGWIN; then
+			: # NOOP
+		elif test_have_prereq DARWIN; then
+			stat -f %Sp text | egrep ^-r-- &&
+			stat -f %Sp text+x | egrep ^-r-x
+		else
 			stat --format=%A text | egrep ^-r-- &&
 			stat --format=%A text+x | egrep ^-r-x
 		fi
-- 
2.5.1

Re: [PATCH v2 3/4] git-p4: Fix t9815 git-p4-submit-fail test case on OS X

[Junio C Hamano]

larsxschneider@gmail.com writes:

> +		if test_have_prereq CYGWIN; then
> +			: # NOOP
> +		elif test_have_prereq DARWIN; then
> +			stat -f %Sp text | egrep ^-r-- &&
> +			stat -f %Sp text+x | egrep ^-r-x
> +		else
>  			stat --format=%A text | egrep ^-r-- &&
>  			stat --format=%A text+x | egrep ^-r-x
>  		fi

Not a new problem but why do we need "stat" here?

Shouldn't "test -r", "! test -x", and their usual friends be
sufficient for the purpose of the test and are more portable?

Re: [PATCH v2 3/4] git-p4: Fix t9815 git-p4-submit-fail test case on OS X

[Lars Schneider]

On 04 Oct 2015, at 11:23, Junio C Hamano <gitster@pobox.com> wrote:

> larsxschneider@gmail.com writes:
> 
>> +		if test_have_prereq CYGWIN; then
>> +			: # NOOP
>> +		elif test_have_prereq DARWIN; then
>> +			stat -f %Sp text | egrep ^-r-- &&
>> +			stat -f %Sp text+x | egrep ^-r-x
>> +		else
>> 			stat --format=%A text | egrep ^-r-- &&
>> 			stat --format=%A text+x | egrep ^-r-x
>> 		fi
> 
> Not a new problem but why do we need "stat" here?
> 
> Shouldn't "test -r", "! test -x", and their usual friends be
> sufficient for the purpose of the test and are more portable?

Good question. The stat call was introduced with df9c545 by Pete Wyckoff.
@Pete, @Luke: Are you aware of any particular reason for stat?

Thanks,
Lars

Re: [PATCH v2 3/4] git-p4: Fix t9815 git-p4-submit-fail test case on OS X

[Pete Wyckoff]

larsxschneider@gmail.com wrote on Sun, 04 Oct 2015 11:44 -0700:
> 
> On 04 Oct 2015, at 11:23, Junio C Hamano <gitster@pobox.com> wrote:
> 
> > larsxschneider@gmail.com writes:
> > 
> >> +		if test_have_prereq CYGWIN; then
> >> +			: # NOOP
> >> +		elif test_have_prereq DARWIN; then
> >> +			stat -f %Sp text | egrep ^-r-- &&
> >> +			stat -f %Sp text+x | egrep ^-r-x
> >> +		else
> >> 			stat --format=%A text | egrep ^-r-- &&
> >> 			stat --format=%A text+x | egrep ^-r-x
> >> 		fi
> > 
> > Not a new problem but why do we need "stat" here?
> > 
> > Shouldn't "test -r", "! test -x", and their usual friends be
> > sufficient for the purpose of the test and are more portable?
> 
> Good question. The stat call was introduced with df9c545 by Pete Wyckoff.
> @Pete, @Luke: Are you aware of any particular reason for stat?

I think you could do this all with test. The key is to make
sure the files are readable, not writable, and either executable
or not. Cygwin and darwin oddities were not on my radar 3 years ago.

See also 4cea4d6 (git p4 test: use test_chmod for cygwin,
2013-01-26) for the description I wrote about what this test is
trying to verify.

		-- Pete

Re: [PATCH v2 3/4] git-p4: Fix t9815 git-p4-submit-fail test case on OS X

[Torsten Bögershausen]

On 04.10.15 20:06, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
> 
> The stats command works differently on OS X compared to Linux. Detect
> OS X and execute the appropriate assertions.
> 
Is there a special need to use the stat() function at all ?

That's what I read in t1301-shared-repo.sh:

modebits () {
	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
}

[PATCH v2 4/4] git-p4: Disable t9819 git-p4-case-folding test on OS X

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

The OS X file system is case insensitive by default. Consequently this
test does not apply.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 t/t9819-git-p4-case-folding.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh
index 78f1d0f..c69ae47 100755
--- a/t/t9819-git-p4-case-folding.sh
+++ b/t/t9819-git-p4-case-folding.sh
@@ -4,6 +4,11 @@ test_description='interaction with P4 case-folding'
 
 . ./lib-git-p4.sh
 
+if test_have_prereq DARWIN; then
+	skip_all='skipping P4 case-folding tests; OS X file system is case insensitive by default'
+	test_done
+fi
+
 test_expect_success 'start p4d with case folding enabled' '
 	start_p4d -C1
 '
-- 
2.5.1

Re: [PATCH v2 4/4] git-p4: Disable t9819 git-p4-case-folding test on OS X

[Junio C Hamano]

larsxschneider@gmail.com writes:

> From: Lars Schneider <larsxschneider@gmail.com>
>
> The OS X file system is case insensitive by default. Consequently this
> test does not apply.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  t/t9819-git-p4-case-folding.sh | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh
> index 78f1d0f..c69ae47 100755
> --- a/t/t9819-git-p4-case-folding.sh
> +++ b/t/t9819-git-p4-case-folding.sh
> @@ -4,6 +4,11 @@ test_description='interaction with P4 case-folding'
>  
>  . ./lib-git-p4.sh
>  
> +if test_have_prereq DARWIN; then
> +	skip_all='skipping P4 case-folding tests; OS X file system is case insensitive by default'
> +	test_done
> +fi

Makes one wonder what should happen on Windows, or vfat mounted on
Linux for that matter.  IOW, shouldn't the prerequisite be more like
"do not run any of these tests if the filesystem does not allow us
to have two files in different cases at the same time"?

Perhaps

        if ! test_have_prereq CASE_INSENSITIVE_FS
        then
                skip_all=...
                test_done
        fi

instead, or something?

Re: [PATCH v2 4/4] git-p4: Disable t9819 git-p4-case-folding test on OS X

[Lars Schneider]

On 04 Oct 2015, at 11:26, Junio C Hamano <gitster@pobox.com> wrote:

> larsxschneider@gmail.com writes:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> The OS X file system is case insensitive by default. Consequently this
>> test does not apply.
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> t/t9819-git-p4-case-folding.sh | 5 +++++
>> 1 file changed, 5 insertions(+)
>> 
>> diff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh
>> index 78f1d0f..c69ae47 100755
>> --- a/t/t9819-git-p4-case-folding.sh
>> +++ b/t/t9819-git-p4-case-folding.sh
>> @@ -4,6 +4,11 @@ test_description='interaction with P4 case-folding'
>> 
>> . ./lib-git-p4.sh
>> 
>> +if test_have_prereq DARWIN; then
>> +	skip_all='skipping P4 case-folding tests; OS X file system is case insensitive by default'
>> +	test_done
>> +fi
> 
> Makes one wonder what should happen on Windows, or vfat mounted on
> Linux for that matter.  IOW, shouldn't the prerequisite be more like
> "do not run any of these tests if the filesystem does not allow us
> to have two files in different cases at the same time"?
> 
> Perhaps
> 
>        if ! test_have_prereq CASE_INSENSITIVE_FS
>        then
>                skip_all=...
>                test_done
>        fi
> 
> instead, or something?
Agreed! Although I think the “!” in the if clause is not correct.
By the way... what formatting should I use?

if foo
then
  bar

or

if foo; then
  bar

I think the latter is more used in the code base.

- Lars

Re: [PATCH v2 4/4] git-p4: Disable t9819 git-p4-case-folding test on OS X

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

> By the way... what formatting should I use?
>
> if foo
> then
>   bar
>
> or
>
> if foo; then
>   bar
>
> I think the latter is more used in the code base.

I've allowed the latter in fringe scripts (i.e. the ones that I do
not care too deeply) over time, but Documentation/CodingGuidelines
is very clear about it.

 - Do not write control structures on a single line with semicolon.

Re: [PATCH v2 0/4] Add Travis CI support

[Johannes Schindelin]

Hi Lars,

On 2015-10-04 20:06, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
> 
> diff to v1:
> * improve readabilty of Travis "before_script" section
> * make OS X Perforce brew robust against changing Perforce builds [1]
> * add a prereq to detect OS X in tests
> * fix t9815 git-p4-submit-fail test case on OS X
> * disable t9819 git-p4-case-folding test on OS X

Nice! I think that this already shows the promise of using Travis CI: more thorough testing, earlier warning when new changes are not quite as portable as we thought they were.

> In a subsequent patch I plan to:
> * add cvs to run t94?? tests
> * add svn to run t91?? tests
> * add apache to run 5539, 5550, and 5561
> * investigate if it is possible to run t1509 root worktree test
> * investigate if it is possible to add jgit to run t5310

Very nice!

Thanks,
Johannes