Re: [PATCH] Allow tracking branches to set up...

Re: [PATCH] Allow tracking branches to set up rebase by default.

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:35

The patch is looking better.

I'd suggest further fixes like the attached patch to address the following
issues:

 - Do not ignore misconfiguration, but diagnose it.

 - die_bad_config() takes a variable name without surrounding explanatory
   text.  If you actually tested your patch and looked at the error
   message, it would have been blatantly obvious and you would have
   noticed it.  Not a good sign.

 - Test not just the success cases but failure cases; test not just
   explicitly configured cases but also default cases.

The last one is something everybody tends to forget, but is very
important.  It is human nature that anybody would eagerly want to
demonstrate what new things their shiny new toy does, but would forget to
make sure that it does not take effect when people do not want it (either
saying 'never' which you do have test, which is good, or not asking for it
by not setting the variable, which you didn't).  You'd notice that I
played lazy and added a check for only the "tracked remote" case when the
new variable is left unspecified, but you might want to add tests for
other variants.  Also I suspect that we would want to test cases where
autosetuprebase is given but autosetupmerge is not.

Another thing we might want to address is to move parsing of branch.*
configuration variables out of git_default_config().  They are unnecessary
for majority of commands that do not create new branches.  But that would
be a separate topic if we were to do so.


 config.c          |    8 +++++---
 t/t3200-branch.sh |   24 ++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index 7d76986..cf2bfd3 100644
--- a/config.c
+++ b/config.c

@@ -487,8 +487,10 @@ int git_default_config(const char *var, const char *value)
 		git_branch_track = git_config_bool(var, value);
 		return 0;
 	}
-	if (value && !strcmp(var, "branch.autosetuprebase")) {
-		if (!strcmp(value, "never"))
+	if (!strcmp(var, "branch.autosetuprebase")) {
+		if (!value)
+			return config_error_nonbool(var);
+		else if (!strcmp(value, "never"))
 			autorebase = AUTOREBASE_NEVER;
 		else if (!strcmp(value, "local"))
 			autorebase = AUTOREBASE_LOCAL;

@@ -497,7 +499,7 @@ int git_default_config(const char *var, const char *value)
 		else if (!strcmp(value, "always"))
 			autorebase = AUTOREBASE_ALWAYS;
 		else
-			die_bad_config("Invalid value for branch.autosetupmerge");
+			return error("Malformed value for %s", var);
 		return 0;
 	}

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index d11dd41..5cfeaa7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh

@@ -144,6 +144,18 @@ test_expect_success 'test tracking setup (non-wildcard, not matching)' \
      ! test "$(git config branch.my5.remote)" = local &&
      ! test "$(git config branch.my5.merge)" = refs/heads/master'
 
+test_expect_success 'detect misconfigured autosetuprebase' '
+	git config branch.autosetuprebase garbage &&
+	test_must_fail git branch
+'
+
+test_expect_success 'detect misconfigured autosetuprebase' '
+	git config --unset branch.autosetuprebase &&
+	echo "[branch] autosetuprebase" >>.git/config &&
+	test_must_fail git branch &&
+	git config --unset branch.autosetuprebase
+'
+
 test_expect_success 'test tracking setup via config' \
     'git config branch.autosetupmerge true &&
      git config remote.local.url . &&

@@ -316,4 +328,16 @@ test_expect_success 'autosetuprebase always on a tracked remote branch' '
 	test "$(git config branch.myr8.rebase)" = true
 '
 
+test_expect_success 'without autosetuprebase' '
+	git config --unset branch.autosetuprebase
+
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --track myr9 local/master &&
+	test "$(git config branch.myr9.remote)" = local &&
+	test "$(git config branch.myr9.merge)" = refs/heads/master &&
+	test "z$(git config branch.myr9.rebase)" = "z"
+'
+
 test_done

Re: [PATCH] Allow tracking branches to set up rebase by default.

From: Dustin Sallings <hidden>
Date: 2016-06-15 22:44:35

On May 10, 2008, at 11:41, Junio C Hamano wrote:

- die_bad_config() takes a variable name without surrounding  
explanatory
  text.  If you actually tested your patch and looked at the error
  message, it would have been blatantly obvious and you would have
  noticed it.  Not a good sign.

- Test not just the success cases but failure cases; test not just
  explicitly configured cases but also default cases.

	You're right, I didn't test misconfiguration cases.  I've updated the  
patch per your recommendations with a lot more tests and have manually  
tested the error messages to ensure they make sense.

	Does it make sense in the cases where a value is bad to list the  
valid values?  I don't see a precedent (so I don't know what format  
would be most desirable), but it might be friendly.

Also I suspect that we would want to test cases where
autosetuprebase is given but autosetupmerge is not.

	This code isn't so much dependent on that variable, but on the  
effects of that variable, which may be overridden by the --track and -- 
no-track parameters.  I think it'd be best to leave the effects of  
that variable in its own tests wrt tracking since this code doesn't  
kick in until after the tracking is set up.

Another thing we might want to address is to move parsing of branch.*
configuration variables out of git_default_config().  They are  
unnecessary
for majority of commands that do not create new branches.  But that  
would
be a separate topic if we were to do so.

	I agree with this.  I tried initially to not have it be part of the  
default config parser, but it seemed a lot more disruptive, so I just  
followed autosetupmerge.

	Thanks for your input again.  New patch to follow.

-- 
Dustin Sallings

[PATCH] Allow tracking branches to set up rebase by default.

From: Dustin Sallings <hidden>
Date: 2016-06-15 22:44:35

Change cd67e4d4 introduced a new configuration parameter that told
pull to automatically perform a rebase instead of a merge.  This
change provides a configuration option to enable this feature
automatically when creating a new branch.

If the variable branch.autosetuprebase applies for a branch that's
being created, that branch will have branch.<name>.rebase set to true.

Signed-off-by: Dustin Sallings <redacted>
---
 Documentation/config.txt |   15 +++
 branch.c                 |   22 ++++-
 cache.h                  |    8 ++
 config.c                 |   15 +++
 environment.c            |    1 +
 t/t3200-branch.sh        |  234 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 294 insertions(+), 1 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a6fc5a2..217980f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -399,6 +399,21 @@ branch.autosetupmerge::
 	done when the starting point is either a local branch or remote
 	branch. This option defaults to true.
 
+branch.autosetuprebase::
+	When a new branch is created with `git-branch` or `git-checkout`
+	that tracks another branch, this variable tells git to set
+	up pull to rebase instead of merge (see "branch.<name>.rebase").
+	When `never`, rebase is never automatically set to true.
+	When `local`, rebase is set to true for tracked branches of
+	other local branches.
+	When `remote`, rebase is set to true for tracked branches of
+	remote branches.
+	When `always`, rebase will be set to true for all tracking
+	branches.
+	See "branch.autosetupmerge" for details on how to set up a
+	branch to track another branch.
+	This option defaults to never.
+
 branch.<name>.remote::
 	When in branch <name>, it tells `git fetch` which remote to fetch.
 	If this option is not given, `git fetch` defaults to remote "origin".

diff --git a/branch.c b/branch.c
index daf862e..56e9492 100644
--- a/branch.c
+++ b/branch.c

@@ -32,6 +32,21 @@ static int find_tracked_branch(struct remote *remote, void *priv)
 	return 0;
 }
 
+static int should_setup_rebase(const struct tracking *tracking)
+{
+	switch (autorebase) {
+	case AUTOREBASE_NEVER:
+		return 0;
+	case AUTOREBASE_LOCAL:
+		return tracking->remote == NULL;
+	case AUTOREBASE_REMOTE:
+		return tracking->remote != NULL;
+	case AUTOREBASE_ALWAYS:
+		return 1;
+	}
+	return 0;
+}
+
 /*
  * This is called when new_ref is branched off of orig_ref, and tries
  * to infer the settings for branch.<new_ref>.{remote,merge} from the

@@ -69,9 +84,14 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
 	git_config_set(key, tracking.remote ?  tracking.remote : ".");
 	sprintf(key, "branch.%s.merge", new_ref);
 	git_config_set(key, tracking.src ? tracking.src : orig_ref);
-	free(tracking.src);
 	printf("Branch %s set up to track %s branch %s.\n", new_ref,
 		tracking.remote ? "remote" : "local", orig_ref);
+	if (should_setup_rebase(&tracking)) {
+		sprintf(key, "branch.%s.rebase", new_ref);
+		git_config_set(key, "true");
+		printf("This branch will rebase on pull.\n");
+	}
+	free(tracking.src);
 
 	return 0;
 }

diff --git a/cache.h b/cache.h
index 7fb8f33..3442130 100644
--- a/cache.h
+++ b/cache.h

@@ -434,7 +434,15 @@ enum branch_track {
 	BRANCH_TRACK_EXPLICIT,
 };
 
+enum rebase_setup_type {
+	AUTOREBASE_NEVER = 0,
+	AUTOREBASE_LOCAL,
+	AUTOREBASE_REMOTE,
+	AUTOREBASE_ALWAYS,
+};
+
 extern enum branch_track git_branch_track;
+extern enum rebase_setup_type autorebase;
 
 #define GIT_REPO_VERSION 0
 extern int repository_format_version;

diff --git a/config.c b/config.c
index b0ada51..cf2bfd3 100644
--- a/config.c
+++ b/config.c

@@ -487,6 +487,21 @@ int git_default_config(const char *var, const char *value)
 		git_branch_track = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "branch.autosetuprebase")) {
+		if (!value)
+			return config_error_nonbool(var);
+		else if (!strcmp(value, "never"))
+			autorebase = AUTOREBASE_NEVER;
+		else if (!strcmp(value, "local"))
+			autorebase = AUTOREBASE_LOCAL;
+		else if (!strcmp(value, "remote"))
+			autorebase = AUTOREBASE_REMOTE;
+		else if (!strcmp(value, "always"))
+			autorebase = AUTOREBASE_ALWAYS;
+		else
+			return error("Malformed value for %s", var);
+		return 0;
+	}
 
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;

diff --git a/environment.c b/environment.c
index fcd1ee5..4892a30 100644
--- a/environment.c
+++ b/environment.c

@@ -38,6 +38,7 @@ int auto_crlf = 0;	/* 1: both ways, -1: only when adding git objects */
 enum safe_crlf safe_crlf = SAFE_CRLF_WARN;
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
+enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
 
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
 char *git_work_tree_cfg;

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index cb5f7a4..8d87686 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh

@@ -224,4 +224,238 @@ test_expect_success 'avoid ambiguous track' '
 	test -z "$(git config branch.all1.merge)"
 '
 
+test_expect_success 'autosetuprebase local on a tracked local branch' '
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	git config branch.autosetuprebase local &&
+	(git show-ref -q refs/remotes/local/o || git-fetch local) &&
+	git branch mybase &&
+	git branch --track myr1 mybase &&
+	test "$(git config branch.myr1.remote)" = . &&
+	test "$(git config branch.myr1.merge)" = refs/heads/mybase &&
+	test "$(git config branch.myr1.rebase)" = true
+'
+
+test_expect_success 'autosetuprebase always on a tracked local branch' '
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	git config branch.autosetuprebase always &&
+	(git show-ref -q refs/remotes/local/o || git-fetch local) &&
+	git branch mybase2 &&
+	git branch --track myr2 mybase &&
+	test "$(git config branch.myr2.remote)" = . &&
+	test "$(git config branch.myr2.merge)" = refs/heads/mybase &&
+	test "$(git config branch.myr2.rebase)" = true
+'
+
+test_expect_success 'autosetuprebase remote on a tracked local branch' '
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	git config branch.autosetuprebase remote &&
+	(git show-ref -q refs/remotes/local/o || git-fetch local) &&
+	git branch mybase3 &&
+	git branch --track myr3 mybase2 &&
+	test "$(git config branch.myr3.remote)" = . &&
+	test "$(git config branch.myr3.merge)" = refs/heads/mybase2 &&
+	! test "$(git config branch.myr3.rebase)" = true
+'
+
+test_expect_success 'autosetuprebase never on a tracked local branch' '
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	git config branch.autosetuprebase never &&
+	(git show-ref -q refs/remotes/local/o || git-fetch local) &&
+	git branch mybase4 &&
+	git branch --track myr4 mybase2 &&
+	test "$(git config branch.myr4.remote)" = . &&
+	test "$(git config branch.myr4.merge)" = refs/heads/mybase2 &&
+	! test "$(git config branch.myr4.rebase)" = true
+'
+
+test_expect_success 'autosetuprebase local on a tracked remote branch' '
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	git config branch.autosetuprebase local &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --track myr5 local/master &&
+	test "$(git config branch.myr5.remote)" = local &&
+	test "$(git config branch.myr5.merge)" = refs/heads/master &&
+	! test "$(git config branch.myr5.rebase)" = true
+'
+
+test_expect_success 'autosetuprebase never on a tracked remote branch' '
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	git config branch.autosetuprebase never &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --track myr6 local/master &&
+	test "$(git config branch.myr6.remote)" = local &&
+	test "$(git config branch.myr6.merge)" = refs/heads/master &&
+	! test "$(git config branch.myr6.rebase)" = true
+'
+
+test_expect_success 'autosetuprebase remote on a tracked remote branch' '
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	git config branch.autosetuprebase remote &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --track myr7 local/master &&
+	test "$(git config branch.myr7.remote)" = local &&
+	test "$(git config branch.myr7.merge)" = refs/heads/master &&
+	test "$(git config branch.myr7.rebase)" = true
+'
+
+test_expect_success 'autosetuprebase always on a tracked remote branch' '
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	git config branch.autosetuprebase remote &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --track myr8 local/master &&
+	test "$(git config branch.myr8.remote)" = local &&
+	test "$(git config branch.myr8.merge)" = refs/heads/master &&
+	test "$(git config branch.myr8.rebase)" = true
+'
+
+test_expect_success 'autosetuprebase unconfigured on a tracked remote branch' '
+	git config --unset branch.autosetuprebase &&
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --track myr9 local/master &&
+	test "$(git config branch.myr9.remote)" = local &&
+	test "$(git config branch.myr9.merge)" = refs/heads/master &&
+	test "z$(git config branch.myr9.rebase)" = z
+'
+
+test_expect_success 'autosetuprebase unconfigured on a tracked local branch' '
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	(git show-ref -q refs/remotes/local/o || git-fetch local) &&
+	git branch mybase10 &&
+	git branch --track myr10 mybase2 &&
+	test "$(git config branch.myr10.remote)" = . &&
+	test "$(git config branch.myr10.merge)" = refs/heads/mybase2 &&
+	test "z$(git config branch.myr10.rebase)" = z
+'
+
+test_expect_success 'autosetuprebase unconfigured on untracked local branch' '
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --no-track myr11 mybase2 &&
+	test "z$(git config branch.myr11.remote)" = z &&
+	test "z$(git config branch.myr11.merge)" = z &&
+	test "z$(git config branch.myr11.rebase)" = z
+'
+
+test_expect_success 'autosetuprebase unconfigured on untracked remote branch' '
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --no-track myr12 local/master &&
+	test "z$(git config branch.myr12.remote)" = z &&
+	test "z$(git config branch.myr12.merge)" = z &&
+	test "z$(git config branch.myr12.rebase)" = z
+'
+
+test_expect_success 'autosetuprebase never on an untracked local branch' '
+	git config branch.autosetuprebase never &&
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --no-track myr13 mybase2 &&
+	test "z$(git config branch.myr13.remote)" = z &&
+	test "z$(git config branch.myr13.merge)" = z &&
+	test "z$(git config branch.myr13.rebase)" = z
+'
+
+test_expect_success 'autosetuprebase local on an untracked local branch' '
+	git config branch.autosetuprebase local &&
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --no-track myr14 mybase2 &&
+	test "z$(git config branch.myr14.remote)" = z &&
+	test "z$(git config branch.myr14.merge)" = z &&
+	test "z$(git config branch.myr14.rebase)" = z
+'
+
+test_expect_success 'autosetuprebase remote on an untracked local branch' '
+	git config branch.autosetuprebase remote &&
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --no-track myr15 mybase2 &&
+	test "z$(git config branch.myr15.remote)" = z &&
+	test "z$(git config branch.myr15.merge)" = z &&
+	test "z$(git config branch.myr15.rebase)" = z
+'
+
+test_expect_success 'autosetuprebase always on an untracked local branch' '
+	git config branch.autosetuprebase always &&
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --no-track myr16 mybase2 &&
+	test "z$(git config branch.myr16.remote)" = z &&
+	test "z$(git config branch.myr16.merge)" = z &&
+	test "z$(git config branch.myr16.rebase)" = z
+'
+
+test_expect_success 'autosetuprebase never on an untracked remote branch' '
+	git config branch.autosetuprebase never &&
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --no-track myr17 local/master &&
+	test "z$(git config branch.myr17.remote)" = z &&
+	test "z$(git config branch.myr17.merge)" = z &&
+	test "z$(git config branch.myr17.rebase)" = z
+'
+
+test_expect_success 'autosetuprebase local on an untracked remote branch' '
+	git config branch.autosetuprebase local &&
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --no-track myr18 local/master &&
+	test "z$(git config branch.myr18.remote)" = z &&
+	test "z$(git config branch.myr18.merge)" = z &&
+	test "z$(git config branch.myr18.rebase)" = z
+'
+
+test_expect_success 'autosetuprebase remote on an untracked remote branch' '
+	git config branch.autosetuprebase remote &&
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --no-track myr19 local/master &&
+	test "z$(git config branch.myr19.remote)" = z &&
+	test "z$(git config branch.myr19.merge)" = z &&
+	test "z$(git config branch.myr19.rebase)" = z
+'
+
+test_expect_success 'autosetuprebase always on an untracked remote branch' '
+	git config branch.autosetuprebase always &&
+	git config remote.local.url . &&
+	git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
+	(git show-ref -q refs/remotes/local/master || git-fetch local) &&
+	git branch --no-track myr20 local/master &&
+	test "z$(git config branch.myr20.remote)" = z &&
+	test "z$(git config branch.myr20.merge)" = z &&
+	test "z$(git config branch.myr20.rebase)" = z
+'
+
+test_expect_success 'detect misconfigured autosetuprebase (bad value)' '
+	git config branch.autosetuprebase garbage &&
+	test_must_fail git branch
+'
+
+test_expect_success 'detect misconfigured autosetuprebase (no value)' '
+	git config --unset branch.autosetuprebase &&
+	echo "[branch] autosetuprebase" >> .git/config &&
+	test_must_fail git branch &&
+	git config --unset branch.autosetuprebase
+'
+
 test_done

-- 
1.5.5.1.179.g0325e.dirty

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help