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

Lars Hjemli [off-list ref] writes:

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.

Thanks.

"--rename newname oldname" feels funny, as already mentioned a
few times on the list.  rename(2) is "rename(old, new)" and
mv(1) is "mv old new".

This seems to do the right thing for both refs and reflogs, but 'make test' 
probably should be expanded with some evil test-cases to confirm my manual
testing.

Certainly.  Let's keep discipline to have tests and docs for new
features.
 
+static void rename_branch(const char *oldname, const char *newname, int force)
+{
+	char oldref[PATH_MAX], newref[PATH_MAX];
+	unsigned char sha1[20];
+
+	snprintf(oldref, sizeof oldref, "refs/heads/%s", oldname);
+	if (check_ref_format(oldref))
+		die("Invalid branch name: %s", oldref);

Although "sizeof type" is valid C, we tend to prefer
"sizeof(type)".  If you use snprintf(), it would make sense to
check its return value.

+	if (resolve_ref(newref, sha1, 1, NULL) && !force)
+		die("A branch named '%s' already exists.\n", newname);

No trailing '\n' is necessary for die().

+	if (!rename_ref(oldref, newref) && !strcmp(oldname, head))
+		create_symref("HEAD", newref);
+}

Can create_symref() fail?

+int rename_ref(const char *oldref, const char *newref)
+{
+	unsigned char sha1[20], orig_sha1[20];
+	int flag = 0, logmoved = 0;
+	struct ref_lock *lock;
+	char msg[PATH_MAX*2 + 100];
+	struct stat stat;
+	int log = !lstat(git_path("logs/%s", oldref), &stat);

This is not wrong per-se, but it made me stop and wonder if we
want to error out when we find out "logs/oldref" is a symlink; I
do not think we care about it that much, but in that case we may
want to say stat() here instead...  Just a minor detail.

+	lock = lock_ref_sha1_basic("tmp-renamed-ref", NULL, NULL);
+	if (!lock)
+		return error("unable to lock tmp-renamed-ref");
+	lock->force_write = 1;
+	if (write_ref_sha1(lock, orig_sha1, msg))
+		return error("unable to save current sha1 in tmp-renamed-ref");
+	if (log && rename(git_path("logs/%s", oldref), git_path("tmp-renamed-log")))
+		return error("unable to move logfile logs/%s to tmp-renamed-log: %s",
+			oldref, strerror(errno));

I am confused with this code.  tmp-renamed-ref is not even a
ref, you lock $GIT_DIR/tmp-renamed-ref and call write-ref_sha1()
with an uninitialized msg[] buffer to write into a logfile. What
is the name of that logfile?  $GIT_DIR/log/tmp-renamed-ref???