Re: [PATCH v2] New config push.default to decide default behavior for push

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 14, 2009, 20:56 UTC
Message-ID: <7viqmbakmt.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20090312115433.GA2848@pvv.org>

Finn Arne Gangstad <finnag@pvv.org> writes:

> Something like this amended into the last commit? I can amend it on top
> of the last one and resend if that is better.

Thanks.

I looked at these two patches after squashing them into one, and I think it makes sense as the final shape of a two patch series.

Although the purist in me tells me that addition of the "tracking" and the "current" should be in a separate commit as they are purely new features, I think it is Ok. In that sense, "nothing" is a new feature anyway, and "current" is something we already have, so the true addition here is only the "tracking" one.

I also reworded the commit log message a bit, like this:

    Author: Finn Arne Gangstad <finnag@pvv.org>
    Date:   Wed Mar 11 23:01:45 2009 +0100

    New config push.default to decide default behavior for push
    
    When "git push" is not told what refspecs to push, it pushes all matching
    branches to the current remote.  For some workflows this default is not
    useful, and surprises new users.  Some have even found that this default
    behaviour too easy to trigger by accident with unwanted consequences.
    
    Introduce a new configuration variable "push.default" that decides what
    action git push should take if no refspecs are given or implied by the
    command line arguments or the current remote configuration. If this
    variable is unconfigured, display a prominent warning when default
    behavior is triggered.
    
    Possible values are:
    
      'nothing'  : Push nothing;
      'matching' : Current default behaviour, push all branches that already
                   exist in the current remote;
      'tracking' : Push the current branch to whatever it is tracking;
      'current'  : Push the current branch to a branch of the same name,
      	           i.e. HEAD.

    Signed-off-by: Finn Arne Gangstad <finnag@pvv.org>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

I however had to scratch my head for 20 seconds wondering where the push happens when do_default_push() codepath is taken, and it turns out that the function is there merely to set up the push refspecs for the default case; the function is misnamed. I'd further squash the following patch in.

diff --git a/builtin-push.c b/builtin-push.c
index 51f4c4a..e4988da 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -76,8 +76,7 @@ static const char *warn_unconfigured_push_msg[] = {
 	"  'nothing'  : Do not push anythig",
 	"  'matching' : Push all matching branches (the current default)",
 	"  'tracking' : Push the current branch to whatever it is tracking",
-	"  'current'  : Push the current branch",
-	""
+	"  'current'  : Push the current branch"
 };
 
 static void warn_unconfigured_push(void)
@@ -87,7 +86,7 @@ static void warn_unconfigured_push(void)
 		warning("%s", warn_unconfigured_push_msg[i]);
 }
 
-static void do_default_push(void)
+static void setup_default_push_refspecs(void)
 {
 	git_config(git_default_config, NULL);
 	switch (push_default) {
@@ -147,7 +146,7 @@ static int do_push(const char *repo, int flags)
 			refspec = remote->push_refspec;
 			refspec_nr = remote->push_refspec_nr;
 		} else if (!(flags & TRANSPORT_PUSH_MIRROR))
-			do_default_push();
+			setup_default_push_refspecs();
 	}
 	errs = 0;
 	for (i = 0; i < remote->url_nr; i++) {