On 11/25/06, Junio C Hamano <junkio@xxxxxxx> wrote:
Lars Hjemli <hjemli@xxxxxxxxx> 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
--
larsh
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html