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

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

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.

By way of explanation I think that's because I started thinking "oh
yeah, it's that one special case where --verbose shows progress..." :)

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.

> On a similar note, it would be nice if this option worked on either side
> of the sub-command, like how you can do either of:
>
>     $ git multi-pack-index --object-dir=... write
>     $ git multi-pack-index write --object-dir=...
>
> But I don't think we should let perfect be the enemy of the good here,
> in case you were suggesting delaying this patch until we sort that issue
> out.

I think per [1] and your [2] we agreed to move forward in not adding any
more of these subcommand-level options. I.e. I was arguing in [1] that
it might be a good idea for e.g. --no-progress or --verbose, but the
consensus was to go for your [2] of declaring that it's always the
subcommand that takes the option.

(Which as an aside, I'm fine with, and have come more around to being
the Way It Should Be since then).

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/

>> [I reversed the order you wrote the following, due to the obvious
>> digression...] :)
>
> ;-), thanks.
>
>> As an aside I think the reftable code "emulates" the D/F conflicts of
>> the files backend, but I'm not sure (this is from vague memory).
>
> Perhaps, though I'm admittedly not familiar enough with that work to
> tell know for sure, either. I don't think I have a ton to add to the
> lower part of your reply, so I'll stop here.

:)