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

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(-)

Lars Hjemli <hjemli@gmail.com> writes:

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.

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.

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.

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.

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.

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

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!

Shawn Pearce wrote:

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

I have thought that command to rename branch was created to deal with simultaneous renaming of reflog + marking rename in reflog.

Jakub Narebski <jnareb@gmail.com> wrote:

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.
 

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

On 11/25/06, Shawn Pearce <spearce@spearce.org> wrote:

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...

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

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.

On 11/25/06, Junio C Hamano <junkio@cox.net> wrote:

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

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(-)

On 11/25/06, Junio C Hamano <junkio@cox.net> wrote:

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....

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.

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

On 11/25/06, Lars Hjemli <hjemli@gmail.com> wrote:

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.

On 11/25/06, Fredrik Kuivinen <frekui@gmail.com> wrote:

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

On Saturday 25 November 2006 00:03, Lars Hjemli wrote:

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