On Wed, Sep 19 2018, Duy Nguyen wrote:
> On Wed, Sep 19, 2018 at 4:23 PM Ævar Arnfjörð Bjarmason
> <avarab@xxxxxxxxx> wrote:
>> Before this change the "git reflog expire" command didn't report any
>> progress.
>
> I love these progress additions you've been pushing lately :)
>
>> diff --git a/builtin/reflog.c b/builtin/reflog.c
>> index 3acef5a0ab..d3075ee75a 100644
>> --- a/builtin/reflog.c
>> +++ b/builtin/reflog.c
>> @@ -10,6 +10,7 @@
>> #include "diff.h"
>> #include "revision.h"
>> #include "reachable.h"
>> +#include "progress.h"
>>
>> /* NEEDSWORK: switch to using parse_options */
>> static const char reflog_expire_usage[] =
>> @@ -225,14 +226,20 @@ static void mark_reachable(struct expire_reflog_policy_cb *cb)
>> struct commit_list *pending;
>> timestamp_t expire_limit = cb->mark_limit;
>> struct commit_list *leftover = NULL;
>> + struct progress *progress = NULL;
>> + int i = 0;
>>
>> for (pending = cb->mark_list; pending; pending = pending->next)
>> pending->item->object.flags &= ~REACHABLE;
>>
>> pending = cb->mark_list;
>> + progress = start_delayed_progress(
>> + _("Marking unreachable commits in reflog for expiry"), 0);
>
> Maybe just "Searching expired reflog entries" or something like that.
> It's not technically as accurate, but I think it's easier to
> understand by by new people.
Or "Pruning reflog entries"? I was aiming for some combination of a)
making it clear what we're doing (pruning stuff) b) that we're doing it
on a subset of the counter of the (very large) values we're showing.
So the current one has "a" && "b", "Searching..." has neither, and
"Pruning..." has "a" but not "b".
Maybe making a && b clear isn't that important, but I'm currently
leaning towards keeping the current one because it's not *that* long and
makes things clearer to the user.
> Do we have --quiet option or something that needs to completely
> suppress this progress thing?
Yes. I also see my commit graph process patches sitting in "next" broke
the "git gc --quiet" mode, and I'll need to submit something on top
(which'll be easy), and submit a v2 on this (pending further
comments...).
Is there a better way to test that (fake up the file descriptor check)
in the tests other than adding getenv("GIT_TEST...") to the progress.c
logic?
>> while (pending) {
>> struct commit_list *parent;
>> struct commit *commit = pop_commit(&pending);
>> +
>> + display_progress(progress, ++i);
>
> maybe rename it to commit_count or something and leave "i" for
> temporary/short lived usage.
Good point. Willdo.
>> if (commit->object.flags & REACHABLE)
>> continue;
>> if (parse_commit(commit))
>> @@ -253,6 +260,7 @@ static void mark_reachable(struct expire_reflog_policy_cb *cb)
>> }
>> }
>> cb->mark_list = leftover;
>> + stop_progress(&progress);
>> }
>>
>> static int unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit, struct object_id *oid)
>> --
>> 2.19.0.444.g18242da7ef
>>
