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

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

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

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

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

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,

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.

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

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

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

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

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 :) ),

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