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.
