[PATCH/RFC] Add progress options

[PATCH/RFC] Add progress options

[Brent Goodrick]

Add --progress, --no-progress, and --progress-verbose options, which
correspond to the GIT_PROGRESS environment variable setting of "1",
"0", and "verbose", respectively.

Signed-off-by: Brent Goodrick <bgoodr@gmail.com>
---

In reference to the "CR codes from git commands" thread, specifically
http://marc.info/?l=git&m=123273617713654&w=2 where Dscho kindly gave
me an outline on how to get started, which I reiterate below to show
where I felt I had to deviate:

Dscho> 	
Dscho> Hi,
Dscho> 
Dscho> On Fri, 23 Jan 2009, Brent Goodrick wrote:
Dscho> 
Dscho> >  - Bare minimum: Add a new --no-cr option
Dscho> 
Dscho> I do not see any value of this over "--progress | tr '\r' '\n'".  (The
Dscho> --progress option being the natural counterpart to --no-progress,
Dscho> _forcing_ the display of the progress.)
Dscho> 
Dscho> And I disagree that --no-progress would be hard to implement.  Just have a
Dscho> look at 7d1864c(Introduce is_bare_repository() and core.bare configuration
Dscho> variable).
Dscho> 
Dscho> Basically, you'll have to
Dscho> 
Dscho> - introduce a global variable to both environment.c and
Dscho> cache.h,
Dscho> 
Dscho> - set it to -1 by default,

I ended up having to rely exclusively upon an environment variable for
this state (indicated by GIT_PROGRESS_ENVIRONMENT in my patch which is
defined to be "GIT_PROGRESS") since parts of the code are in different
processes. This turned out to be a nice side-effect since I plan on
setting "GIT_PROGRESS" in my shell startup environment to be
"verbose", as for my work, I do indeed want to see all progress, but
just not all of the CR codes.

Dscho> 
Dscho> - handle a "--progress" and "--no-progress" option in git.c, setting the
Dscho>  global variable git_show_progress to 1 or 0, respectively,
Dscho> 
Dscho> - teach start_progress_delay() to return NULL if
Dscho> git_show_progress == 0,

I tried that, but then I got a worse condition of not seeing any
messaging at all for long-running processing. This might lead people
to think that something is hung up, and then CTRL-C'ing out, leaving
the repo in a bad state. What I thought we really needed instead is
just the final "done" messages, but not the intervening progress
messages such as percentage complete messages. To that end, I allowed
progress.c to continue to create the SIGALRM handler and logic as
before, but modify its output ever so slightly.

Dscho> 
Dscho> - modify all users of start_progress*() to respect git_show_progress == 1,
Dscho>  which probably means to look for "isatty" in builtin-pack-objects.c and
Dscho>  builtin-unpack-objects.c

I ended up not having to mess with isatty logic at all since only the
lines that are emitted with the CR codes had to be addressed.

Dscho> 
Dscho> - add documentation to Documentation/git.txt what --progress and
Dscho>  --no-progress do,
Dscho> 
Dscho> - add a simple test script to t/ (maybe t/t0005-progress.sh) that tests
Dscho>  that --progress works -- maybe you find a clever way to test
Dscho>  --no-progress, too, but that would be harder, as the progress is turned
Dscho>  off by default for the scripts anyway...)

I did not at this point attempt to write the test as a shell script,
but had to manually test it via my FSF Emacs session. There might be a
way to test this out via some elisp code in FSF Emacs, but I doubt we
want to do that since that would create a dependency on Emacs in the
git test suite.

Note that I found that the CR codes show up in git clone output in the
Emacs compile mode (M-x compile) which are displayed as "^M", but not
in the shell buffers (M-x shell).

I had sent a question to the mailing list about this specific issue
but got no answer, so I concluded to just proceed with the PATCH/RFC
first to see what folks say to the changes.

Thanks,
bgoodr

 Documentation/git.txt                  |   15 +++++++++++++++
 builtin-pack-objects.c                 |    2 +-
 cache.h                                |    1 +
 contrib/completion/git-completion.bash |    3 +++
 git.c                                  |   10 ++++++++--
 progress.c                             |   31 ++++++++++++++++++++++++++-----
 sideband.c                             |   29 +++++++++++++++++++++++++----
 7 files changed, 79 insertions(+), 12 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 0c7bba3..2f9d4e8 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 [verse]
 'git' [--version] [--exec-path[=GIT_EXEC_PATH]]
     [-p|--paginate|--no-pager]
+    [--progress|--no-progress|--progress-verbose]
     [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE]
     [--help] COMMAND [ARGS]
 
@@ -179,6 +180,20 @@ help ...`.
 --no-pager::
 	Do not pipe git output into a pager.
 
+--progress::
+	Show progress for long-running processing. This corresponds to
+	setting the GIT_PROGRESS environment variable to 1. This is
+	the default.
+
+--no-progress::
+	Instead of showing intermediate progress, just show
+	completion of major steps in processing. This corresponds to
+	setting the GIT_PROGRESS environment variable to 0.
+
+--progress-verbose::
+	Enable progress and show intermediate progress results as one
+	per line.
+
 --git-dir=<path>::
 	Set the path to the repository. This can also be controlled by
 	setting the GIT_DIR environment variable. It can be an absolute
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index cb51916..7ba5268 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -81,7 +81,7 @@ static int depth = 50;
 static int delta_search_threads;
 static int pack_to_stdout;
 static int num_preferred_base;
-static struct progress *progress_state;
+static struct progress *progress_state = NULL;
 static int pack_compression_level = Z_DEFAULT_COMPRESSION;
 static int pack_compression_seen;
 
diff --git a/cache.h b/cache.h
index 2d889de..6246fce 100644
--- a/cache.h
+++ b/cache.h
@@ -371,6 +371,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define GITATTRIBUTES_FILE ".gitattributes"
 #define INFOATTRIBUTES_FILE "info/attributes"
 #define ATTRIBUTE_MACRO_PREFIX "[attr]"
+#define GIT_PROGRESS_ENVIRONMENT "GIT_PROGRESS"
 #define GIT_NOTES_REF_ENVIRONMENT "GIT_NOTES_REF"
 #define GIT_NOTES_DEFAULT_REF "refs/notes/commits"
 
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 307bf5d..ca8b2d9 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1723,6 +1723,9 @@ _git ()
 		--*)   __gitcomp "
 			--paginate
 			--no-pager
+			--progress
+			--no-progress
+			--progress-verbose
 			--git-dir=
 			--bare
 			--version
diff --git a/git.c b/git.c
index c2b181e..0409471 100644
--- a/git.c
+++ b/git.c
@@ -5,7 +5,7 @@
 #include "run-command.h"
 
 const char git_usage_string[] =
-	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
+	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--progress|--no-progress|--progress-verbose] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
 
 const char git_more_info_string[] =
 	"See 'git help COMMAND' for more information on a specific command.";
@@ -116,7 +116,13 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);
 			if (envchanged)
 				*envchanged = 1;
-		} else {
+		} else if (!strcmp(cmd, "--progress"))
+			setenv(GIT_PROGRESS_ENVIRONMENT, "1", 1);
+		else if (!strcmp(cmd, "--no-progress"))
+			setenv(GIT_PROGRESS_ENVIRONMENT, "0", 1);
+		else if (!strcmp(cmd, "--progress-verbose"))
+			setenv(GIT_PROGRESS_ENVIRONMENT, "verbose", 1);
+		else {
 			fprintf(stderr, "Unknown option: %s\n", cmd);
 			usage(git_usage_string);
 		}
diff --git a/progress.c b/progress.c
index 55a8687..c96d594 100644
--- a/progress.c
+++ b/progress.c
@@ -10,6 +10,7 @@
 
 #include "git-compat-util.h"
 #include "progress.h"
+#include "cache.h"
 
 #define TP_IDX_MAX      8
 
@@ -36,6 +37,8 @@ struct progress {
 };
 
 static volatile sig_atomic_t progress_update;
+static int show_progress = 1;
+static int show_progress_verbose = 0;
 
 static void progress_interval(int signum)
 {
@@ -90,15 +93,22 @@ static int display(struct progress *progress, unsigned n, const char *done)
 
 	progress->last_value = n;
 	tp = (progress->throughput) ? progress->throughput->display : "";
-	eol = done ? done : "   \r";
+	/* When show_progress_verbose is true, put each progress message on a
+	 * separate line: */
+	eol = done ? done : (show_progress_verbose ? "   \n" : "   \r");
 	if (progress->total) {
 		unsigned percent = n * 100 / progress->total;
 		if (percent != progress->last_percent || progress_update) {
 			progress->last_percent = percent;
-			fprintf(stderr, "%s: %3u%% (%u/%u)%s%s",
-				progress->title, percent, n,
-				progress->total, tp, eol);
-			fflush(stderr);
+			/* Show progress everytime if specified. But if progress is turned
+			 * off, only show the last message (when done is non-NULL) as a
+			 * summary: */
+			if (show_progress || done) {
+				fprintf(stderr, "%s: %3u%% (%u/%u)%s%s",
+						progress->title, percent, n,
+						progress->total, tp, eol);
+				fflush(stderr);
+			}
 			progress_update = 0;
 			return 1;
 		}
@@ -206,6 +216,17 @@ struct progress *start_progress_delay(const char *title, unsigned total,
 				       unsigned percent_treshold, unsigned delay)
 {
 	struct progress *progress = malloc(sizeof(*progress));
+	const char * show_progress_env = getenv(GIT_PROGRESS_ENVIRONMENT);
+	if (show_progress_env) {
+		if (!strcmp(show_progress_env, "true"))
+			show_progress_env = "1";
+		if (!strcmp(show_progress_env, "false"))
+			show_progress_env = "0";
+		if (!strcmp(show_progress_env, "verbose"))
+			show_progress_verbose = 1;
+		else if (!strcmp(show_progress_env, "0"))
+			show_progress = 0;
+	}
 	if (!progress) {
 		/* unlikely, but here's a good fallback */
 		fprintf(stderr, "%s...\n", title);
diff --git a/sideband.c b/sideband.c
index cca3360..4bfb206 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,5 +1,6 @@
 #include "pkt-line.h"
 #include "sideband.h"
+#include "cache.h"
 
 /*
  * Receive multiplexed output stream over git native protocol.
@@ -26,7 +27,19 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 	char buf[LARGE_PACKET_MAX + 2*FIX_SIZE];
 	char *suffix, *term;
 	int skip_pf = 0;
-
+	int show_progress = 1;
+	int show_progress_verbose = 0;
+	const char * show_progress_env = getenv(GIT_PROGRESS_ENVIRONMENT);
+	if (show_progress_env) {
+		if (!strcmp(show_progress_env, "true"))
+			show_progress_env = "1";
+		if (!strcmp(show_progress_env, "false"))
+			show_progress_env = "0";
+		if (!strcmp(show_progress_env, "0"))
+			show_progress = 0;
+		else if (!strcmp(show_progress_env, "verbose"))
+			show_progress_verbose = 1;
+	}
 	memcpy(buf, PREFIX, pf);
 	term = getenv("TERM");
 	if (term && strcmp(term, "dumb"))
@@ -58,6 +71,7 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 			do {
 				char *b = buf;
 				int brk = 0;
+				int cr = 0;
 
 				/*
 				 * If the last buffer didn't end with a line
@@ -78,9 +92,14 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 						brk = 0;
 						break;
 					}
-					if (b[brk-1] == '\n' ||
-					    b[brk-1] == '\r')
+					if (b[brk-1] == '\n')
 						break;
+					else if (b[brk-1] == '\r') {
+						if (show_progress_verbose)
+							b[brk-1] = '\n';
+						cr = 1;
+						break;
+					}
 				}
 
 				/*
@@ -95,7 +114,9 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 					memcpy(save, b + brk, sf);
 					b[brk + sf - 1] = b[brk - 1];
 					memcpy(b + brk - 1, suffix, sf);
-					safe_write(err, b, brk + sf);
+					/* Progress messages are those that end in CR codes */
+					if (show_progress || !cr)
+						safe_write(err, b, brk + sf);
 					memcpy(b + brk, save, sf);
 					len -= brk;
 				} else {
-- 
1.6.2.rc0.3.ge80f6

[PATCH 0/4] clone: use --progress to mean -v

[Tay Ray Chuan]

This series makes git-clone follow the "argument convention" of
git-pack-objects, where the option --progress is used to force
reporting of reporting. This was previously done with -v/--verbose.

This in effect ensures a single consistent convention regarding
progress reporting for git commands. Having two conventions may
potentially confuse users.

On a related note, Brent wrote a patch a while back [1]. This series is
not as ambitious his and does not deal with the main git options. In
fact, only the last patch effects the titular change. If a consensus is
reached on this though, I don't rule out a separate patch/series to
set the convention at the main git level.

PS. If someone can enlighten me on the proper noun for the git
    executable (I said "main git"), I would be very thankful.

PPS. Merry Christmas and happy holidays. :)

Tay Ray Chuan (4):
  check stderr with isatty() instead of stdout when deciding to show
    progress
  git-clone.txt: reword description of progress behaviour
  clone: set transport->verbose when -v/--verbose is used
  clone: use --progress to force progress reporting

 Documentation/git-clone.txt |   12 +++++++++---
 builtin-clone.c             |    6 ++++++
 t/t5702-clone-options.sh    |    3 ++-
 transport-helper.c          |    2 +-
 transport.c                 |    2 +-
 transport.h                 |    2 +-
 6 files changed, 20 insertions(+), 7 deletions(-)

Footnotes:
[1] http://marc.info/?l=git&m=123415527432713

--
Cheers,
Ray Chuan

[PATCH 1/4] check stderr with isatty() instead of stdout when deciding to show progress

[Tay Ray Chuan]

Make transport code (viz. transport.c::fetch_refs_via_pack() and
transport-helper.c::standard_options()) that decides to show progress
check if stderr is a terminal, instead of stdout. After all, progress
reports (via the API in progress.[ch]) are sent to stderr.

Update the documentation for git-clone to say "standard error" as well.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 Documentation/git-clone.txt |    2 +-
 transport-helper.c          |    2 +-
 transport.c                 |    2 +-
 transport.h                 |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 7ccd742..f298fdd 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -101,7 +101,7 @@ objects from the source repository into a pack in the cloned repository.
 
 --verbose::
 -v::
-	Display the progress bar, even in case the standard output is not
+	Display the progress bar, even in case the standard error is not
 	a terminal.
 
 --no-checkout::
diff --git a/transport-helper.c b/transport-helper.c
index 11f3d7e..b886985 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -169,7 +169,7 @@ static void standard_options(struct transport *t)
 	char buf[16];
 	int n;
 	int v = t->verbose;
-	int no_progress = v < 0 || (!t->progress && !isatty(1));
+	int no_progress = v < 0 || (!t->progress && !isatty(2));
 
 	set_helper_option(t, "progress", !no_progress ? "true" : "false");
 
diff --git a/transport.c b/transport.c
index 3eea836..24c7f1d 100644
--- a/transport.c
+++ b/transport.c
@@ -476,7 +476,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.include_tag = data->followtags;
 	args.verbose = (transport->verbose > 0);
 	args.quiet = (transport->verbose < 0);
-	args.no_progress = args.quiet || (!transport->progress && !isatty(1));
+	args.no_progress = args.quiet || (!transport->progress && !isatty(2));
 	args.depth = data->depth;
 
 	for (i = 0; i < nr_heads; i++)
diff --git a/transport.h b/transport.h
index 9e74406..68fda6a 100644
--- a/transport.h
+++ b/transport.h
@@ -63,7 +63,7 @@ struct transport {
 	int (*disconnect)(struct transport *connection);
 	char *pack_lockfile;
 	signed verbose : 3;
-	/* Force progress even if the output is not a tty */
+	/* Force progress even if stderr is not a tty */
 	unsigned progress : 1;
 };
 
-- 
1.6.6.278.g3f5f

[PATCH 2/4] git-clone.txt: reword description of progress behaviour

[Tay Ray Chuan]

Mention progress reporting behaviour in the descriptions for -q/
--quiet and -v/--verbose options, in the style of git-pack-objects.txt.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 Documentation/git-clone.txt |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index f298fdd..e722e6c 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -96,13 +96,16 @@ objects from the source repository into a pack in the cloned repository.
 
 --quiet::
 -q::
-	Operate quietly.  This flag is also passed to the `rsync'
+	Operate quietly.  Progress is not reported to the standard
+	error stream. This flag is also passed to the `rsync'
 	command when given.
 
 --verbose::
 -v::
-	Display the progress bar, even in case the standard error is not
-	a terminal.
+	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.
 
 --no-checkout::
 -n::
-- 
1.6.6.278.g3f5f

[PATCH 3/4] clone: set transport->verbose when -v/--verbose is used

[Tay Ray Chuan]

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 builtin-clone.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 5df8b0f..463fbe4 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -525,8 +525,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 		if (option_quiet)
 			transport->verbose = -1;
-		else if (option_verbose)
+		else if (option_verbose) {
+			transport->verbose = 1;
 			transport->progress = 1;
+		}
 
 		if (option_upload_pack)
 			transport_set_option(transport, TRANS_OPT_UPLOADPACK,
-- 
1.6.6.278.g3f5f

[PATCH 4/4] clone: use --progress to force progress reporting

[Tay Ray Chuan]

Follow the argument convention of git-pack-objects, such that a
separate option (--preogress) is used to force progress reporting
instead of -v/--verbose.

-v/--verbose now does not force progress reporting. Make git-clone.txt
say so.

This should cover all the bases in 21188b1 (Implement git clone -v),
which implemented the option to force progress reporting.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 Documentation/git-clone.txt |    3 +++
 builtin-clone.c             |    8 ++++++--
 t/t5702-clone-options.sh    |    3 ++-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index e722e6c..f43c8b2 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -102,6 +102,9 @@ objects from the source repository into a pack in the cloned repository.
 
 --verbose::
 -v::
+	Run verbosely.
+
+--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
diff --git a/builtin-clone.c b/builtin-clone.c
index 463fbe4..58bacbd 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -44,10 +44,13 @@ static char *option_origin = NULL;
 static char *option_branch = NULL;
 static char *option_upload_pack = "git-upload-pack";
 static int option_verbose;
+static int option_progress;
 
 static struct option builtin_clone_options[] = {
 	OPT__QUIET(&option_quiet),
 	OPT__VERBOSE(&option_verbose),
+	OPT_BOOLEAN(0, "progress", &option_progress,
+			"force progress reporting"),
 	OPT_BOOLEAN('n', "no-checkout", &option_no_checkout,
 		    "don't create a checkout"),
 	OPT_BOOLEAN(0, "bare", &option_bare, "create a bare repository"),
@@ -525,10 +528,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 		if (option_quiet)
 			transport->verbose = -1;
-		else if (option_verbose) {
+		else if (option_verbose)
 			transport->verbose = 1;
+
+		if (option_progress)
 			transport->progress = 1;
-		}
 
 		if (option_upload_pack)
 			transport_set_option(transport, TRANS_OPT_UPLOADPACK,
diff --git a/t/t5702-clone-options.sh b/t/t5702-clone-options.sh
index 27825f5..02cb024 100755
--- a/t/t5702-clone-options.sh
+++ b/t/t5702-clone-options.sh
@@ -27,7 +27,8 @@ test_expect_success 'redirected clone' '
 '
 test_expect_success 'redirected clone -v' '
 
-	git clone -v "file://$(pwd)/parent" clone-redirected-v >out 2>err &&
+	git clone --progress "file://$(pwd)/parent" clone-redirected-progress \
+		>out 2>err &&
 	test -s err
 
 '
-- 
1.6.6.278.g3f5f

Re: [PATCH 4/4] clone: use --progress to force progress reporting

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

On Sat, Dec 26, 2009 at 01:12:06AM +0800, Tay Ray Chuan <rctay89@gmail.com> wrote:
> -v/--verbose now does not force progress reporting. Make git-clone.txt
> say so.
> 
> This should cover all the bases in 21188b1 (Implement git clone -v),
> which implemented the option to force progress reporting.
> 
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
> ---
>  Documentation/git-clone.txt |    3 +++
>  builtin-clone.c             |    8 ++++++--
>  t/t5702-clone-options.sh    |    3 ++-
>  3 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index e722e6c..f43c8b2 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -102,6 +102,9 @@ objects from the source repository into a pack in the cloned repository.
>  
>  --verbose::
>  -v::
> +	Run verbosely.

What about mentioning this "does not force progress reporting" behaviour
in documentation of the "new" -v option?

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 4/4] clone: use --progress to force progress reporting

[Tay Ray Chuan]

Hi,

On Sun, Dec 27, 2009 at 9:20 AM, Miklos Vajna <vmiklos@frugalware.org> wrote:
> On Sat, Dec 26, 2009 at 01:12:06AM +0800, Tay Ray Chuan <rctay89@gmail.com> wrote:
>> -v/--verbose now does not force progress reporting. Make git-clone.txt
>> say so.
>>
>> This should cover all the bases in 21188b1 (Implement git clone -v),
>> which implemented the option to force progress reporting.
>>
>> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
>> ---
>>  Documentation/git-clone.txt |    3 +++
>>  builtin-clone.c             |    8 ++++++--
>>  t/t5702-clone-options.sh    |    3 ++-
>>  3 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
>> index e722e6c..f43c8b2 100644
>> --- a/Documentation/git-clone.txt
>> +++ b/Documentation/git-clone.txt
>> @@ -102,6 +102,9 @@ objects from the source repository into a pack in the cloned repository.
>>
>>  --verbose::
>>  -v::
>> +     Run verbosely.
>
> What about mentioning this "does not force progress reporting" behaviour
> in documentation of the "new" -v option?

Ok.

-- 
Cheers,
Ray Chuan

Re: [PATCH 0/4] clone: use --progress to mean -v

[Johannes Schindelin]

Hi,

On Sat, 26 Dec 2009, Tay Ray Chuan wrote:

> This series makes git-clone follow the "argument convention" of 
> git-pack-objects, where the option --progress is used to force reporting 
> of reporting. This was previously done with -v/--verbose.

No objections from my side, although you might want to advertise more that 
this is a change in behavior.  (Meaning in the release notes)

> PS. If someone can enlighten me on the proper noun for the git
>     executable (I said "main git"), I would be very thankful.

I call it the "Git wrapper", although less polite words exist, too.

Ciao,
Dscho

Re: [PATCH 0/4] clone: use --progress to mean -v

[Tay Ray Chuan]

Hi,

On Sat, Dec 26, 2009 at 4:53 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Sat, 26 Dec 2009, Tay Ray Chuan wrote:
>
>> This series makes git-clone follow the "argument convention" of
>> git-pack-objects, where the option --progress is used to force reporting
>> of reporting. This was previously done with -v/--verbose.
>
> No objections from my side, although you might want to advertise more that
> this is a change in behavior.  (Meaning in the release notes)

Indeed, -v/--verbose to force reporting of progress was done sometime
last year (Thu Oct 9 2008) so there may be scripts/applications
dependent on this option.

Junio, do you have any advice on this front?

>> PS. If someone can enlighten me on the proper noun for the git
>>     executable (I said "main git"), I would be very thankful.
>
> I call it the "Git wrapper", although less polite words exist, too.

I see. Thanks!

-- 
Cheers,
Ray Chuan

Re: [PATCH 0/4] clone: use --progress to mean -v

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> On Sat, Dec 26, 2009 at 4:53 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> On Sat, 26 Dec 2009, Tay Ray Chuan wrote:
>>
>>> This series makes git-clone follow the "argument convention" of
>>> git-pack-objects, where the option --progress is used to force reporting
>>> of reporting. This was previously done with -v/--verbose.
>>
>> No objections from my side, although you might want to advertise more that
>> this is a change in behavior.  (Meaning in the release notes)
>
> Indeed, -v/--verbose to force reporting of progress was done sometime
> last year (Thu Oct 9 2008) so there may be scripts/applications
> dependent on this option.
>
> Junio, do you have any advice on this front?

[1/4] sounds like a sane thing to do regardless of the remainder of the
series, as stderr is where we write the progress output anyway. [2/4]
looks trivially correct.

It is unclear what impact [3/4] has.  I can read "With this patch,
transport can pay attention to the verbose option given from the end user
and act more verbosely, which was not something they couldn't do before",
but what is the practical difference our existing users would see?  IOW,
which transports are silent without this patch even when the user gives -v
from the command line?

And continuing the theme to separate the "verbosity" and the "progress"
into two separate switches, and push them down to the transport layer by
[3/4], [4/4] sounds like a logical conclusion.

I however wonder if it is of lessor impact if we only added --progress
but without removing the progress from -v.  Is there a downside?

Re: [PATCH 0/4] clone: use --progress to mean -v

[Tay Ray Chuan]

Hi,

On Tue, Dec 29, 2009 at 9:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Tay Ray Chuan <rctay89@gmail.com> writes:
>
>> On Sat, Dec 26, 2009 at 4:53 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>> On Sat, 26 Dec 2009, Tay Ray Chuan wrote:
>>>
>>>> This series makes git-clone follow the "argument convention" of
>>>> git-pack-objects, where the option --progress is used to force reporting
>>>> of reporting. This was previously done with -v/--verbose.
>>>
>>> No objections from my side, although you might want to advertise more that
>>> this is a change in behavior.  (Meaning in the release notes)
>>
>> Indeed, -v/--verbose to force reporting of progress was done sometime
>> last year (Thu Oct 9 2008) so there may be scripts/applications
>> dependent on this option.
>>
>> Junio, do you have any advice on this front?
>
> [1/4] sounds like a sane thing to do regardless of the remainder of the
> series, as stderr is where we write the progress output anyway. [2/4]
> looks trivially correct.
>
> It is unclear what impact [3/4] has.  I can read "With this patch,
> transport can pay attention to the verbose option given from the end user
> and act more verbosely, which was not something they couldn't do before",
> but what is the practical difference our existing users would see?  IOW,
> which transports are silent without this patch even when the user gives -v
> from the command line?

I know at least one transport which behaves in this manner (ie. silent
even when -v is supplied to git-clone), and that is the http (via
curl) transport.

> I however wonder if it is of lessor impact if we only added --progress
> but without removing the progress from -v.  Is there a downside?

(Just to clarify: progress reporting will be done if stderr is a
terminal - it will be done even if -v or --progress isn't present.
What -v/--progress does is force progress reporting even if stderr is
not a terminal.)

Leaving -v as it is (ie. forcing progress reporting) while adding
--progress would be a "safe" option, as it won't break people's
existing setups (ie. those that depend on -v to force progress
reporting), which the patch series does. I have in mind IDEs/editors
that use this behaviour to monitor progress.

On the other hand, if we decide -v shouldn't imply forcing progress
reporting, then I think this breakable change should be made soon,
when only a small minority of git commands are affected (only one,
git-clone). That way, we don't give users/integrators the impression
that -v forces progress reporting with git commands. They won't get
annoyed when try -v to force progress reporting and find that it isn't
the case.

By the way, I got this "-v doesn't imply forced progress reporting"
rule from Jeff (added to Cc list), who mentioned it some time ago:

  Date: Mon, 8 Jun 2009 07:54:31 -0400
  From: Jeff King <peff@peff.net>
  Subject: Re: [Patch] Prevent cloning over http from spewing
  Message-ID: <20090608115431.GC13775@coredump.intra.peff.net>

  I was imagining:

    - without "-q", show progress if isatty(1).

    - with "-q", never show progress

    - with "-v", show the "getting pack" and "walk" output we show now;
      without "-v", don't show it. "-v" has no impact on the progress
      indicator.

-- 
Cheers,
Ray Chuan