[PATCH v1 1/1] tests: add Git submodule version consistency test

[PATCH v1 1/1] tests: add Git submodule version consistency test

[mailings]

From: Ferry Huberts <ferry.huberts at pelagic.nl>

To ensure the versions are in sync

Signed-off-by: Ferry Huberts <ferry.huberts at pelagic.nl>
---
 tests/t0001-validate-git-versions.sh | 63 ++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100755 tests/t0001-validate-git-versions.sh

diff --git a/tests/t0001-validate-git-versions.sh b/tests/t0001-validate-git-versions.sh
new file mode 100755
index 0000000..22066c0
--- /dev/null
+++ b/tests/t0001-validate-git-versions.sh
@@ -0,0 +1,63 @@
+#!/bin/sh
+
+. ./setup.sh
+
+
+test_versions()
+{
+	curdir=`pwd`
+	cd ..
+
+	gitSubmoduleStatus=`git submodule status git`
+	echo "gitSubmoduleStatus          = $gitSubmoduleStatus"
+
+	gitSubmoduleStatusFirstChar=`echo "$gitSubmoduleStatus" | cut -c -1`
+	echo "gitSubmoduleStatusFirstChar = $gitSubmoduleStatusFirstChar"
+
+	# Fail the test if the Git submodule is not initialised
+	test "$gitSubmoduleStatusFirstChar" == "-" && \
+		echo "The Git submodule is not initialised" && \
+		return 1 
+
+	# Fail the test if the Git submodule is not clean
+	test "$gitSubmoduleStatusFirstChar" != " " && \
+		echo "The Git submodule is not clean" && \
+		return 1
+
+	# Get the SHA1 from the (clean) Git submodule
+	gitSubmoduleVersionSha=`git submodule status git| awk '{ print $1 }' | cut -c 1-`
+	echo "gitSubmoduleVersionSha      = $gitSubmoduleVersionSha"
+
+	# Extract the Git version of the archive from the Makefile
+	regex='^[[:space:]]*GIT_VER[[:space:]]*=[[:space:]]*(.*)[[:space:]]*$'
+	archiveVersion=`grep -E "$regex" Makefile | sed -r "s/$regex/\1/"`
+	echo "archiveVersion              = $archiveVersion"
+
+	# Compare the Git submodule version and the Makefile Git version
+	cd git
+	git diff --exit-code --quiet "$gitSubmoduleVersionSha..v$archiveVersion"
+	diffExitCode=$?
+	echo "diffExitCode                = $diffExitCode"
+	cd ..
+
+	# Return to the original directory
+	cd "$curdir"
+	
+	# Determine exit code
+	test $diffExitCode -ne 0 && return 1
+	return 0
+}
+
+
+prepare_tests 'Check Git version consistency'
+
+git=`which git`
+test -n "$git" || {
+	echo "Skipping test: git not found"
+	tests_done
+	exit
+}
+
+run_test 'test versions' 'test_versions'
+
+tests_done
-- 
1.7.11.7

[PATCH v1 1/1] tests: add Git submodule version consistency test

[john]

On Tue, Mar 19, 2013 at 01:01:45AM +0100, Ferry Huberts wrote:
> From: Ferry Huberts <ferry.huberts at pelagic.nl>
> 
> To ensure the versions are in sync
> 
> Signed-off-by: Ferry Huberts <ferry.huberts at pelagic.nl>
> ---
>  tests/t0001-validate-git-versions.sh | 63 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100755 tests/t0001-validate-git-versions.sh
> 
> diff --git a/tests/t0001-validate-git-versions.sh b/tests/t0001-validate-git-versions.sh
> new file mode 100755
> index 0000000..22066c0
> --- /dev/null
> +++ b/tests/t0001-validate-git-versions.sh
> @@ -0,0 +1,63 @@
> +#!/bin/sh
> +
> +. ./setup.sh
> +
> +
> +test_versions()
> +{
> +	curdir=`pwd`

Please use $(pwd) rather than backticks.

> +	cd ..

If you cd, we should do so in a subshell to avoid leaving $PWD in the
wrong place if we exit early.  I don't see any need to cd for all the
tests here though, the only time we care is when we need to find the Git
submodule version.

> +
> +	gitSubmoduleStatus=`git submodule status git`

Generally variables are written_like_this, not in camelCase.

> +	echo "gitSubmoduleStatus          = $gitSubmoduleStatus"
> +
> +	gitSubmoduleStatusFirstChar=`echo "$gitSubmoduleStatus" | cut -c -1`
> +	echo "gitSubmoduleStatusFirstChar = $gitSubmoduleStatusFirstChar"
> +
> +	# Fail the test if the Git submodule is not initialised
> +	test "$gitSubmoduleStatusFirstChar" == "-" && \
> +		echo "The Git submodule is not initialised" && \
> +		return 1 

I don't know if we need these diagnostics, can't we just get the two
versions and output the difference?

> +
> +	# Fail the test if the Git submodule is not clean
> +	test "$gitSubmoduleStatusFirstChar" != " " && \
> +		echo "The Git submodule is not clean" && \
> +		return 1
> +
> +	# Get the SHA1 from the (clean) Git submodule
> +	gitSubmoduleVersionSha=`git submodule status git| awk '{ print $1 }' | cut -c 1-`
> +	echo "gitSubmoduleVersionSha      = $gitSubmoduleVersionSha"
> +
> +	# Extract the Git version of the archive from the Makefile
> +	regex='^[[:space:]]*GIT_VER[[:space:]]*=[[:space:]]*(.*)[[:space:]]*$'
> +	archiveVersion=`grep -E "$regex" Makefile | sed -r "s/$regex/\1/"`

We can just use sed here, there's no need for grep as well:

    sed -n -e '/^GIT_VER[ 	]*=/ {
        s/^GIT_VER[ 	]*=[ 	]*//
        p
    }' Makefile

(Anyone who puts leading spaces before variables in the Makefile
deserves what they get here ;-) )

> +	echo "archiveVersion              = $archiveVersion"
> +
> +	# Compare the Git submodule version and the Makefile Git version
> +	cd git
> +	git diff --exit-code --quiet "$gitSubmoduleVersionSha..v$archiveVersion"
> +	diffExitCode=$?
> +	echo "diffExitCode                = $diffExitCode"
> +	cd ..
> +
> +	# Return to the original directory
> +	cd "$curdir"
> +	
> +	# Determine exit code
> +	test $diffExitCode -ne 0 && return 1
> +	return 0
> +}
> +
> +
> +prepare_tests 'Check Git version consistency'
> +
> +git=`which git`
> +test -n "$git" || {
> +	echo "Skipping test: git not found"
> +	tests_done
> +	exit
> +}

I don't think any of the tests will run without Git (how can they create
tests repositories?) so this is unnecessary.

> +
> +run_test 'test versions' 'test_versions'

The test should be inline in run_test, there's no need for the
test_versions function.

> +
> +tests_done


I like the idea of this, but I think we should be able to get away with
something much simpler like this:

run_test 'Git versions are consistent' '
	( cd ../git && git describe || echo "No submodule!" ) >tmp/sm_version &&
	sed -n -e "/^GIT_VER[ 	]*=/ {
		s/^GIT_VER[ 	]*=[ 	]*//
		p
	}" >tmp/make_version &&
	diff -u tmp/sm_version tmp/make_version
'


John

[PATCH v1 1/1] tests: add Git submodule version consistency test

[mailings]

On 19/03/13 11:43, John Keeping wrote:
> On Tue, Mar 19, 2013 at 01:01:45AM +0100, Ferry Huberts wrote:
>> From: Ferry Huberts <ferry.huberts at pelagic.nl>
>>
>> To ensure the versions are in sync
>>
>> Signed-off-by: Ferry Huberts <ferry.huberts at pelagic.nl>
>> ---
>>   tests/t0001-validate-git-versions.sh | 63 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 63 insertions(+)
>>   create mode 100755 tests/t0001-validate-git-versions.sh
>>
>> diff --git a/tests/t0001-validate-git-versions.sh b/tests/t0001-validate-git-versions.sh
>> new file mode 100755
>> index 0000000..22066c0
>> --- /dev/null
>> +++ b/tests/t0001-validate-git-versions.sh
>> @@ -0,0 +1,63 @@
>> +#!/bin/sh
>> +
>> +. ./setup.sh
>> +
>> +
>> +test_versions()
>> +{
>> +	curdir=`pwd`
>
> Please use $(pwd) rather than backticks.
>
>> +	cd ..
>
> If you cd, we should do so in a subshell to avoid leaving $PWD in the
> wrong place if we exit early.  I don't see any need to cd for all the
> tests here though, the only time we care is when we need to find the Git
> submodule version.
>
>> +
>> +	gitSubmoduleStatus=`git submodule status git`
>
> Generally variables are written_like_this, not in camelCase.
>
>> +	echo "gitSubmoduleStatus          = $gitSubmoduleStatus"
>> +
>> +	gitSubmoduleStatusFirstChar=`echo "$gitSubmoduleStatus" | cut -c -1`
>> +	echo "gitSubmoduleStatusFirstChar = $gitSubmoduleStatusFirstChar"
>> +
>> +	# Fail the test if the Git submodule is not initialised
>> +	test "$gitSubmoduleStatusFirstChar" == "-" && \
>> +		echo "The Git submodule is not initialised" && \
>> +		return 1
>
> I don't know if we need these diagnostics, can't we just get the two
> versions and output the difference?
>
>> +
>> +	# Fail the test if the Git submodule is not clean
>> +	test "$gitSubmoduleStatusFirstChar" != " " && \
>> +		echo "The Git submodule is not clean" && \
>> +		return 1
>> +
>> +	# Get the SHA1 from the (clean) Git submodule
>> +	gitSubmoduleVersionSha=`git submodule status git| awk '{ print $1 }' | cut -c 1-`
>> +	echo "gitSubmoduleVersionSha      = $gitSubmoduleVersionSha"
>> +
>> +	# Extract the Git version of the archive from the Makefile
>> +	regex='^[[:space:]]*GIT_VER[[:space:]]*=[[:space:]]*(.*)[[:space:]]*$'
>> +	archiveVersion=`grep -E "$regex" Makefile | sed -r "s/$regex/\1/"`
>
> We can just use sed here, there's no need for grep as well:
>
>      sed -n -e '/^GIT_VER[ 	]*=/ {
>          s/^GIT_VER[ 	]*=[ 	]*//
>          p
>      }' Makefile
>
> (Anyone who puts leading spaces before variables in the Makefile
> deserves what they get here ;-) )
>
>> +	echo "archiveVersion              = $archiveVersion"
>> +
>> +	# Compare the Git submodule version and the Makefile Git version
>> +	cd git
>> +	git diff --exit-code --quiet "$gitSubmoduleVersionSha..v$archiveVersion"
>> +	diffExitCode=$?
>> +	echo "diffExitCode                = $diffExitCode"
>> +	cd ..
>> +
>> +	# Return to the original directory
>> +	cd "$curdir"
>> +	
>> +	# Determine exit code
>> +	test $diffExitCode -ne 0 && return 1
>> +	return 0
>> +}
>> +
>> +
>> +prepare_tests 'Check Git version consistency'
>> +
>> +git=`which git`
>> +test -n "$git" || {
>> +	echo "Skipping test: git not found"
>> +	tests_done
>> +	exit
>> +}
>
> I don't think any of the tests will run without Git (how can they create
> tests repositories?) so this is unnecessary.
>
>> +
>> +run_test 'test versions' 'test_versions'
>
> The test should be inline in run_test, there's no need for the
> test_versions function.
>
>> +
>> +tests_done
>
>
> I like the idea of this, but I think we should be able to get away with
> something much simpler like this:
>
> run_test 'Git versions are consistent' '
> 	( cd ../git && git describe || echo "No submodule!" ) >tmp/sm_version &&
> 	sed -n -e "/^GIT_VER[ 	]*=/ {
> 		s/^GIT_VER[ 	]*=[ 	]*//
> 		p
> 	}" >tmp/make_version &&
> 	diff -u tmp/sm_version tmp/make_version
> '
>


Did you test this?
Because the submodule SHA points to the commit, not to the tag.
So the GIT_VER and submodule SHA are actually different.
That is the reason I did the git diff

It appears your script-fu is better than mine ;-)

>
> John
>

-- 
Ferry Huberts

[PATCH v1 1/1] tests: add Git submodule version consistency test

[john]

On Tue, Mar 19, 2013 at 12:10:45PM +0100, Ferry Huberts wrote:
> 
> 
> On 19/03/13 11:43, John Keeping wrote:
> > On Tue, Mar 19, 2013 at 01:01:45AM +0100, Ferry Huberts wrote:
> >> From: Ferry Huberts <ferry.huberts at pelagic.nl>
> >>
> >> To ensure the versions are in sync
> > I like the idea of this, but I think we should be able to get away with
> > something much simpler like this:
> >
> > run_test 'Git versions are consistent' '
> > 	( cd ../git && git describe || echo "No submodule!" ) >tmp/sm_version &&
> > 	sed -n -e "/^GIT_VER[ 	]*=/ {
> > 		s/^GIT_VER[ 	]*=[ 	]*//
> > 		p
> > 	}" >tmp/make_version &&
> > 	diff -u tmp/sm_version tmp/make_version
> > '
> >
> 
> 
> Did you test this?
> Because the submodule SHA points to the commit, not to the tag.

That's why I use "git describe" which will work in this scenario.  Of
course, this doesn't check the submodule version that's committed, just
what is checked out in ./git, but thinking about it I'm not sure if
that's actually a bad thing because it will let the tests run even when
someone is checking a Git update and it will point out a not-up-to-date
submodule anyway.

What I did miss is stripping the initial "v" from the version returned
by git-describe, so I think it can be made more robust by doing this
(tested this time, so I've also fixed "tmp -> trash" and missing
argument to sed):

run_test 'Git versions are consistent' '
	(
		cd ../git &&
		git describe --match "v[0-9]*" || echo "No submodule!"
	) | sed -e "s/^v//" >trash/sm_version &&
	sed -n -e "/^GIT_VER[ 	]*=/ {
		s/^GIT_VER[ 	]*=[ 	]*//
		p
	}" ../Makefile >trash/make_version &&
	diff -u trash/sm_version trash/make_version
'


John

[PATCH v1 1/1] tests: add Git submodule version consistency test

[john]

Subject: [PATCH] tests: check that Git version are in sync

This ensures that the Git version pointed at by the submodule is the
same as the one that will be fetched using "make get-git".

Suggested-by: Ferry Huberts <ferry.huberts at pelagic.nl>
Signed-off-by: John Keeping <john at keeping.me.uk>
---
This is an enhanced version of what I posted earlier, which now checks
that the correct Git version is available even if "make get-git" has
been used to fetch it, by examining Git's GIT-VERSION-FILE.

I also changed the submodule test to check the version of the submodule
in the index rather than whatever happens to be checked out.  I think
this is a more sensible test for people who are experimenting with
different Git versions.

 tests/t0001-validate-git-versions.sh | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100755 tests/t0001-validate-git-versions.sh

diff --git a/tests/t0001-validate-git-versions.sh b/tests/t0001-validate-git-versions.sh
new file mode 100755
index 0000000..3378358
--- /dev/null
+++ b/tests/t0001-validate-git-versions.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+. ./setup.sh
+
+prepare_tests 'Check Git version is correct'
+
+run_test 'extract Git version from Makefile' '
+	sed -n -e "/^GIT_VER[ 	]*=/ {
+		s/^GIT_VER[ 	]*=[ 	]*//
+		p
+	}" ../Makefile >trash/makefile_version
+'
+
+run_test 'test Git version matches Makefile' '
+	( cat ../git/GIT-VERSION-FILE || echo "No GIT-VERSION-FILE" ) |
+	sed -e "s/GIT_VERSION[ 	]*=[ 	]*//" >trash/git_version &&
+	diff -u trash/git_version trash/makefile_version
+'
+
+run_test 'test submodule version matches Makefile' '
+	if ! test -e ../git/.git
+	then
+		echo "git/ is not a Git repository" >&2
+	else
+		(
+			cd .. &&
+			sm_sha1=$(git ls-files --stage -- git |
+				sed -e "s/^[0-9]* \\([0-9a-f]*\\) [0-9]	.*$/\\1/") &&
+			cd git &&
+			git describe --match "v[0-9]*" $sm_sha1
+		) | sed -e "s/^v//" >trash/sm_version &&
+		diff -u trash/sm_version trash/makefile_version
+	fi
+'
+
+tests_done
-- 
1.8.2.279.g744670c

[PATCH v1 1/1] tests: add Git submodule version consistency test

[mailings]

On 19/03/13 21:00, John Keeping wrote:
> Subject: [PATCH] tests: check that Git version are in sync
>
> This ensures that the Git version pointed at by the submodule is the
> same as the one that will be fetched using "make get-git".
>
> Suggested-by: Ferry Huberts <ferry.huberts at pelagic.nl>
> Signed-off-by: John Keeping <john at keeping.me.uk>
> ---
> This is an enhanced version of what I posted earlier, which now checks
> that the correct Git version is available even if "make get-git" has
> been used to fetch it, by examining Git's GIT-VERSION-FILE.

I don't agree with that.
IMHO we should check that the submodule SHA as stored in the repository 
matches the version specified in the Makefile.

Now you check 2 versions that might be 'dirty' against each other; the 
version in the Makefile to whatever someone has put in the git 
directory. That can easily pass testing locally but fail testing once 
committed (commit dirty Makefile but forget to adjust submodule SHA to 
match 'development' git directory unpacked from a random git tar download).

I'd prefer a situation in which we make sure that we test as close to a 
clean checkout as possible, which means that we should use committed 
state (when possible).

>
> I also changed the submodule test to check the version of the submodule
> in the index rather than whatever happens to be checked out.  I think
> this is a more sensible test for people who are experimenting with
> different Git versions.
>
>   tests/t0001-validate-git-versions.sh | 36 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
>   create mode 100755 tests/t0001-validate-git-versions.sh
>
> diff --git a/tests/t0001-validate-git-versions.sh b/tests/t0001-validate-git-versions.sh
> new file mode 100755
> index 0000000..3378358
> --- /dev/null
> +++ b/tests/t0001-validate-git-versions.sh
> @@ -0,0 +1,36 @@
> +#!/bin/sh
> +
> +. ./setup.sh
> +
> +prepare_tests 'Check Git version is correct'
> +
> +run_test 'extract Git version from Makefile' '
> +	sed -n -e "/^GIT_VER[ 	]*=/ {
> +		s/^GIT_VER[ 	]*=[ 	]*//
> +		p
> +	}" ../Makefile >trash/makefile_version
> +'
> +
> +run_test 'test Git version matches Makefile' '
> +	( cat ../git/GIT-VERSION-FILE || echo "No GIT-VERSION-FILE" ) |
> +	sed -e "s/GIT_VERSION[ 	]*=[ 	]*//" >trash/git_version &&
> +	diff -u trash/git_version trash/makefile_version
> +'
> +
> +run_test 'test submodule version matches Makefile' '
> +	if ! test -e ../git/.git
> +	then
> +		echo "git/ is not a Git repository" >&2
> +	else
> +		(
> +			cd .. &&
> +			sm_sha1=$(git ls-files --stage -- git |
> +				sed -e "s/^[0-9]* \\([0-9a-f]*\\) [0-9]	.*$/\\1/") &&
> +			cd git &&
> +			git describe --match "v[0-9]*" $sm_sha1
> +		) | sed -e "s/^v//" >trash/sm_version &&
> +		diff -u trash/sm_version trash/makefile_version
> +	fi
> +'
> +
> +tests_done
>

-- 
Ferry Huberts

[PATCH v1 1/1] tests: add Git submodule version consistency test

[john]

On Tue, Mar 19, 2013 at 09:22:06PM +0100, Ferry Huberts wrote:
> 
> 
> On 19/03/13 21:00, John Keeping wrote:
> > Subject: [PATCH] tests: check that Git version are in sync
> >
> > This ensures that the Git version pointed at by the submodule is the
> > same as the one that will be fetched using "make get-git".
> >
> > Suggested-by: Ferry Huberts <ferry.huberts at pelagic.nl>
> > Signed-off-by: John Keeping <john at keeping.me.uk>
> > ---
> > This is an enhanced version of what I posted earlier, which now checks
> > that the correct Git version is available even if "make get-git" has
> > been used to fetch it, by examining Git's GIT-VERSION-FILE.
> 
> I don't agree with that.
> IMHO we should check that the submodule SHA as stored in the repository 
> matches the version specified in the Makefile.
> 
> Now you check 2 versions that might be 'dirty' against each other; the 
> version in the Makefile to whatever someone has put in the git 
> directory. That can easily pass testing locally but fail testing once 
> committed (commit dirty Makefile but forget to adjust submodule SHA to 
> match 'development' git directory unpacked from a random git tar download).

The test using GIT-VERSION-FILE will catch this, that's generated during
the Git build process to match precisely what is built (including a
dirty flag).

> I'd prefer a situation in which we make sure that we test as close to a 
> clean checkout as possible, which means that we should use committed 
> state (when possible).

The second check is doing that (or close to it, we're checking staged
state rather than committed state, which is better in the case that
you've got a commit ready and want to sanity check it before typing "git
commit").

I think the combination of these two tests gives what you want, the
GIT-VERSION-FILE tests that the version of Git being used to build CGit
matches the Makefile and the "git ls-files --stage -- git" test checks
that what's in the index matches.  We could change that to "git ls-tree
HEAD -- git" but I think that is unnecessarily annoying for people
trying out new Git versions.

> >
> > I also changed the submodule test to check the version of the submodule
> > in the index rather than whatever happens to be checked out.  I think
> > this is a more sensible test for people who are experimenting with
> > different Git versions.
> >
> >   tests/t0001-validate-git-versions.sh | 36 ++++++++++++++++++++++++++++++++++++
> >   1 file changed, 36 insertions(+)
> >   create mode 100755 tests/t0001-validate-git-versions.sh
> >
> > diff --git a/tests/t0001-validate-git-versions.sh b/tests/t0001-validate-git-versions.sh
> > new file mode 100755
> > index 0000000..3378358
> > --- /dev/null
> > +++ b/tests/t0001-validate-git-versions.sh
> > @@ -0,0 +1,36 @@
> > +#!/bin/sh
> > +
> > +. ./setup.sh
> > +
> > +prepare_tests 'Check Git version is correct'
> > +
> > +run_test 'extract Git version from Makefile' '
> > +	sed -n -e "/^GIT_VER[ 	]*=/ {
> > +		s/^GIT_VER[ 	]*=[ 	]*//
> > +		p
> > +	}" ../Makefile >trash/makefile_version
> > +'
> > +
> > +run_test 'test Git version matches Makefile' '
> > +	( cat ../git/GIT-VERSION-FILE || echo "No GIT-VERSION-FILE" ) |
> > +	sed -e "s/GIT_VERSION[ 	]*=[ 	]*//" >trash/git_version &&
> > +	diff -u trash/git_version trash/makefile_version
> > +'
> > +
> > +run_test 'test submodule version matches Makefile' '
> > +	if ! test -e ../git/.git
> > +	then
> > +		echo "git/ is not a Git repository" >&2
> > +	else
> > +		(
> > +			cd .. &&
> > +			sm_sha1=$(git ls-files --stage -- git |
> > +				sed -e "s/^[0-9]* \\([0-9a-f]*\\) [0-9]	.*$/\\1/") &&
> > +			cd git &&
> > +			git describe --match "v[0-9]*" $sm_sha1
> > +		) | sed -e "s/^v//" >trash/sm_version &&
> > +		diff -u trash/sm_version trash/makefile_version
> > +	fi
> > +'
> > +
> > +tests_done
> >
> 
> -- 
> Ferry Huberts
> 
> _______________________________________________
> cgit mailing list
> cgit at hjemli.net
> http://hjemli.net/mailman/listinfo/cgit

[PATCH v1 1/1] tests: add Git submodule version consistency test

[mailings]

On 19/03/13 21:33, John Keeping wrote:
> On Tue, Mar 19, 2013 at 09:22:06PM +0100, Ferry Huberts wrote:
>>
>>
>> On 19/03/13 21:00, John Keeping wrote:
>>> Subject: [PATCH] tests: check that Git version are in sync
>>>
>>> This ensures that the Git version pointed at by the submodule is the
>>> same as the one that will be fetched using "make get-git".
>>>
>>> Suggested-by: Ferry Huberts <ferry.huberts at pelagic.nl>
>>> Signed-off-by: John Keeping <john at keeping.me.uk>
>>> ---
>>> This is an enhanced version of what I posted earlier, which now checks
>>> that the correct Git version is available even if "make get-git" has
>>> been used to fetch it, by examining Git's GIT-VERSION-FILE.
>>
>> I don't agree with that.
>> IMHO we should check that the submodule SHA as stored in the repository
>> matches the version specified in the Makefile.
>>
>> Now you check 2 versions that might be 'dirty' against each other; the
>> version in the Makefile to whatever someone has put in the git
>> directory. That can easily pass testing locally but fail testing once
>> committed (commit dirty Makefile but forget to adjust submodule SHA to
>> match 'development' git directory unpacked from a random git tar download).
>
> The test using GIT-VERSION-FILE will catch this, that's generated during
> the Git build process to match precisely what is built (including a
> dirty flag).
>

Goes to prove that I don't know enough about the Git build process :-)

>> I'd prefer a situation in which we make sure that we test as close to a
>> clean checkout as possible, which means that we should use committed
>> state (when possible).
>
> The second check is doing that (or close to it, we're checking staged
> state rather than committed state, which is better in the case that
> you've got a commit ready and want to sanity check it before typing "git
> commit").

Fair enough.


>
> I think the combination of these two tests gives what you want, the
> GIT-VERSION-FILE tests that the version of Git being used to build CGit
> matches the Makefile and the "git ls-files --stage -- git" test checks
> that what's in the index matches.  We could change that to "git ls-tree
> HEAD -- git" but I think that is unnecessarily annoying for people
> trying out new Git versions.
>

Agree


Reviewed-by: Ferry Huberts <ferry.huberts at pelagic.nl>


>>>
>>> I also changed the submodule test to check the version of the submodule
>>> in the index rather than whatever happens to be checked out.  I think
>>> this is a more sensible test for people who are experimenting with
>>> different Git versions.
>>>
>>>    tests/t0001-validate-git-versions.sh | 36 ++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 36 insertions(+)
>>>    create mode 100755 tests/t0001-validate-git-versions.sh
>>>
>>> diff --git a/tests/t0001-validate-git-versions.sh b/tests/t0001-validate-git-versions.sh
>>> new file mode 100755
>>> index 0000000..3378358
>>> --- /dev/null
>>> +++ b/tests/t0001-validate-git-versions.sh
>>> @@ -0,0 +1,36 @@
>>> +#!/bin/sh
>>> +
>>> +. ./setup.sh
>>> +
>>> +prepare_tests 'Check Git version is correct'
>>> +
>>> +run_test 'extract Git version from Makefile' '
>>> +	sed -n -e "/^GIT_VER[ 	]*=/ {
>>> +		s/^GIT_VER[ 	]*=[ 	]*//
>>> +		p
>>> +	}" ../Makefile >trash/makefile_version
>>> +'
>>> +
>>> +run_test 'test Git version matches Makefile' '
>>> +	( cat ../git/GIT-VERSION-FILE || echo "No GIT-VERSION-FILE" ) |
>>> +	sed -e "s/GIT_VERSION[ 	]*=[ 	]*//" >trash/git_version &&
>>> +	diff -u trash/git_version trash/makefile_version
>>> +'
>>> +
>>> +run_test 'test submodule version matches Makefile' '
>>> +	if ! test -e ../git/.git
>>> +	then
>>> +		echo "git/ is not a Git repository" >&2
>>> +	else
>>> +		(
>>> +			cd .. &&
>>> +			sm_sha1=$(git ls-files --stage -- git |
>>> +				sed -e "s/^[0-9]* \\([0-9a-f]*\\) [0-9]	.*$/\\1/") &&
>>> +			cd git &&
>>> +			git describe --match "v[0-9]*" $sm_sha1
>>> +		) | sed -e "s/^v//" >trash/sm_version &&
>>> +		diff -u trash/sm_version trash/makefile_version
>>> +	fi
>>> +'
>>> +
>>> +tests_done
>>>
>>
>> --
>> Ferry Huberts
>>
>> _______________________________________________
>> cgit mailing list
>> cgit at hjemli.net
>> http://hjemli.net/mailman/listinfo/cgit

-- 
Ferry Huberts

[PATCH v1 1/1] tests: add Git submodule version consistency test

[Jason]

This is the correct way indeed to do things. Thanks for the great
patch John. Merged to master.

[PATCH v1 1/1] tests: add Git submodule version consistency test

[mailings]

On 20/03/13 21:09, Jason A. Donenfeld wrote:
> This is the correct way indeed to do things. Thanks for the great
> patch John. Merged to master.
>



you're welcome...

-- 
Ferry Huberts

[PATCH v1 1/1] tests: add Git submodule version consistency test

[Jason]

On Wed, Mar 20, 2013 at 9:13 PM, Ferry Huberts <mailings at hupie.com> wrote:
>
> you're welcome...

GAH!

Sorry Ferry. YES THANK YOU FERRY TOO AND ESPECIALLY FOR THE ORIGINAL
IDEA! My apologies.

[PATCH v1 1/1] tests: add Git submodule version consistency test

[mailings]

On 20/03/13 21:15, Jason A. Donenfeld wrote:
> On Wed, Mar 20, 2013 at 9:13 PM, Ferry Huberts <mailings at hupie.com> wrote:
>>
>> you're welcome...
>
> GAH!
>
> Sorry Ferry. YES THANK YOU FERRY TOO AND ESPECIALLY FOR THE ORIGINAL
> IDEA! My apologies.
>

Actually, the original idea was from Lars.
I implemented the first patch, but John's script-fu was better

-- 
Ferry Huberts