threads / patch / 29193

patchmake "git push -v" actually verbose

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

## tl;dr

4 messages between Dec 17, 2011 and Dec 17, 2011. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Jeff King· Dec 17, 2011, 09:37 UTC · lore

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(-)

Show changes to transport.c +5 −5

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

Jeff King· Dec 17, 2011, 09:41 UTC · re: Jeff King · lore

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

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

Junio C Hamano· Dec 17, 2011, 19:34 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

Show 12 quoted lines

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

Jeff King· Dec 17, 2011, 19:50 UTC · re: Junio C Hamano · lore

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

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

Show 12 quoted lines

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