From: Eric Sunshine <hidden> Date: 2016-06-16 02:19:16
This series modernizes t1500; it takes an entirely different approach
than [1][2] and is intended to replace that series. Whereas [1][2]
dropped the systematic function-driven testing of git-rev-parse in favor
of dozens of nearly identical copy/paste tests, this series retains the
structure of the existing script and instead updates the tests to set up
their own needed state rather than relying upon transient and fragile
manipulation of global state.
Due to its systematic function-driven approach, the original script is
small at 87 lines, and easily understood. When [1] dropped the
systematic approach and replaced it with individual copy/paste tests,
the script ballooned to a whopping 573 lines; its v2 successor, while
smaller, still inflated it to 322 lines. Writing that amount of code
correctly (even when primarily copy/paste) is difficult; reviewing it
for correctness is tedious, mind-numbing, and error-prone, as evidenced
by [3].
This series, on the other hand, values the concision of the original and
only makes changes necessary to modernize it; the script size remains
about the same; it drops from 87 to 83 lines.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/291087/focus=291088
[2]: http://thread.gmane.org/gmane.comp.version-control.git/291729/focus=291731
[3]: http://thread.gmane.org/gmane.comp.version-control.git/291729/focus=291745
Eric Sunshine (6):
t1500: test_rev_parse: facilitate future test enhancements
t1500: reduce dependence upon global state
t1500: avoid changing working directory outside of tests
t1500: avoid setting configuration options outside of tests
t1500: avoid setting environment variables outside of tests
t1500: be considerate to future potential tests
t/t1500-rev-parse.sh | 116 +++++++++++++++++++++++++--------------------------
1 file changed, 56 insertions(+), 60 deletions(-)
--
2.8.2.530.g51d527d
From: Eric Sunshine <hidden> Date: 2016-06-16 02:19:16
Ideally, each test should be responsible for setting up state it needs
rather than relying upon transient global state. Toward this end, teach
test_rev_parse() to accept a "-b <value>" option to allow callers to set
"core.bare" explicitly or undefine it, and take advantage of this new
option to avoid setting "core.bare" outside of tests.
Under the hood, "-b <value>" invokes "test_config -C <dir>" (or
"test_unconfig -C <dir>"), which means that git-config knows explicitly
where to find its configuration file. Consequently, the global
GIT_CONFIG environment variable, which was needed by the manual
git-config invocations outside of tests, is no longer needed, and is
thus dropped.
Signed-off-by: Eric Sunshine <redacted>
---
t/t1500-rev-parse.sh | 39 ++++++++++++++++++++-------------------
1 file changed, 20 insertions(+), 19 deletions(-)
From: Eric Sunshine <hidden> Date: 2016-06-16 02:19:16
Ideally, each test should be responsible for setting up state it needs
rather than relying upon transient global state. Toward this end, teach
test_rev_parse() to accept a "-g <dir>" option to allow callers to
specify the value of the GIT_DIR environment variable explicitly, and
take advantage of this new option to avoid polluting the global scope
with GIT_DIR assignments.
Regarding the implementation: Typically, tests avoid polluting the
global state by wrapping transient environment variable assignments
within a subshell, however, this technique can not be used here since
test_config() and test_unconfig() need to know GIT_DIR, as well, but
neither function can be used within a subshell. Consequently, GIT_DIR is
cleared manually via sane_unset().
Signed-off-by: Eric Sunshine <redacted>
---
t/t1500-rev-parse.sh | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
From: Eric Sunshine <hidden> Date: 2016-06-16 02:19:16
Ideally, each test should be responsible for setting up state it needs
rather than relying upon transient global state. Toward this end, teach
test_rev_parse() to accept a "-C <dir>" option to allow callers to
instruct it explicitly in which directory its tests should be run, and
take advantage of this new option to avoid changing the working
directory outside of tests.
Signed-off-by: Eric Sunshine <redacted>
---
t/t1500-rev-parse.sh | 44 ++++++++++++++++++++++++--------------------
1 file changed, 24 insertions(+), 20 deletions(-)
From: Eric Sunshine <hidden> Date: 2016-06-16 02:19:16
The final batch of git-rev-parse tests work against a non-local object
database named ../repo.git rather than the typically-named ../.git. It
prepares by renaming .git/ to repo.git/ and pointing GIT_DIR at
../repo.git, but never restores the name to .git/, which can be
problematic for tests added in the future. Be more friendly by instead
making repo.git/ a copy of .git/.
Signed-off-by: Eric Sunshine <redacted>
---
t/t1500-rev-parse.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Eric Sunshine <hidden> Date: 2016-06-16 02:19:16
Tests run by test_rev_parse() are nearly identical; each invokes
git-rev-parse with a single option and compares the result against an
expected value. Such duplication makes it onerous to extend the tests
since any change needs to be repeated in each test. Reduce the
duplication by parameterizing the test and driving it via a for-loop.
Signed-off-by: Eric Sunshine <redacted>
---
t/t1500-rev-parse.sh | 44 +++++++++++++++++---------------------------
1 file changed, 17 insertions(+), 27 deletions(-)
From: Eric Sunshine <hidden> Date: 2016-06-16 02:19:16
This script pollutes its global state by changing its working directory
and capturing that state via $(pwd) in the GIT_CONFIG environment
variable. This makes it difficult to modernize the script since tests
ideally should be self-contained and not rely upon such transient global
state.
With the ultimate goal of eliminating working directory changes, as a
first step, avoid capturing global state by instead taking advantage of
$ROOT which captured $(pwd) prior to any working directory changes, and
compose GIT_CONFIG from it and local knowledge of the working directory.
Subsequent patches will eliminate changes of working directory and drop
GIT_CONFIG altogether.
Signed-off-by: Eric Sunshine <redacted>
---
t/t1500-rev-parse.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Eric Sunshine <hidden> Date: 2016-06-16 02:19:17
On Tue, May 10, 2016 at 1:20 AM, Eric Sunshine [off-list ref] wrote:
quoted hunk
Ideally, each test should be responsible for setting up state it needs
rather than relying upon transient global state. Toward this end, teach
test_rev_parse() to accept a "-b <value>" option to allow callers to set
"core.bare" explicitly or undefine it, and take advantage of this new
option to avoid setting "core.bare" outside of tests.
[...snip...]
Signed-off-by: Eric Sunshine <redacted>
---
This will expand $2 inside $env, which is later eval'd. So funny things
happen if there are spaces or metacharacters. It looks like you only use
it with short relative paths ("../repo.git", etc), which is OK, but this
would probably break badly if we ever used absolute paths.
I don't know if it's worth worrying about or not. The usual solution is
something like:
env_git_dir=$2
env='GIT_DIR=$env_git_dir; export GIT_DIR'
...
eval "$env"
I was surprised not to see quoting around $env here, but it probably
doesn't matter (I think it may affect how some whitespace is treated,
but the contents of $env are pretty tame).
This will set up the sane_unset regardless of whether $env does
anything. Would it make more sense to stick the test_when_finished
inside $env? You could use regular unset then, too, since you know the
variable would be set.
-Peff
@@ -7,11 +7,13 @@ test_description='test git rev-parse' while : do case "$1" in -C) dir="-C $2"; shift; shift ;; -b) bare="$2"; shift; shift ;;+ -g) env="GIT_DIR=$2; export GIT_DIR"; shift; shift ;;
This will expand $2 inside $env, which is later eval'd. So funny things
happen if there are spaces or metacharacters. It looks like you only use
it with short relative paths ("../repo.git", etc), which is OK, but this
would probably break badly if we ever used absolute paths.
I don't know if it's worth worrying about or not. The usual solution is
something like:
env_git_dir=$2
env='GIT_DIR=$env_git_dir; export GIT_DIR'
...
eval "$env"
Makes sense; I wasn't quite happy with having $2 interpolated
unquoted. Like you, though, I don't know if it's worth worrying
about...
I was surprised not to see quoting around $env here, but it probably
doesn't matter (I think it may affect how some whitespace is treated,
but the contents of $env are pretty tame).
I flip-flopped on this one several times, quoting, and not quoting.
Documentation for 'eval' says:
The args are read and concatenated together into a single
command.
so, I ultimately left it unquoted, but don't feel strongly about it.
This will set up the sane_unset regardless of whether $env does
anything. Would it make more sense to stick the test_when_finished
inside $env? You could use regular unset then, too, since you know the
variable would be set.
I didn't worry about it too much because the end result is effectively
the same and, with all the 'case' arms being short one-liners, I think
the code is a bit easier to read as-is; bundling 'test_when_finished'
into the 'env' assignment line would probably require wrapping the
line. I do like the improved encapsulation of your suggestion but
don't otherwise feel strongly about it.
Nevertheless, I can re-roll with these changes if you feel more
strongly than I about it.
This will set up the sane_unset regardless of whether $env does
anything. Would it make more sense to stick the test_when_finished
inside $env? You could use regular unset then, too, since you know the
variable would be set.
I didn't worry about it too much because the end result is effectively
the same and, with all the 'case' arms being short one-liners, I think
the code is a bit easier to read as-is; bundling 'test_when_finished'
into the 'env' assignment line would probably require wrapping the
line. I do like the improved encapsulation of your suggestion but
don't otherwise feel strongly about it.
Actually, I think we can have improved encapsulation and maintain
readability like this:
case "$1" in
...
-g) env="$2"; shift; shift ;;
...
esac
...
test_expect_success "..." '
if test -n "$env"
do
test_when_finished "unset GIT_DIR"
GIT_DIR="$env"
export GIT_DIR
fi
...
'
From: Eric Sunshine <hidden> Date: 2016-06-16 02:19:17
On Tue, May 10, 2016 at 3:48 PM, Eric Sunshine [off-list ref] wrote:
Actually, I think we can have improved encapsulation and maintain
readability like this:
case "$1" in
...
-g) env="$2"; shift; shift ;;
...
esac
...
test_expect_success "..." '
if test -n "$env"
do
test_when_finished "unset GIT_DIR"
GIT_DIR="$env"
export GIT_DIR
fi
...
'
At this point, I'd also rename 'env' to 'gitdir' to be more meaningful.
From: Jeff King <hidden> Date: 2016-06-16 02:19:18
On Tue, May 10, 2016 at 03:59:42PM -0400, Eric Sunshine wrote:
On Tue, May 10, 2016 at 3:48 PM, Eric Sunshine [off-list ref] wrote:
quoted
Actually, I think we can have improved encapsulation and maintain
readability like this:
case "$1" in
...
-g) env="$2"; shift; shift ;;
...
esac
...
test_expect_success "..." '
if test -n "$env"
do
test_when_finished "unset GIT_DIR"
GIT_DIR="$env"
export GIT_DIR
fi
...
'
At this point, I'd also rename 'env' to 'gitdir' to be more meaningful.
Yeah, I like this even better. As much as I enjoy eval tricks, I think
this case is more readable with the condition written out.
-Peff
This will set up the sane_unset regardless of whether $env does
anything. Would it make more sense to stick the test_when_finished
inside $env? You could use regular unset then, too, since you know the
variable would be set.
I didn't worry about it too much because the end result is effectively
the same and, with all the 'case' arms being short one-liners, I think
the code is a bit easier to read as-is; bundling 'test_when_finished'
into the 'env' assignment line would probably require wrapping the
line. I do like the improved encapsulation of your suggestion but
don't otherwise feel strongly about it.
Actually, I think we can have improved encapsulation and maintain
readability like this:
case "$1" in
...
-g) env="$2"; shift; shift ;;
...
esac
...
test_expect_success "..." '
if test -n "$env"
do
test_when_finished "unset GIT_DIR"
GIT_DIR="$env"
export GIT_DIR
fi
...
'
I wonder if is it really necessary to specify the path to the .git
directory via $GIT_DIR. Would 'git --git-dir=/over/there' be just as
good? If yes, then how about
git ${gitdir:+--git-dir="$gitdir"} rev-parse ...