Re: [PATCH v4 04/14] Add new simplified git-remote-testgit
On 11/02/2012 03:02 AM, Felipe Contreras wrote:
It's way simpler. It exerceises the same features of remote helpers.
It's easy to read and understand. It doesn't depend on python.
It does _not_ exercise the python remote helper framework; there's
another tool and another test for that.
For now let's just copy the old remote-helpers test script, although
some of those tests don't make sense for this testgit (they still pass).
In addition, this script would be able to test other features not
currently being tested.
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
Documentation/git-remote-testgit.txt | 2 +-
git-remote-testgit | 62
t/t5801-remote-helpers.sh| 134
+++
3 files changed, 197 insertions(+), 1 deletion(-)
create mode 100755 git-remote-testgit
create mode 100755 t/t5801-remote-helpers.sh
diff --git a/git-remote-testgit b/git-remote-testgit
new file mode 100755
index 000..6650402
--- /dev/null
+++ b/git-remote-testgit
@@ -0,0 +1,62 @@
+#!/bin/bash
I think git can't assume the existence of bash unconditionally, neither
in its scripts, nor in its tests (the exception being the tests on
bash completion, of course). This script probably need to be re-written
to be a valid POSIX shell script.
It almost is, anyway, apart from the nits below ...
+# Copyright (c) 2012 Felipe Contreras
+
+alias=$1
Just FYI: the double quoting here (and in several variable assignments
below) is redundant. You can portably write it as:
alias=$1
and still be safe in the face of spaces and metacharacters in $1.
I'm not sure whether the Git coding guidelines suggest the use of
quoting in this situation though; if this is the case, feel free
to disregard my observation.
+url=$2
+
+# huh?
+url=${url#file://}
+
+dir=$GIT_DIR/testgit/$alias
+prefix=refs/testgit/$alias
+refspec=refs/heads/*:${prefix}/heads/*
+
+gitmarks=$dir/git.marks
+testgitmarks=$dir/testgit.marks
+
+export GIT_DIR=$url/.git
+
I believe this should be rewritten as:
GIT_DIR=$url/.git; export GIT_DIR
in order to be portable to all the POSIX shells targeted by Git.
+mkdir -p $dir
+
+test -e $gitmarks || echo -n $gitmarks
+test -e $testgitmarks || echo -n $testgitmarks
+
The '-n' option to echo is not portable. To create an empty
file, you can just use
: file
or
true file
+while read line; do
+case $line in
Useless double quoting (my previous observation about Git coding
guidelines applies here as well, of course).
+capabilities)
+echo 'import'
+echo 'export'
+echo refspec $refspec
+echo *import-marks $gitmarks
+echo *export-marks $gitmarks
+echo
+;;
+list)
+git for-each-ref --format='? %(refname)' 'refs/heads/'
+head=$(git symbolic-ref HEAD)
+echo @$head HEAD
+echo
+;;
+import*)
+# read all import lines
+while true; do
+ref=${line#* }
+refs=$refs $ref
+read line
+test ${line%% *} != import break
+done
+
+echo feature import-marks=$gitmarks
+echo feature export-marks=$gitmarks
+git fast-export --use-done-feature
--{import,export}-marks=$testgitmarks $refs | \
Better avoid the tricky {foo,bar} bashism:
git fast-export --use-done-feature \
--import-marks=$testgitmarks \
--export-marks=$testgitmarks \
$refs | \
+sed -e s#refs/heads/#${prefix}/heads/#g
+;;
+export)
+git fast-import --{import,export}-marks=$testgitmarks --quiet
Here too.
+echo
+;;
+'')
+exit
I'd put an explicit exit status here, for clarity (this is a matter
of personal preference, so feel free to disregard this nit).
+;;
+esac
+done
Regards,
Stefano
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 04/14] Add new simplified git-remote-testgit
On Fri, Nov 02, 2012 at 02:55:41PM +0100, Stefano Lattarini wrote: --- /dev/null +++ b/git-remote-testgit @@ -0,0 +1,62 @@ +#!/bin/bash I think git can't assume the existence of bash unconditionally, neither in its scripts, nor in its tests (the exception being the tests on bash completion, of course). This script probably need to be re-written to be a valid POSIX shell script. No, we can't assume bash. But this is replacing a script written in python, which we also can't assume. So if it were optional, and skipped gracefully when bash was not available, that would be OK. Of course if it could be done in portable bourne shell, that would be even better (I haven't looked closely, but your comments all seem reasonable). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 04/14] Add new simplified git-remote-testgit
On Fri, Nov 2, 2012 at 2:55 PM, Stefano Lattarini
stefano.lattar...@gmail.com wrote:
+#!/bin/bash
I think git can't assume the existence of bash unconditionally, neither
in its scripts, nor in its tests (the exception being the tests on
bash completion, of course). This script probably need to be re-written
to be a valid POSIX shell script.
Well, this is a _reference_ script, and that is used only for testing
purposes. The test itself can be like the bash completion tests, and
simply be skipped.
The reason I chose bash is because associative arrays, which you see
in a later patch.
It almost is, anyway, apart from the nits below ...
+# Copyright (c) 2012 Felipe Contreras
+
+alias=$1
Just FYI: the double quoting here (and in several variable assignments
below) is redundant. You can portably write it as:
alias=$1
and still be safe in the face of spaces and metacharacters in $1.
I'm not sure whether the Git coding guidelines suggest the use of
quoting in this situation though; if this is the case, feel free
to disregard my observation.
What happens when you call this with:
./script alias with spaces
+url=$2
+
+# huh?
+url=${url#file://}
+
+dir=$GIT_DIR/testgit/$alias
+prefix=refs/testgit/$alias
+refspec=refs/heads/*:${prefix}/heads/*
+
+gitmarks=$dir/git.marks
+testgitmarks=$dir/testgit.marks
+
+export GIT_DIR=$url/.git
+
I believe this should be rewritten as:
GIT_DIR=$url/.git; export GIT_DIR
in order to be portable to all the POSIX shells targeted by Git.
_If_ we want this as POSIX, yeah.
+mkdir -p $dir
+
+test -e $gitmarks || echo -n $gitmarks
+test -e $testgitmarks || echo -n $testgitmarks
+
The '-n' option to echo is not portable. To create an empty
file, you can just use
: file
or
true file
All right, thanks.
+while read line; do
+case $line in
Useless double quoting (my previous observation about Git coding
guidelines applies here as well, of course).
What if line has multiple spaces? To me it makes sense to quote it.
+echo feature import-marks=$gitmarks
+echo feature export-marks=$gitmarks
+git fast-export --use-done-feature
--{import,export}-marks=$testgitmarks $refs | \
Better avoid the tricky {foo,bar} bashism:
git fast-export --use-done-feature \
--import-marks=$testgitmarks \
--export-marks=$testgitmarks \
$refs | \
If that's what we want, yeah.
--
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 04/14] Add new simplified git-remote-testgit
On 11/02/2012 04:42 PM, Felipe Contreras wrote:
On Fri, Nov 2, 2012 at 2:55 PM, Stefano Lattarini
stefano.lattar...@gmail.com wrote:
+#!/bin/bash
I think git can't assume the existence of bash unconditionally, neither
in its scripts, nor in its tests (the exception being the tests on
bash completion, of course). This script probably need to be re-written
to be a valid POSIX shell script.
Well, this is a _reference_ script, and that is used only for testing
purposes. The test itself can be like the bash completion tests, and
simply be skipped.
The reason I chose bash is because associative arrays, which you see
in a later patch.
It almost is, anyway, apart from the nits below ...
+# Copyright (c) 2012 Felipe Contreras
+
+alias=$1
Just FYI: the double quoting here (and in several variable assignments
below) is redundant. You can portably write it as:
alias=$1
and still be safe in the face of spaces and metacharacters in $1.
I'm not sure whether the Git coding guidelines suggest the use of
quoting in this situation though; if this is the case, feel free
to disregard my observation.
What happens when you call this with:
./script alias with spaces
'$alias' will correctly expand to alias with spaces. Try out:
$ sh -c 'alias=$1; echo $alias' dummy '1 2*3'
1 2*3
This works consistently with every known shell (even non-POSIX
relics like Solaris /bin/sh).
+url=$2
+
+# huh?
+url=${url#file://}
+
+dir=$GIT_DIR/testgit/$alias
+prefix=refs/testgit/$alias
+refspec=refs/heads/*:${prefix}/heads/*
+
+gitmarks=$dir/git.marks
+testgitmarks=$dir/testgit.marks
+
+export GIT_DIR=$url/.git
+
I believe this should be rewritten as:
GIT_DIR=$url/.git; export GIT_DIR
in order to be portable to all the POSIX shells targeted by Git.
_If_ we want this as POSIX, yeah.
Why don't we? Why add an extra requirement for a test that
1. can be easily written in POSIX shell, and
2. tests a feature that doesn't require bash to work (unless
I'm sorely mistaken, that is)?
Honest question. But of course, if the Git active contributors
deem the extra requirement (which is not an invasive one, given
how often bash is installed even on non-Linux systems) acceptable
in order to have the test case simpler and clearer, feel free to
disregard all my observations in this thread.
+mkdir -p $dir
+
+test -e $gitmarks || echo -n $gitmarks
+test -e $testgitmarks || echo -n $testgitmarks
+
The '-n' option to echo is not portable. To create an empty
file, you can just use
: file
or
true file
All right, thanks.
+while read line; do
+case $line in
Useless double quoting (my previous observation about Git coding
guidelines applies here as well, of course).
What if line has multiple spaces?
Still no problem, as in the case of the 'alias=$1' assignment before:
$ sh -c 'case $1 in *x x*) echo ok;; *) exit 1;; esac' dummy 'x x'
ok
To me it makes sense to quote it.
Surely it doesn't cause any problem to over-quote in this case;
it's better than risking to under-quote in other. I just pointed
out that the quoting it's not really necessary, in case you weren't
aware of that.
+echo feature import-marks=$gitmarks
+echo feature export-marks=$gitmarks
+git fast-export --use-done-feature
--{import,export}-marks=$testgitmarks $refs | \
Better avoid the tricky {foo,bar} bashism:
git fast-export --use-done-feature \
--import-marks=$testgitmarks \
--export-marks=$testgitmarks \
$refs | \
If that's what we want, yeah.
Honestly, I find my longer-and-more-explicit version clearer, even
if you can assume bash for your script. But that's a matter of
personal preference (sorry for not stating that right away), so
feel free to ignore it if you decide to keep the bash requirement
in the end.
Regards,
Stefano
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 04/14] Add new simplified git-remote-testgit
On Fri, Nov 2, 2012 at 5:03 PM, Stefano Lattarini
stefano.lattar...@gmail.com wrote:
On 11/02/2012 04:42 PM, Felipe Contreras wrote:
What happens when you call this with:
./script alias with spaces
'$alias' will correctly expand to alias with spaces. Try out:
$ sh -c 'alias=$1; echo $alias' dummy '1 2*3'
1 2*3
This works consistently with every known shell (even non-POSIX
relics like Solaris /bin/sh).
All right.
_If_ we want this as POSIX, yeah.
Why don't we? Why add an extra requirement for a test that
1. can be easily written in POSIX shell, and
2. tests a feature that doesn't require bash to work (unless
I'm sorely mistaken, that is)?
Honest question. But of course, if the Git active contributors
deem the extra requirement (which is not an invasive one, given
how often bash is installed even on non-Linux systems) acceptable
in order to have the test case simpler and clearer, feel free to
disregard all my observations in this thread.
Because the code will be more complicated. Most of the changes
required are relatively small, so it's not a big issue, but I would
like to see how you replace the code that uses associative arrays. I
don't know know of a clean, simple way to do it in POSIX. If you can
find one, I don't see any strong reason to use bash.
+while read line; do
+case $line in
Useless double quoting (my previous observation about Git coding
guidelines applies here as well, of course).
What if line has multiple spaces?
Still no problem, as in the case of the 'alias=$1' assignment before:
$ sh -c 'case $1 in *x x*) echo ok;; *) exit 1;; esac' dummy 'x x'
ok
All right.
+echo feature import-marks=$gitmarks
+echo feature export-marks=$gitmarks
+git fast-export --use-done-feature
--{import,export}-marks=$testgitmarks $refs | \
Better avoid the tricky {foo,bar} bashism:
git fast-export --use-done-feature \
--import-marks=$testgitmarks \
--export-marks=$testgitmarks \
$refs | \
If that's what we want, yeah.
Honestly, I find my longer-and-more-explicit version clearer, even
if you can assume bash for your script. But that's a matter of
personal preference (sorry for not stating that right away), so
feel free to ignore it if you decide to keep the bash requirement
in the end.
And I prefer the other form. I fact, I would prefer if both tools
simply had a --marks option set both, and didn't require the file to
be created beforehand. I've _never_ seen a situation where separate
marks for import and export made sense. But that's off-topic.
If you find a way to replace the associative arrays code, I have no
trouble in switching this to the POSIX version.
Cheers.
--
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 04/14] Add new simplified git-remote-testgit
It's way simpler. It exerceises the same features of remote helpers.
It's easy to read and understand. It doesn't depend on python.
It does _not_ exercise the python remote helper framework; there's
another tool and another test for that.
For now let's just copy the old remote-helpers test script, although
some of those tests don't make sense for this testgit (they still pass).
In addition, this script would be able to test other features not
currently being tested.
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
Documentation/git-remote-testgit.txt | 2 +-
git-remote-testgit | 62
t/t5801-remote-helpers.sh| 134 +++
3 files changed, 197 insertions(+), 1 deletion(-)
create mode 100755 git-remote-testgit
create mode 100755 t/t5801-remote-helpers.sh
diff --git a/Documentation/git-remote-testgit.txt
b/Documentation/git-remote-testgit.txt
index 2a67d45..612a625 100644
--- a/Documentation/git-remote-testgit.txt
+++ b/Documentation/git-remote-testgit.txt
@@ -19,7 +19,7 @@ testcase for the remote-helper functionality, and as an
example to
show remote-helper authors one possible implementation.
The best way to learn more is to read the comments and source code in
-'git-remote-testgit.py'.
+'git-remote-testgit'.
SEE ALSO
diff --git a/git-remote-testgit b/git-remote-testgit
new file mode 100755
index 000..6650402
--- /dev/null
+++ b/git-remote-testgit
@@ -0,0 +1,62 @@
+#!/bin/bash
+# Copyright (c) 2012 Felipe Contreras
+
+alias=$1
+url=$2
+
+# huh?
+url=${url#file://}
+
+dir=$GIT_DIR/testgit/$alias
+prefix=refs/testgit/$alias
+refspec=refs/heads/*:${prefix}/heads/*
+
+gitmarks=$dir/git.marks
+testgitmarks=$dir/testgit.marks
+
+export GIT_DIR=$url/.git
+
+mkdir -p $dir
+
+test -e $gitmarks || echo -n $gitmarks
+test -e $testgitmarks || echo -n $testgitmarks
+
+while read line; do
+case $line in
+capabilities)
+echo 'import'
+echo 'export'
+echo refspec $refspec
+echo *import-marks $gitmarks
+echo *export-marks $gitmarks
+echo
+;;
+list)
+git for-each-ref --format='? %(refname)' 'refs/heads/'
+head=$(git symbolic-ref HEAD)
+echo @$head HEAD
+echo
+;;
+import*)
+# read all import lines
+while true; do
+ref=${line#* }
+refs=$refs $ref
+read line
+test ${line%% *} != import break
+done
+
+echo feature import-marks=$gitmarks
+echo feature export-marks=$gitmarks
+git fast-export --use-done-feature
--{import,export}-marks=$testgitmarks $refs | \
+sed -e s#refs/heads/#${prefix}/heads/#g
+;;
+export)
+git fast-import --{import,export}-marks=$testgitmarks --quiet
+echo
+;;
+'')
+exit
+;;
+esac
+done
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
new file mode 100755
index 000..67bc8eb
--- /dev/null
+++ b/t/t5801-remote-helpers.sh
@@ -0,0 +1,134 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Sverre Rabbelier
+#
+
+test_description='Test remote-helper import and export commands'
+
+. ./test-lib.sh
+
+compare_refs() {
+ git --git-dir=$1/.git rev-parse --verify $2 expect
+ git --git-dir=$3/.git rev-parse --verify $4 actual
+ test_cmp expect actual
+}
+
+test_expect_success 'setup repository' '
+ git init --bare server/.git
+ git clone server public
+ (cd public
+echo content file
+git add file
+git commit -m one
+git push origin master)
+'
+
+test_expect_success 'cloning from local repo' '
+ git clone testgit::${PWD}/server localclone
+ test_cmp public/file localclone/file
+'
+
+test_expect_success 'cloning from remote repo' '
+ git clone testgit::file://${PWD}/server clone
+ test_cmp public/file clone/file
+'
+
+test_expect_success 'create new commit on remote' '
+ (cd public
+echo content file
+git commit -a -m two
+git push)
+'
+
+test_expect_success 'pulling from local repo' '
+ (cd localclone git pull)
+ test_cmp public/file localclone/file
+'
+
+test_expect_success 'pulling from remote remote' '
+ (cd clone git pull)
+ test_cmp public/file clone/file
+'
+
+test_expect_success 'pushing to local repo' '
+ (cd localclone
+ echo content file
+ git commit -a -m three
+ git push)
+ compare_refs localclone HEAD server HEAD
+'
+
+# Generally, skip this test. It demonstrates a now-fixed race in
+# git-remote-testgit, but is too slow to leave in for general use.
+: test_expect_success 'racily pushing to local repo' '
+ test_when_finished rm -rf server2 localclone2
+ cp -R server server2
+ git clone testgit::${PWD}/server2 localclone2
+ (cd localclone2
+ echo content file
+ git
