| Message ID | d5b0a4b71027619123b7284611692d3a9c128518.1646346287.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 56710a7ae0170404d7bc6411bd5c9a18124c0629 |
| Headers | show |
| Series | remote: show progress display when renaming | expand |
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
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
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.
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" &&
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(-)