[PATCH v2 0/6] Support triangular workflows

[PATCH v2 0/6] Support triangular workflows

[Ramkumar Ramachandra]

This follows-up [1], with three important differences:

1. pushremote_get() and remote_get() share code better.  Thanks Jeff.

2. All spelling mistakes have been corrected.  Thanks Eric.

3. One new test for each of the new configuration variables.  The
extra two parts [2/6] and [3/6] preprare the file for introducing
tests.  However, I've not gone overboard in this preparation; I don't
replicate the work done by Jonathan in [2].

Thanks for reading.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/218410
[2]: http://thread.gmane.org/gmane.comp.version-control.git/218451/focus=218465

Ramkumar Ramachandra (6):
  remote.c: simplify a bit of code using git_config_string()
  t5516 (fetch-push): update test description
  t5516 (fetch-push): introduce mk_test_with_name()
  remote.c: introduce a way to have different remotes for fetch/push
  remote.c: introduce remote.pushdefault
  remote.c: introduce branch.<name>.pushremote

 Documentation/config.txt | 23 +++++++++++++++---
 builtin/push.c           |  2 +-
 remote.c                 | 36 +++++++++++++++++++++------
 remote.h                 |  1 +
 t/t5516-fetch-push.sh    | 63 ++++++++++++++++++++++++++++++++++++++++--------
 5 files changed, 104 insertions(+), 21 deletions(-)

-- 
1.8.2

[PATCH 1/6] remote.c: simplify a bit of code using git_config_string()

[Ramkumar Ramachandra]

A small segment where handle_config() parses the branch.remote
configuration variable can be simplified using git_config_string().

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 remote.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/remote.c b/remote.c
index e53a6eb..45b69d6 100644
--- a/remote.c
+++ b/remote.c
@@ -356,9 +356,7 @@ static int handle_config(const char *key, const char *value, void *cb)
 			return 0;
 		branch = make_branch(name, subkey - name);
 		if (!strcmp(subkey, ".remote")) {
-			if (!value)
-				return config_error_nonbool(key);
-			branch->remote_name = xstrdup(value);
+			git_config_string(&branch->remote_name, key, value);
 			if (branch == current_branch) {
 				default_remote_name = branch->remote_name;
 				explicit_default_remote_name = 1;
-- 
1.8.2

Re: [PATCH 1/6] remote.c: simplify a bit of code using git_config_string()

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> --- a/remote.c
> +++ b/remote.c
> @@ -356,9 +356,7 @@ static int handle_config(const char *key, const char *value, void *cb)
>  			return 0;
>  		branch = make_branch(name, subkey - name);
>  		if (!strcmp(subkey, ".remote")) {
> -			if (!value)
> -				return config_error_nonbool(key);
> -			branch->remote_name = xstrdup(value);
> +			git_config_string(&branch->remote_name, key, value);

Shouldn't this say

			if (git_config_string(&branch->remote_name, key, value))
				return -1;

or something?

Thanks,
Jonathan

Re: [PATCH 1/6] remote.c: simplify a bit of code using git_config_string()

[Ramkumar Ramachandra]

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -356,9 +356,7 @@ static int handle_config(const char *key, const char *value, void *cb)
>>                       return 0;
>>               branch = make_branch(name, subkey - name);
>>               if (!strcmp(subkey, ".remote")) {
>> -                     if (!value)
>> -                             return config_error_nonbool(key);
>> -                     branch->remote_name = xstrdup(value);
>> +                     git_config_string(&branch->remote_name, key, value);
>
> Shouldn't this say
>
>                         if (git_config_string(&branch->remote_name, key, value))
>                                 return -1;
>
> or something?

Yes, and so should the instances in [5/6] and [6/6].  Thanks for catching it.

[PATCH 2/6] t5516 (fetch-push): update test description

[Ramkumar Ramachandra]

The file was originally created in bcdb34f (Test wildcard push/fetch,
2007-06-08), and only contained tests that exercised wildcard
functionality at the time.  In subsequent commits, many other tests
unrelated to wildcards were added but the test description was never
updated.  Fix this.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5516-fetch-push.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index c31e5c1..bfeec60 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='fetching and pushing, with or without wildcard'
+test_description='fetching and pushing'
 
 . ./test-lib.sh
 
-- 
1.8.2

Re: [PATCH 2/6] t5516 (fetch-push): update test description

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1,6 +1,6 @@
>  #!/bin/sh
>  
> -test_description='fetching and pushing, with or without wildcard'
> +test_description='fetching and pushing'

I'm not thrilled with the description before or after.  Would it make
sense to do something like the following?

	test_description='Tests of basic fetch/push functionality.
	
	These tests create small test repositories and fetch from and
	push to them, testing:

	 * commandline syntax
	 * refspecs and default refspecs
	 * fast-forward detection and overriding fast-forward detection
	 * configuration (insteadOf, pushInsteadOf, [remote "name"] push,
	   etc)
	 * hooks
	 * --porcelain output format
	 * hiderefs
	'

Re: [PATCH 2/6] t5516 (fetch-push): update test description

[Ramkumar Ramachandra]

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> --- a/t/t5516-fetch-push.sh
>> +++ b/t/t5516-fetch-push.sh
>> @@ -1,6 +1,6 @@
>>  #!/bin/sh
>>
>> -test_description='fetching and pushing, with or without wildcard'
>> +test_description='fetching and pushing'
>
> I'm not thrilled with the description before or after.  Would it make
> sense to do something like the following?
>
>         test_description='Tests of basic fetch/push functionality.
>
>         These tests create small test repositories and fetch from and
>         push to them, testing:
>
>          * commandline syntax
>          * refspecs and default refspecs
>          * fast-forward detection and overriding fast-forward detection
>          * configuration (insteadOf, pushInsteadOf, [remote "name"] push,
>            etc)
>          * hooks
>          * --porcelain output format
>          * hiderefs
>         '

No.  When I want to add a test for branch.<name>.pushremote, I grep
for branch.*.pushurl, and open files with sensible names; I'm not
going to open up the file and read a long description of what tests it
already contains.  The filename and test headlines are sufficient.
Our test suite is bad enough as it is (inconsistent style, missing &&,
false positives)- I'm against adding to the maintenance burden.

Re: [PATCH 2/6] t5516 (fetch-push): update test description

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

>      When I want to add a test for branch.<name>.pushremote, I grep
> for branch.*.pushurl, and open files with sensible names; I'm not
> going to open up the file and read a long description of what tests it
> already contains.

Huh?  The test_description is output for "./t5516-* --help" and is
supposed to help people hacking on the test to understand its setup
and its purpose.

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

[Ramkumar Ramachandra]

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().

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5516-fetch-push.sh | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index bfeec60..a546c2c 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -7,27 +7,32 @@ test_description='fetching and pushing'
 D=`pwd`
 
 mk_empty () {
-	rm -fr testrepo &&
-	mkdir testrepo &&
+	repo_name="$1"
+	test -z "$repo_name" && repo_name=testrepo
+	rm -fr $repo_name &&
+	mkdir $repo_name &&
 	(
-		cd testrepo &&
+		cd $repo_name &&
 		git init &&
 		git config receive.denyCurrentBranch warn &&
 		mv .git/hooks .git/hooks-disabled
 	)
 }
 
-mk_test () {
-	mk_empty &&
+mk_test_with_name () {
+	repo_name="$1"
+	shift
+
+	mk_empty $repo_name &&
 	(
 		for ref in "$@"
 		do
-			git push testrepo $the_first_commit:refs/$ref || {
+			git push $repo_name $the_first_commit:refs/$ref || {
 				echo "Oops, push refs/$ref failure"
 				exit 1
 			}
 		done &&
-		cd testrepo &&
+		cd $repo_name &&
 		for ref in "$@"
 		do
 			r=$(git show-ref -s --verify refs/$ref) &&
@@ -40,6 +45,10 @@ mk_test () {
 	)
 }
 
+mk_test () {
+	mk_test_with_name testrepo "$@"
+}
+
 mk_test_with_hooks() {
 	mk_test "$@" &&
 	(
@@ -79,9 +88,12 @@ mk_child() {
 	git clone testrepo "$1"
 }
 
-check_push_result () {
+check_push_result_with_name () {
+	repo_name="$1"
+	shift
+
 	(
-		cd testrepo &&
+		cd $repo_name &&
 		it="$1" &&
 		shift
 		for ref in "$@"
@@ -96,6 +108,10 @@ check_push_result () {
 	)
 }
 
+check_push_result () {
+	check_push_result_with_name testrepo "$@"
+}
+
 test_expect_success setup '
 
 	>path1 &&
-- 
1.8.2

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

[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

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

[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.

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

[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

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

[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

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

[Junio C Hamano]

Jeff King <peff@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.

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

[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.

[PATCH 4/6] remote.c: introduce a way to have different remotes for fetch/push

[Ramkumar Ramachandra]

Currently, do_push() in push.c calls remote_get(), which gets the
configured remote for fetching and pushing.  Replace this call with a
call to pushremote_get() instead, a new function that will return the
remote configured specifically for pushing.  This function tries to
work with the string pushremote_name, before falling back to the
codepath of remote_get().  This patch has no visible impact, but
serves to enable future patches to introduce configuration variables
to set this variable.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/push.c |  2 +-
 remote.c       | 25 +++++++++++++++++++++----
 remote.h       |  1 +
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 42b129d..d447a80 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -322,7 +322,7 @@ static int push_with_options(struct transport *transport, int flags)
 static int do_push(const char *repo, int flags)
 {
 	int i, errs;
-	struct remote *remote = remote_get(repo);
+	struct remote *remote = pushremote_get(repo);
 	const char **url;
 	int url_nr;
 
diff --git a/remote.c b/remote.c
index 45b69d6..cf7a65d 100644
--- a/remote.c
+++ b/remote.c
@@ -48,6 +48,7 @@ static int branches_nr;
 
 static struct branch *current_branch;
 static const char *default_remote_name;
+static const char *pushremote_name;
 static int explicit_default_remote_name;
 
 static struct rewrites rewrites;
@@ -669,17 +670,21 @@ static int valid_remote_nick(const char *name)
 	return !strchr(name, '/'); /* no slash */
 }
 
-struct remote *remote_get(const char *name)
+static struct remote *remote_get_1(const char *name, const char *pushremote_name)
 {
 	struct remote *ret;
 	int name_given = 0;
 
-	read_config();
 	if (name)
 		name_given = 1;
 	else {
-		name = default_remote_name;
-		name_given = explicit_default_remote_name;
+		if (pushremote_name) {
+			name = pushremote_name;
+			name_given = 1;
+		} else {
+			name = default_remote_name;
+			name_given = explicit_default_remote_name;
+		}
 	}
 
 	ret = make_remote(name, 0);
@@ -698,6 +703,18 @@ struct remote *remote_get(const char *name)
 	return ret;
 }
 
+struct remote *remote_get(const char *name)
+{
+	read_config();
+	return remote_get_1(name, NULL);
+}
+
+struct remote *pushremote_get(const char *name)
+{
+	read_config();
+	return remote_get_1(name, pushremote_name);
+}
+
 int remote_is_configured(const char *name)
 {
 	int i;
diff --git a/remote.h b/remote.h
index 251d8fd..99a437f 100644
--- a/remote.h
+++ b/remote.h
@@ -51,6 +51,7 @@ struct remote {
 };
 
 struct remote *remote_get(const char *name);
+struct remote *pushremote_get(const char *name);
 int remote_is_configured(const char *name);
 
 typedef int each_remote_fn(struct remote *remote, void *priv);
-- 
1.8.2

Re: [PATCH 4/6] remote.c: introduce a way to have different remotes for fetch/push

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Currently, do_push() in push.c calls remote_get(), which gets the
> configured remote for fetching and pushing.  Replace this call with a
> call to pushremote_get() instead, a new function that will return the
> remote configured specifically for pushing.  This function tries to
> work with the string pushremote_name, before falling back to the
> codepath of remote_get().  This patch has no visible impact, but
> serves to enable future patches to introduce configuration variables
> to set this variable.

The above description does not make the impact of this change clear to
me.  Could you give a before-and-after example?  How will this
internal API change make my life easier as a developer?

Hope that helps,
Jonathan

Re: [PATCH 4/6] remote.c: introduce a way to have different remotes for fetch/push

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>  	if (name)
>  		name_given = 1;
>  	else {
> -		name = default_remote_name;
> -		name_given = explicit_default_remote_name;
> +		if (pushremote_name) {
> +			name = pushremote_name;
> +			name_given = 1;
> +		} else {
> +			name = default_remote_name;
> +			name_given = explicit_default_remote_name;
> +		}
>  	}

The code to read branch.$name.remote configuration flips
explicit_default_remote_name to one when it is used to set the
default_remote_name, and that controls the value of name_given in
this codepath.  At this point in the series, you do not have a
corresponding branch.$name.pushremote, but your [6/6] does not seem
to do the same.

Why isn't it necessary to add explicit_default_pushremote_name and
do the same here in patch [6/6]?

Re: [PATCH 4/6] remote.c: introduce a way to have different remotes for fetch/push

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>>       if (name)
>>               name_given = 1;
>>       else {
>> -             name = default_remote_name;
>> -             name_given = explicit_default_remote_name;
>> +             if (pushremote_name) {
>> +                     name = pushremote_name;
>> +                     name_given = 1;
>> +             } else {
>> +                     name = default_remote_name;
>> +                     name_given = explicit_default_remote_name;
>> +             }
>>       }
>
> The code to read branch.$name.remote configuration flips
> explicit_default_remote_name to one when it is used to set the
> default_remote_name, and that controls the value of name_given in
> this codepath.  At this point in the series, you do not have a
> corresponding branch.$name.pushremote, but your [6/6] does not seem
> to do the same.
>
> Why isn't it necessary to add explicit_default_pushremote_name and
> do the same here in patch [6/6]?

Sorry, I'm still trying to understand your comment.  Okay, yes:
branch.$name.remote does flip explicit_default_remote_name, because we
need to know if the default remote name was explicitly given.  Wait,
how is explicit_default_remote_name used to set default_remote_name?
Don't you mean name_given?  It controls name_give, yes.  At this point
I don't have .pushremote, yes: I'm setting up for [5/6] and [6/6].  My
[6/6] doesn't seem to do the "same"?  The same thing as .remote?  Are
you asking why .pushremote doesn't flip explicit_default_remote_name
like .remote does?  Because .pushremote can only ever be specified
explicitly: otherwise, it falls back to the .remote logic.

Okay, next paragraph.  Why isn't it necessary to add
explicit_default_pushremote_name?  Like I said, .pushremote can only
ever be specified explicitly.  There is no implicit fallback (like
"origin"): it just falls back to the .remote codepath, if not
explicitly specified.  In other words, it's just a small override on
the .remote codepath.

Re: [PATCH 4/6] remote.c: introduce a way to have different remotes for fetch/push

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> ...  There is no implicit fallback (like
> "origin"): it just falls back to the .remote codepath, if not
> explicitly specified.

That one sentence is enough to explain the apparent asymmetry, which
bothered me.

[PATCH 5/6] remote.c: introduce remote.pushdefault

[Ramkumar Ramachandra]

This new configuration variable defines the default remote to push to,
and overrides `branch.<name>.remote` for all branches.  It is useful
in the typical triangular-workflow setup, where the remote you're
fetching from is different from the remote you're pushing to.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/config.txt | 13 ++++++++++---
 remote.c                 |  4 ++++
 t/t5516-fetch-push.sh    | 12 ++++++++++++
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bbba728..e813c33 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -723,9 +723,12 @@ branch.autosetuprebase::
 	This option defaults to never.
 
 branch.<name>.remote::
-	When in branch <name>, it tells 'git fetch' and 'git push' which
-	remote to fetch from/push to.  It defaults to `origin` if no remote is
-	configured. `origin` is also used if you are not on any branch.
+	When on branch <name>, it tells 'git fetch' and 'git push'
+	which remote to fetch from/push to.  The remote to push to
+	may be overridden with `remote.pushdefault` (for all branches).
+	If no remote is configured, or if you are not on any branch,
+	it defaults to `origin` for fetching and `remote.pushdefault`
+	for pushing.
 
 branch.<name>.merge::
 	Defines, together with branch.<name>.remote, the upstream branch
@@ -1894,6 +1897,10 @@ receive.updateserverinfo::
 	If set to true, git-receive-pack will run git-update-server-info
 	after receiving data from git-push and updating refs.
 
+remote.pushdefault::
+	The remote to push to by default.  Overrides
+	`branch.<name>.remote` for all branches.
+
 remote.<name>.url::
 	The URL of a remote repository.  See linkgit:git-fetch[1] or
 	linkgit:git-push[1].
diff --git a/remote.c b/remote.c
index cf7a65d..68056c7 100644
--- a/remote.c
+++ b/remote.c
@@ -350,6 +350,10 @@ static int handle_config(const char *key, const char *value, void *cb)
 	const char *subkey;
 	struct remote *remote;
 	struct branch *branch;
+	if (!prefixcmp(key, "remote.")) {
+		if (!strcmp(key + 7, "pushdefault"))
+			git_config_string(&pushremote_name, key, value);
+	}
 	if (!prefixcmp(key, "branch.")) {
 		name = key + 7;
 		subkey = strrchr(name, '.');
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index a546c2c..63171f1 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -517,6 +517,18 @@ test_expect_success 'push with config remote.*.push = HEAD' '
 git config --remove-section remote.there
 git config --remove-section branch.master
 
+test_expect_success 'push with remote.pushdefault' '
+	mk_test_with_name up_repo heads/frotz &&
+	mk_test_with_name down_repo heads/master &&
+	test_config remote.up.url up_repo &&
+	test_config remote.down.url down_repo &&
+	test_config branch.master.remote up &&
+	test_config remote.pushdefault down &&
+	git push &&
+	test_must_fail check_push_result_with_name up_repo $the_commit heads/master &&
+	check_push_result_with_name down_repo $the_commit heads/master
+'
+
 test_expect_success 'push with config remote.*.pushurl' '
 
 	mk_test heads/master &&
-- 
1.8.2

Re: [PATCH 5/6] remote.c: introduce remote.pushdefault

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> This new configuration variable defines the default remote to push to,
> and overrides `branch.<name>.remote` for all branches.

Micronit: I think this would be easier to explain if it came after
patch 6, since then you could say "In other words, it is a default for
branch.<name>.pushremote for all branches" and readers would not have
to wonder "Why does the more generic configuration override the more
specific one?".

My two cents,
Jonathan

Re: [PATCH 5/6] remote.c: introduce remote.pushdefault

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Ramkumar Ramachandra wrote:
>
>> This new configuration variable defines the default remote to push to,
>> and overrides `branch.<name>.remote` for all branches.
>
> Micronit: I think this would be easier to explain if it came after
> patch 6, since then you could say "In other words, it is a default for
> branch.<name>.pushremote for all branches" and readers would not have
> to wonder "Why does the more generic configuration override the more
> specific one?".
>
> My two cents,
> Jonathan

Thanks for all good comments (not only to this one but to others in
the series).  I agree with all of them.

Re: [PATCH 5/6] remote.c: introduce remote.pushdefault

[Ramkumar Ramachandra]

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> This new configuration variable defines the default remote to push to,
>> and overrides `branch.<name>.remote` for all branches.
>
> Micronit: I think this would be easier to explain if it came after
> patch 6, since then you could say "In other words, it is a default for
> branch.<name>.pushremote for all branches" and readers would not have
> to wonder "Why does the more generic configuration override the more
> specific one?".

I'm sorry, but this is not worth a rebase.

[PATCH 6/6] remote.c: introduce branch.<name>.pushremote

[Ramkumar Ramachandra]

This new configuration variable overrides `remote.pushdefault` and
`branch.<name>.remote` for pushes.  In a typical triangular-workflow
setup, you would want to set `remote.pushdefault` to specify the
remote to push to for all branches, and use this option to override it
for a specific branch.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/config.txt | 18 ++++++++++++++----
 remote.c                 |  3 +++
 t/t5516-fetch-push.sh    | 15 +++++++++++++++
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e813c33..4b9647a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -726,9 +726,18 @@ branch.<name>.remote::
 	When on branch <name>, it tells 'git fetch' and 'git push'
 	which remote to fetch from/push to.  The remote to push to
 	may be overridden with `remote.pushdefault` (for all branches).
-	If no remote is configured, or if you are not on any branch,
-	it defaults to `origin` for fetching and `remote.pushdefault`
-	for pushing.
+	The remote to push to, for the current branch, may be further
+	overridden by `branch.<name>.pushremote`.  If no remote is
+	configured, or if you are not on any branch, it defaults to
+	`origin` for fetching and `remote.pushdefault` for pushing.
+
+branch.<name>.pushremote::
+	When on branch <name>, it overrides `branch.<name>.remote`
+	when pushing.  It also overrides `remote.pushdefault` when
+	pushing from branch <name>.  In a typical triangular-workflow
+	setup, you would want to set `remote.pushdefault` to specify
+	the remote to push to for all branches, and use this option to
+	override it for a specific branch.
 
 branch.<name>.merge::
 	Defines, together with branch.<name>.remote, the upstream branch
@@ -1899,7 +1908,8 @@ receive.updateserverinfo::
 
 remote.pushdefault::
 	The remote to push to by default.  Overrides
-	`branch.<name>.remote` for all branches.
+	`branch.<name>.remote` for all branches, and is overridden by
+	`branch.<name>.pushremote` for specific branches.
 
 remote.<name>.url::
 	The URL of a remote repository.  See linkgit:git-fetch[1] or
diff --git a/remote.c b/remote.c
index 68056c7..c988168 100644
--- a/remote.c
+++ b/remote.c
@@ -366,6 +366,9 @@ static int handle_config(const char *key, const char *value, void *cb)
 				default_remote_name = branch->remote_name;
 				explicit_default_remote_name = 1;
 			}
+		} else if (!strcmp(subkey, ".pushremote")) {
+			if (branch == current_branch)
+				git_config_string(&pushremote_name, key, value);
 		} else if (!strcmp(subkey, ".merge")) {
 			if (!value)
 				return config_error_nonbool(key);
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 63171f1..3f91874 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -539,6 +539,21 @@ test_expect_success 'push with config remote.*.pushurl' '
 	check_push_result $the_commit heads/master
 '
 
+test_expect_success 'push with config branch.*.pushremote' '
+	mk_test_with_name up_repo heads/frotz &&
+	mk_test_with_name side_repo heads/quux &&
+	mk_test_with_name down_repo heads/master &&
+	test_config remote.up.url up_repo &&
+	test_config remote.pushdefault side_repo &&
+	test_config remote.down.url down_repo &&
+	test_config branch.master.remote up &&
+	test_config branch.master.pushremote down &&
+	git push &&
+	test_must_fail check_push_result_with_name up_repo $the_commit heads/master &&
+	test_must_fail check_push_result_with_name side_repo $the_commit heads/master &&
+	check_push_result_with_name down_repo $the_commit heads/master
+'
+
 # clean up the cruft left with the previous one
 git config --remove-section remote.there
 
-- 
1.8.2

Re: [PATCH 6/6] remote.c: introduce branch.<name>.pushremote

[Tay Ray Chuan]

On Wed, Mar 20, 2013 at 8:45 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> This new configuration variable overrides `remote.pushdefault` and
> `branch.<name>.remote` for pushes.  In a typical triangular-workflow
> setup, you would want to set `remote.pushdefault` to specify the
> remote to push to for all branches, and use this option to override it
> for a specific branch.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  Documentation/config.txt | 18 ++++++++++++++----
>  remote.c                 |  3 +++
>  t/t5516-fetch-push.sh    | 15 +++++++++++++++
>  3 files changed, 32 insertions(+), 4 deletions(-)

Shouldn't this patch be squashed into 5/6 because of...

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index e813c33..4b9647a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -726,9 +726,18 @@ branch.<name>.remote::
>         When on branch <name>, it tells 'git fetch' and 'git push'
>         which remote to fetch from/push to.  The remote to push to
>         may be overridden with `remote.pushdefault` (for all branches).
> -       If no remote is configured, or if you are not on any branch,
> -       it defaults to `origin` for fetching and `remote.pushdefault`
> -       for pushing.
> +       The remote to push to, for the current branch, may be further
> +       overridden by `branch.<name>.pushremote`.  If no remote is
> +       configured, or if you are not on any branch, it defaults to
> +       `origin` for fetching and `remote.pushdefault` for pushing.
> +

...this? (Since this description was introduced in 5/6)

-- 
Cheers,
Ray Chuan

Re: [PATCH 6/6] remote.c: introduce branch.<name>.pushremote

[Ramkumar Ramachandra]

Tay Ray Chuan wrote:
> On Wed, Mar 20, 2013 at 8:45 PM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> This new configuration variable overrides `remote.pushdefault` and
>> `branch.<name>.remote` for pushes.  In a typical triangular-workflow
>> setup, you would want to set `remote.pushdefault` to specify the
>> remote to push to for all branches, and use this option to override it
>> for a specific branch.
>>
>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>> ---
>>  Documentation/config.txt | 18 ++++++++++++++----
>>  remote.c                 |  3 +++
>>  t/t5516-fetch-push.sh    | 15 +++++++++++++++
>>  3 files changed, 32 insertions(+), 4 deletions(-)
>
> Shouldn't this patch be squashed into 5/6 because of...
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index e813c33..4b9647a 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -726,9 +726,18 @@ branch.<name>.remote::
>>         When on branch <name>, it tells 'git fetch' and 'git push'
>>         which remote to fetch from/push to.  The remote to push to
>>         may be overridden with `remote.pushdefault` (for all branches).
>> -       If no remote is configured, or if you are not on any branch,
>> -       it defaults to `origin` for fetching and `remote.pushdefault`
>> -       for pushing.
>> +       The remote to push to, for the current branch, may be further
>> +       overridden by `branch.<name>.pushremote`.  If no remote is
>> +       configured, or if you are not on any branch, it defaults to
>> +       `origin` for fetching and `remote.pushdefault` for pushing.
>> +
>
> ...this? (Since this description was introduced in 5/6)

Huh?  This patch introduces branch.<name>.pushremote: the relevant
code and documentation changes.  5/6 introduces remote.pushdefault,
which is completely different.

Re: [PATCH v2 0/6] Support triangular workflows

[Tay Ray Chuan]

On Wed, Mar 20, 2013 at 8:44 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
>   remote.c: introduce remote.pushdefault
>   remote.c: introduce branch.<name>.pushremote

Perhaps we should clarify how this differs from remote.pushurl in the
documentation for it, in git-config and/or git-push. Maybe even
include the design decisions behind it from [1]. :)

http://thread.gmane.org/gmane.comp.version-control.git/215702/focus=215717

--
Cheers,
Ray Chuan

Re: [PATCH v2 0/6] Support triangular workflows

[Ramkumar Ramachandra]

Tay Ray Chuan wrote:
> On Wed, Mar 20, 2013 at 8:44 PM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>>   remote.c: introduce remote.pushdefault
>>   remote.c: introduce branch.<name>.pushremote
>
> Perhaps we should clarify how this differs from remote.pushurl in the
> documentation for it, in git-config and/or git-push. Maybe even
> include the design decisions behind it from [1]. :)
>
> http://thread.gmane.org/gmane.comp.version-control.git/215702/focus=215717

Actually, it's quite obvious when you think about it: remote.pushurl
doesn't allow remote tracking branches, and this is a very serious
limitation.  If anything, the documentation for remote.pushurl should
be updated to explain that it has a very narrow usecase (will do in a
future patch).

Re: [PATCH v2 0/6] Support triangular workflows

[Philip Oakley]

From: "Ramkumar Ramachandra" <artagnon@gmail.com>
Sent: Wednesday, March 20, 2013 12:44 PM
> This follows-up [1], with three important differences:
>
> 1. pushremote_get() and remote_get() share code better.  Thanks Jeff.
>
> 2. All spelling mistakes have been corrected.  Thanks Eric.
>
> 3. One new test for each of the new configuration variables.  The
> extra two parts [2/6] and [3/6] preprare the file for introducing
> tests.  However, I've not gone overboard in this preparation; I don't
> replicate the work done by Jonathan in [2].
>
> Thanks for reading.
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/218410
> [2]: 
> http://thread.gmane.org/gmane.comp.version-control.git/218451/focus=218465
>
> Ramkumar Ramachandra (6):
>  remote.c: simplify a bit of code using git_config_string()
>  t5516 (fetch-push): update test description
>  t5516 (fetch-push): introduce mk_test_with_name()
>  remote.c: introduce a way to have different remotes for fetch/push
>  remote.c: introduce remote.pushdefault
>  remote.c: introduce branch.<name>.pushremote
>
> Documentation/config.txt | 23 +++++++++++++++---

Shouldn't Documentation/gitworkflows.txt also be updated with the 
triangular workflow and its configuration?

> builtin/push.c           |  2 +-
> remote.c                 | 36 +++++++++++++++++++++------
> remote.h                 |  1 +
> t/t5516-fetch-push.sh    | 63 
> ++++++++++++++++++++++++++++++++++++++++--------
> 5 files changed, 104 insertions(+), 21 deletions(-)
>
> -- 
> 1.8.2
>

Re: [PATCH v2 0/6] Support triangular workflows

[Ramkumar Ramachandra]

Philip Oakley wrote:
> From: "Ramkumar Ramachandra" <artagnon@gmail.com>
> Sent: Wednesday, March 20, 2013 12:44 PM
>
>> This follows-up [1], with three important differences:
>>
>> 1. pushremote_get() and remote_get() share code better.  Thanks Jeff.
>>
>> 2. All spelling mistakes have been corrected.  Thanks Eric.
>>
>> 3. One new test for each of the new configuration variables.  The
>> extra two parts [2/6] and [3/6] preprare the file for introducing
>> tests.  However, I've not gone overboard in this preparation; I don't
>> replicate the work done by Jonathan in [2].
>>
>> Thanks for reading.
>>
>> [1]: http://thread.gmane.org/gmane.comp.version-control.git/218410
>> [2]:
>> http://thread.gmane.org/gmane.comp.version-control.git/218451/focus=218465
>>
>> Ramkumar Ramachandra (6):
>>  remote.c: simplify a bit of code using git_config_string()
>>  t5516 (fetch-push): update test description
>>  t5516 (fetch-push): introduce mk_test_with_name()
>>  remote.c: introduce a way to have different remotes for fetch/push
>>  remote.c: introduce remote.pushdefault
>>  remote.c: introduce branch.<name>.pushremote
>>
>> Documentation/config.txt | 23 +++++++++++++++---
>
>
> Shouldn't Documentation/gitworkflows.txt also be updated with the triangular
> workflow and its configuration?

Currently, Documentation/gitworkflows.txt makes no effort to explain
how to set up configuration variables for centralized or distributed
workflows.  I don't see how I could patch the existing document to
include this workflow, without changing the entire structure of the
document (left as an exercise for future patches).

Re: [PATCH v2 0/6] Support triangular workflows

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> Shouldn't Documentation/gitworkflows.txt also be updated with the
> triangular workflow and its configuration?

What is missing from gitworkflows documentation is actually a
non-triangular workflow, where people pull from and push into the
same central repository.

The "merge workflow" part in the "distributed workflows" section
teaches:

 * fetching others' work from <remote> and merging it to yours;
 * publishing your work to <remote>; and
 * advertising your work.

and it is written for the triangular workflow.

Two triangles are involved.  The maintainer may pull from you but
pushes to her own, which is one triangle, and you pull from the
maintainer and push to your own, which is another.

As to your suggestion, I do think it is reasonable to clarify these
triangles with an illustration, and to even add descriptions for
short-cut configurations as a side note.

Re: [PATCH v2 0/6] Support triangular workflows

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> Shouldn't Documentation/gitworkflows.txt also be updated with the
>> triangular workflow and its configuration?
>
> What is missing from gitworkflows documentation is actually a
> non-triangular workflow, where people pull from and push into the
> same central repository.
>
> The "merge workflow" part in the "distributed workflows" section
> teaches:
>
>  * fetching others' work from <remote> and merging it to yours;
>  * publishing your work to <remote>; and
>  * advertising your work.
>
> and it is written for the triangular workflow.
>
> Two triangles are involved.  The maintainer may pull from you but
> pushes to her own, which is one triangle, and you pull from the
> maintainer and push to your own, which is another.
>
> As to your suggestion, I do think it is reasonable to clarify these
> triangles with an illustration, and to even add descriptions for
> short-cut configurations as a side note.

I think the gitworkflows document should be rewritten with focus on
setting up remotes and configuration variables for specific workflows.
 Once these are set up, pushing/ pulling (git pull is currently
broken, although I'm working on fixing it) should Just Work.  To push
to a different remote/ refspec, the user can read the manpage of git
push/ git pull.  A workflow is about setting up good defaults that
work 90% of the time with no additional effort.

[PATCH v3 0/6] Support triangular workflows

[Ramkumar Ramachandra]

After Jonathan's review of [1], I've decided to pick two changes to
apply in this iteration:

1. [1/6], [5/6] and [6/6] now use the return value of
   git_config_string(), hence handling configuration errors properly.
   I consider this a major and important improvement.

2. The commit message in [4/6] has been augmented with a small
   example.  This is mainly a nit, as full-blown examples are already
   present in the next two patches in the series.

Thanks.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/218598

Ramkumar Ramachandra (6):
  remote.c: simplify a bit of code using git_config_string()
  t5516 (fetch-push): update test description
  t5516 (fetch-push): introduce mk_test_with_name()
  remote.c: introduce a way to have different remotes for fetch/push
  remote.c: introduce remote.pushdefault
  remote.c: introduce branch.<name>.pushremote

 Documentation/config.txt | 23 +++++++++++++++---
 builtin/push.c           |  2 +-
 remote.c                 | 39 ++++++++++++++++++++++++------
 remote.h                 |  1 +
 t/t5516-fetch-push.sh    | 63 ++++++++++++++++++++++++++++++++++++++++--------
 5 files changed, 107 insertions(+), 21 deletions(-)

-- 
1.8.2.62.ga35d936.dirty

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

[Ramkumar Ramachandra]

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().

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5516-fetch-push.sh | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index bfeec60..a546c2c 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -7,27 +7,32 @@ test_description='fetching and pushing'
 D=`pwd`
 
 mk_empty () {
-	rm -fr testrepo &&
-	mkdir testrepo &&
+	repo_name="$1"
+	test -z "$repo_name" && repo_name=testrepo
+	rm -fr $repo_name &&
+	mkdir $repo_name &&
 	(
-		cd testrepo &&
+		cd $repo_name &&
 		git init &&
 		git config receive.denyCurrentBranch warn &&
 		mv .git/hooks .git/hooks-disabled
 	)
 }
 
-mk_test () {
-	mk_empty &&
+mk_test_with_name () {
+	repo_name="$1"
+	shift
+
+	mk_empty $repo_name &&
 	(
 		for ref in "$@"
 		do
-			git push testrepo $the_first_commit:refs/$ref || {
+			git push $repo_name $the_first_commit:refs/$ref || {
 				echo "Oops, push refs/$ref failure"
 				exit 1
 			}
 		done &&
-		cd testrepo &&
+		cd $repo_name &&
 		for ref in "$@"
 		do
 			r=$(git show-ref -s --verify refs/$ref) &&
@@ -40,6 +45,10 @@ mk_test () {
 	)
 }
 
+mk_test () {
+	mk_test_with_name testrepo "$@"
+}
+
 mk_test_with_hooks() {
 	mk_test "$@" &&
 	(
@@ -79,9 +88,12 @@ mk_child() {
 	git clone testrepo "$1"
 }
 
-check_push_result () {
+check_push_result_with_name () {
+	repo_name="$1"
+	shift
+
 	(
-		cd testrepo &&
+		cd $repo_name &&
 		it="$1" &&
 		shift
 		for ref in "$@"
@@ -96,6 +108,10 @@ check_push_result () {
 	)
 }
 
+check_push_result () {
+	check_push_result_with_name testrepo "$@"
+}
+
 test_expect_success setup '
 
 	>path1 &&
-- 
1.8.2.62.ga35d936.dirty

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

[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 <peff@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
 '
 
 test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '
-	mk_empty &&
+	mk_empty testrepo &&
 	TRASH="$(pwd)/" &&
 	git config "url.trash2/.pushInsteadOf" trash/ &&
 	git config remote.r.url trash/wrong &&
@@ -262,242 +254,242 @@ test_expect_success 'push with config remote.*.push = HEAD' '
 
 test_expect_success 'push with matching heads' '
 
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	git push testrepo &&
-	check_push_result $the_commit heads/master
+	check_push_result testrepo $the_commit heads/master
 
 '
 
 test_expect_success 'push with matching heads on the command line' '
 
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	git push testrepo : &&
-	check_push_result $the_commit heads/master
+	check_push_result testrepo $the_commit heads/master
 
 '
 
 test_expect_success 'failed (non-fast-forward) push with matching heads' '
 
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	git push testrepo : &&
 	git commit --amend -massaged &&
 	test_must_fail git push testrepo &&
-	check_push_result $the_commit heads/master &&
+	check_push_result testrepo $the_commit heads/master &&
 	git reset --hard $the_commit
 
 '
 
 test_expect_success 'push --force with matching heads' '
 
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	git push testrepo : &&
 	git commit --amend -massaged &&
 	git push --force testrepo &&
-	! check_push_result $the_commit heads/master &&
+	! check_push_result testrepo $the_commit heads/master &&
 	git reset --hard $the_commit
 
 '
 
 test_expect_success 'push with matching heads and forced update' '
 
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	git push testrepo : &&
 	git commit --amend -massaged &&
 	git push testrepo +: &&
-	! check_push_result $the_commit heads/master &&
+	! check_push_result testrepo $the_commit heads/master &&
 	git reset --hard $the_commit
 
 '
 
 test_expect_success 'push with no ambiguity (1)' '
 
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	git push testrepo master:master &&
-	check_push_result $the_commit heads/master
+	check_push_result testrepo $the_commit heads/master
 
 '
 
 test_expect_success 'push with no ambiguity (2)' '
 
-	mk_test remotes/origin/master &&
+	mk_test testrepo remotes/origin/master &&
 	git push testrepo master:origin/master &&
-	check_push_result $the_commit remotes/origin/master
+	check_push_result testrepo $the_commit remotes/origin/master
 
 '
 
 test_expect_success 'push with colon-less refspec, no ambiguity' '
 
-	mk_test heads/master heads/t/master &&
+	mk_test testrepo heads/master heads/t/master &&
 	git branch -f t/master master &&
 	git push testrepo master &&
-	check_push_result $the_commit heads/master &&
-	check_push_result $the_first_commit heads/t/master
+	check_push_result testrepo $the_commit heads/master &&
+	check_push_result testrepo $the_first_commit heads/t/master
 
 '
 
 test_expect_success 'push with weak ambiguity (1)' '
 
-	mk_test heads/master remotes/origin/master &&
+	mk_test testrepo heads/master remotes/origin/master &&
 	git push testrepo master:master &&
-	check_push_result $the_commit heads/master &&
-	check_push_result $the_first_commit remotes/origin/master
+	check_push_result testrepo $the_commit heads/master &&
+	check_push_result testrepo $the_first_commit remotes/origin/master
 
 '
 
 test_expect_success 'push with weak ambiguity (2)' '
 
-	mk_test heads/master remotes/origin/master remotes/another/master &&
+	mk_test testrepo heads/master remotes/origin/master remotes/another/master &&
 	git push testrepo master:master &&
-	check_push_result $the_commit heads/master &&
-	check_push_result $the_first_commit remotes/origin/master remotes/another/master
+	check_push_result testrepo $the_commit heads/master &&
+	check_push_result testrepo $the_first_commit remotes/origin/master remotes/another/master
 
 '
 
 test_expect_success 'push with ambiguity' '
 
-	mk_test heads/frotz tags/frotz &&
+	mk_test testrepo heads/frotz tags/frotz &&
 	if git push testrepo master:frotz
 	then
 		echo "Oops, should have failed"
 		false
 	else
-		check_push_result $the_first_commit heads/frotz tags/frotz
+		check_push_result testrepo $the_first_commit heads/frotz tags/frotz
 	fi
 
 '
 
 test_expect_success 'push with colon-less refspec (1)' '
 
-	mk_test heads/frotz tags/frotz &&
+	mk_test testrepo heads/frotz tags/frotz &&
 	git branch -f frotz master &&
 	git push testrepo frotz &&
-	check_push_result $the_commit heads/frotz &&
-	check_push_result $the_first_commit tags/frotz
+	check_push_result testrepo $the_commit heads/frotz &&
+	check_push_result testrepo $the_first_commit tags/frotz
 
 '
 
 test_expect_success 'push with colon-less refspec (2)' '
 
-	mk_test heads/frotz tags/frotz &&
+	mk_test testrepo heads/frotz tags/frotz &&
 	if git show-ref --verify -q refs/heads/frotz
 	then
 		git branch -D frotz
 	fi &&
 	git tag -f frotz &&
 	git push -f testrepo frotz &&
-	check_push_result $the_commit tags/frotz &&
-	check_push_result $the_first_commit heads/frotz
+	check_push_result testrepo $the_commit tags/frotz &&
+	check_push_result testrepo $the_first_commit heads/frotz
 
 '
 
 test_expect_success 'push with colon-less refspec (3)' '
 
-	mk_test &&
+	mk_test testrepo &&
 	if git show-ref --verify -q refs/tags/frotz
 	then
 		git tag -d frotz
 	fi &&
 	git branch -f frotz master &&
 	git push testrepo frotz &&
-	check_push_result $the_commit heads/frotz &&
+	check_push_result testrepo $the_commit heads/frotz &&
 	test 1 = $( cd testrepo && git show-ref | wc -l )
 '
 
 test_expect_success 'push with colon-less refspec (4)' '
 
-	mk_test &&
+	mk_test testrepo &&
 	if git show-ref --verify -q refs/heads/frotz
 	then
 		git branch -D frotz
 	fi &&
 	git tag -f frotz &&
 	git push testrepo frotz &&
-	check_push_result $the_commit tags/frotz &&
+	check_push_result testrepo $the_commit tags/frotz &&
 	test 1 = $( cd testrepo && git show-ref | wc -l )
 
 '
 
 test_expect_success 'push head with non-existent, incomplete dest' '
 
-	mk_test &&
+	mk_test testrepo &&
 	git push testrepo master:branch &&
-	check_push_result $the_commit heads/branch
+	check_push_result testrepo $the_commit heads/branch
 
 '
 
 test_expect_success 'push tag with non-existent, incomplete dest' '
 
-	mk_test &&
+	mk_test testrepo &&
 	git tag -f v1.0 &&
 	git push testrepo v1.0:tag &&
-	check_push_result $the_commit tags/tag
+	check_push_result testrepo $the_commit tags/tag
 
 '
 
 test_expect_success 'push sha1 with non-existent, incomplete dest' '
 
-	mk_test &&
+	mk_test testrepo &&
 	test_must_fail git push testrepo `git rev-parse master`:foo
 
 '
 
 test_expect_success 'push ref expression with non-existent, incomplete dest' '
 
-	mk_test &&
+	mk_test testrepo &&
 	test_must_fail git push testrepo master^:branch
 
 '
 
 test_expect_success 'push with HEAD' '
 
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	git checkout master &&
 	git push testrepo HEAD &&
-	check_push_result $the_commit heads/master
+	check_push_result testrepo $the_commit heads/master
 
 '
 
 test_expect_success 'push with HEAD nonexisting at remote' '
 
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	git checkout -b local master &&
 	git push testrepo HEAD &&
-	check_push_result $the_commit heads/local
+	check_push_result testrepo $the_commit heads/local
 '
 
 test_expect_success 'push with +HEAD' '
 
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	git checkout master &&
 	git branch -D local &&
 	git checkout -b local &&
 	git push testrepo master local &&
-	check_push_result $the_commit heads/master &&
-	check_push_result $the_commit heads/local &&
+	check_push_result testrepo $the_commit heads/master &&
+	check_push_result testrepo $the_commit heads/local &&
 
 	# Without force rewinding should fail
 	git reset --hard HEAD^ &&
 	test_must_fail git push testrepo HEAD &&
-	check_push_result $the_commit heads/local &&
+	check_push_result testrepo $the_commit heads/local &&
 
 	# With force rewinding should succeed
 	git push testrepo +HEAD &&
-	check_push_result $the_first_commit heads/local
+	check_push_result testrepo $the_first_commit heads/local
 
 '
 
 test_expect_success 'push HEAD with non-existent, incomplete dest' '
 
-	mk_test &&
+	mk_test testrepo &&
 	git checkout master &&
 	git push testrepo HEAD:branch &&
-	check_push_result $the_commit heads/branch
+	check_push_result testrepo $the_commit heads/branch
 
 '
 
 test_expect_success 'push with config remote.*.push = HEAD' '
 
-	mk_test heads/local &&
+	mk_test testrepo heads/local &&
 	git checkout master &&
 	git branch -f local $the_commit &&
 	(
@@ -509,8 +501,8 @@ test_expect_success 'push with config remote.*.push = HEAD' '
 	git config remote.there.push HEAD &&
 	git config branch.master.remote there &&
 	git push &&
-	check_push_result $the_commit heads/master &&
-	check_push_result $the_first_commit heads/local
+	check_push_result testrepo $the_commit heads/master &&
+	check_push_result testrepo $the_first_commit heads/local
 '
 
 # clean up the cruft left with the previous one
@@ -519,12 +511,12 @@ test_expect_success 'push with config remote.*.pushurl' '
 
 test_expect_success 'push with config remote.*.pushurl' '
 
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	git checkout master &&
 	git config remote.there.url test2repo &&
 	git config remote.there.pushurl testrepo &&
 	git push there &&
-	check_push_result $the_commit heads/master
+	check_push_result testrepo $the_commit heads/master
 '
 
 # clean up the cruft left with the previous one
@@ -532,19 +524,19 @@ test_expect_success 'push updates local refs' '
 
 test_expect_success 'push with dry-run' '
 
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	(
 		cd testrepo &&
 		old_commit=$(git show-ref -s --verify refs/heads/master)
 	) &&
 	git push --dry-run testrepo &&
-	check_push_result $old_commit heads/master
+	check_push_result testrepo $old_commit heads/master
 '
 
 test_expect_success 'push updates local refs' '
 
-	mk_test heads/master &&
-	mk_child child &&
+	mk_test testrepo heads/master &&
+	mk_child testrepo child &&
 	(
 		cd child &&
 		git pull .. master &&
@@ -557,9 +549,9 @@ test_expect_success 'push updates up-to-date local refs' '
 
 test_expect_success 'push updates up-to-date local refs' '
 
-	mk_test heads/master &&
-	mk_child child1 &&
-	mk_child child2 &&
+	mk_test testrepo heads/master &&
+	mk_child testrepo child1 &&
+	mk_child testrepo child2 &&
 	(cd child1 && git pull .. master && git push) &&
 	(
 		cd child2 &&
@@ -573,8 +565,8 @@ test_expect_success 'push preserves up-to-date packed refs' '
 
 test_expect_success 'push preserves up-to-date packed refs' '
 
-	mk_test heads/master &&
-	mk_child child &&
+	mk_test testrepo heads/master &&
+	mk_child testrepo child &&
 	(
 		cd child &&
 		git push &&
@@ -585,8 +577,8 @@ test_expect_success 'push does not update local refs on failure' '
 
 test_expect_success 'push does not update local refs on failure' '
 
-	mk_test heads/master &&
-	mk_child child &&
+	mk_test testrepo heads/master &&
+	mk_child testrepo child &&
 	mkdir testrepo/.git/hooks &&
 	echo "#!/no/frobnication/today" >testrepo/.git/hooks/pre-receive &&
 	chmod +x testrepo/.git/hooks/pre-receive &&
@@ -602,7 +594,7 @@ test_expect_success 'allow deleting an invalid remote ref' '
 
 test_expect_success 'allow deleting an invalid remote ref' '
 
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	rm -f testrepo/.git/objects/??/* &&
 	git push testrepo :refs/heads/master &&
 	(cd testrepo && test_must_fail git rev-parse --verify refs/heads/master)
@@ -610,7 +602,7 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
 '
 
 test_expect_success 'pushing valid refs triggers post-receive and post-update hooks' '
-	mk_test_with_hooks heads/master heads/next &&
+	mk_test_with_hooks testrepo heads/master heads/next &&
 	orgmaster=$(cd testrepo && git show-ref -s --verify refs/heads/master) &&
 	newmaster=$(git show-ref -s --verify refs/heads/master) &&
 	orgnext=$(cd testrepo && git show-ref -s --verify refs/heads/next) &&
@@ -646,7 +638,7 @@ test_expect_success 'deleting dangling ref triggers hooks with correct args' '
 '
 
 test_expect_success 'deleting dangling ref triggers hooks with correct args' '
-	mk_test_with_hooks heads/master &&
+	mk_test_with_hooks testrepo heads/master &&
 	rm -f testrepo/.git/objects/??/* &&
 	git push testrepo :refs/heads/master &&
 	(
@@ -675,7 +667,7 @@ test_expect_success 'deletion of a non-existent ref is not fed to post-receive a
 '
 
 test_expect_success 'deletion of a non-existent ref is not fed to post-receive and post-update hooks' '
-	mk_test_with_hooks heads/master &&
+	mk_test_with_hooks testrepo heads/master &&
 	orgmaster=$(cd testrepo && git show-ref -s --verify refs/heads/master) &&
 	newmaster=$(git show-ref -s --verify refs/heads/master) &&
 	git push testrepo master :refs/heads/nonexistent &&
@@ -707,7 +699,7 @@ test_expect_success 'deletion of a non-existent ref alone does trigger post-rece
 '
 
 test_expect_success 'deletion of a non-existent ref alone does trigger post-receive and post-update hooks' '
-	mk_test_with_hooks heads/master &&
+	mk_test_with_hooks testrepo heads/master &&
 	git push testrepo :refs/heads/nonexistent &&
 	(
 		cd testrepo/.git &&
@@ -727,7 +719,7 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w
 '
 
 test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks with correct input' '
-	mk_test_with_hooks heads/master heads/next heads/pu &&
+	mk_test_with_hooks testrepo heads/master heads/next heads/pu &&
 	orgmaster=$(cd testrepo && git show-ref -s --verify refs/heads/master) &&
 	newmaster=$(git show-ref -s --verify refs/heads/master) &&
 	orgnext=$(cd testrepo && git show-ref -s --verify refs/heads/next) &&
@@ -773,14 +765,14 @@ test_expect_success 'allow deleting a tag using --delete' '
 '
 
 test_expect_success 'allow deleting a ref using --delete' '
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	(cd testrepo && git config receive.denyDeleteCurrent warn) &&
 	git push testrepo --delete master &&
 	(cd testrepo && test_must_fail git rev-parse --verify refs/heads/master)
 '
 
 test_expect_success 'allow deleting a tag using --delete' '
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	git tag -a -m dummy_message deltag heads/master &&
 	git push testrepo --tags &&
 	(cd testrepo && git rev-parse --verify -q refs/tags/deltag) &&
@@ -789,17 +781,17 @@ test_expect_success 'warn on push to HEAD of non-bare repository' '
 '
 
 test_expect_success 'push --delete without args aborts' '
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	test_must_fail git push testrepo --delete
 '
 
 test_expect_success 'push --delete refuses src:dest refspecs' '
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	test_must_fail git push testrepo --delete master:foo
 '
 
 test_expect_success 'warn on push to HEAD of non-bare repository' '
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	(
 		cd testrepo &&
 		git checkout master &&
@@ -810,7 +802,7 @@ test_expect_success 'deny push to HEAD of non-bare repository' '
 '
 
 test_expect_success 'deny push to HEAD of non-bare repository' '
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	(
 		cd testrepo &&
 		git checkout master &&
@@ -820,7 +812,7 @@ test_expect_success 'allow push to HEAD of bare repository (bare)' '
 '
 
 test_expect_success 'allow push to HEAD of bare repository (bare)' '
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	(
 		cd testrepo &&
 		git checkout master &&
@@ -832,7 +824,7 @@ test_expect_success 'allow push to HEAD of non-bare repository (config)' '
 '
 
 test_expect_success 'allow push to HEAD of non-bare repository (config)' '
-	mk_test heads/master &&
+	mk_test testrepo heads/master &&
 	(
 		cd testrepo &&
 		git checkout master &&
@@ -843,7 +835,7 @@ test_expect_success 'fetch with branches' '
 '
 
 test_expect_success 'fetch with branches' '
-	mk_empty &&
+	mk_empty testrepo &&
 	git branch second $the_first_commit &&
 	git checkout second &&
 	echo ".." > testrepo/.git/branches/branch1 &&
@@ -858,7 +850,7 @@ test_expect_success 'fetch with branches containing #' '
 '
 
 test_expect_success 'fetch with branches containing #' '
-	mk_empty &&
+	mk_empty testrepo &&
 	echo "..#second" > testrepo/.git/branches/branch2 &&
 	(
 		cd testrepo &&
@@ -871,7 +863,7 @@ test_expect_success 'push with branches' '
 '
 
 test_expect_success 'push with branches' '
-	mk_empty &&
+	mk_empty testrepo &&
 	git checkout second &&
 	echo "testrepo" > .git/branches/branch1 &&
 	git push branch1 &&
@@ -884,7 +876,7 @@ test_expect_success 'push with branches containing #' '
 '
 
 test_expect_success 'push with branches containing #' '
-	mk_empty &&
+	mk_empty testrepo &&
 	echo "testrepo#branch3" > .git/branches/branch2 &&
 	git push branch2 &&
 	(
@@ -897,9 +889,9 @@ test_expect_success 'push into aliased refs (consistent)' '
 '
 
 test_expect_success 'push into aliased refs (consistent)' '
-	mk_test heads/master &&
-	mk_child child1 &&
-	mk_child child2 &&
+	mk_test testrepo heads/master &&
+	mk_child testrepo child1 &&
+	mk_child testrepo child2 &&
 	(
 		cd child1 &&
 		git branch foo &&
@@ -919,9 +911,9 @@ test_expect_success 'push into aliased refs (inconsistent)' '
 '
 
 test_expect_success 'push into aliased refs (inconsistent)' '
-	mk_test heads/master &&
-	mk_child child1 &&
-	mk_child child2 &&
+	mk_test testrepo heads/master &&
+	mk_child testrepo child1 &&
+	mk_child testrepo child2 &&
 	(
 		cd child1 &&
 		git branch foo &&
@@ -946,9 +938,9 @@ test_expect_success 'push requires --force to update lightweight tag' '
 '
 
 test_expect_success 'push requires --force to update lightweight tag' '
-	mk_test heads/master &&
-	mk_child child1 &&
-	mk_child child2 &&
+	mk_test testrepo heads/master &&
+	mk_child testrepo child1 &&
+	mk_child testrepo child2 &&
 	(
 		cd child1 &&
 		git tag Tag &&
@@ -967,7 +959,7 @@ test_expect_success 'push --porcelain' '
 '
 
 test_expect_success 'push --porcelain' '
-	mk_empty &&
+	mk_empty testrepo &&
 	echo >.git/foo  "To testrepo" &&
 	echo >>.git/foo "*	refs/heads/master:refs/remotes/origin/master	[new branch]"  &&
 	echo >>.git/foo "Done" &&
@@ -982,13 +974,13 @@ test_expect_success 'push --porcelain rejected' '
 '
 
 test_expect_success 'push --porcelain bad url' '
-	mk_empty &&
+	mk_empty testrepo &&
 	test_must_fail git push >.git/bar --porcelain asdfasdfasd refs/heads/master:refs/remotes/origin/master &&
 	test_must_fail grep -q Done .git/bar
 '
 
 test_expect_success 'push --porcelain rejected' '
-	mk_empty &&
+	mk_empty testrepo &&
 	git push testrepo refs/heads/master:refs/remotes/origin/master &&
 	(cd testrepo &&
 		git reset --hard origin/master^
@@ -1002,7 +994,7 @@ test_expect_success 'push --porcelain --dry-run rejected' '
 '
 
 test_expect_success 'push --porcelain --dry-run rejected' '
-	mk_empty &&
+	mk_empty testrepo &&
 	git push testrepo refs/heads/master:refs/remotes/origin/master &&
 	(cd testrepo &&
 		git reset --hard origin/master
@@ -1017,25 +1009,25 @@ do
 '
 
 test_expect_success 'push --prune' '
-	mk_test heads/master heads/second heads/foo heads/bar &&
+	mk_test testrepo heads/master heads/second heads/foo heads/bar &&
 	git push --prune testrepo &&
-	check_push_result $the_commit heads/master &&
-	check_push_result $the_first_commit heads/second &&
-	! check_push_result $the_first_commit heads/foo heads/bar
+	check_push_result testrepo $the_commit heads/master &&
+	check_push_result testrepo $the_first_commit heads/second &&
+	! check_push_result testrepo $the_first_commit heads/foo heads/bar
 '
 
 test_expect_success 'push --prune refspec' '
-	mk_test tmp/master tmp/second tmp/foo tmp/bar &&
+	mk_test testrepo tmp/master tmp/second tmp/foo tmp/bar &&
 	git push --prune testrepo "refs/heads/*:refs/tmp/*" &&
-	check_push_result $the_commit tmp/master &&
-	check_push_result $the_first_commit tmp/second &&
-	! check_push_result $the_first_commit tmp/foo tmp/bar
+	check_push_result testrepo $the_commit tmp/master &&
+	check_push_result testrepo $the_first_commit tmp/second &&
+	! check_push_result testrepo $the_first_commit tmp/foo tmp/bar
 '
 
 for configsection in transfer receive
 do
 	test_expect_success "push to update a ref hidden by $configsection.hiderefs" '
-		mk_test heads/master hidden/one hidden/two hidden/three &&
+		mk_test testrepo heads/master hidden/one hidden/two hidden/three &&
 		(
 			cd testrepo &&
 			git config $configsection.hiderefs refs/hidden
@@ -1043,32 +1035,32 @@ test_expect_success 'fetch exact SHA1' '
 
 		# push to unhidden ref succeeds normally
 		git push testrepo master:refs/heads/master &&
-		check_push_result $the_commit heads/master &&
+		check_push_result testrepo $the_commit heads/master &&
 
 		# push to update a hidden ref should fail
 		test_must_fail git push testrepo master:refs/hidden/one &&
-		check_push_result $the_first_commit hidden/one &&
+		check_push_result testrepo $the_first_commit hidden/one &&
 
 		# push to delete a hidden ref should fail
 		test_must_fail git push testrepo :refs/hidden/two &&
-		check_push_result $the_first_commit hidden/two &&
+		check_push_result testrepo $the_first_commit hidden/two &&
 
 		# idempotent push to update a hidden ref should fail
 		test_must_fail git push testrepo $the_first_commit:refs/hidden/three &&
-		check_push_result $the_first_commit hidden/three
+		check_push_result testrepo $the_first_commit hidden/three
 	'
 done
 
 test_expect_success 'fetch exact SHA1' '
-	mk_test heads/master hidden/one &&
+	mk_test testrepo heads/master hidden/one &&
 	git push testrepo master:refs/hidden/one &&
 	(
 		cd testrepo &&
 		git config transfer.hiderefs refs/hidden
 	) &&
-	check_push_result $the_commit hidden/one &&
+	check_push_result testrepo $the_commit hidden/one &&
 
-	mk_child child &&
+	mk_child testrepo child &&
 	(
 		cd child &&
 
-- 
1.8.2.13.g0f18d3c

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

[Junio C Hamano]

Jeff King <peff@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.

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

[Jeff King]

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

> Jeff King <peff@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

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

[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.

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

[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.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@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-<.

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

[Jeff King]

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

> Ramkumar Ramachandra <artagnon@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