[PATCH 0/2] Make it easy to use branch --track on existing branch

[PATCH 0/2] Make it easy to use branch --track on existing branch

[Matthieu Moy]

I'm starting a new thread to avoid hiding the message in another one,
but this is a followup to a message in the "git push --track" thread:

http://article.gmane.org/gmane.comp.version-control.git/137066

I wrote:
> Junio C Hamano <gitster <at> pobox.com> writes:
> 
> > The small nit is that "branch -f --track me origin/me" will happily
> > overwrite "me", even when your "me" is not up to date with "origin/me",
> > losing commits.
> 
> And another issue is:
> 
> $ git branch -f --track my-branch origin/my-branch
> fatal: Cannot force update the current branch.
> $ git branch --track my-branch origin/my-branch
> fatal: A branch named 'my-branch' already exists.
> 
> Actually, I just can't find a natural set of commands doing:
> 
> 1. create a branch (git checkout -b)
> 2. work on it
> 3. send it upstream (git push)
> 4. set the upstream as tracking (???)
> 
> with the current version of Git. I just do 4. with $EDITOR
> .git/config ...

The first patch makes it possible to use branch --track on an existing
branch (checked-out or not, regardless of -f), and the second warns on
a newly introduced irrelevant case.

This should be a nice complement to "push --set-upstream". I think
"push --set-upstream" is the most natural in 99% of cases, but using
"git branch" should work too.

Matthieu Moy (2):
  branch: allow creating a branch with same name and same starting
    point.
  branch: warn and refuse to set a branch as a tracking branch of
    itself.

 branch.c                 |   59 ++++++++++++++++++++++++++++++++-------------
 t/t6040-tracking-info.sh |    8 ++++++
 2 files changed, 50 insertions(+), 17 deletions(-)

[PATCH 1/2] branch: allow creating a branch with same name and same starting point.

[Matthieu Moy]

Previously, "git branch --track newname old" was rejected if newname
existed, even if it already had the same value. This patch allows it,
even without --force. This has two advantages:

* Not requiring --force for a safe operation, hence allowing the user to
  benefit from the other safety checks.

* Allow changing the configuration of the checked-out branch.

As a result, "git branch --track" becomes a conveinient and safe way to
set up upstream information for an existing branch.

The error message "A branch named '%s' already exists" is no longer
precise enough to refuse to act, since part of the reason of the failure
is the old branch value, so we include this value in the message.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 branch.c                 |   45 ++++++++++++++++++++++++++++++---------------
 t/t6040-tracking-info.sh |    8 ++++++++
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/branch.c b/branch.c
index 05ef3f5..d187891 100644
--- a/branch.c
+++ b/branch.c
@@ -128,27 +128,39 @@ void create_branch(const char *head,
 		   const char *name, const char *start_name,
 		   int force, int reflog, enum branch_track track)
 {
-	struct ref_lock *lock;
+	struct ref_lock *lock = NULL;
 	struct commit *commit;
-	unsigned char sha1[20];
+	unsigned char old_sha1[20], sha1[20];
 	char *real_ref, msg[PATH_MAX + 20];
 	struct strbuf ref = STRBUF_INIT;
 	int forcing = 0;
+	int dont_change_ref = 0;
 
 	if (strbuf_check_branch_ref(&ref, name))
 		die("'%s' is not a valid branch name.", name);
 
-	if (resolve_ref(ref.buf, sha1, 1, NULL)) {
-		if (!force)
-			die("A branch named '%s' already exists.", name);
-		else if (!is_bare_repository() && !strcmp(head, name))
-			die("Cannot force update the current branch.");
+	if (get_sha1(start_name, sha1))
+		die("Not a valid object name: '%s'.", start_name);
+
+	if (resolve_ref(ref.buf, old_sha1, 1, NULL)) {
+		if (hashcmp(old_sha1, sha1)) {
+			if (!force)
+				die("A branch named '%s' already exists pointing to %.*s.",
+				    name, 8, sha1_to_hex(old_sha1));
+			else if (!is_bare_repository() && !strcmp(head, name))
+				die("Cannot force update the current branch.");
+		} else {
+			/*
+			 * Re-creating a branch with the same value.
+			 * Safe, and usefull to set up the upstream
+			 * branch without touching the ref.
+			 */
+			dont_change_ref = 1;
+		}
 		forcing = 1;
 	}
 
 	real_ref = NULL;
-	if (get_sha1(start_name, sha1))
-		die("Not a valid object name: '%s'.", start_name);
 
 	switch (dwim_ref(start_name, strlen(start_name), sha1, &real_ref)) {
 	case 0:
@@ -170,9 +182,11 @@ void create_branch(const char *head,
 		die("Not a valid branch point: '%s'.", start_name);
 	hashcpy(sha1, commit->object.sha1);
 
-	lock = lock_any_ref_for_update(ref.buf, NULL, 0);
-	if (!lock)
-		die_errno("Failed to lock ref for update");
+	if (!dont_change_ref) {
+		lock = lock_any_ref_for_update(ref.buf, NULL, 0);
+		if (!lock)
+			die_errno("Failed to lock ref for update");
+	}
 
 	if (reflog)
 		log_all_ref_updates = 1;
@@ -180,15 +194,16 @@ void create_branch(const char *head,
 	if (forcing)
 		snprintf(msg, sizeof msg, "branch: Reset from %s",
 			 start_name);
-	else
+	else if (!dont_change_ref)
 		snprintf(msg, sizeof msg, "branch: Created from %s",
 			 start_name);
 
 	if (real_ref && track)
 		setup_tracking(name, real_ref, track);
 
-	if (write_ref_sha1(lock, sha1, msg) < 0)
-		die_errno("Failed to write ref");
+	if (!dont_change_ref)
+		if (write_ref_sha1(lock, sha1, msg) < 0)
+			die_errno("Failed to write ref");
 
 	strbuf_release(&ref);
 	free(real_ref);
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 664b0f8..3c200b6 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -89,4 +89,12 @@ test_expect_success 'status when tracking annotated tags' '
 	grep "set up to track" actual &&
 	git checkout heavytrack
 '
+
+test_expect_success 'setup tracking with branch --track on existing branch' '
+	git branch from-master master &&
+	test_must_fail git config branch.from-master.merge > actual &&
+	git branch from-master master --track &&
+	git config branch.from-master.merge > actual &&
+	grep -q "^refs/heads/master$" actual
+'
 test_done
-- 
1.6.6.198.gbaea2

[PATCH 2/2] branch: warn and refuse to set a branch as a tracking branch of itself.

[Matthieu Moy]

Previous patch allows commands like "git branch foo foo --track", which
doesn't make much sense. Warn the user and don't change the configuration
in this case. Don't die to let the caller finish its job in such case.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 branch.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/branch.c b/branch.c
index d187891..d769c8a 100644
--- a/branch.c
+++ b/branch.c
@@ -49,9 +49,19 @@ static int should_setup_rebase(const char *origin)
 
 void install_branch_config(int flag, const char *local, const char *origin, const char *remote)
 {
+	const char *shortname = remote + 11;
+	int remote_is_branch = !prefixcmp(remote, "refs/heads/");
 	struct strbuf key = STRBUF_INIT;
 	int rebasing = should_setup_rebase(origin);
 
+	if (remote_is_branch
+	    && !strcmp(local, shortname)
+	    && !origin) {
+		warning("Not setting branch %s as its own upstream.",
+			local);
+		return;
+	}
+
 	strbuf_addf(&key, "branch.%s.remote", local);
 	git_config_set(key.buf, origin ? origin : ".");
 
@@ -71,8 +81,8 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 		strbuf_addstr(&key, origin ? "remote" : "local");
 
 		/* Are we tracking a proper "branch"? */
-		if (!prefixcmp(remote, "refs/heads/")) {
-			strbuf_addf(&key, " branch %s", remote + 11);
+		if (remote_is_branch) {
+			strbuf_addf(&key, " branch %s", shortname);
 			if (origin)
 				strbuf_addf(&key, " from %s", origin);
 		}
-- 
1.6.6.198.gbaea2

Re: [PATCH 0/2] Make it easy to use branch --track on existing branch

[Ilari Liusvaara]

On Sun, Jan 17, 2010 at 03:06:50PM +0100, Matthieu Moy wrote:
> 
> The first patch makes it possible to use branch --track on an existing
> branch (checked-out or not, regardless of -f), and the second warns on
> a newly introduced irrelevant case.

Yay. This is one of smaller entries from my todo list (the remaining
entry that's small enough to still make it into 1.7 is command to
set URL remote points to[*]).

But If I read the patch correctly, you can't just arbitrarily set the
tracking branch since the IDs must match. So if somebody screws the
ref creation so that it lacks upstream data, then both local and upstream
change. Push will fail. Pull will fail. And now neither checkout --track
(due to same-hash check) nor push --set-upstream (due to pushed or up to
date) help.

[*] Yes, I know you can edit .git/config, but I would want "official sounding"
(read: git remote subcommand) command to edit it (and no, delete & recreate
doesn't do the right thing).

-Ilari

Re: [PATCH 0/2] Make it easy to use branch --track on existing branch

[Matthieu Moy]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:

> But If I read the patch correctly, you can't just arbitrarily set the
> tracking branch since the IDs must match.

Yes. The patch really helps when you already have synchronized, or
when you haven't already desynchronized your local branch and its
upstream.

We could go one step further, and allow the new branch point to be
different as long as it has the old point as an ancestor, but that
would still be a ref update.

> [*] Yes, I know you can edit .git/config, but I would want "official sounding"
> (read: git remote subcommand) command to edit it (and no, delete & recreate
> doesn't do the right thing).

Having a "git remote subcommand" to do the same thing could help too,
but it could just come in addition to my patch.

My patch doesn't introduce new complexity: the command is already
there, and the use-case I'm allowing are legitimate. So, it can help
to let users run in, and it doesn't harm.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 0/2] Make it easy to use branch --track on existing branch

[Ilari Liusvaara]

On Sun, Jan 17, 2010 at 03:53:04PM +0100, Matthieu Moy wrote:
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
> 
> > [*] Yes, I know you can edit .git/config, but I would want "official sounding"
> > (read: git remote subcommand) command to edit it (and no, delete & recreate
> > doesn't do the right thing).
> 
> Having a "git remote subcommand" to do the same thing could help too,
> but it could just come in addition to my patch.

Err... That was about changing URL, not about changing tracking branch.

-Ilari

Re: [PATCH 1/2] branch: allow creating a branch with same name and same starting point.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> Previously, "git branch --track newname old" was rejected if newname
> existed, even if it already had the same value. This patch allows it,
> even without --force. This has two advantages:
>
> * Not requiring --force for a safe operation, hence allowing the user to
>   benefit from the other safety checks.
>
> * Allow changing the configuration of the checked-out branch.

Two issues I have to ask because you didn't cover these cases in your
tests [*1*]:

 - What should "git branch new old" do when (0) local branch new already
   exists and points at old, (1) branch.new.{remote,rebase,merge} do not
   currently exist, and (2) branch.autosetup{merge,rebase} is set?

 - What should "git branch --no-track new old" do when (0) local branch
   new already exists and points at old, (1)
   branch.new.{remote,rebase,merge} do currently exist?

No matter what they do, should there be some extra message when the new
code does something that the old code didn't do?

> +test_expect_success 'setup tracking with branch --track on existing branch' '
> ...
> +	git branch from-master master --track &&

The parser may be too loosely implemented and might allow this today, but
please stick to "command names then dashed options and then arguments".

We have to fix the parser someday to be more strict and broken order in
tests like this will get in the way.

> +	git config branch.from-master.merge > actual &&
> +	grep -q "^refs/heads/master$" actual
> +'
>  test_done

[Footnote]

*1* it is an easy trap to fall into to test only what it newly does while
showing your shiny new toy and forget to test conditions that the shiny
new toy should not kick in.