{"thread":{"id":"46077","subject":"[PATCH 0/2] Add progress to format-patch and rebase","startedAt":"2017-05-31T15:04:54Z","lastAt":"2017-08-20T07:43:25Z","messageCount":34,"participants":["Kevin Willford","Stefan Beller","Jeff King","Junio C Hamano","Johannes Schindelin","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"321134","messageId":"20170531150427.7820-1-kewillf@microsoft.com","threadId":"46077","inReplyTo":null,"subject":"[PATCH 0/2] Add progress to format-patch and rebase","fromName":"Kevin Willford","fromEmail":"kcwillford@gmail.com","sentAt":"2017-05-31T15:04:25Z","receivedAt":"2017-05-31T15:04:54Z","isPatch":true,"sender":{"key":"kcwillford@gmail.com","avatar":null},"body":"This patch series is to add progress for the user when generating\npatches in format-patch. This is to aid the user when performing \nlarge rebases and have some indication to the user that progress\nis being made. \n\nThese patches just add the normal progress indication when\ngenerating patches and makes it the default when running rebase.\nIt will also respect the -q[uiet] flag and not show when it\nis present.\n\nKevin Willford (2):\n format-patch: have progress option while generating patches\n rebase: turn on progress option by default for format-patch\n\n Documentation/git-format-patch.txt | 8 ++++++++\n builtin/log.c | 10 ++++++++++\n git-rebase--am.sh | 5 +++--\n git-rebase.sh | 2 ++\n 4 files changed, 23 insertions(+), 2 deletions(-)\n\n-- \n2.13.0.92.g73a4ce6a77\n\n"},{"id":"321135","messageId":"20170531150427.7820-2-kewillf@microsoft.com","threadId":"46077","inReplyTo":"20170531150427.7820-1-kewillf@microsoft.com","subject":"[PATCH 1/2] format-patch: have progress option while generating patches","fromName":"Kevin Willford","fromEmail":"kcwillford@gmail.com","sentAt":"2017-05-31T15:04:26Z","receivedAt":"2017-05-31T15:04:55Z","isPatch":true,"sender":{"key":"kcwillford@gmail.com","avatar":null},"body":"When generating patches for the rebase command if the user does\nnot realize the branch they are rebasing onto is thousands of\ncommits different there is no progress indication after initial\nrewinding message.\n\nThis patch allows a progress option to be passed to format-patch\nso that the user can be informed the progress of generating the\npatch. This option will then be used by the rebase command when\ncalling format-patch.\n\nSigned-off-by: Kevin Willford \n---\n Documentation/git-format-patch.txt | 8 ++++++++\n builtin/log.c | 10 ++++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex c890328b02..ee5f99f606 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -23,6 +23,7 @@ SYNOPSIS\n \t\t [(--reroll-count|-v) ]\n \t\t [--to=] [--cc=]\n \t\t [--[no-]cover-letter] [--quiet] [--notes[=]]\n+\t\t [--progress]\n \t\t []\n \t\t [ | ]\n \n@@ -260,6 +261,7 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.\n -q::\n --quiet::\n \tDo not print the names of the generated files to standard output.\n+\tProgress is not reported to the standard error stream.\n \n --no-binary::\n \tDo not output contents of changes in binary files, instead\n@@ -283,6 +285,12 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.\n \trange are always formatted as creation patches, independently\n \tof this flag.\n \n+--progress::\n+\tProgress status is reported on the standard error stream\n+\tby default when it is attached to a terminal, unless -q\n+\tis specified. This flag forces progress status even if the\n+\tstandard error stream is not directed to a terminal.\n+\n CONFIGURATION\n -------------\n You can specify extra mail header lines to be added to each message,\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 631fbc984f..02c50431b6 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -26,6 +26,7 @@\n #include \"version.h\"\n #include \"mailmap.h\"\n #include \"gpg-interface.h\"\n+#include \"progress.h\"\n \n /* Set a default date-time format for git log (\"log.date\" config variable) */\n static const char *default_date_mode = NULL;\n@@ -1409,6 +1410,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tchar *branch_name = NULL;\n \tchar *base_commit = NULL;\n \tstruct base_tree_info bases;\n+\tint show_progress = 0;\n+\tstruct progress *progress = NULL;\n \n \tconst struct option builtin_format_patch_options[] = {\n \t\t{ OPTION_CALLBACK, 'n', \"numbered\", &numbered, NULL,\n@@ -1480,6 +1483,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tOPT_FILENAME(0, \"signature-file\", &signature_file,\n \t\t\t\tN_(\"add a signature from a file\")),\n \t\tOPT__QUIET(&quiet, N_(\"don't print the patch filenames\")),\n+\t\tOPT_BOOL(0, \"progress\", &show_progress,\n+\t\t\t N_(\"show progress\")),\n \t\tOPT_END()\n \t};\n \n@@ -1739,8 +1744,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tstart_number--;\n \t}\n \trev.add_signoff = do_signoff;\n+\n+\tif (show_progress && !quiet)\n+\t\tprogress = start_progress(_(\"Generating patch\"), total);\n \twhile (0 <= --nr) {\n \t\tint shown;\n+\t\tdisplay_progress(progress, total - nr);\n \t\tcommit = list[nr];\n \t\trev.nr = total - nr + (start_number - 1);\n \t\t/* Make the second and subsequent mails replies to the first */\n@@ -1805,6 +1814,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tif (!use_stdout)\n \t\t\tfclose(rev.diffopt.file);\n \t}\n+\tstop_progress(&progress);\n \tfree(list);\n \tfree(branch_name);\n \tstring_list_clear(&extra_to, 0);\n-- \n2.13.0.92.g73a4ce6a77\n\n"},{"id":"321136","messageId":"20170531150427.7820-3-kewillf@microsoft.com","threadId":"46077","inReplyTo":"20170531150427.7820-1-kewillf@microsoft.com","subject":"[PATCH 2/2] rebase: turn on progress option by default for format-patch","fromName":"Kevin Willford","fromEmail":"kcwillford@gmail.com","sentAt":"2017-05-31T15:04:27Z","receivedAt":"2017-05-31T15:04:57Z","isPatch":true,"sender":{"key":"kcwillford@gmail.com","avatar":null},"body":"This change passes the progress option of format-patch by\ndefault and passes the -q --quiet option through to the\nformat-patch call so that it is respected as well.\n\nSigned-off-by: Kevin Willford \n---\n git-rebase--am.sh | 5 +++--\n git-rebase.sh | 2 ++\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex 375239341f..ab2be30abf 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -51,8 +51,9 @@ then\n else\n \trm -f \"$GIT_DIR/rebased-patches\"\n \n-\tgit format-patch -k --stdout --full-index --cherry-pick --right-only \\\n-\t\t--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \\\n+\tgit format-patch $git_format_patch_opt -k --stdout --full-index \\\n+\t\t--cherry-pick --right-only --src-prefix=a/ --dst-prefix=b/ \\\n+\t\t--no-renames --no-cover-letter --progress \\\n \t\t\"$revisions\" ${restrict_revision+^$restrict_revision} \\\n \t\t>\"$GIT_DIR/rebased-patches\"\n \tret=$?\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex db1deed846..b72e319ac9 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -73,6 +73,7 @@ test \"$(git config --bool rebase.stat)\" = true && diffstat=t\n autostash=\"$(git config --bool rebase.autostash || echo false)\"\n fork_point=auto\n git_am_opt=\n+git_format_patch_opt=\n rebase_root=\n force_rebase=\n allow_rerere_autoupdate=\n@@ -308,6 +309,7 @@ do\n \t--quiet)\n \t\tGIT_QUIET=t\n \t\tgit_am_opt=\"$git_am_opt -q\"\n+\t\tgit_format_patch_opt=\"$git_format_patch_opt -q\"\n \t\tverbose=\n \t\tdiffstat=\n \t\t;;\n-- \n2.13.0.92.g73a4ce6a77\n\n"},{"id":"321151","messageId":"CAGZ79kYVN4JHq273-AWhpFVbOQR=qU6AfPT8O16N+9siacXvqA@mail.gmail.com","threadId":"46077","inReplyTo":"20170531150427.7820-2-kewillf@microsoft.com","subject":"Re: [PATCH 1/2] format-patch: have progress option while generating patches","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-31T18:40:00Z","receivedAt":"2017-05-31T18:40:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 31, 2017 at 8:04 AM, Kevin Willford wrote:\n> When generating patches for the rebase command if the user does\n> not realize the branch they are rebasing onto is thousands of\n> commits different there is no progress indication after initial\n> rewinding message.\n>\n> This patch allows a progress option to be passed to format-patch\n> so that the user can be informed the progress of generating the\n> patch. This option will then be used by the rebase command when\n> calling format-patch.\n\nAfter reading the code, I was looking for some explanation\non the underlying assumptions, such as:\n\n The progress meter as presented in this patch assumes the thousands of\n patches to have a fine granularity as well as assuming to require all the\n same amount of work/time for each, such that a steady progress bar\n is achieved.\n\n We do not want to estimate the time for each patch based e.g.\n on their size or number of touched files (or parents) as that is too\n expensive for just a progress meter.\n\n\n>\n> Signed-off-by: Kevin Willford \n> ---\n> Documentation/git-format-patch.txt | 8 ++++++++\n> builtin/log.c | 10 ++++++++++\n> 2 files changed, 18 insertions(+)\n>\n> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\n> index c890328b02..ee5f99f606 100644\n> --- a/Documentation/git-format-patch.txt\n> +++ b/Documentation/git-format-patch.txt\n> @@ -23,6 +23,7 @@ SYNOPSIS\n> [(--reroll-count|-v) ]\n> [--to=] [--cc=]\n> [--[no-]cover-letter] [--quiet] [--notes[=]]\n> + [--progress]\n> []\n> [ | ]\n>\n> @@ -260,6 +261,7 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.\n> -q::\n> --quiet::\n> Do not print the names of the generated files to standard output.\n> + Progress is not reported to the standard error stream.\n>\n> --no-binary::\n> Do not output contents of changes in binary files, instead\n> @@ -283,6 +285,12 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.\n> range are always formatted as creation patches, independently\n> of this flag.\n>\n> +--progress::\n> + Progress status is reported on the standard error stream\n> + by default when it is attached to a terminal, unless -q\n> + is specified. This flag forces progress status even if the\n> + standard error stream is not directed to a terminal.\n> +\n> CONFIGURATION\n> -------------\n> You can specify extra mail header lines to be added to each message,\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 631fbc984f..02c50431b6 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -26,6 +26,7 @@\n> #include \"version.h\"\n> #include \"mailmap.h\"\n> #include \"gpg-interface.h\"\n> +#include \"progress.h\"\n>\n> /* Set a default date-time format for git log (\"log.date\" config variable) */\n> static const char *default_date_mode = NULL;\n> @@ -1409,6 +1410,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> char *branch_name = NULL;\n> char *base_commit = NULL;\n> struct base_tree_info bases;\n> + int show_progress = 0;\n> + struct progress *progress = NULL;\n>\n> const struct option builtin_format_patch_options[] = {\n> { OPTION_CALLBACK, 'n', \"numbered\", &numbered, NULL,\n> @@ -1480,6 +1483,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> OPT_FILENAME(0, \"signature-file\", &signature_file,\n> N_(\"add a signature from a file\")),\n> OPT__QUIET(&quiet, N_(\"don't print the patch filenames\")),\n> + OPT_BOOL(0, \"progress\", &show_progress,\n> + N_(\"show progress\")),\n> OPT_END()\n> };\n>\n> @@ -1739,8 +1744,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> start_number--;\n> }\n> rev.add_signoff = do_signoff;\n> +\n> + if (show_progress && !quiet)\n> + progress = start_progress(_(\"Generating patch\"), total);\n> while (0 <= --nr) {\n> int shown;\n> + display_progress(progress, total - nr);\n> commit = list[nr];\n> rev.nr = total - nr + (start_number - 1);\n> /* Make the second and subsequent mails replies to the first */\n> @@ -1805,6 +1814,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> if (!use_stdout)\n> fclose(rev.diffopt.file);\n> }\n> + stop_progress(&progress);\n> free(list);\n> free(branch_name);\n> string_list_clear(&extra_to, 0);\n> --\n> 2.13.0.92.g73a4ce6a77\n>\n"},{"id":"321156","messageId":"CAGZ79kYns3CWS5acxGRk67mqdfBvPC=hngnNyovrOkjRRwhRCA@mail.gmail.com","threadId":"46077","inReplyTo":"20170531150427.7820-3-kewillf@microsoft.com","subject":"Re: [PATCH 2/2] rebase: turn on progress option by default for format-patch","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-31T19:08:34Z","receivedAt":"2017-05-31T19:08:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 31, 2017 at 8:04 AM, Kevin Willford wrote:\n> This change passes the progress option of format-patch by\n> default and passes the -q --quiet option through to the\n> format-patch call so that it is respected as well.\n\nThis is not conflicting with Johannes rewrite of rebase in C?\n(rebase is a huge beast IIUC)\n\nWhen passing the progress option by default to formatting patches,\nmaybe we should use start_progress_delay in the previous patch instead\nto omit the progress for short lived patch formatting sessions?\n(say a delay of one second?)\n\nThanks,\nStefan\n\n>\n> Signed-off-by: Kevin Willford \n> ---\n> git-rebase--am.sh | 5 +++--\n> git-rebase.sh | 2 ++\n> 2 files changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-rebase--am.sh b/git-rebase--am.sh\n> index 375239341f..ab2be30abf 100644\n> --- a/git-rebase--am.sh\n> +++ b/git-rebase--am.sh\n> @@ -51,8 +51,9 @@ then\n> else\n> rm -f \"$GIT_DIR/rebased-patches\"\n>\n> - git format-patch -k --stdout --full-index --cherry-pick --right-only \\\n> - --src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \\\n> + git format-patch $git_format_patch_opt -k --stdout --full-index \\\n> + --cherry-pick --right-only --src-prefix=a/ --dst-prefix=b/ \\\n> + --no-renames --no-cover-letter --progress \\\n> \"$revisions\" ${restrict_revision+^$restrict_revision} \\\n> >\"$GIT_DIR/rebased-patches\"\n> ret=$?\n> diff --git a/git-rebase.sh b/git-rebase.sh\n> index db1deed846..b72e319ac9 100755\n> --- a/git-rebase.sh\n> +++ b/git-rebase.sh\n> @@ -73,6 +73,7 @@ test \"$(git config --bool rebase.stat)\" = true && diffstat=t\n> autostash=\"$(git config --bool rebase.autostash || echo false)\"\n> fork_point=auto\n> git_am_opt=\n> +git_format_patch_opt=\n> rebase_root=\n> force_rebase=\n> allow_rerere_autoupdate=\n> @@ -308,6 +309,7 @@ do\n> --quiet)\n> GIT_QUIET=t\n> git_am_opt=\"$git_am_opt -q\"\n> + git_format_patch_opt=\"$git_format_patch_opt -q\"\n> verbose=\n> diffstat=\n> ;;\n> --\n> 2.13.0.92.g73a4ce6a77\n>\n"},{"id":"321158","messageId":"SN2PR03MB23337CC1D9BBB4DF5923D9CBB7F10@SN2PR03MB2333.namprd03.prod.outlook.com","threadId":"46077","inReplyTo":"CAGZ79kYVN4JHq273-AWhpFVbOQR=qU6AfPT8O16N+9siacXvqA@mail.gmail.com","subject":"RE: [PATCH 1/2] format-patch: have progress option while generating patches","fromName":"Kevin Willford","fromEmail":"kewillf@microsoft.com","sentAt":"2017-05-31T19:31:14Z","receivedAt":"2017-05-31T19:31:32Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"> -----Original Message-----\n> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On\n> Behalf Of Stefan Beller\n> Sent: Wednesday, May 31, 2017 12:40 PM\n> To: Kevin Willford \n> Cc: git@vger.kernel.org; Junio C Hamano ; Kevin\n> Willford \n> Subject: Re: [PATCH 1/2] format-patch: have progress option while\n> generating patches\n> \n> On Wed, May 31, 2017 at 8:04 AM, Kevin Willford \n> wrote:\n> > When generating patches for the rebase command if the user does not\n> > realize the branch they are rebasing onto is thousands of commits\n> > different there is no progress indication after initial rewinding\n> > message.\n> >\n> > This patch allows a progress option to be passed to format-patch so\n> > that the user can be informed the progress of generating the patch.\n> > This option will then be used by the rebase command when calling\n> > format-patch.\n> \n> After reading the code, I was looking for some explanation on the underlying\n> assumptions, such as:\n> \n> The progress meter as presented in this patch assumes the thousands of\n> patches to have a fine granularity as well as assuming to require all the\n> same amount of work/time for each, such that a steady progress bar\n> is achieved.\n> \n> We do not want to estimate the time for each patch based e.g.\n> on their size or number of touched files (or parents) as that is too\n> expensive for just a progress meter.\n> \n\nSounds good. I will add some explanation to the commit message.\n\n> \n> >\n> > Signed-off-by: Kevin Willford \n> > ---\n> > Documentation/git-format-patch.txt | 8 ++++++++\n> > builtin/log.c | 10 ++++++++++\n> > 2 files changed, 18 insertions(+)\n> >\n> > diff --git a/Documentation/git-format-patch.txt\n> > b/Documentation/git-format-patch.txt\n> > index c890328b02..ee5f99f606 100644\n> > --- a/Documentation/git-format-patch.txt\n> > +++ b/Documentation/git-format-patch.txt\n> > @@ -23,6 +23,7 @@ SYNOPSIS\n> > [(--reroll-count|-v) ]\n> > [--to=] [--cc=]\n> > [--[no-]cover-letter] [--quiet] [--notes[=]]\n> > + [--progress]\n> > []\n> > [ | ]\n> >\n> > @@ -260,6 +261,7 @@ you can use `--suffix=-patch` to get `0001-\n> description-of-my-change-patch`.\n> > -q::\n> > --quiet::\n> > Do not print the names of the generated files to standard output.\n> > + Progress is not reported to the standard error stream.\n> >\n> > --no-binary::\n> > Do not output contents of changes in binary files, instead @@\n> > -283,6 +285,12 @@ you can use `--suffix=-patch` to get `0001-description-\n> of-my-change-patch`.\n> > range are always formatted as creation patches, independently\n> > of this flag.\n> >\n> > +--progress::\n> > + Progress status is reported on the standard error stream\n> > + by default when it is attached to a terminal, unless -q\n> > + is specified. This flag forces progress status even if the\n> > + standard error stream is not directed to a terminal.\n> > +\n> > CONFIGURATION\n> > -------------\n> > You can specify extra mail header lines to be added to each message,\n> > diff --git a/builtin/log.c b/builtin/log.c index\n> > 631fbc984f..02c50431b6 100644\n> > --- a/builtin/log.c\n> > +++ b/builtin/log.c\n> > @@ -26,6 +26,7 @@\n> > #include \"version.h\"\n> > #include \"mailmap.h\"\n> > #include \"gpg-interface.h\"\n> > +#include \"progress.h\"\n> >\n> > /* Set a default date-time format for git log (\"log.date\" config\n> > variable) */ static const char *default_date_mode = NULL; @@ -1409,6\n> > +1410,8 @@ int cmd_format_patch(int argc, const char **argv, const char\n> *prefix)\n> > char *branch_name = NULL;\n> > char *base_commit = NULL;\n> > struct base_tree_info bases;\n> > + int show_progress = 0;\n> > + struct progress *progress = NULL;\n> >\n> > const struct option builtin_format_patch_options[] = {\n> > { OPTION_CALLBACK, 'n', \"numbered\", &numbered, NULL,\n> > @@ -1480,6 +1483,8 @@ int cmd_format_patch(int argc, const char **argv,\n> const char *prefix)\n> > OPT_FILENAME(0, \"signature-file\", &signature_file,\n> > N_(\"add a signature from a file\")),\n> > OPT__QUIET(&quiet, N_(\"don't print the patch\n> > filenames\")),\n> > + OPT_BOOL(0, \"progress\", &show_progress,\n> > + N_(\"show progress\")),\n> > OPT_END()\n> > };\n> >\n> > @@ -1739,8 +1744,12 @@ int cmd_format_patch(int argc, const char\n> **argv, const char *prefix)\n> > start_number--;\n> > }\n> > rev.add_signoff = do_signoff;\n> > +\n> > + if (show_progress && !quiet)\n> > + progress = start_progress(_(\"Generating patch\"),\n> > + total);\n> > while (0 <= --nr) {\n> > int shown;\n> > + display_progress(progress, total - nr);\n> > commit = list[nr];\n> > rev.nr = total - nr + (start_number - 1);\n> > /* Make the second and subsequent mails replies to the\n> > first */ @@ -1805,6 +1814,7 @@ int cmd_format_patch(int argc, const char\n> **argv, const char *prefix)\n> > if (!use_stdout)\n> > fclose(rev.diffopt.file);\n> > }\n> > + stop_progress(&progress);\n> > free(list);\n> > free(branch_name);\n> > string_list_clear(&extra_to, 0);\n> > --\n> > 2.13.0.92.g73a4ce6a77\n> >\n"},{"id":"321159","messageId":"SN2PR03MB23331225571F62753E88572CB7F10@SN2PR03MB2333.namprd03.prod.outlook.com","threadId":"46077","inReplyTo":"CAGZ79kYns3CWS5acxGRk67mqdfBvPC=hngnNyovrOkjRRwhRCA@mail.gmail.com","subject":"RE: [PATCH 2/2] rebase: turn on progress option by default for format-patch","fromName":"Kevin Willford","fromEmail":"kewillf@microsoft.com","sentAt":"2017-05-31T19:46:48Z","receivedAt":"2017-05-31T19:46:58Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"> -----Original Message-----\n> From: Stefan Beller [mailto:sbeller@google.com]\n> Sent: Wednesday, May 31, 2017 1:09 PM\n> To: Kevin Willford \n> Cc: git@vger.kernel.org; Junio C Hamano ; Kevin\n> Willford \n> Subject: Re: [PATCH 2/2] rebase: turn on progress option by default for\n> format-patch\n> \n> On Wed, May 31, 2017 at 8:04 AM, Kevin Willford \n> wrote:\n> > This change passes the progress option of format-patch by default and\n> > passes the -q --quiet option through to the format-patch call so that\n> > it is respected as well.\n> \n> This is not conflicting with Johannes rewrite of rebase in C?\n> (rebase is a huge beast IIUC)\n\nI will check with Johannes and see what possible conflicts there could be.\nSince these are flags that get passed to the format-patch code, it shouldn't take much to put it in the C code as well.\n\n> \n> When passing the progress option by default to formatting patches, maybe\n> we should use start_progress_delay in the previous patch instead to omit\n> the progress for short lived patch formatting sessions?\n> (say a delay of one second?)\n> \n\nI thought about that and certainly could do it but I have found it nice to have the number of patches that are generated in the output even for a small number or commits. For example when I run a `git rebase master` and expect there to be only 2 commits, the message \"Generating patch: 100% (2/2), done.\" Gives me that good feeling that I did it right and didn't mess something up. I'm good either way though.\n\n> Thanks,\n> Stefan\n> \n> >\n> > Signed-off-by: Kevin Willford \n> > ---\n> > git-rebase--am.sh | 5 +++--\n> > git-rebase.sh | 2 ++\n> > 2 files changed, 5 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/git-rebase--am.sh b/git-rebase--am.sh index\n> > 375239341f..ab2be30abf 100644\n> > --- a/git-rebase--am.sh\n> > +++ b/git-rebase--am.sh\n> > @@ -51,8 +51,9 @@ then\n> > else\n> > rm -f \"$GIT_DIR/rebased-patches\"\n> >\n> > - git format-patch -k --stdout --full-index --cherry-pick --right-only \\\n> > - --src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \\\n> > + git format-patch $git_format_patch_opt -k --stdout --full-index \\\n> > + --cherry-pick --right-only --src-prefix=a/ --dst-prefix=b/ \\\n> > + --no-renames --no-cover-letter --progress \\\n> > \"$revisions\" ${restrict_revision+^$restrict_revision} \\\n> > >\"$GIT_DIR/rebased-patches\"\n> > ret=$?\n> > diff --git a/git-rebase.sh b/git-rebase.sh index\n> > db1deed846..b72e319ac9 100755\n> > --- a/git-rebase.sh\n> > +++ b/git-rebase.sh\n> > @@ -73,6 +73,7 @@ test \"$(git config --bool rebase.stat)\" = true &&\n> > diffstat=t autostash=\"$(git config --bool rebase.autostash || echo false)\"\n> > fork_point=auto\n> > git_am_opt=\n> > +git_format_patch_opt=\n> > rebase_root=\n> > force_rebase=\n> > allow_rerere_autoupdate=\n> > @@ -308,6 +309,7 @@ do\n> > --quiet)\n> > GIT_QUIET=t\n> > git_am_opt=\"$git_am_opt -q\"\n> > + git_format_patch_opt=\"$git_format_patch_opt -q\"\n> > verbose=\n> > diffstat=\n> > ;;\n> > --\n> > 2.13.0.92.g73a4ce6a77\n> >\n"},{"id":"321160","messageId":"CAGZ79kbmd7D5btJO5BWgOnSNUf221tYSYEUkgM9axzRPx7RnAQ@mail.gmail.com","threadId":"46077","inReplyTo":"SN2PR03MB23331225571F62753E88572CB7F10@SN2PR03MB2333.namprd03.prod.outlook.com","subject":"Re: [PATCH 2/2] rebase: turn on progress option by default for format-patch","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-31T20:27:57Z","receivedAt":"2017-05-31T20:28:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 31, 2017 at 12:46 PM, Kevin Willford wrote:\n\n>\n> I thought about that and certainly could do it but I have found it nice to have the number of patches that are generated in the output even for a small number or commits. For example when I run a `git rebase master` and expect there to be only 2 commits, the message \"Generating patch: 100% (2/2), done.\" Gives me that good feeling that I did it right and didn't mess something up. I'm good either way though.\n>\n\nOh I see, that number matching makes sense.\nThough by reading the code, I have the impression\nthat the final value would not change. So if it would take\na really long time for these 2 patches, you'd still get the\n(2/2), to know it was 2 patches indeed.\n\nWhen it goes quickly, I'd be more concerned about\nscreen real estate, driving off the recent history from\nthe screen.\n\nAlso we maybe want to have\n \"Generating patches: 100% (2/2), done.\"\nplural, as (when using the delay)\nit is more likely to kick in for more than just one patch?\n\nSorry, if this comes across as bike shedding.\nIt's meant as food for thought. :)\n\nThanks,\nStefan\n"},{"id":"321205","messageId":"20170531220100.t27w3w642sn33h7s@sigill.intra.peff.net","threadId":"46077","inReplyTo":"20170531150427.7820-2-kewillf@microsoft.com","subject":"Re: [PATCH 1/2] format-patch: have progress option while generating patches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-31T22:01:01Z","receivedAt":"2017-05-31T22:01:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 31, 2017 at 08:04:26AM -0700, Kevin Willford wrote:\n\n> When generating patches for the rebase command if the user does\n> not realize the branch they are rebasing onto is thousands of\n> commits different there is no progress indication after initial\n> rewinding message.\n> \n> This patch allows a progress option to be passed to format-patch\n> so that the user can be informed the progress of generating the\n> patch. This option will then be used by the rebase command when\n> calling format-patch.\n\nI'm generally in favor of progress meters, though it does seem a little\nfunny to me that we'd need one on format-patch. It's basically the same\noperation as \"git log -p\", after all. I guess one difference is that\nusually the output of \"log\" would stream to the pager, so the user would\nsee immediate output. That's true of format-patch, too, but in the\ninstance you care about we're probably doing something more like:\n\n git format-patch \"$@\" >patches\n\nand the user sees nothing.\n\nThat makes me wonder two things:\n\n 1. Should this really be tied to isatty(2), as the documentation\n claims? It seems like you'd really only want it to kick in for\n certain cases. Arguably whenever stdout is _not_ going to a tty\n you'd want progress, but I think probably the caller should\n probably just decide whether to ask for it or not.\n\n 2. Should this apply to other commands in the log family besides\n format-patch? E.g., should \"git log --progress -p >commits\" work?\n\n I added a progress meter to rev-list a while ago (for connectivity\n checks). I don't think we could push this down as far as the\n revision traversal code, because only its callers really understand\n what's the right unit to be counting.\n\n It may not be worth the trouble, though. Only \"format-patch\" does a\n pre-pass to find the total number of commits. So \"git log\n --progress\" would inherently just be counting up, with no clue what\n the final number is.\n\n> @@ -283,6 +285,12 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.\n> \trange are always formatted as creation patches, independently\n> \tof this flag.\n> \n> +--progress::\n> +\tProgress status is reported on the standard error stream\n> +\tby default when it is attached to a terminal, unless -q\n> +\tis specified. This flag forces progress status even if the\n> +\tstandard error stream is not directed to a terminal.\n\nChecking whether stderr is a tty would match our usual progress meters.\nBut I don't actually see any code in the patch implementing this.\n\nAs I said above, I think I'd prefer it to require \"--progress\", as\nformat-patch is quite often used as plumbing. But we'd need to fix the\ndocumentation here to match.\n\n> @@ -1739,8 +1744,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \t\tstart_number--;\n> \t}\n> \trev.add_signoff = do_signoff;\n> +\n> +\tif (show_progress && !quiet)\n> +\t\tprogress = start_progress(_(\"Generating patch\"), total);\n\nThe meter code here all looks correct, but let me bikeshed for a moment. :)\n\nShould this use start_progress_delay()? In most cases the command will\ncomplete very quickly, and the progress report is just noise. For many\ncommands (e.g., checkout) we wait 1-2 seconds before bothering to show\nprogress output.\n\nI would have expected this to say \"Generating patches\"; most of our\nother progress messages are pluralized. You could use Q_() to handle the\nmono/plural case, but I think it's fine to just always say \"patches\"\n(that's what other messages do).\n\nOne final thought is whether callers would want to customize this\nmessage, since it will often be used as plumbing. E.g., would rebase\nwant to say something besides \"Generating patches\". I'm not sure.\nAnyway, if you're interested in that direction, there's prior art in\nthe way rev-list handles \"--progress\" (and its sole caller,\ncheck_connected()).\n\n-Peff\n"},{"id":"321208","messageId":"20170531221151.pxdikxgxy37g7zub@sigill.intra.peff.net","threadId":"46077","inReplyTo":"20170531150427.7820-3-kewillf@microsoft.com","subject":"Re: [PATCH 2/2] rebase: turn on progress option by default for format-patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-31T22:11:51Z","receivedAt":"2017-05-31T22:11:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 31, 2017 at 08:04:27AM -0700, Kevin Willford wrote:\n\n> This change passes the progress option of format-patch by\n> default and passes the -q --quiet option through to the\n> format-patch call so that it is respected as well.\n\nThat makes sense. Is it a bug that we aren't propagating \"-q\" already?\n\nI think the answer is \"no\", because that option only claims to silence\nthe printing of the filenames that format-patch writes. But since we're\nusing --stdout, it wouldn't write those anyway.\n\nCome to think of it, I'm not sure that \"-q\" silencing \"--progress\" in\nyour patch 1 actually makes sense. If you do:\n\n git format-patch -o out/\n\nyou don't need progress, because we're already writing the filenames to\nstdout. So it's only if you did:\n\n git format-patch -o out/ -q\n\nthat you'd actually want progress. But \"-q\" would override any\n--progress option! So I actually think we may be better off keeping the\ntwo as distinct features (especially if we follow the \"no progress\nunless --progress is given\" rule I mentioned in the earlier message).\n\n> diff --git a/git-rebase--am.sh b/git-rebase--am.sh\n> index 375239341f..ab2be30abf 100644\n> --- a/git-rebase--am.sh\n> +++ b/git-rebase--am.sh\n> @@ -51,8 +51,9 @@ then\n> else\n> \trm -f \"$GIT_DIR/rebased-patches\"\n> \n> -\tgit format-patch -k --stdout --full-index --cherry-pick --right-only \\\n> -\t\t--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \\\n> +\tgit format-patch $git_format_patch_opt -k --stdout --full-index \\\n> +\t\t--cherry-pick --right-only --src-prefix=a/ --dst-prefix=b/ \\\n> +\t\t--no-renames --no-cover-letter --progress \\\n> \t\t\"$revisions\" ${restrict_revision+^$restrict_revision} \\\n> \t\t>\"$GIT_DIR/rebased-patches\"\n\nHere we pass --progress unconditionally, which tells format-patch to\noutput progress information. Shouldn't we be checking whether stderr is\na tty before making that decision? And that we're not in --quiet mode?\n\nThat explains why you want to pass \"-q\" in the other hunk; to\ncountermand the explicit --progress here. But if we separate the two as\nI mentioned above, you'd want logic more like:\n\n if test -t 2 && test \"$GIT_QUIET\" != \"t\"\n\tgit_format_patch_opt=\"$git_format_patch_opt --progress\"\n fi\n\n-Peff\n"},{"id":"321243","messageId":"xmqqpoeo73g7.fsf@gitster.mtv.corp.google.com","threadId":"46077","inReplyTo":"20170531220100.t27w3w642sn33h7s@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] format-patch: have progress option while generating patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-01T04:10:48Z","receivedAt":"2017-06-01T04:10:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> As I said above, I think I'd prefer it to require \"--progress\", as\n> format-patch is quite often used as plumbing.\n\nYes, that sounds sensible.\n\nInitially, my reaction was \"Why do we even need --progress for\nformat-patch, when it gives one-line per patch output to show the\nprogress anyway?\", but if that output is redirected to a file, of\ncourse you'd need --progress independently.\n\n\n> Should this use start_progress_delay()? In most cases the command will\n> complete very quickly, and the progress report is just noise. For many\n> commands (e.g., checkout) we wait 1-2 seconds before bothering to show\n> progress output.\n\nIt is better to use the \"delay\" version for progress meters for\ncommands that may or may not last very long, and this may be a good\ncandidate to heed that principle.\n\nThe subcommands that use start_progress() tend to be older and more\nbatch oriented operations, e.g. fsck, pack-objects, etc., that are\nexpected to last longer. It may be a good idea to convert them to\nthe \"delay\" variant, but obviously that is outside the scope of this\npatch.\n\n> I would have expected this to say \"Generating patches\"; most of our\n> other progress messages are pluralized. You could use Q_() to handle the\n> mono/plural case, but I think it's fine to just always say \"patches\"\n> (that's what other messages do).\n>\n> One final thought is whether callers would want to customize this\n> message, since it will often be used as plumbing. E.g., would rebase\n> want to say something besides \"Generating patches\". I'm not sure.\n> Anyway, if you're interested in that direction, there's prior art in\n> the way rev-list handles \"--progress\" (and its sole caller,\n> check_connected()).\n\nThese are all good suggestions.\n"},{"id":"321277","messageId":"alpine.DEB.2.21.1.1706011311290.3610@virtualbox","threadId":"46077","inReplyTo":"CAGZ79kYns3CWS5acxGRk67mqdfBvPC=hngnNyovrOkjRRwhRCA@mail.gmail.com","subject":"Re: [PATCH 2/2] rebase: turn on progress option by default for format-patch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-01T11:11:42Z","receivedAt":"2017-06-01T11:12:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Wed, 31 May 2017, Stefan Beller wrote:\n\n> On Wed, May 31, 2017 at 8:04 AM, Kevin Willford wrote:\n> > This change passes the progress option of format-patch by\n> > default and passes the -q --quiet option through to the\n> > format-patch call so that it is respected as well.\n> \n> This is not conflicting with Johannes rewrite of rebase in C?\n> (rebase is a huge beast IIUC)\n\nNo.\n\nCiao,\nDscho\n"},{"id":"321278","messageId":"alpine.DEB.2.21.1.1706011312510.3610@virtualbox","threadId":"46077","inReplyTo":"20170531220100.t27w3w642sn33h7s@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] format-patch: have progress option while generating patches","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-06-01T11:15:57Z","receivedAt":"2017-06-01T11:16:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 31 May 2017, Jeff King wrote:\n\n> I'm generally in favor of progress meters, though it does seem a little\n> funny to me that we'd need one on format-patch.\n\nWhen working with huge repositories with a large number of branches, it is\nall too easy to pick the wrong branch to rebase to. In that case, it can\ntake a long time for the `git rebase` call to even generate the mbox.\n\nKevin's patch is a visual indication when this is happening.\n\nInstead of being puzzled by `git rebase` \"hanging\", the user will now see\nthat (.../21957) patch is generated and will have a chance to say \"wait a\nminute, *that* many patches? I think I meant a different branch\".\n\nAnd since format-patch generates the patches for `git rebase` (actually,\nit generates an mbox that then has to be split again, but you get the\ngist), it is format-patch that needs to output the progress.\n\nCiao,\ndscho\n"},{"id":"321295","messageId":"20170601155426.hx7ximvrvqmdteoj@sigill.intra.peff.net","threadId":"46077","inReplyTo":"alpine.DEB.2.21.1.1706011312510.3610@virtualbox","subject":"Re: [PATCH 1/2] format-patch: have progress option while generating patches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-01T15:54:26Z","receivedAt":"2017-06-01T15:54:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 01, 2017 at 01:15:57PM +0200, Johannes Schindelin wrote:\n\n> On Wed, 31 May 2017, Jeff King wrote:\n> \n> > I'm generally in favor of progress meters, though it does seem a little\n> > funny to me that we'd need one on format-patch.\n> \n> When working with huge repositories with a large number of branches, it is\n> all too easy to pick the wrong branch to rebase to. In that case, it can\n> take a long time for the `git rebase` call to even generate the mbox.\n> \n> Kevin's patch is a visual indication when this is happening.\n\nRight, sorry if it wasn't clear from the rest of my message. My\nstatement was one of surprise, not objection. I understand his use case.\n\n-Peff\n"},{"id":"321489","messageId":"xmqqmv9ows7h.fsf@gitster.mtv.corp.google.com","threadId":"46077","inReplyTo":"20170531221151.pxdikxgxy37g7zub@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] rebase: turn on progress option by default for format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-03T23:45:54Z","receivedAt":"2017-06-03T23:46:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ...\n> That explains why you want to pass \"-q\" in the other hunk; to\n> countermand the explicit --progress here. But if we separate the two as\n> I mentioned above, you'd want logic more like:\n>\n> if test -t 2 && test \"$GIT_QUIET\" != \"t\"\n> \tgit_format_patch_opt=\"$git_format_patch_opt --progress\"\n> fi\n\nThis looks like a sensible line of thought. Kevin, do you want to\ntry this again with the \"explicit --progress option\" approach? I\nthink the gist of the v1 review is that everybody agrees that the\nend goal of showing a progress eye-candy from a long-running\n\"format-patch\" phase in a \"rebase\" is a good idea, but it is dubious\n\"format-patch\" should show it in cases other than when --progress is\nexplicitly asked for.\n\nThanks.\n"},{"id":"326054","messageId":"20170810183256.12668-1-kewillf@microsoft.com","threadId":"46077","inReplyTo":"20170531150427.7820-1-kewillf@microsoft.com","subject":"[PATCH v2 0/2] Add progress for format-patch and rebase","fromName":"Kevin Willford","fromEmail":"kcwillford@gmail.com","sentAt":"2017-08-10T18:32:54Z","receivedAt":"2017-08-10T18:33:20Z","isPatch":true,"sender":{"key":"kcwillford@gmail.com","avatar":null},"body":"Changes since last patch:\n1. Use start_progress_delay so progress isn't shown if generating\n the patches is fast enough\n2. Updated to have text of \"Generating patches\"\n3. Only show progress when the --progress flag is passed\n4. In the rebase script check stderr and the quiet option is not\n set before propagating the progress flag to format-patch\n\nKevin Willford (2):\n format-patch: have progress option while generating patches\n rebase: turn on progress option by default for format-patch\n\n Documentation/git-format-patch.txt | 4 ++++\n builtin/log.c | 10 ++++++++++\n git-rebase--am.sh | 1 +\n git-rebase.sh | 6 ++++++\n 4 files changed, 21 insertions(+)\n\n-- \n2.14.0.rc0.286.g44127d70e4\n\n"},{"id":"326055","messageId":"20170810183256.12668-3-kewillf@microsoft.com","threadId":"46077","inReplyTo":"20170810183256.12668-1-kewillf@microsoft.com","subject":"[PATCH v2 2/2] rebase: turn on progress option by default for format-patch","fromName":"Kevin Willford","fromEmail":"kcwillford@gmail.com","sentAt":"2017-08-10T18:32:56Z","receivedAt":"2017-08-10T18:33:23Z","isPatch":true,"sender":{"key":"kcwillford@gmail.com","avatar":null},"body":"This change passes the progress option of format-patch checking\nthat stderr is attached and rebase is not being run in quiet mode.\n\nSigned-off-by: Kevin Willford \n---\n git-rebase--am.sh | 1 +\n git-rebase.sh | 6 ++++++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex 375239341f..ff98fe3a73 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -53,6 +53,7 @@ else\n \n \tgit format-patch -k --stdout --full-index --cherry-pick --right-only \\\n \t\t--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \\\n+\t\t$git_format_patch_opt \\\n \t\t\"$revisions\" ${restrict_revision+^$restrict_revision} \\\n \t\t>\"$GIT_DIR/rebased-patches\"\n \tret=$?\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex f8b3d1fd97..ad8415e3cf 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -74,6 +74,7 @@ test \"$(git config --bool rebase.stat)\" = true && diffstat=t\n autostash=\"$(git config --bool rebase.autostash || echo false)\"\n fork_point=auto\n git_am_opt=\n+git_format_patch_opt=\n rebase_root=\n force_rebase=\n allow_rerere_autoupdate=\n@@ -445,6 +446,11 @@ else\n \tstate_dir=\"$apply_dir\"\n fi\n \n+if test -t 2 && test -z \"$GIT_QUIET\"\n+then\n+\tgit_format_patch_opt=\"$git_format_patch_opt --progress\"\n+fi\n+\n if test -z \"$rebase_root\"\n then\n \tcase \"$#\" in\n-- \n2.14.0.rc0.286.g44127d70e4\n\n"},{"id":"326056","messageId":"20170810183256.12668-2-kewillf@microsoft.com","threadId":"46077","inReplyTo":"20170810183256.12668-1-kewillf@microsoft.com","subject":"[PATCH v2 1/2] format-patch: have progress option while generating patches","fromName":"Kevin Willford","fromEmail":"kcwillford@gmail.com","sentAt":"2017-08-10T18:32:55Z","receivedAt":"2017-08-10T18:33:26Z","isPatch":true,"sender":{"key":"kcwillford@gmail.com","avatar":null},"body":"When generating patches for the rebase command if the user does\nnot realize the branch they are rebasing onto is thousands of\ncommits different there is no progress indication after initial\nrewinding message.\n\nThe progress meter as presented in this patch assumes the thousands of\npatches to have a fine granularity as well as assuming to require all the\nsame amount of work/time for each, such that a steady progress bar\nis achieved.\n\nWe do not want to estimate the time for each patch based e.g.\non their size or number of touched files (or parents) as that is too\nexpensive for just a progress meter.\n\nThis patch allows a progress option to be passed to format-patch\nso that the user can be informed the progress of generating the\npatch. This option is then used by the rebase command when\ncalling format-patch.\n\nSigned-off-by: Kevin Willford \n---\n Documentation/git-format-patch.txt | 4 ++++\n builtin/log.c | 10 ++++++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex c890328b02..6cbe462a77 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -23,6 +23,7 @@ SYNOPSIS\n \t\t [(--reroll-count|-v) ]\n \t\t [--to=] [--cc=]\n \t\t [--[no-]cover-letter] [--quiet] [--notes[=]]\n+\t\t [--progress]\n \t\t []\n \t\t [ | ]\n \n@@ -283,6 +284,9 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.\n \trange are always formatted as creation patches, independently\n \tof this flag.\n \n+--progress::\n+\tShow progress reports on stderr as patches are generated.\n+\n CONFIGURATION\n -------------\n You can specify extra mail header lines to be added to each message,\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 725c7b8a1a..b07a5529c2 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -27,6 +27,7 @@\n #include \"version.h\"\n #include \"mailmap.h\"\n #include \"gpg-interface.h\"\n+#include \"progress.h\"\n \n /* Set a default date-time format for git log (\"log.date\" config variable) */\n static const char *default_date_mode = NULL;\n@@ -1422,6 +1423,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tchar *branch_name = NULL;\n \tchar *base_commit = NULL;\n \tstruct base_tree_info bases;\n+\tint show_progress = 0;\n+\tstruct progress *progress = NULL;\n \n \tconst struct option builtin_format_patch_options[] = {\n \t\t{ OPTION_CALLBACK, 'n', \"numbered\", &numbered, NULL,\n@@ -1493,6 +1496,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tOPT_FILENAME(0, \"signature-file\", &signature_file,\n \t\t\t\tN_(\"add a signature from a file\")),\n \t\tOPT__QUIET(&quiet, N_(\"don't print the patch filenames\")),\n+\t\tOPT_BOOL(0, \"progress\", &show_progress,\n+\t\t\t N_(\"show progress while generating patches\")),\n \t\tOPT_END()\n \t};\n \n@@ -1752,8 +1757,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tstart_number--;\n \t}\n \trev.add_signoff = do_signoff;\n+\n+\tif (show_progress)\n+\t\tprogress = start_progress_delay(_(\"Generating patches\"), total, 0, 1);\n \twhile (0 <= --nr) {\n \t\tint shown;\n+\t\tdisplay_progress(progress, total - nr);\n \t\tcommit = list[nr];\n \t\trev.nr = total - nr + (start_number - 1);\n \t\t/* Make the second and subsequent mails replies to the first */\n@@ -1818,6 +1827,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tif (!use_stdout)\n \t\t\tfclose(rev.diffopt.file);\n \t}\n+\tstop_progress(&progress);\n \tfree(list);\n \tfree(branch_name);\n \tstring_list_clear(&extra_to, 0);\n-- \n2.14.0.rc0.286.g44127d70e4\n\n"},{"id":"326114","messageId":"xmqqbmnn11kw.fsf@gitster.mtv.corp.google.com","threadId":"46077","inReplyTo":"20170810183256.12668-1-kewillf@microsoft.com","subject":"Re: [PATCH v2 0/2] Add progress for format-patch and rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-10T22:48:31Z","receivedAt":"2017-08-10T22:48:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Willford writes:\n\n> Changes since last patch:\n> 1. Use start_progress_delay so progress isn't shown if generating\n> the patches is fast enough\n> 2. Updated to have text of \"Generating patches\"\n> 3. Only show progress when the --progress flag is passed\n> 4. In the rebase script check stderr and the quiet option is not\n> set before propagating the progress flag to format-patch\n>\n> Kevin Willford (2):\n> format-patch: have progress option while generating patches\n> rebase: turn on progress option by default for format-patch\n\nDo you have a pointer to the previous discussion handy (if you do\nnot, that is OK---I think I can dig the list archive myself)?\n\nAlso, your patch messages appear from you at gmail yet signed off by\nyou at microsoft. Please make them consistent (I can fix them up\nwhile queuing _this_ time, but I do not want forever doing that for\nfuture patches from you or other people). One easy workaround to do\nso without convincing your mailer to put your microsoft address on\nthe sender's From: line in the e-mail is to put an extra\n\n From: Kevin Without \n\nline, followed by a blank line, at the very beginning of the body of\nthe e-mail message.\n\nThanks.\n\n"},{"id":"326119","messageId":"20170810231706.vixs6wms5cpwwsab@sigill.intra.peff.net","threadId":"46077","inReplyTo":"xmqqbmnn11kw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/2] Add progress for format-patch and rebase","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-10T23:17:06Z","receivedAt":"2017-08-10T23:17:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 10, 2017 at 03:48:31PM -0700, Junio C Hamano wrote:\n\n> Kevin Willford writes:\n> \n> > Changes since last patch:\n> > 1. Use start_progress_delay so progress isn't shown if generating\n> > the patches is fast enough\n> > 2. Updated to have text of \"Generating patches\"\n> > 3. Only show progress when the --progress flag is passed\n> > 4. In the rebase script check stderr and the quiet option is not\n> > set before propagating the progress flag to format-patch\n> >\n> > Kevin Willford (2):\n> > format-patch: have progress option while generating patches\n> > rebase: turn on progress option by default for format-patch\n> \n> Do you have a pointer to the previous discussion handy (if you do\n> not, that is OK---I think I can dig the list archive myself)?\n\nhttps://public-inbox.org/git/20170531150427.7820-1-kewillf@microsoft.com/\n\nis what I turned up.\n\nOverall this version looks good to me, and addresses all of the previous\npoints. I have two minor points which I'll make inline.\n\n-Peff\n"},{"id":"326121","messageId":"20170810232033.46ujnozvnodkguog@sigill.intra.peff.net","threadId":"46077","inReplyTo":"20170810183256.12668-2-kewillf@microsoft.com","subject":"Re: [PATCH v2 1/2] format-patch: have progress option while generating patches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-10T23:20:34Z","receivedAt":"2017-08-10T23:20:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 10, 2017 at 02:32:55PM -0400, Kevin Willford wrote:\n\n> @@ -1493,6 +1496,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \t\tOPT_FILENAME(0, \"signature-file\", &signature_file,\n> \t\t\t\tN_(\"add a signature from a file\")),\n> \t\tOPT__QUIET(&quiet, N_(\"don't print the patch filenames\")),\n> +\t\tOPT_BOOL(0, \"progress\", &show_progress,\n> +\t\t\t N_(\"show progress while generating patches\")),\n\nEarlier I suggested allowing --progress=\"custom text\" since this may be\ndriven as plumbing for other commands. But I don't think there's any\nneed to worry about it now. It can be added seamlessly later if we find\nsuch a caller.\n\n> @@ -1752,8 +1757,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \t\tstart_number--;\n> \t}\n> \trev.add_signoff = do_signoff;\n> +\n> +\tif (show_progress)\n> +\t\tprogress = start_progress_delay(_(\"Generating patches\"), total, 0, 1);\n\nI don't really have an opinion on a 1 second delay versus 2. I thought\nwe used 2 pretty consistently, though grepping around I do see a couple\nof 1's. It probably doesn't matter, but just a curiosity.\n\n-Peff\n"},{"id":"326175","messageId":"xmqqwp69ycim.fsf@gitster.mtv.corp.google.com","threadId":"46077","inReplyTo":"20170810232033.46ujnozvnodkguog@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] format-patch: have progress option while generating patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-11T22:18:09Z","receivedAt":"2017-08-11T22:18:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Aug 10, 2017 at 02:32:55PM -0400, Kevin Willford wrote:\n>\n>> @@ -1493,6 +1496,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>> \t\tOPT_FILENAME(0, \"signature-file\", &signature_file,\n>> \t\t\t\tN_(\"add a signature from a file\")),\n>> \t\tOPT__QUIET(&quiet, N_(\"don't print the patch filenames\")),\n>> +\t\tOPT_BOOL(0, \"progress\", &show_progress,\n>> +\t\t\t N_(\"show progress while generating patches\")),\n>\n> Earlier I suggested allowing --progress=\"custom text\" since this may be\n> driven as plumbing for other commands. But I don't think there's any\n> need to worry about it now. It can be added seamlessly later if we find\n> such a caller.\n>\n>> @@ -1752,8 +1757,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>> \t\tstart_number--;\n>> \t}\n>> \trev.add_signoff = do_signoff;\n>> +\n>> +\tif (show_progress)\n>> +\t\tprogress = start_progress_delay(_(\"Generating patches\"), total, 0, 1);\n>\n> I don't really have an opinion on a 1 second delay versus 2. I thought\n> we used 2 pretty consistently, though grepping around I do see a couple\n> of 1's. It probably doesn't matter, but just a curiosity.\n\nYeah, I also thought 2-second was what we used by default. Perhaps\nwe would want to bring others in line?\n"},{"id":"326176","messageId":"xmqqshgxycal.fsf@gitster.mtv.corp.google.com","threadId":"46077","inReplyTo":"20170810183256.12668-3-kewillf@microsoft.com","subject":"Re: [PATCH v2 2/2] rebase: turn on progress option by default for format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-11T22:22:58Z","receivedAt":"2017-08-11T22:23:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Willford writes:\n\n> This change passes the progress option of format-patch checking\n> that stderr is attached and rebase is not being run in quiet mode.\n\n Pass the \"--progress\" option to format-patch when the standard\n error stream goes to the terminal and the command is not run in\n \"--quiet\" mode.\n\nThanks for posting a refreshed version. Both patches make sense to\nme.\n\n>\n> Signed-off-by: Kevin Willford \n\nWe want to see our authors and signed-off match. Do you want the\nemployer address appear in our \"shortlog -e\" output, or your\npersonal gmail address? I'll tenatively \"fix\" your author address\nto match the one at @microsoft.com while queuing.\n\n> ---\n> git-rebase--am.sh | 1 +\n> git-rebase.sh | 6 ++++++\n> 2 files changed, 7 insertions(+)\n>\n> diff --git a/git-rebase--am.sh b/git-rebase--am.sh\n> index 375239341f..ff98fe3a73 100644\n> --- a/git-rebase--am.sh\n> +++ b/git-rebase--am.sh\n> @@ -53,6 +53,7 @@ else\n> \n> \tgit format-patch -k --stdout --full-index --cherry-pick --right-only \\\n> \t\t--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \\\n> +\t\t$git_format_patch_opt \\\n> \t\t\"$revisions\" ${restrict_revision+^$restrict_revision} \\\n> \t\t>\"$GIT_DIR/rebased-patches\"\n> \tret=$?\n> diff --git a/git-rebase.sh b/git-rebase.sh\n> index f8b3d1fd97..ad8415e3cf 100755\n> --- a/git-rebase.sh\n> +++ b/git-rebase.sh\n> @@ -74,6 +74,7 @@ test \"$(git config --bool rebase.stat)\" = true && diffstat=t\n> autostash=\"$(git config --bool rebase.autostash || echo false)\"\n> fork_point=auto\n> git_am_opt=\n> +git_format_patch_opt=\n> rebase_root=\n> force_rebase=\n> allow_rerere_autoupdate=\n> @@ -445,6 +446,11 @@ else\n> \tstate_dir=\"$apply_dir\"\n> fi\n> \n> +if test -t 2 && test -z \"$GIT_QUIET\"\n> +then\n> +\tgit_format_patch_opt=\"$git_format_patch_opt --progress\"\n> +fi\n> +\n> if test -z \"$rebase_root\"\n> then\n> \tcase \"$#\" in\n"},{"id":"326206","messageId":"EA124B72FA7542DBA1C31213235F1B94@PhilipOakley","threadId":"46077","inReplyTo":"xmqqwp69ycim.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/2] format-patch: have progress option while generating patches","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2017-08-12T08:06:18Z","receivedAt":"2017-08-12T08:59:41Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" \n> Jeff King writes:\n>\n>> On Thu, Aug 10, 2017 at 02:32:55PM -0400, Kevin Willford wrote:\n>>\n>>> @@ -1493,6 +1496,8 @@ int cmd_format_patch(int argc, const char **argv, \n>>> const char *prefix)\n>>> OPT_FILENAME(0, \"signature-file\", &signature_file,\n>>> N_(\"add a signature from a file\")),\n>>> OPT__QUIET(&quiet, N_(\"don't print the patch filenames\")),\n>>> + OPT_BOOL(0, \"progress\", &show_progress,\n>>> + N_(\"show progress while generating patches\")),\n>>\n>> Earlier I suggested allowing --progress=\"custom text\" since this may be\n>> driven as plumbing for other commands. But I don't think there's any\n>> need to worry about it now. It can be added seamlessly later if we find\n>> such a caller.\n>>\n>>> @@ -1752,8 +1757,12 @@ int cmd_format_patch(int argc, const char **argv, \n>>> const char *prefix)\n>>> start_number--;\n>>> }\n>>> rev.add_signoff = do_signoff;\n>>> +\n>>> + if (show_progress)\n>>> + progress = start_progress_delay(_(\"Generating patches\"), total, 0, 1);\n>>\n>> I don't really have an opinion on a 1 second delay versus 2. I thought\n>> we used 2 pretty consistently, though grepping around I do see a couple\n>> of 1's. It probably doesn't matter, but just a curiosity.\n>\n> Yeah, I also thought 2-second was what we used by default. Perhaps\n> we would want to bring others in line?\n\n Surely the choice should depend on the purpose of the delay. IIRC \nthe 1 second between patches on the formal 'sent' time was simply to ensure \nthe patches had the right sequence. Delays for warnings and progress have \ndifferent purposes, so I think it's more about the purpose than \nstandardising the time (along with expectations [least surprise] if other \nmessages are displayed).\n--\nPhilip \n\n"},{"id":"326230","messageId":"20170813043940.muj7z3dvl3nh4k6a@sigill.intra.peff.net","threadId":"46077","inReplyTo":"EA124B72FA7542DBA1C31213235F1B94@PhilipOakley","subject":"Re: [PATCH v2 1/2] format-patch: have progress option while generating patches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-13T04:39:40Z","receivedAt":"2017-08-13T04:39:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 12, 2017 at 09:06:18AM +0100, Philip Oakley wrote:\n\n> > > > + progress = start_progress_delay(_(\"Generating patches\"), total, 0, 1);\n> > > \n> > > I don't really have an opinion on a 1 second delay versus 2. I thought\n> > > we used 2 pretty consistently, though grepping around I do see a couple\n> > > of 1's. It probably doesn't matter, but just a curiosity.\n> > \n> > Yeah, I also thought 2-second was what we used by default. Perhaps\n> > we would want to bring others in line?\n> \n> Surely the choice should depend on the purpose of the delay. IIRC\n> the 1 second between patches on the formal 'sent' time was simply to ensure\n> the patches had the right sequence. Delays for warnings and progress have\n> different purposes, so I think it's more about the purpose than\n> standardising the time (along with expectations [least surprise] if other\n> messages are displayed).\n\nI think you're confusing two unrelated things. There's a 1-second fake\ntime-advance on patches, but that's done by send-email, not\nformat-patch.\n\nHere we're just talking about calls to start_progress_delay(), and how\nlong it waits before deciding that the operation is slow enough to show\nprogress. Blame, rename detection, and checkout use 1 second. Prune,\nprune-packed, and connectivity checks all use 2 seconds. I doubt it\nmatters all that much, but presumably the purpose in all is \"how long\nbefore a user starts to wonder if things are actually happening\", which\nis probably command-independent.\n\n-Peff\n"},{"id":"326275","messageId":"xmqqpobyw11t.fsf@gitster.mtv.corp.google.com","threadId":"46077","inReplyTo":"20170813043940.muj7z3dvl3nh4k6a@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] format-patch: have progress option while generating patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-14T16:45:34Z","receivedAt":"2017-08-14T16:45:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Aug 12, 2017 at 09:06:18AM +0100, Philip Oakley wrote:\n>\n>> > > > + progress = start_progress_delay(_(\"Generating patches\"), total, 0, 1);\n>> > > \n>> > > I don't really have an opinion on a 1 second delay versus 2. I thought\n>> > > we used 2 pretty consistently, though grepping around I do see a couple\n>> > > of 1's. It probably doesn't matter, but just a curiosity.\n>> > \n> ...\n> Here we're just talking about calls to start_progress_delay(), and how\n> long it waits before deciding that the operation is slow enough to show\n> progress. Blame, rename detection, and checkout use 1 second. Prune,\n> prune-packed, and connectivity checks all use 2 seconds. I doubt it\n> matters all that much, but presumably the purpose in all is \"how long\n> before a user starts to wonder if things are actually happening\", which\n> is probably command-independent.\n\nI feel comfortable moving forward, basing our decisions on the\nassumption that the \"delay before the user wonders is independent of\nthe command\" without anybody actually proving it.\n\nEven though I think that it is natural for people to expect longer\ndelay from some operation than others (due to some chicken-and-egg\nreasons), trying to scientifically measure and to come up with\ndifferent delay value that suited for each and every operation is\nwaste of our brain cycles.\n\nPerhaps we may want to replace the calls to progress_delay() with a\ncall to a simpler wrapper that does not let the callers give their\nown delay threashold to simplify the API.\n"},{"id":"326294","messageId":"xmqqshguuhe2.fsf@gitster.mtv.corp.google.com","threadId":"46077","inReplyTo":"xmqqpobyw11t.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/2] format-patch: have progress option while generating patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-14T18:35:33Z","receivedAt":"2017-08-14T18:35:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Perhaps we may want to replace the calls to progress_delay() with a\n> call to a simpler wrapper that does not let the callers give their\n> own delay threashold to simplify the API.\n\n... which does not look too bad, but because it makes me wonder if\nwe even need to make the delay-threshold customizable per callers,\nI'll wait further discussions before committing to the approach.\n\nFor example, I do not quite understand why 95 is a good value for\nprune-packed while 0 is a good value for prune. \n\nThe rename detection in diffcore-rename, delay in blame, and\ncheckout (via unpack-trees) all of which use 50-percent threshold\nwith 1 second delay, sort of make sense to me in that if we\ncompleted 50 percent within 1 second, it is likely we will finish\nall in 2 seconds (which is the norm for everybody else), perhaps as\na better version of 0-percent 2 seconds rule.\n\n builtin/blame.c | 3 +--\n builtin/fsck.c | 2 +-\n builtin/prune-packed.c | 4 ++--\n builtin/prune.c | 2 +-\n builtin/rev-list.c | 2 +-\n diffcore-rename.c | 4 ++--\n progress.c | 10 ++++++++--\n progress.h | 4 ++--\n unpack-trees.c | 3 +--\n 9 files changed, 19 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex bda1a78726..05115900f3 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -925,8 +925,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tsb.found_guilty_entry = &found_guilty_entry;\n \tsb.found_guilty_entry_data = π\n \tif (show_progress)\n-\t\tpi.progress = start_progress_delay(_(\"Blaming lines\"),\n-\t\t\t\t\t\t sb.num_lines, 50, 1);\n+\t\tpi.progress = start_delayed_progress(_(\"Blaming lines\"), sb.num_lines, 50);\n \n \tassign_blame(&sb, opt);\n \ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 99dea7adf6..6a26079ebc 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -188,7 +188,7 @@ static int traverse_reachable(void)\n \tunsigned int nr = 0;\n \tint result = 0;\n \tif (show_progress)\n-\t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n+\t\tprogress = start_delayed_progress(_(\"Checking connectivity\"), 0, 0);\n \twhile (pending.nr) {\n \t\tstruct object_array_entry *entry;\n \t\tstruct object *obj;\ndiff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\nindex ac978ad401..4fc6b175de 100644\n--- a/builtin/prune-packed.c\n+++ b/builtin/prune-packed.c\n@@ -37,8 +37,8 @@ static int prune_object(const struct object_id *oid, const char *path,\n void prune_packed_objects(int opts)\n {\n \tif (opts & PRUNE_PACKED_VERBOSE)\n-\t\tprogress = start_progress_delay(_(\"Removing duplicate objects\"),\n-\t\t\t256, 95, 2);\n+\t\tprogress = start_delayed_progress(_(\"Removing duplicate objects\"),\n+\t\t\t\t\t\t 256, 95);\n \n \tfor_each_loose_file_in_objdir(get_object_directory(),\n \t\t\t\t prune_object, NULL, prune_subdir, &opts);\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex c378690545..f32adaa0e9 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -138,7 +138,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \tif (show_progress == -1)\n \t\tshow_progress = isatty(2);\n \tif (show_progress)\n-\t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n+\t\tprogress = start_delayed_progress(_(\"Checking connectivity\"), 0, 0);\n \n \tmark_reachable_objects(&revs, 1, expire, progress);\n \tstop_progress(&progress);\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 95d84d5cda..5cfc4b35f4 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -364,7 +364,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\trevs.limited = 1;\n \n \tif (show_progress)\n-\t\tprogress = start_progress_delay(show_progress, 0, 0, 2);\n+\t\tprogress = start_delayed_progress(show_progress, 0, 0);\n \n \tif (use_bitmap_index && !revs.prune) {\n \t\tif (revs.count && !revs.left_right && !revs.cherry_mark) {\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 786f389498..0eafa43618 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -532,9 +532,9 @@ void diffcore_rename(struct diff_options *options)\n \t}\n \n \tif (options->show_rename_progress) {\n-\t\tprogress = start_progress_delay(\n+\t\tprogress = start_delayed_progress(\n \t\t\t\t_(\"Performing inexact rename detection\"),\n-\t\t\t\trename_dst_nr * rename_src_nr, 50, 1);\n+\t\t\t\trename_dst_nr * rename_src_nr, 50);\n \t}\n \n \tmx = xcalloc(st_mult(NUM_CANDIDATE_PER_DST, num_create), sizeof(*mx));\ndiff --git a/progress.c b/progress.c\nindex 73e36d4a42..eeebe15b6f 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -205,8 +205,8 @@ int display_progress(struct progress *progress, unsigned n)\n \treturn progress ? display(progress, n, NULL) : 0;\n }\n \n-struct progress *start_progress_delay(const char *title, unsigned total,\n-\t\t\t\t unsigned percent_treshold, unsigned delay)\n+static struct progress *start_progress_delay(const char *title, unsigned total,\n+\t\t\t\t\t unsigned percent_treshold, unsigned delay)\n {\n \tstruct progress *progress = malloc(sizeof(*progress));\n \tif (!progress) {\n@@ -227,6 +227,12 @@ struct progress *start_progress_delay(const char *title, unsigned total,\n \treturn progress;\n }\n \n+struct progress *start_delayed_progress(const char *title,\n+\t\t\t\t\tunsigned total,\tunsigned percent_treshold)\n+{\n+\treturn start_progress_delay(title, total, percent_treshold, 2);\n+}\n+\n struct progress *start_progress(const char *title, unsigned total)\n {\n \treturn start_progress_delay(title, total, 0, 0);\ndiff --git a/progress.h b/progress.h\nindex 611e4c4d42..c7fc431989 100644\n--- a/progress.h\n+++ b/progress.h\n@@ -6,8 +6,8 @@ struct progress;\n void display_throughput(struct progress *progress, off_t total);\n int display_progress(struct progress *progress, unsigned n);\n struct progress *start_progress(const char *title, unsigned total);\n-struct progress *start_progress_delay(const char *title, unsigned total,\n-\t\t\t\t unsigned percent_treshold, unsigned delay);\n+struct progress *start_delayed_progress(const char *title, unsigned total,\n+\t\t\t\t\tunsigned percent_treshold);\n void stop_progress(struct progress **progress);\n void stop_progress_msg(struct progress **progress, const char *msg);\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex dd535bc849..03d1b37578 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -343,8 +343,7 @@ static struct progress *get_progress(struct unpack_trees_options *o)\n \t\t\ttotal++;\n \t}\n \n-\treturn start_progress_delay(_(\"Checking out files\"),\n-\t\t\t\t total, 50, 1);\n+\treturn start_delayed_progress(_(\"Checking out files\"), total, 50);\n }\n \n static int check_updates(struct unpack_trees_options *o)\n\n"},{"id":"326332","messageId":"20170814222947.edvuz7b2hxuwcsqj@sigill.intra.peff.net","threadId":"46077","inReplyTo":"xmqqshguuhe2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/2] format-patch: have progress option while generating patches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-14T22:29:47Z","receivedAt":"2017-08-14T22:29:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 14, 2017 at 11:35:33AM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Perhaps we may want to replace the calls to progress_delay() with a\n> > call to a simpler wrapper that does not let the callers give their\n> > own delay threashold to simplify the API.\n> \n> ... which does not look too bad, but because it makes me wonder if\n> we even need to make the delay-threshold customizable per callers,\n> I'll wait further discussions before committing to the approach.\n\nFor what it's worth, I had a similar \"why is this even part of the API\"\nthought when writing earlier in the thread.\n\n> For example, I do not quite understand why 95 is a good value for\n> prune-packed while 0 is a good value for prune.\n\nAnd I also wondered this same thing. I do not have a good answer.\n\n> The rename detection in diffcore-rename, delay in blame, and\n> checkout (via unpack-trees) all of which use 50-percent threshold\n> with 1 second delay, sort of make sense to me in that if we\n> completed 50 percent within 1 second, it is likely we will finish\n> all in 2 seconds (which is the norm for everybody else), perhaps as\n> a better version of 0-percent 2 seconds rule.\n\nJust thinking what could go wrong for a moment.\n\nIf we reach 51% in one second, would we then never show progress? That\nseems like a misfeature when the counter is spiky. E.g., consider\nsomething like object transfer (which doesn't do a delayed progress, but\npretend for a moment as it makes a simple example). Imagine we have 100\nobjects, 99 of which are 10KB and one of which is 100MB. And that the\nbig object comes at slot 75.\n\nNo matter how the delay works, the counter is going to jump quickly to\n75% as we send the small objects, and then appear to stall on the large\none. That's unavoidable without feeding the progress code more data\nabout the items. But what does the user see?\n\nWith (0,2), we start the progress meter at 2 seconds, the user sees it\nstall at 75%, and then eventually it unclogs.\n\nWith (50,1), we check the percentage after 1 second and see we are\nalready at 75%. We then disable the meter totally. After 2 seconds, we\nget the same stall but the user sees nothing.\n\nSo in that case we should always base our decision only on time taken.\n\nAnd that gives me an answer for your question above: the difference is\nwhether we expect the counter to move smoothly, or to be spiky.\n\nIf it's smooth, the (50,1) case is slightly nicer in that it puts the\nprogress in front of the user more quickly. I'm not sure if that's\nactually worth pushing an additional decision onto the person writing\nthe calling code, though (especially when we are just now puzzling out\nthe method for making such a decision from first principles).\n\nSo I'd vote to drop that parameter entirely. And if 1 second seems\nnoticeably snappier, then we should probably just move everything to a 1\nsecond delay (I don't have a strong feeling either way).\n\n-Peff\n"},{"id":"326333","messageId":"xmqq8tilu5yx.fsf@gitster.mtv.corp.google.com","threadId":"46077","inReplyTo":"20170814222947.edvuz7b2hxuwcsqj@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] format-patch: have progress option while generating patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-14T22:42:14Z","receivedAt":"2017-08-14T22:42:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If it's smooth, the (50,1) case is slightly nicer in that it puts the\n> progress in front of the user more quickly. I'm not sure if that's\n> actually worth pushing an additional decision onto the person writing\n> the calling code, though (especially when we are just now puzzling out\n> the method for making such a decision from first principles).\n>\n> So I'd vote to drop that parameter entirely. And if 1 second seems\n> noticeably snappier, then we should probably just move everything to a 1\n> second delay (I don't have a strong feeling either way).\n\nSounds like a good idea to me. \n\nI've already locally tweaked Kevin's patch to use (0,2) instead of\n(0,1) without introducing the simpler wrapper. It should be trivial\nto do a wrapper to catch and migrate all the (0,2) users to a\nstart_delayed_progress() that takes neither percentage or time with\nmechanical replacement.\n\n"},{"id":"326342","messageId":"20170814230829.4elsuvosezybw3qn@sigill.intra.peff.net","threadId":"46077","inReplyTo":"xmqq8tilu5yx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/2] format-patch: have progress option while generating patches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-14T23:08:30Z","receivedAt":"2017-08-14T23:08:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 14, 2017 at 03:42:14PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > If it's smooth, the (50,1) case is slightly nicer in that it puts the\n> > progress in front of the user more quickly. I'm not sure if that's\n> > actually worth pushing an additional decision onto the person writing\n> > the calling code, though (especially when we are just now puzzling out\n> > the method for making such a decision from first principles).\n> >\n> > So I'd vote to drop that parameter entirely. And if 1 second seems\n> > noticeably snappier, then we should probably just move everything to a 1\n> > second delay (I don't have a strong feeling either way).\n> \n> Sounds like a good idea to me. \n> \n> I've already locally tweaked Kevin's patch to use (0,2) instead of\n> (0,1) without introducing the simpler wrapper. It should be trivial\n> to do a wrapper to catch and migrate all the (0,2) users to a\n> start_delayed_progress() that takes neither percentage or time with\n> mechanical replacement.\n\nI was actually proposing to move (50,1) cases to the simpler wrapper,\ntoo. IOW, drop the delayed_percent_treshold code entirely.\n\n-Peff\n"},{"id":"326344","messageId":"xmqq4lt9u42o.fsf@gitster.mtv.corp.google.com","threadId":"46077","inReplyTo":"20170814230829.4elsuvosezybw3qn@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] format-patch: have progress option while generating patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-14T23:23:11Z","receivedAt":"2017-08-14T23:23:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 14, 2017 at 03:42:14PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > If it's smooth, the (50,1) case is slightly nicer in that it puts the\n>> > progress in front of the user more quickly. I'm not sure if that's\n>> > actually worth pushing an additional decision onto the person writing\n>> > the calling code, though (especially when we are just now puzzling out\n>> > the method for making such a decision from first principles).\n>> >\n>> > So I'd vote to drop that parameter entirely. And if 1 second seems\n>> > noticeably snappier, then we should probably just move everything to a 1\n>> > second delay (I don't have a strong feeling either way).\n>> \n>> Sounds like a good idea to me. \n>> \n>> I've already locally tweaked Kevin's patch to use (0,2) instead of\n>> (0,1) without introducing the simpler wrapper. It should be trivial\n>> to do a wrapper to catch and migrate all the (0,2) users to a\n>> start_delayed_progress() that takes neither percentage or time with\n>> mechanical replacement.\n>\n> I was actually proposing to move (50,1) cases to the simpler wrapper,\n> too. IOW, drop the delayed_percent_treshold code entirely.\n\nI should have mentioned that (50,1) should also be treated just like\n(0,2) case--they should mean the same thing. Other oddness like 95\nmight merit individual consideration, and I didn't want to add (0,1)\nas another oddball to the mix.\n"},{"id":"326778","messageId":"xmqqy3qf8nj6.fsf_-_@gitster.mtv.corp.google.com","threadId":"46077","inReplyTo":"20170814222947.edvuz7b2hxuwcsqj@sigill.intra.peff.net","subject":"[PATCH] progress: simplify \"delayed\" progress API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-19T17:39:41Z","receivedAt":"2017-08-19T17:39:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We used to expose the full power of the delayed progress API to the\ncallers, so that they can specify, not just the message to show and\nexpected total amount of work that is used to compute the percentage\nof work performed so far, the percent-threshold parameter P and the\ndelay-seconds parameter N. The progress meter starts to show at N\nseconds into the operation only if the amount of work completed\nexceeds P.\n\nMost callers used either (0%, 2s) or (50%, 1s) as (P, N), but there\nare oddballs that chose more random-looking values like 95%.\n\nFor a smoother workload, (50%, 1s) would allow us to start showing\nthe progress meter earlier than (0%, 2s), while keeping the chance\nof not showing progress meter for long running operation the same as\nthe latter. For a task that would take 2s or more to complete, it\nis likely that less than half of it would complete within the first\nsecond, if the workload is smooth. But for a spiky workload whose\nearlier part is easier, such a setting is likely to fail to show the\nprogress meter entirely and (0%, 2s) is more appropriate.\n\nBut that is merely a theory. Realistically, it is of dubious value\nto ask each codepath to carefully consider smoothness of their\nworkload and specify their own setting by passing two extra\nparameters. Let's simplify the API by dropping both parameters and\nhave everybody use (0%, 2s).\n\nOh, by the way, the percent-threshold parameter and the structure\nmember were consistently misspelled, which also is now fixed ;-)\n\nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n\n * So before I forget all about this topic, here is a patch to\n actually do this.\n\n > So I'd vote to drop that parameter entirely. And if 1 second seems\n > noticeably snappier, then we should probably just move everything to a 1\n > second delay (I don't have a strong feeling either way).\n\n I was also tempted to do this, but decided to keep it closer to\n the original for majority of callers by leaving the delay at 2s.\n With this patch, such a change as a follow-up is made a lot\n easier (somebody may want to even make a configuration out of it,\n but that would not be me).\n\n builtin/blame.c | 3 +--\n builtin/fsck.c | 2 +-\n builtin/prune-packed.c | 3 +--\n builtin/prune.c | 2 +-\n builtin/rev-list.c | 2 +-\n diffcore-rename.c | 4 ++--\n progress.c | 15 ++++++++++-----\n progress.h | 3 +--\n unpack-trees.c | 3 +--\n 9 files changed, 19 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex bda1a78726..e0daf17548 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -925,8 +925,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tsb.found_guilty_entry = &found_guilty_entry;\n \tsb.found_guilty_entry_data = π\n \tif (show_progress)\n-\t\tpi.progress = start_progress_delay(_(\"Blaming lines\"),\n-\t\t\t\t\t\t sb.num_lines, 50, 1);\n+\t\tpi.progress = start_delayed_progress(_(\"Blaming lines\"), sb.num_lines);\n \n \tassign_blame(&sb, opt);\n \ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 99dea7adf6..0031439fc4 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -188,7 +188,7 @@ static int traverse_reachable(void)\n \tunsigned int nr = 0;\n \tint result = 0;\n \tif (show_progress)\n-\t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n+\t\tprogress = start_delayed_progress(_(\"Checking connectivity\"), 0);\n \twhile (pending.nr) {\n \t\tstruct object_array_entry *entry;\n \t\tstruct object *obj;\ndiff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\nindex ac978ad401..8f41f7c20e 100644\n--- a/builtin/prune-packed.c\n+++ b/builtin/prune-packed.c\n@@ -37,8 +37,7 @@ static int prune_object(const struct object_id *oid, const char *path,\n void prune_packed_objects(int opts)\n {\n \tif (opts & PRUNE_PACKED_VERBOSE)\n-\t\tprogress = start_progress_delay(_(\"Removing duplicate objects\"),\n-\t\t\t256, 95, 2);\n+\t\tprogress = start_delayed_progress(_(\"Removing duplicate objects\"), 256);\n \n \tfor_each_loose_file_in_objdir(get_object_directory(),\n \t\t\t\t prune_object, NULL, prune_subdir, &opts);\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex c378690545..cddabf26a9 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -138,7 +138,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \tif (show_progress == -1)\n \t\tshow_progress = isatty(2);\n \tif (show_progress)\n-\t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n+\t\tprogress = start_delayed_progress(_(\"Checking connectivity\"), 0);\n \n \tmark_reachable_objects(&revs, 1, expire, progress);\n \tstop_progress(&progress);\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 95d84d5cda..dfad8e847a 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -364,7 +364,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\trevs.limited = 1;\n \n \tif (show_progress)\n-\t\tprogress = start_progress_delay(show_progress, 0, 0, 2);\n+\t\tprogress = start_delayed_progress(show_progress, 0);\n \n \tif (use_bitmap_index && !revs.prune) {\n \t\tif (revs.count && !revs.left_right && !revs.cherry_mark) {\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 786f389498..0d8c3d2ee4 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -532,9 +532,9 @@ void diffcore_rename(struct diff_options *options)\n \t}\n \n \tif (options->show_rename_progress) {\n-\t\tprogress = start_progress_delay(\n+\t\tprogress = start_delayed_progress(\n \t\t\t\t_(\"Performing inexact rename detection\"),\n-\t\t\t\trename_dst_nr * rename_src_nr, 50, 1);\n+\t\t\t\trename_dst_nr * rename_src_nr);\n \t}\n \n \tmx = xcalloc(st_mult(NUM_CANDIDATE_PER_DST, num_create), sizeof(*mx));\ndiff --git a/progress.c b/progress.c\nindex 73e36d4a42..289678d43d 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -34,7 +34,7 @@ struct progress {\n \tunsigned total;\n \tunsigned last_percent;\n \tunsigned delay;\n-\tunsigned delayed_percent_treshold;\n+\tunsigned delayed_percent_threshold;\n \tstruct throughput *throughput;\n \tuint64_t start_ns;\n };\n@@ -88,7 +88,7 @@ static int display(struct progress *progress, unsigned n, const char *done)\n \t\t\treturn 0;\n \t\tif (progress->total) {\n \t\t\tunsigned percent = n * 100 / progress->total;\n-\t\t\tif (percent > progress->delayed_percent_treshold) {\n+\t\t\tif (percent > progress->delayed_percent_threshold) {\n \t\t\t\t/* inhibit this progress report entirely */\n \t\t\t\tclear_progress_signal();\n \t\t\t\tprogress->delay = -1;\n@@ -205,8 +205,8 @@ int display_progress(struct progress *progress, unsigned n)\n \treturn progress ? display(progress, n, NULL) : 0;\n }\n \n-struct progress *start_progress_delay(const char *title, unsigned total,\n-\t\t\t\t unsigned percent_treshold, unsigned delay)\n+static struct progress *start_progress_delay(const char *title, unsigned total,\n+\t\t\t\t\t unsigned percent_threshold, unsigned delay)\n {\n \tstruct progress *progress = malloc(sizeof(*progress));\n \tif (!progress) {\n@@ -219,7 +219,7 @@ struct progress *start_progress_delay(const char *title, unsigned total,\n \tprogress->total = total;\n \tprogress->last_value = -1;\n \tprogress->last_percent = -1;\n-\tprogress->delayed_percent_treshold = percent_treshold;\n+\tprogress->delayed_percent_threshold = percent_threshold;\n \tprogress->delay = delay;\n \tprogress->throughput = NULL;\n \tprogress->start_ns = getnanotime();\n@@ -227,6 +227,11 @@ struct progress *start_progress_delay(const char *title, unsigned total,\n \treturn progress;\n }\n \n+struct progress *start_delayed_progress(const char *title, unsigned total)\n+{\n+\treturn start_progress_delay(title, total, 0, 2);\n+}\n+\n struct progress *start_progress(const char *title, unsigned total)\n {\n \treturn start_progress_delay(title, total, 0, 0);\ndiff --git a/progress.h b/progress.h\nindex 611e4c4d42..6392b63371 100644\n--- a/progress.h\n+++ b/progress.h\n@@ -6,8 +6,7 @@ struct progress;\n void display_throughput(struct progress *progress, off_t total);\n int display_progress(struct progress *progress, unsigned n);\n struct progress *start_progress(const char *title, unsigned total);\n-struct progress *start_progress_delay(const char *title, unsigned total,\n-\t\t\t\t unsigned percent_treshold, unsigned delay);\n+struct progress *start_delayed_progress(const char *title, unsigned total);\n void stop_progress(struct progress **progress);\n void stop_progress_msg(struct progress **progress, const char *msg);\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex dd535bc849..e5ae7fe183 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -343,8 +343,7 @@ static struct progress *get_progress(struct unpack_trees_options *o)\n \t\t\ttotal++;\n \t}\n \n-\treturn start_progress_delay(_(\"Checking out files\"),\n-\t\t\t\t total, 50, 1);\n+\treturn start_delayed_progress(_(\"Checking out files\"), total);\n }\n \n static int check_updates(struct unpack_trees_options *o)\n-- \n2.14.1-396-gef255185d2\n\n"},{"id":"326790","messageId":"xmqqlgmf8ebb.fsf@gitster.mtv.corp.google.com","threadId":"46077","inReplyTo":"xmqqy3qf8nj6.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH] progress: simplify \"delayed\" progress API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-19T20:58:48Z","receivedAt":"2017-08-19T20:59:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> We used to expose the full power of the delayed progress API to the\n> callers, so that they can specify, not just the message to show and\n> expected total amount of work that is used to compute the percentage\n> of work performed so far, the percent-threshold parameter P and the\n> delay-seconds parameter N. The progress meter starts to show at N\n> seconds into the operation only if the amount of work completed\n> exceeds P.\n\nOops, we inspect the doneness at N seconds and show only if we have\nnot yet completed P percent of the total work.\n\nPerhaps\n\n\t... at N seconds into the operation only if we have not\n\tcompleted P per-cent of the total work.\n\n"},{"id":"326795","messageId":"20170820074318.3fchxwbuux5zbkah@sigill.intra.peff.net","threadId":"46077","inReplyTo":"xmqqy3qf8nj6.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH] progress: simplify \"delayed\" progress API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-20T07:43:19Z","receivedAt":"2017-08-20T07:43:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 19, 2017 at 10:39:41AM -0700, Junio C Hamano wrote:\n\n> We used to expose the full power of the delayed progress API to the\n> callers, so that they can specify, not just the message to show and\n> expected total amount of work that is used to compute the percentage\n> of work performed so far, the percent-threshold parameter P and the\n> delay-seconds parameter N. The progress meter starts to show at N\n> seconds into the operation only if the amount of work completed\n> exceeds P.\n> \n> Most callers used either (0%, 2s) or (50%, 1s) as (P, N), but there\n> are oddballs that chose more random-looking values like 95%.\n> \n> For a smoother workload, (50%, 1s) would allow us to start showing\n> the progress meter earlier than (0%, 2s), while keeping the chance\n> of not showing progress meter for long running operation the same as\n> the latter. For a task that would take 2s or more to complete, it\n> is likely that less than half of it would complete within the first\n> second, if the workload is smooth. But for a spiky workload whose\n> earlier part is easier, such a setting is likely to fail to show the\n> progress meter entirely and (0%, 2s) is more appropriate.\n> \n> But that is merely a theory. Realistically, it is of dubious value\n> to ask each codepath to carefully consider smoothness of their\n> workload and specify their own setting by passing two extra\n> parameters. Let's simplify the API by dropping both parameters and\n> have everybody use (0%, 2s).\n\nNicely explained (modulo the reversal you noticed in the first\nparagraph). The patch looks good to me.\n\n> Oh, by the way, the percent-threshold parameter and the structure\n> member were consistently misspelled, which also is now fixed ;-)\n\nHeh, that was bugging me, too. :)\n\n> * So before I forget all about this topic, here is a patch to\n> actually do this.\n> \n> > So I'd vote to drop that parameter entirely. And if 1 second seems\n> > noticeably snappier, then we should probably just move everything to a 1\n> > second delay (I don't have a strong feeling either way).\n> \n> I was also tempted to do this, but decided to keep it closer to\n> the original for majority of callers by leaving the delay at 2s.\n> With this patch, such a change as a follow-up is made a lot\n> easier (somebody may want to even make a configuration out of it,\n> but that would not be me).\n\nYeah, I agree that it can come later on top (if at all).\n\nThanks for finishing this off.\n\n-Peff\n"}]}