[PATCH] Implement git remote mv

[PATCH] Implement git remote mv

[Miklos Vajna]

The new rename subcommand does the followings:

1) Renames the remote.foo configuration section to remote.bar

2) Updates the remote.bar.fetch refspecs

3) Updates the branch.*.remote settings

4) Renames the tracking branches.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 Documentation/git-remote.txt |    6 +++
 builtin-remote.c             |  102 ++++++++++++++++++++++++++++++++++++++++++
 t/t5505-remote.sh            |   14 ++++++
 3 files changed, 122 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index bb99810..4b5542a 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 [verse]
 'git remote' [-v | --verbose]
 'git remote add' [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>
+'git remote mv' <old> <new>
 'git remote rm' <name>
 'git remote show' [-n] <name>
 'git remote prune' [-n | --dry-run] <name>
@@ -61,6 +62,11 @@ only makes sense in bare repositories.  If a remote uses mirror
 mode, furthermore, `git push` will always behave as if `\--mirror`
 was passed.
 
+'mv'::
+
+Rename the remote named <old> to <new>. All remote tracking branches and
+configuration settings for the remote are updated.
+
 'rm'::
 
 Remove the remote named <name>. All remote tracking branches and
diff --git a/builtin-remote.c b/builtin-remote.c
index 6b3325d..4a23738 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -10,6 +10,7 @@
 static const char * const builtin_remote_usage[] = {
 	"git remote",
 	"git remote add <name> <url>",
+	"git remote mv <old> <new>",
 	"git remote rm <name>",
 	"git remote show <name>",
 	"git remote prune <name>",
@@ -329,6 +330,105 @@ static int add_branch_for_removal(const char *refname,
 	return 0;
 }
 
+struct rename_info {
+	const char *old;
+	const char *new;
+	struct string_list *remote_branches;
+};
+
+static int read_remote_branches(const char *refname,
+	const unsigned char *sha1, int flags, void *cb_data)
+{
+	struct rename_info *rename = cb_data;
+	struct strbuf buf = STRBUF_INIT;
+
+	strbuf_addf(&buf, "refs/remotes/%s", rename->old);
+	if(!prefixcmp(refname, buf.buf))
+		string_list_append(xstrdup(refname), rename->remote_branches);
+
+	return 0;
+}
+
+static int mv(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+	struct remote *oldremote, *newremote;
+	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;
+	struct string_list remote_branches = { NULL, 0, 0, 0 };
+	struct rename_info rename = { argv[1], argv[2], &remote_branches };
+	int i;
+
+	if (argc != 3)
+		usage_with_options(builtin_remote_usage, options);
+
+	oldremote = remote_get(rename.old);
+	if (!oldremote)
+		die("No such remote: %s", rename.old);
+
+	newremote = remote_get(rename.new);
+	if (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))
+		die("remote %s already exists.", rename.new);
+
+	strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
+	if (!valid_fetch_refspec(buf.buf))
+		die("'%s' is not a valid remote name", rename.new);
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "remote.%s", rename.old);
+	strbuf_addf(&buf2, "remote.%s", rename.new);
+	if (git_config_rename_section(buf.buf, buf2.buf) < 1)
+		return error("Could not rename config section '%s' to '%s'",
+				buf.buf, buf2.buf);
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "remote.%s.fetch", rename.new);
+	if (git_config_set_multivar(buf.buf, NULL, NULL, 1))
+		return error("Could not remove config section '%s'", buf.buf);
+	for (i = 0; i < oldremote->fetch_refspec_nr; i++) {
+		char *ptr;
+
+		strbuf_reset(&buf2);
+		strbuf_addstr(&buf2, oldremote->fetch_refspec[i]);
+		ptr = strstr(buf2.buf, rename.old);
+		if (ptr)
+			strbuf_splice(&buf2, ptr-buf2.buf, strlen(rename.old),
+					rename.new, strlen(rename.new));
+		if (git_config_set_multivar(buf.buf, buf2.buf, "^$", 0))
+			return error("Could not append '%s'", buf.buf);
+	}
+
+	read_branches();
+	for (i = 0; i < branch_list.nr; i++) {
+		struct string_list_item *item = branch_list.items + i;
+		struct branch_info *info = item->util;
+		if (info->remote && !strcmp(info->remote, rename.old)) {
+			strbuf_reset(&buf);
+			strbuf_addf(&buf, "branch.%s.remote", item->string);
+			if (git_config_set(buf.buf, rename.new)) {
+				return error("Could not set '%s'", buf.buf);
+			}
+		}
+	}
+
+	for_each_ref(read_remote_branches, &rename);
+	for (i = 0; i < remote_branches.nr; i++) {
+		struct string_list_item *item = remote_branches.items + i;
+		strbuf_reset(&buf);
+		strbuf_addstr(&buf, item->string);
+		strbuf_splice(&buf, strlen("refs/remotes/"), strlen(rename.old),
+				rename.new, strlen(rename.new));
+		strbuf_reset(&buf2);
+		strbuf_addf(&buf2, "remote: renamed %s to %s",
+				item->string, buf.buf);
+		if (rename_ref(item->string, buf.buf, buf2.buf))
+			die("renaming '%s' failed", item->string);
+	}
+
+	return 0;
+}
+
 static int remove_branches(struct string_list *branches)
 {
 	int i, result = 0;
@@ -696,6 +796,8 @@ int cmd_remote(int argc, const char **argv, const char *prefix)
 		result = show_all();
 	else if (!strcmp(argv[0], "add"))
 		result = add(argc, argv);
+	else if (!strcmp(argv[0], "mv"))
+		result = mv(argc, argv);
 	else if (!strcmp(argv[0], "rm"))
 		result = rm(argc, argv);
 	else if (!strcmp(argv[0], "show"))
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index c449663..9b11fd3 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -324,4 +324,18 @@ test_expect_success 'reject adding remote with an invalid name' '
 
 '
 
+# The first three tests if the config is properly updated, the last one
+# checks if the branches are renamed.
+
+test_expect_success 'rename a remote' '
+
+	git clone one four &&
+	(cd four &&
+	 git remote mv origin upstream &&
+	 git remote show |grep -q upstream &&
+	 git config remote.upstream.fetch |grep -q upstream &&
+	 test $(git config branch.master.remote) = "upstream" &&
+	 git for-each-ref|grep -q refs/remotes/upstream)
+
+'
 test_done
-- 
1.6.0.2

Re: [PATCH] Implement git remote mv

[Brandon Casey]

Miklos Vajna wrote:

> +static int mv(int argc, const char **argv)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	struct remote *oldremote, *newremote;
> +	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;
> +	struct string_list remote_branches = { NULL, 0, 0, 0 };
> +	struct rename_info rename = { argv[1], argv[2], &remote_branches };

I think some non-c99 compilers would have issues with this run-time
initialization from function arguments. Plus, what if argv doesn't have
3 elements? I see you have a check for that below...

> +	int i;
> +
> +	if (argc != 3)
> +		usage_with_options(builtin_remote_usage, options);

-brandon

[PATCH] Implement git remote mv

[Miklos Vajna]

The new rename subcommand does the followings:

1) Renames the remote.foo configuration section to remote.bar

2) Updates the remote.bar.fetch refspecs

3) Updates the branch.*.remote settings

4) Renames the tracking branches.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Wed, Oct 22, 2008 at 11:52:54AM -0500, Brandon Casey <casey@nrlssc.navy.mil> wrote:
> > +   struct rename_info rename = { argv[1], argv[2], &remote_branches
> > };
>
> I think some non-c99 compilers would have issues with this run-time
> initialization from function arguments. Plus, what if argv doesn't
> have
> 3 elements? I see you have a check for that below...

Hm true. Here is a corrected version.

 Documentation/git-remote.txt |    6 ++
 builtin-remote.c             |  106 ++++++++++++++++++++++++++++++++++++++++++
 t/t5505-remote.sh            |   14 ++++++
 3 files changed, 126 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index bb99810..4b5542a 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 [verse]
 'git remote' [-v | --verbose]
 'git remote add' [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>
+'git remote mv' <old> <new>
 'git remote rm' <name>
 'git remote show' [-n] <name>
 'git remote prune' [-n | --dry-run] <name>
@@ -61,6 +62,11 @@ only makes sense in bare repositories.  If a remote uses mirror
 mode, furthermore, `git push` will always behave as if `\--mirror`
 was passed.
 
+'mv'::
+
+Rename the remote named <old> to <new>. All remote tracking branches and
+configuration settings for the remote are updated.
+
 'rm'::
 
 Remove the remote named <name>. All remote tracking branches and
diff --git a/builtin-remote.c b/builtin-remote.c
index 6b3325d..8a54bca 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -10,6 +10,7 @@
 static const char * const builtin_remote_usage[] = {
 	"git remote",
 	"git remote add <name> <url>",
+	"git remote mv <old> <new>",
 	"git remote rm <name>",
 	"git remote show <name>",
 	"git remote prune <name>",
@@ -329,6 +330,109 @@ static int add_branch_for_removal(const char *refname,
 	return 0;
 }
 
+struct rename_info {
+	const char *old;
+	const char *new;
+	struct string_list *remote_branches;
+};
+
+static int read_remote_branches(const char *refname,
+	const unsigned char *sha1, int flags, void *cb_data)
+{
+	struct rename_info *rename = cb_data;
+	struct strbuf buf = STRBUF_INIT;
+
+	strbuf_addf(&buf, "refs/remotes/%s", rename->old);
+	if(!prefixcmp(refname, buf.buf))
+		string_list_append(xstrdup(refname), rename->remote_branches);
+
+	return 0;
+}
+
+static int mv(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+	struct remote *oldremote, *newremote;
+	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;
+	struct string_list remote_branches = { NULL, 0, 0, 0 };
+	struct rename_info rename;
+	int i;
+
+	if (argc != 3)
+		usage_with_options(builtin_remote_usage, options);
+
+	rename.old = argv[1];
+	rename.new = argv[2];
+	rename.remote_branches = &remote_branches;
+
+	oldremote = remote_get(rename.old);
+	if (!oldremote)
+		die("No such remote: %s", rename.old);
+
+	newremote = remote_get(rename.new);
+	if (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))
+		die("remote %s already exists.", rename.new);
+
+	strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
+	if (!valid_fetch_refspec(buf.buf))
+		die("'%s' is not a valid remote name", rename.new);
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "remote.%s", rename.old);
+	strbuf_addf(&buf2, "remote.%s", rename.new);
+	if (git_config_rename_section(buf.buf, buf2.buf) < 1)
+		return error("Could not rename config section '%s' to '%s'",
+				buf.buf, buf2.buf);
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "remote.%s.fetch", rename.new);
+	if (git_config_set_multivar(buf.buf, NULL, NULL, 1))
+		return error("Could not remove config section '%s'", buf.buf);
+	for (i = 0; i < oldremote->fetch_refspec_nr; i++) {
+		char *ptr;
+
+		strbuf_reset(&buf2);
+		strbuf_addstr(&buf2, oldremote->fetch_refspec[i]);
+		ptr = strstr(buf2.buf, rename.old);
+		if (ptr)
+			strbuf_splice(&buf2, ptr-buf2.buf, strlen(rename.old),
+					rename.new, strlen(rename.new));
+		if (git_config_set_multivar(buf.buf, buf2.buf, "^$", 0))
+			return error("Could not append '%s'", buf.buf);
+	}
+
+	read_branches();
+	for (i = 0; i < branch_list.nr; i++) {
+		struct string_list_item *item = branch_list.items + i;
+		struct branch_info *info = item->util;
+		if (info->remote && !strcmp(info->remote, rename.old)) {
+			strbuf_reset(&buf);
+			strbuf_addf(&buf, "branch.%s.remote", item->string);
+			if (git_config_set(buf.buf, rename.new)) {
+				return error("Could not set '%s'", buf.buf);
+			}
+		}
+	}
+
+	for_each_ref(read_remote_branches, &rename);
+	for (i = 0; i < remote_branches.nr; i++) {
+		struct string_list_item *item = remote_branches.items + i;
+		strbuf_reset(&buf);
+		strbuf_addstr(&buf, item->string);
+		strbuf_splice(&buf, strlen("refs/remotes/"), strlen(rename.old),
+				rename.new, strlen(rename.new));
+		strbuf_reset(&buf2);
+		strbuf_addf(&buf2, "remote: renamed %s to %s",
+				item->string, buf.buf);
+		if (rename_ref(item->string, buf.buf, buf2.buf))
+			die("renaming '%s' failed", item->string);
+	}
+
+	return 0;
+}
+
 static int remove_branches(struct string_list *branches)
 {
 	int i, result = 0;
@@ -696,6 +800,8 @@ int cmd_remote(int argc, const char **argv, const char *prefix)
 		result = show_all();
 	else if (!strcmp(argv[0], "add"))
 		result = add(argc, argv);
+	else if (!strcmp(argv[0], "mv"))
+		result = mv(argc, argv);
 	else if (!strcmp(argv[0], "rm"))
 		result = rm(argc, argv);
 	else if (!strcmp(argv[0], "show"))
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index c449663..9b11fd3 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -324,4 +324,18 @@ test_expect_success 'reject adding remote with an invalid name' '
 
 '
 
+# The first three tests if the config is properly updated, the last one
+# checks if the branches are renamed.
+
+test_expect_success 'rename a remote' '
+
+	git clone one four &&
+	(cd four &&
+	 git remote mv origin upstream &&
+	 git remote show |grep -q upstream &&
+	 git config remote.upstream.fetch |grep -q upstream &&
+	 test $(git config branch.master.remote) = "upstream" &&
+	 git for-each-ref|grep -q refs/remotes/upstream)
+
+'
 test_done
-- 
1.6.0.2

Re: [PATCH] Implement git remote mv

[Jeff King]

On Thu, Oct 23, 2008 at 03:18:24AM +0200, Miklos Vajna wrote:

> The new rename subcommand does the followings:
> 
> 1) Renames the remote.foo configuration section to remote.bar
> 
> 2) Updates the remote.bar.fetch refspecs
> 
> 3) Updates the branch.*.remote settings
> 
> 4) Renames the tracking branches.

I can't help but notice that the word "rename" appears all over the
commit description and in the code, but not in the user interface. Maybe
"rename" would be a better name for the command instead of (or in
addition to) "mv"?

-Peff

[PATCH] Implement git remote rename

[Miklos Vajna]

The new rename subcommand does the followings:

1) Renames the remote.foo configuration section to remote.bar

2) Updates the remote.bar.fetch refspecs

3) Updates the branch.*.remote settings

4) Renames the tracking branches.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Wed, Oct 22, 2008 at 11:52:14PM -0400, Jeff King <peff@peff.net> wrote:
> I can't help but notice that the word "rename" appears all over the
> commit description and in the code, but not in the user interface.
> Maybe
> "rename" would be a better name for the command instead of (or in
> addition to) "mv"?

I called it "mv" because of "rm" (it is not "remove") and
git-mv/git-add, but I don't think it's a problem if it's called
"rename". Here is an updated patch.

The function name is still "mv" because of rename(2).

 Documentation/git-remote.txt |    6 ++
 builtin-remote.c             |  106 ++++++++++++++++++++++++++++++++++++++++++
 t/t5505-remote.sh            |   14 ++++++
 3 files changed, 126 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index bb99810..7b227b3 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 [verse]
 'git remote' [-v | --verbose]
 'git remote add' [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>
+'git remote rename' <old> <new>
 'git remote rm' <name>
 'git remote show' [-n] <name>
 'git remote prune' [-n | --dry-run] <name>
@@ -61,6 +62,11 @@ only makes sense in bare repositories.  If a remote uses mirror
 mode, furthermore, `git push` will always behave as if `\--mirror`
 was passed.
 
+'rename'::
+
+Rename the remote named <old> to <new>. All remote tracking branches and
+configuration settings for the remote are updated.
+
 'rm'::
 
 Remove the remote named <name>. All remote tracking branches and
diff --git a/builtin-remote.c b/builtin-remote.c
index 6b3325d..106d6f6 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -10,6 +10,7 @@
 static const char * const builtin_remote_usage[] = {
 	"git remote",
 	"git remote add <name> <url>",
+	"git remote rename <old> <new>",
 	"git remote rm <name>",
 	"git remote show <name>",
 	"git remote prune <name>",
@@ -329,6 +330,109 @@ static int add_branch_for_removal(const char *refname,
 	return 0;
 }
 
+struct rename_info {
+	const char *old;
+	const char *new;
+	struct string_list *remote_branches;
+};
+
+static int read_remote_branches(const char *refname,
+	const unsigned char *sha1, int flags, void *cb_data)
+{
+	struct rename_info *rename = cb_data;
+	struct strbuf buf = STRBUF_INIT;
+
+	strbuf_addf(&buf, "refs/remotes/%s", rename->old);
+	if(!prefixcmp(refname, buf.buf))
+		string_list_append(xstrdup(refname), rename->remote_branches);
+
+	return 0;
+}
+
+static int mv(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+	struct remote *oldremote, *newremote;
+	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;
+	struct string_list remote_branches = { NULL, 0, 0, 0 };
+	struct rename_info rename;
+	int i;
+
+	if (argc != 3)
+		usage_with_options(builtin_remote_usage, options);
+
+	rename.old = argv[1];
+	rename.new = argv[2];
+	rename.remote_branches = &remote_branches;
+
+	oldremote = remote_get(rename.old);
+	if (!oldremote)
+		die("No such remote: %s", rename.old);
+
+	newremote = remote_get(rename.new);
+	if (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))
+		die("remote %s already exists.", rename.new);
+
+	strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
+	if (!valid_fetch_refspec(buf.buf))
+		die("'%s' is not a valid remote name", rename.new);
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "remote.%s", rename.old);
+	strbuf_addf(&buf2, "remote.%s", rename.new);
+	if (git_config_rename_section(buf.buf, buf2.buf) < 1)
+		return error("Could not rename config section '%s' to '%s'",
+				buf.buf, buf2.buf);
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "remote.%s.fetch", rename.new);
+	if (git_config_set_multivar(buf.buf, NULL, NULL, 1))
+		return error("Could not remove config section '%s'", buf.buf);
+	for (i = 0; i < oldremote->fetch_refspec_nr; i++) {
+		char *ptr;
+
+		strbuf_reset(&buf2);
+		strbuf_addstr(&buf2, oldremote->fetch_refspec[i]);
+		ptr = strstr(buf2.buf, rename.old);
+		if (ptr)
+			strbuf_splice(&buf2, ptr-buf2.buf, strlen(rename.old),
+					rename.new, strlen(rename.new));
+		if (git_config_set_multivar(buf.buf, buf2.buf, "^$", 0))
+			return error("Could not append '%s'", buf.buf);
+	}
+
+	read_branches();
+	for (i = 0; i < branch_list.nr; i++) {
+		struct string_list_item *item = branch_list.items + i;
+		struct branch_info *info = item->util;
+		if (info->remote && !strcmp(info->remote, rename.old)) {
+			strbuf_reset(&buf);
+			strbuf_addf(&buf, "branch.%s.remote", item->string);
+			if (git_config_set(buf.buf, rename.new)) {
+				return error("Could not set '%s'", buf.buf);
+			}
+		}
+	}
+
+	for_each_ref(read_remote_branches, &rename);
+	for (i = 0; i < remote_branches.nr; i++) {
+		struct string_list_item *item = remote_branches.items + i;
+		strbuf_reset(&buf);
+		strbuf_addstr(&buf, item->string);
+		strbuf_splice(&buf, strlen("refs/remotes/"), strlen(rename.old),
+				rename.new, strlen(rename.new));
+		strbuf_reset(&buf2);
+		strbuf_addf(&buf2, "remote: renamed %s to %s",
+				item->string, buf.buf);
+		if (rename_ref(item->string, buf.buf, buf2.buf))
+			die("renaming '%s' failed", item->string);
+	}
+
+	return 0;
+}
+
 static int remove_branches(struct string_list *branches)
 {
 	int i, result = 0;
@@ -696,6 +800,8 @@ int cmd_remote(int argc, const char **argv, const char *prefix)
 		result = show_all();
 	else if (!strcmp(argv[0], "add"))
 		result = add(argc, argv);
+	else if (!strcmp(argv[0], "rename"))
+		result = mv(argc, argv);
 	else if (!strcmp(argv[0], "rm"))
 		result = rm(argc, argv);
 	else if (!strcmp(argv[0], "show"))
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index c449663..58bd7bf 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -324,4 +324,18 @@ test_expect_success 'reject adding remote with an invalid name' '
 
 '
 
+# The first three tests if the config is properly updated, the last one
+# checks if the branches are renamed.
+
+test_expect_success 'rename a remote' '
+
+	git clone one four &&
+	(cd four &&
+	 git remote rename origin upstream &&
+	 git remote show |grep -q upstream &&
+	 git config remote.upstream.fetch |grep -q upstream &&
+	 test $(git config branch.master.remote) = "upstream" &&
+	 git for-each-ref|grep -q refs/remotes/upstream)
+
+'
 test_done
-- 
1.6.0.2

Re: [PATCH] Implement git remote rename

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> +static int mv(int argc, const char **argv)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	struct remote *oldremote, *newremote;
> +	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;
> +	struct string_list remote_branches = { NULL, 0, 0, 0 };
> +	struct rename_info rename;
> +	int i;
> +
> +	if (argc != 3)
> +		usage_with_options(builtin_remote_usage, options);
> +
> +	rename.old = argv[1];
> +	rename.new = argv[2];
> +	rename.remote_branches = &remote_branches;
> +
> +	oldremote = remote_get(rename.old);
> +	if (!oldremote)
> +		die("No such remote: %s", rename.old);
> +
> +	newremote = remote_get(rename.new);
> +	if (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))
> +		die("remote %s already exists.", rename.new);
> +
> +	strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
> +	if (!valid_fetch_refspec(buf.buf))
> +		die("'%s' is not a valid remote name", rename.new);
> +
> +	strbuf_reset(&buf);
> +	strbuf_addf(&buf, "remote.%s", rename.old);
> +	strbuf_addf(&buf2, "remote.%s", rename.new);
> +	if (git_config_rename_section(buf.buf, buf2.buf) < 1)
> +		return error("Could not rename config section '%s' to '%s'",
> +				buf.buf, buf2.buf);

Hmm, remote_get() can read from all three supported places that you can
define remotes.  Could you explain what happens if the old remote is read
from say $GIT_DIR/remotes/origin and you are renaming it to "upstream"
with "git remote rename origin upstream"?

I suspect that if you record where you read the configuration from in
"struct remote" and add necessary code to remove the original when
rename.old is *not* coming from in-config definition, you would make it
possible for repositories initialized with older git that has either
$GIT_DIR/branches/origin or $GIT_DIR/remotes/origin to be migrated to the
in-config format using "git remote rename origin origin".

[PATCH 0/2] Fixes for git branch -m / update-ref --no-deref -d

[Miklos Vajna]

On Fri, Oct 24, 2008 at 04:33:28PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Hmm, remote_get() can read from all three supported places that you
> can
> define remotes.  Could you explain what happens if the old remote is
> read
> from say $GIT_DIR/remotes/origin and you are renaming it to "upstream"
> with "git remote rename origin upstream"?

While trying to answer your question, I noticed that
rename_ref()/delete_ref() did not really handled symrefs.

Regarding rename_ref() (for users: git branch -m) I think you can't
create symrefs in the refs/heads namespace without using plumbing, so
most users are not affected.

Regarding delete_ref() (for users: git update-ref --no-deref -d) in most
repos you just have HEAD as symref and you never want to delete it, but
in case the user asks for it, I think we just have to do so.

Here are two patches to fix these issues (and in fact they will be
required for git remote rename as well).

Miklos Vajna (2):
  Fix git branch -m for symrefs.
  Fix git update-ref --no-deref -d.

 builtin-branch.c       |    2 +-
 builtin-receive-pack.c |    2 +-
 builtin-remote.c       |    4 +-
 builtin-reset.c        |    2 +-
 builtin-send-pack.c    |    2 +-
 builtin-tag.c          |    2 +-
 builtin-update-ref.c   |    8 ++++--
 cache.h                |    2 +-
 refs.c                 |   56 ++++++++++++++++++++++++++++++------------------
 t/t1400-update-ref.sh  |    7 ++++++
 t/t3200-branch.sh      |    9 +++++++
 11 files changed, 64 insertions(+), 32 deletions(-)

[PATCH 1/2] Fix git branch -m for symrefs.

[Miklos Vajna]

This had two problems with symrefs. First, it copied the actual sha1
instead of the "pointer", second it failed to remove the old ref after a
successful rename.

Given that till now delete_ref() always dereferenced symrefs, a new
parameters has been introduced to delete_ref() to allow deleting refs
without a dereference.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-branch.c       |    2 +-
 builtin-receive-pack.c |    2 +-
 builtin-remote.c       |    4 +-
 builtin-reset.c        |    2 +-
 builtin-send-pack.c    |    2 +-
 builtin-tag.c          |    2 +-
 builtin-update-ref.c   |    2 +-
 cache.h                |    2 +-
 refs.c                 |   56 ++++++++++++++++++++++++++++++------------------
 t/t3200-branch.sh      |    9 +++++++
 10 files changed, 53 insertions(+), 30 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 8d634ff..2b3613f 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -160,7 +160,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 			continue;
 		}
 
-		if (delete_ref(name, sha1)) {
+		if (delete_ref(name, sha1, 0)) {
 			error("Error deleting %sbranch '%s'", remote,
 			       argv[i]);
 			ret = 1;
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 45e3cd9..ab5fa1c 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -224,7 +224,7 @@ static const char *update(struct command *cmd)
 			warning ("Allowing deletion of corrupt ref.");
 			old_sha1 = NULL;
 		}
-		if (delete_ref(name, old_sha1)) {
+		if (delete_ref(name, old_sha1, 0)) {
 			error("failed to delete %s", name);
 			return "failed to delete";
 		}
diff --git a/builtin-remote.c b/builtin-remote.c
index a5883df..3f2113c 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -441,7 +441,7 @@ static int remove_branches(struct string_list *branches)
 		const char *refname = item->string;
 		unsigned char *sha1 = item->util;
 
-		if (delete_ref(refname, sha1))
+		if (delete_ref(refname, sha1, 0))
 			result |= error("Could not remove branch %s", refname);
 	}
 	return result;
@@ -669,7 +669,7 @@ static int prune(int argc, const char **argv)
 			const char *refname = states.stale.items[i].util;
 
 			if (!dry_run)
-				result |= delete_ref(refname, NULL);
+				result |= delete_ref(refname, NULL, 0);
 
 			printf(" * [%s] %s\n", dry_run ? "would prune" : "pruned",
 			       abbrev_ref(refname, "refs/remotes/"));
diff --git a/builtin-reset.c b/builtin-reset.c
index 16e6bb2..9514b77 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -279,7 +279,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		update_ref(msg, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
 	}
 	else if (old_orig)
-		delete_ref("ORIG_HEAD", old_orig);
+		delete_ref("ORIG_HEAD", old_orig, 0);
 	prepend_reflog_action("updating HEAD", msg, sizeof(msg));
 	update_ref_status = update_ref(msg, "HEAD", sha1, orig, 0, MSG_ON_ERR);
 
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 910db92..bbf6e0a 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -234,7 +234,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 		if (args.verbose)
 			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
 		if (ref->deletion) {
-			delete_ref(rs.dst, NULL);
+			delete_ref(rs.dst, NULL, 0);
 		} else
 			update_ref("update by push", rs.dst,
 					ref->new_sha1, NULL, 0, 0);
diff --git a/builtin-tag.c b/builtin-tag.c
index b13fa34..1ff7b37 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -125,7 +125,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
 static int delete_tag(const char *name, const char *ref,
 				const unsigned char *sha1)
 {
-	if (delete_ref(ref, sha1))
+	if (delete_ref(ref, sha1, 0))
 		return 1;
 	printf("Deleted tag '%s'\n", name);
 	return 0;
diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index 56a0b1b..d8f3142 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -48,7 +48,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		die("%s: not a valid old SHA1", oldval);
 
 	if (delete)
-		return delete_ref(refname, oldval ? oldsha1 : NULL);
+		return delete_ref(refname, oldval ? oldsha1 : NULL, 0);
 	else
 		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
 				  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);
diff --git a/cache.h b/cache.h
index b0edbf9..9951952 100644
--- a/cache.h
+++ b/cache.h
@@ -434,7 +434,7 @@ extern int commit_locked_index(struct lock_file *);
 extern void set_alternate_index_output(const char *);
 extern int close_lock_file(struct lock_file *);
 extern void rollback_lock_file(struct lock_file *);
-extern int delete_ref(const char *, const unsigned char *sha1);
+extern int delete_ref(const char *, const unsigned char *sha1, int flags);
 
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
diff --git a/refs.c b/refs.c
index 0a126fa..93a61e1 100644
--- a/refs.c
+++ b/refs.c
@@ -921,25 +921,32 @@ static int repack_without_ref(const char *refname)
 	return commit_lock_file(&packlock);
 }
 
-int delete_ref(const char *refname, const unsigned char *sha1)
+int delete_ref(const char *refname, const unsigned char *sha1, int flags)
 {
 	struct ref_lock *lock;
-	int err, i, ret = 0, flag = 0;
+	int err, i = 0, ret = 0, flag = 0;
+	char *path;
 
 	lock = lock_ref_sha1_basic(refname, sha1, 0, &flag);
 	if (!lock)
 		return 1;
 	if (!(flag & REF_ISPACKED)) {
 		/* loose */
-		i = strlen(lock->lk->filename) - 5; /* .lock */
-		lock->lk->filename[i] = 0;
-		err = unlink(lock->lk->filename);
+		if (!(flags & REF_NODEREF)) {
+			i = strlen(lock->lk->filename) - 5; /* .lock */
+			lock->lk->filename[i] = 0;
+			path = lock->lk->filename;
+		} else {
+			path = git_path(refname);
+		}
+		err = unlink(path);
 		if (err && errno != ENOENT) {
 			ret = 1;
 			error("unlink(%s) failed: %s",
-			      lock->lk->filename, strerror(errno));
+			      path, strerror(errno));
 		}
-		lock->lk->filename[i] = '.';
+		if (!(flags & REF_NODEREF))
+			lock->lk->filename[i] = '.';
 	}
 	/* removing the loose one could have resurrected an earlier
 	 * packed one.  Also, if it was not loose we need to repack
@@ -964,10 +971,15 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	struct ref_lock *lock;
 	struct stat loginfo;
 	int log = !lstat(git_path("logs/%s", oldref), &loginfo);
+	const char *symref = NULL;
+	int is_symref = 0;
 
 	if (S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldref);
 
+	symref = resolve_ref(oldref, orig_sha1, 0, &flag);
+	if (flag & REF_ISSYMREF)
+		is_symref = 1;
 	if (!resolve_ref(oldref, orig_sha1, 1, &flag))
 		return error("refname %s not found", oldref);
 
@@ -988,12 +1000,12 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 		return error("unable to move logfile logs/%s to tmp-renamed-log: %s",
 			oldref, strerror(errno));
 
-	if (delete_ref(oldref, orig_sha1)) {
+	if (delete_ref(oldref, orig_sha1, REF_NODEREF)) {
 		error("unable to delete old %s", oldref);
 		goto rollback;
 	}
 
-	if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1)) {
+	if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1, REF_NODEREF)) {
 		if (errno==EISDIR) {
 			if (remove_empty_directories(git_path("%s", newref))) {
 				error("Directory not empty: %s", newref);
@@ -1031,18 +1043,20 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	}
 	logmoved = log;
 
-	lock = lock_ref_sha1_basic(newref, NULL, 0, NULL);
-	if (!lock) {
-		error("unable to lock %s for update", newref);
-		goto rollback;
-	}
-
-	lock->force_write = 1;
-	hashcpy(lock->old_sha1, orig_sha1);
-	if (write_ref_sha1(lock, orig_sha1, logmsg)) {
-		error("unable to write current sha1 into %s", newref);
-		goto rollback;
-	}
+	if (!is_symref) {
+		lock = lock_ref_sha1_basic(newref, NULL, 0, NULL);
+		if (!lock) {
+			error("unable to lock %s for update", newref);
+			goto rollback;
+		}
+		lock->force_write = 1;
+		hashcpy(lock->old_sha1, orig_sha1);
+		if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+			error("unable to write current sha1 into %s", newref);
+			goto rollback;
+		}
+	} else
+		create_symref(newref, symref, logmsg);
 
 	return 0;
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 2147eac..fdeb1f5 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -112,6 +112,15 @@ test_expect_success 'config information was renamed, too' \
 	"test $(git config branch.s.dummy) = Hello &&
 	 test_must_fail git config branch.s/s/dummy"
 
+test_expect_success 'renaming a symref' \
+'
+	git symbolic-ref refs/heads/master2 refs/heads/master &&
+	git branch -m master2 master3 &&
+	git symbolic-ref refs/heads/master3 &&
+	test -f .git/refs/heads/master &&
+	! test -f .git/refs/heads/master2
+'
+
 test_expect_success \
     'git branch -m u v should fail when the reflog for u is a symlink' '
      git branch -l u &&
-- 
1.6.0.2

[PATCH 2/2] Fix git update-ref --no-deref -d.

[Miklos Vajna]

Till now --no-deref was just ignored when deleting refs, fix this.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-update-ref.c  |    8 +++++---
 t/t1400-update-ref.sh |    7 +++++++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index d8f3142..378dc1b 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -13,7 +13,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 {
 	const char *refname, *oldval, *msg=NULL;
 	unsigned char sha1[20], oldsha1[20];
-	int delete = 0, no_deref = 0;
+	int delete = 0, no_deref = 0, flags = 0;
 	struct option options[] = {
 		OPT_STRING( 'm', NULL, &msg, "reason", "reason of the update"),
 		OPT_BOOLEAN('d', NULL, &delete, "deletes the reference"),
@@ -47,9 +47,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	if (oldval && *oldval && get_sha1(oldval, oldsha1))
 		die("%s: not a valid old SHA1", oldval);
 
+	if (no_deref)
+		flags = REF_NODEREF;
 	if (delete)
-		return delete_ref(refname, oldval ? oldsha1 : NULL, 0);
+		return delete_ref(refname, oldval ? oldsha1 : NULL, flags);
 	else
 		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
-				  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);
+				  flags, DIE_ON_ERR);
 }
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 04c2b16..8139cd6 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -75,6 +75,13 @@ test_expect_success "delete $m (by HEAD)" '
 '
 rm -f .git/$m
 
+cp -f .git/HEAD .git/HEAD.orig
+test_expect_success "delete symref without dereference" '
+	git update-ref --no-deref -d HEAD &&
+	! test -f .git/HEAD
+'
+cp -f .git/HEAD.orig .git/HEAD
+
 test_expect_success '(not) create HEAD with old sha1' "
 	test_must_fail git update-ref HEAD $A $B
 "
-- 
1.6.0.2

Re: [PATCH 1/2] Fix git branch -m for symrefs.

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> -int delete_ref(const char *refname, const unsigned char *sha1)
> +int delete_ref(const char *refname, const unsigned char *sha1, int flags)
>  {
>  	struct ref_lock *lock;
> -	int err, i, ret = 0, flag = 0;
> +	int err, i = 0, ret = 0, flag = 0;
> +	char *path;
>  
>  	lock = lock_ref_sha1_basic(refname, sha1, 0, &flag);
>  	if (!lock)
>  		return 1;
>  	if (!(flag & REF_ISPACKED)) {

Two variables flag vs flags is a bit confusing, isn't it?  How about
naming the new one "delopt" or something?

The new variable "char *path" at the toplevel can be confined in the scope
of this if () {} block and probably can become "const char *", right?

> @@ -964,10 +971,15 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
>  	struct ref_lock *lock;
>  	struct stat loginfo;
>  	int log = !lstat(git_path("logs/%s", oldref), &loginfo);
> +	const char *symref = NULL;
> +	int is_symref = 0;
>  
>  	if (S_ISLNK(loginfo.st_mode))
>  		return error("reflog for %s is a symlink", oldref);

Possible bug in the context.  When there is no reflog for the ref being
renamed, lstat would fail; it doesn't feel right to have this S_ISLNK()
before checking the result of the lstat which is in "log".

> +	symref = resolve_ref(oldref, orig_sha1, 0, &flag);
> +	if (flag & REF_ISSYMREF)
> +		is_symref = 1;
>  	if (!resolve_ref(oldref, orig_sha1, 1, &flag))
>  		return error("refname %s not found", oldref);

Do we really need two calls to resolve_ref()?  Your new call calls it
without must-exist bit --- why?  Immediately after that, the existing call
will barf if it does not exist anyway.

I agree it is good to have symref aware delete_ref(), but I am not sure
supporting symref in rename_ref() is either needed or necessarily a good
idea.  You also need to worry about a symref pointing at a branch yet to
be born.

In the meantime, I think we should just check (flag & REF_ISSYMREF) after
the existing resolve_ref() we can see in the context above, and error out
saying you cannot rename a symref, and do nothing else.

[PATCH 0/3] symref rename/delete fixes

[Miklos Vajna]

On Sat, Oct 25, 2008 at 11:31:01AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > +int delete_ref(const char *refname, const unsigned char *sha1, int
> > flags)
> >  {
> >     struct ref_lock *lock;
> > -   int err, i, ret = 0, flag = 0;
> > +   int err, i = 0, ret = 0, flag = 0;
> > +   char *path;

> Two variables flag vs flags is a bit confusing, isn't it?  How about
> naming the new one "delopt" or something?

Renamed.

> The new variable "char *path" at the toplevel can be confined in the
> scope
> of this if () {} block and probably can become "const char *", right?

Yes - moved.

> >     int log = !lstat(git_path("logs/%s", oldref), &loginfo);
> > +   const char *symref = NULL;
> > +   int is_symref = 0;
> >
> >     if (S_ISLNK(loginfo.st_mode))
> >             return error("reflog for %s is a symlink", oldref);
>
> Possible bug in the context.  When there is no reflog for the ref
> being
> renamed, lstat would fail; it doesn't feel right to have this
> S_ISLNK()
> before checking the result of the lstat which is in "log".

OK, this went to a separate patch.

> > +   symref = resolve_ref(oldref, orig_sha1, 0, &flag);
> > +   if (flag & REF_ISSYMREF)
> > +           is_symref = 1;
> >     if (!resolve_ref(oldref, orig_sha1, 1, &flag))
> >             return error("refname %s not found", oldref);
>
> Do we really need two calls to resolve_ref()?  Your new call calls it
> without must-exist bit --- why?  Immediately after that, the existing
> call
> will barf if it does not exist anyway.

Just having

        if (!symref)
                return error("refname %s not found", oldref);

first looks weird, given that the error message is not "refname %s is
not a symref", but you are right, changed.

> I agree it is good to have symref aware delete_ref(), but I am not
> sure
> supporting symref in rename_ref() is either needed or necessarily a
> good
> idea.  You also need to worry about a symref pointing at a branch yet
> to
> be born.

That is currently not supported and the error message of 'git branch -m'
is (in case foo points to refs/heads/bar and bar is not yet born):

error: refname refs/heads/foo not found
fatal: Branch rename failed

which is quite acceptable, IMHO[1].

> In the meantime, I think we should just check (flag & REF_ISSYMREF)
> after
> the existing resolve_ref() we can see in the context above, and error
> out
> saying you cannot rename a symref, and do nothing else.

A symref-aware rename_ref() is needed by git remove rename, since it
typically does origin/HEAD -> upstream/HEAD symref renames there.

Of course you can say that this should be handled by git-remote itself,
without using rename_ref() but that not seem to be a good solution to
me. (Workaround in the wrong layer, instead of a solution in a good
one.)

[1] I mean, I have a real-world scenario (git remove rename) for "why
renaming a symref is a good idea", but I don't think renaming a ref
pointing to a yet-to-be-born ref has any real world users.

Miklos Vajna (3):
  Fix git branch -m for symrefs.
  rename_ref(): handle the case when the reflog of a ref does not exist
  Fix git update-ref --no-deref -d.

 builtin-branch.c       |    2 +-
 builtin-receive-pack.c |    2 +-
 builtin-remote.c       |    4 +-
 builtin-reset.c        |    2 +-
 builtin-send-pack.c    |    2 +-
 builtin-tag.c          |    2 +-
 builtin-update-ref.c   |    8 ++++--
 cache.h                |    2 +-
 refs.c                 |   61 +++++++++++++++++++++++++++++------------------
 t/t1400-update-ref.sh  |    7 +++++
 t/t3200-branch.sh      |    9 +++++++
 11 files changed, 67 insertions(+), 34 deletions(-)

Also available in the 'symref-mv' branch of 'git://repo.or.cz/git/vmiklos.git'.

[PATCH 1/3] Fix git branch -m for symrefs.

[Miklos Vajna]

This had two problems with symrefs. First, it copied the actual sha1
instead of the "pointer", second it failed to remove the old ref after a
successful rename.

Given that till now delete_ref() always dereferenced symrefs, a new
parameters has been introduced to delete_ref() to allow deleting refs
without a dereference.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-branch.c       |    2 +-
 builtin-receive-pack.c |    2 +-
 builtin-remote.c       |    4 +-
 builtin-reset.c        |    2 +-
 builtin-send-pack.c    |    2 +-
 builtin-tag.c          |    2 +-
 builtin-update-ref.c   |    2 +-
 cache.h                |    2 +-
 refs.c                 |   59 ++++++++++++++++++++++++++++++------------------
 t/t3200-branch.sh      |    9 +++++++
 10 files changed, 55 insertions(+), 31 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 8d634ff..2b3613f 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -160,7 +160,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 			continue;
 		}
 
-		if (delete_ref(name, sha1)) {
+		if (delete_ref(name, sha1, 0)) {
 			error("Error deleting %sbranch '%s'", remote,
 			       argv[i]);
 			ret = 1;
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 45e3cd9..ab5fa1c 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -224,7 +224,7 @@ static const char *update(struct command *cmd)
 			warning ("Allowing deletion of corrupt ref.");
 			old_sha1 = NULL;
 		}
-		if (delete_ref(name, old_sha1)) {
+		if (delete_ref(name, old_sha1, 0)) {
 			error("failed to delete %s", name);
 			return "failed to delete";
 		}
diff --git a/builtin-remote.c b/builtin-remote.c
index df2be06..e396a3a 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -337,7 +337,7 @@ static int remove_branches(struct string_list *branches)
 		const char *refname = item->string;
 		unsigned char *sha1 = item->util;
 
-		if (delete_ref(refname, sha1))
+		if (delete_ref(refname, sha1, 0))
 			result |= error("Could not remove branch %s", refname);
 	}
 	return result;
@@ -565,7 +565,7 @@ static int prune(int argc, const char **argv)
 			const char *refname = states.stale.items[i].util;
 
 			if (!dry_run)
-				result |= delete_ref(refname, NULL);
+				result |= delete_ref(refname, NULL, 0);
 
 			printf(" * [%s] %s\n", dry_run ? "would prune" : "pruned",
 			       abbrev_ref(refname, "refs/remotes/"));
diff --git a/builtin-reset.c b/builtin-reset.c
index 16e6bb2..9514b77 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -279,7 +279,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		update_ref(msg, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
 	}
 	else if (old_orig)
-		delete_ref("ORIG_HEAD", old_orig);
+		delete_ref("ORIG_HEAD", old_orig, 0);
 	prepend_reflog_action("updating HEAD", msg, sizeof(msg));
 	update_ref_status = update_ref(msg, "HEAD", sha1, orig, 0, MSG_ON_ERR);
 
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 910db92..bbf6e0a 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -234,7 +234,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 		if (args.verbose)
 			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
 		if (ref->deletion) {
-			delete_ref(rs.dst, NULL);
+			delete_ref(rs.dst, NULL, 0);
 		} else
 			update_ref("update by push", rs.dst,
 					ref->new_sha1, NULL, 0, 0);
diff --git a/builtin-tag.c b/builtin-tag.c
index b13fa34..1ff7b37 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -125,7 +125,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
 static int delete_tag(const char *name, const char *ref,
 				const unsigned char *sha1)
 {
-	if (delete_ref(ref, sha1))
+	if (delete_ref(ref, sha1, 0))
 		return 1;
 	printf("Deleted tag '%s'\n", name);
 	return 0;
diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index 56a0b1b..d8f3142 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -48,7 +48,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		die("%s: not a valid old SHA1", oldval);
 
 	if (delete)
-		return delete_ref(refname, oldval ? oldsha1 : NULL);
+		return delete_ref(refname, oldval ? oldsha1 : NULL, 0);
 	else
 		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
 				  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);
diff --git a/cache.h b/cache.h
index b0edbf9..a3c77f0 100644
--- a/cache.h
+++ b/cache.h
@@ -434,7 +434,7 @@ extern int commit_locked_index(struct lock_file *);
 extern void set_alternate_index_output(const char *);
 extern int close_lock_file(struct lock_file *);
 extern void rollback_lock_file(struct lock_file *);
-extern int delete_ref(const char *, const unsigned char *sha1);
+extern int delete_ref(const char *, const unsigned char *sha1, int delopt);
 
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
diff --git a/refs.c b/refs.c
index 0a126fa..70c0967 100644
--- a/refs.c
+++ b/refs.c
@@ -921,25 +921,33 @@ static int repack_without_ref(const char *refname)
 	return commit_lock_file(&packlock);
 }
 
-int delete_ref(const char *refname, const unsigned char *sha1)
+int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 {
 	struct ref_lock *lock;
-	int err, i, ret = 0, flag = 0;
+	int err, i = 0, ret = 0, flag = 0;
 
 	lock = lock_ref_sha1_basic(refname, sha1, 0, &flag);
 	if (!lock)
 		return 1;
 	if (!(flag & REF_ISPACKED)) {
 		/* loose */
-		i = strlen(lock->lk->filename) - 5; /* .lock */
-		lock->lk->filename[i] = 0;
-		err = unlink(lock->lk->filename);
+		const char *path;
+
+		if (!(delopt & REF_NODEREF)) {
+			i = strlen(lock->lk->filename) - 5; /* .lock */
+			lock->lk->filename[i] = 0;
+			path = lock->lk->filename;
+		} else {
+			path = git_path(refname);
+		}
+		err = unlink(path);
 		if (err && errno != ENOENT) {
 			ret = 1;
 			error("unlink(%s) failed: %s",
-			      lock->lk->filename, strerror(errno));
+			      path, strerror(errno));
 		}
-		lock->lk->filename[i] = '.';
+		if (!(delopt & REF_NODEREF))
+			lock->lk->filename[i] = '.';
 	}
 	/* removing the loose one could have resurrected an earlier
 	 * packed one.  Also, if it was not loose we need to repack
@@ -964,11 +972,16 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	struct ref_lock *lock;
 	struct stat loginfo;
 	int log = !lstat(git_path("logs/%s", oldref), &loginfo);
+	const char *symref = NULL;
+	int is_symref = 0;
 
 	if (S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldref);
 
-	if (!resolve_ref(oldref, orig_sha1, 1, &flag))
+	symref = resolve_ref(oldref, orig_sha1, 1, &flag);
+	if (flag & REF_ISSYMREF)
+		is_symref = 1;
+	if (!symref)
 		return error("refname %s not found", oldref);
 
 	if (!is_refname_available(newref, oldref, get_packed_refs(), 0))
@@ -988,12 +1001,12 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 		return error("unable to move logfile logs/%s to tmp-renamed-log: %s",
 			oldref, strerror(errno));
 
-	if (delete_ref(oldref, orig_sha1)) {
+	if (delete_ref(oldref, orig_sha1, REF_NODEREF)) {
 		error("unable to delete old %s", oldref);
 		goto rollback;
 	}
 
-	if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1)) {
+	if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1, REF_NODEREF)) {
 		if (errno==EISDIR) {
 			if (remove_empty_directories(git_path("%s", newref))) {
 				error("Directory not empty: %s", newref);
@@ -1031,18 +1044,20 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	}
 	logmoved = log;
 
-	lock = lock_ref_sha1_basic(newref, NULL, 0, NULL);
-	if (!lock) {
-		error("unable to lock %s for update", newref);
-		goto rollback;
-	}
-
-	lock->force_write = 1;
-	hashcpy(lock->old_sha1, orig_sha1);
-	if (write_ref_sha1(lock, orig_sha1, logmsg)) {
-		error("unable to write current sha1 into %s", newref);
-		goto rollback;
-	}
+	if (!is_symref) {
+		lock = lock_ref_sha1_basic(newref, NULL, 0, NULL);
+		if (!lock) {
+			error("unable to lock %s for update", newref);
+			goto rollback;
+		}
+		lock->force_write = 1;
+		hashcpy(lock->old_sha1, orig_sha1);
+		if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+			error("unable to write current sha1 into %s", newref);
+			goto rollback;
+		}
+	} else
+		create_symref(newref, symref, logmsg);
 
 	return 0;
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 2147eac..fdeb1f5 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -112,6 +112,15 @@ test_expect_success 'config information was renamed, too' \
 	"test $(git config branch.s.dummy) = Hello &&
 	 test_must_fail git config branch.s/s/dummy"
 
+test_expect_success 'renaming a symref' \
+'
+	git symbolic-ref refs/heads/master2 refs/heads/master &&
+	git branch -m master2 master3 &&
+	git symbolic-ref refs/heads/master3 &&
+	test -f .git/refs/heads/master &&
+	! test -f .git/refs/heads/master2
+'
+
 test_expect_success \
     'git branch -m u v should fail when the reflog for u is a symlink' '
      git branch -l u &&
-- 
1.6.0.2

[PATCH 2/3] rename_ref(): handle the case when the reflog of a ref does not exist

[Miklos Vajna]

We tried to check if a reflog of a ref is a symlink without first
checking if it exists, which is a bug.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 refs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/refs.c b/refs.c
index 70c0967..3c39a31 100644
--- a/refs.c
+++ b/refs.c
@@ -975,7 +975,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	const char *symref = NULL;
 	int is_symref = 0;
 
-	if (S_ISLNK(loginfo.st_mode))
+	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldref);
 
 	symref = resolve_ref(oldref, orig_sha1, 1, &flag);
-- 
1.6.0.2

[PATCH 3/3] Fix git update-ref --no-deref -d.

[Miklos Vajna]

Till now --no-deref was just ignored when deleting refs, fix this.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-update-ref.c  |    8 +++++---
 t/t1400-update-ref.sh |    7 +++++++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index d8f3142..378dc1b 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -13,7 +13,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 {
 	const char *refname, *oldval, *msg=NULL;
 	unsigned char sha1[20], oldsha1[20];
-	int delete = 0, no_deref = 0;
+	int delete = 0, no_deref = 0, flags = 0;
 	struct option options[] = {
 		OPT_STRING( 'm', NULL, &msg, "reason", "reason of the update"),
 		OPT_BOOLEAN('d', NULL, &delete, "deletes the reference"),
@@ -47,9 +47,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	if (oldval && *oldval && get_sha1(oldval, oldsha1))
 		die("%s: not a valid old SHA1", oldval);
 
+	if (no_deref)
+		flags = REF_NODEREF;
 	if (delete)
-		return delete_ref(refname, oldval ? oldsha1 : NULL, 0);
+		return delete_ref(refname, oldval ? oldsha1 : NULL, flags);
 	else
 		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
-				  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);
+				  flags, DIE_ON_ERR);
 }
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 04c2b16..8139cd6 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -75,6 +75,13 @@ test_expect_success "delete $m (by HEAD)" '
 '
 rm -f .git/$m
 
+cp -f .git/HEAD .git/HEAD.orig
+test_expect_success "delete symref without dereference" '
+	git update-ref --no-deref -d HEAD &&
+	! test -f .git/HEAD
+'
+cp -f .git/HEAD.orig .git/HEAD
+
 test_expect_success '(not) create HEAD with old sha1' "
 	test_must_fail git update-ref HEAD $A $B
 "
-- 
1.6.0.2

Re: [PATCH 0/3] symref rename/delete fixes

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> A symref-aware rename_ref() is needed by git remove rename, since it
> typically does origin/HEAD -> upstream/HEAD symref renames there.
>
> Of course you can say that this should be handled by git-remote itself,
> without using rename_ref() but that not seem to be a good solution to
> me. (Workaround in the wrong layer, instead of a solution in a good
> one.)

I do not think it is a workaround at all.

I would even say that the renaming of symref that "git remote rename"
needs to do is fundamentally different from what rename_ref() is about,
and trying to cram it into rename_ref() is a grave mistake.

If you "git remote rename origin upstream" when origin/HEAD points at
refs/remotes/origin/master, you need to make the renamed one point at
refs/remotes/upstream/master, as you will be renaming origin/master to
upstream/master.

Normal "rename_ref()" would just rename the ref without touching its
contents, and if you used it to implement "git remote rename", your
upstream/HEAD would point at the old name "origin/master" that will
disappear when rename is finished, wouldn't it?  I do not think it is
useful.

There may be cases where you would really want to rename the symbolic ref
without changing its value (e.g. which other ref it points at), but as you
mentioned, even "git branch -m" is not such a usecase.

I think it is better to simply forbid renaming of a symref _until_ we know
what we want it to mean.  It is a lot easier to start strict and then add
features, than start loosely and implement an unclean semantics, and then
having to fix that semantics after people start to rely on the initial
(potentially crazy) semantics.

Re: [PATCH 0/3] symref rename/delete fixes

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]

On Sun, Oct 26, 2008 at 10:31:09PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > Of course you can say that this should be handled by git-remote itself,
> > without using rename_ref() but that not seem to be a good solution to
> > me. (Workaround in the wrong layer, instead of a solution in a good
> > one.)
> 
> I do not think it is a workaround at all.
> 
> I would even say that the renaming of symref that "git remote rename"
> needs to do is fundamentally different from what rename_ref() is about,
> and trying to cram it into rename_ref() is a grave mistake.
> 
> If you "git remote rename origin upstream" when origin/HEAD points at
> refs/remotes/origin/master, you need to make the renamed one point at
> refs/remotes/upstream/master, as you will be renaming origin/master to
> upstream/master.
> 
> Normal "rename_ref()" would just rename the ref without touching its
> contents, and if you used it to implement "git remote rename", your
> upstream/HEAD would point at the old name "origin/master" that will
> disappear when rename is finished, wouldn't it?  I do not think it is
> useful.

Ah, I missed that. You convienced me, I'll post updated patches later
today. :)

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 0/3] symref rename/delete fixes

[Miklos Vajna]

On Mon, Oct 27, 2008 at 09:50:55AM +0100, Miklos Vajna <vmiklos@frugalware.org> wrote:
> Ah, I missed that. You convienced me, I'll post updated patches later
> today. :)

Here they are. The first is updated: the delete_ref() part is unchanged,
but the rename_ref() part is dropped and it just bails out with an error
if it detects a symref. The testcase is updated according to this as
well.

The two others are just rebased on top of the new first one, no
functional changes.

Miklos Vajna (3):
  Disallow git branch -m for symrefs.
  rename_ref(): handle the case when the reflog of a ref does not exist
  Fix git update-ref --no-deref -d.

 builtin-branch.c       |    2 +-
 builtin-receive-pack.c |    2 +-
 builtin-remote.c       |    4 ++--
 builtin-reset.c        |    2 +-
 builtin-send-pack.c    |    2 +-
 builtin-tag.c          |    2 +-
 builtin-update-ref.c   |    8 +++++---
 cache.h                |    2 +-
 refs.c                 |   35 ++++++++++++++++++++++++-----------
 t/t1400-update-ref.sh  |    7 +++++++
 t/t3200-branch.sh      |    9 +++++++++
 11 files changed, 53 insertions(+), 22 deletions(-)

[PATCH 1/3] Disallow git branch -m for symrefs.

[Miklos Vajna]

This had two problems with symrefs. First, it copied the actual sha1
instead of the "pointer", second it failed to remove the old ref after a
successful rename.

Given that till now delete_ref() always dereferenced symrefs, a new
parameters has been introduced to delete_ref() to allow deleting refs
without a dereference.

For now, we just don't allow renaming symref as no real-word workflow
needs it and doing so would be just an accent.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-branch.c       |    2 +-
 builtin-receive-pack.c |    2 +-
 builtin-remote.c       |    4 ++--
 builtin-reset.c        |    2 +-
 builtin-send-pack.c    |    2 +-
 builtin-tag.c          |    2 +-
 builtin-update-ref.c   |    2 +-
 cache.h                |    2 +-
 refs.c                 |   33 +++++++++++++++++++++++----------
 t/t3200-branch.sh      |    9 +++++++++
 10 files changed, 41 insertions(+), 19 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 8d634ff..2b3613f 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -160,7 +160,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 			continue;
 		}
 
-		if (delete_ref(name, sha1)) {
+		if (delete_ref(name, sha1, 0)) {
 			error("Error deleting %sbranch '%s'", remote,
 			       argv[i]);
 			ret = 1;
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 45e3cd9..ab5fa1c 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -224,7 +224,7 @@ static const char *update(struct command *cmd)
 			warning ("Allowing deletion of corrupt ref.");
 			old_sha1 = NULL;
 		}
-		if (delete_ref(name, old_sha1)) {
+		if (delete_ref(name, old_sha1, 0)) {
 			error("failed to delete %s", name);
 			return "failed to delete";
 		}
diff --git a/builtin-remote.c b/builtin-remote.c
index df2be06..e396a3a 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -337,7 +337,7 @@ static int remove_branches(struct string_list *branches)
 		const char *refname = item->string;
 		unsigned char *sha1 = item->util;
 
-		if (delete_ref(refname, sha1))
+		if (delete_ref(refname, sha1, 0))
 			result |= error("Could not remove branch %s", refname);
 	}
 	return result;
@@ -565,7 +565,7 @@ static int prune(int argc, const char **argv)
 			const char *refname = states.stale.items[i].util;
 
 			if (!dry_run)
-				result |= delete_ref(refname, NULL);
+				result |= delete_ref(refname, NULL, 0);
 
 			printf(" * [%s] %s\n", dry_run ? "would prune" : "pruned",
 			       abbrev_ref(refname, "refs/remotes/"));
diff --git a/builtin-reset.c b/builtin-reset.c
index 16e6bb2..9514b77 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -279,7 +279,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		update_ref(msg, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
 	}
 	else if (old_orig)
-		delete_ref("ORIG_HEAD", old_orig);
+		delete_ref("ORIG_HEAD", old_orig, 0);
 	prepend_reflog_action("updating HEAD", msg, sizeof(msg));
 	update_ref_status = update_ref(msg, "HEAD", sha1, orig, 0, MSG_ON_ERR);
 
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 910db92..bbf6e0a 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -234,7 +234,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 		if (args.verbose)
 			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
 		if (ref->deletion) {
-			delete_ref(rs.dst, NULL);
+			delete_ref(rs.dst, NULL, 0);
 		} else
 			update_ref("update by push", rs.dst,
 					ref->new_sha1, NULL, 0, 0);
diff --git a/builtin-tag.c b/builtin-tag.c
index b13fa34..1ff7b37 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -125,7 +125,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
 static int delete_tag(const char *name, const char *ref,
 				const unsigned char *sha1)
 {
-	if (delete_ref(ref, sha1))
+	if (delete_ref(ref, sha1, 0))
 		return 1;
 	printf("Deleted tag '%s'\n", name);
 	return 0;
diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index 56a0b1b..d8f3142 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -48,7 +48,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		die("%s: not a valid old SHA1", oldval);
 
 	if (delete)
-		return delete_ref(refname, oldval ? oldsha1 : NULL);
+		return delete_ref(refname, oldval ? oldsha1 : NULL, 0);
 	else
 		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
 				  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);
diff --git a/cache.h b/cache.h
index b0edbf9..a3c77f0 100644
--- a/cache.h
+++ b/cache.h
@@ -434,7 +434,7 @@ extern int commit_locked_index(struct lock_file *);
 extern void set_alternate_index_output(const char *);
 extern int close_lock_file(struct lock_file *);
 extern void rollback_lock_file(struct lock_file *);
-extern int delete_ref(const char *, const unsigned char *sha1);
+extern int delete_ref(const char *, const unsigned char *sha1, int delopt);
 
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
diff --git a/refs.c b/refs.c
index 0a126fa..31a2057 100644
--- a/refs.c
+++ b/refs.c
@@ -921,25 +921,33 @@ static int repack_without_ref(const char *refname)
 	return commit_lock_file(&packlock);
 }
 
-int delete_ref(const char *refname, const unsigned char *sha1)
+int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 {
 	struct ref_lock *lock;
-	int err, i, ret = 0, flag = 0;
+	int err, i = 0, ret = 0, flag = 0;
 
 	lock = lock_ref_sha1_basic(refname, sha1, 0, &flag);
 	if (!lock)
 		return 1;
 	if (!(flag & REF_ISPACKED)) {
 		/* loose */
-		i = strlen(lock->lk->filename) - 5; /* .lock */
-		lock->lk->filename[i] = 0;
-		err = unlink(lock->lk->filename);
+		const char *path;
+
+		if (!(delopt & REF_NODEREF)) {
+			i = strlen(lock->lk->filename) - 5; /* .lock */
+			lock->lk->filename[i] = 0;
+			path = lock->lk->filename;
+		} else {
+			path = git_path(refname);
+		}
+		err = unlink(path);
 		if (err && errno != ENOENT) {
 			ret = 1;
 			error("unlink(%s) failed: %s",
-			      lock->lk->filename, strerror(errno));
+			      path, strerror(errno));
 		}
-		lock->lk->filename[i] = '.';
+		if (!(delopt & REF_NODEREF))
+			lock->lk->filename[i] = '.';
 	}
 	/* removing the loose one could have resurrected an earlier
 	 * packed one.  Also, if it was not loose we need to repack
@@ -964,11 +972,16 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	struct ref_lock *lock;
 	struct stat loginfo;
 	int log = !lstat(git_path("logs/%s", oldref), &loginfo);
+	const char *symref = NULL;
 
 	if (S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldref);
 
-	if (!resolve_ref(oldref, orig_sha1, 1, &flag))
+	symref = resolve_ref(oldref, orig_sha1, 1, &flag);
+	if (flag & REF_ISSYMREF)
+		return error("refname %s is a symbolic ref, renaming it is not supported",
+				oldref);
+	if (!symref)
 		return error("refname %s not found", oldref);
 
 	if (!is_refname_available(newref, oldref, get_packed_refs(), 0))
@@ -988,12 +1001,12 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 		return error("unable to move logfile logs/%s to tmp-renamed-log: %s",
 			oldref, strerror(errno));
 
-	if (delete_ref(oldref, orig_sha1)) {
+	if (delete_ref(oldref, orig_sha1, 0)) {
 		error("unable to delete old %s", oldref);
 		goto rollback;
 	}
 
-	if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1)) {
+	if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1, 0)) {
 		if (errno==EISDIR) {
 			if (remove_empty_directories(git_path("%s", newref))) {
 				error("Directory not empty: %s", newref);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 2147eac..20b6c18 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -112,6 +112,15 @@ test_expect_success 'config information was renamed, too' \
 	"test $(git config branch.s.dummy) = Hello &&
 	 test_must_fail git config branch.s/s/dummy"
 
+test_expect_success 'renaming a symref should fail' \
+'
+	git symbolic-ref refs/heads/master2 refs/heads/master &&
+	test_must_fail git branch -m master2 master3 &&
+	git symbolic-ref refs/heads/master2 &&
+	test -f .git/refs/heads/master &&
+	! test -f .git/refs/heads/master3
+'
+
 test_expect_success \
     'git branch -m u v should fail when the reflog for u is a symlink' '
      git branch -l u &&
-- 
1.6.0.2

[PATCH 2/3] rename_ref(): handle the case when the reflog of a ref does not exist

[Miklos Vajna]

We tried to check if a reflog of a ref is a symlink without first
checking if it exists, which is a bug.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 refs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/refs.c b/refs.c
index 31a2057..116eb59 100644
--- a/refs.c
+++ b/refs.c
@@ -974,7 +974,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	int log = !lstat(git_path("logs/%s", oldref), &loginfo);
 	const char *symref = NULL;
 
-	if (S_ISLNK(loginfo.st_mode))
+	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldref);
 
 	symref = resolve_ref(oldref, orig_sha1, 1, &flag);
-- 
1.6.0.2

[PATCH 3/3] Fix git update-ref --no-deref -d.

[Miklos Vajna]

Till now --no-deref was just ignored when deleting refs, fix this.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-update-ref.c  |    8 +++++---
 t/t1400-update-ref.sh |    7 +++++++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index d8f3142..378dc1b 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -13,7 +13,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 {
 	const char *refname, *oldval, *msg=NULL;
 	unsigned char sha1[20], oldsha1[20];
-	int delete = 0, no_deref = 0;
+	int delete = 0, no_deref = 0, flags = 0;
 	struct option options[] = {
 		OPT_STRING( 'm', NULL, &msg, "reason", "reason of the update"),
 		OPT_BOOLEAN('d', NULL, &delete, "deletes the reference"),
@@ -47,9 +47,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	if (oldval && *oldval && get_sha1(oldval, oldsha1))
 		die("%s: not a valid old SHA1", oldval);
 
+	if (no_deref)
+		flags = REF_NODEREF;
 	if (delete)
-		return delete_ref(refname, oldval ? oldsha1 : NULL, 0);
+		return delete_ref(refname, oldval ? oldsha1 : NULL, flags);
 	else
 		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
-				  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);
+				  flags, DIE_ON_ERR);
 }
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 04c2b16..8139cd6 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -75,6 +75,13 @@ test_expect_success "delete $m (by HEAD)" '
 '
 rm -f .git/$m
 
+cp -f .git/HEAD .git/HEAD.orig
+test_expect_success "delete symref without dereference" '
+	git update-ref --no-deref -d HEAD &&
+	! test -f .git/HEAD
+'
+cp -f .git/HEAD.orig .git/HEAD
+
 test_expect_success '(not) create HEAD with old sha1' "
 	test_must_fail git update-ref HEAD $A $B
 "
-- 
1.6.0.2

Re: [PATCH 0/3] symref rename/delete fixes

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

On Mon, Oct 27, 2008 at 08:50:19PM +0100, Miklos Vajna <vmiklos@frugalware.org> wrote:
> Here they are. The first is updated: the delete_ref() part is unchanged,
> but the rename_ref() part is dropped and it just bails out with an error
> if it detects a symref. The testcase is updated according to this as
> well.
> 
> The two others are just rebased on top of the new first one, no
> functional changes.

I forgot to fetch and did not notice the previous round is already in
'next'. Just forget about these patches then, I'll rebase the relevant
part against mv/maint-branch-m-symref and resend.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] git branch -m: forbid renaming of a symref

[Miklos Vajna]

There may be cases where one would really want to rename the symbolic
ref without changing its value, but "git branch -m" is not such a
use-case.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

This applies on top of 'mv/maint-branch-m-symref'.

 refs.c            |   29 +++++++++++++----------------
 t/t3200-branch.sh |    8 ++++----
 2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/refs.c b/refs.c
index b39e6f2..8a38e08 100644
--- a/refs.c
+++ b/refs.c
@@ -964,14 +964,14 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	struct stat loginfo;
 	int log = !lstat(git_path("logs/%s", oldref), &loginfo);
 	const char *symref = NULL;
-	int is_symref = 0;
 
 	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldref);
 
 	symref = resolve_ref(oldref, orig_sha1, 1, &flag);
 	if (flag & REF_ISSYMREF)
-		is_symref = 1;
+		return error("refname %s is a symbolic ref, renaming it is not supported",
+			oldref);
 	if (!symref)
 		return error("refname %s not found", oldref);
 
@@ -1035,20 +1035,17 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	}
 	logmoved = log;
 
-	if (!is_symref) {
-		lock = lock_ref_sha1_basic(newref, NULL, 0, NULL);
-		if (!lock) {
-			error("unable to lock %s for update", newref);
-			goto rollback;
-		}
-		lock->force_write = 1;
-		hashcpy(lock->old_sha1, orig_sha1);
-		if (write_ref_sha1(lock, orig_sha1, logmsg)) {
-			error("unable to write current sha1 into %s", newref);
-			goto rollback;
-		}
-	} else
-		create_symref(newref, symref, logmsg);
+	lock = lock_ref_sha1_basic(newref, NULL, 0, NULL);
+	if (!lock) {
+		error("unable to lock %s for update", newref);
+		goto rollback;
+	}
+	lock->force_write = 1;
+	hashcpy(lock->old_sha1, orig_sha1);
+	if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+		error("unable to write current sha1 into %s", newref);
+		goto rollback;
+	}
 
 	return 0;
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index fdeb1f5..25e9971 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -112,13 +112,13 @@ test_expect_success 'config information was renamed, too' \
 	"test $(git config branch.s.dummy) = Hello &&
 	 test_must_fail git config branch.s/s/dummy"
 
-test_expect_success 'renaming a symref' \
+test_expect_success 'renaming a symref is not allowed' \
 '
 	git symbolic-ref refs/heads/master2 refs/heads/master &&
-	git branch -m master2 master3 &&
-	git symbolic-ref refs/heads/master3 &&
+	test_must_fail git branch -m master2 master3 &&
+	git symbolic-ref refs/heads/master2 &&
 	test -f .git/refs/heads/master &&
-	! test -f .git/refs/heads/master2
+	! test -f .git/refs/heads/master3
 '
 
 test_expect_success \
-- 
1.6.0.2

[PATCH] Implement git remote rename

[Miklos Vajna]

The new rename subcommand does the followings:

1) Renames the remote.foo configuration section to remote.bar

2) Updates the remote.bar.fetch refspecs

3) Updates the branch.*.remote settings

4) Renames the tracking branches: renames the normal refs and rewrites
   the symrefs to point to the new refs.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Fri, Oct 24, 2008 at 04:33:28PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Hmm, remote_get() can read from all three supported places that you
> can define remotes.  Could you explain what happens if the old remote
> is read from say $GIT_DIR/remotes/origin and you are renaming it to
> "upstream" with "git remote rename origin upstream"?

Currently it dies because it can't rename config section 'remote.origin'
to 'remote.upstream'.

> I suspect that if you record where you read the configuration from in
> "struct remote" and add necessary code to remove the original when
> rename.old is *not* coming from in-config definition, you would make
> it possible for repositories initialized with older git that has
> either $GIT_DIR/branches/origin or $GIT_DIR/remotes/origin to be
> migrated to the in-config format using "git remote rename origin
> origin".

Yes, that's possible. I think the patch is already large enough that it
would be better to do it as a separate patch, but I like the idea.

One important step is that currently we have to care about the
remote.upstream.fetch variable, but once migration is supported, we have
to pay attention to remote.upstream.url/push as well.

In the meantime here is an updated patch which applies on top of
mv/maint-branch-m-symref. However, it is not meant for 'maint', of
course. :)

The old version did not "rename" (in fact it is not just a rename but it
replaces origin with upstream in the contents) symrefs properly, now the
testcases are extended to check for this, too.

 Documentation/git-remote.txt |    6 ++
 builtin-remote.c             |  153 ++++++++++++++++++++++++++++++++++++++++++
 t/t5505-remote.sh            |   15 ++++
 3 files changed, 174 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index bb99810..7b227b3 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 [verse]
 'git remote' [-v | --verbose]
 'git remote add' [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>
+'git remote rename' <old> <new>
 'git remote rm' <name>
 'git remote show' [-n] <name>
 'git remote prune' [-n | --dry-run] <name>
@@ -61,6 +62,11 @@ only makes sense in bare repositories.  If a remote uses mirror
 mode, furthermore, `git push` will always behave as if `\--mirror`
 was passed.
 
+'rename'::
+
+Rename the remote named <old> to <new>. All remote tracking branches and
+configuration settings for the remote are updated.
+
 'rm'::
 
 Remove the remote named <name>. All remote tracking branches and
diff --git a/builtin-remote.c b/builtin-remote.c
index e396a3a..1ca6cdb 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -10,6 +10,7 @@
 static const char * const builtin_remote_usage[] = {
 	"git remote",
 	"git remote add <name> <url>",
+	"git remote rename <old> <new>",
 	"git remote rm <name>",
 	"git remote show <name>",
 	"git remote prune <name>",
@@ -329,6 +330,156 @@ static int add_branch_for_removal(const char *refname,
 	return 0;
 }
 
+struct rename_info {
+	const char *old;
+	const char *new;
+	struct string_list *remote_branches;
+};
+
+static int read_remote_branches(const char *refname,
+	const unsigned char *sha1, int flags, void *cb_data)
+{
+	struct rename_info *rename = cb_data;
+	struct strbuf buf = STRBUF_INIT;
+	struct string_list_item *item;
+	int flag;
+	unsigned char orig_sha1[20];
+	const char *symref;
+
+	strbuf_addf(&buf, "refs/remotes/%s", rename->old);
+	if(!prefixcmp(refname, buf.buf)) {
+		item = string_list_append(xstrdup(refname), rename->remote_branches);
+		symref = resolve_ref(refname, orig_sha1, 1, &flag);
+		if (flag & REF_ISSYMREF)
+			item->util = xstrdup(symref);
+		else
+			item->util = NULL;
+	}
+
+	return 0;
+}
+
+static int mv(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+	struct remote *oldremote, *newremote;
+	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT;
+	struct string_list remote_branches = { NULL, 0, 0, 0 };
+	struct rename_info rename;
+	int i;
+
+	if (argc != 3)
+		usage_with_options(builtin_remote_usage, options);
+
+	rename.old = argv[1];
+	rename.new = argv[2];
+	rename.remote_branches = &remote_branches;
+
+	oldremote = remote_get(rename.old);
+	if (!oldremote)
+		die("No such remote: %s", rename.old);
+
+	newremote = remote_get(rename.new);
+	if (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))
+		die("remote %s already exists.", rename.new);
+
+	strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
+	if (!valid_fetch_refspec(buf.buf))
+		die("'%s' is not a valid remote name", rename.new);
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "remote.%s", rename.old);
+	strbuf_addf(&buf2, "remote.%s", rename.new);
+	if (git_config_rename_section(buf.buf, buf2.buf) < 1)
+		return error("Could not rename config section '%s' to '%s'",
+				buf.buf, buf2.buf);
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "remote.%s.fetch", rename.new);
+	if (git_config_set_multivar(buf.buf, NULL, NULL, 1))
+		return error("Could not remove config section '%s'", buf.buf);
+	for (i = 0; i < oldremote->fetch_refspec_nr; i++) {
+		char *ptr;
+
+		strbuf_reset(&buf2);
+		strbuf_addstr(&buf2, oldremote->fetch_refspec[i]);
+		ptr = strstr(buf2.buf, rename.old);
+		if (ptr)
+			strbuf_splice(&buf2, ptr-buf2.buf, strlen(rename.old),
+					rename.new, strlen(rename.new));
+		if (git_config_set_multivar(buf.buf, buf2.buf, "^$", 0))
+			return error("Could not append '%s'", buf.buf);
+	}
+
+	read_branches();
+	for (i = 0; i < branch_list.nr; i++) {
+		struct string_list_item *item = branch_list.items + i;
+		struct branch_info *info = item->util;
+		if (info->remote && !strcmp(info->remote, rename.old)) {
+			strbuf_reset(&buf);
+			strbuf_addf(&buf, "branch.%s.remote", item->string);
+			if (git_config_set(buf.buf, rename.new)) {
+				return error("Could not set '%s'", buf.buf);
+			}
+		}
+	}
+
+	/*
+	 * First remove symrefs, then rename the rest, finally create
+	 * the new symrefs.
+	 */
+	for_each_ref(read_remote_branches, &rename);
+	for (i = 0; i < remote_branches.nr; i++) {
+		struct string_list_item *item = remote_branches.items + i;
+		int flag = 0;
+		unsigned char sha1[20];
+		const char *symref;
+
+		symref = resolve_ref(item->string, sha1, 1, &flag);
+		if (!(flag & REF_ISSYMREF))
+			continue;
+		if (delete_ref(item->string, NULL, REF_NODEREF))
+			die("deleting '%s' failed", item->string);
+	}
+	for (i = 0; i < remote_branches.nr; i++) {
+		struct string_list_item *item = remote_branches.items + i;
+
+		if (item->util)
+			continue;
+		strbuf_reset(&buf);
+		strbuf_addstr(&buf, item->string);
+		strbuf_splice(&buf, strlen("refs/remotes/"), strlen(rename.old),
+				rename.new, strlen(rename.new));
+		strbuf_reset(&buf2);
+		strbuf_addf(&buf2, "remote: renamed %s to %s",
+				item->string, buf.buf);
+		if (rename_ref(item->string, buf.buf, buf2.buf))
+			die("renaming '%s' failed", item->string);
+	}
+	for (i = 0; i < remote_branches.nr; i++) {
+		struct string_list_item *item = remote_branches.items + i;
+
+		if (!item->util)
+			continue;
+		strbuf_reset(&buf);
+		strbuf_addstr(&buf, item->string);
+		strbuf_splice(&buf, strlen("refs/remotes/"), strlen(rename.old),
+				rename.new, strlen(rename.new));
+		strbuf_reset(&buf2);
+		strbuf_addstr(&buf2, item->util);
+		strbuf_splice(&buf2, strlen("refs/remotes/"), strlen(rename.old),
+				rename.new, strlen(rename.new));
+		strbuf_reset(&buf3);
+		strbuf_addf(&buf3, "remote: renamed %s to %s",
+				item->string, buf.buf);
+		if (create_symref(buf.buf, buf2.buf, buf3.buf))
+			die("creating '%s' failed", buf.buf);
+	}
+	return 0;
+}
+
 static int remove_branches(struct string_list *branches)
 {
 	int i, result = 0;
@@ -695,6 +846,8 @@ int cmd_remote(int argc, const char **argv, const char *prefix)
 		result = show_all();
 	else if (!strcmp(argv[0], "add"))
 		result = add(argc, argv);
+	else if (!strcmp(argv[0], "rename"))
+		result = mv(argc, argv);
 	else if (!strcmp(argv[0], "rm"))
 		result = rm(argc, argv);
 	else if (!strcmp(argv[0], "show"))
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index c4380c7..0c956ba 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -328,4 +328,19 @@ test_expect_success 'reject adding remote with an invalid name' '
 
 '
 
+# The first three test if the tracking branches are properly renamed,
+# the last two ones check if the config is updated.
+
+test_expect_success 'rename a remote' '
+
+	git clone one four &&
+	(cd four &&
+	 git remote rename origin upstream &&
+	 rmdir .git/refs/remotes/origin &&
+	 test "$(git symbolic-ref refs/remotes/upstream/HEAD)" = "refs/remotes/upstream/master" &&
+	 test "$(git rev-parse upstream/master)" = "$(git rev-parse master)" &&
+	 test "$(git config remote.upstream.fetch)" = "+refs/heads/*:refs/remotes/upstream/*" &&
+	 test "$(git config branch.master.remote)" = "upstream")
+
+'
 test_done
-- 
1.6.0.2

Re: [PATCH] Implement git remote rename

[Miklos Vajna]

On Fri, Oct 24, 2008 at 04:33:28PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> I suspect that if you record where you read the configuration from in
> "struct remote" and add necessary code to remove the original when
> rename.old is *not* coming from in-config definition, you would make
> it possible for repositories initialized with older git that has
> either $GIT_DIR/branches/origin or $GIT_DIR/remotes/origin to be
> migrated to the in-config format using "git remote rename origin
> origin".

Here are 4 patches to implement this + add the related
testcases/documentation.

Miklos Vajna (4):
  remote: add a new 'origin' variable to the struct
  git-remote rename: support remotes->config migration
  git-remote rename: support branches->config migration
  git-remote: document the migration feature of the rename subcommand

 Documentation/git-remote.txt |    4 ++++
 builtin-remote.c             |   35 +++++++++++++++++++++++++++++++++++
 remote.c                     |    3 +++
 remote.h                     |    7 +++++++
 t/t5505-remote.sh            |   33 +++++++++++++++++++++++++++++++++
 5 files changed, 82 insertions(+), 0 deletions(-)

[PATCH 1/4] remote: add a new 'origin' variable to the struct

[Miklos Vajna]

This allows one to track where was the remote's original source, so that
it's possible to decide if it makes sense to migrate it to the config
format or not.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 remote.c |    3 +++
 remote.h |    7 +++++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/remote.c b/remote.c
index e530a21..cbb3e48 100644
--- a/remote.c
+++ b/remote.c
@@ -201,6 +201,7 @@ static void read_remotes_file(struct remote *remote)
 
 	if (!f)
 		return;
+	remote->origin = REMOTE_REMOTES;
 	while (fgets(buffer, BUF_SIZE, f)) {
 		int value_list;
 		char *s, *p;
@@ -261,6 +262,7 @@ static void read_branches_file(struct remote *remote)
 		s++;
 	if (!*s)
 		return;
+	remote->origin = REMOTE_BRANCHES;
 	p = s + strlen(s);
 	while (isspace(p[-1]))
 		*--p = 0;
@@ -350,6 +352,7 @@ static int handle_config(const char *key, const char *value, void *cb)
 	if (!subkey)
 		return error("Config with no key for remote %s", name);
 	remote = make_remote(name, subkey - name);
+	remote->origin = REMOTE_CONFIG;
 	if (!strcmp(subkey, ".mirror"))
 		remote->mirror = git_config_bool(key, value);
 	else if (!strcmp(subkey, ".skipdefaultupdate"))
diff --git a/remote.h b/remote.h
index d2e170c..a46a5be 100644
--- a/remote.h
+++ b/remote.h
@@ -1,8 +1,15 @@
 #ifndef REMOTE_H
 #define REMOTE_H
 
+enum {
+	REMOTE_CONFIG,
+	REMOTE_REMOTES,
+	REMOTE_BRANCHES
+};
+
 struct remote {
 	const char *name;
+	int origin;
 
 	const char **url;
 	int url_nr;
-- 
1.6.0.2

[PATCH 2/4] git-remote rename: support remotes->config migration

[Miklos Vajna]

This patch makes it possible to migrate a remote stored in a
$GIT_DIR/remotes/nick file to the configuration file format.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-remote.c  |   33 +++++++++++++++++++++++++++++++++
 t/t5505-remote.sh |   21 +++++++++++++++++++++
 2 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 1ca6cdb..d9d0ba3 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -359,6 +359,36 @@ static int read_remote_branches(const char *refname,
 	return 0;
 }
 
+static int migrate_file(struct remote *remote)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int i;
+	char *path = NULL;
+
+	strbuf_addf(&buf, "remote.%s.url", remote->name);
+	for (i = 0; i < remote->url_nr; i++)
+		if (git_config_set_multivar(buf.buf, remote->url[i], "^$", 0))
+			return error("Could not append '%s' to '%s'",
+					remote->url[i], buf.buf);
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "remote.%s.push", remote->name);
+	for (i = 0; i < remote->push_refspec_nr; i++)
+		if (git_config_set_multivar(buf.buf, remote->push_refspec[i], "^$", 0))
+			return error("Could not append '%s' to '%s'",
+					remote->push_refspec[i], buf.buf);
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "remote.%s.fetch", remote->name);
+	for (i = 0; i < remote->fetch_refspec_nr; i++)
+		if (git_config_set_multivar(buf.buf, remote->fetch_refspec[i], "^$", 0))
+			return error("Could not append '%s' to '%s'",
+					remote->fetch_refspec[i], buf.buf);
+	if (remote->origin == REMOTE_REMOTES)
+		path = git_path("remotes/%s", remote->name);
+	if (path && unlink(path))
+		warning("failed to remove '%s'", path);
+	return 0;
+}
+
 static int mv(int argc, const char **argv)
 {
 	struct option options[] = {
@@ -381,6 +411,9 @@ static int mv(int argc, const char **argv)
 	if (!oldremote)
 		die("No such remote: %s", rename.old);
 
+	if (!strcmp(rename.old, rename.new) && oldremote->origin != REMOTE_CONFIG)
+		return migrate_file(oldremote);
+
 	newremote = remote_get(rename.new);
 	if (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))
 		die("remote %s already exists.", rename.new);
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 0c956ba..1567631 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -343,4 +343,25 @@ test_expect_success 'rename a remote' '
 	 test "$(git config branch.master.remote)" = "upstream")
 
 '
+
+cat > remotes_origin << EOF
+URL: $(pwd)/one
+Push: refs/heads/master:refs/heads/upstream
+Pull: refs/heads/master:refs/heads/origin
+EOF
+
+test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
+	git clone one five &&
+	origin_url=$(pwd)/one &&
+	(cd five &&
+	 git remote rm origin &&
+	 mkdir -p .git/remotes &&
+	 cat ../remotes_origin > .git/remotes/origin &&
+	 git remote rename origin origin &&
+	 ! test -f .git/remotes/origin &&
+	 test "$(git config remote.origin.url)" = "$origin_url" &&
+	 test "$(git config remote.origin.push)" = "refs/heads/master:refs/heads/upstream" &&
+	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
+'
+
 test_done
-- 
1.6.0.2

[PATCH 3/4] git-remote rename: support branches->config migration

[Miklos Vajna]

This is similar to the remotes->config one, but it makes the
branches->config conversion possible.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-remote.c  |    2 ++
 t/t5505-remote.sh |   12 ++++++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index d9d0ba3..3af1876 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -384,6 +384,8 @@ static int migrate_file(struct remote *remote)
 					remote->fetch_refspec[i], buf.buf);
 	if (remote->origin == REMOTE_REMOTES)
 		path = git_path("remotes/%s", remote->name);
+	else if (remote->origin == REMOTE_BRANCHES)
+		path = git_path("branches/%s", remote->name);
 	if (path && unlink(path))
 		warning("failed to remove '%s'", path);
 	return 0;
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 1567631..1f59960 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -364,4 +364,16 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
 	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
 '
 
+test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
+	git clone one six &&
+	origin_url=$(pwd)/one &&
+	(cd six &&
+	 git remote rm origin &&
+	 echo "$origin_url" > .git/branches/origin &&
+	 git remote rename origin origin &&
+	 ! test -f .git/branches/origin &&
+	 test "$(git config remote.origin.url)" = "$origin_url" &&
+	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
+'
+
 test_done
-- 
1.6.0.2

[PATCH 4/4] git-remote: document the migration feature of the rename subcommand

[Miklos Vajna]

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 Documentation/git-remote.txt |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 7b227b3..fad983e 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -66,6 +66,10 @@ was passed.
 
 Rename the remote named <old> to <new>. All remote tracking branches and
 configuration settings for the remote are updated.
++
+In case <old> and <new> are the same, and <old> is a file under
+`$GIT_DIR/remotes` or `$GIT_DIR/branches`, the remote is converted to
+the configuration file format.
 
 'rm'::
 
-- 
1.6.0.2

Re: [PATCH 3/4] git-remote rename: support branches->config migration

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> This is similar to the remotes->config one, but it makes the
> branches->config conversion possible.
>
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> ---
>  builtin-remote.c  |    2 ++
>  t/t5505-remote.sh |   12 ++++++++++++
>  2 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/builtin-remote.c b/builtin-remote.c
> index d9d0ba3..3af1876 100644
> --- a/builtin-remote.c
> +++ b/builtin-remote.c
> @@ -384,6 +384,8 @@ static int migrate_file(struct remote *remote)
>  					remote->fetch_refspec[i], buf.buf);
>  	if (remote->origin == REMOTE_REMOTES)
>  		path = git_path("remotes/%s", remote->name);
> +	else if (remote->origin == REMOTE_BRANCHES)
> +		path = git_path("branches/%s", remote->name);

There is something fishy going on between 2/4 and 3/4.  2/4 was advertised
to migrate remotes to config and had a call to migrate_file() for that
purpose.  Here this one now allows to convert branches but there is no
change to the callsite of migrate_file().

Which would mean that 2/4 would convert branches/foo too.  And this one is
only to remove the leftover branches/foo file.

Or am I utterly confused?

Re: [PATCH 3/4] git-remote rename: support branches->config migration

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

On Tue, Nov 11, 2008 at 04:49:14PM -0800, Junio C Hamano <gitster@pobox.com> wrote:
> There is something fishy going on between 2/4 and 3/4.  2/4 was advertised
> to migrate remotes to config and had a call to migrate_file() for that
> purpose.  Here this one now allows to convert branches but there is no
> change to the callsite of migrate_file().
> 
> Which would mean that 2/4 would convert branches/foo too.  And this one is
> only to remove the leftover branches/foo file.
> 
> Or am I utterly confused?

The trick is that 2/4 already added support for remotes/foo as it uses
remote_get() and that detects remotes/foo as well, but that is
completely unintentional. If you think it makes sense, just squash 3/4
to 2/4, I made it two separate patches because I think these are
logically separate changes.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 3/4] git-remote rename: support branches->config migration

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> On Tue, Nov 11, 2008 at 04:49:14PM -0800, Junio C Hamano <gitster@pobox.com> wrote:
>> There is something fishy going on between 2/4 and 3/4.  2/4 was advertised
>> to migrate remotes to config and had a call to migrate_file() for that
>> purpose.  Here this one now allows to convert branches but there is no
>> change to the callsite of migrate_file().
>> 
>> Which would mean that 2/4 would convert branches/foo too.  And this one is
>> only to remove the leftover branches/foo file.
>> 
>> Or am I utterly confused?
>
> The trick is that 2/4 already added support for remotes/foo as it uses
> remote_get() and that detects remotes/foo as well, but that is
> completely unintentional.

That is not a trick; it merely is a broken code.

The function migrate_file() introduced by [2/4] is called for any remote
definition that did not come from config (by definition, it either came
from remotes/foo or branches/foo).  The function adds the entries for the
given remote definition to the config file, and then removes remotes/foo
file if the remote definition came from it.  So it is a logically
consistent change if you only called this function only for remote
definitions that came from remotes/foo.

But the function is called for a remote definition that originally came
from branches/foo as well.  It happily adds the definition to the config,
even though it *fails to remove* branches/foo file.

Do you still think 2/4 is a logically contained good change?

If you apply this to 5505 (taken from 3/4, but removing the check for
branches/origin file), and look at resulting t/trash*/six/.git/config
file, you will see you have already migrated the remote definition to the
config.

diff --git i/t/t5505-remote.sh w/t/t5505-remote.sh
index 1567631..60bb9e5 100755
--- i/t/t5505-remote.sh
+++ w/t/t5505-remote.sh
@@ -364,4 +364,15 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
 	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
 '
 
-test_done
+test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
+	git clone one six &&
+	origin_url=$(pwd)/one &&
+	(cd six &&
+	 git remote rm origin &&
+	 echo "$origin_url" > .git/branches/origin &&
+	 git remote rename origin origin &&
+	 test "$(git config remote.origin.url)" = "$origin_url" &&
+	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
+'
+
+: test_done

[PATCH v2 0/4] git-remote rename: support branches->config migration

[Miklos Vajna]

On Tue, Nov 11, 2008 at 08:22:38PM -0800, Junio C Hamano <gitster@pobox.com> wrote:
> The function migrate_file() introduced by [2/4] is called for any
> remote definition that did not come from config (by definition, it
> either came from remotes/foo or branches/foo).  The function adds the
> entries for the given remote definition to the config file, and then
> removes remotes/foo file if the remote definition came from it.  So it
> is a logically consistent change if you only called this function only
> for remote definitions that came from remotes/foo.
>
> But the function is called for a remote definition that originally
> came from branches/foo as well.  It happily adds the definition to the
> config, even though it *fails to remove* branches/foo file.
>
> Do you still think 2/4 is a logically contained good change?

OK, here is an updated series, which is supposed to fix this issue. 1/4
and 4/4 is unchanged.

Miklos Vajna (4):
  remote: add a new 'origin' variable to the struct
  git-remote rename: support remotes->config migration
  git-remote rename: support branches->config migration
  git-remote: document the migration feature of the rename subcommand

 Documentation/git-remote.txt |    4 ++++
 builtin-remote.c             |   37 +++++++++++++++++++++++++++++++++++++
 remote.c                     |    3 +++
 remote.h                     |    7 +++++++
 t/t5505-remote.sh            |   33 +++++++++++++++++++++++++++++++++
 5 files changed, 84 insertions(+), 0 deletions(-)

[PATCH 1/4] remote: add a new 'origin' variable to the struct

[Miklos Vajna]

This allows one to track where was the remote's original source, so that
it's possible to decide if it makes sense to migrate it to the config
format or not.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 remote.c |    3 +++
 remote.h |    7 +++++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/remote.c b/remote.c
index e530a21..cbb3e48 100644
--- a/remote.c
+++ b/remote.c
@@ -201,6 +201,7 @@ static void read_remotes_file(struct remote *remote)
 
 	if (!f)
 		return;
+	remote->origin = REMOTE_REMOTES;
 	while (fgets(buffer, BUF_SIZE, f)) {
 		int value_list;
 		char *s, *p;
@@ -261,6 +262,7 @@ static void read_branches_file(struct remote *remote)
 		s++;
 	if (!*s)
 		return;
+	remote->origin = REMOTE_BRANCHES;
 	p = s + strlen(s);
 	while (isspace(p[-1]))
 		*--p = 0;
@@ -350,6 +352,7 @@ static int handle_config(const char *key, const char *value, void *cb)
 	if (!subkey)
 		return error("Config with no key for remote %s", name);
 	remote = make_remote(name, subkey - name);
+	remote->origin = REMOTE_CONFIG;
 	if (!strcmp(subkey, ".mirror"))
 		remote->mirror = git_config_bool(key, value);
 	else if (!strcmp(subkey, ".skipdefaultupdate"))
diff --git a/remote.h b/remote.h
index d2e170c..a46a5be 100644
--- a/remote.h
+++ b/remote.h
@@ -1,8 +1,15 @@
 #ifndef REMOTE_H
 #define REMOTE_H
 
+enum {
+	REMOTE_CONFIG,
+	REMOTE_REMOTES,
+	REMOTE_BRANCHES
+};
+
 struct remote {
 	const char *name;
+	int origin;
 
 	const char **url;
 	int url_nr;
-- 
1.6.0.2

[PATCH 2/4] git-remote rename: support remotes->config migration

[Miklos Vajna]

This patch makes it possible to migrate a remote stored in a
$GIT_DIR/remotes/nick file to the configuration file format.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-remote.c  |   33 +++++++++++++++++++++++++++++++++
 t/t5505-remote.sh |   21 +++++++++++++++++++++
 2 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 1ca6cdb..4fa64a2 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -359,6 +359,36 @@ static int read_remote_branches(const char *refname,
 	return 0;
 }
 
+static int migrate_file(struct remote *remote)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int i;
+	char *path = NULL;
+
+	strbuf_addf(&buf, "remote.%s.url", remote->name);
+	for (i = 0; i < remote->url_nr; i++)
+		if (git_config_set_multivar(buf.buf, remote->url[i], "^$", 0))
+			return error("Could not append '%s' to '%s'",
+					remote->url[i], buf.buf);
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "remote.%s.push", remote->name);
+	for (i = 0; i < remote->push_refspec_nr; i++)
+		if (git_config_set_multivar(buf.buf, remote->push_refspec[i], "^$", 0))
+			return error("Could not append '%s' to '%s'",
+					remote->push_refspec[i], buf.buf);
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "remote.%s.fetch", remote->name);
+	for (i = 0; i < remote->fetch_refspec_nr; i++)
+		if (git_config_set_multivar(buf.buf, remote->fetch_refspec[i], "^$", 0))
+			return error("Could not append '%s' to '%s'",
+					remote->fetch_refspec[i], buf.buf);
+	if (remote->origin == REMOTE_REMOTES)
+		path = git_path("remotes/%s", remote->name);
+	if (path && unlink(path))
+		warning("failed to remove '%s'", path);
+	return 0;
+}
+
 static int mv(int argc, const char **argv)
 {
 	struct option options[] = {
@@ -381,6 +411,9 @@ static int mv(int argc, const char **argv)
 	if (!oldremote)
 		die("No such remote: %s", rename.old);
 
+	if (!strcmp(rename.old, rename.new) && oldremote->origin == REMOTE_REMOTES)
+		return migrate_file(oldremote);
+
 	newremote = remote_get(rename.new);
 	if (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))
 		die("remote %s already exists.", rename.new);
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 0c956ba..1567631 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -343,4 +343,25 @@ test_expect_success 'rename a remote' '
 	 test "$(git config branch.master.remote)" = "upstream")
 
 '
+
+cat > remotes_origin << EOF
+URL: $(pwd)/one
+Push: refs/heads/master:refs/heads/upstream
+Pull: refs/heads/master:refs/heads/origin
+EOF
+
+test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
+	git clone one five &&
+	origin_url=$(pwd)/one &&
+	(cd five &&
+	 git remote rm origin &&
+	 mkdir -p .git/remotes &&
+	 cat ../remotes_origin > .git/remotes/origin &&
+	 git remote rename origin origin &&
+	 ! test -f .git/remotes/origin &&
+	 test "$(git config remote.origin.url)" = "$origin_url" &&
+	 test "$(git config remote.origin.push)" = "refs/heads/master:refs/heads/upstream" &&
+	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
+'
+
 test_done
-- 
1.6.0.2

[PATCH 3/4] git-remote rename: support branches->config migration

[Miklos Vajna]

This is similar to the remotes->config one, but it makes the
branches->config conversion possible.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-remote.c  |    6 +++++-
 t/t5505-remote.sh |   12 ++++++++++++
 2 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 4fa64a2..5b525c7 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -384,6 +384,8 @@ static int migrate_file(struct remote *remote)
 					remote->fetch_refspec[i], buf.buf);
 	if (remote->origin == REMOTE_REMOTES)
 		path = git_path("remotes/%s", remote->name);
+	else if (remote->origin == REMOTE_BRANCHES)
+		path = git_path("branches/%s", remote->name);
 	if (path && unlink(path))
 		warning("failed to remove '%s'", path);
 	return 0;
@@ -411,7 +413,9 @@ static int mv(int argc, const char **argv)
 	if (!oldremote)
 		die("No such remote: %s", rename.old);
 
-	if (!strcmp(rename.old, rename.new) && oldremote->origin == REMOTE_REMOTES)
+	if (!strcmp(rename.old, rename.new) &&
+			(oldremote->origin == REMOTE_REMOTES ||
+			 oldremote->origin == REMOTE_BRANCHES))
 		return migrate_file(oldremote);
 
 	newremote = remote_get(rename.new);
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 1567631..1f59960 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -364,4 +364,16 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
 	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
 '
 
+test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
+	git clone one six &&
+	origin_url=$(pwd)/one &&
+	(cd six &&
+	 git remote rm origin &&
+	 echo "$origin_url" > .git/branches/origin &&
+	 git remote rename origin origin &&
+	 ! test -f .git/branches/origin &&
+	 test "$(git config remote.origin.url)" = "$origin_url" &&
+	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
+'
+
 test_done
-- 
1.6.0.2

[PATCH 4/4] git-remote: document the migration feature of the rename subcommand

[Miklos Vajna]

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 Documentation/git-remote.txt |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 7b227b3..fad983e 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -66,6 +66,10 @@ was passed.
 
 Rename the remote named <old> to <new>. All remote tracking branches and
 configuration settings for the remote are updated.
++
+In case <old> and <new> are the same, and <old> is a file under
+`$GIT_DIR/remotes` or `$GIT_DIR/branches`, the remote is converted to
+the configuration file format.
 
 'rm'::
 
-- 
1.6.0.2