Re: [PATCH 2/2] push: Add '--current', which pushes only the current branch

[Steffen Prohaska <prohaska@zib.de>]

[-- Attachment #1: Type: text/plain, Size: 6202 bytes --]


On Nov 19, 2007, at 2:28 AM, Junio C Hamano wrote:

> Steffen Prohaska <prohaska@zib.de> writes:
>
>> Often you want to push only the current branch to the default
>> remote.  This was awkward to do in the past.
>
> While I think --current is a handy shorthand to have, I do not
> think the above description is correct.
>
> Wouldn't your earlier patch have allowed the configuration setting:
>
> 	[remote "$there"]
>         	push = HEAD
> 	[branch "$current"]
>         	remote = $there
>
> to work very well already?

No.  This was the case in the verst first version of the patch
series.  Someone, I don't remember who, proposed to put the
resolution of HEAD into builtin-push.c.  This simplified code
a lot.  Now, HEAD is resolved when parsing the command line.
At that time it is replaced with the full local refname.
The refspec parsing code never sees HEAD, and it won't
understand it.  Try the above setup, and you'll see that it
doesn't work.

Anyway it's not needed if we proceed as outlined below.


> I do not think it is "Often you want" that makes it awkward.
>
> Instead, the awkward case is if you do the "only the current"
> push NOT often enough.  If it is often enough, you set the
> configuration once and the awkwardness is behind you.
>
> If however it is not often enough, you cannot afford to have the
> configuration above, because that would force you to tell from
> the command line which branches, not just the current one, to
> push, and that is inconvenient because it is not rare enough.

Will try to rephrase the commit message.


> Together with your [PATCH 1/2], I like the general direction
> these patckes are taking us, but it feels a bit too hasty.  I
> personally am not convinced that switching to --current for
> everybody is a good move.
>
>> ...
>> Maybe in two years (that's twice an eternity in git time scales):
>>
>> 4) make "git push --current" the default.
>
> If these, both the uncertainly expressed by "Maybe" and "twice
> an eternity" are true, which they are, the new warning in the
> current patch are inappropriate.  Many people's settings depend
> on a working "push the matching refs" behaviour, and we need a
> very good excuse to annoy the existing happy users with such a
> warning.

I think 3) is the interesting case.  "git push" should do
nothing by default.  Either you can configure "git push" to do
something by setting a remote.$remote.push line or you need
to provide a command line switch.  But if you do not tell
explicitly what you want, "git push" will not do anything
for you.

I'm not sure if we ever switch to 4).  But already with 3) the
default changed.  So a warning seems to be appropriate.  But if
we go as outlined below, it's probably a different warning.

I attached a patch that illustrates what "do nothing by default"
means.  This patch should _not_ be applied.  It's only an
illustration what I'm working on.


> Remember, how much vocal the dissenters might have been on the
> list in the recent discussions, we need to consider the needs of
> the silent majority that has been content with the current
> behaviour for a long time.
>
> The "warning" to annoy them may be a way to get their attention
> and get them involved in a discussion to decide what the default
> should be.  But changing the default without giving the people
> who do not like the _new_ default a way to avoid inconvenience
> of always typing --matching or --current is not nice.  And
> honestly, I do not think there is one single default that is
> good for everybody.

Personally, I'd switch to the do-nothing default immediately.
But you are right.  More work is needed to have a smooth transition.


> We should be doing better.
>
> A smoother transition route would be:
>
>  - Keep "matching" the built-in default for now;
>
>  - Take your patches (but drop "warning" bits at this point) to
>    introduce 'matching' and 'current' behaviours, and a way to
>    override the built-in default from the command line;
>
>  - Introduce a configuration 'push.defaultRefs' ('matching' or
>    'current') to override that built-in default, so people who
>    prefer 'current' can override the built-in default, without
>    having to type --current every time.

Sounds like a plan.

If we have the configuration variable, maybe we could switch
off the default behaviour immediately.  Setting a single global
config variable once would be sufficient to get it back.  So,
we could change the default and print a recommendation to run
'git config --global push.defaultRefs matching' to get it back.

...

> After all that happens, we can start discussing what the
> built-in default should be.  When it is changed after the
> discussion concludes (which may never happen), people who want
> to keep 'matching' behaviour would have had the configuration
> mechanism to override that built-in default for some time during
> the discussion period.  So the beginning of that discussion
> period is when we should start talking about "We might change
> the default soon; set the configuration to your liking if you do
> not want to get affected" in the warning.

... And we'd not even start the discussion.  Because there's no
need to.  Every user should make a choice, once.  We do not
provide a default (which obviously will trigger another discussion ;)


> Then after that, we may or may not decide to change the default.
> Even if we end up not changing the default, 'current' people
> will then have a way (push.matching = false) to tailor their git
> for their liking, so everybody wins.
>
>>  DESCRIPTION
>> @@ -63,6 +63,12 @@ the remote repository.
>>  	Instead of naming each ref to push, specifies that all
>>  	refs under `$GIT_DIR/refs/heads/` be pushed.
>>
>> +\--matching::
>> +	Instead of naming each ref to push, specifies that matching
>> +	refs under `$GIT_DIR/refs/heads/` be pushed.  Matching means
>> +	the branch exists locally and at the remote under the same name.
>> +	Currently, this is the default.  But this will change in the  
>> future.
>
> For the above reason, "But this will..." is a bit premature.

I'll change the plan and will come back with a full
implementation.

Thanks for the helpful comments.

	Steffen



[-- Attachment #2: 0001-push-do-nothing-by-default.patch --]
[-- Type: application/octet-stream, Size: 3054 bytes --]

From a97c117794c631f556f87419a3dbaa702b858d95 Mon Sep 17 00:00:00 2001
From: Steffen Prohaska <prohaska@zib.de>
Date: Sun, 18 Nov 2007 19:22:30 +0100
Subject: [PATCH] push: do nothing by default

We used to push all matching branches.  This behaviour
was suitable in many situations, but sometimes confusing.

This commit switches off the default.  push now dies instead.

WORK IN PROGRESS.  NOT-SIGNED-OFF.
---
 builtin-push.c        |    9 +++++++--
 t/t5516-fetch-push.sh |   13 ++++---------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index e5646d4..e637540 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -143,8 +143,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		fprintf(stderr, "--all, --matching, and --current are mutual exclusive.\n");
 		usage_with_options(push_usage, options);
 	}
-	if ((all || matching || current) && refspec)
-		usage_with_options(push_usage, options);
+	if (all || matching || current) {
+		if (refspec)
+			usage_with_options(push_usage, options);
+	} else {
+		if (!refspec)
+			die("No default action found.");
+	}
 	if (!all && !matching && !current && !refspec)
 		fprintf(stderr, "Warning: assuming '--matching'."
 		                " This default will change in the future.\n");
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 20e0796..48689b9 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -108,11 +108,6 @@ test_expect_code 129 'push command line options (2)' '
 	git push --matching testrepo master
 '
 
-test_expect_success 'push command line options (3)' '
-	git push testrepo 2>stderr.txt &&
-	grep -q "Warning: assuming.*--matching" stderr.txt
-'
-
 test_expect_code 129 'push command line options (4)' '
 	git push --all --current testrepo
 '
@@ -154,7 +149,7 @@ test_expect_success 'push with wildcard' '
 test_expect_success 'push with matching heads' '
 
 	mk_test heads/master &&
-	git push testrepo &&
+	git push --matching testrepo &&
 	check_push_result $the_commit heads/master
 
 '
@@ -319,7 +314,7 @@ test_expect_success 'push with dry-run' '
 	cd testrepo &&
 	old_commit=$(git show-ref -s --verify refs/heads/master) &&
 	cd .. &&
-	git push --dry-run testrepo &&
+	git push --dry-run --matching testrepo &&
 	check_push_result $old_commit heads/master
 '
 
@@ -331,7 +326,7 @@ test_expect_success 'push updates local refs' '
 	cd .. &&
 	git clone parent child && cd child &&
 		echo two >foo && git commit -a -m two &&
-		git push &&
+		git push --matching &&
 	test $(git rev-parse master) = $(git rev-parse remotes/origin/master)
 
 '
@@ -346,7 +341,7 @@ test_expect_success 'push does not update local refs on failure' '
 	cd .. &&
 	git clone parent child && cd child &&
 		echo two >foo && git commit -a -m two || exit 1
-		git push && exit 1
+		git push --matching && exit 1
 	test $(git rev-parse master) != $(git rev-parse remotes/origin/master)
 
 '
-- 
1.5.3.5.750.gc43b


[-- Attachment #3: Type: text/plain, Size: 1 bytes --]