All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] builtin/remote.c: show progress when renaming remote references
@ 2022-03-01 22:20 Taylor Blau
  2022-03-02 14:32 ` Derrick Stolee
                   ` (4 more replies)
  0 siblings, 5 replies; 23+ messages in thread
From: Taylor Blau @ 2022-03-01 22:20 UTC (permalink / raw)
  To: git; +Cc: sandals, gitster

When renaming a remote, Git needs to rename all remote tracking
references to the remote's new name (e.g., renaming
"refs/remotes/old/foo" to "refs/remotes/new/foo" when renaming a remote
from "old" to "new").

This can be somewhat slow when there are many references to rename,
since each rename is done in a separate call to rename_ref() as opposed
to grouping all renames together into the same transaction. It would be
nice to execute all renames as a single transaction, but there is a
snag: the reference transaction backend doesn't support renames during a
transaction (only individually, via rename_ref()).

The reasons there are described in more detail in [1], but the main
problem is that in order to preserve the existing reflog, it must be
moved while holding both locks (i.e., on "oldname" and "newname"), and
the ref transaction code doesn't support inserting arbitrary actions
into the middle of a transaction like that.

As an aside, adding support for this to the ref transaction code is
less straightforward than inserting both a ref_update() and ref_delete()
call into the same transaction. rename_ref()'s special handling to
detect D/F conflicts would need to be rewritten for the transaction code
if we wanted to proactively catch D/F conflicts when renaming a
reference during a transaction. The reftable backend could support this
much more readily because of its lack of D/F conflicts.

Instead of a more complex modification to the ref transaction code,
display a progress meter when running verbosely in order to convince the
user that Git is doing work while renaming a remote.

This is mostly done as-expected, with the minor caveat that we
intentionally count symrefs renames twice, since renaming a symref takes
place over two separate calls (one to delete the old one, and another to
create the new one).

[1]: https://lore.kernel.org/git/572367B4.4050207@alum.mit.edu/

Suggested-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
A small patch to improve the user experience of `git remote -v rename`
with a large number of remote tracking references, in lieu of a more
complicated changes to the reference transaction code.

 Documentation/git-remote.txt |  2 +-
 builtin/remote.c             | 26 ++++++++++++++++++++++----
 t/t5505-remote.sh            |  3 ++-
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 2bebc32566..626f9d0afc 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git remote' [-v | --verbose]
 'git remote add' [-t <branch>] [-m <master>] [-f] [--[no-]tags] [--mirror=(fetch|push)] <name> <URL>
-'git remote rename' <old> <new>
+'git remote' [-v | --verbose] 'rename' <old> <new>
 'git remote remove' <name>
 'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)
 'git remote set-branches' [--add] <name> <branch>...
diff --git a/builtin/remote.c b/builtin/remote.c
index 6f27ddc47b..a18b8532e2 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -12,11 +12,12 @@
 #include "object-store.h"
 #include "strvec.h"
 #include "commit-reach.h"
+#include "progress.h"

 static const char * const builtin_remote_usage[] = {
 	"git remote [-v | --verbose]",
 	N_("git remote add [-t <branch>] [-m <master>] [-f] [--tags | --no-tags] [--mirror=<fetch|push>] <name> <url>"),
-	N_("git remote rename <old> <new>"),
+	N_("git remote [-v | --verbose] rename <old> <new>"),
 	N_("git remote remove <name>"),
 	N_("git remote set-head <name> (-a | --auto | -d | --delete | <branch>)"),
 	N_("git remote [-v | --verbose] show [-n] <name>"),
@@ -571,6 +572,7 @@ struct rename_info {
 	const char *old_name;
 	const char *new_name;
 	struct string_list *remote_branches;
+	uint32_t symrefs_nr;
 };

 static int read_remote_branches(const char *refname,
@@ -587,10 +589,12 @@ static int read_remote_branches(const char *refname,
 		item = string_list_append(rename->remote_branches, refname);
 		symref = resolve_ref_unsafe(refname, RESOLVE_REF_READING,
 					    NULL, &flag);
-		if (symref && (flag & REF_ISSYMREF))
+		if (symref && (flag & REF_ISSYMREF)) {
 			item->util = xstrdup(symref);
-		else
+			rename->symrefs_nr++;
+		} else {
 			item->util = NULL;
+		}
 	}
 	strbuf_release(&buf);

@@ -682,7 +686,8 @@ static int mv(int argc, const char **argv)
 		old_remote_context = STRBUF_INIT;
 	struct string_list remote_branches = STRING_LIST_INIT_DUP;
 	struct rename_info rename;
-	int i, refspec_updated = 0;
+	int i, j = 0, refspec_updated = 0;
+	struct progress *progress = NULL;

 	if (argc != 3)
 		usage_with_options(builtin_remote_rename_usage, options);
@@ -690,6 +695,7 @@ static int mv(int argc, const char **argv)
 	rename.old_name = argv[1];
 	rename.new_name = argv[2];
 	rename.remote_branches = &remote_branches;
+	rename.symrefs_nr = 0;

 	oldremote = remote_get(rename.old_name);
 	if (!remote_is_configured(oldremote, 1)) {
@@ -764,6 +770,14 @@ static int mv(int argc, const char **argv)
 	 * the new symrefs.
 	 */
 	for_each_ref(read_remote_branches, &rename);
+	if (verbose) {
+		/*
+		 * Count symrefs twice, since "renaming" them is done by
+		 * deleting and recreating them in two separate passes.
+		 */
+		progress = start_progress(_("Renaming remote references"),
+					  rename.remote_branches->nr + rename.symrefs_nr);
+	}
 	for (i = 0; i < remote_branches.nr; i++) {
 		struct string_list_item *item = remote_branches.items + i;
 		int flag = 0;
@@ -773,6 +787,7 @@ static int mv(int argc, const char **argv)
 			continue;
 		if (delete_ref(NULL, item->string, NULL, REF_NO_DEREF))
 			die(_("deleting '%s' failed"), item->string);
+		display_progress(progress, ++j);
 	}
 	for (i = 0; i < remote_branches.nr; i++) {
 		struct string_list_item *item = remote_branches.items + i;
@@ -788,6 +803,7 @@ static int mv(int argc, const char **argv)
 				item->string, buf.buf);
 		if (rename_ref(item->string, buf.buf, buf2.buf))
 			die(_("renaming '%s' failed"), item->string);
+		display_progress(progress, ++j);
 	}
 	for (i = 0; i < remote_branches.nr; i++) {
 		struct string_list_item *item = remote_branches.items + i;
@@ -807,7 +823,9 @@ static int mv(int argc, const char **argv)
 				item->string, buf.buf);
 		if (create_symref(buf.buf, buf2.buf, buf3.buf))
 			die(_("creating '%s' failed"), buf.buf);
+		display_progress(progress, ++j);
 	}
+	stop_progress(&progress);
 	string_list_clear(&remote_branches, 1);

 	handle_push_default(rename.old_name, rename.new_name);
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 9ab315424c..c4b76485e0 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -753,8 +753,9 @@ test_expect_success 'rename a remote' '
 	(
 		cd four &&
 		git config branch.main.pushRemote origin &&
-		git remote rename origin upstream &&
+		GIT_PROGRESS_DELAY=0 git remote -v rename origin upstream 2>err &&
 		grep "pushRemote" .git/config &&
+		grep "Renaming remote references: 100% (4/4), done" err &&
 		test -z "$(git for-each-ref refs/remotes/origin)" &&
 		test "$(git symbolic-ref refs/remotes/upstream/HEAD)" = "refs/remotes/upstream/main" &&
 		test "$(git rev-parse upstream/main)" = "$(git rev-parse main)" &&
--
2.35.1.73.gccc5557600

^ permalink raw reply related	[flat|nested] 23+ messages in thread
end of thread, other threads:[~2022-03-07 11:12 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-01 22:20 [PATCH] builtin/remote.c: show progress when renaming remote references Taylor Blau
2022-03-02 14:32 ` Derrick Stolee
2022-03-02 15:52   ` Taylor Blau
2022-03-02 18:58     ` Derrick Stolee
2022-03-02 19:03     ` Junio C Hamano
2022-03-02 19:00 ` Ævar Arnfjörð Bjarmason
2022-03-02 22:55   ` Taylor Blau
2022-03-03 10:51     ` Ævar Arnfjörð Bjarmason
2022-03-03 19:54       ` Taylor Blau
2022-03-07 10:34       ` Han-Wen Nienhuys
2022-03-02 22:21 ` brian m. carlson
2022-03-02 22:57   ` Taylor Blau
2022-03-03 16:09     ` Derrick Stolee
2022-03-03 19:58       ` Taylor Blau
2022-03-02 23:00 ` [PATCH v2] " Taylor Blau
2022-03-03 11:04   ` Ævar Arnfjörð Bjarmason
2022-03-03 22:25 ` [PATCH v3 0/2] remote: show progress display when renaming Taylor Blau
2022-03-03 22:25   ` [PATCH v3 1/2] builtin/remote.c: parse options in 'rename' Taylor Blau
2022-03-05 14:28     ` Ævar Arnfjörð Bjarmason
2022-03-03 22:25   ` [PATCH v3 2/2] builtin/remote.c: show progress when renaming remote references Taylor Blau
2022-03-03 23:20     ` Junio C Hamano
2022-03-03 23:30       ` Taylor Blau
2022-03-05 14:31     ` Ævar Arnfjörð Bjarmason

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.