[PATCH] make "git push -v" actually verbose

[PATCH] make "git push -v" actually verbose

[Jeff King]

Providing a single "-v" to "git push" currently does
nothing. Giving two flags ("git push -v -v") turns on the
first level of verbosity.

This is caused by a regression introduced in 8afd8dc (push:
support multiple levels of verbosity, 2010-02-24). Before
the series containing 8afd8dc, the verbosity handling for
fetching and pushing was completely separate. Commit bde873c
refactored the verbosity handling out of the fetch side, and
then 8afd8dc converted push to use the refactored code.

However, the fetch and push sides numbered and passed along
their verbosity levels differently. For both, a verbosity
level of "-1" meant "quiet", and "0" meant "default output".
But from there they differed.

For fetch, a verbosity level of "1" indicated to the "fetch"
program that it should make the status table slightly more
verbose, showing up-to-date entries. A verbosity level of
"2" meant that we should pass a verbose flag to the
transport; in the case of fetch-pack, this displays protocol
debugging information.

As a result, the refactored code in bde873c checks for
"verbosity >= 2", and only then passes it on to the
transport. From the transport code's perspective, a
verbosity of 0 or 1 both meant "0".

Push, on the other hand, does not show its own status table;
that is always handled by the transport layer or below
(originally send-pack itself, but these days it is done by
the transport code). So a verbosity level of 1 meant that we
should pass the verbose flag to send-pack, so that it knows
we want a verbose status table. However, once 8afd8dc
switched it to the refactored fetch code, a verbosity level
of 1 was now being ignored.  Thus, you needed to
artificially bump the verbosity to 2 (via "-v -v") to have
any effect.

We can fix this by letting the transport code know about the
true verbosity level (i.e., let it distinguish level 0 or
1).

We then have to also make an adjustment to any transport
methods that assumed "verbose > 0" meant they could spew
lots of debugging information. Before, they could only get
"0" or "2", but now they will also receive "1". They need to
adjust their condition for turning on such spew from
"verbose > 0" to "verbose > 1".

Signed-off-by: Jeff King <peff@peff.net>
---
This is an old bug, obviously, but it has been bugging me
off and on for the past year, so I finally decided to track
it down.

Sorry for the epic-length commit message. It was a subtle
bug, and the actual patch makes it very difficult to
understand why it works and doesn't regress the fetch case.
Hopefully it made sense. :)

 transport.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/transport.c b/transport.c
index 51814b5..48002b9 100644
--- a/transport.c
+++ b/transport.c
@@ -215,7 +215,7 @@ static struct ref *get_refs_via_rsync(struct transport *transport, int for_push)
 	rsync.argv = args;
 	rsync.stdout_to_stderr = 1;
 	args[0] = "rsync";
-	args[1] = (transport->verbose > 0) ? "-rv" : "-r";
+	args[1] = (transport->verbose > 1) ? "-rv" : "-r";
 	args[2] = buf.buf;
 	args[3] = temp_dir.buf;
 	args[4] = NULL;
@@ -268,7 +268,7 @@ static int fetch_objs_via_rsync(struct transport *transport,
 	rsync.argv = args;
 	rsync.stdout_to_stderr = 1;
 	args[0] = "rsync";
-	args[1] = (transport->verbose > 0) ? "-rv" : "-r";
+	args[1] = (transport->verbose > 1) ? "-rv" : "-r";
 	args[2] = "--ignore-existing";
 	args[3] = "--exclude";
 	args[4] = "info";
@@ -351,7 +351,7 @@ static int rsync_transport_push(struct transport *transport,
 	args[i++] = "-a";
 	if (flags & TRANSPORT_PUSH_DRY_RUN)
 		args[i++] = "--dry-run";
-	if (transport->verbose > 0)
+	if (transport->verbose > 1)
 		args[i++] = "-v";
 	args[i++] = "--ignore-existing";
 	args[i++] = "--exclude";
@@ -527,7 +527,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.lock_pack = 1;
 	args.use_thin_pack = data->options.thin;
 	args.include_tag = data->options.followtags;
-	args.verbose = (transport->verbose > 0);
+	args.verbose = (transport->verbose > 1);
 	args.quiet = (transport->verbose < 0);
 	args.no_progress = !transport->progress;
 	args.depth = data->options.depth;
@@ -981,7 +981,7 @@ int transport_set_option(struct transport *transport,
 void transport_set_verbosity(struct transport *transport, int verbosity,
 	int force_progress)
 {
-	if (verbosity >= 2)
+	if (verbosity >= 1)
 		transport->verbose = verbosity <= 3 ? verbosity : 3;
 	if (verbosity < 0)
 		transport->verbose = -1;
-- 
1.7.7.4.13.g57bf4

Re: [PATCH] make "git push -v" actually verbose

[Jeff King]

On Sat, Dec 17, 2011 at 04:37:15AM -0500, Jeff King wrote:

> Providing a single "-v" to "git push" currently does
> nothing. Giving two flags ("git push -v -v") turns on the
> first level of verbosity.

One minor clarification: it is not technically true that "git push -v"
does nothing. It just does not do the interesting "show a verbose status
table" operation, which is almost certainly what the user wants (and
what happened before the commits I mentioned). It does print "Pushing to
$url", since that happens above the transport layer. But I'm pretty sure
that is not what users of "-v" are interested in. :)

-Peff

Re: [PATCH] make "git push -v" actually verbose

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Dec 17, 2011 at 04:37:15AM -0500, Jeff King wrote:
>
>> Providing a single "-v" to "git push" currently does
>> nothing. Giving two flags ("git push -v -v") turns on the
>> first level of verbosity.
>
> One minor clarification: it is not technically true that "git push -v"
> does nothing. It just does not do the interesting "show a verbose status
> table" operation, which is almost certainly what the user wants (and
> what happened before the commits I mentioned). It does print "Pushing to
> $url", since that happens above the transport layer. But I'm pretty sure
> that is not what users of "-v" are interested in. :)

Thanks, but don't be so sure about that.

When you are so used to say "git push ko", from time to time you want to
check which one of your kernel.org machine you are pushing into; that
"pushing to this exact url" is actually quite useful.

Re: [PATCH] make "git push -v" actually verbose

[Jeff King]

On Sat, Dec 17, 2011 at 11:34:59AM -0800, Junio C Hamano wrote:

> > One minor clarification: it is not technically true that "git push -v"
> > does nothing. It just does not do the interesting "show a verbose status
> > table" operation, which is almost certainly what the user wants (and
> > what happened before the commits I mentioned). It does print "Pushing to
> > $url", since that happens above the transport layer. But I'm pretty sure
> > that is not what users of "-v" are interested in. :)
> 
> Thanks, but don't be so sure about that.
> 
> When you are so used to say "git push ko", from time to time you want to
> check which one of your kernel.org machine you are pushing into; that
> "pushing to this exact url" is actually quite useful.

But we will also print that as part of the status table, so it really is
redundant. The only difference is that it comes before the object
transfer phase, so if the whole thing barfs before you even make it to
the status table, you get a little more debugging output (although
typically the URL is mentioned in the die() message, anyway).

E.g.:

  $ git push github
  Counting objects: 214, done.
  Compressing objects: 100% (131/131), done.
  Writing objects: 100% (135/135), 33.73 KiB, done.
  Total 135 (delta 102), reused 9 (delta 4)
  To https://github.com/peff/git.git
   + 710a44a...0fa8107 private -> private (forced update)

Before my patch, adding "-v" would just put the "Pushing to https://..."
before the object phase.

Anyway, regardless of the utility of that message, I think the fix makes
sense. It really looks like an unintended regression in 8afd8dc, and
certainly the original intent of the code was to match fetch's use of
"-v" to show up-to-date entries in the status table (which I know
because I wrote it).

-Peff