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

[Taylor Blau <me@ttaylorr.com>]

On Thu, Mar 03, 2022 at 11:51:08AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Mar 02 2022, Taylor Blau wrote:
>
> > On Wed, Mar 02, 2022 at 08:00:59PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> So please have this by "int progress = 1", and have a "PARSE_OPT_NONEG"
> >> "no-progress" option instead, there's no reason we need to propagate the
> >> existing UX mistake in "reflog expire".
> >
> > Yes, I agree that `git remote` is unlike other commands where you have
> > to opt _in_ to see progress, not out.
> >
> > But I deliberately avoided doing anything about it here, since this
> > patch is more about making an existing set of arguments (`git remote -v
> > rename`) do something more useful than before, and not adding a new
> > option.
> >
> > In other words, I felt that because you could already run:
> >
> >     $ git remote -v rename old new
> >
> > that it was better to punt on any changes to the option itself until
> > later.
>
> So first, I must say that I was just entirely confused when I wrote
> this. I managed to somehow read this code and also mentally mix up that
> you were patching git-remote and not git-reflog, hence linking to those
> "reflog" patches.

No problem, your response below jogged my memory on the "should
`--[no-]progress` be accepted on either side of a sub-command or not?"
discussion we had a while ago.

> But in any case, for *this patch* I think that also means that using
> --verbose here makes even less sense, because --verbose for "git-remote"
> is:
>
>     Be a little more verbose and show remote url after name.
>     NOTE: This must be placed between remote and subcommand.
>
> Although reading it over it seems that was written for "git remote -v
> [show]", but we don't entirely hold to it already, but in any case let's
> not also conflate it with what should be a "--no-progress" here.

Thank you for pointing that out. I agree that between the documentation
(which I agree is over-fitted for `git remote -v`) and my (admittedly,
foggy!) memory of our earlier discussions, I think that `git remote
rename --[no-progress]` is the right way to go here.

> So just doing:
>
> 	diff --git a/builtin/remote.c b/builtin/remote.c
> 	index 6f27ddc47bd..047bcda57c5 100644
> 	--- a/builtin/remote.c
> 	+++ b/builtin/remote.c
> 	@@ -674,7 +674,9 @@ static void handle_push_default(const char* old_name, const char* new_name)
>
> 	 static int mv(int argc, const char **argv)
> 	 {
> 	+	int progress = 1;
> 	 	struct option options[] = {
> 	+		OPT_BOOL(0, "progress", &progress, N_("show progress")),
> 	 		OPT_END()
> 	 	};
> 	 	struct remote *oldremote, *newremote;
>
> Would be consistent with [2] and not mix up --verbose and its (probably
> somewhat inaccurate already) documented promises with this new output,
> and improve the UX by turning it on by default.
>
> 1. https://lore.kernel.org/git/87zgsad6mn.fsf@evledraar.gmail.com/
> 2. https://lore.kernel.org/git/cover.1631980949.git.me@ttaylorr.com/

Agreed, thanks!

Thanks,
Taylor