patchbuiltin/remote.c: show progress when renaming remote references

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

On 3/1/2022 5:20 PM, Taylor Blau wrote:

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

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

On Wed, Mar 02, 2022 at 09:32:48AM -0500, Derrick Stolee wrote:

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.

How about s/j/refs_renamed_nr ?

Thanks, Taylor

On 3/2/2022 10:52 AM, Taylor Blau wrote:

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

Even better!

Thanks, -Stolee

Taylor Blau <me@ttaylorr.com> writes:

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.

On Tue, Mar 01 2022, Taylor Blau wrote:

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

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

On Wed, Mar 02, 2022 at 08:00:59PM +0100, Ævar Arnfjörð Bjarmason wrote:

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.

;-), thanks.

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

On Wed, Mar 02 2022, Taylor Blau wrote:

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.

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/

:)

On Thu, Mar 03, 2022 at 11:51:08AM +0100, Ævar Arnfjörð Bjarmason wrote:

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.

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.

Agreed, thanks!

Thanks, Taylor

On Thu, Mar 3, 2022 at 12:04 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

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

On 2022-03-01 at 22:20:33, Taylor Blau wrote:

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.

On Wed, Mar 02, 2022 at 10:21:16PM +0000, brian m. carlson wrote:

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

On 3/2/2022 5:57 PM, Taylor Blau 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.

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

On Thu, Mar 03, 2022 at 11:09:20AM -0500, Derrick Stolee wrote:

Yep, agreed.

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

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

On Wed, Mar 02 2022, Taylor Blau wrote:

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.

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

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

On Thu, Mar 03 2022, Taylor Blau wrote:

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.

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

Taylor Blau <me@ttaylorr.com> writes:

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.

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

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

On Thu, Mar 03, 2022 at 03:20:48PM -0800, Junio C Hamano wrote:

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.

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

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

On Thu, Mar 03 2022, Taylor Blau wrote:

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/

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

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

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

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

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