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

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

[Gábor Bernát]

From: Gabor Bernat <gabor.bernat@gravityrd.com>

adds seconds progress and estimated seconds time if getting the current
timestamp is supported by the date %+s command

Signed-off-by: Gabor Bernat <gabor.bernat@gravityrd.com>
---

I've submitted this first to this list as a feature request, however
in the meantime with the help of Jeff King <peff@peff.net>, Junio C
Hamano <gitster@pobox.com>, Eric Sunshine <sunshine@sunshineco.com>
and Mikael Magnusson <mikachu@gmail.com> came up with solution, so now
I submit it as a revised patch.

The current solution updates the progress for all commits until 1
second time is elapsed. Afterwards updates it at most once a second.
---
 git-filter-branch.sh | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5b3f63d..5e9ae0f 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -277,9 +277,43 @@ test $commits -eq 0 && die "Found nothing to rewrite"
 # Rewrite the commits
 
 git_filter_branch__commit_count=0
+
+echo $(date +%s) | grep -q '^[0-9]+$';  2>/dev/null && show_seconds=t
+case "$show_seconds" in
+	t)
+		start_timestamp=$(date +%s)
+		next_sample_at=0
+		;;
+	'')
+		progress=""
+		;;
+esac
+
 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
 	"")
-- 
2.5.1.408.g431338e

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

[Junio C Hamano]

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.

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

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

[Eric Sunshine]

On Fri, Sep 4, 2015 at 2:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Gábor Bernát <bernat@primeranks.net> writes:
>> +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

The final format suggested[1] for this test was:

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

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

Primarily my fault. I don't know what I was thinking when suggesting that.

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

This seems to have mutated from the suggested form.

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

This also mutated. The suggested form wanted to suppress errors from
'date' if it complained about "%s", and from 'grep'. In retrospect,
applying it to 'grep' is questionable. I was recalling this warning
from the Autoconf manual[2]:

    Some of the options required by Posix are not portable in
    practice. Don't use ‘grep -q’ to suppress output, because many
    grep implementations (e.g., Solaris) do not support -q. Don't use
    ‘grep -s’ to suppress output either, because Posix says -s does
    not suppress output, only some error messages; also, the -s
    option of traditional grep behaved like -q does in most modern
    implementations. Instead, redirect the standard output and
    standard error (in case the file doesn't exist) of grep to
    /dev/null. Check the exit status of grep to determine whether it
    found a match.

however, Git tests use 'grep -q' heavily, so perhaps we don't worry about that.

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

The empty assignment was implied in my example, but I should have been
more explicit and shown a more complete snippet:

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

The suggested 'if' form has the attribute of being clearer.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/276531/focus=276837
[2]: https://www.gnu.org/software/autoconf/manual/autoconf.html#grep

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

[Gabor Bernat]

On Fri, Sep 4, 2015 at 10:15 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Sep 4, 2015 at 2:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Gábor Bernát <bernat@primeranks.net> writes:
>>> +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
>
> The final format suggested[1] for this test was:
>
>     { echo $(date +%s) | grep -q '^[0-9][0-9]*$'; } 2>/dev/null &&
>         show_eta=t
>
>> 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.
>
> Primarily my fault. I don't know what I was thinking when suggesting that.

Yes, the initial construct was different, willing to make this change.

>
>>  * "grep" without -E (or "egrep") takes BRE, which "+" (one or more)
>>    is not part of.
>
> This seems to have mutated from the suggested form.
>
>>  * 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.
>
> This also mutated. The suggested form wanted to suppress errors from
> 'date' if it complained about "%s", and from 'grep'. In retrospect,
> applying it to 'grep' is questionable. I was recalling this warning
> from the Autoconf manual[2]:
>
>     Some of the options required by Posix are not portable in
>     practice. Don't use ‘grep -q’ to suppress output, because many
>     grep implementations (e.g., Solaris) do not support -q. Don't use
>     ‘grep -s’ to suppress output either, because Posix says -s does
>     not suppress output, only some error messages; also, the -s
>     option of traditional grep behaved like -q does in most modern
>     implementations. Instead, redirect the standard output and
>     standard error (in case the file doesn't exist) of grep to
>     /dev/null. Check the exit status of grep to determine whether it
>     found a match.
>
> however, Git tests use 'grep -q' heavily, so perhaps we don't worry about that.

So we should keep it as it is.

>
>>  * 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.
>
> The empty assignment was implied in my example, but I should have been
> more explicit and shown a more complete snippet:
>
>     show_eta=
>     ...
>     { echo $(date +%s) | grep -q '^[0-9][0-9]*$'; } 2>/dev/null &&
>         show_eta=t
>
> The suggested 'if' form has the attribute of being clearer.

My bad, sorry for that. Will amend.

>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/276531/focus=276837
> [2]: https://www.gnu.org/software/autoconf/manual/autoconf.html#grep

Any other pain points, or this construction will satisfy everybody?

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

[Eric Sunshine]

On Sun, Sep 6, 2015 at 5:49 AM, Gabor Bernat <bernat@primeranks.net> wrote:
> On Fri, Sep 4, 2015 at 10:15 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Fri, Sep 4, 2015 at 2:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Gábor Bernát <bernat@primeranks.net> writes:
>>>> +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
>>
>> This also mutated. The suggested form wanted to suppress errors from
>> 'date' if it complained about "%s", and from 'grep'. In retrospect,
>> applying it to 'grep' is questionable. I was recalling this warning
>> from the Autoconf manual[2]:
>>
>>     Some of the options required by Posix are not portable in
>>     practice. Don't use ‘grep -q’ to suppress output, because many
>>     grep implementations (e.g., Solaris) do not support -q. Don't use
>>     ‘grep -s’ to suppress output either, because Posix says -s does
>>     not suppress output, only some error messages; also, the -s
>>     option of traditional grep behaved like -q does in most modern
>>     implementations. Instead, redirect the standard output and
>>     standard error (in case the file doesn't exist) of grep to
>>     /dev/null. Check the exit status of grep to determine whether it
>>     found a match.
>>
>> however, Git tests use 'grep -q' heavily, so perhaps we don't worry about that.
>
> So we should keep it as it is.

Use of 'grep -q' seems to be fine, however, Junio's comment was about
the errant semicolon, which should not be kept.

>>>  * 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.
>>
>> The empty assignment was implied in my example, but I should have been
>> more explicit and shown a more complete snippet:
>>
>>     show_eta=
>>     ...
>>     { echo $(date +%s) | grep -q '^[0-9][0-9]*$'; } 2>/dev/null &&
>>         show_eta=t
>>
>> The suggested 'if' form has the attribute of being clearer.
>
> My bad, sorry for that. Will amend.
>
> Any other pain points, or this construction will satisfy everybody?

Junio's proposed if/then/else construct should be satisfactory.

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

[Gábor Bernát]

From: Gabor Bernat <gabor.bernat@gravityrd.com>

adds seconds progress and estimated seconds time if getting the current
timestamp is supported by the date %+s command

Signed-off-by: Gabor Bernat <gabor.bernat@gravityrd.com>
---

I've submitted this first to this list as a feature request, however
in the meantime with the help of Jeff King <peff@peff.net>, Junio C
Hamano <gitster@pobox.com>, Eric Sunshine <sunshine@sunshineco.com>
and Mikael Magnusson <mikachu@gmail.com> came up with solution, so now
I submit it as a revised patch.

The current solution updates the progress for all commits until 1
second time is elapsed. Afterwards updates it at most once a second.
---
 git-filter-branch.sh | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5b3f63d..924cf3d 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -277,9 +277,49 @@ test $commits -eq 0 && die "Found nothing to rewrite"
 # Rewrite the commits
 
 git_filter_branch__commit_count=0
+
+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
+
 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
 	"")
-- 
2.6.0.rc0.3.gb3280a4

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

[Junio C Hamano]

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

> +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 ;-).

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

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

[Gábor Bernát]

From: Gabor Bernat <gabor.bernat@gravityrd.com>

adds seconds progress and estimated seconds time if getting the current
timestamp is supported by the date %+s command

Signed-off-by: Gabor Bernat <gabor.bernat@gravityrd.com>
---

I've submitted this first to this list as a feature request, however
in the meantime with the help of Jeff King <peff@peff.net>, Junio C
Hamano <gitster@pobox.com>, Eric Sunshine <sunshine@sunshineco.com>
and Mikael Magnusson <mikachu@gmail.com> came up with solution, so now
I submit it as a revised patch.

The current solution updates the progress for all commits until 1
second time is elapsed. Afterwards updates it at most once a second.

Ammended build up as agreed at [1].

[1] http://thread.gmane.org/gmane.comp.version-control.git/277314
---
 git-filter-branch.sh | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5b3f63d..565144a 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -275,11 +275,41 @@ commits=$(wc -l <../revs | tr -d " ")
 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
+	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
+fi
+printf "\rRewrite $commit ($git_filter_branch__commit_count/$commits)$progress"
+}
 
 git_filter_branch__commit_count=0
+
+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
+
 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)"
+
+	report_progress
 
 	case "$filter_subdir" in
 	"")
-- 
2.6.0.rc0.3.gb3280a4

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

[Ramsay Jones]

On 07/09/15 13:31, Gábor Bernát wrote:
> From: Gabor Bernat <gabor.bernat@gravityrd.com>
>
> adds seconds progress and estimated seconds time if getting the current
> timestamp is supported by the date %+s command

s/%+s/+%s/

ATB,
Ramsay Jones

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

[Gábor Bernát]

From: Gabor Bernat <gabor.bernat@gravityrd.com>

adds seconds progress and estimated seconds time if getting the current
timestamp is supported by the date +%s command

Signed-off-by: Gabor Bernat <gabor.bernat@gravityrd.com>
---

I've submitted this first to this list as a feature request, however
in the meantime with the help of Jeff King <peff@peff.net>, Junio C
Hamano <gitster@pobox.com>, Eric Sunshine <sunshine@sunshineco.com>
and Mikael Magnusson <mikachu@gmail.com> came up with solution, so now
I submit it as a revised patch.

The current solution updates the progress for all commits until 1
second time is elapsed. Afterwards updates it at most once a second.

Ammended build up as agreed at [1].

[1] http://thread.gmane.org/gmane.comp.version-control.git/277314
---
 git-filter-branch.sh | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5b3f63d..565144a 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -275,11 +275,41 @@ commits=$(wc -l <../revs | tr -d " ")
 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
+	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
+fi
+printf "\rRewrite $commit ($git_filter_branch__commit_count/$commits)$progress"
+}
 
 git_filter_branch__commit_count=0
+
+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
+
 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)"
+
+	report_progress
 
 	case "$filter_subdir" in
 	"")
-- 
2.6.0.rc0.3.gb3280a4

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

[Eric Sunshine]

On Mon, Sep 7, 2015 at 9:52 AM, Gábor Bernát <bernat@primeranks.net> wrote:
> From: Gabor Bernat <gabor.bernat@gravityrd.com>
>
> adds seconds progress and estimated seconds time if getting the current
> timestamp is supported by the date +%s command
>
> Signed-off-by: Gabor Bernat <gabor.bernat@gravityrd.com>
> ---
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 5b3f63d..565144a 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -275,11 +275,41 @@ commits=$(wc -l <../revs | tr -d " ")
>  test $commits -eq 0 && die "Found nothing to rewrite"
>
>  # Rewrite the commits
> +report_progress ()
> +{
> +if test -n "$progress"
> +then

Indent code within the function...

> +       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
> +fi
> +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? While it's true that elapsed seconds will increase,
estimated seconds may jump around, requiring different numbers of
digits to display. This may leave "garbage" digits at the end of line
from previous iterations, can't it?

> +}
>
>  git_filter_branch__commit_count=0
> +
> +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
> +
>  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)"
> +
> +       report_progress
>
>         case "$filter_subdir" in
>         "")
> --
> 2.6.0.rc0.3.gb3280a4

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

[Junio C Hamano]

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

Also git_filter_branch__commit_count is now used only inside this
function, so it is easier to follow to increment it here.

I suspect that the variable has this unwieldy name for historic
reasons, perhaps an attempt to avoid name clashes with the end user
script, but it has many variables (e.g. $commits, $ref, etc.) that
are way too generic and that I can see no attempt of name clash
avoidance, so renaming it to $total_commits or something _might_
make some sense.

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

 git-filter-branch.sh | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 565144a..30ef513 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -277,27 +277,28 @@ 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
+	git_filter_branch__commit_count=$(($git_filter_branch__commit_count+1))
+
+	if test -n "$progress"
 	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
+		if test "$git_filter_branch__commit_count" -gt "$next_sample_at"
 		then
-			next_sample_at=$(( ($elapsed_seconds + 1) * $git_filter_branch__commit_count / $elapsed_seconds ))
-		else
-			next_sample_at=$(($next_sample_at + 1))
+			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=" ($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
-
 progress= start_timestamp=
 if date '+%s' 2>/dev/null | grep -q '^[0-9][0-9]*$'
 then
@@ -306,9 +307,8 @@ then
 		start_timestamp=$(date '+%s')
 fi
 
-while read commit parents; do
-	git_filter_branch__commit_count=$(($git_filter_branch__commit_count+1))
-
+while read commit parents
+do
 	report_progress
 
 	case "$filter_subdir" in

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

[Eric Sunshine]

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...
>
> Also git_filter_branch__commit_count is now used only inside this
> function, so it is easier to follow to increment it here.

Make sense.

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

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

[Junio C Hamano]

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...
>>
>> Also git_filter_branch__commit_count is now used only inside this
>> function, so it is easier to follow to increment it here.
>
> Make sense.
>
>>>> +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).

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.


 git-filter-branch.sh | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

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

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

[Eric Sunshine]

On Mon, Sep 21, 2015 at 3:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

> 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

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

[Gabor Bernat]

On Mon, Sep 21, 2015 at 11:24 PM, Gábor Bernát
<gabor.bernat@gravityrd.com> wrote:
> On Mon, Sep 21, 2015 at 11:22 PM, Eric Sunshine <sunshine@sunshineco.com>
> wrote:
>>
>> On Mon, Sep 21, 2015 at 3:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> > 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.
>>
>> > 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
>
>
> Agreed, :) did not abandoned this, just got caught up with many stuff.
> Thanks for the help,
>

So do I need to do anything else with this? :)

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

[Junio C Hamano]

Gabor Bernat <bernat@primeranks.net> writes:

> On Mon, Sep 21, 2015 at 11:24 PM, Gábor Bernát
> ...
>> Agreed, :) did not abandoned this, just got caught up with many stuff.
>> Thanks for the help,
>
> So do I need to do anything else with this? :)

If you can fetch from me to see if the output from

    git log -p origin/master..71400d97b12a

looks sensible, that would be good.  There are two commits.

Thanks.

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

[Gabor Bernat]

On Tue, Sep 22, 2015 at 6:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Gabor Bernat <bernat@primeranks.net> writes:
>
>> On Mon, Sep 21, 2015 at 11:24 PM, Gábor Bernát
>> ...
>>> Agreed, :) did not abandoned this, just got caught up with many stuff.
>>> Thanks for the help,
>>
>> So do I need to do anything else with this? :)
>
> If you can fetch from me to see if the output from
>
>     git log -p origin/master..71400d97b12a
>
> looks sensible, that would be good.  There are two commits.
>
> Thanks.
I can sign this off as good and sensible. Nice work, thanks :)

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

[Jeff King]

On Tue, Sep 08, 2015 at 10:32:12AM -0700, Junio C Hamano wrote:

> Also git_filter_branch__commit_count is now used only inside this
> function, so it is easier to follow to increment it here.
> 
> I suspect that the variable has this unwieldy name for historic
> reasons, perhaps an attempt to avoid name clashes with the end user
> script, but it has many variables (e.g. $commits, $ref, etc.) that
> are way too generic and that I can see no attempt of name clash
> avoidance, so renaming it to $total_commits or something _might_
> make some sense.

I briefly wondered if it had the opposite reason; could it have a
well-defined name because it is meant to be a public value the
user-defined shell snippets can access?

But it is not documented, and I can imagine that "the current count" is
not really useful without "total number of commits", so in practice I
doubt anybody's filter branch script is relying on it.

And looking through the history turns up d5b0c97 (git-filter-branch:
avoid collisions with variables in eval'ed commands, 2009-03-25), which
seems fairly clear. :)

The original name was "i", which I think is probably too short. Calling
it something meaningful but longer than one character is probably
sufficient.

-Peff