[RFC] Teach git-branch howto rename a branch

[RFC] Teach git-branch howto rename a branch

[Lars Hjemli]

This adds a '--rename' option to git branch. If specified, branch
creation becomes branch renaming.

With a single branchname, the current branch is renamed and .git/HEAD is
updated.

With two branchnames, the second name is renamed to the first.

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 builtin-branch.c |  117 +++++++++++++++++++++++++++++++++++------------------
 1 files changed, 77 insertions(+), 40 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index f14d814..e714569 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -11,7 +11,7 @@
 #include "builtin.h"
 
 static const char builtin_branch_usage[] =
-"git-branch (-d | -D) <branchname> | [-l] [-f] <branchname> [<start-point>] | [-r | -a] [-v] [--abbrev=<length>] ";
+"git-branch (-d | -D) <branchname> | [-l] [-f] [--rename] <branchname> [<start-point>] | [-r | -a] [-v] [--abbrev=<length>] ";
 
 
 static const char *head;
@@ -36,47 +36,55 @@ static int in_merge_bases(const unsigned
 	return ret;
 }
 
-static void delete_branches(int argc, const char **argv, int force)
+static void delete_branch(const char *branch, int force, struct commit *head_rev)
 {
-	struct commit *rev, *head_rev;
-	unsigned char sha1[20];
+	struct commit *rev;
 	char *name;
-	int i;
+	unsigned char sha1[20];
 
-	head_rev = lookup_commit_reference(head_sha1);
-	for (i = 0; i < argc; i++) {
-		if (!strcmp(head, argv[i]))
-			die("Cannot delete the branch you are currently on.");
-
-		name = xstrdup(mkpath("refs/heads/%s", argv[i]));
-		if (!resolve_ref(name, sha1, 1, NULL))
-			die("Branch '%s' not found.", argv[i]);
-
-		rev = lookup_commit_reference(sha1);
-		if (!rev || !head_rev)
-			die("Couldn't look up commit objects.");
-
-		/* This checks whether the merge bases of branch and
-		 * HEAD contains branch -- which means that the HEAD
-		 * contains everything in both.
-		 */
-
-		if (!force &&
-		    !in_merge_bases(sha1, rev, head_rev)) {
-			fprintf(stderr,
-				"The branch '%s' is not a strict subset of your current HEAD.\n"
-				"If you are sure you want to delete it, run 'git branch -D %s'.\n",
-				argv[i], argv[i]);
-			exit(1);
-		}
+	if (!strcmp(head, branch))
+		die("Cannot delete the branch you are currently on.");
+
+	name = xstrdup(mkpath("refs/heads/%s", branch));
+	if (!resolve_ref(name, sha1, 1, NULL))
+		die("Branch '%s' not found.", branch);
+
+	rev = lookup_commit_reference(sha1);
+	if (!rev)
+		die("Couldn't look up commit objects for %s", branch);
+
+	/* This checks whether the merge bases of branch and
+	 * HEAD contains branch -- which means that the HEAD
+	 * contains everything in both.
+	 */
+
+	if (head_rev && !force &&
+	    !in_merge_bases(sha1, rev, head_rev)) {
+		fprintf(stderr,
+			"The branch '%s' is not a strict subset of your current HEAD.\n"
+			"If you are sure you want to delete it, run 'git branch -D %s'.\n",
+			branch, branch);
+		exit(1);
+	}
 
-		if (delete_ref(name, sha1))
-			printf("Error deleting branch '%s'\n", argv[i]);
-		else
-			printf("Deleted branch %s.\n", argv[i]);
+	if (delete_ref(name, sha1))
+		printf("Error deleting branch '%s'\n", branch);
+	else
+		printf("Deleted branch %s.\n", branch);
 
-		free(name);
-	}
+	free(name);
+}
+
+static void delete_branches(int argc, const char **argv, int force)
+{
+	struct commit *head_rev;
+	int i;
+	
+	head_rev = lookup_commit_reference(head_sha1);
+	if (!head_rev)
+		die("Couldn't look up commit object for current HEAD.");
+	for (i = 0; i < argc; i++) 
+		delete_branch(argv[i], force, head_rev);
 }
 
 #define REF_UNKNOWN_TYPE    0x00
@@ -200,15 +208,18 @@ static void print_ref_list(int kinds, in
 	free_ref_list(&ref_list);
 }
 
-static void create_branch(const char *name, const char *start,
+static char *create_branch(const char *name, const char *start,
 			  int force, int reflog)
 {
 	struct ref_lock *lock;
 	struct commit *commit;
 	unsigned char sha1[20];
-	char ref[PATH_MAX], msg[PATH_MAX + 20];
+	char *ref, msg[PATH_MAX + 20];
+	int reflen;
 
-	snprintf(ref, sizeof ref, "refs/heads/%s", name);
+	reflen = strlen(name) + 12;
+	ref = xmalloc(reflen+1);
+	snprintf(ref, reflen, "refs/heads/%s", name);
 	if (check_ref_format(ref))
 		die("'%s' is not a valid branch name.", name);
 
@@ -235,12 +246,30 @@ static void create_branch(const char *na
 
 	if (write_ref_sha1(lock, sha1, msg) < 0)
 		die("Failed to write ref: %s.", strerror(errno));
+	return ref;
+}
+
+static void rename_branch(const char *newname, const char *oldname, int force, int reflog)
+{
+	char ref[PATH_MAX];
+
+	snprintf(ref, sizeof ref, "refs/heads/%s", oldname);
+	if (check_ref_format(ref))
+		die("'%s' is not a valid branch name.", oldname);
+
+	newname = create_branch(newname, oldname, force, reflog);
+	if (!strcmp(oldname, head)) {
+		create_symref("HEAD", newname);
+		head = newname + 11;
+	}
+	delete_branch(oldname, force, NULL);
 }
 
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
 	int delete = 0, force_delete = 0, force_create = 0;
 	int verbose = 0, abbrev = DEFAULT_ABBREV;
+	int rename = 0;
 	int reflog = 0;
 	int kinds = REF_LOCAL_BRANCH;
 	int i;
@@ -269,6 +298,10 @@ int cmd_branch(int argc, const char **ar
 			force_create = 1;
 			continue;
 		}
+		if (!strcmp(arg, "--rename")) {
+			rename = 1;
+			continue;
+		}
 		if (!strcmp(arg, "-r")) {
 			kinds = REF_REMOTE_BRANCH;
 			continue;
@@ -303,6 +336,10 @@ int cmd_branch(int argc, const char **ar
 		delete_branches(argc - i, argv + i, force_delete);
 	else if (i == argc)
 		print_ref_list(kinds, verbose, abbrev);
+	else if (rename && (i == argc - 1))
+		rename_branch(argv[i], head, force_create, reflog);
+	else if (rename && (i == argc - 2))
+		rename_branch(argv[i], argv[i + 1], force_create, reflog);
 	else if (i == argc - 1)
 		create_branch(argv[i], head, force_create, reflog);
 	else if (i == argc - 2)
-- 
1.4.4

Re: [RFC] Teach git-branch howto rename a branch

[Junio C Hamano]

Lars Hjemli <hjemli@gmail.com> writes:

> +static void delete_branch(const char *branch, int force, struct commit *head_rev)
>  {
>...
> +}

Refactoring the single ref deletion into this function feels
sane.  I think you do not need a separate force parameter to
this function anymore; if the caller wants to force the deletion
it can send in a NULL for head_rev to signal that there is no
need for the "subset" check.

> +static void delete_branches(int argc, const char **argv, int force)
> +{
> +	struct commit *head_rev;
> +	int i;
> +	
> +	head_rev = lookup_commit_reference(head_sha1);
> +	if (!head_rev)
> +		die("Couldn't look up commit object for current HEAD.");
> +	for (i = 0; i < argc; i++) 
> +		delete_branch(argv[i], force, head_rev);
>  }

I do not think this die() is a good idea.  I think it is
reasonable to allow the following sequence:

	$ mkdir newdir && cd newdir
	$ git init-db
        $ git fetch $other_repo refs/heads/master:refs/heads/othre
        $ git branch -D othre ;# oops, typo
        $ git fetch $other_repo refs/heads/master:refs/heads/other

When forcing a deletion, we do not care about ancestry relation
between the HEAD and the branch being deleted, so we should not
even bother checking if HEAD is already valid.  The original
code before your patch shares the same problem.

> -static void create_branch(const char *name, const char *start,
> +static char *create_branch(const char *name, const char *start,
>  			  int force, int reflog)

This makes the returned names leak but probably we do not care
about it too much.  However, the only caller that cares about
the new refname is rename_branch, and we are talking only about
branches, so I think handcrafting the refname just like you
handcraft the refname for oldname in rename_branch() would be
a better solution without introducing new leaks.

> +static void rename_branch(const char *newname, const char *oldname, int force, int reflog)
> +{
> +	char ref[PATH_MAX];
> +
> +	snprintf(ref, sizeof ref, "refs/heads/%s", oldname);
> +	if (check_ref_format(ref))
> +		die("'%s' is not a valid branch name.", oldname);
> +
> +	newname = create_branch(newname, oldname, force, reflog);

This does not feel right.  The 'start' parameter to
create_branch is arbitrary SHA-1 expression so it can take
'master', 'heads/master' and 'refs/heads/master' to mean the
same thing, as long as they are unambiguous, but here you would
want to accept only 'master' because the paramter is supposed to
be the name of the branch you are renaming.  create_branch()
does not want to do that check for its start parameter, so you
should do the checking yourself here, and check_ref_format() is
not good enough for that.  Probably calling resolve_ref() on ref
(= "refs/heads/oldname") for reading (because you also want to
make sure oldname talks about an existing branch) is needed.

> +	if (!strcmp(oldname, head)) {
> +		create_symref("HEAD", newname);
> +		head = newname + 11;
> +	}
> +	delete_branch(oldname, force, NULL);
>  }

What is the right thing that should happen when newname talks
about an existing branch (I am not asking "what does your code
do?")?

Without -f, it should barf.  With -f, we would want the rename
to happen.  In the latter case, I think it should work the same
way as deleting it and creating it anew, and that would make
sure that reflog for the old one will be lost and a new log is
started afresh; otherwise, the log would say old history for
that branch and it won't be a "rename" anymore.

Also what happens when oldname is "frotz" and newname is
"frotz/nitfol"?  You would need to read the value of "frotz",
make sure you can delete it (perhaps the usual fast-forward
check as needed), and delete it to make room and then create
"frotz/nitfol".  I suspect your patch does not handle that
case.

Re: [RFC] Teach git-branch howto rename a branch

[Shawn Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> Without -f, it should barf.  With -f, we would want the rename
> to happen.  In the latter case, I think it should work the same
> way as deleting it and creating it anew, and that would make
> sure that reflog for the old one will be lost and a new log is
> started afresh; otherwise, the log would say old history for
> that branch and it won't be a "rename" anymore.

This patch doesn't rename the reflog when the branch renames.
Myself and a few other users I support want the reflog preserved
when a branch renames, we all see the reflog as part of the history
of that branch and a rename is the same branch but stored under a
different name...

I had planned to do a rename branch command myself, but its been
lower priority than everything else, so I have just never gotten
around to it.  I'm glad to see someone is attempting it!

-- 

[PATCH] git-branch -D: make it work even when on a yet-to-be-born branch

[Junio C Hamano]

This makes "git branch -D other_branch" work even when HEAD
points at a yet-to-be-born branch.

Earlier, we checked the HEAD ref for the purpose of "subset"
check even when the deletion was forced (i.e. not -d but -D).
Because of this, you cannot delete a branch even with -D while
on a yet-to-be-born branch.

With this change, the following sequence that now works:

	mkdir newdir && cd newdir
	git init-db
	git fetch -k $other_repo refs/heads/master:refs/heads/othre
	# oops, typo
	git branch other othre
	git branch -D othre

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

  Junio C Hamano <junkio@cox.net> writes:

  > When forcing a deletion, we do not care about ancestry relation
  > between the HEAD and the branch being deleted, so we should not
  > even bother checking if HEAD is already valid.  The original
  > code before your patch shares the same problem.

  And here is a fix for that.  This is on top of your -v change
  which I've applied.

 builtin-branch.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 69b7b55..3d5cb0e 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -38,12 +38,16 @@ static int in_merge_bases(const unsigned char *sha1,
 
 static void delete_branches(int argc, const char **argv, int force)
 {
-	struct commit *rev, *head_rev;
+	struct commit *rev, *head_rev = head_rev;
 	unsigned char sha1[20];
 	char *name;
 	int i;
 
-	head_rev = lookup_commit_reference(head_sha1);
+	if (!force) {
+		head_rev = lookup_commit_reference(head_sha1);
+		if (!head_rev)
+			die("Couldn't look up commit object for HEAD");
+	}
 	for (i = 0; i < argc; i++) {
 		if (!strcmp(head, argv[i]))
 			die("Cannot delete the branch you are currently on.");
@@ -53,8 +57,8 @@ static void delete_branches(int argc, const char **argv, int force)
 			die("Branch '%s' not found.", argv[i]);
 
 		rev = lookup_commit_reference(sha1);
-		if (!rev || !head_rev)
-			die("Couldn't look up commit objects.");
+		if (!rev)
+			die("Couldn't look up commit object for '%s'", name);
 
 		/* This checks whether the merge bases of branch and
 		 * HEAD contains branch -- which means that the HEAD
-- 
1.4.4.1.g61fba

Re: [RFC] Teach git-branch howto rename a branch

[Lars Hjemli]

On 11/25/06, Junio C Hamano <junkio@cox.net> wrote:
> Lars Hjemli <hjemli@gmail.com> writes:
>
> > +static void rename_branch(const char *newname, const char *oldname, int force, int reflog)
> > +{
> > +     char ref[PATH_MAX];
> > +
> > +     snprintf(ref, sizeof ref, "refs/heads/%s", oldname);
> > +     if (check_ref_format(ref))
> > +             die("'%s' is not a valid branch name.", oldname);
> > +
> > +     newname = create_branch(newname, oldname, force, reflog);
>
> This does not feel right.  The 'start' parameter to
> create_branch is arbitrary SHA-1 expression so it can take
> 'master', 'heads/master' and 'refs/heads/master' to mean the
> same thing, as long as they are unambiguous, but here you would
> want to accept only 'master' because the paramter is supposed to
> be the name of the branch you are renaming.  create_branch()
> does not want to do that check for its start parameter, so you
> should do the checking yourself here, and check_ref_format() is
> not good enough for that.  Probably calling resolve_ref() on ref
> (= "refs/heads/oldname") for reading (because you also want to
> make sure oldname talks about an existing branch) is needed.

I forgot to use the handcrafted ref when calling 'create_branch':

   newname = create_branch(newname, ref, force, reflog);

This would force the 'refs/heads' prefix, but let 'create_branch'
check if it's a valid commit reference. I _think_ this would be good
enough....


>
> > +     if (!strcmp(oldname, head)) {
> > +             create_symref("HEAD", newname);
> > +             head = newname + 11;
> > +     }
> > +     delete_branch(oldname, force, NULL);
> >  }
>
> What is the right thing that should happen when newname talks
> about an existing branch (I am not asking "what does your code
> do?")?
>
> Without -f, it should barf.  With -f, we would want the rename
> to happen.  In the latter case, I think it should work the same
> way as deleting it and creating it anew, and that would make
> sure that reflog for the old one will be lost and a new log is
> started afresh; otherwise, the log would say old history for
> that branch and it won't be a "rename" anymore.

Yes, the missing piece here is to copy the 'old' reflog to it's new
location after the call to create_branch. I belive create_branch
handles the -f cases.


> Also what happens when oldname is "frotz" and newname is
> "frotz/nitfol"?  You would need to read the value of "frotz",
> make sure you can delete it (perhaps the usual fast-forward
> check as needed), and delete it to make room and then create
> "frotz/nitfol".  I suspect your patch does not handle that
> case.

Hmm, you're right, I didn't think of such renaming. But I don't want
to delete the old ref before the new one is in place. How about
renaming the old one to a temporary name first?


I'l redo the patch on top of your 'git-branch -D' fix

-- 

Re: [RFC] Teach git-branch howto rename a branch

[Jakub Narebski]

Shawn Pearce wrote:

> Junio C Hamano <junkio@cox.net> wrote:
>> Without -f, it should barf.  With -f, we would want the rename
>> to happen.  In the latter case, I think it should work the same
>> way as deleting it and creating it anew, and that would make
>> sure that reflog for the old one will be lost and a new log is
>> started afresh; otherwise, the log would say old history for
>> that branch and it won't be a "rename" anymore.
> 
> This patch doesn't rename the reflog when the branch renames.
> Myself and a few other users I support want the reflog preserved
> when a branch renames, we all see the reflog as part of the history
> of that branch and a rename is the same branch but stored under a
> different name...

And of course reflog should store the fact of renaming branch.

> I had planned to do a rename branch command myself, but its been
> lower priority than everything else, so I have just never gotten
> around to it.  I'm glad to see someone is attempting it!

I have thought that command to rename branch was created to deal
with simultaneous renaming of reflog + marking rename in reflog.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [RFC] Teach git-branch howto rename a branch

[Shawn Pearce]

Jakub Narebski <jnareb@gmail.com> wrote:
> Shawn Pearce wrote:
> 
> > Junio C Hamano <junkio@cox.net> wrote:
> >> Without -f, it should barf.  With -f, we would want the rename
> >> to happen.  In the latter case, I think it should work the same
> >> way as deleting it and creating it anew, and that would make
> >> sure that reflog for the old one will be lost and a new log is
> >> started afresh; otherwise, the log would say old history for
> >> that branch and it won't be a "rename" anymore.
> > 
> > This patch doesn't rename the reflog when the branch renames.
> > Myself and a few other users I support want the reflog preserved
> > when a branch renames, we all see the reflog as part of the history
> > of that branch and a rename is the same branch but stored under a
> > different name...
> 
> And of course reflog should store the fact of renaming branch.

Yes, I think that's a worthwhile thing to log.  Problem is the
logging system tends to throw away pointless entries (sha1 ->
same sha1) so the rename log entry needs to be forced somehow...

Although without a UI to show the content of the reflog having the
rename entry in there isn't all that critical.
 
> > I had planned to do a rename branch command myself, but its been
> > lower priority than everything else, so I have just never gotten
> > around to it.  I'm glad to see someone is attempting it!
> 
> I have thought that command to rename branch was created to deal
> with simultaneous renaming of reflog + marking rename in reflog.

Yes, that's one of the complex parts of it. :-)

-- 

Re: [RFC] Teach git-branch howto rename a branch

[Lars Hjemli]

On 11/25/06, Shawn Pearce <spearce@spearce.org> wrote:
> Jakub Narebski <jnareb@gmail.com> wrote:
> >
> > And of course reflog should store the fact of renaming branch.
>
> Yes, I think that's a worthwhile thing to log.  Problem is the
> logging system tends to throw away pointless entries (sha1 ->
> same sha1) so the rename log entry needs to be forced somehow...

Is it ok to put

static int log_ref_write(struct ref_lock *lock,
	const unsigned char *sha1, const char *msg)

into refs.h?

Then it should be possibly to do something like this:

  lock = lock_ref_sha1(ref, sha1);
  lock->force_write = 1;
  log_ref_write(lock, sha1, "Renamed oldname to newname");
  unlock_ref(lock);

...after moving the reflog...

-- 

Re: [RFC] Teach git-branch howto rename a branch

[Junio C Hamano]

"Lars Hjemli" <lh@elementstorage.no> writes:

> Is it ok to put
>
> static int log_ref_write(struct ref_lock *lock,
> 	const unsigned char *sha1, const char *msg)
>
> into refs.h?

I think a cleaner implementation that does not have such a
layering violation would involve defining rename_refs()
interface in refs.c, next to the delete_ref() that exists there.

The division of labor would be for builtin-branch.c to make sure
both oldname and newname are branch names, and rename_refs() to
rename the reflog (if exists) and the ref at the same time.  To
deal with D/F conflicts sanely, I suspect it would involve a
call to rename(2) to move the reflog to a temporary location,
perhaps $GIT_DIR/.tmp-renamed-log, deletion of the old ref by
calling delete_ref(), and then another rename(2) to move that
temporary one to its final location, followed by the usual "ref
creation dance" of calling lock_any_ref_for_update() and
write_ref_sha1().

Side note:

If we do not mind losing the reflog of oldname, then "delete and
then create" would be sufficient, but that is not the case.

Unlike "git does not track individual files so we do not record
renames" mantra, branches, and more in general, refs, have
meaningful identity (at least locally), and unlike the file
contents, the "contents" of a ref does not migrate _partially_
from ref to ref.  It would make sense to keep track of renames
for them.

Re: [RFC] Teach git-branch howto rename a branch

[Fredrik Kuivinen]

On 11/25/06, Lars Hjemli <hjemli@gmail.com> wrote:
> This adds a '--rename' option to git branch. If specified, branch
> creation becomes branch renaming.
>
> With a single branchname, the current branch is renamed and .git/HEAD is
> updated.
>
> With two branchnames, the second name is renamed to the first.

Nice idea. But wouldn't it be more sensible to rename the first branch to the
second instead of the other way around? That is, the syntax would be

    git branch --rename FROM TO

which is more similar to how "mv" works.

Re: [RFC] Teach git-branch howto rename a branch

[Lars Hjemli]

On 11/25/06, Junio C Hamano <junkio@cox.net> wrote:
> "Lars Hjemli" <lh@elementstorage.no> writes:
>
> > Is it ok to put
> >
> > static int log_ref_write(struct ref_lock *lock,
> >       const unsigned char *sha1, const char *msg)
> >
> > into refs.h?
>
> I think a cleaner implementation that does not have such a
> layering violation would involve defining rename_refs()
> interface in refs.c, next to the delete_ref() that exists there.

Yes, that looks a lot nicer. I'll give it a try.

-- 

Re: [RFC] Teach git-branch howto rename a branch

[Lars Hjemli]

On 11/25/06, Fredrik Kuivinen <frekui@gmail.com> wrote:
> On 11/25/06, Lars Hjemli <hjemli@gmail.com> wrote:
> > This adds a '--rename' option to git branch. If specified, branch
> > creation becomes branch renaming.
> >
> > With a single branchname, the current branch is renamed and .git/HEAD is
> > updated.
> >
> > With two branchnames, the second name is renamed to the first.
>
> Nice idea. But wouldn't it be more sensible to rename the first branch to the
> second instead of the other way around? That is, the syntax would be
>
>     git branch --rename FROM TO
>
> which is more similar to how "mv" works.
>

Possibly, but then we would have

  git branch newbranch [oldbranch]

when creating a new branch, and

  git branch --rename [oldbranch] newbranch

for rename/move.

I'd prefer to be "internally consistent", but it does look and feel a
little strange...

Another option would be:

  git branch [--rename] [--from <branch>] newbranch

and deprecate the usage of two unnamed argumens for create/rename

-- 

Re: [RFC] Teach git-branch howto rename a branch

[Josef Weidendorfer]

On Saturday 25 November 2006 00:03, Lars Hjemli wrote:
> This adds a '--rename' option to git branch. If specified, branch
> creation becomes branch renaming.

This probably also should rename all config keys
 branch.<oldname>.*
into
 branch.<newname>.*