Re: [PATCH] reflog expire: add progress output

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 19, 2018, 17:22 UTC
Message-ID: <87tvmljtaz.fsf@evledraar.gmail.com>
In-Reply-To: <CACsJy8CX8xspbsW7Ta3aOD6LHh55ZaJ0tdrYeWDP_Vyw70NXtA@mail.gmail.com>

On Wed, Sep 19 2018, Duy Nguyen wrote:

Show 36 quoted lines

> On Wed, Sep 19, 2018 at 4:23 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> 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?

Show 8 quoted lines

>>         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.

Show 14 quoted lines

>>                 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
>>