Re: [PATCH] filter-branch: add passed/remaining seconds on progress

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 21, 2015, 21:22 UTC
Message-ID: <CAPig+cRnVzRoyKOzPSJZd4JK_hB+_CBn0kjg4yYv=wWb-5vf7w@mail.gmail.com>
In-Reply-To: <xmqq6133a6tf.fsf@gitster.mtv.corp.google.com>

On Mon, Sep 21, 2015 at 3:52 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 41 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
>> On Tue, Sep 8, 2015 at 1:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>>> On Mon, Sep 7, 2015 at 9:52 AM, Gábor Bernát <bernat@primeranks.net> wrote:
>>>>...
>>>>>  # Rewrite the commits
>>>>> +report_progress ()
>>>>> +{
>>>>> +if test -n "$progress"
>>>>> +then
>>>>
>>>> Indent code within the function...
>>>
>>>>> +printf "\rRewrite $commit ($git_filter_branch__commit_count/$commits)$progress"
>>>>
>>>> The "\r" causes this status line to be overwritten each time through,
>>>> and since the processed commit count always increases, we know that
>>>> the original (without ETA) will never leave junk at the end of the
>>>> line. However, with estimated seconds also being displayed, does this
>>>> still hold?
>>>
>>> Good point.
>>> Perhaps like this squashed in?
>>>
>>> -printf "\rRewrite $commit ($git_filter_branch__commit_count/$commits)$progress"
>>> + printf "\rRewrite $commit
>>> ($git_filter_branch__commit_count/$commits)$progress "
>>
>> Yes, for an expedient "fix", this is what I had in mind, although I
>> would also have added an equal number of backspaces (\b) following the
>> spaces, as a minor aesthetic improvement.
>
> This topic seems to have stalled.  I do not want to discard topics
> because that means all the effort we spent to review and polish the
> patch so far gets wasted, but we cannot leave unfinished topics
> linger for too long.
>
> For now, I'll queue this SQUASH??? on top as a minimum fix (renaming
> of variables and other things noticed during the review may be worth
> doing, but they are not as grave as the issues this fixes, which are
> show stoppers).

Looks like a reasonable squash for moving this topic forward. Thanks.

Show 31 quoted lines

> I do not think our in-core progress code does that (and we do not
> use ESC[0K either), so I'll leave it out of the minimum fix.
>
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 565144a..71102d5 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -277,9 +277,8 @@ test $commits -eq 0 && die "Found nothing to rewrite"
>  # Rewrite the commits
>  report_progress ()
>  {
> -if test -n "$progress"
> -then
> -       if test $git_filter_branch__commit_count -gt $next_sample_at
> +       if test -n "$progress" &&
> +               test $git_filter_branch__commit_count -gt $next_sample_at
>         then
>                 now_timestamp=$(date +%s)
>                 elapsed_seconds=$(($now_timestamp - $start_timestamp))
> @@ -292,8 +291,7 @@ then
>                 fi
>                 progress=" ($elapsed_seconds seconds passed, remaining $remaining_second predicted)"
>         fi
> -fi
> -printf "\rRewrite $commit ($git_filter_branch__commit_count/$commits)$progress"
> +       printf "\rRewrite $commit ($git_filter_branch__commit_count/$commits)$progress    "
>  }
>
>  git_filter_branch__commit_count=0
> --
> 2.6.0-rc2-220-gd6fe230