Re: [PATCH 3/4] rev-parse test: use test_cmp instead of "test" builtin
Junio C Hamano wrote:
> After applying this patch and running "git show | grep test_cmp_rev_output",
> I notice that the second is always "git rev-parse ". Do
> we still need to eval these, or would it be sufficient to do
>
> test_cmp_rev_output () {
> git rev-parse --verify "$1" >expect &&
> git rev-parse --verify "$2" >actual &&
> test_cmp expect actual
> }
>
> here, and make users like so:
>
> - test_cmp_rev_output tags/start "git rev-parse start^0"
> + test_cmp_rev_output tags/start start^0
>
> Am I missing something?
I was tempted to use test_cmp_rev, which would have the same effect.
The original test checked the output of "git rev-parse start^0", while
the test as modified above checks the output of "git rev-parse
--verify start^0".
I slightly prefer the version without --verify because "git rev-parse
--verify" is well exercised elsewhere in the testsuite (but then, so
is rev-parse without --verify, so it's not a big deal).
Abbreviating as follows
foo () {
git rev-parse --verify "$1" >expect &&
git rev-parse "$2" >actual &&
test_cmp expect actual
}
would also be fine with me. All it would take is a good replacement
for the placeholder "foo". The added flexibility of "compare a rev
to the output of an arbitrary command" doesn't get used.
Jonathan
--
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 3/4] rev-parse test: use test_cmp instead of "test" builtin
On Tue, Sep 3, 2013 at 12:07 PM, Jonathan Nieder wrote: > test_expect_success 'start^0' ' > - test $(cat .git/refs/tags/start) = $(git rev-parse start^0) > + test_cmp_rev_output tags/start "git rev-parse start^0" > ' Backwards and yodaish this is. -- 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 3/4] rev-parse test: use test_cmp instead of "test" builtin
Jonathan Nieder writes:
> Use test_cmp instead of passing two command substitutions to the
> "test" builtin. This way:
>
> - when tests fail, they can print a helpful diff if run with
>"--verbose"
>
> - the argument order "test_cmp expect actual" feels natural,
>unlike test = that seems a little backwards
I do not mind to drop s/a little // here, by the way.
> - the exit status from invoking git is checked, so if rev-parse
>starts segfaulting then the test will notice and fail
>
> Use a custom function for this instead of test_cmp_rev to emphasize
> that we are testing the output from "git rev-parse" with certain
> arguments, not checking that the revisions are equal in abstract.
>
> Reported-by: Felipe Contreras
> Signed-off-by: Jonathan Nieder
> ---
> t/t6101-rev-parse-parents.sh | 33 +++--
> 1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh
> index 416067c..8a6ff66 100755
> --- a/t/t6101-rev-parse-parents.sh
> +++ b/t/t6101-rev-parse-parents.sh
> @@ -8,6 +8,12 @@ test_description='Test git rev-parse with different parent
> options'
> . ./test-lib.sh
> . "$TEST_DIRECTORY"/lib-t6000.sh # t6xxx specific functions
>
> +test_cmp_rev_output () {
> + git rev-parse --verify "$1" >expect &&
> + eval "$2" >actual &&
> + test_cmp expect actual
> +}
After applying this patch and running "git show | grep test_cmp_rev_output",
I notice that the second is always "git rev-parse ". Do
we still need to eval these, or would it be sufficient to do
test_cmp_rev_output () {
git rev-parse --verify "$1" >expect &&
git rev-parse --verify "$2" >actual &&
test_cmp expect actual
}
here, and make users like so:
- test_cmp_rev_output tags/start "git rev-parse start^0"
+ test_cmp_rev_output tags/start start^0
Am I missing something?
--
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
