{"thread":{"id":"46632","subject":"[PATCH v1] convert: display progress for filtered objects that have been delayed","startedAt":"2017-08-20T15:47:35Z","lastAt":"2017-10-04T13:34:08Z","messageCount":6,"participants":["Lars Schneider","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"326818","messageId":"20170820154720.32259-1-larsxschneider@gmail.com","threadId":"46632","inReplyTo":null,"subject":"[PATCH v1] convert: display progress for filtered objects that have been delayed","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-08-20T15:47:20Z","receivedAt":"2017-08-20T15:47:35Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"In 2841e8f (\"convert: add \"status=delayed\" to filter process protocol\",\n2017-06-30) we taught the filter process protocol to delayed responses.\nThese responses are processed after the \"Checking out files\" phase.\nIf the processing takes noticeable time, then the user might think Git\nis stuck.\n\nDisplay the progress of the delayed responses to let the user know that\nGit is still processing objects. This works very well for objects that\ncan be filtered quickly. If filtering of an individual object takes\nnoticeable time, then the user might still think that Git is stuck.\nHowever, in that case the user would at least know what Git is doing.\n\nIt would be technical more correct to display \"Checking out files whose\ncontent filtering has been delayed\". For brevity we only print\n\"Filtering content\".\n\nThe finish_delayed_checkout() call was moved below the stop_progress()\ncall in unpack-trees.c to ensure that the \"Checking out files\" progress\nis properly stopped before the \"Filtering content\" progress starts in\nfinish_delayed_checkout().\n\nSigned-off-by: Lars Schneider \nSuggested-by: Taylor Blau \n---\n\nNotes:\n Base Ref: master\n Web-Diff: https://github.com/larsxschneider/git/commit/8c3f433083\n Checkout: git fetch https://github.com/larsxschneider/git filter-process/progress-v1 && git checkout 8c3f433083\n\n entry.c | 16 +++++++++++++++-\n unpack-trees.c | 2 +-\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 65458f07a4..1d1a09f47e 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -3,6 +3,7 @@\n #include \"dir.h\"\n #include \"streaming.h\"\n #include \"submodule.h\"\n+#include \"progress.h\"\n \n static void create_directories(const char *path, int path_len,\n \t\t\t const struct checkout *state)\n@@ -161,16 +162,23 @@ static int remove_available_paths(struct string_list_item *item, void *cb_data)\n int finish_delayed_checkout(struct checkout *state)\n {\n \tint errs = 0;\n+\tunsigned delayed_object_count;\n+\toff_t filtered_bytes = 0;\n \tstruct string_list_item *filter, *path;\n+\tstruct progress *progress;\n \tstruct delayed_checkout *dco = state->delayed_checkout;\n \n \tif (!state->delayed_checkout)\n \t\treturn errs;\n \n \tdco->state = CE_RETRY;\n+\tdelayed_object_count = dco->paths.nr;\n+\tprogress = start_progress_delay(\n+\t\t_(\"Filtering content\"), delayed_object_count, 50, 1);\n \twhile (dco->filters.nr > 0) {\n \t\tfor_each_string_list_item(filter, &dco->filters) {\n \t\t\tstruct string_list available_paths = STRING_LIST_INIT_NODUP;\n+\t\t\tdisplay_progress(progress, delayed_object_count - dco->paths.nr);\n \n \t\t\tif (!async_query_available_blobs(filter->string, &available_paths)) {\n \t\t\t\t/* Filter reported an error */\n@@ -216,11 +224,17 @@ int finish_delayed_checkout(struct checkout *state)\n \t\t\t\t}\n \t\t\t\tce = index_file_exists(state->istate, path->string,\n \t\t\t\t\t\t strlen(path->string), 0);\n-\t\t\t\terrs |= (ce ? checkout_entry(ce, state, NULL) : 1);\n+\t\t\t\tif (ce) {\n+\t\t\t\t\terrs |= checkout_entry(ce, state, NULL);\n+\t\t\t\t\tfiltered_bytes += ce->ce_stat_data.sd_size;\n+\t\t\t\t\tdisplay_throughput(progress, filtered_bytes);\n+\t\t\t\t} else\n+\t\t\t\t\terrs = 1;\n \t\t\t}\n \t\t}\n \t\tstring_list_remove_empty_items(&dco->filters, 0);\n \t}\n+\tstop_progress(&progress);\n \tstring_list_clear(&dco->filters, 0);\n \n \t/* At this point we should not have any delayed paths anymore. */\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 862cfce661..90fb270d77 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -395,8 +395,8 @@ static int check_updates(struct unpack_trees_options *o)\n \t\t\t}\n \t\t}\n \t}\n-\terrs |= finish_delayed_checkout(&state);\n \tstop_progress(&progress);\n+\terrs |= finish_delayed_checkout(&state);\n \tif (o->update)\n \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN, NULL);\n \treturn errs != 0;\n\nbase-commit: b3622a4ee94e4916cd05e6d96e41eeb36b941182\n-- \n2.14.1\n\n"},{"id":"327150","messageId":"xmqqwp5svjne.fsf@gitster.mtv.corp.google.com","threadId":"46632","inReplyTo":"20170820154720.32259-1-larsxschneider@gmail.com","subject":"Re: [PATCH v1] convert: display progress for filtered objects that have been delayed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-24T19:40:53Z","receivedAt":"2017-08-24T19:41:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> In 2841e8f (\"convert: add \"status=delayed\" to filter process protocol\",\n> 2017-06-30) we taught the filter process protocol to delayed responses.\n> These responses are processed after the \"Checking out files\" phase.\n> If the processing takes noticeable time, then the user might think Git\n> is stuck.\n>\n> Display the progress of the delayed responses to let the user know that\n> Git is still processing objects. This works very well for objects that\n> can be filtered quickly. If filtering of an individual object takes\n> noticeable time, then the user might still think that Git is stuck.\n> However, in that case the user would at least know what Git is doing.\n>\n> It would be technical more correct to display \"Checking out files whose\n> content filtering has been delayed\". For brevity we only print\n> \"Filtering content\".\n>\n> The finish_delayed_checkout() call was moved below the stop_progress()\n> call in unpack-trees.c to ensure that the \"Checking out files\" progress\n> is properly stopped before the \"Filtering content\" progress starts in\n> finish_delayed_checkout().\n>\n> Signed-off-by: Lars Schneider \n> Suggested-by: Taylor Blau \n> ---\n\nMakes sense. The only thing that made me wonder was if we want the\nchange in unpack-trees.c in this patch. After all, the procedure to\nfinish up the delayed checkout _is_ a part of the work need to be\ndone to populate the working tree files, so stopping the progress\nbefore feels somewhat wrong at the phylosophical level.\n\nI think our output cannot express nested progress bars, and I think\nthat is the reason why this patch tweaks unpack-trees.c; so I am\nfine with the end result (and that is why I said \"made me wonder\nwas\", not \"makes me wonder\", the latter would imply \"this we might\nwant fix before applying\", but I do not think we want to change\nanything this patch does to unpack-trees.c in this case).\n\nThe delayed progress API is being simplified so I'll probably do a\nbit of evil merge while merging this to 'pu'.\n\nThanks.\n\n> entry.c | 16 +++++++++++++++-\n> unpack-trees.c | 2 +-\n> 2 files changed, 16 insertions(+), 2 deletions(-)\n>\n> diff --git a/entry.c b/entry.c\n> index 65458f07a4..1d1a09f47e 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -3,6 +3,7 @@\n> #include \"dir.h\"\n> #include \"streaming.h\"\n> #include \"submodule.h\"\n> +#include \"progress.h\"\n> \n> static void create_directories(const char *path, int path_len,\n> \t\t\t const struct checkout *state)\n> @@ -161,16 +162,23 @@ static int remove_available_paths(struct string_list_item *item, void *cb_data)\n> int finish_delayed_checkout(struct checkout *state)\n> {\n> \tint errs = 0;\n> +\tunsigned delayed_object_count;\n> +\toff_t filtered_bytes = 0;\n> \tstruct string_list_item *filter, *path;\n> +\tstruct progress *progress;\n> \tstruct delayed_checkout *dco = state->delayed_checkout;\n> \n> \tif (!state->delayed_checkout)\n> \t\treturn errs;\n> \n> \tdco->state = CE_RETRY;\n> +\tdelayed_object_count = dco->paths.nr;\n> +\tprogress = start_progress_delay(\n> +\t\t_(\"Filtering content\"), delayed_object_count, 50, 1);\n> \twhile (dco->filters.nr > 0) {\n> \t\tfor_each_string_list_item(filter, &dco->filters) {\n> \t\t\tstruct string_list available_paths = STRING_LIST_INIT_NODUP;\n> +\t\t\tdisplay_progress(progress, delayed_object_count - dco->paths.nr);\n> \n> \t\t\tif (!async_query_available_blobs(filter->string, &available_paths)) {\n> \t\t\t\t/* Filter reported an error */\n> @@ -216,11 +224,17 @@ int finish_delayed_checkout(struct checkout *state)\n> \t\t\t\t}\n> \t\t\t\tce = index_file_exists(state->istate, path->string,\n> \t\t\t\t\t\t strlen(path->string), 0);\n> -\t\t\t\terrs |= (ce ? checkout_entry(ce, state, NULL) : 1);\n> +\t\t\t\tif (ce) {\n> +\t\t\t\t\terrs |= checkout_entry(ce, state, NULL);\n> +\t\t\t\t\tfiltered_bytes += ce->ce_stat_data.sd_size;\n> +\t\t\t\t\tdisplay_throughput(progress, filtered_bytes);\n> +\t\t\t\t} else\n> +\t\t\t\t\terrs = 1;\n> \t\t\t}\n> \t\t}\n> \t\tstring_list_remove_empty_items(&dco->filters, 0);\n> \t}\n> +\tstop_progress(&progress);\n> \tstring_list_clear(&dco->filters, 0);\n> \n> \t/* At this point we should not have any delayed paths anymore. */\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 862cfce661..90fb270d77 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -395,8 +395,8 @@ static int check_updates(struct unpack_trees_options *o)\n> \t\t\t}\n> \t\t}\n> \t}\n> -\terrs |= finish_delayed_checkout(&state);\n> \tstop_progress(&progress);\n> +\terrs |= finish_delayed_checkout(&state);\n> \tif (o->update)\n> \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN, NULL);\n> \treturn errs != 0;\n>\n> base-commit: b3622a4ee94e4916cd05e6d96e41eeb36b941182\n"},{"id":"329675","messageId":"1AD84BB7-5BCA-4982-B157-944890F796EE@gmail.com","threadId":"46632","inReplyTo":"xmqqwp5svjne.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] convert: display progress for filtered objects that have been delayed","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-10-04T10:52:25Z","receivedAt":"2017-10-04T10:52:35Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 24 Aug 2017, at 21:40, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>> In 2841e8f (\"convert: add \"status=delayed\" to filter process protocol\",\n>> 2017-06-30) we taught the filter process protocol to delayed responses.\n>> These responses are processed after the \"Checking out files\" phase.\n>> If the processing takes noticeable time, then the user might think Git\n>> is stuck.\n>> \n>> Display the progress of the delayed responses to let the user know that\n>> Git is still processing objects. This works very well for objects that\n>> can be filtered quickly. If filtering of an individual object takes\n>> noticeable time, then the user might still think that Git is stuck.\n>> However, in that case the user would at least know what Git is doing.\n>> \n>> It would be technical more correct to display \"Checking out files whose\n>> content filtering has been delayed\". For brevity we only print\n>> \"Filtering content\".\n>> \n>> The finish_delayed_checkout() call was moved below the stop_progress()\n>> call in unpack-trees.c to ensure that the \"Checking out files\" progress\n>> is properly stopped before the \"Filtering content\" progress starts in\n>> finish_delayed_checkout().\n>> \n>> Signed-off-by: Lars Schneider \n>> Suggested-by: Taylor Blau \n>> ---\n> \n> Makes sense. The only thing that made me wonder was if we want the\n> change in unpack-trees.c in this patch. After all, the procedure to\n> finish up the delayed checkout _is_ a part of the work need to be\n> done to populate the working tree files, so stopping the progress\n> before feels somewhat wrong at the phylosophical level.\n> \n> I think our output cannot express nested progress bars, and I think\n> that is the reason why this patch tweaks unpack-trees.c; so I am\n> fine with the end result (and that is why I said \"made me wonder\n> was\", not \"makes me wonder\", the latter would imply \"this we might\n> want fix before applying\", but I do not think we want to change\n> anything this patch does to unpack-trees.c in this case).\n> \n> The delayed progress API is being simplified so I'll probably do a\n> bit of evil merge while merging this to 'pu'.\n> \n> Thanks.\n\nHi Junio,\n\nI just realized that this patch got lost :-(\nThat means 2.14.2 supports the delayed filters but does not show\nprogress to the user. To the user Git will appear hanging.\n\nCan you merge this this topic for 2.14.3 / 2.15 ?\n\nThank you,\nLars\n"},{"id":"329678","messageId":"C5857D7A-4B8E-4DA2-B2BA-EFE6373011E2@gmail.com","threadId":"46632","inReplyTo":"1AD84BB7-5BCA-4982-B157-944890F796EE@gmail.com","subject":"Re: [PATCH v1] convert: display progress for filtered objects that have been delayed","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-10-04T11:55:21Z","receivedAt":"2017-10-04T11:55:30Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 04 Oct 2017, at 12:52, Lars Schneider wrote:\n> \n> \n>> On 24 Aug 2017, at 21:40, Junio C Hamano wrote:\n>> \n>> Lars Schneider writes:\n>> \n>>> In 2841e8f (\"convert: add \"status=delayed\" to filter process protocol\",\n>>> 2017-06-30) we taught the filter process protocol to delayed responses.\n>>> These responses are processed after the \"Checking out files\" phase.\n>>> If the processing takes noticeable time, then the user might think Git\n>>> is stuck.\n>>> \n>>> Display the progress of the delayed responses to let the user know that\n>>> Git is still processing objects. This works very well for objects that\n>>> can be filtered quickly. If filtering of an individual object takes\n>>> noticeable time, then the user might still think that Git is stuck.\n>>> However, in that case the user would at least know what Git is doing.\n>>> \n>>> It would be technical more correct to display \"Checking out files whose\n>>> content filtering has been delayed\". For brevity we only print\n>>> \"Filtering content\".\n>>> \n>>> The finish_delayed_checkout() call was moved below the stop_progress()\n>>> call in unpack-trees.c to ensure that the \"Checking out files\" progress\n>>> is properly stopped before the \"Filtering content\" progress starts in\n>>> finish_delayed_checkout().\n>>> \n>>> Signed-off-by: Lars Schneider \n>>> Suggested-by: Taylor Blau \n>>> ---\n>> \n>> Makes sense. The only thing that made me wonder was if we want the\n>> change in unpack-trees.c in this patch. After all, the procedure to\n>> finish up the delayed checkout _is_ a part of the work need to be\n>> done to populate the working tree files, so stopping the progress\n>> before feels somewhat wrong at the phylosophical level.\n>> \n>> I think our output cannot express nested progress bars, and I think\n>> that is the reason why this patch tweaks unpack-trees.c; so I am\n>> fine with the end result (and that is why I said \"made me wonder\n>> was\", not \"makes me wonder\", the latter would imply \"this we might\n>> want fix before applying\", but I do not think we want to change\n>> anything this patch does to unpack-trees.c in this case).\n>> \n>> The delayed progress API is being simplified so I'll probably do a\n>> bit of evil merge while merging this to 'pu'.\n>> \n>> Thanks.\n> \n> Hi Junio,\n> \n> I just realized that this patch got lost :-(\n> That means 2.14.2 supports the delayed filters but does not show\n> progress to the user. To the user Git will appear hanging.\n> \n> Can you merge this this topic for 2.14.3 / 2.15 ?\n> \n> Thank you,\n> Lars\n\nI should have expressed myself more clearly:\nThe patch made it into master with 52f1d62eb44faf569edca360ec9af9ddd4045fe0 .\nTherefore, I think it will be released with 2.15, right?\n\nThe patch just did not make it into 2.14.2 and I wonder if you could\nmerge the patch for 2.14.3?\n\nThank you,\nLars\n\n"},{"id":"329679","messageId":"xmqqr2uj9lfk.fsf@gitster.mtv.corp.google.com","threadId":"46632","inReplyTo":"1AD84BB7-5BCA-4982-B157-944890F796EE@gmail.com","subject":"Re: [PATCH v1] convert: display progress for filtered objects that have been delayed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-04T11:55:43Z","receivedAt":"2017-10-04T11:55:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> The delayed progress API is being simplified so I'll probably do a\n>> bit of evil merge while merging this to 'pu'.\n>\n> I just realized that this patch got lost :-(\n\nReally? Isn't it 52f1d62e (\"convert: display progress for filtered\nobjects that have been delayed\", 2017-08-20)? It already is in\n'master' since about 3 weeks ago.\n\nAs this is merely a new \"feature\", I do not think it is reasonable\nto expect it to be merged down to 'maint'. In fact, I think your\noriginal patch <20170820154720.32259-1-larsxschneider@gmail.com>\ndeclared that its Base Ref was \"master\", and I queued the topic on\ntop of the then-current 'master', so it is not mergeable to 'maint'.\n\nIt may be cherry-pickable, but I do not think it deserves to be. It\nis not a fix for a grave bug or anything like that, right?\n\n"},{"id":"329686","messageId":"5625E29A-D24F-4A5F-AA52-58F2EBCA9648@gmail.com","threadId":"46632","inReplyTo":"xmqqr2uj9lfk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] convert: display progress for filtered objects that have been delayed","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-10-04T13:33:54Z","receivedAt":"2017-10-04T13:34:08Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 04 Oct 2017, at 13:55, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>>> The delayed progress API is being simplified so I'll probably do a\n>>> bit of evil merge while merging this to 'pu'.\n>> \n>> I just realized that this patch got lost :-(\n> \n> Really? Isn't it 52f1d62e (\"convert: display progress for filtered\n> objects that have been delayed\", 2017-08-20)? It already is in\n> 'master' since about 3 weeks ago.\n> \n> As this is merely a new \"feature\", I do not think it is reasonable\n> to expect it to be merged down to 'maint'. In fact, I think your\n> original patch <20170820154720.32259-1-larsxschneider@gmail.com>\n> declared that its Base Ref was \"master\", and I queued the topic on\n> top of the then-current 'master', so it is not mergeable to 'maint'.\n> \n> It may be cherry-pickable, but I do not think it deserves to be. It\n> is not a fix for a grave bug or anything like that, right?\n\nThe original topic 2841e8f (\"convert: add \"status=delayed\" to filter \nprocess protocol\", 2017-06-30) is a new feature. That's why I was\nsurprised to see it in 2.14.2! The problem is that this topic\nmight appear \"broken\" to the user without the progress output\nintroduce in 52f1d62e (\"convert: display progress for filtered\nobjects that have been delayed\", 2017-08-20).\n\nIn summary: 2841e8f was merged to 2.14.2 and 52f1d62e was not.\nTherefore, it would be great to have 52f1d62e in 2.14.3.\nDoes this sound reasonable to you?\n\nThanks,\nLars\n"}]}