[PATCH] builtin/remote.c: show progress when renaming remote references

[PATCH] builtin/remote.c: show progress when renaming remote references

[Taylor Blau]

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

Re: [PATCH] builtin/remote.c: show progress when renaming remote references

[Derrick Stolee]

On 3/1/2022 5:20 PM, Taylor Blau wrote:
> 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.

Thanks for this patch. It improves the user experience through
useful feedback.

> @@ -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;

My only complaint is that 'j' is not informative enough here.

'j' as a loop iterator is good, but we aren't looping "on" j,
but instead tracking a progress_count across multiple loops.

Thanks,
-Stolee

Re: [PATCH] builtin/remote.c: show progress when renaming remote references

[Taylor Blau]

On Wed, Mar 02, 2022 at 09:32:48AM -0500, Derrick Stolee wrote:
> > 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.
>
> Thanks for this patch. It improves the user experience through
> useful feedback.

Admittedly, it feels like a little bit of a shortcut to avoid modifying
the ref transaction code, but I think it's an OK short-term solution.
Thanks for reviewing.

> > @@ -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;
>
> My only complaint is that 'j' is not informative enough here.
>
> 'j' as a loop iterator is good, but we aren't looping "on" j,
> but instead tracking a progress_count across multiple loops.

How about s/j/refs_renamed_nr ?

Thanks,
Taylor

Re: [PATCH] builtin/remote.c: show progress when renaming remote references

[Derrick Stolee]

On 3/2/2022 10:52 AM, Taylor Blau wrote:
> On Wed, Mar 02, 2022 at 09:32:48AM -0500, Derrick Stolee wrote:
>>> 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.
>>
>> Thanks for this patch. It improves the user experience through
>> useful feedback.
> 
> Admittedly, it feels like a little bit of a shortcut to avoid modifying
> the ref transaction code, but I think it's an OK short-term solution.
> Thanks for reviewing.

Absolutely. Help the user first. Think about the harder fix on
another timescale.

>>> @@ -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;
>>
>> My only complaint is that 'j' is not informative enough here.
>>
>> 'j' as a loop iterator is good, but we aren't looping "on" j,
>> but instead tracking a progress_count across multiple loops.
> 
> How about s/j/refs_renamed_nr ?

Even better!

Thanks,
-Stolee

Re: [PATCH] builtin/remote.c: show progress when renaming remote references

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 01 2022, Taylor Blau wrote:

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

I realize that you're just copying an existing pattern within
builtin/reflog.c here, but we should really call this "--no-progress",
not "-v" or "--verbose".

I did that in the last 3 patches here, which I was waiting on
re-submitting:
https://lore.kernel.org/git/cover-00.12-00000000000-20211130T213319Z-avarab@gmail.com/

I.e. the whole reason we use --verbose here is entirely historical, it
used to (well, still is without those patches) very verbose line-by-line
output.

Whereas for other things we turn progress on by default, and --verbose
is something very different.

So please have this by "int progress = 1", and have a "PARSE_OPT_NONEG"
"no-progress" option instead, there's no reason we need to propagate the
existing UX mistake in "reflog expire".

[I reversed the order you wrote the following, due to the obvious
digression...] :)

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

As an aside I think the reftable code "emulates" the D/F conflicts of
the files backend, but I'm not sure (this is from vague memory).

And I've run into the same limitations you're describing here with the
ref transactions code. I tried to transaction-ize the "branch rename"
the other day, there we copy an existing reflog in its entirety.

It would be really nice if you could plug things into the
transaction. It would have to be things that could support the same
do-stuff/abort-stuff semantics, e.g. being able to create a file with
arbitrary contents, then unlink() it if we're rolling back.

But in any case, all of that is a much larger change than is really
required here, so I (also) digress :)

As an even further aside: If we had a slight "twist" on the ref backend
where we were able to store the reflogs next to the actual ref files,
e.g.:

    .git/refs/heads/master
    .git/refs/heads/.master.reflog

Which would work since that '.master.reflog' name isn't a valid refname
(it contains "."), couldn't we safely do this whole operation on a POSIX
FS with a simple:

    mv .git/refs/remotes/origin .git/refs/remotes/newname

I.e. even if you had concurrent *.lock files in play from other
transactions having those would mean that we had a file descriptor open
to file in question, and a file being renamed underneath you doesn't
change your ability to write to that open file.

Although I guess we rename() them into place, and if that target
moves..., so we'd need to pre-open them.

Hrm.

Anyway, that's an even larger digression & can of worms, but something I
wondered about when I saw this take a LOOOONG time in the past, "why
doesn't it just rename the folders?".

Re: [PATCH] builtin/remote.c: show progress when renaming remote references

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> On Wed, Mar 02, 2022 at 09:32:48AM -0500, Derrick Stolee wrote:
>> > 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.
>>
>> Thanks for this patch. It improves the user experience through
>> useful feedback.
>
> Admittedly, it feels like a little bit of a shortcut to avoid modifying
> the ref transaction code, but I think it's an OK short-term solution.
> Thanks for reviewing.
>
>> > @@ -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;
>>
>> My only complaint is that 'j' is not informative enough here.
>>
>> 'j' as a loop iterator is good, but we aren't looping "on" j,
>> but instead tracking a progress_count across multiple loops.
>
> How about s/j/refs_renamed_nr ?

Meaning "number of renamed refs"?  start_progress() got the number
of remote branches to be renamed (i.e. rename.remote_branches->nr,
with symref adjustments), and approaching that ceiling from 0 by
counting the renamed ones so far makes sense.

Re: [PATCH] builtin/remote.c: show progress when renaming remote references

[brian m. carlson]

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

On 2022-03-01 at 22:20:33, Taylor Blau wrote:
> 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>

As I mentioned to you personally, I think this looks good.

For context, I discovered this when I tried to rename a remote with tens
of thousands of branches and it just ran silently for an extended period
of time without any output.  I actually interrupted it with Ctrl-C
because I thought it had hung, so I'm hoping this will provide a better
experience for users in that situation.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH] builtin/remote.c: show progress when renaming remote references

[Taylor Blau]

On Wed, Mar 02, 2022 at 08:00:59PM +0100, Ævar Arnfjörð Bjarmason wrote:
> So please have this by "int progress = 1", and have a "PARSE_OPT_NONEG"
> "no-progress" option instead, there's no reason we need to propagate the
> existing UX mistake in "reflog expire".

Yes, I agree that `git remote` is unlike other commands where you have
to opt _in_ to see progress, not out.

But I deliberately avoided doing anything about it here, since this
patch is more about making an existing set of arguments (`git remote -v
rename`) do something more useful than before, and not adding a new
option.

In other words, I felt that because you could already run:

    $ git remote -v rename old new

that it was better to punt on any changes to the option itself until
later.

On a similar note, it would be nice if this option worked on either side
of the sub-command, like how you can do either of:

    $ git multi-pack-index --object-dir=... write
    $ git multi-pack-index write --object-dir=...

But I don't think we should let perfect be the enemy of the good here,
in case you were suggesting delaying this patch until we sort that issue
out.

> [I reversed the order you wrote the following, due to the obvious
> digression...] :)

;-), thanks.

> As an aside I think the reftable code "emulates" the D/F conflicts of
> the files backend, but I'm not sure (this is from vague memory).

Perhaps, though I'm admittedly not familiar enough with that work to
tell know for sure, either. I don't think I have a ton to add to the
lower part of your reply, so I'll stop here.

Thanks,
Taylor

Re: [PATCH] builtin/remote.c: show progress when renaming remote references

[Taylor Blau]

On Wed, Mar 02, 2022 at 10:21:16PM +0000, brian m. carlson wrote:
> For context, I discovered this when I tried to rename a remote with tens
> of thousands of branches and it just ran silently for an extended period
> of time without any output.  I actually interrupted it with Ctrl-C
> because I thought it had hung, so I'm hoping this will provide a better
> experience for users in that situation.

Thanks again for pointing it out to me. To be honest, I'm skeptical that
this patch alone will improve things much, since you still have to pass
the '-v' flag to see the new progress meter.

But perhaps users who suspect the command is hung will re-run it with
the '-v' flag instinctually and get more helpful output. I'll look at
making `git remote`'s `-v` behave a little more like `--[no-]progress'
in another series.

Thanks,
Taylor

[PATCH v2] builtin/remote.c: show progress when renaming remote references

[Taylor Blau]

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 reroll which renames the variable used to keep track of our
progress from "j" to "refs_renamed_nr" for clarity.

 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..00668af5c9 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, refs_renamed_nr = 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, ++refs_renamed_nr);
 	}
 	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, ++refs_renamed_nr);
 	}
 	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, ++refs_renamed_nr);
 	}
+	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

Re: [PATCH] builtin/remote.c: show progress when renaming remote references

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 02 2022, Taylor Blau wrote:

> On Wed, Mar 02, 2022 at 08:00:59PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> So please have this by "int progress = 1", and have a "PARSE_OPT_NONEG"
>> "no-progress" option instead, there's no reason we need to propagate the
>> existing UX mistake in "reflog expire".
>
> Yes, I agree that `git remote` is unlike other commands where you have
> to opt _in_ to see progress, not out.
>
> But I deliberately avoided doing anything about it here, since this
> patch is more about making an existing set of arguments (`git remote -v
> rename`) do something more useful than before, and not adding a new
> option.
>
> In other words, I felt that because you could already run:
>
>     $ git remote -v rename old new
>
> that it was better to punt on any changes to the option itself until
> later.

So first, I must say that I was just entirely confused when I wrote
this. I managed to somehow read this code and also mentally mix up that
you were patching git-remote and not git-reflog, hence linking to those
"reflog" patches.

By way of explanation I think that's because I started thinking "oh
yeah, it's that one special case where --verbose shows progress..." :)

But in any case, for *this patch* I think that also means that using
--verbose here makes even less sense, because --verbose for "git-remote"
is:

    Be a little more verbose and show remote url after name.
    NOTE: This must be placed between remote and subcommand.

Although reading it over it seems that was written for "git remote -v
[show]", but we don't entirely hold to it already, but in any case let's
not also conflate it with what should be a "--no-progress" here.

> On a similar note, it would be nice if this option worked on either side
> of the sub-command, like how you can do either of:
>
>     $ git multi-pack-index --object-dir=... write
>     $ git multi-pack-index write --object-dir=...
>
> But I don't think we should let perfect be the enemy of the good here,
> in case you were suggesting delaying this patch until we sort that issue
> out.

I think per [1] and your [2] we agreed to move forward in not adding any
more of these subcommand-level options. I.e. I was arguing in [1] that
it might be a good idea for e.g. --no-progress or --verbose, but the
consensus was to go for your [2] of declaring that it's always the
subcommand that takes the option.

(Which as an aside, I'm fine with, and have come more around to being
the Way It Should Be since then).

So just doing:
	
	diff --git a/builtin/remote.c b/builtin/remote.c
	index 6f27ddc47bd..047bcda57c5 100644
	--- a/builtin/remote.c
	+++ b/builtin/remote.c
	@@ -674,7 +674,9 @@ static void handle_push_default(const char* old_name, const char* new_name)
	 
	 static int mv(int argc, const char **argv)
	 {
	+	int progress = 1;
	 	struct option options[] = {
	+		OPT_BOOL(0, "progress", &progress, N_("show progress")),
	 		OPT_END()
	 	};
	 	struct remote *oldremote, *newremote;

Would be consistent with [2] and not mix up --verbose and its (probably
somewhat inaccurate already) documented promises with this new output,
and improve the UX by turning it on by default.

1. https://lore.kernel.org/git/87zgsad6mn.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/cover.1631980949.git.me@ttaylorr.com/

>> [I reversed the order you wrote the following, due to the obvious
>> digression...] :)
>
> ;-), thanks.
>
>> As an aside I think the reftable code "emulates" the D/F conflicts of
>> the files backend, but I'm not sure (this is from vague memory).
>
> Perhaps, though I'm admittedly not familiar enough with that work to
> tell know for sure, either. I don't think I have a ton to add to the
> lower part of your reply, so I'll stop here.

:)

Re: [PATCH v2] builtin/remote.c: show progress when renaming remote references

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 02 2022, Taylor Blau wrote:


> @@ -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 &&

Just on this part. You can use some variant of "test_region" to do this with trace2:

    git grep -C10 TRACE.*progress -- t

Which as an improvement also tests that you called stop_progress(),
i.e. that the trace2 region is ended.

>  		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)" &&

Re: [PATCH] builtin/remote.c: show progress when renaming remote references

[Derrick Stolee]

On 3/2/2022 5:57 PM, Taylor Blau wrote:
> On Wed, Mar 02, 2022 at 10:21:16PM +0000, brian m. carlson wrote:
>> For context, I discovered this when I tried to rename a remote with tens
>> of thousands of branches and it just ran silently for an extended period
>> of time without any output.  I actually interrupted it with Ctrl-C
>> because I thought it had hung, so I'm hoping this will provide a better
>> experience for users in that situation.
> 
> Thanks again for pointing it out to me. To be honest, I'm skeptical that
> this patch alone will improve things much, since you still have to pass
> the '-v' flag to see the new progress meter.
> 
> But perhaps users who suspect the command is hung will re-run it with
> the '-v' flag instinctually and get more helpful output. I'll look at
> making `git remote`'s `-v` behave a little more like `--[no-]progress'
> in another series.

I guess one big reason for Ævar's suggestion about using --[no-]progress
as the signal for progress is that we can make --progress the default
when isatty(2) is true. We should not do the same for --verbose.

Looking at other examples, I see that 'fsck' has --verbose imply
--no-progress, probably because the verbose output would write lines
that become interleaved with the progress indicators (and those lines
act as progress in themselves). Not sure if that's the right choice in
this case, too.

Thanks,
-Stolee

Re: [PATCH] builtin/remote.c: show progress when renaming remote references

[Taylor Blau]

On Thu, Mar 03, 2022 at 11:51:08AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Mar 02 2022, Taylor Blau wrote:
>
> > On Wed, Mar 02, 2022 at 08:00:59PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> So please have this by "int progress = 1", and have a "PARSE_OPT_NONEG"
> >> "no-progress" option instead, there's no reason we need to propagate the
> >> existing UX mistake in "reflog expire".
> >
> > Yes, I agree that `git remote` is unlike other commands where you have
> > to opt _in_ to see progress, not out.
> >
> > But I deliberately avoided doing anything about it here, since this
> > patch is more about making an existing set of arguments (`git remote -v
> > rename`) do something more useful than before, and not adding a new
> > option.
> >
> > In other words, I felt that because you could already run:
> >
> >     $ git remote -v rename old new
> >
> > that it was better to punt on any changes to the option itself until
> > later.
>
> So first, I must say that I was just entirely confused when I wrote
> this. I managed to somehow read this code and also mentally mix up that
> you were patching git-remote and not git-reflog, hence linking to those
> "reflog" patches.

No problem, your response below jogged my memory on the "should
`--[no-]progress` be accepted on either side of a sub-command or not?"
discussion we had a while ago.

> But in any case, for *this patch* I think that also means that using
> --verbose here makes even less sense, because --verbose for "git-remote"
> is:
>
>     Be a little more verbose and show remote url after name.
>     NOTE: This must be placed between remote and subcommand.
>
> Although reading it over it seems that was written for "git remote -v
> [show]", but we don't entirely hold to it already, but in any case let's
> not also conflate it with what should be a "--no-progress" here.

Thank you for pointing that out. I agree that between the documentation
(which I agree is over-fitted for `git remote -v`) and my (admittedly,
foggy!) memory of our earlier discussions, I think that `git remote
rename --[no-progress]` is the right way to go here.

> So just doing:
>
> 	diff --git a/builtin/remote.c b/builtin/remote.c
> 	index 6f27ddc47bd..047bcda57c5 100644
> 	--- a/builtin/remote.c
> 	+++ b/builtin/remote.c
> 	@@ -674,7 +674,9 @@ static void handle_push_default(const char* old_name, const char* new_name)
>
> 	 static int mv(int argc, const char **argv)
> 	 {
> 	+	int progress = 1;
> 	 	struct option options[] = {
> 	+		OPT_BOOL(0, "progress", &progress, N_("show progress")),
> 	 		OPT_END()
> 	 	};
> 	 	struct remote *oldremote, *newremote;
>
> Would be consistent with [2] and not mix up --verbose and its (probably
> somewhat inaccurate already) documented promises with this new output,
> and improve the UX by turning it on by default.
>
> 1. https://lore.kernel.org/git/87zgsad6mn.fsf@evledraar.gmail.com/
> 2. https://lore.kernel.org/git/cover.1631980949.git.me@ttaylorr.com/

Agreed, thanks!

Thanks,
Taylor

Re: [PATCH] builtin/remote.c: show progress when renaming remote references

[Taylor Blau]

On Thu, Mar 03, 2022 at 11:09:20AM -0500, Derrick Stolee wrote:
> I guess one big reason for Ævar's suggestion about using --[no-]progress
> as the signal for progress is that we can make --progress the default
> when isatty(2) is true. We should not do the same for --verbose.

Yep, agreed.

> Looking at other examples, I see that 'fsck' has --verbose imply
> --no-progress, probably because the verbose output would write lines
> that become interleaved with the progress indicators (and those lines
> act as progress in themselves). Not sure if that's the right choice in
> this case, too.

I think for `git remote rename` it won't matter, since it doesn't do
anything with the `-v` option anyway (so there's no output to clobber in
the first place).

Thanks,
Taylor

[PATCH v3 0/2] remote: show progress display when renaming

[Taylor Blau]

Here is a reroll of the patch we've been discussing to add progress
output when renaming remote-tracking references (as part of a `git
remote rename <old> <new>`).

This single patch is now two (and hence, has a cover letter!) because of
a slight change in approach which amounts to changing this behavior from
an opt-in

    git remote -v rename <old> <new>

to something much more in line with our existing use of the
`--[no-]progress` option like:

    git remote rename [--[no-]progress] <old> <new>

(where `--progress` is the default exactly when `isatty(2)` is true).

I think similar treatment could be applied to other `git remote`
sub-commands, but I'm reasonably happy starting with just `git remote
rename` and looking at the others later.

Taylor Blau (2):
  builtin/remote.c: parse options in 'rename'
  builtin/remote.c: show progress when renaming remote references

 Documentation/git-remote.txt |  2 +-
 builtin/remote.c             | 39 ++++++++++++++++++++++++++++--------
 t/t5505-remote.sh            |  4 +++-
 3 files changed, 35 insertions(+), 10 deletions(-)

Range-diff against v2:
-:  ---------- > 1:  b76da50b54 builtin/remote.c: parse options in 'rename'
1:  dc63ec91ab ! 2:  d5b0a4b710 builtin/remote.c: show progress when renaming remote references
    @@ Documentation/git-remote.txt: SYNOPSIS
      '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 rename' [--[no-]progress] <old> <new>
      'git remote remove' <name>
      'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)
      'git remote set-branches' [--add] <name> <branch>...
    @@ builtin/remote.c
      	"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 rename [--[no-]progress] <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>"),
    +@@ builtin/remote.c: static const char * const builtin_remote_add_usage[] = {
    + };
    + 
    + static const char * const builtin_remote_rename_usage[] = {
    +-	N_("git remote rename <old> <new>"),
    ++	N_("git remote rename [--[no-]progress] <old> <new>"),
    + 	NULL
    + };
    + 
     @@ builtin/remote.c: struct rename_info {
      	const char *old_name;
      	const char *new_name;
    @@ builtin/remote.c: static int read_remote_branches(const char *refname,
      	}
      	strbuf_release(&buf);
      
    +@@ builtin/remote.c: static void handle_push_default(const char* old_name, const char* new_name)
    + 
    + static int mv(int argc, const char **argv)
    + {
    ++	int show_progress = isatty(2);
    + 	struct option options[] = {
    ++		OPT_BOOL(0, "progress", &show_progress, N_("force progress reporting")),
    + 		OPT_END()
    + 	};
    + 	struct remote *oldremote, *newremote;
     @@ builtin/remote.c: static int mv(int argc, const char **argv)
      		old_remote_context = STRBUF_INIT;
      	struct string_list remote_branches = STRING_LIST_INIT_DUP;
    @@ builtin/remote.c: static int mv(int argc, const char **argv)
     +	int i, refs_renamed_nr = 0, refspec_updated = 0;
     +	struct progress *progress = NULL;
      
    - 	if (argc != 3)
    - 		usage_with_options(builtin_remote_rename_usage, options);
    + 	argc = parse_options(argc, argv, NULL, options,
    + 			     builtin_remote_rename_usage, 0);
     @@ builtin/remote.c: static int mv(int argc, const char **argv)
    - 	rename.old_name = argv[1];
    - 	rename.new_name = argv[2];
    + 	rename.old_name = argv[0];
    + 	rename.new_name = argv[1];
      	rename.remote_branches = &remote_branches;
     +	rename.symrefs_nr = 0;
      
    @@ builtin/remote.c: static int mv(int argc, const char **argv)
      	 * the new symrefs.
      	 */
      	for_each_ref(read_remote_branches, &rename);
    -+	if (verbose) {
    ++	if (show_progress) {
     +		/*
     +		 * Count symrefs twice, since "renaming" them is done by
     +		 * deleting and recreating them in two separate passes.
    @@ t/t5505-remote.sh: 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 &&
    ++		GIT_TRACE2_EVENT=$(pwd)/trace \
    ++			git remote rename --progress origin upstream &&
    ++		test_region progress "Renaming remote references" trace &&
      		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

[PATCH v3 1/2] builtin/remote.c: parse options in 'rename'

[Taylor Blau]

The 'git remote rename' command doesn't currently take any command-line
arguments besides the existing and new name of a remote, and so has no
need to call parse_options().

But the subsequent patch will add a `--[no-]progress` option, in which
case we will need to call parse_options().

Do so now so as to avoid cluttering the following patch with noise, like
adjusting setting `rename.{old,new}_name` to argv[0] and argv[1], since
parse_options handles advancing argv past the name of the sub-command.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/remote.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 6f27ddc47b..824fb8099c 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -684,11 +684,14 @@ static int mv(int argc, const char **argv)
 	struct rename_info rename;
 	int i, refspec_updated = 0;
 
-	if (argc != 3)
+	argc = parse_options(argc, argv, NULL, options,
+			     builtin_remote_rename_usage, 0);
+
+	if (argc != 2)
 		usage_with_options(builtin_remote_rename_usage, options);
 
-	rename.old_name = argv[1];
-	rename.new_name = argv[2];
+	rename.old_name = argv[0];
+	rename.new_name = argv[1];
 	rename.remote_branches = &remote_branches;
 
 	oldremote = remote_get(rename.old_name);
-- 
2.35.1.73.gccc5557600

[PATCH v3 2/2] builtin/remote.c: show progress when renaming remote references

[Taylor Blau]

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>
---
 Documentation/git-remote.txt |  2 +-
 builtin/remote.c             | 30 +++++++++++++++++++++++++-----
 t/t5505-remote.sh            |  4 +++-
 3 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 2bebc32566..cde9614e36 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 rename' [--[no-]progress] <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 824fb8099c..950e7958d5 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 rename [--[no-]progress] <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>"),
@@ -36,7 +37,7 @@ static const char * const builtin_remote_add_usage[] = {
 };
 
 static const char * const builtin_remote_rename_usage[] = {
-	N_("git remote rename <old> <new>"),
+	N_("git remote rename [--[no-]progress] <old> <new>"),
 	NULL
 };
 
@@ -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);
 
@@ -674,7 +678,9 @@ static void handle_push_default(const char* old_name, const char* new_name)
 
 static int mv(int argc, const char **argv)
 {
+	int show_progress = isatty(2);
 	struct option options[] = {
+		OPT_BOOL(0, "progress", &show_progress, N_("force progress reporting")),
 		OPT_END()
 	};
 	struct remote *oldremote, *newremote;
@@ -682,7 +688,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, refs_renamed_nr = 0, refspec_updated = 0;
+	struct progress *progress = NULL;
 
 	argc = parse_options(argc, argv, NULL, options,
 			     builtin_remote_rename_usage, 0);
@@ -693,6 +700,7 @@ static int mv(int argc, const char **argv)
 	rename.old_name = argv[0];
 	rename.new_name = argv[1];
 	rename.remote_branches = &remote_branches;
+	rename.symrefs_nr = 0;
 
 	oldremote = remote_get(rename.old_name);
 	if (!remote_is_configured(oldremote, 1)) {
@@ -767,6 +775,14 @@ static int mv(int argc, const char **argv)
 	 * the new symrefs.
 	 */
 	for_each_ref(read_remote_branches, &rename);
+	if (show_progress) {
+		/*
+		 * 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;
@@ -776,6 +792,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, ++refs_renamed_nr);
 	}
 	for (i = 0; i < remote_branches.nr; i++) {
 		struct string_list_item *item = remote_branches.items + i;
@@ -791,6 +808,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, ++refs_renamed_nr);
 	}
 	for (i = 0; i < remote_branches.nr; i++) {
 		struct string_list_item *item = remote_branches.items + i;
@@ -810,7 +828,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, ++refs_renamed_nr);
 	}
+	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..c90cf47acd 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -753,7 +753,9 @@ test_expect_success 'rename a remote' '
 	(
 		cd four &&
 		git config branch.main.pushRemote origin &&
-		git remote rename origin upstream &&
+		GIT_TRACE2_EVENT=$(pwd)/trace \
+			git remote rename --progress origin upstream &&
+		test_region progress "Renaming remote references" trace &&
 		grep "pushRemote" .git/config &&
 		test -z "$(git for-each-ref refs/remotes/origin)" &&
 		test "$(git symbolic-ref refs/remotes/upstream/HEAD)" = "refs/remotes/upstream/main" &&
-- 
2.35.1.73.gccc5557600

Re: [PATCH v3 2/2] builtin/remote.c: show progress when renaming remote references

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

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

Is it still "when running verbosely"?

I thought that tying this to --[no-]progress was the whole point of
iterating another round.

	... when the standard error output is connected to a
	terminal, so that user knows Git is not completely stuck.

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

That's a nice note to leave here, as it is a bit tricky to reason about.

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



>  Documentation/git-remote.txt |  2 +-
>  builtin/remote.c             | 30 +++++++++++++++++++++++++-----
>  t/t5505-remote.sh            |  4 +++-
>  3 files changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
> index 2bebc32566..cde9614e36 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 rename' [--[no-]progress] <old> <new>
>  'git remote remove' <name>
>  'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)
>  'git remote set-branches' [--add] <name> <branch>...

Do we already have an entry for the --progress option in the
description part of the documentation?  I think the way progress
works in the context of this command is pretty much bog-standard
one that we may not have to, once the user has seen how progress
options work elsewhere.

If not, then we'd want something like this squashed in, perhaps?

 Documentation/git-remote.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git c/Documentation/git-remote.txt w/Documentation/git-remote.txt
index cde9614e36..e04810ee70 100644
--- c/Documentation/git-remote.txt
+++ w/Documentation/git-remote.txt
@@ -83,7 +83,10 @@ 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.
+configuration settings for the remote are updated.  When used interactively
+(i.e. the standard error stream is connected to a terminal),
+a progress bar may be shown while remote-tracking branches are renamed,
+which can be silenced by passing the `--no-progress` option.
 +
 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

Re: [PATCH v3 2/2] builtin/remote.c: show progress when renaming remote references

[Taylor Blau]

On Thu, Mar 03, 2022 at 03:20:48PM -0800, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > 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.
>
> Is it still "when running verbosely"?
>
> I thought that tying this to --[no-]progress was the whole point of
> iterating another round.
>
> 	... when the standard error output is connected to a
> 	terminal, so that user knows Git is not completely stuck.

Ah, I glossed over this (stale) reference to the verbose option. I'm
almost willing to let it go, since it doesn't mention `--verbose`
directly, but happy to change it, too.

> > 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).
>
> That's a nice note to leave here, as it is a bit tricky to reason about.

Thanks, it's the sort of thing that I'd hope to find in a commit message
if I were confused about something.

> Do we already have an entry for the --progress option in the
> description part of the documentation?  I think the way progress
> works in the context of this command is pretty much bog-standard
> one that we may not have to, once the user has seen how progress
> options work elsewhere.
>
> If not, then we'd want something like this squashed in, perhaps?

I like the suggestion, but I agree it's probably not necessary since
this usage is standard. I thought I had written something explaining the
option explicitly in this section, but apparently dropped it when
preparing this patch. Ugh.

Thanks,
Taylor

Re: [PATCH v3 1/2] builtin/remote.c: parse options in 'rename'

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 03 2022, Taylor Blau wrote:

> The 'git remote rename' command doesn't currently take any command-line
> arguments besides the existing and new name of a remote, and so has no
> need to call parse_options().

I think nothing needs to change here in the body of the test, but just a
correction: We do in fact use parse_options() here in the pre-image. I.e.:

    git remote -h
    git remote rename -h

I.e. we'll detect "-h", emit the appropriate usage etc.

Re: [PATCH v3 2/2] builtin/remote.c: show progress when renaming remote references

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 03 2022, Taylor Blau wrote:

> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
> index 2bebc32566..cde9614e36 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 rename' [--[no-]progress] <old> <new>
>  'git remote remove' <name>
>  'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)
>  'git remote set-branches' [--add] <name> <branch>...

Thanks, this looks much better.

But now that we don't piggy-back on --verbose (which as noted, would
have needed to be reworded if we still did) we should add a
--no-progress, --progress to the OPTIONS section, see e.g.:

    git grep '^--.*progress::' -- Documentation/

> @@ -571,6 +572,7 @@ struct rename_info {
>  	const char *old_name;
>  	const char *new_name;
>  	struct string_list *remote_branches;
> +	uint32_t symrefs_nr;
>  };

I didn't notice this in previous iterations, but why the uint32_t over
say a size_t?

The string_list is "unsigned int" (but we should really use size_t
there), but there's some code in refs.c that thinks about number of refs
as size_t already...

>  
>  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);

Just FWIW this could also be, if you prefer to skip the brace additions:
	
	@@ -588,9 +590,10 @@ static int read_remote_branches(const char *refname,
	 		symref = resolve_ref_unsafe(refname, RESOLVE_REF_READING,
	 					    NULL, &flag);
	 		if (symref && (flag & REF_ISSYMREF))
	-			item->util = xstrdup(symref);
	+			rename->symrefs_nr++;
	 		else
	-			item->util = NULL;
	+			symref = NULL;
	+		item->util = xstrdup_or_null(symref);
	 	}
	 	strbuf_release(&buf);

> @@ -682,7 +688,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, refs_renamed_nr = 0, refspec_updated = 0;

Another type mixup nit, refs_renamed_nr should be "size_t" (as above,
it's looping over the "unsigned int" string_list, but we can just use
size_t for future-proofing...)

> +	struct progress *progress = NULL;
>  
>  	argc = parse_options(argc, argv, NULL, options,
>  			     builtin_remote_rename_usage, 0);
> @@ -693,6 +700,7 @@ static int mv(int argc, const char **argv)
>  	rename.old_name = argv[0];
>  	rename.new_name = argv[1];
>  	rename.remote_branches = &remote_branches;
> +	rename.symrefs_nr = 0;
>  
>  	oldremote = remote_get(rename.old_name);
>  	if (!remote_is_configured(oldremote, 1)) {
> @@ -767,6 +775,14 @@ static int mv(int argc, const char **argv)
>  	 * the new symrefs.
>  	 */
>  	for_each_ref(read_remote_branches, &rename);
> +	if (show_progress) {
> +		/*
> +		 * Count symrefs twice, since "renaming" them is done by
> +		 * deleting and recreating them in two separate passes.
> +		 */

I didn't look this over all that carefully before, but is the count that
we'll get in rename.symrefs_nr ever different than in
resolve_ref_unsafe() in read_remote_branches()? If not that's an issue
that pre-exists here, i.e. why do we need to find out twice for each ref
it's a symref?

And in any case the "total" fed to start_progress() will be wrong since
in the two later loops we "continue" if "item->util" is true....

> +		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;
> @@ -776,6 +792,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, ++refs_renamed_nr);

...I think it makes sense to display_progress() at the start of the
loop, otherwise we expose ourselves to the progress bar stalling if
we're just looping over a bunch of stuff where we "continue", and it's
easier to reason about.

Re: [PATCH] builtin/remote.c: show progress when renaming remote references

[Han-Wen Nienhuys]

On Thu, Mar 3, 2022 at 12:04 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> >> As an aside I think the reftable code "emulates" the D/F conflicts of
> >> the files backend, but I'm not sure (this is from vague memory).
> >
> > Perhaps, though I'm admittedly not familiar enough with that work to
> > tell know for sure, either. I don't think I have a ton to add to the

correct. Look for the 'skip_name_check' option, and reftable/refname.c

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.