Re: [PATCH 3/6] t5516 (fetch-push): introduce mk_test_with_name()

2013-03-28 Thread Ramkumar Ramachandra
Jeff King wrote:
 Subject: [PATCH] t5516: drop implicit arguments from helper functions

Thanks a lot for this!  I just had to s/ $repo_name/ $repo_name/ to
fix the quoting.

Will post a re-roll soon.
--
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/6] t5516 (fetch-push): introduce mk_test_with_name()

2013-03-22 Thread Jeff King
On Fri, Mar 22, 2013 at 01:22:33PM +0530, Ramkumar Ramachandra wrote:

 mk_test() creates a repository with the constant name testrepo, and
 this may be limiting for tests that need to create more than one
 repository for testing.  To fix this, create a new mk_test_with_name()
 which accepts the repository name as $1.  Reimplement mk_test() as a
 special case of this function, making sure that no tests need to be
 rewritten.  Do the same thing for check_push_result().

I think this is OK, and I do not mind if it gets applied. But what I was
hinting at in my earlier mail was that we might want to do this (I have
it as a separate patch on top of your 3/6 here, but it would make more
sense squashed in):

-- 8 --
Subject: [PATCH] t5516: drop implicit arguments from helper functions

Many of the tests in t5516 look like:

  mk_empty 
  git push testrepo ... 
  check_push_result $commit heads/master

It's reasonably easy to see what is being tested, with the
exception that testrepo is a magic global name (it is
implicitly used in the helpers, but we have to name it
explicitly when calling git directly). Let's make it
explicit when call the helpers, too. This is slightly more
typing, but makes the test snippets read more naturally.

It also makes it easy for future tests to use an alternate
or multiple repositories, without a proliferation of helper
functions.

Signed-off-by: Jeff King p...@peff.net
---
 t/t5516-fetch-push.sh | 268 --
 1 file changed, 130 insertions(+), 138 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 05579b6..d27b8d3 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -8,7 +8,6 @@ mk_empty () {
 
 mk_empty () {
repo_name=$1
-   test -z $repo_name  repo_name=testrepo
rm -fr $repo_name 
mkdir $repo_name 
(
@@ -19,7 +18,7 @@ mk_empty () {
)
 }
 
-mk_test_with_name () {
+mk_test () {
repo_name=$1
shift
 
@@ -45,14 +44,11 @@ mk_test_with_hooks() {
)
 }
 
-mk_test () {
-   mk_test_with_name testrepo $@
-}
-
 mk_test_with_hooks() {
+   repo_name=$1
mk_test $@ 
(
-   cd testrepo 
+   cd $repo_name 
mkdir .git/hooks 
cd .git/hooks 
 
@@ -84,11 +80,11 @@ mk_child() {
 }
 
 mk_child() {
-   rm -rf $1 
-   git clone testrepo $1
+   rm -rf $2 
+   git clone $1 $2
 }
 
-check_push_result_with_name () {
+check_push_result () {
repo_name=$1
shift
 
@@ -108,10 +104,6 @@ check_push_result_with_name () {
)
 }
 
-check_push_result () {
-   check_push_result_with_name testrepo $@
-}
-
 test_expect_success setup '
 
path1 
@@ -129,7 +121,7 @@ test_expect_success 'fetch without wildcard' '
 '
 
 test_expect_success 'fetch without wildcard' '
-   mk_empty 
+   mk_empty testrepo 
(
cd testrepo 
git fetch .. refs/heads/master:refs/remotes/origin/master 
@@ -142,7 +134,7 @@ test_expect_success 'fetch with wildcard' '
 '
 
 test_expect_success 'fetch with wildcard' '
-   mk_empty 
+   mk_empty testrepo 
(
cd testrepo 
git config remote.up.url .. 
@@ -157,7 +149,7 @@ test_expect_success 'fetch with insteadOf' '
 '
 
 test_expect_success 'fetch with insteadOf' '
-   mk_empty 
+   mk_empty testrepo 
(
TRASH=$(pwd)/ 
cd testrepo 
@@ -174,7 +166,7 @@ test_expect_success 'fetch with pushInsteadOf (should not 
rewrite)' '
 '
 
 test_expect_success 'fetch with pushInsteadOf (should not rewrite)' '
-   mk_empty 
+   mk_empty testrepo 
(
TRASH=$(pwd)/ 
cd testrepo 
@@ -191,7 +183,7 @@ test_expect_success 'push without wildcard' '
 '
 
 test_expect_success 'push without wildcard' '
-   mk_empty 
+   mk_empty testrepo 
 
git push testrepo refs/heads/master:refs/remotes/origin/master 
(
@@ -204,7 +196,7 @@ test_expect_success 'push with wildcard' '
 '
 
 test_expect_success 'push with wildcard' '
-   mk_empty 
+   mk_empty testrepo 
 
git push testrepo refs/heads/*:refs/remotes/origin/* 
(
@@ -217,7 +209,7 @@ test_expect_success 'push with insteadOf' '
 '
 
 test_expect_success 'push with insteadOf' '
-   mk_empty 
+   mk_empty testrepo 
TRASH=$(pwd)/ 
git config url.$TRASH.insteadOf trash/ 
git push trash/testrepo refs/heads/master:refs/remotes/origin/master 
@@ -231,7 +223,7 @@ test_expect_success 'push with pushInsteadOf' '
 '
 
 test_expect_success 'push with pushInsteadOf' '
-   mk_empty 
+   mk_empty testrepo 
TRASH=$(pwd)/ 
git config url.$TRASH.pushInsteadOf trash/ 
git push trash/testrepo refs/heads/master:refs/remotes/origin/master 
@@ -245,7 +237,7 @@ test_expect_success 'push with pushInsteadOf and explicit 
pushurl (pushInsteadOf
 '
 

Re: [PATCH 3/6] t5516 (fetch-push): introduce mk_test_with_name()

2013-03-22 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 I think this is OK, and I do not mind if it gets applied. But what I was
 hinting at in my earlier mail was that we might want to do this (I have
 it as a separate patch on top of your 3/6 here, but it would make more
 sense squashed in):

I would prefer to see a preparatory patch to teach mk_test/mk_empty
to _always_ take the new name (i.e. the result of your patch) and
then do whatever new things on top.

By the way, I am planning to _not_ look at new stuff today. I'd
rather see known recent regressions addressed (and unknown ones
discovered and squashed) before we move forward, and I would
appreciate if regular contributors did the same.

Thanks.
--
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/6] t5516 (fetch-push): introduce mk_test_with_name()

2013-03-22 Thread Junio C Hamano
Ramkumar Ramachandra artag...@gmail.com writes:

  mk_empty () {
 - rm -fr testrepo 
 - mkdir testrepo 
 + repo_name=$1
 + test -z $repo_name  repo_name=testrepo
 + rm -fr $repo_name 
 + mkdir $repo_name 

Your quoting is sloppy in this entire patch X-.



--
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/6] t5516 (fetch-push): introduce mk_test_with_name()

2013-03-22 Thread Jeff King
On Fri, Mar 22, 2013 at 07:54:06AM -0700, Junio C Hamano wrote:

 Ramkumar Ramachandra artag...@gmail.com writes:
 
   mk_empty () {
  -   rm -fr testrepo 
  -   mkdir testrepo 
  +   repo_name=$1
  +   test -z $repo_name  repo_name=testrepo
  +   rm -fr $repo_name 
  +   mkdir $repo_name 
 
 Your quoting is sloppy in this entire patch X-.

I meant to mention that, too. It doesn't matter, as the intent is for it
always to be a simple name like testrepo, but that assumption is not
documented anywhere. I suspect the quoting in my patch is sloppy, too.

-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 3/6] t5516 (fetch-push): introduce mk_test_with_name()

2013-03-22 Thread Jeff King
On Fri, Mar 22, 2013 at 07:52:47AM -0700, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  I think this is OK, and I do not mind if it gets applied. But what I was
  hinting at in my earlier mail was that we might want to do this (I have
  it as a separate patch on top of your 3/6 here, but it would make more
  sense squashed in):
 
 I would prefer to see a preparatory patch to teach mk_test/mk_empty
 to _always_ take the new name (i.e. the result of your patch) and
 then do whatever new things on top.

I think that is what my patch does (it is meant to come at the point of
3/6, and then the rest would need to be rebased to just use mk_test
instead of mk_test_with_name).

 By the way, I am planning to _not_ look at new stuff today. I'd
 rather see known recent regressions addressed (and unknown ones
 discovered and squashed) before we move forward, and I would
 appreciate if regular contributors did the same.

Yeah, I have several to look at (the subdir/ in gitattributes is the
biggest one, I think).

-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 3/6] t5516 (fetch-push): introduce mk_test_with_name()

2013-03-22 Thread Jonathan Nieder
Junio C Hamano wrote:

 I would prefer to see a preparatory patch to teach mk_test/mk_empty
 to _always_ take the new name (i.e. the result of your patch) and
 then do whatever new things on top.

Yes, that sounds like a good way to go.

 By the way, I am planning to _not_ look at new stuff today. I'd
 rather see known recent regressions addressed (and unknown ones
 discovered and squashed) before we move forward, and I would
 appreciate if regular contributors did the same.

I've been flushing out my thoughts to avoid forgetting them. ;-)
Agreed, though.  Thanks.
--
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/6] t5516 (fetch-push): introduce mk_test_with_name()

2013-03-20 Thread Jonathan Nieder
Ramkumar Ramachandra wrote:

 mk_test() creates a repository with the constant name testrepo, and
 this may be limiting for tests that need to create more than one
 repository for testing.  To fix this, create a new mk_test_with_name()
 which accepts the repository name as $1.  Reimplement mk_test() as a
 special case of this function, making sure that no tests need to be
 rewritten.

Why not give mk_test an optional parameter?

repo_name=${1:-testrepo}

Oh, it is because mk_test already takes arguments naming refs to push.
I suppose the change description could make this clearer.

Why not use mk_test and then rename, like so?

mk_test ...refs... 
mv testrepo testrepo-a 

mk_test ...refs... 
mv testrepo testrepo-b 
...

I dunno.  The helper functions at the top of this test are already
intimidating, so I guess I am looking for a way to avoid making that
problem worse.  One way would be to add an opening comment before
the function definition explaining how it is meant to be used.  See
t/test-lib-functions.sh for examples, such as test_cmp.

Hope that helps,
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/6] t5516 (fetch-push): introduce mk_test_with_name()

2013-03-20 Thread Ramkumar Ramachandra
Jonathan Nieder wrote:
 Ramkumar Ramachandra wrote:

 mk_test() creates a repository with the constant name testrepo, and
 this may be limiting for tests that need to create more than one
 repository for testing.  To fix this, create a new mk_test_with_name()
 which accepts the repository name as $1.  Reimplement mk_test() as a
 special case of this function, making sure that no tests need to be
 rewritten.

 Why not give mk_test an optional parameter?

 repo_name=${1:-testrepo}

 Oh, it is because mk_test already takes arguments naming refs to push.
 I suppose the change description could make this clearer.

Isn't it obvious?

 Why not use mk_test and then rename, like so?

 mk_test ...refs... 
 mv testrepo testrepo-a 

 mk_test ...refs... 
 mv testrepo testrepo-b 
 ...

No.  This is ugly.  mk_test() should not hardcode testrepo.

 I dunno.  The helper functions at the top of this test are already
 intimidating, so I guess I am looking for a way to avoid making that
 problem worse.  One way would be to add an opening comment before
 the function definition explaining how it is meant to be used.  See
 t/test-lib-functions.sh for examples, such as test_cmp.

My patch does not make the situation worse in any way: it just adds
one line that passes $1 as a parameter to existing code.  Yes, the
functions and tests can be improved greatly, but I refrained from
doing so because of your series.  We can save it for later.
--
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/6] t5516 (fetch-push): introduce mk_test_with_name()

2013-03-20 Thread Jonathan Nieder
Ramkumar Ramachandra wrote:
 Jonathan Nieder wrote:

 I dunno.  The helper functions at the top of this test are already
 intimidating, so I guess I am looking for a way to avoid making that
 problem worse.
[...]
 My patch does not make the situation worse in any way:

Um, yes it does.  It adds another function to learn to an already
intimidating list.

Hoping that clarifies,
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/6] t5516 (fetch-push): introduce mk_test_with_name()

2013-03-20 Thread Jeff King
On Wed, Mar 20, 2013 at 11:41:57AM -0700, Jonathan Nieder wrote:

 Ramkumar Ramachandra wrote:
  Jonathan Nieder wrote:
 
  I dunno.  The helper functions at the top of this test are already
  intimidating, so I guess I am looking for a way to avoid making that
  problem worse.
 [...]
  My patch does not make the situation worse in any way:
 
 Um, yes it does.  It adds another function to learn to an already
 intimidating list.

Personally I do not find the set of helper functions intimidating. I
tend to read the tests in a top-down manner: a test is interesting
(usually because it fails), and then I want to see what it is doing, so
I look at any functions it calls, and so forth.

What I usually find _much_ harder to debug is when there is hidden state
leftover from other tests. So even though it is longer to write, I would
much rather see:

  test_expect_success 'check that frob only affects foo' '
  set_state_of foo 
  set_state_of bar 
  git frob 
  check_state_of foo 
  check_state_of bar
  '

than for the test to assume the state of foo or bar prior to the
test. And I think helper functions can help make writing those sorts of
pre-conditions more reasonable (and without looking too hard, I think
t5516 does an OK job of that).

Related to that is when the helper functions operate on hidden state. In
this instance, we have tests that do things like:

mk_empty 
git push testrepo refs/heads/master:refs/remotes/origin/master

and as a reader I say wait, what's in testrepo?. I can follow mk_empty
and see that it hardcodes testrepo, but it is even better if the
function and its arguments are named in a way that I don't have to. So
even though it is more typing, I would argue that:

  mk_empty testrepo 
  git push testrepo ...

is better, because the test script is more readable as a unit.

None of this is that huge a deal to me (and yet I seem to have written a
page about it :) ), but I figure while we are bikeshedding about test
style...

-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 3/6] t5516 (fetch-push): introduce mk_test_with_name()

2013-03-20 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 ... even though it is more typing, I would argue that:

   mk_empty testrepo 
   git push testrepo ...

 is better, because the test script is more readable as a unit.

 None of this is that huge a deal to me (and yet I seem to have written a
 page about it :) ), but I figure while we are bikeshedding about test
 style...

;-)  But the above is a good point.
--
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/6] t5516 (fetch-push): introduce mk_test_with_name()

2013-03-20 Thread Jonathan Nieder
Jeff King wrote:

I
 tend to read the tests in a top-down manner: a test is interesting
 (usually because it fails), and then I want to see what it is doing, so
 I look at any functions it calls, and so forth.

 What I usually find _much_ harder to debug is when there is hidden state
 leftover from other tests.

Thanks for articulating this.  I agree that keeping track of state is
the hardest part of working with git's tests.

To clarify my earlier comment, I was reading the test script from the
point of view of someone who wants to add an additional test, rather
than someone debugging an existing one.  That person has a difficult
task: she needs to understand

 * What do the existing tests in the script do?  This information
   is important in deciding whether the new test will be redundant.

 * How do I work with the particular dialect used in the script,
   including helpers?  A new test should fit in with the style of its
   surroundings to avoid contributing to an unmaintainable mess.

 * What is the intended scope of the test script?  Does my new test
   belong in this script?

 * What is the logical progression of the script?  What story does this
   script tell?  Where should I insert my test to maintain a logical
   ordering?

 * What state that later tests rely on do I need to avoid clobbering?

Generally the best way to help such a person is to make the script
very simple.  In particular, using standard helpers instead of custom
functions when appropriate and documenting helpers used to give new
readers a quick introduction to the dialect can help a lot.
--
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