* [PATCH v1] convert: display progress for filtered objects that have been delayed
@ 2017-08-20 15:47 Lars Schneider
2017-08-24 19:40 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Lars Schneider @ 2017-08-20 15:47 UTC (permalink / raw)
To: git; +Cc: me, gitster
In 2841e8f ("convert: add "status=delayed" to filter process protocol",
2017-06-30) we taught the filter process protocol to delayed responses.
These responses are processed after the "Checking out files" phase.
If the processing takes noticeable time, then the user might think Git
is stuck.
Display the progress of the delayed responses to let the user know that
Git is still processing objects. This works very well for objects that
can be filtered quickly. If filtering of an individual object takes
noticeable time, then the user might still think that Git is stuck.
However, in that case the user would at least know what Git is doing.
It would be technical more correct to display "Checking out files whose
content filtering has been delayed". For brevity we only print
"Filtering content".
The finish_delayed_checkout() call was moved below the stop_progress()
call in unpack-trees.c to ensure that the "Checking out files" progress
is properly stopped before the "Filtering content" progress starts in
finish_delayed_checkout().
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Suggested-by: Taylor Blau <me@ttaylorr.com>
---
Notes:
Base Ref: master
Web-Diff: https://github.com/larsxschneider/git/commit/8c3f433083
Checkout: git fetch https://github.com/larsxschneider/git filter-process/progress-v1 && git checkout 8c3f433083
entry.c | 16 +++++++++++++++-
unpack-trees.c | 2 +-
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/entry.c b/entry.c
index 65458f07a4..1d1a09f47e 100644
--- a/entry.c
+++ b/entry.c
@@ -3,6 +3,7 @@
#include "dir.h"
#include "streaming.h"
#include "submodule.h"
+#include "progress.h"
static void create_directories(const char *path, int path_len,
const struct checkout *state)
@@ -161,16 +162,23 @@ static int remove_available_paths(struct string_list_item *item, void *cb_data)
int finish_delayed_checkout(struct checkout *state)
{
int errs = 0;
+ unsigned delayed_object_count;
+ off_t filtered_bytes = 0;
struct string_list_item *filter, *path;
+ struct progress *progress;
struct delayed_checkout *dco = state->delayed_checkout;
if (!state->delayed_checkout)
return errs;
dco->state = CE_RETRY;
+ delayed_object_count = dco->paths.nr;
+ progress = start_progress_delay(
+ _("Filtering content"), delayed_object_count, 50, 1);
while (dco->filters.nr > 0) {
for_each_string_list_item(filter, &dco->filters) {
struct string_list available_paths = STRING_LIST_INIT_NODUP;
+ display_progress(progress, delayed_object_count - dco->paths.nr);
if (!async_query_available_blobs(filter->string, &available_paths)) {
/* Filter reported an error */
@@ -216,11 +224,17 @@ int finish_delayed_checkout(struct checkout *state)
}
ce = index_file_exists(state->istate, path->string,
strlen(path->string), 0);
- errs |= (ce ? checkout_entry(ce, state, NULL) : 1);
+ if (ce) {
+ errs |= checkout_entry(ce, state, NULL);
+ filtered_bytes += ce->ce_stat_data.sd_size;
+ display_throughput(progress, filtered_bytes);
+ } else
+ errs = 1;
}
}
string_list_remove_empty_items(&dco->filters, 0);
}
+ stop_progress(&progress);
string_list_clear(&dco->filters, 0);
/* At this point we should not have any delayed paths anymore. */
diff --git a/unpack-trees.c b/unpack-trees.c
index 862cfce661..90fb270d77 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -395,8 +395,8 @@ static int check_updates(struct unpack_trees_options *o)
}
}
}
- errs |= finish_delayed_checkout(&state);
stop_progress(&progress);
+ errs |= finish_delayed_checkout(&state);
if (o->update)
git_attr_set_direction(GIT_ATTR_CHECKIN, NULL);
return errs != 0;
base-commit: b3622a4ee94e4916cd05e6d96e41eeb36b941182
--
2.14.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1] convert: display progress for filtered objects that have been delayed
2017-08-20 15:47 [PATCH v1] convert: display progress for filtered objects that have been delayed Lars Schneider
@ 2017-08-24 19:40 ` Junio C Hamano
2017-10-04 10:52 ` Lars Schneider
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2017-08-24 19:40 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, me
Lars Schneider <larsxschneider@gmail.com> writes:
> In 2841e8f ("convert: add "status=delayed" to filter process protocol",
> 2017-06-30) we taught the filter process protocol to delayed responses.
> These responses are processed after the "Checking out files" phase.
> If the processing takes noticeable time, then the user might think Git
> is stuck.
>
> Display the progress of the delayed responses to let the user know that
> Git is still processing objects. This works very well for objects that
> can be filtered quickly. If filtering of an individual object takes
> noticeable time, then the user might still think that Git is stuck.
> However, in that case the user would at least know what Git is doing.
>
> It would be technical more correct to display "Checking out files whose
> content filtering has been delayed". For brevity we only print
> "Filtering content".
>
> The finish_delayed_checkout() call was moved below the stop_progress()
> call in unpack-trees.c to ensure that the "Checking out files" progress
> is properly stopped before the "Filtering content" progress starts in
> finish_delayed_checkout().
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> Suggested-by: Taylor Blau <me@ttaylorr.com>
> ---
Makes sense. The only thing that made me wonder was if we want the
change in unpack-trees.c in this patch. After all, the procedure to
finish up the delayed checkout _is_ a part of the work need to be
done to populate the working tree files, so stopping the progress
before feels somewhat wrong at the phylosophical level.
I think our output cannot express nested progress bars, and I think
that is the reason why this patch tweaks unpack-trees.c; so I am
fine with the end result (and that is why I said "made me wonder
was", not "makes me wonder", the latter would imply "this we might
want fix before applying", but I do not think we want to change
anything this patch does to unpack-trees.c in this case).
The delayed progress API is being simplified so I'll probably do a
bit of evil merge while merging this to 'pu'.
Thanks.
> entry.c | 16 +++++++++++++++-
> unpack-trees.c | 2 +-
> 2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index 65458f07a4..1d1a09f47e 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -3,6 +3,7 @@
> #include "dir.h"
> #include "streaming.h"
> #include "submodule.h"
> +#include "progress.h"
>
> static void create_directories(const char *path, int path_len,
> const struct checkout *state)
> @@ -161,16 +162,23 @@ static int remove_available_paths(struct string_list_item *item, void *cb_data)
> int finish_delayed_checkout(struct checkout *state)
> {
> int errs = 0;
> + unsigned delayed_object_count;
> + off_t filtered_bytes = 0;
> struct string_list_item *filter, *path;
> + struct progress *progress;
> struct delayed_checkout *dco = state->delayed_checkout;
>
> if (!state->delayed_checkout)
> return errs;
>
> dco->state = CE_RETRY;
> + delayed_object_count = dco->paths.nr;
> + progress = start_progress_delay(
> + _("Filtering content"), delayed_object_count, 50, 1);
> while (dco->filters.nr > 0) {
> for_each_string_list_item(filter, &dco->filters) {
> struct string_list available_paths = STRING_LIST_INIT_NODUP;
> + display_progress(progress, delayed_object_count - dco->paths.nr);
>
> if (!async_query_available_blobs(filter->string, &available_paths)) {
> /* Filter reported an error */
> @@ -216,11 +224,17 @@ int finish_delayed_checkout(struct checkout *state)
> }
> ce = index_file_exists(state->istate, path->string,
> strlen(path->string), 0);
> - errs |= (ce ? checkout_entry(ce, state, NULL) : 1);
> + if (ce) {
> + errs |= checkout_entry(ce, state, NULL);
> + filtered_bytes += ce->ce_stat_data.sd_size;
> + display_throughput(progress, filtered_bytes);
> + } else
> + errs = 1;
> }
> }
> string_list_remove_empty_items(&dco->filters, 0);
> }
> + stop_progress(&progress);
> string_list_clear(&dco->filters, 0);
>
> /* At this point we should not have any delayed paths anymore. */
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 862cfce661..90fb270d77 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -395,8 +395,8 @@ static int check_updates(struct unpack_trees_options *o)
> }
> }
> }
> - errs |= finish_delayed_checkout(&state);
> stop_progress(&progress);
> + errs |= finish_delayed_checkout(&state);
> if (o->update)
> git_attr_set_direction(GIT_ATTR_CHECKIN, NULL);
> return errs != 0;
>
> base-commit: b3622a4ee94e4916cd05e6d96e41eeb36b941182
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] convert: display progress for filtered objects that have been delayed
2017-08-24 19:40 ` Junio C Hamano
@ 2017-10-04 10:52 ` Lars Schneider
2017-10-04 11:55 ` Lars Schneider
2017-10-04 11:55 ` Junio C Hamano
0 siblings, 2 replies; 6+ messages in thread
From: Lars Schneider @ 2017-10-04 10:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, me
> On 24 Aug 2017, at 21:40, Junio C Hamano <gitster@pobox.com> wrote:
>
> Lars Schneider <larsxschneider@gmail.com> writes:
>
>> In 2841e8f ("convert: add "status=delayed" to filter process protocol",
>> 2017-06-30) we taught the filter process protocol to delayed responses.
>> These responses are processed after the "Checking out files" phase.
>> If the processing takes noticeable time, then the user might think Git
>> is stuck.
>>
>> Display the progress of the delayed responses to let the user know that
>> Git is still processing objects. This works very well for objects that
>> can be filtered quickly. If filtering of an individual object takes
>> noticeable time, then the user might still think that Git is stuck.
>> However, in that case the user would at least know what Git is doing.
>>
>> It would be technical more correct to display "Checking out files whose
>> content filtering has been delayed". For brevity we only print
>> "Filtering content".
>>
>> The finish_delayed_checkout() call was moved below the stop_progress()
>> call in unpack-trees.c to ensure that the "Checking out files" progress
>> is properly stopped before the "Filtering content" progress starts in
>> finish_delayed_checkout().
>>
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> Suggested-by: Taylor Blau <me@ttaylorr.com>
>> ---
>
> Makes sense. The only thing that made me wonder was if we want the
> change in unpack-trees.c in this patch. After all, the procedure to
> finish up the delayed checkout _is_ a part of the work need to be
> done to populate the working tree files, so stopping the progress
> before feels somewhat wrong at the phylosophical level.
>
> I think our output cannot express nested progress bars, and I think
> that is the reason why this patch tweaks unpack-trees.c; so I am
> fine with the end result (and that is why I said "made me wonder
> was", not "makes me wonder", the latter would imply "this we might
> want fix before applying", but I do not think we want to change
> anything this patch does to unpack-trees.c in this case).
>
> The delayed progress API is being simplified so I'll probably do a
> bit of evil merge while merging this to 'pu'.
>
> Thanks.
Hi Junio,
I just realized that this patch got lost :-(
That means 2.14.2 supports the delayed filters but does not show
progress to the user. To the user Git will appear hanging.
Can you merge this this topic for 2.14.3 / 2.15 ?
Thank you,
Lars
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] convert: display progress for filtered objects that have been delayed
2017-10-04 10:52 ` Lars Schneider
@ 2017-10-04 11:55 ` Lars Schneider
2017-10-04 11:55 ` Junio C Hamano
1 sibling, 0 replies; 6+ messages in thread
From: Lars Schneider @ 2017-10-04 11:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, me
> On 04 Oct 2017, at 12:52, Lars Schneider <larsxschneider@gmail.com> wrote:
>
>
>> On 24 Aug 2017, at 21:40, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Lars Schneider <larsxschneider@gmail.com> writes:
>>
>>> In 2841e8f ("convert: add "status=delayed" to filter process protocol",
>>> 2017-06-30) we taught the filter process protocol to delayed responses.
>>> These responses are processed after the "Checking out files" phase.
>>> If the processing takes noticeable time, then the user might think Git
>>> is stuck.
>>>
>>> Display the progress of the delayed responses to let the user know that
>>> Git is still processing objects. This works very well for objects that
>>> can be filtered quickly. If filtering of an individual object takes
>>> noticeable time, then the user might still think that Git is stuck.
>>> However, in that case the user would at least know what Git is doing.
>>>
>>> It would be technical more correct to display "Checking out files whose
>>> content filtering has been delayed". For brevity we only print
>>> "Filtering content".
>>>
>>> The finish_delayed_checkout() call was moved below the stop_progress()
>>> call in unpack-trees.c to ensure that the "Checking out files" progress
>>> is properly stopped before the "Filtering content" progress starts in
>>> finish_delayed_checkout().
>>>
>>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>>> Suggested-by: Taylor Blau <me@ttaylorr.com>
>>> ---
>>
>> Makes sense. The only thing that made me wonder was if we want the
>> change in unpack-trees.c in this patch. After all, the procedure to
>> finish up the delayed checkout _is_ a part of the work need to be
>> done to populate the working tree files, so stopping the progress
>> before feels somewhat wrong at the phylosophical level.
>>
>> I think our output cannot express nested progress bars, and I think
>> that is the reason why this patch tweaks unpack-trees.c; so I am
>> fine with the end result (and that is why I said "made me wonder
>> was", not "makes me wonder", the latter would imply "this we might
>> want fix before applying", but I do not think we want to change
>> anything this patch does to unpack-trees.c in this case).
>>
>> The delayed progress API is being simplified so I'll probably do a
>> bit of evil merge while merging this to 'pu'.
>>
>> Thanks.
>
> Hi Junio,
>
> I just realized that this patch got lost :-(
> That means 2.14.2 supports the delayed filters but does not show
> progress to the user. To the user Git will appear hanging.
>
> Can you merge this this topic for 2.14.3 / 2.15 ?
>
> Thank you,
> Lars
I should have expressed myself more clearly:
The patch made it into master with 52f1d62eb44faf569edca360ec9af9ddd4045fe0 .
Therefore, I think it will be released with 2.15, right?
The patch just did not make it into 2.14.2 and I wonder if you could
merge the patch for 2.14.3?
Thank you,
Lars
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] convert: display progress for filtered objects that have been delayed
2017-10-04 10:52 ` Lars Schneider
2017-10-04 11:55 ` Lars Schneider
@ 2017-10-04 11:55 ` Junio C Hamano
2017-10-04 13:33 ` Lars Schneider
1 sibling, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2017-10-04 11:55 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, me
Lars Schneider <larsxschneider@gmail.com> writes:
>> The delayed progress API is being simplified so I'll probably do a
>> bit of evil merge while merging this to 'pu'.
>
> I just realized that this patch got lost :-(
Really? Isn't it 52f1d62e ("convert: display progress for filtered
objects that have been delayed", 2017-08-20)? It already is in
'master' since about 3 weeks ago.
As this is merely a new "feature", I do not think it is reasonable
to expect it to be merged down to 'maint'. In fact, I think your
original patch <20170820154720.32259-1-larsxschneider@gmail.com>
declared that its Base Ref was "master", and I queued the topic on
top of the then-current 'master', so it is not mergeable to 'maint'.
It may be cherry-pickable, but I do not think it deserves to be. It
is not a fix for a grave bug or anything like that, right?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] convert: display progress for filtered objects that have been delayed
2017-10-04 11:55 ` Junio C Hamano
@ 2017-10-04 13:33 ` Lars Schneider
0 siblings, 0 replies; 6+ messages in thread
From: Lars Schneider @ 2017-10-04 13:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, me
> On 04 Oct 2017, at 13:55, Junio C Hamano <gitster@pobox.com> wrote:
>
> Lars Schneider <larsxschneider@gmail.com> writes:
>
>>> The delayed progress API is being simplified so I'll probably do a
>>> bit of evil merge while merging this to 'pu'.
>>
>> I just realized that this patch got lost :-(
>
> Really? Isn't it 52f1d62e ("convert: display progress for filtered
> objects that have been delayed", 2017-08-20)? It already is in
> 'master' since about 3 weeks ago.
>
> As this is merely a new "feature", I do not think it is reasonable
> to expect it to be merged down to 'maint'. In fact, I think your
> original patch <20170820154720.32259-1-larsxschneider@gmail.com>
> declared that its Base Ref was "master", and I queued the topic on
> top of the then-current 'master', so it is not mergeable to 'maint'.
>
> It may be cherry-pickable, but I do not think it deserves to be. It
> is not a fix for a grave bug or anything like that, right?
The original topic 2841e8f ("convert: add "status=delayed" to filter
process protocol", 2017-06-30) is a new feature. That's why I was
surprised to see it in 2.14.2! The problem is that this topic
might appear "broken" to the user without the progress output
introduce in 52f1d62e ("convert: display progress for filtered
objects that have been delayed", 2017-08-20).
In summary: 2841e8f was merged to 2.14.2 and 52f1d62e was not.
Therefore, it would be great to have 52f1d62e in 2.14.3.
Does this sound reasonable to you?
Thanks,
Lars
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-04 13:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-20 15:47 [PATCH v1] convert: display progress for filtered objects that have been delayed Lars Schneider
2017-08-24 19:40 ` Junio C Hamano
2017-10-04 10:52 ` Lars Schneider
2017-10-04 11:55 ` Lars Schneider
2017-10-04 11:55 ` Junio C Hamano
2017-10-04 13:33 ` Lars Schneider
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).