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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 6, 2015, 18:13 UTC
Message-ID: <xmqqpp1va00p.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1441545064-3126-1-git-send-email-bernat@primeranks.net>

Gábor Bernát <bernat@primeranks.net> writes:

Show 16 quoted lines

> +if date '+%s' 2>/dev/null | grep -q '^[0-9][0-9]*$'
> +then
> +	show_seconds=t
> +else
> +	show_seconds=
> +fi
> +
> +case "$show_seconds" in
> +t)
> +	start_timestamp=$(date +%s)
> +	next_sample_at=0
> +	;;
> +'')
> +	progress=""
> +	;;
> +esac

Why the case statement here?

For that matter, you probably do not need $show_seconds and use the fact that start_timestamp and progress are only set to a non-empty string when we measure progress, i.e. making all of the above something more like this:

        progress= start_timestamp=
        if date '+%s' 2>/dev/null | grep -q '^[0-9][0-9]*$'
        then
                next_sample_at=0
                progress="dummy to ensure this is not empty"
                start_timestamp=$(date '+%s')
        fi

If you are more efficiency-minded, I think you could even do with a single call to /bin/date, perhaps like so:

        next_sample_at= progress=
        start_timestamp=$(date '+%s' 2>/dev/null) 
        case "$start_timestamp" in
        *[!0-9]* | "")
                # not a "digit only" output
                ;;
        ?*)
                progress="dummy to ensure this is not empty"
                next_sample_at=0
                ;;
        esac

but I suspect that may be going a bit too far ;-).

Show 29 quoted lines

>  while read commit parents; do
>  	git_filter_branch__commit_count=$(($git_filter_branch__commit_count+1))
> -	printf "\rRewrite $commit ($git_filter_branch__commit_count/$commits)"
> +
> +	case "$show_seconds" in
> +	t)
> +		if test $git_filter_branch__commit_count -gt $next_sample_at
> +		then
> +			now_timestamp=$(date +%s)
> +			elapsed_seconds=$(($now_timestamp - $start_timestamp))
> +			remaining_second=$(( ($commits - $git_filter_branch__commit_count) * $elapsed_seconds / $git_filter_branch__commit_count ))
> +			if test $elapsed_seconds -gt 0
> +			then
> +				next_sample_at=$(( ($elapsed_seconds + 1) * $git_filter_branch__commit_count / $elapsed_seconds ))
> +			else
> +				next_sample_at=$(($next_sample_at + 1))
> +			fi
> +			progress=" ($elapsed_seconds seconds passed, remaining $remaining_second predicted)"
> +		fi
> +		;;
> +	'')
> +		progress=""
> +		;;
> +	esac
> +
> +	printf "\rRewrite $commit ($git_filter_branch__commit_count/$commits)$progress"
>  
>  	case "$filter_subdir" in
>  	"")

It would be easier to follow the logic of this loop whose _primary_ point is to rewrite one commit if you moved this part into a helper function. As written above, the deeply nested case statement whose purpose is only to show the progress is very distracting.

Then the loop would read like:

        while read commit parents
        do
                : $(( $git_filter_branch__commit_count++ ))
                report_progress

                case "$filter_subdir" in
                ...

                # all the work that is about rewriting this commit
                # comes here.

        done

which would be much less noisy and helpful to people who are interested in learning what it is doing.

As I said earlier in this message, the report_progress helper function can use the fact that $progress or $start_timestamp are non-empty if and only if you need to compute and tack $progress at the end of the output, i.e.

        report_progress ()
        {
                if test -n "$progress"
                then
                        ... do rate computation here ...
                        progress=" ($elapsed_seconds seconds passed,..."
                fi
                printf "\rRewrite $commit (...)$progress"
        }