[PATCH] Teach git-branch howto rename a branch

Subsystems: the rest

STALE3660d

4 messages, 3 authors, 2016-08-11 · open the first message on its own page

[PATCH] Teach git-branch howto rename a branch

From: Lars Hjemli <hidden>
Date: 2016-08-11 19:26:10

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

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.


 builtin-branch.c |   30 ++++++++++-
 refs.c           |  162 ++++++++++++++++++++++++++++++++++++++++++++++--------
 refs.h           |    3 +
 3 files changed, 171 insertions(+), 24 deletions(-)
diff --git a/builtin-branch.c b/builtin-branch.c
index 3d5cb0e..2a21263 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -245,9 +245,29 @@ static void create_branch(const char *name, const char *start,
 		die("Failed to write ref: %s.", strerror(errno));
 }
 
+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);
+
+	snprintf(newref, sizeof newref, "refs/heads/%s", newname);
+	if (check_ref_format(newref))
+		die("Invalid branch name: %s", newref);
+
+	if (resolve_ref(newref, sha1, 1, NULL) && !force)
+		die("A branch named '%s' already exists.\n", newname);
+
+	if (!rename_ref(oldref, newref) && !strcmp(oldname, head))
+		create_symref("HEAD", newref);
+}
+
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
-	int delete = 0, force_delete = 0, force_create = 0;
+	int delete = 0, rename = 0, force_delete = 0, force_create = 0;
 	int verbose = 0, abbrev = DEFAULT_ABBREV;
 	int reflog = 0;
 	int kinds = REF_LOCAL_BRANCH;
@@ -277,6 +297,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			force_create = 1;
 			continue;
 		}
+		if (!strcmp(arg, "--rename")) {
+			rename = 1;
+			continue;
+		}
 		if (!strcmp(arg, "-r")) {
 			kinds = REF_REMOTE_BRANCH;
 			continue;
@@ -311,6 +335,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		delete_branches(argc - i, argv + i, force_delete);
 	else if (i == argc)
 		print_ref_list(kinds, verbose, abbrev);
+	else if (rename && (i == argc - 1))
+		rename_branch(head, argv[i], force_create);
+	else if (rename && (i == argc - 2))
+		rename_branch(argv[i + 1], argv[i], force_create);
 	else if (i == argc - 1)
 		create_branch(argv[i], head, force_create, reflog);
 	else if (i == argc - 2)
diff --git a/refs.c b/refs.c
index 0e156c5..1cb610f 100644
--- a/refs.c
+++ b/refs.c
@@ -534,6 +534,29 @@ static int remove_empty_directories(char *file)
 	return remove_empty_dir_recursive(path, len);
 }
 
+static int is_refname_available(const char *ref, const char *oldref, 
+				struct ref_list *list, int quiet)
+{
+	int namlen = strlen(ref); /* e.g. 'foo/bar' */
+	while (list) {
+		/* list->name could be 'foo' or 'foo/bar/baz' */
+		if (!oldref || strcmp(oldref, list->name)) {
+			int len = strlen(list->name);
+			int cmplen = (namlen < len) ? namlen : len;
+			const char *lead = (namlen < len) ? list->name : ref;
+			if (!strncmp(ref, list->name, cmplen) && 
+			    lead[cmplen] == '/') { 
+				if (!quiet)
+					error("'%s' exists; cannot create '%s'", 
+					      list->name, ref);
+				return 0;
+			}
+		}
+		list = list->next;
+	}
+	return 1;
+}
+
 static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char *old_sha1, int *flag)
 {
 	char *ref_file;
@@ -567,29 +590,14 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 			orig_ref, strerror(errno));
 		goto error_return;
 	}
-	if (is_null_sha1(lock->old_sha1)) {
-		/* The ref did not exist and we are creating it.
-		 * Make sure there is no existing ref that is packed
-		 * whose name begins with our refname, nor a ref whose
-		 * name is a proper prefix of our refname.
-		 */
-		int namlen = strlen(ref); /* e.g. 'foo/bar' */
-		struct ref_list *list = get_packed_refs();
-		while (list) {
-			/* list->name could be 'foo' or 'foo/bar/baz' */
-			int len = strlen(list->name);
-			int cmplen = (namlen < len) ? namlen : len;
-			const char *lead = (namlen < len) ? list->name : ref;
-
-			if (!strncmp(ref, list->name, cmplen) &&
-			    lead[cmplen] == '/') {
-				error("'%s' exists; cannot create '%s'",
-				      list->name, ref);
-				goto error_return;
-			}
-			list = list->next;
-		}
-	}
+	/* When the ref did not exist and we are creating it,
+	 * make sure there is no existing ref that is packed
+	 * whose name begins with our refname, nor a ref whose
+	 * name is a proper prefix of our refname.
+	 */
+	if (is_null_sha1(lock->old_sha1) && 
+            !is_refname_available(ref, NULL, get_packed_refs(), 0))
+		goto error_return;
 
 	lock->lk = xcalloc(1, sizeof(struct lock_file));
 
@@ -700,6 +708,114 @@ int delete_ref(const char *refname, unsigned char *sha1)
 	return ret;
 }
 
+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);
+
+	if (!resolve_ref(oldref, orig_sha1, 1, &flag))
+		return error("refname %s not found", oldref);
+
+	if (!is_refname_available(newref, oldref, get_packed_refs(), 0))
+		return 1;
+
+	if (!is_refname_available(newref, oldref, get_loose_refs(), 0))
+		return 1;
+
+	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));
+	
+	if (delete_ref(oldref, orig_sha1)) {
+		error("unable to delete old %s", oldref);
+		goto rollback;
+	}
+
+	if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1)) {
+		if (errno==EISDIR) {
+			if (remove_empty_directories(git_path("%s", newref))) {
+				error("Directory not empty: %s", newref);
+				goto rollback;
+			}
+		} else {
+			error("unable to delete existing %s", newref);
+			goto rollback;
+		}
+	} 
+
+	if (log && safe_create_leading_directories(git_path("logs/%s", newref))) {
+		error("unable to create directory for %s", newref);
+		goto rollback;
+	}
+
+ retry:
+	if (log && rename(git_path("tmp-renamed-log"), git_path("logs/%s", newref))) {
+		if (errno==EISDIR) {
+			if (remove_empty_directories(git_path("logs/%s", newref))) {
+				error("Directory not empty: logs/%s", newref);
+				goto rollback;
+			}
+			goto retry;
+		} else {
+			error("unable to move logfile tmp-renamed-log to logs/%s: %s",
+				newref, strerror(errno));
+			goto rollback;
+		}
+	}
+	logmoved = log;
+
+	lock = lock_ref_sha1_basic(newref, NULL, NULL);	
+	if (!lock) {
+		error("unable to lock %s for update", newref);
+		goto rollback;
+	}
+
+	snprintf(msg, sizeof msg, "renamed from %s to %s", oldref, newref);
+	lock->force_write = 1;
+	hashcpy(lock->old_sha1, orig_sha1);
+	if (write_ref_sha1(lock, orig_sha1, msg)) {
+		error("unable to write current sha1 into %s", newref);
+		goto rollback;
+	}
+
+	return 0;
+
+ rollback:
+	lock = lock_ref_sha1_basic(oldref, NULL, NULL);	
+	if (!lock) {
+		error("unable to lock %s for rollback", oldref);
+		goto rollbacklog;
+	}
+
+	lock->force_write = 1;
+	flag = log_all_ref_updates;
+	log_all_ref_updates = 0;
+	if (write_ref_sha1(lock, orig_sha1, NULL))
+		error("unable to write current sha1 into %s", oldref);
+	log_all_ref_updates = flag;
+
+ rollbacklog:
+	if (logmoved && rename(git_path("logs/%s", newref), git_path("logs/%s", oldref)))
+		error("unable to restore logfile %s from %s: %s",
+			oldref, newref, strerror(errno));
+	if (!logmoved && log && 
+	    rename(git_path("tmp-renamed-log"), git_path("logs/%s", oldref)))
+		error("unable to restore logfile %s from tmp-renamed-log: %s",
+			oldref, strerror(errno));
+
+	return 1;
+}
+
 void unlock_ref(struct ref_lock *lock)
 {
 	if (lock->lock_fd >= 0) {
diff --git a/refs.h b/refs.h
index a57d437..61419db 100644
--- a/refs.h
+++ b/refs.h
@@ -44,4 +44,7 @@ extern int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned
 /** Returns 0 if target has the right format for a ref. **/
 extern int check_ref_format(const char *target);
 
+/** rename ref, return 0 on success **/
+extern int rename_ref(const char *oldref, const char *newref);
+
 #endif /* REFS_H */
-- 
1.4.4.1.gf64d

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

From: Lars Hjemli <hidden>
Date: 2016-08-11 19:49:43

On 11/28/06, Junio C Hamano [off-list ref] wrote:
Lars Hjemli [off-list ref] writes:
quoted
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".
Ok, how about

  git branch [-m|-M] [oldbranch] newbranch

where -m is 'move' and -M is 'force move'?

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

Can create_symref() fail?
Yes... But what can been done if/when it fails? create_symref()
already seems to be pretty verbose about errors, so the only thing I
can think of is to return the errorcode to the caller (which I should
have done in the first place, git-branch ought to have a usable
exitcode)
+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.
Well, it's a good point. If it's a symlink that's a pretty strong
indication that someone has been messing with the log for some reason,
so to error out is probably the right thing to do.

+       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???
My goal was to save the ref in a tmp-file before deleting the old ref,
not to log the event. I think of it as a way to get out of trouble if
rename_ref should fail badly.

Btw: I't _might_ be interesting to have $GIT_DIR/logs/tmp-renamed-ref
(or something similar) as a branch-independent log of branch renames


Anyway, I'l fix up the mentioned issues in a new patch

--

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

From: Jakub Narebski <hidden>
Date: 2016-08-11 19:55:00

Lars Hjemli wrote:
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.
Could you please document this new feature?

And wouldn't it be better to use <source> <target>, aka <branch> <newname>
like in git-mv and other rename/move commands? "Second name is renamed to
the first" although fits with "<branchname> [<start-point>]" is not natural
for rename.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

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

From: Junio C Hamano <hidden>
Date: 2016-08-11 20:04:05

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???
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help