Re: [PATCH v3 2/2] builtin/remote.c: show progress when renaming remote references

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.