Re: [PATCH v3 2/2] builtin/remote.c: show progress when renaming remote references
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-03-05 14:59:18
On Thu, Mar 03 2022, Taylor Blau wrote:
quoted hunk ↗ jump to hunk
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/
quoted hunk ↗ jump to hunk
@@ -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...
quoted hunk ↗ jump to hunk
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);
quoted hunk ↗ jump to hunk
@@ -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...)
quoted hunk ↗ jump to hunk
+ 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....
quoted hunk ↗ jump to hunk
+ 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.