From: Lars Hjemli <hjemli@gmail.com>
Date: Sat, 25 Nov 2006 08:52:45 GMT
Subject: Re: [RFC] Teach git-branch howto rename a branch
Message-ID: <8c5c35580611250052k19bf54aelcc3833506c2a09f9@mail.gmail.com>
In-Reply-To: <7v1wns6q41.fsf@assigned-by-dhcp.cox.net>

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

--