Re: [PATCH v3 2/2] builtin/remote.c: show progress when renaming remote references
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 5, 2022, 14:31 UTC
- Message-ID
- <220305.86pmn030ou.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <d5b0a4b71027619123b7284611692d3a9c128518.1646346287.git.me@ttaylorr.com>
On Thu, Mar 03 2022, Taylor Blau wrote:
Show 13 quoted lines
> 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/
Show 6 quoted lines
> @@ -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...
Show 16 quoted lines
>
> 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);
Show 6 quoted lines
> @@ -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...)
Show 21 quoted lines
> + 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....
Show 11 quoted lines
> + 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.