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

From: Junio C Hamano <junkio@cox.net>
Date: Nov 25, 2006, 05:40 UTC
Message-ID: <7v1wns6q41.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <1164409429445-git-send-email-hjemli@gmail.com>

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.

Show 11 quoted lines

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

Show 9 quoted lines

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

Show 6 quoted lines

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