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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 4, 2015, 18:34 UTC
Message-ID: <xmqqk2s6f2zj.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1441379798-15453-1-git-send-email-bernat@primeranks.net>

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

> @@ -277,9 +277,43 @@ test $commits -eq 0 && die "Found nothing to rewrite"
>  # Rewrite the commits
>  
>  git_filter_branch__commit_count=0

This is not a new problem, but I wonder why we need such a cumbersomely long variable name. It is not like this is a part of some shell script library that needs to be careful about namespace pollution.

> +echo $(date +%s) | grep -q '^[0-9]+$';  2>/dev/null && show_seconds=t

That is very strange construct. I think you meant to say something like

	if date '+%s' 2>/dev/null | grep -q '^[0-9][0-9]*$'
	then
		show_seconds=t
	else
        	show_seconds=
	fi

A handful of points:

 * "echo $(any-command)" is suspect, unless you are trying to let
   the shell munge output from any-command, which is not the case.

 * "grep" without -E (or "egrep") takes BRE, which "+" (one or more)
   is not part of.

 * That semicolon is a syntax error.  I think whoever suggested you
   to use it meant to squelch possible errors from "date" that does
   not understand the "%s" format.

 * I do not think you are clearing show_seconds to empty anywhere,
   so an environment variable the user may have when s/he starts
   filter-branch will seep through and confuse you.

Show 9 quoted lines

> +case "$show_seconds" in
> +	t)
> +		start_timestamp=$(date +%s)
> +		next_sample_at=0
> +		;;
> +	'')
> +		progress=""
> +		;;
> +esac

In our codebase case labels and case/esac align, like you did in the later part of the patch.

Show 27 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"

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. Then the loop would look more 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