Re: [PATCH 3/4] rev-parse test: use test_cmp instead of test builtin

2013-09-03 Thread Junio C Hamano
Jonathan Nieder jrnie...@gmail.com 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 known = unknown 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 felipe.contre...@gmail.com
 Signed-off-by: Jonathan Nieder jrnie...@gmail.com
 ---
  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 something.  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


Re: [PATCH 3/4] rev-parse test: use test_cmp instead of test builtin

2013-09-03 Thread Felipe Contreras
On Tue, Sep 3, 2013 at 12:07 PM, Jonathan Nieder jrnie...@gmail.com 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

2013-09-03 Thread Jonathan Nieder
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 something.  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