Re: [PATCH] add --progress to git-gc and git-repack

From: Jeff King <peff@peff.net>
Date: Sep 27, 2011, 21:29 UTC
Message-ID: <20110927212910.GA5176@sigill.intra.peff.net>
In-Reply-To: <9F9C752E-6B3C-401B-9E01-8B1F5544A82F@gmail.com>

On Tue, Sep 27, 2011 at 10:32:45PM +0200, Oleg Andreev wrote:

> From 1f261e13e72770deabd77087e354f304be850efc Mon Sep 17 00:00:00 2001
> From: Oleg Andreev <oleganza@gmail.com>
> Date: Tue, 27 Sep 2011 08:24:25 +0200
> Subject: [PATCH 1/2] git-repack: pass --progress down to git-pack-objects

Please follow Documentation/SubmittingPatches. This should be part of the actual email headers. And there should be one patch per email.

My first thought was "doesn't git-repack already show progress?". There's no motivation in the commit message, so I have to guess why you want this. I assume you want to override the isatty(2) decision that pack-objects uses?

Show 13 quoted lines

> diff --git a/git-repack.sh b/git-repack.sh
> index 624feec..b86d60e 100755
> --- a/git-repack.sh
> +++ b/git-repack.sh
> [...]
> @@ -35,6 +36,7 @@ do
>  		unpack_unreachable=--unpack-unreachable ;;
>  	-d)	remove_redundant=t ;;
>  	-q)	GIT_QUIET=t ;;
> +	--progress) progress=--progress ;;
>  	-f)	no_reuse=--no-reuse-delta ;;
>  	-F)	no_reuse=--no-reuse-object ;;
>  	-l)	local=--local ;;

Should this also handle --no-progress? Maybe it is not a big deal, as "-q" will also suppress progress, but it would be consistent with other git commands that take --progress.

Show 9 quoted lines

> diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
> index 815afcb..b65fa3e 100644
> --- a/Documentation/git-gc.txt
> +++ b/Documentation/git-gc.txt
> [...]
>  --quiet::
> -	Suppress all progress reports.
> +	Suppress all progress reports. Progress is not reported to
> +	the standard error stream.

This just seems redundant to me.

Show 5 quoted lines

> +--progress::
> +	Progress status is reported on the standard error stream
> +	by default when it is attached to a terminal, unless -q
> +	is specified. This flag forces progress status even if the
> +	standard error stream is not directed to a terminal.

Though this is a nice description.

Show 5 quoted lines

> --- a/builtin/gc.c
> +++ b/builtin/gc.c
>  	struct option builtin_gc_options[] = {
>  		OPT__QUIET(&quiet, "suppress progress reporting"),
> +		OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),

This will handle --no-progress for us automatically, which is good.

Show 14 quoted lines

> diff --git a/contrib/examples/git-gc.sh b/contrib/examples/git-gc.sh
> index 1597e9f..52ea808 100755
> --- a/contrib/examples/git-gc.sh
> +++ b/contrib/examples/git-gc.sh
> [...]
>  while test $# != 0
>  do
>  	case "$1" in
>  	--prune)
>  		no_prune=
>  		;;
> +	--progress)
> +		progress=--progress
> +		;;

This won't, but I think this whole hunk is unnecessary. The files in contrib/examples are kept around for people to see how git commands can be composed of plumbing building blocks. But they don't need to gain new features as their C counterparts do. I think we can just leave them frozen in time as of when each script was rewritten in C.

-Peff