{"thread":{"id":"57492","subject":"[PATCH] builtin/remote.c: show progress when renaming remote references","startedAt":"2022-03-01T22:20:38Z","lastAt":"2022-03-07T11:12:52Z","messageCount":23,"participants":["Taylor Blau","Derrick Stolee","Junio C Hamano","Ævar Arnfjörð Bjarmason","brian m. carlson","Han-Wen Nienhuys"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"450026","messageId":"70a0325ca8ab0492a9b0873ee3fba576c5ab90b9.1646173186.git.me@ttaylorr.com","threadId":"57492","inReplyTo":null,"subject":"[PATCH] builtin/remote.c: show progress when renaming remote references","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-01T22:20:33Z","receivedAt":"2022-03-01T22:20:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When renaming a remote, Git needs to rename all remote tracking\nreferences to the remote's new name (e.g., renaming\n\"refs/remotes/old/foo\" to \"refs/remotes/new/foo\" when renaming a remote\nfrom \"old\" to \"new\").\n\nThis can be somewhat slow when there are many references to rename,\nsince each rename is done in a separate call to rename_ref() as opposed\nto grouping all renames together into the same transaction. It would be\nnice to execute all renames as a single transaction, but there is a\nsnag: the reference transaction backend doesn't support renames during a\ntransaction (only individually, via rename_ref()).\n\nThe reasons there are described in more detail in [1], but the main\nproblem is that in order to preserve the existing reflog, it must be\nmoved while holding both locks (i.e., on \"oldname\" and \"newname\"), and\nthe ref transaction code doesn't support inserting arbitrary actions\ninto the middle of a transaction like that.\n\nAs an aside, adding support for this to the ref transaction code is\nless straightforward than inserting both a ref_update() and ref_delete()\ncall into the same transaction. rename_ref()'s special handling to\ndetect D/F conflicts would need to be rewritten for the transaction code\nif we wanted to proactively catch D/F conflicts when renaming a\nreference during a transaction. The reftable backend could support this\nmuch more readily because of its lack of D/F conflicts.\n\nInstead of a more complex modification to the ref transaction code,\ndisplay a progress meter when running verbosely in order to convince the\nuser that Git is doing work while renaming a remote.\n\nThis is mostly done as-expected, with the minor caveat that we\nintentionally count symrefs renames twice, since renaming a symref takes\nplace over two separate calls (one to delete the old one, and another to\ncreate the new one).\n\n[1]: https://lore.kernel.org/git/572367B4.4050207@alum.mit.edu/\n\nSuggested-by: brian m. carlson <sandals@crustytoothpaste.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nA small patch to improve the user experience of `git remote -v rename`\nwith a large number of remote tracking references, in lieu of a more\ncomplicated changes to the reference transaction code.\n\n Documentation/git-remote.txt |  2 +-\n builtin/remote.c             | 26 ++++++++++++++++++++++----\n t/t5505-remote.sh            |  3 ++-\n 3 files changed, 25 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt\nindex 2bebc32566..626f9d0afc 100644\n--- a/Documentation/git-remote.txt\n+++ b/Documentation/git-remote.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git remote' [-v | --verbose]\n 'git remote add' [-t <branch>] [-m <master>] [-f] [--[no-]tags] [--mirror=(fetch|push)] <name> <URL>\n-'git remote rename' <old> <new>\n+'git remote' [-v | --verbose] 'rename' <old> <new>\n 'git remote remove' <name>\n 'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)\n 'git remote set-branches' [--add] <name> <branch>...\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 6f27ddc47b..a18b8532e2 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -12,11 +12,12 @@\n #include \"object-store.h\"\n #include \"strvec.h\"\n #include \"commit-reach.h\"\n+#include \"progress.h\"\n\n static const char * const builtin_remote_usage[] = {\n \t\"git remote [-v | --verbose]\",\n \tN_(\"git remote add [-t <branch>] [-m <master>] [-f] [--tags | --no-tags] [--mirror=<fetch|push>] <name> <url>\"),\n-\tN_(\"git remote rename <old> <new>\"),\n+\tN_(\"git remote [-v | --verbose] rename <old> <new>\"),\n \tN_(\"git remote remove <name>\"),\n \tN_(\"git remote set-head <name> (-a | --auto | -d | --delete | <branch>)\"),\n \tN_(\"git remote [-v | --verbose] show [-n] <name>\"),\n@@ -571,6 +572,7 @@ struct rename_info {\n \tconst char *old_name;\n \tconst char *new_name;\n \tstruct string_list *remote_branches;\n+\tuint32_t symrefs_nr;\n };\n\n static int read_remote_branches(const char *refname,\n@@ -587,10 +589,12 @@ static int read_remote_branches(const char *refname,\n \t\titem = string_list_append(rename->remote_branches, refname);\n \t\tsymref = resolve_ref_unsafe(refname, RESOLVE_REF_READING,\n \t\t\t\t\t    NULL, &flag);\n-\t\tif (symref && (flag & REF_ISSYMREF))\n+\t\tif (symref && (flag & REF_ISSYMREF)) {\n \t\t\titem->util = xstrdup(symref);\n-\t\telse\n+\t\t\trename->symrefs_nr++;\n+\t\t} else {\n \t\t\titem->util = NULL;\n+\t\t}\n \t}\n \tstrbuf_release(&buf);\n\n@@ -682,7 +686,8 @@ static int mv(int argc, const char **argv)\n \t\told_remote_context = STRBUF_INIT;\n \tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n \tstruct rename_info rename;\n-\tint i, refspec_updated = 0;\n+\tint i, j = 0, refspec_updated = 0;\n+\tstruct progress *progress = NULL;\n\n \tif (argc != 3)\n \t\tusage_with_options(builtin_remote_rename_usage, options);\n@@ -690,6 +695,7 @@ static int mv(int argc, const char **argv)\n \trename.old_name = argv[1];\n \trename.new_name = argv[2];\n \trename.remote_branches = &remote_branches;\n+\trename.symrefs_nr = 0;\n\n \toldremote = remote_get(rename.old_name);\n \tif (!remote_is_configured(oldremote, 1)) {\n@@ -764,6 +770,14 @@ static int mv(int argc, const char **argv)\n \t * the new symrefs.\n \t */\n \tfor_each_ref(read_remote_branches, &rename);\n+\tif (verbose) {\n+\t\t/*\n+\t\t * Count symrefs twice, since \"renaming\" them is done by\n+\t\t * deleting and recreating them in two separate passes.\n+\t\t */\n+\t\tprogress = start_progress(_(\"Renaming remote references\"),\n+\t\t\t\t\t  rename.remote_branches->nr + rename.symrefs_nr);\n+\t}\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n \t\tint flag = 0;\n@@ -773,6 +787,7 @@ static int mv(int argc, const char **argv)\n \t\t\tcontinue;\n \t\tif (delete_ref(NULL, item->string, NULL, REF_NO_DEREF))\n \t\t\tdie(_(\"deleting '%s' failed\"), item->string);\n+\t\tdisplay_progress(progress, ++j);\n \t}\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n@@ -788,6 +803,7 @@ static int mv(int argc, const char **argv)\n \t\t\t\titem->string, buf.buf);\n \t\tif (rename_ref(item->string, buf.buf, buf2.buf))\n \t\t\tdie(_(\"renaming '%s' failed\"), item->string);\n+\t\tdisplay_progress(progress, ++j);\n \t}\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n@@ -807,7 +823,9 @@ static int mv(int argc, const char **argv)\n \t\t\t\titem->string, buf.buf);\n \t\tif (create_symref(buf.buf, buf2.buf, buf3.buf))\n \t\t\tdie(_(\"creating '%s' failed\"), buf.buf);\n+\t\tdisplay_progress(progress, ++j);\n \t}\n+\tstop_progress(&progress);\n \tstring_list_clear(&remote_branches, 1);\n\n \thandle_push_default(rename.old_name, rename.new_name);\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 9ab315424c..c4b76485e0 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -753,8 +753,9 @@ test_expect_success 'rename a remote' '\n \t(\n \t\tcd four &&\n \t\tgit config branch.main.pushRemote origin &&\n-\t\tgit remote rename origin upstream &&\n+\t\tGIT_PROGRESS_DELAY=0 git remote -v rename origin upstream 2>err &&\n \t\tgrep \"pushRemote\" .git/config &&\n+\t\tgrep \"Renaming remote references: 100% (4/4), done\" err &&\n \t\ttest -z \"$(git for-each-ref refs/remotes/origin)\" &&\n \t\ttest \"$(git symbolic-ref refs/remotes/upstream/HEAD)\" = \"refs/remotes/upstream/main\" &&\n \t\ttest \"$(git rev-parse upstream/main)\" = \"$(git rev-parse main)\" &&\n--\n2.35.1.73.gccc5557600\n"},{"id":"450108","messageId":"06ef2e2c-7048-1101-870a-4774a2dcd988@github.com","threadId":"57492","inReplyTo":"70a0325ca8ab0492a9b0873ee3fba576c5ab90b9.1646173186.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/remote.c: show progress when renaming remote references","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-02T14:32:48Z","receivedAt":"2022-03-02T14:32:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/1/2022 5:20 PM, Taylor Blau wrote:\n> When renaming a remote, Git needs to rename all remote tracking\n> references to the remote's new name (e.g., renaming\n> \"refs/remotes/old/foo\" to \"refs/remotes/new/foo\" when renaming a remote\n> from \"old\" to \"new\").\n> \n> This can be somewhat slow when there are many references to rename,\n> since each rename is done in a separate call to rename_ref() as opposed\n> to grouping all renames together into the same transaction. It would be\n> nice to execute all renames as a single transaction, but there is a\n> snag: the reference transaction backend doesn't support renames during a\n> transaction (only individually, via rename_ref()).\n> \n> The reasons there are described in more detail in [1], but the main\n> problem is that in order to preserve the existing reflog, it must be\n> moved while holding both locks (i.e., on \"oldname\" and \"newname\"), and\n> the ref transaction code doesn't support inserting arbitrary actions\n> into the middle of a transaction like that.\n> \n> As an aside, adding support for this to the ref transaction code is\n> less straightforward than inserting both a ref_update() and ref_delete()\n> call into the same transaction. rename_ref()'s special handling to\n> detect D/F conflicts would need to be rewritten for the transaction code\n> if we wanted to proactively catch D/F conflicts when renaming a\n> reference during a transaction. The reftable backend could support this\n> much more readily because of its lack of D/F conflicts.\n> \n> Instead of a more complex modification to the ref transaction code,\n> display a progress meter when running verbosely in order to convince the\n> user that Git is doing work while renaming a remote.\n\nThanks for this patch. It improves the user experience through\nuseful feedback.\n\n> @@ -682,7 +686,8 @@ static int mv(int argc, const char **argv)\n>  \t\told_remote_context = STRBUF_INIT;\n>  \tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n>  \tstruct rename_info rename;\n> -\tint i, refspec_updated = 0;\n> +\tint i, j = 0, refspec_updated = 0;\n\nMy only complaint is that 'j' is not informative enough here.\n\n'j' as a loop iterator is good, but we aren't looping \"on\" j,\nbut instead tracking a progress_count across multiple loops.\n\nThanks,\n-Stolee\n"},{"id":"450110","messageId":"Yh+Sp5BxqxYhBv0M@nand.local","threadId":"57492","inReplyTo":"06ef2e2c-7048-1101-870a-4774a2dcd988@github.com","subject":"Re: [PATCH] builtin/remote.c: show progress when renaming remote references","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-02T15:52:07Z","receivedAt":"2022-03-02T15:52:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 02, 2022 at 09:32:48AM -0500, Derrick Stolee wrote:\n> > Instead of a more complex modification to the ref transaction code,\n> > display a progress meter when running verbosely in order to convince the\n> > user that Git is doing work while renaming a remote.\n>\n> Thanks for this patch. It improves the user experience through\n> useful feedback.\n\nAdmittedly, it feels like a little bit of a shortcut to avoid modifying\nthe ref transaction code, but I think it's an OK short-term solution.\nThanks for reviewing.\n\n> > @@ -682,7 +686,8 @@ static int mv(int argc, const char **argv)\n> >  \t\told_remote_context = STRBUF_INIT;\n> >  \tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n> >  \tstruct rename_info rename;\n> > -\tint i, refspec_updated = 0;\n> > +\tint i, j = 0, refspec_updated = 0;\n>\n> My only complaint is that 'j' is not informative enough here.\n>\n> 'j' as a loop iterator is good, but we aren't looping \"on\" j,\n> but instead tracking a progress_count across multiple loops.\n\nHow about s/j/refs_renamed_nr ?\n\nThanks,\nTaylor\n"},{"id":"450151","messageId":"31bfb179-59b9-c68e-1389-44da376ef12f@github.com","threadId":"57492","inReplyTo":"Yh+Sp5BxqxYhBv0M@nand.local","subject":"Re: [PATCH] builtin/remote.c: show progress when renaming remote references","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-02T18:58:02Z","receivedAt":"2022-03-02T18:58:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/2/2022 10:52 AM, Taylor Blau wrote:\n> On Wed, Mar 02, 2022 at 09:32:48AM -0500, Derrick Stolee wrote:\n>>> Instead of a more complex modification to the ref transaction code,\n>>> display a progress meter when running verbosely in order to convince the\n>>> user that Git is doing work while renaming a remote.\n>>\n>> Thanks for this patch. It improves the user experience through\n>> useful feedback.\n> \n> Admittedly, it feels like a little bit of a shortcut to avoid modifying\n> the ref transaction code, but I think it's an OK short-term solution.\n> Thanks for reviewing.\n\nAbsolutely. Help the user first. Think about the harder fix on\nanother timescale.\n\n>>> @@ -682,7 +686,8 @@ static int mv(int argc, const char **argv)\n>>>  \t\told_remote_context = STRBUF_INIT;\n>>>  \tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n>>>  \tstruct rename_info rename;\n>>> -\tint i, refspec_updated = 0;\n>>> +\tint i, j = 0, refspec_updated = 0;\n>>\n>> My only complaint is that 'j' is not informative enough here.\n>>\n>> 'j' as a loop iterator is good, but we aren't looping \"on\" j,\n>> but instead tracking a progress_count across multiple loops.\n> \n> How about s/j/refs_renamed_nr ?\n\nEven better!\n\nThanks,\n-Stolee\n"},{"id":"450153","messageId":"xmqqsfs0kwhd.fsf@gitster.g","threadId":"57492","inReplyTo":"Yh+Sp5BxqxYhBv0M@nand.local","subject":"Re: [PATCH] builtin/remote.c: show progress when renaming remote references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-02T19:03:42Z","receivedAt":"2022-03-02T19:03:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> On Wed, Mar 02, 2022 at 09:32:48AM -0500, Derrick Stolee wrote:\n>> > Instead of a more complex modification to the ref transaction code,\n>> > display a progress meter when running verbosely in order to convince the\n>> > user that Git is doing work while renaming a remote.\n>>\n>> Thanks for this patch. It improves the user experience through\n>> useful feedback.\n>\n> Admittedly, it feels like a little bit of a shortcut to avoid modifying\n> the ref transaction code, but I think it's an OK short-term solution.\n> Thanks for reviewing.\n>\n>> > @@ -682,7 +686,8 @@ static int mv(int argc, const char **argv)\n>> >  \t\told_remote_context = STRBUF_INIT;\n>> >  \tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n>> >  \tstruct rename_info rename;\n>> > -\tint i, refspec_updated = 0;\n>> > +\tint i, j = 0, refspec_updated = 0;\n>>\n>> My only complaint is that 'j' is not informative enough here.\n>>\n>> 'j' as a loop iterator is good, but we aren't looping \"on\" j,\n>> but instead tracking a progress_count across multiple loops.\n>\n> How about s/j/refs_renamed_nr ?\n\nMeaning \"number of renamed refs\"?  start_progress() got the number\nof remote branches to be renamed (i.e. rename.remote_branches->nr,\nwith symref adjustments), and approaching that ceiling from 0 by\ncounting the renamed ones so far makes sense.\n"},{"id":"450154","messageId":"220302.865yow6u8a.gmgdl@evledraar.gmail.com","threadId":"57492","inReplyTo":"70a0325ca8ab0492a9b0873ee3fba576c5ab90b9.1646173186.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/remote.c: show progress when renaming remote references","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-02T19:00:59Z","receivedAt":"2022-03-02T19:16:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 01 2022, Taylor Blau wrote:\n\n> @@ -11,7 +11,7 @@ SYNOPSIS\n>  [verse]\n>  'git remote' [-v | --verbose]\n>  'git remote add' [-t <branch>] [-m <master>] [-f] [--[no-]tags] [--mirror=(fetch|push)] <name> <URL>\n> -'git remote rename' <old> <new>\n> +'git remote' [-v | --verbose] 'rename' <old> <new>\n\nI realize that you're just copying an existing pattern within\nbuiltin/reflog.c here, but we should really call this \"--no-progress\",\nnot \"-v\" or \"--verbose\".\n\nI did that in the last 3 patches here, which I was waiting on\nre-submitting:\nhttps://lore.kernel.org/git/cover-00.12-00000000000-20211130T213319Z-avarab@gmail.com/\n\nI.e. the whole reason we use --verbose here is entirely historical, it\nused to (well, still is without those patches) very verbose line-by-line\noutput.\n\nWhereas for other things we turn progress on by default, and --verbose\nis something very different.\n\nSo please have this by \"int progress = 1\", and have a \"PARSE_OPT_NONEG\"\n\"no-progress\" option instead, there's no reason we need to propagate the\nexisting UX mistake in \"reflog expire\".\n\n[I reversed the order you wrote the following, due to the obvious\ndigression...] :)\n\n> When renaming a remote, Git needs to rename all remote tracking\n> references to the remote's new name (e.g., renaming\n> \"refs/remotes/old/foo\" to \"refs/remotes/new/foo\" when renaming a remote\n> from \"old\" to \"new\").\n>\n> This can be somewhat slow when there are many references to rename,\n> since each rename is done in a separate call to rename_ref() as opposed\n> to grouping all renames together into the same transaction. It would be\n> nice to execute all renames as a single transaction, but there is a\n> snag: the reference transaction backend doesn't support renames during a\n> transaction (only individually, via rename_ref()).\n>\n> The reasons there are described in more detail in [1], but the main\n> problem is that in order to preserve the existing reflog, it must be\n> moved while holding both locks (i.e., on \"oldname\" and \"newname\"), and\n> the ref transaction code doesn't support inserting arbitrary actions\n> into the middle of a transaction like that.\n>\n> As an aside, adding support for this to the ref transaction code is\n> less straightforward than inserting both a ref_update() and ref_delete()\n> call into the same transaction. rename_ref()'s special handling to\n> detect D/F conflicts would need to be rewritten for the transaction code\n> if we wanted to proactively catch D/F conflicts when renaming a\n> reference during a transaction. The reftable backend could support this\n> much more readily because of its lack of D/F conflicts.\n\nAs an aside I think the reftable code \"emulates\" the D/F conflicts of\nthe files backend, but I'm not sure (this is from vague memory).\n\nAnd I've run into the same limitations you're describing here with the\nref transactions code. I tried to transaction-ize the \"branch rename\"\nthe other day, there we copy an existing reflog in its entirety.\n\nIt would be really nice if you could plug things into the\ntransaction. It would have to be things that could support the same\ndo-stuff/abort-stuff semantics, e.g. being able to create a file with\narbitrary contents, then unlink() it if we're rolling back.\n\nBut in any case, all of that is a much larger change than is really\nrequired here, so I (also) digress :)\n\nAs an even further aside: If we had a slight \"twist\" on the ref backend\nwhere we were able to store the reflogs next to the actual ref files,\ne.g.:\n\n    .git/refs/heads/master\n    .git/refs/heads/.master.reflog\n\nWhich would work since that '.master.reflog' name isn't a valid refname\n(it contains \".\"), couldn't we safely do this whole operation on a POSIX\nFS with a simple:\n\n    mv .git/refs/remotes/origin .git/refs/remotes/newname\n\nI.e. even if you had concurrent *.lock files in play from other\ntransactions having those would mean that we had a file descriptor open\nto file in question, and a file being renamed underneath you doesn't\nchange your ability to write to that open file.\n\nAlthough I guess we rename() them into place, and if that target\nmoves..., so we'd need to pre-open them.\n\nHrm.\n\nAnyway, that's an even larger digression & can of worms, but something I\nwondered about when I saw this take a LOOOONG time in the past, \"why\ndoesn't it just rename the folders?\".\n\n"},{"id":"450184","messageId":"Yh/t3HfKiEMx957i@camp.crustytoothpaste.net","threadId":"57492","inReplyTo":"70a0325ca8ab0492a9b0873ee3fba576c5ab90b9.1646173186.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/remote.c: show progress when renaming remote references","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-03-02T22:21:16Z","receivedAt":"2022-03-02T22:21:24Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-03-01 at 22:20:33, Taylor Blau wrote:\n> When renaming a remote, Git needs to rename all remote tracking\n> references to the remote's new name (e.g., renaming\n> \"refs/remotes/old/foo\" to \"refs/remotes/new/foo\" when renaming a remote\n> from \"old\" to \"new\").\n> \n> This can be somewhat slow when there are many references to rename,\n> since each rename is done in a separate call to rename_ref() as opposed\n> to grouping all renames together into the same transaction. It would be\n> nice to execute all renames as a single transaction, but there is a\n> snag: the reference transaction backend doesn't support renames during a\n> transaction (only individually, via rename_ref()).\n> \n> The reasons there are described in more detail in [1], but the main\n> problem is that in order to preserve the existing reflog, it must be\n> moved while holding both locks (i.e., on \"oldname\" and \"newname\"), and\n> the ref transaction code doesn't support inserting arbitrary actions\n> into the middle of a transaction like that.\n> \n> As an aside, adding support for this to the ref transaction code is\n> less straightforward than inserting both a ref_update() and ref_delete()\n> call into the same transaction. rename_ref()'s special handling to\n> detect D/F conflicts would need to be rewritten for the transaction code\n> if we wanted to proactively catch D/F conflicts when renaming a\n> reference during a transaction. The reftable backend could support this\n> much more readily because of its lack of D/F conflicts.\n> \n> Instead of a more complex modification to the ref transaction code,\n> display a progress meter when running verbosely in order to convince the\n> user that Git is doing work while renaming a remote.\n> \n> This is mostly done as-expected, with the minor caveat that we\n> intentionally count symrefs renames twice, since renaming a symref takes\n> place over two separate calls (one to delete the old one, and another to\n> create the new one).\n> \n> [1]: https://lore.kernel.org/git/572367B4.4050207@alum.mit.edu/\n> \n> Suggested-by: brian m. carlson <sandals@crustytoothpaste.net>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n\nAs I mentioned to you personally, I think this looks good.\n\nFor context, I discovered this when I tried to rename a remote with tens\nof thousands of branches and it just ran silently for an extended period\nof time without any output.  I actually interrupted it with Ctrl-C\nbecause I thought it had hung, so I'm hoping this will provide a better\nexperience for users in that situation.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"450196","messageId":"Yh/1zFChEXz7r1vN@nand.local","threadId":"57492","inReplyTo":"220302.865yow6u8a.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] builtin/remote.c: show progress when renaming remote references","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-02T22:55:08Z","receivedAt":"2022-03-02T23:53:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 02, 2022 at 08:00:59PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> So please have this by \"int progress = 1\", and have a \"PARSE_OPT_NONEG\"\n> \"no-progress\" option instead, there's no reason we need to propagate the\n> existing UX mistake in \"reflog expire\".\n\nYes, I agree that `git remote` is unlike other commands where you have\nto opt _in_ to see progress, not out.\n\nBut I deliberately avoided doing anything about it here, since this\npatch is more about making an existing set of arguments (`git remote -v\nrename`) do something more useful than before, and not adding a new\noption.\n\nIn other words, I felt that because you could already run:\n\n    $ git remote -v rename old new\n\nthat it was better to punt on any changes to the option itself until\nlater.\n\nOn a similar note, it would be nice if this option worked on either side\nof the sub-command, like how you can do either of:\n\n    $ git multi-pack-index --object-dir=... write\n    $ git multi-pack-index write --object-dir=...\n\nBut I don't think we should let perfect be the enemy of the good here,\nin case you were suggesting delaying this patch until we sort that issue\nout.\n\n> [I reversed the order you wrote the following, due to the obvious\n> digression...] :)\n\n;-), thanks.\n\n> As an aside I think the reftable code \"emulates\" the D/F conflicts of\n> the files backend, but I'm not sure (this is from vague memory).\n\nPerhaps, though I'm admittedly not familiar enough with that work to\ntell know for sure, either. I don't think I have a ton to add to the\nlower part of your reply, so I'll stop here.\n\nThanks,\nTaylor\n"},{"id":"450199","messageId":"Yh/2Pkc493ABWv46@nand.local","threadId":"57492","inReplyTo":"Yh/t3HfKiEMx957i@camp.crustytoothpaste.net","subject":"Re: [PATCH] builtin/remote.c: show progress when renaming remote references","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-02T22:57:02Z","receivedAt":"2022-03-02T23:58:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 02, 2022 at 10:21:16PM +0000, brian m. carlson wrote:\n> For context, I discovered this when I tried to rename a remote with tens\n> of thousands of branches and it just ran silently for an extended period\n> of time without any output.  I actually interrupted it with Ctrl-C\n> because I thought it had hung, so I'm hoping this will provide a better\n> experience for users in that situation.\n\nThanks again for pointing it out to me. To be honest, I'm skeptical that\nthis patch alone will improve things much, since you still have to pass\nthe '-v' flag to see the new progress meter.\n\nBut perhaps users who suspect the command is hung will re-run it with\nthe '-v' flag instinctually and get more helpful output. I'll look at\nmaking `git remote`'s `-v` behave a little more like `--[no-]progress'\nin another series.\n\nThanks,\nTaylor\n"},{"id":"450201","messageId":"dc63ec91ab24eb24a276de074138745fa061eba1.1646261969.git.me@ttaylorr.com","threadId":"57492","inReplyTo":"70a0325ca8ab0492a9b0873ee3fba576c5ab90b9.1646173186.git.me@ttaylorr.com","subject":"[PATCH v2] builtin/remote.c: show progress when renaming remote references","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-02T23:00:12Z","receivedAt":"2022-03-03T00:08:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When renaming a remote, Git needs to rename all remote tracking\nreferences to the remote's new name (e.g., renaming\n\"refs/remotes/old/foo\" to \"refs/remotes/new/foo\" when renaming a remote\nfrom \"old\" to \"new\").\n\nThis can be somewhat slow when there are many references to rename,\nsince each rename is done in a separate call to rename_ref() as opposed\nto grouping all renames together into the same transaction. It would be\nnice to execute all renames as a single transaction, but there is a\nsnag: the reference transaction backend doesn't support renames during a\ntransaction (only individually, via rename_ref()).\n\nThe reasons there are described in more detail in [1], but the main\nproblem is that in order to preserve the existing reflog, it must be\nmoved while holding both locks (i.e., on \"oldname\" and \"newname\"), and\nthe ref transaction code doesn't support inserting arbitrary actions\ninto the middle of a transaction like that.\n\nAs an aside, adding support for this to the ref transaction code is\nless straightforward than inserting both a ref_update() and ref_delete()\ncall into the same transaction. rename_ref()'s special handling to\ndetect D/F conflicts would need to be rewritten for the transaction code\nif we wanted to proactively catch D/F conflicts when renaming a\nreference during a transaction. The reftable backend could support this\nmuch more readily because of its lack of D/F conflicts.\n\nInstead of a more complex modification to the ref transaction code,\ndisplay a progress meter when running verbosely in order to convince the\nuser that Git is doing work while renaming a remote.\n\nThis is mostly done as-expected, with the minor caveat that we\nintentionally count symrefs renames twice, since renaming a symref takes\nplace over two separate calls (one to delete the old one, and another to\ncreate the new one).\n\n[1]: https://lore.kernel.org/git/572367B4.4050207@alum.mit.edu/\n\nSuggested-by: brian m. carlson <sandals@crustytoothpaste.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nA small reroll which renames the variable used to keep track of our\nprogress from \"j\" to \"refs_renamed_nr\" for clarity.\n\n Documentation/git-remote.txt |  2 +-\n builtin/remote.c             | 26 ++++++++++++++++++++++----\n t/t5505-remote.sh            |  3 ++-\n 3 files changed, 25 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt\nindex 2bebc32566..626f9d0afc 100644\n--- a/Documentation/git-remote.txt\n+++ b/Documentation/git-remote.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git remote' [-v | --verbose]\n 'git remote add' [-t <branch>] [-m <master>] [-f] [--[no-]tags] [--mirror=(fetch|push)] <name> <URL>\n-'git remote rename' <old> <new>\n+'git remote' [-v | --verbose] 'rename' <old> <new>\n 'git remote remove' <name>\n 'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)\n 'git remote set-branches' [--add] <name> <branch>...\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 6f27ddc47b..00668af5c9 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -12,11 +12,12 @@\n #include \"object-store.h\"\n #include \"strvec.h\"\n #include \"commit-reach.h\"\n+#include \"progress.h\"\n\n static const char * const builtin_remote_usage[] = {\n \t\"git remote [-v | --verbose]\",\n \tN_(\"git remote add [-t <branch>] [-m <master>] [-f] [--tags | --no-tags] [--mirror=<fetch|push>] <name> <url>\"),\n-\tN_(\"git remote rename <old> <new>\"),\n+\tN_(\"git remote [-v | --verbose] rename <old> <new>\"),\n \tN_(\"git remote remove <name>\"),\n \tN_(\"git remote set-head <name> (-a | --auto | -d | --delete | <branch>)\"),\n \tN_(\"git remote [-v | --verbose] show [-n] <name>\"),\n@@ -571,6 +572,7 @@ struct rename_info {\n \tconst char *old_name;\n \tconst char *new_name;\n \tstruct string_list *remote_branches;\n+\tuint32_t symrefs_nr;\n };\n\n static int read_remote_branches(const char *refname,\n@@ -587,10 +589,12 @@ static int read_remote_branches(const char *refname,\n \t\titem = string_list_append(rename->remote_branches, refname);\n \t\tsymref = resolve_ref_unsafe(refname, RESOLVE_REF_READING,\n \t\t\t\t\t    NULL, &flag);\n-\t\tif (symref && (flag & REF_ISSYMREF))\n+\t\tif (symref && (flag & REF_ISSYMREF)) {\n \t\t\titem->util = xstrdup(symref);\n-\t\telse\n+\t\t\trename->symrefs_nr++;\n+\t\t} else {\n \t\t\titem->util = NULL;\n+\t\t}\n \t}\n \tstrbuf_release(&buf);\n\n@@ -682,7 +686,8 @@ static int mv(int argc, const char **argv)\n \t\told_remote_context = STRBUF_INIT;\n \tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n \tstruct rename_info rename;\n-\tint i, refspec_updated = 0;\n+\tint i, refs_renamed_nr = 0, refspec_updated = 0;\n+\tstruct progress *progress = NULL;\n\n \tif (argc != 3)\n \t\tusage_with_options(builtin_remote_rename_usage, options);\n@@ -690,6 +695,7 @@ static int mv(int argc, const char **argv)\n \trename.old_name = argv[1];\n \trename.new_name = argv[2];\n \trename.remote_branches = &remote_branches;\n+\trename.symrefs_nr = 0;\n\n \toldremote = remote_get(rename.old_name);\n \tif (!remote_is_configured(oldremote, 1)) {\n@@ -764,6 +770,14 @@ static int mv(int argc, const char **argv)\n \t * the new symrefs.\n \t */\n \tfor_each_ref(read_remote_branches, &rename);\n+\tif (verbose) {\n+\t\t/*\n+\t\t * Count symrefs twice, since \"renaming\" them is done by\n+\t\t * deleting and recreating them in two separate passes.\n+\t\t */\n+\t\tprogress = start_progress(_(\"Renaming remote references\"),\n+\t\t\t\t\t  rename.remote_branches->nr + rename.symrefs_nr);\n+\t}\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n \t\tint flag = 0;\n@@ -773,6 +787,7 @@ static int mv(int argc, const char **argv)\n \t\t\tcontinue;\n \t\tif (delete_ref(NULL, item->string, NULL, REF_NO_DEREF))\n \t\t\tdie(_(\"deleting '%s' failed\"), item->string);\n+\t\tdisplay_progress(progress, ++refs_renamed_nr);\n \t}\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n@@ -788,6 +803,7 @@ static int mv(int argc, const char **argv)\n \t\t\t\titem->string, buf.buf);\n \t\tif (rename_ref(item->string, buf.buf, buf2.buf))\n \t\t\tdie(_(\"renaming '%s' failed\"), item->string);\n+\t\tdisplay_progress(progress, ++refs_renamed_nr);\n \t}\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n@@ -807,7 +823,9 @@ static int mv(int argc, const char **argv)\n \t\t\t\titem->string, buf.buf);\n \t\tif (create_symref(buf.buf, buf2.buf, buf3.buf))\n \t\t\tdie(_(\"creating '%s' failed\"), buf.buf);\n+\t\tdisplay_progress(progress, ++refs_renamed_nr);\n \t}\n+\tstop_progress(&progress);\n \tstring_list_clear(&remote_branches, 1);\n\n \thandle_push_default(rename.old_name, rename.new_name);\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 9ab315424c..c4b76485e0 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -753,8 +753,9 @@ test_expect_success 'rename a remote' '\n \t(\n \t\tcd four &&\n \t\tgit config branch.main.pushRemote origin &&\n-\t\tgit remote rename origin upstream &&\n+\t\tGIT_PROGRESS_DELAY=0 git remote -v rename origin upstream 2>err &&\n \t\tgrep \"pushRemote\" .git/config &&\n+\t\tgrep \"Renaming remote references: 100% (4/4), done\" err &&\n \t\ttest -z \"$(git for-each-ref refs/remotes/origin)\" &&\n \t\ttest \"$(git symbolic-ref refs/remotes/upstream/HEAD)\" = \"refs/remotes/upstream/main\" &&\n \t\ttest \"$(git rev-parse upstream/main)\" = \"$(git rev-parse main)\" &&\n--\n2.35.1.73.gccc5557600\n"},{"id":"450257","messageId":"220303.8635jz5mcz.gmgdl@evledraar.gmail.com","threadId":"57492","inReplyTo":"Yh/1zFChEXz7r1vN@nand.local","subject":"Re: [PATCH] builtin/remote.c: show progress when renaming remote references","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-03T10:51:08Z","receivedAt":"2022-03-03T11:03:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 02 2022, Taylor Blau wrote:\n\n> On Wed, Mar 02, 2022 at 08:00:59PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> So please have this by \"int progress = 1\", and have a \"PARSE_OPT_NONEG\"\n>> \"no-progress\" option instead, there's no reason we need to propagate the\n>> existing UX mistake in \"reflog expire\".\n>\n> Yes, I agree that `git remote` is unlike other commands where you have\n> to opt _in_ to see progress, not out.\n>\n> But I deliberately avoided doing anything about it here, since this\n> patch is more about making an existing set of arguments (`git remote -v\n> rename`) do something more useful than before, and not adding a new\n> option.\n>\n> In other words, I felt that because you could already run:\n>\n>     $ git remote -v rename old new\n>\n> that it was better to punt on any changes to the option itself until\n> later.\n\nSo first, I must say that I was just entirely confused when I wrote\nthis. I managed to somehow read this code and also mentally mix up that\nyou were patching git-remote and not git-reflog, hence linking to those\n\"reflog\" patches.\n\nBy way of explanation I think that's because I started thinking \"oh\nyeah, it's that one special case where --verbose shows progress...\" :)\n\nBut in any case, for *this patch* I think that also means that using\n--verbose here makes even less sense, because --verbose for \"git-remote\"\nis:\n\n    Be a little more verbose and show remote url after name.\n    NOTE: This must be placed between remote and subcommand.\n\nAlthough reading it over it seems that was written for \"git remote -v\n[show]\", but we don't entirely hold to it already, but in any case let's\nnot also conflate it with what should be a \"--no-progress\" here.\n\n> On a similar note, it would be nice if this option worked on either side\n> of the sub-command, like how you can do either of:\n>\n>     $ git multi-pack-index --object-dir=... write\n>     $ git multi-pack-index write --object-dir=...\n>\n> But I don't think we should let perfect be the enemy of the good here,\n> in case you were suggesting delaying this patch until we sort that issue\n> out.\n\nI think per [1] and your [2] we agreed to move forward in not adding any\nmore of these subcommand-level options. I.e. I was arguing in [1] that\nit might be a good idea for e.g. --no-progress or --verbose, but the\nconsensus was to go for your [2] of declaring that it's always the\nsubcommand that takes the option.\n\n(Which as an aside, I'm fine with, and have come more around to being\nthe Way It Should Be since then).\n\nSo just doing:\n\t\n\tdiff --git a/builtin/remote.c b/builtin/remote.c\n\tindex 6f27ddc47bd..047bcda57c5 100644\n\t--- a/builtin/remote.c\n\t+++ b/builtin/remote.c\n\t@@ -674,7 +674,9 @@ static void handle_push_default(const char* old_name, const char* new_name)\n\t \n\t static int mv(int argc, const char **argv)\n\t {\n\t+\tint progress = 1;\n\t \tstruct option options[] = {\n\t+\t\tOPT_BOOL(0, \"progress\", &progress, N_(\"show progress\")),\n\t \t\tOPT_END()\n\t \t};\n\t \tstruct remote *oldremote, *newremote;\n\nWould be consistent with [2] and not mix up --verbose and its (probably\nsomewhat inaccurate already) documented promises with this new output,\nand improve the UX by turning it on by default.\n\n1. https://lore.kernel.org/git/87zgsad6mn.fsf@evledraar.gmail.com/\n2. https://lore.kernel.org/git/cover.1631980949.git.me@ttaylorr.com/\n\n>> [I reversed the order you wrote the following, due to the obvious\n>> digression...] :)\n>\n> ;-), thanks.\n>\n>> As an aside I think the reftable code \"emulates\" the D/F conflicts of\n>> the files backend, but I'm not sure (this is from vague memory).\n>\n> Perhaps, though I'm admittedly not familiar enough with that work to\n> tell know for sure, either. I don't think I have a ton to add to the\n> lower part of your reply, so I'll stop here.\n\n:)\n"},{"id":"450258","messageId":"220303.86y21r47oh.gmgdl@evledraar.gmail.com","threadId":"57492","inReplyTo":"dc63ec91ab24eb24a276de074138745fa061eba1.1646261969.git.me@ttaylorr.com","subject":"Re: [PATCH v2] builtin/remote.c: show progress when renaming remote references","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-03T11:04:22Z","receivedAt":"2022-03-03T11:06:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 02 2022, Taylor Blau wrote:\n\n\n> @@ -753,8 +753,9 @@ test_expect_success 'rename a remote' '\n>  \t(\n>  \t\tcd four &&\n>  \t\tgit config branch.main.pushRemote origin &&\n> -\t\tgit remote rename origin upstream &&\n> +\t\tGIT_PROGRESS_DELAY=0 git remote -v rename origin upstream 2>err &&\n>  \t\tgrep \"pushRemote\" .git/config &&\n> +\t\tgrep \"Renaming remote references: 100% (4/4), done\" err &&\n\nJust on this part. You can use some variant of \"test_region\" to do this with trace2:\n\n    git grep -C10 TRACE.*progress -- t\n\nWhich as an improvement also tests that you called stop_progress(),\ni.e. that the trace2 region is ended.\n\n>  \t\ttest -z \"$(git for-each-ref refs/remotes/origin)\" &&\n>  \t\ttest \"$(git symbolic-ref refs/remotes/upstream/HEAD)\" = \"refs/remotes/upstream/main\" &&\n>  \t\ttest \"$(git rev-parse upstream/main)\" = \"$(git rev-parse main)\" &&\n\n"},{"id":"450280","messageId":"95207e46-defb-f36e-ea3c-a2454c15bd8e@github.com","threadId":"57492","inReplyTo":"Yh/2Pkc493ABWv46@nand.local","subject":"Re: [PATCH] builtin/remote.c: show progress when renaming remote references","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-03T16:09:20Z","receivedAt":"2022-03-03T16:09:26Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/2/2022 5:57 PM, Taylor Blau wrote:\n> On Wed, Mar 02, 2022 at 10:21:16PM +0000, brian m. carlson wrote:\n>> For context, I discovered this when I tried to rename a remote with tens\n>> of thousands of branches and it just ran silently for an extended period\n>> of time without any output.  I actually interrupted it with Ctrl-C\n>> because I thought it had hung, so I'm hoping this will provide a better\n>> experience for users in that situation.\n> \n> Thanks again for pointing it out to me. To be honest, I'm skeptical that\n> this patch alone will improve things much, since you still have to pass\n> the '-v' flag to see the new progress meter.\n> \n> But perhaps users who suspect the command is hung will re-run it with\n> the '-v' flag instinctually and get more helpful output. I'll look at\n> making `git remote`'s `-v` behave a little more like `--[no-]progress'\n> in another series.\n\nI guess one big reason for Ævar's suggestion about using --[no-]progress\nas the signal for progress is that we can make --progress the default\nwhen isatty(2) is true. We should not do the same for --verbose.\n\nLooking at other examples, I see that 'fsck' has --verbose imply\n--no-progress, probably because the verbose output would write lines\nthat become interleaved with the progress indicators (and those lines\nact as progress in themselves). Not sure if that's the right choice in\nthis case, too.\n\nThanks,\n-Stolee\n\n"},{"id":"450300","messageId":"YiEdEDN8D0MqnElC@nand.local","threadId":"57492","inReplyTo":"220303.8635jz5mcz.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] builtin/remote.c: show progress when renaming remote references","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-03T19:54:56Z","receivedAt":"2022-03-03T19:55:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 03, 2022 at 11:51:08AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Wed, Mar 02 2022, Taylor Blau wrote:\n>\n> > On Wed, Mar 02, 2022 at 08:00:59PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> >> So please have this by \"int progress = 1\", and have a \"PARSE_OPT_NONEG\"\n> >> \"no-progress\" option instead, there's no reason we need to propagate the\n> >> existing UX mistake in \"reflog expire\".\n> >\n> > Yes, I agree that `git remote` is unlike other commands where you have\n> > to opt _in_ to see progress, not out.\n> >\n> > But I deliberately avoided doing anything about it here, since this\n> > patch is more about making an existing set of arguments (`git remote -v\n> > rename`) do something more useful than before, and not adding a new\n> > option.\n> >\n> > In other words, I felt that because you could already run:\n> >\n> >     $ git remote -v rename old new\n> >\n> > that it was better to punt on any changes to the option itself until\n> > later.\n>\n> So first, I must say that I was just entirely confused when I wrote\n> this. I managed to somehow read this code and also mentally mix up that\n> you were patching git-remote and not git-reflog, hence linking to those\n> \"reflog\" patches.\n\nNo problem, your response below jogged my memory on the \"should\n`--[no-]progress` be accepted on either side of a sub-command or not?\"\ndiscussion we had a while ago.\n\n> But in any case, for *this patch* I think that also means that using\n> --verbose here makes even less sense, because --verbose for \"git-remote\"\n> is:\n>\n>     Be a little more verbose and show remote url after name.\n>     NOTE: This must be placed between remote and subcommand.\n>\n> Although reading it over it seems that was written for \"git remote -v\n> [show]\", but we don't entirely hold to it already, but in any case let's\n> not also conflate it with what should be a \"--no-progress\" here.\n\nThank you for pointing that out. I agree that between the documentation\n(which I agree is over-fitted for `git remote -v`) and my (admittedly,\nfoggy!) memory of our earlier discussions, I think that `git remote\nrename --[no-progress]` is the right way to go here.\n\n> So just doing:\n>\n> \tdiff --git a/builtin/remote.c b/builtin/remote.c\n> \tindex 6f27ddc47bd..047bcda57c5 100644\n> \t--- a/builtin/remote.c\n> \t+++ b/builtin/remote.c\n> \t@@ -674,7 +674,9 @@ static void handle_push_default(const char* old_name, const char* new_name)\n>\n> \t static int mv(int argc, const char **argv)\n> \t {\n> \t+\tint progress = 1;\n> \t \tstruct option options[] = {\n> \t+\t\tOPT_BOOL(0, \"progress\", &progress, N_(\"show progress\")),\n> \t \t\tOPT_END()\n> \t \t};\n> \t \tstruct remote *oldremote, *newremote;\n>\n> Would be consistent with [2] and not mix up --verbose and its (probably\n> somewhat inaccurate already) documented promises with this new output,\n> and improve the UX by turning it on by default.\n>\n> 1. https://lore.kernel.org/git/87zgsad6mn.fsf@evledraar.gmail.com/\n> 2. https://lore.kernel.org/git/cover.1631980949.git.me@ttaylorr.com/\n\nAgreed, thanks!\n\nThanks,\nTaylor\n"},{"id":"450301","messageId":"YiEd4zs9KhjhH9fp@nand.local","threadId":"57492","inReplyTo":"95207e46-defb-f36e-ea3c-a2454c15bd8e@github.com","subject":"Re: [PATCH] builtin/remote.c: show progress when renaming remote references","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-03T19:58:27Z","receivedAt":"2022-03-03T19:58:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 03, 2022 at 11:09:20AM -0500, Derrick Stolee wrote:\n> I guess one big reason for Ævar's suggestion about using --[no-]progress\n> as the signal for progress is that we can make --progress the default\n> when isatty(2) is true. We should not do the same for --verbose.\n\nYep, agreed.\n\n> Looking at other examples, I see that 'fsck' has --verbose imply\n> --no-progress, probably because the verbose output would write lines\n> that become interleaved with the progress indicators (and those lines\n> act as progress in themselves). Not sure if that's the right choice in\n> this case, too.\n\nI think for `git remote rename` it won't matter, since it doesn't do\nanything with the `-v` option anyway (so there's no output to clobber in\nthe first place).\n\nThanks,\nTaylor\n"},{"id":"450315","messageId":"cover.1646346286.git.me@ttaylorr.com","threadId":"57492","inReplyTo":"70a0325ca8ab0492a9b0873ee3fba576c5ab90b9.1646173186.git.me@ttaylorr.com","subject":"[PATCH v3 0/2] remote: show progress display when renaming","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-03T22:25:13Z","receivedAt":"2022-03-03T22:25:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a reroll of the patch we've been discussing to add progress\noutput when renaming remote-tracking references (as part of a `git\nremote rename <old> <new>`).\n\nThis single patch is now two (and hence, has a cover letter!) because of\na slight change in approach which amounts to changing this behavior from\nan opt-in\n\n    git remote -v rename <old> <new>\n\nto something much more in line with our existing use of the\n`--[no-]progress` option like:\n\n    git remote rename [--[no-]progress] <old> <new>\n\n(where `--progress` is the default exactly when `isatty(2)` is true).\n\nI think similar treatment could be applied to other `git remote`\nsub-commands, but I'm reasonably happy starting with just `git remote\nrename` and looking at the others later.\n\nTaylor Blau (2):\n  builtin/remote.c: parse options in 'rename'\n  builtin/remote.c: show progress when renaming remote references\n\n Documentation/git-remote.txt |  2 +-\n builtin/remote.c             | 39 ++++++++++++++++++++++++++++--------\n t/t5505-remote.sh            |  4 +++-\n 3 files changed, 35 insertions(+), 10 deletions(-)\n\nRange-diff against v2:\n-:  ---------- > 1:  b76da50b54 builtin/remote.c: parse options in 'rename'\n1:  dc63ec91ab ! 2:  d5b0a4b710 builtin/remote.c: show progress when renaming remote references\n    @@ Documentation/git-remote.txt: SYNOPSIS\n      'git remote' [-v | --verbose]\n      'git remote add' [-t <branch>] [-m <master>] [-f] [--[no-]tags] [--mirror=(fetch|push)] <name> <URL>\n     -'git remote rename' <old> <new>\n    -+'git remote' [-v | --verbose] 'rename' <old> <new>\n    ++'git remote rename' [--[no-]progress] <old> <new>\n      'git remote remove' <name>\n      'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)\n      'git remote set-branches' [--add] <name> <branch>...\n    @@ builtin/remote.c\n      \t\"git remote [-v | --verbose]\",\n      \tN_(\"git remote add [-t <branch>] [-m <master>] [-f] [--tags | --no-tags] [--mirror=<fetch|push>] <name> <url>\"),\n     -\tN_(\"git remote rename <old> <new>\"),\n    -+\tN_(\"git remote [-v | --verbose] rename <old> <new>\"),\n    ++\tN_(\"git remote rename [--[no-]progress] <old> <new>\"),\n      \tN_(\"git remote remove <name>\"),\n      \tN_(\"git remote set-head <name> (-a | --auto | -d | --delete | <branch>)\"),\n      \tN_(\"git remote [-v | --verbose] show [-n] <name>\"),\n    +@@ builtin/remote.c: static const char * const builtin_remote_add_usage[] = {\n    + };\n    + \n    + static const char * const builtin_remote_rename_usage[] = {\n    +-\tN_(\"git remote rename <old> <new>\"),\n    ++\tN_(\"git remote rename [--[no-]progress] <old> <new>\"),\n    + \tNULL\n    + };\n    + \n     @@ builtin/remote.c: struct rename_info {\n      \tconst char *old_name;\n      \tconst char *new_name;\n    @@ builtin/remote.c: static int read_remote_branches(const char *refname,\n      \t}\n      \tstrbuf_release(&buf);\n      \n    +@@ builtin/remote.c: static void handle_push_default(const char* old_name, const char* new_name)\n    + \n    + static int mv(int argc, const char **argv)\n    + {\n    ++\tint show_progress = isatty(2);\n    + \tstruct option options[] = {\n    ++\t\tOPT_BOOL(0, \"progress\", &show_progress, N_(\"force progress reporting\")),\n    + \t\tOPT_END()\n    + \t};\n    + \tstruct remote *oldremote, *newremote;\n     @@ builtin/remote.c: static int mv(int argc, const char **argv)\n      \t\told_remote_context = STRBUF_INIT;\n      \tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n    @@ builtin/remote.c: static int mv(int argc, const char **argv)\n     +\tint i, refs_renamed_nr = 0, refspec_updated = 0;\n     +\tstruct progress *progress = NULL;\n      \n    - \tif (argc != 3)\n    - \t\tusage_with_options(builtin_remote_rename_usage, options);\n    + \targc = parse_options(argc, argv, NULL, options,\n    + \t\t\t     builtin_remote_rename_usage, 0);\n     @@ builtin/remote.c: static int mv(int argc, const char **argv)\n    - \trename.old_name = argv[1];\n    - \trename.new_name = argv[2];\n    + \trename.old_name = argv[0];\n    + \trename.new_name = argv[1];\n      \trename.remote_branches = &remote_branches;\n     +\trename.symrefs_nr = 0;\n      \n    @@ builtin/remote.c: static int mv(int argc, const char **argv)\n      \t * the new symrefs.\n      \t */\n      \tfor_each_ref(read_remote_branches, &rename);\n    -+\tif (verbose) {\n    ++\tif (show_progress) {\n     +\t\t/*\n     +\t\t * Count symrefs twice, since \"renaming\" them is done by\n     +\t\t * deleting and recreating them in two separate passes.\n    @@ t/t5505-remote.sh: test_expect_success 'rename a remote' '\n      \t\tcd four &&\n      \t\tgit config branch.main.pushRemote origin &&\n     -\t\tgit remote rename origin upstream &&\n    -+\t\tGIT_PROGRESS_DELAY=0 git remote -v rename origin upstream 2>err &&\n    ++\t\tGIT_TRACE2_EVENT=$(pwd)/trace \\\n    ++\t\t\tgit remote rename --progress origin upstream &&\n    ++\t\ttest_region progress \"Renaming remote references\" trace &&\n      \t\tgrep \"pushRemote\" .git/config &&\n    -+\t\tgrep \"Renaming remote references: 100% (4/4), done\" err &&\n      \t\ttest -z \"$(git for-each-ref refs/remotes/origin)\" &&\n      \t\ttest \"$(git symbolic-ref refs/remotes/upstream/HEAD)\" = \"refs/remotes/upstream/main\" &&\n    - \t\ttest \"$(git rev-parse upstream/main)\" = \"$(git rev-parse main)\" &&\n-- \n2.35.1.73.gccc5557600\n"},{"id":"450316","messageId":"b76da50b549d71b3e61f74c6250fea595ee2b2c1.1646346287.git.me@ttaylorr.com","threadId":"57492","inReplyTo":"cover.1646346286.git.me@ttaylorr.com","subject":"[PATCH v3 1/2] builtin/remote.c: parse options in 'rename'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-03T22:25:16Z","receivedAt":"2022-03-03T22:25:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The 'git remote rename' command doesn't currently take any command-line\narguments besides the existing and new name of a remote, and so has no\nneed to call parse_options().\n\nBut the subsequent patch will add a `--[no-]progress` option, in which\ncase we will need to call parse_options().\n\nDo so now so as to avoid cluttering the following patch with noise, like\nadjusting setting `rename.{old,new}_name` to argv[0] and argv[1], since\nparse_options handles advancing argv past the name of the sub-command.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/remote.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 6f27ddc47b..824fb8099c 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -684,11 +684,14 @@ static int mv(int argc, const char **argv)\n \tstruct rename_info rename;\n \tint i, refspec_updated = 0;\n \n-\tif (argc != 3)\n+\targc = parse_options(argc, argv, NULL, options,\n+\t\t\t     builtin_remote_rename_usage, 0);\n+\n+\tif (argc != 2)\n \t\tusage_with_options(builtin_remote_rename_usage, options);\n \n-\trename.old_name = argv[1];\n-\trename.new_name = argv[2];\n+\trename.old_name = argv[0];\n+\trename.new_name = argv[1];\n \trename.remote_branches = &remote_branches;\n \n \toldremote = remote_get(rename.old_name);\n-- \n2.35.1.73.gccc5557600\n\n"},{"id":"450317","messageId":"d5b0a4b71027619123b7284611692d3a9c128518.1646346287.git.me@ttaylorr.com","threadId":"57492","inReplyTo":"cover.1646346286.git.me@ttaylorr.com","subject":"[PATCH v3 2/2] builtin/remote.c: show progress when renaming remote references","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-03T22:25:18Z","receivedAt":"2022-03-03T22:25:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When renaming a remote, Git needs to rename all remote tracking\nreferences to the remote's new name (e.g., renaming\n\"refs/remotes/old/foo\" to \"refs/remotes/new/foo\" when renaming a remote\nfrom \"old\" to \"new\").\n\nThis can be somewhat slow when there are many references to rename,\nsince each rename is done in a separate call to rename_ref() as opposed\nto grouping all renames together into the same transaction. It would be\nnice to execute all renames as a single transaction, but there is a\nsnag: the reference transaction backend doesn't support renames during a\ntransaction (only individually, via rename_ref()).\n\nThe reasons there are described in more detail in [1], but the main\nproblem is that in order to preserve the existing reflog, it must be\nmoved while holding both locks (i.e., on \"oldname\" and \"newname\"), and\nthe ref transaction code doesn't support inserting arbitrary actions\ninto the middle of a transaction like that.\n\nAs an aside, adding support for this to the ref transaction code is\nless straightforward than inserting both a ref_update() and ref_delete()\ncall into the same transaction. rename_ref()'s special handling to\ndetect D/F conflicts would need to be rewritten for the transaction code\nif we wanted to proactively catch D/F conflicts when renaming a\nreference during a transaction. The reftable backend could support this\nmuch more readily because of its lack of D/F conflicts.\n\nInstead of a more complex modification to the ref transaction code,\ndisplay a progress meter when running verbosely in order to convince the\nuser that Git is doing work while renaming a remote.\n\nThis is mostly done as-expected, with the minor caveat that we\nintentionally count symrefs renames twice, since renaming a symref takes\nplace over two separate calls (one to delete the old one, and another to\ncreate the new one).\n\n[1]: https://lore.kernel.org/git/572367B4.4050207@alum.mit.edu/\n\nSuggested-by: brian m. carlson <sandals@crustytoothpaste.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-remote.txt |  2 +-\n builtin/remote.c             | 30 +++++++++++++++++++++++++-----\n t/t5505-remote.sh            |  4 +++-\n 3 files changed, 29 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt\nindex 2bebc32566..cde9614e36 100644\n--- a/Documentation/git-remote.txt\n+++ b/Documentation/git-remote.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git remote' [-v | --verbose]\n 'git remote add' [-t <branch>] [-m <master>] [-f] [--[no-]tags] [--mirror=(fetch|push)] <name> <URL>\n-'git remote rename' <old> <new>\n+'git remote rename' [--[no-]progress] <old> <new>\n 'git remote remove' <name>\n 'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)\n 'git remote set-branches' [--add] <name> <branch>...\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 824fb8099c..950e7958d5 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -12,11 +12,12 @@\n #include \"object-store.h\"\n #include \"strvec.h\"\n #include \"commit-reach.h\"\n+#include \"progress.h\"\n \n static const char * const builtin_remote_usage[] = {\n \t\"git remote [-v | --verbose]\",\n \tN_(\"git remote add [-t <branch>] [-m <master>] [-f] [--tags | --no-tags] [--mirror=<fetch|push>] <name> <url>\"),\n-\tN_(\"git remote rename <old> <new>\"),\n+\tN_(\"git remote rename [--[no-]progress] <old> <new>\"),\n \tN_(\"git remote remove <name>\"),\n \tN_(\"git remote set-head <name> (-a | --auto | -d | --delete | <branch>)\"),\n \tN_(\"git remote [-v | --verbose] show [-n] <name>\"),\n@@ -36,7 +37,7 @@ static const char * const builtin_remote_add_usage[] = {\n };\n \n static const char * const builtin_remote_rename_usage[] = {\n-\tN_(\"git remote rename <old> <new>\"),\n+\tN_(\"git remote rename [--[no-]progress] <old> <new>\"),\n \tNULL\n };\n \n@@ -571,6 +572,7 @@ struct rename_info {\n \tconst char *old_name;\n \tconst char *new_name;\n \tstruct string_list *remote_branches;\n+\tuint32_t symrefs_nr;\n };\n \n static int read_remote_branches(const char *refname,\n@@ -587,10 +589,12 @@ static int read_remote_branches(const char *refname,\n \t\titem = string_list_append(rename->remote_branches, refname);\n \t\tsymref = resolve_ref_unsafe(refname, RESOLVE_REF_READING,\n \t\t\t\t\t    NULL, &flag);\n-\t\tif (symref && (flag & REF_ISSYMREF))\n+\t\tif (symref && (flag & REF_ISSYMREF)) {\n \t\t\titem->util = xstrdup(symref);\n-\t\telse\n+\t\t\trename->symrefs_nr++;\n+\t\t} else {\n \t\t\titem->util = NULL;\n+\t\t}\n \t}\n \tstrbuf_release(&buf);\n \n@@ -674,7 +678,9 @@ static void handle_push_default(const char* old_name, const char* new_name)\n \n static int mv(int argc, const char **argv)\n {\n+\tint show_progress = isatty(2);\n \tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"progress\", &show_progress, N_(\"force progress reporting\")),\n \t\tOPT_END()\n \t};\n \tstruct remote *oldremote, *newremote;\n@@ -682,7 +688,8 @@ static int mv(int argc, const char **argv)\n \t\told_remote_context = STRBUF_INIT;\n \tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n \tstruct rename_info rename;\n-\tint i, refspec_updated = 0;\n+\tint i, refs_renamed_nr = 0, refspec_updated = 0;\n+\tstruct progress *progress = NULL;\n \n \targc = parse_options(argc, argv, NULL, options,\n \t\t\t     builtin_remote_rename_usage, 0);\n@@ -693,6 +700,7 @@ static int mv(int argc, const char **argv)\n \trename.old_name = argv[0];\n \trename.new_name = argv[1];\n \trename.remote_branches = &remote_branches;\n+\trename.symrefs_nr = 0;\n \n \toldremote = remote_get(rename.old_name);\n \tif (!remote_is_configured(oldremote, 1)) {\n@@ -767,6 +775,14 @@ static int mv(int argc, const char **argv)\n \t * the new symrefs.\n \t */\n \tfor_each_ref(read_remote_branches, &rename);\n+\tif (show_progress) {\n+\t\t/*\n+\t\t * Count symrefs twice, since \"renaming\" them is done by\n+\t\t * deleting and recreating them in two separate passes.\n+\t\t */\n+\t\tprogress = start_progress(_(\"Renaming remote references\"),\n+\t\t\t\t\t  rename.remote_branches->nr + rename.symrefs_nr);\n+\t}\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n \t\tint flag = 0;\n@@ -776,6 +792,7 @@ static int mv(int argc, const char **argv)\n \t\t\tcontinue;\n \t\tif (delete_ref(NULL, item->string, NULL, REF_NO_DEREF))\n \t\t\tdie(_(\"deleting '%s' failed\"), item->string);\n+\t\tdisplay_progress(progress, ++refs_renamed_nr);\n \t}\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n@@ -791,6 +808,7 @@ static int mv(int argc, const char **argv)\n \t\t\t\titem->string, buf.buf);\n \t\tif (rename_ref(item->string, buf.buf, buf2.buf))\n \t\t\tdie(_(\"renaming '%s' failed\"), item->string);\n+\t\tdisplay_progress(progress, ++refs_renamed_nr);\n \t}\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n@@ -810,7 +828,9 @@ static int mv(int argc, const char **argv)\n \t\t\t\titem->string, buf.buf);\n \t\tif (create_symref(buf.buf, buf2.buf, buf3.buf))\n \t\t\tdie(_(\"creating '%s' failed\"), buf.buf);\n+\t\tdisplay_progress(progress, ++refs_renamed_nr);\n \t}\n+\tstop_progress(&progress);\n \tstring_list_clear(&remote_branches, 1);\n \n \thandle_push_default(rename.old_name, rename.new_name);\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 9ab315424c..c90cf47acd 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -753,7 +753,9 @@ test_expect_success 'rename a remote' '\n \t(\n \t\tcd four &&\n \t\tgit config branch.main.pushRemote origin &&\n-\t\tgit remote rename origin upstream &&\n+\t\tGIT_TRACE2_EVENT=$(pwd)/trace \\\n+\t\t\tgit remote rename --progress origin upstream &&\n+\t\ttest_region progress \"Renaming remote references\" trace &&\n \t\tgrep \"pushRemote\" .git/config &&\n \t\ttest -z \"$(git for-each-ref refs/remotes/origin)\" &&\n \t\ttest \"$(git symbolic-ref refs/remotes/upstream/HEAD)\" = \"refs/remotes/upstream/main\" &&\n-- \n2.35.1.73.gccc5557600\n"},{"id":"450320","messageId":"xmqqk0da7hdb.fsf@gitster.g","threadId":"57492","inReplyTo":"d5b0a4b71027619123b7284611692d3a9c128518.1646346287.git.me@ttaylorr.com","subject":"Re: [PATCH v3 2/2] builtin/remote.c: show progress when renaming remote references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-03T23:20:48Z","receivedAt":"2022-03-03T23:20:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> Instead of a more complex modification to the ref transaction code,\n> display a progress meter when running verbosely in order to convince the\n> user that Git is doing work while renaming a remote.\n\nIs it still \"when running verbosely\"?\n\nI thought that tying this to --[no-]progress was the whole point of\niterating another round.\n\n\t... when the standard error output is connected to a\n\tterminal, so that user knows Git is not completely stuck.\n\n> This is mostly done as-expected, with the minor caveat that we\n> intentionally count symrefs renames twice, since renaming a symref takes\n> place over two separate calls (one to delete the old one, and another to\n> create the new one).\n\nThat's a nice note to leave here, as it is a bit tricky to reason about.\n\n> [1]: https://lore.kernel.org/git/572367B4.4050207@alum.mit.edu/\n>\n> Suggested-by: brian m. carlson <sandals@crustytoothpaste.net>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n\n\n\n>  Documentation/git-remote.txt |  2 +-\n>  builtin/remote.c             | 30 +++++++++++++++++++++++++-----\n>  t/t5505-remote.sh            |  4 +++-\n>  3 files changed, 29 insertions(+), 7 deletions(-)\n>\n> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt\n> index 2bebc32566..cde9614e36 100644\n> --- a/Documentation/git-remote.txt\n> +++ b/Documentation/git-remote.txt\n> @@ -11,7 +11,7 @@ SYNOPSIS\n>  [verse]\n>  'git remote' [-v | --verbose]\n>  'git remote add' [-t <branch>] [-m <master>] [-f] [--[no-]tags] [--mirror=(fetch|push)] <name> <URL>\n> -'git remote rename' <old> <new>\n> +'git remote rename' [--[no-]progress] <old> <new>\n>  'git remote remove' <name>\n>  'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)\n>  'git remote set-branches' [--add] <name> <branch>...\n\nDo we already have an entry for the --progress option in the\ndescription part of the documentation?  I think the way progress\nworks in the context of this command is pretty much bog-standard\none that we may not have to, once the user has seen how progress\noptions work elsewhere.\n\nIf not, then we'd want something like this squashed in, perhaps?\n\n Documentation/git-remote.txt | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git c/Documentation/git-remote.txt w/Documentation/git-remote.txt\nindex cde9614e36..e04810ee70 100644\n--- c/Documentation/git-remote.txt\n+++ w/Documentation/git-remote.txt\n@@ -83,7 +83,10 @@ will always behave as if `--mirror` was passed.\n 'rename'::\n \n Rename the remote named <old> to <new>. All remote-tracking branches and\n-configuration settings for the remote are updated.\n+configuration settings for the remote are updated.  When used interactively\n+(i.e. the standard error stream is connected to a terminal),\n+a progress bar may be shown while remote-tracking branches are renamed,\n+which can be silenced by passing the `--no-progress` option.\n +\n In case <old> and <new> are the same, and <old> is a file under\n `$GIT_DIR/remotes` or `$GIT_DIR/branches`, the remote is converted to\n"},{"id":"450321","messageId":"YiFPep6PU/7wcEQH@nand.local","threadId":"57492","inReplyTo":"xmqqk0da7hdb.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] builtin/remote.c: show progress when renaming remote references","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-03T23:30:02Z","receivedAt":"2022-03-03T23:30:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 03, 2022 at 03:20:48PM -0800, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > Instead of a more complex modification to the ref transaction code,\n> > display a progress meter when running verbosely in order to convince the\n> > user that Git is doing work while renaming a remote.\n>\n> Is it still \"when running verbosely\"?\n>\n> I thought that tying this to --[no-]progress was the whole point of\n> iterating another round.\n>\n> \t... when the standard error output is connected to a\n> \tterminal, so that user knows Git is not completely stuck.\n\nAh, I glossed over this (stale) reference to the verbose option. I'm\nalmost willing to let it go, since it doesn't mention `--verbose`\ndirectly, but happy to change it, too.\n\n> > This is mostly done as-expected, with the minor caveat that we\n> > intentionally count symrefs renames twice, since renaming a symref takes\n> > place over two separate calls (one to delete the old one, and another to\n> > create the new one).\n>\n> That's a nice note to leave here, as it is a bit tricky to reason about.\n\nThanks, it's the sort of thing that I'd hope to find in a commit message\nif I were confused about something.\n\n> Do we already have an entry for the --progress option in the\n> description part of the documentation?  I think the way progress\n> works in the context of this command is pretty much bog-standard\n> one that we may not have to, once the user has seen how progress\n> options work elsewhere.\n>\n> If not, then we'd want something like this squashed in, perhaps?\n\nI like the suggestion, but I agree it's probably not necessary since\nthis usage is standard. I thought I had written something explaining the\noption explicitly in this section, but apparently dropped it when\npreparing this patch. Ugh.\n\nThanks,\nTaylor\n"},{"id":"450496","messageId":"220305.86tucc3213.gmgdl@evledraar.gmail.com","threadId":"57492","inReplyTo":"b76da50b549d71b3e61f74c6250fea595ee2b2c1.1646346287.git.me@ttaylorr.com","subject":"Re: [PATCH v3 1/2] builtin/remote.c: parse options in 'rename'","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-05T14:28:31Z","receivedAt":"2022-03-05T14:30:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Mar 03 2022, Taylor Blau wrote:\n\n> The 'git remote rename' command doesn't currently take any command-line\n> arguments besides the existing and new name of a remote, and so has no\n> need to call parse_options().\n\nI think nothing needs to change here in the body of the test, but just a\ncorrection: We do in fact use parse_options() here in the pre-image. I.e.:\n\n    git remote -h\n    git remote rename -h\n\nI.e. we'll detect \"-h\", emit the appropriate usage etc.\n"},{"id":"450497","messageId":"220305.86pmn030ou.gmgdl@evledraar.gmail.com","threadId":"57492","inReplyTo":"d5b0a4b71027619123b7284611692d3a9c128518.1646346287.git.me@ttaylorr.com","subject":"Re: [PATCH v3 2/2] builtin/remote.c: show progress when renaming remote references","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-05T14:31:05Z","receivedAt":"2022-03-05T14:59:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Mar 03 2022, Taylor Blau wrote:\n\n> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt\n> index 2bebc32566..cde9614e36 100644\n> --- a/Documentation/git-remote.txt\n> +++ b/Documentation/git-remote.txt\n> @@ -11,7 +11,7 @@ SYNOPSIS\n>  [verse]\n>  'git remote' [-v | --verbose]\n>  'git remote add' [-t <branch>] [-m <master>] [-f] [--[no-]tags] [--mirror=(fetch|push)] <name> <URL>\n> -'git remote rename' <old> <new>\n> +'git remote rename' [--[no-]progress] <old> <new>\n>  'git remote remove' <name>\n>  'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)\n>  'git remote set-branches' [--add] <name> <branch>...\n\nThanks, this looks much better.\n\nBut now that we don't piggy-back on --verbose (which as noted, would\nhave needed to be reworded if we still did) we should add a\n--no-progress, --progress to the OPTIONS section, see e.g.:\n\n    git grep '^--.*progress::' -- Documentation/\n\n> @@ -571,6 +572,7 @@ struct rename_info {\n>  \tconst char *old_name;\n>  \tconst char *new_name;\n>  \tstruct string_list *remote_branches;\n> +\tuint32_t symrefs_nr;\n>  };\n\nI didn't notice this in previous iterations, but why the uint32_t over\nsay a size_t?\n\nThe string_list is \"unsigned int\" (but we should really use size_t\nthere), but there's some code in refs.c that thinks about number of refs\nas size_t already...\n\n>  \n>  static int read_remote_branches(const char *refname,\n> @@ -587,10 +589,12 @@ static int read_remote_branches(const char *refname,\n>  \t\titem = string_list_append(rename->remote_branches, refname);\n>  \t\tsymref = resolve_ref_unsafe(refname, RESOLVE_REF_READING,\n>  \t\t\t\t\t    NULL, &flag);\n> -\t\tif (symref && (flag & REF_ISSYMREF))\n> +\t\tif (symref && (flag & REF_ISSYMREF)) {\n>  \t\t\titem->util = xstrdup(symref);\n> -\t\telse\n> +\t\t\trename->symrefs_nr++;\n> +\t\t} else {\n>  \t\t\titem->util = NULL;\n> +\t\t}\n>  \t}\n>  \tstrbuf_release(&buf);\n\nJust FWIW this could also be, if you prefer to skip the brace additions:\n\t\n\t@@ -588,9 +590,10 @@ static int read_remote_branches(const char *refname,\n\t \t\tsymref = resolve_ref_unsafe(refname, RESOLVE_REF_READING,\n\t \t\t\t\t\t    NULL, &flag);\n\t \t\tif (symref && (flag & REF_ISSYMREF))\n\t-\t\t\titem->util = xstrdup(symref);\n\t+\t\t\trename->symrefs_nr++;\n\t \t\telse\n\t-\t\t\titem->util = NULL;\n\t+\t\t\tsymref = NULL;\n\t+\t\titem->util = xstrdup_or_null(symref);\n\t \t}\n\t \tstrbuf_release(&buf);\n\n> @@ -682,7 +688,8 @@ static int mv(int argc, const char **argv)\n>  \t\told_remote_context = STRBUF_INIT;\n>  \tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n>  \tstruct rename_info rename;\n> -\tint i, refspec_updated = 0;\n> +\tint i, refs_renamed_nr = 0, refspec_updated = 0;\n\nAnother type mixup nit, refs_renamed_nr should be \"size_t\" (as above,\nit's looping over the \"unsigned int\" string_list, but we can just use\nsize_t for future-proofing...)\n\n> +\tstruct progress *progress = NULL;\n>  \n>  \targc = parse_options(argc, argv, NULL, options,\n>  \t\t\t     builtin_remote_rename_usage, 0);\n> @@ -693,6 +700,7 @@ static int mv(int argc, const char **argv)\n>  \trename.old_name = argv[0];\n>  \trename.new_name = argv[1];\n>  \trename.remote_branches = &remote_branches;\n> +\trename.symrefs_nr = 0;\n>  \n>  \toldremote = remote_get(rename.old_name);\n>  \tif (!remote_is_configured(oldremote, 1)) {\n> @@ -767,6 +775,14 @@ static int mv(int argc, const char **argv)\n>  \t * the new symrefs.\n>  \t */\n>  \tfor_each_ref(read_remote_branches, &rename);\n> +\tif (show_progress) {\n> +\t\t/*\n> +\t\t * Count symrefs twice, since \"renaming\" them is done by\n> +\t\t * deleting and recreating them in two separate passes.\n> +\t\t */\n\nI didn't look this over all that carefully before, but is the count that\nwe'll get in rename.symrefs_nr ever different than in\nresolve_ref_unsafe() in read_remote_branches()? If not that's an issue\nthat pre-exists here, i.e. why do we need to find out twice for each ref\nit's a symref?\n\nAnd in any case the \"total\" fed to start_progress() will be wrong since\nin the two later loops we \"continue\" if \"item->util\" is true....\n\n> +\t\tprogress = start_progress(_(\"Renaming remote references\"),\n> +\t\t\t\t\t  rename.remote_branches->nr + rename.symrefs_nr);\n> +\t}\n>  \tfor (i = 0; i < remote_branches.nr; i++) {\n>  \t\tstruct string_list_item *item = remote_branches.items + i;\n>  \t\tint flag = 0;\n> @@ -776,6 +792,7 @@ static int mv(int argc, const char **argv)\n>  \t\t\tcontinue;\n>  \t\tif (delete_ref(NULL, item->string, NULL, REF_NO_DEREF))\n>  \t\t\tdie(_(\"deleting '%s' failed\"), item->string);\n> +\t\tdisplay_progress(progress, ++refs_renamed_nr);\n\n...I think it makes sense to display_progress() at the start of the\nloop, otherwise we expose ourselves to the progress bar stalling if\nwe're just looping over a bunch of stuff where we \"continue\", and it's\neasier to reason about.\n"},{"id":"450517","messageId":"CAFQ2z_Po4erN5Dpd7SJs9qANxNYh6rvcuYwnVdOkb0DbBeFDEw@mail.gmail.com","threadId":"57492","inReplyTo":"220303.8635jz5mcz.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] builtin/remote.c: show progress when renaming remote references","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2022-03-07T10:34:40Z","receivedAt":"2022-03-07T11:12:52Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Thu, Mar 3, 2022 at 12:04 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> >> As an aside I think the reftable code \"emulates\" the D/F conflicts of\n> >> the files backend, but I'm not sure (this is from vague memory).\n> >\n> > Perhaps, though I'm admittedly not familiar enough with that work to\n> > tell know for sure, either. I don't think I have a ton to add to the\n\ncorrect. Look for the 'skip_name_check' option, and reftable/refname.c\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n"}]}