git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: 1.7.10 doesn't show file pushstatus
       [not found] <20120501010609.GA14715@jupiter.local>
@ 2012-05-01  6:58 ` Jeff King
  2012-05-01  7:33   ` Jeff King
  0 siblings, 1 reply; 14+ messages in thread
From: Jeff King @ 2012-05-01  6:58 UTC (permalink / raw)
  To: Clemens Buchacher; +Cc: dfowler, git, msysgit, Paul Betts, David Ebbo

> I just updated to msysGit 1.7.10 and I noticed I don't see any details
> while pushing (like file upload speed and % completion). Was this
> intentionally removed? If so why?

No, it's a regression. I can reproduce it easily, and it bisects to
Clemens' 01fdc21 (push/fetch/clone --no-progress suppresses progress
output) which went into v1.7.9.2 (and v1.7.10).

The problematic hunk is:

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 71f258e..9df341c 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -58,7 +58,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
                argv[i++] = "--thin";
        if (args->use_ofs_delta)
                argv[i++] = "--delta-base-offset";
-       if (args->quiet)
+       if (args->quiet || !args->progress)
                argv[i++] = "-q";
        if (args->progress)
                argv[i++] = "--progress";

which seems wrong to me. In send-pack, args->progress may be unset if we
didn't get a --progress flag on the command line. Shouldn't we be
falling back to isatty in that case (or leaving "-q" unset so that
pack-objects can do so)? Does it need to also be converted into a
tri-state of yes/no/unknown as the other places in that patch were?
Clemens?

-Peff

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: 1.7.10 doesn't show file pushstatus
  2012-05-01  6:58 ` 1.7.10 doesn't show file pushstatus Jeff King
@ 2012-05-01  7:33   ` Jeff King
  2012-05-01  8:40     ` Jeff King
  0 siblings, 1 reply; 14+ messages in thread
From: Jeff King @ 2012-05-01  7:33 UTC (permalink / raw)
  To: Clemens Buchacher; +Cc: dfowler, git, msysgit, Paul Betts, David Ebbo

On Tue, May 01, 2012 at 02:58:32AM -0400, Jeff King wrote:

> > I just updated to msysGit 1.7.10 and I noticed I don't see any details
> > while pushing (like file upload speed and % completion). Was this
> > intentionally removed? If so why?
> 
> No, it's a regression. I can reproduce it easily, and it bisects to
> Clemens' 01fdc21 (push/fetch/clone --no-progress suppresses progress
> output) which went into v1.7.9.2 (and v1.7.10).

Hmm. OK, I think I see what is going on. For most protocols, send_pack
happens without a separate process these days. So the stack is like:

  - transport_push
    - git_transport_push
      - send_pack

where the "progress" flag to send_pack is a boolean; we have already
figured out at the transport layer whether we want progress, and are
telling it what to do. Thus this hunk from 01fdc21 makes sense:

> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 71f258e..9df341c 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -58,7 +58,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
>                 argv[i++] = "--thin";
>         if (args->use_ofs_delta)
>                 argv[i++] = "--delta-base-offset";
> -       if (args->quiet)
> +       if (args->quiet || !args->progress)
>                 argv[i++] = "-q";
>         if (args->progress)
>                 argv[i++] = "--progress";

If we have been asked not to have progress, then we tell pack-objects
"-q" (which is, for historical reasons, the way to spell "--no-progress"
for pack-objects).

But send-pack is also a command in its own right, and gets invoked
separately for the --stateless-rpc case (i.e., smart http). In that case
you end up with:

  - transport_push
    - transport-helper.c:push_refs
      - [pipes to git-remote-https]
        - remote-curl.c:push_git
          - [forks send-pack]
            - cmd_send_pack
              - send_pack

In this case, send pack gets its arguments from the command-line, not
from the options set at the transport layer. Remote-curl will pass along
"--quiet" if we get that from the transport layer, but it does not
otherwise pass along the "progress" flag. So there are two problems:

  1. send-pack defaults its progress boolean to 0. Before 01fdc21, that
     was OK, because it meant "don't explicitly ask for progress". But
     after 01fdc21 that now means "explicitly ask for no progress", and
     the direct-transport code paths were updated without updating
     cmd_send_pack.

  2. There's no way to tell send-pack explicitly "yes, I would like
     progress, no matter what isatty(2) says". I doubt anybody cares
     much, but it probably makes sense to handle that for the sake of
     completeness.

-Peff

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: 1.7.10 doesn't show file pushstatus
  2012-05-01  7:33   ` Jeff King
@ 2012-05-01  8:40     ` Jeff King
  2012-05-01  8:41       ` [PATCH 1/3] send-pack: show progress when isatty(2) Jeff King
                         ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Jeff King @ 2012-05-01  8:40 UTC (permalink / raw)
  To: Clemens Buchacher
  Cc: Junio C Hamano, dfowler, git, msysgit, Paul Betts, David Ebbo

On Tue, May 01, 2012 at 03:33:26AM -0400, Jeff King wrote:

> In this case, send pack gets its arguments from the command-line, not
> from the options set at the transport layer. Remote-curl will pass along
> "--quiet" if we get that from the transport layer, but it does not
> otherwise pass along the "progress" flag. So there are two problems:
> 
>   1. send-pack defaults its progress boolean to 0. Before 01fdc21, that
>      was OK, because it meant "don't explicitly ask for progress". But
>      after 01fdc21 that now means "explicitly ask for no progress", and
>      the direct-transport code paths were updated without updating
>      cmd_send_pack.
> 
>   2. There's no way to tell send-pack explicitly "yes, I would like
>      progress, no matter what isatty(2) says". I doubt anybody cares
>      much, but it probably makes sense to handle that for the sake of
>      completeness.

The following patch series fixes this:

  [1/3]: send-pack: show progress when isatty(2)
  [2/3]: teach send-pack about --[no-]progress
  [3/3]: t5541: test more combinations of --progress

The first patch fixes (1) above, restoring send-pack's original behavior
(for remote-curl as well as anybody else who happens to call it). Note
that in doing so, it breaks "push --no-progress" for http remotes that
01fdc21 tried to fix. But it didn't actually fix it; it only appeared to
work because progress was _never_ on for http.  Fortunately, the
existing test in t5541 still passes because it's poorly written (it uses
both "--quiet" and "--no-progress", unmindful of the fact that the
latter does absolutely nothing).

The second patch fixes it correctly by propagating the options through
remote-curl (and as a bonus, it makes (2) above work). Other transport
helpers that use send-pack would want to do the same thing (but I don't
know of any that exist).

The third patch just expands the test coverage.

These are prepared directly on top of 01fdc21, so they can go on the
maint track.

-Peff

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/3] send-pack: show progress when isatty(2)
  2012-05-01  8:40     ` Jeff King
@ 2012-05-01  8:41       ` Jeff King
  2012-05-01  8:42       ` [PATCH 2/3] teach send-pack about --[no-]progress Jeff King
                         ` (3 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Jeff King @ 2012-05-01  8:41 UTC (permalink / raw)
  To: Clemens Buchacher
  Cc: Junio C Hamano, dfowler, git, msysgit, Paul Betts, David Ebbo

The send_pack_args struct has two verbosity flags: "quiet"
and "progress". Originally, if "quiet" was set, we would
tell pack-objects explicitly to be quiet, and if "progress"
was set, we would tell it to show progress. Otherwise, we
told it neither, and it relied on isatty(2) to make the
decision itself.

However, commit 01fdc21 changed the meaning of these
variables. Now both "quiet" and "!progress" instruct us to
tell pack-objects to be quiet (and a non-zero "progress"
means the same as before). This works well for transports
which call send_pack directly, as the transport code copies
transport->progress into send_pack_args->progress, and they
both have the same meaning.

However, the code path of calling "git send-pack" was left
behind. It always sets "progress" to 0, and thus always
tells pack-objects to be quiet.  We can work around this by
checking isatty(2) ourselves in the cmd_send_pack code path,
restoring the original behavior of the send-pack command.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/send-pack.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 9df341c..7d22715 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -492,6 +492,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	if (!args.quiet)
+		args.progress = isatty(2);
+
 	if (args.stateless_rpc) {
 		conn = NULL;
 		fd[0] = 0;
-- 
1.7.10.630.g31718

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/3] teach send-pack about --[no-]progress
  2012-05-01  8:40     ` Jeff King
  2012-05-01  8:41       ` [PATCH 1/3] send-pack: show progress when isatty(2) Jeff King
@ 2012-05-01  8:42       ` Jeff King
  2012-05-01  8:43       ` [PATCH 3/3] t5541: test more combinations of --progress Jeff King
                         ` (2 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Jeff King @ 2012-05-01  8:42 UTC (permalink / raw)
  To: Clemens Buchacher
  Cc: Junio C Hamano, dfowler, git, msysgit, Paul Betts, David Ebbo

The send_pack function gets a "progress" flag saying "yes,
definitely show progress" or "no, definitely do not show
progress". This gets set properly by transport_push when
send_pack is called directly.

However, when the send-pack command is executed separately
(as it is for the remote-curl helper), there is no way to
tell it "definitely do this". As a result, we do not
properly respect "git push --no-progress" for smart-http
remotes; you will still get progress if stderr is a tty.

This patch teaches send-pack --progress and --no-progress,
and teaches remote-curl to pass the appropriate option to
override send-pack's isatty check. This fixes the
--no-progress case above, and as a bonus, also makes "git
push --progress" work when stderr is not a tty.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/send-pack.c |   14 ++++++++++++--
 remote-curl.c       |    1 +
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 7d22715..d5d7105 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -410,6 +410,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	const char *receivepack = "git-receive-pack";
 	int flags;
 	int nonfastforward = 0;
+	int progress = -1;
 
 	argv++;
 	for (i = 1; i < argc; i++, argv++) {
@@ -452,6 +453,14 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 				args.verbose = 1;
 				continue;
 			}
+			if (!strcmp(arg, "--progress")) {
+				progress = 1;
+				continue;
+			}
+			if (!strcmp(arg, "--no-progress")) {
+				progress = 0;
+				continue;
+			}
 			if (!strcmp(arg, "--thin")) {
 				args.use_thin_pack = 1;
 				continue;
@@ -492,8 +501,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	if (!args.quiet)
-		args.progress = isatty(2);
+	if (progress == -1)
+		progress = !args.quiet && isatty(2);
+	args.progress = progress;
 
 	if (args.stateless_rpc) {
 		conn = NULL;
diff --git a/remote-curl.c b/remote-curl.c
index d159fe7..e5e9490 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -774,6 +774,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
 		argv[argc++] = "--quiet";
 	else if (options.verbosity > 1)
 		argv[argc++] = "--verbose";
+	argv[argc++] = options.progress ? "--progress" : "--no-progress";
 	argv[argc++] = url;
 	for (i = 0; i < nr_spec; i++)
 		argv[argc++] = specs[i];
-- 
1.7.10.630.g31718

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/3] t5541: test more combinations of --progress
  2012-05-01  8:40     ` Jeff King
  2012-05-01  8:41       ` [PATCH 1/3] send-pack: show progress when isatty(2) Jeff King
  2012-05-01  8:42       ` [PATCH 2/3] teach send-pack about --[no-]progress Jeff King
@ 2012-05-01  8:43       ` Jeff King
  2012-05-01  9:35         ` Clemens Buchacher
  2012-05-01 20:45         ` Zbigniew Jędrzejewski-Szmek
  2012-05-01  9:32       ` 1.7.10 doesn't show file pushstatus Clemens Buchacher
  2012-05-02  1:04       ` Johannes Schindelin
  4 siblings, 2 replies; 14+ messages in thread
From: Jeff King @ 2012-05-01  8:43 UTC (permalink / raw)
  To: Clemens Buchacher
  Cc: Junio C Hamano, dfowler, git, msysgit, Paul Betts, David Ebbo

Previously, we tested only that "push --quiet --no-progress"
was silent. However, there are many other combinations that
were not tested:

  1. no options at all (but stderr as a tty)
  2. --no-progress by itself
  3. --quiet by itself
  4. --progress (when stderr not a tty)

These are tested elsewhere for general "push", but it is
important to test them separately for http. It follows a
very different code path than git://, and options must be
relayed across a remote helper to a separate send-pack
process (and in fact cases (1), (2), and (4) have all been
broken just for http at some point in the past).

We can drop the "--quiet --no-progress" test, as it is not
really interesting (it is already handled by testing them
separately in (2) and (3) above).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5541-http-push.sh |   27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index d66ed24..a1b10bd 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -215,12 +215,35 @@ test_expect_success 'push --mirror to repo with alternates' '
 	git push --mirror "$HTTPD_URL"/smart/alternates-mirror.git
 '
 
-test_expect_success TTY 'quiet push' '
+test_expect_success TTY 'push shows progress when stderr is a tty' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	test_commit noisy &&
+	test_terminal git push 2>&1 | tee output &&
+	grep "^Writing objects" output
+'
+
+test_expect_success TTY 'push --quiet silences status and progress' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	test_commit quiet &&
-	test_terminal git push --quiet --no-progress 2>&1 | tee output &&
+	test_terminal git push --quiet 2>&1 | tee output &&
 	test_cmp /dev/null output
 '
 
+test_expect_success TTY 'push --no-progress silences progress but not status' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	test_commit no-progress &&
+	test_terminal git push --no-progress 2>&1 | tee output &&
+	grep "^To http" output &&
+	! grep "^Writing objects"
+'
+
+test_expect_success 'push --progress shows progress to non-tty' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	test_commit progress &&
+	git push --progress 2>&1 | tee output &&
+	grep "^To http" output &&
+	grep "^Writing objects" output
+'
+
 stop_httpd
 test_done
-- 
1.7.10.630.g31718

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: 1.7.10 doesn't show file pushstatus
  2012-05-01  8:40     ` Jeff King
                         ` (2 preceding siblings ...)
  2012-05-01  8:43       ` [PATCH 3/3] t5541: test more combinations of --progress Jeff King
@ 2012-05-01  9:32       ` Clemens Buchacher
  2012-05-01 17:33         ` Junio C Hamano
  2012-05-02  1:04       ` Johannes Schindelin
  4 siblings, 1 reply; 14+ messages in thread
From: Clemens Buchacher @ 2012-05-01  9:32 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, dfowler, git, msysgit, Paul Betts, David Ebbo

On Tue, May 01, 2012 at 04:40:49AM -0400, Jeff King wrote:
> 
> The following patch series fixes this:
> 
>   [1/3]: send-pack: show progress when isatty(2)
>   [2/3]: teach send-pack about --[no-]progress
>   [3/3]: t5541: test more combinations of --progress

Thanks. Looks good to me, although I completely missed this before. But
the tests should hopefully take care of any future regressions.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] t5541: test more combinations of --progress
  2012-05-01  8:43       ` [PATCH 3/3] t5541: test more combinations of --progress Jeff King
@ 2012-05-01  9:35         ` Clemens Buchacher
  2012-05-01  9:37           ` Jeff King
  2012-05-01 20:45         ` Zbigniew Jędrzejewski-Szmek
  1 sibling, 1 reply; 14+ messages in thread
From: Clemens Buchacher @ 2012-05-01  9:35 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, dfowler, git, msysgit, Paul Betts, David Ebbo

Can we add this on top or squashed in? I regret using tee when I
originally wrote the test.

--8<--
Subject: [PATCH] t5541: check return codes

By piping output to tee, the return code of the command is hidden.
Instead, redirect output to a file directly.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 t/t5541-http-push.sh |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 986210a..c07973e 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -218,21 +218,21 @@ test_expect_success 'push --mirror to repo with alternates' '
 test_expect_success TTY 'push shows progress when stderr is a tty' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	test_commit noisy &&
-	test_terminal git push 2>&1 | tee output &&
+	test_terminal git push >output 2>&1 &&
 	grep "^Writing objects" output
 '
 
 test_expect_success TTY 'push --quiet silences status and progress' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	test_commit quiet &&
-	test_terminal git push --quiet 2>&1 | tee output &&
+	test_terminal git push --quiet >output 2>&1 &&
 	test_cmp /dev/null output
 '
 
 test_expect_success TTY 'push --no-progress silences progress but not status' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	test_commit no-progress &&
-	test_terminal git push --no-progress 2>&1 | tee output &&
+	test_terminal git push --no-progress >output 2>&1 &&
 	grep "^To http" output &&
 	! grep "^Writing objects"
 '
@@ -240,7 +240,7 @@ test_expect_success TTY 'push --no-progress silences progress but not status' '
 test_expect_success 'push --progress shows progress to non-tty' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	test_commit progress &&
-	git push --progress 2>&1 | tee output &&
+	git push --progress >output 2>&1 &&
 	grep "^To http" output &&
 	grep "^Writing objects" output
 '
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] t5541: test more combinations of --progress
  2012-05-01  9:35         ` Clemens Buchacher
@ 2012-05-01  9:37           ` Jeff King
  2012-05-01 17:53             ` David Ebbo
  0 siblings, 1 reply; 14+ messages in thread
From: Jeff King @ 2012-05-01  9:37 UTC (permalink / raw)
  To: Clemens Buchacher
  Cc: Junio C Hamano, dfowler, git, msysgit, Paul Betts, David Ebbo

On Tue, May 01, 2012 at 11:35:01AM +0200, Clemens Buchacher wrote:

> Can we add this on top or squashed in? I regret using tee when I
> originally wrote the test.
> 
> --8<--
> Subject: [PATCH] t5541: check return codes
> 
> By piping output to tee, the return code of the command is hidden.
> Instead, redirect output to a file directly.

Ugh, absolutely. I blindly followed the existing style without thinking
about whether using tee was sane or not. And it's not. Thanks.

-Peff

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: 1.7.10 doesn't show file pushstatus
  2012-05-01  9:32       ` 1.7.10 doesn't show file pushstatus Clemens Buchacher
@ 2012-05-01 17:33         ` Junio C Hamano
  0 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2012-05-01 17:33 UTC (permalink / raw)
  To: Clemens Buchacher
  Cc: Jeff King, Junio C Hamano, dfowler, git, msysgit, Paul Betts,
	David Ebbo

Thanks, both.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] t5541: test more combinations of --progress
  2012-05-01  9:37           ` Jeff King
@ 2012-05-01 17:53             ` David Ebbo
  0 siblings, 0 replies; 14+ messages in thread
From: David Ebbo @ 2012-05-01 17:53 UTC (permalink / raw)
  To: msysgit
  Cc: Clemens Buchacher, Junio C Hamano, dfowler, git, Paul Betts,
	David Ebbo

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

Many thanks to Jeff King and others to help track this one down! 

David

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

[-- Attachment #2: Type: text/html, Size: 923 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] t5541: test more combinations of --progress
  2012-05-01  8:43       ` [PATCH 3/3] t5541: test more combinations of --progress Jeff King
  2012-05-01  9:35         ` Clemens Buchacher
@ 2012-05-01 20:45         ` Zbigniew Jędrzejewski-Szmek
  2012-05-01 20:53           ` Junio C Hamano
  1 sibling, 1 reply; 14+ messages in thread
From: Zbigniew Jędrzejewski-Szmek @ 2012-05-01 20:45 UTC (permalink / raw)
  To: Jeff King
  Cc: Clemens Buchacher, Junio C Hamano, dfowler, git, msysgit,
	Paul Betts, David Ebbo

On 05/01/2012 10:43 AM, Jeff King wrote:
> -test_expect_success TTY 'quiet push' '
> +test_expect_success TTY 'push shows progress when stderr is a tty' '
> +	cd "$ROOT_PATH"/test_repo_clone &&
> +	test_commit noisy &&
> +	test_terminal git push 2>&1 | tee output &&
> +	grep "^Writing objects" output
> +'
> +
> +test_expect_success TTY 'push --quiet silences status and progress' '
>  	cd "$ROOT_PATH"/test_repo_clone &&
>  	test_commit quiet &&
> -	test_terminal git push --quiet --no-progress 2>&1 | tee output &&
> +	test_terminal git push --quiet 2>&1 | tee output &&
>  	test_cmp /dev/null output
>  '
>  
> +test_expect_success TTY 'push --no-progress silences progress but not status' '
> +	cd "$ROOT_PATH"/test_repo_clone &&
> +	test_commit no-progress &&
> +	test_terminal git push --no-progress 2>&1 | tee output &&
> +	grep "^To http" output &&
> +	! grep "^Writing objects"
        ! grep "^Writing objects" output

> +'
> +
> +test_expect_success 'push --progress shows progress to non-tty' '
> +	cd "$ROOT_PATH"/test_repo_clone &&
> +	test_commit progress &&
> +	git push --progress 2>&1 | tee output &&
> +	grep "^To http" output &&
> +	grep "^Writing objects" output
> +'
> +
I understand that test_i18ngrep is not necessary, because pack-objects.c
is not internationalized. But wouldn't it make sense to use
test_i18ngrep in preparation, so that tests don't have to be modified
later on?

-
Zbyszek

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] t5541: test more combinations of --progress
  2012-05-01 20:45         ` Zbigniew Jędrzejewski-Szmek
@ 2012-05-01 20:53           ` Junio C Hamano
  0 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2012-05-01 20:53 UTC (permalink / raw)
  To: Zbigniew Jędrzejewski-Szmek
  Cc: Jeff King, Clemens Buchacher, dfowler, git, msysgit, Paul Betts,
	David Ebbo

Zbigniew Jędrzejewski-Szmek  <zbyszek@in.waw.pl> writes:

>> +test_expect_success 'push --progress shows progress to non-tty' '
>> +	cd "$ROOT_PATH"/test_repo_clone &&
>> +	test_commit progress &&
>> +	git push --progress 2>&1 | tee output &&
>> +	grep "^To http" output &&
>> +	grep "^Writing objects" output
>> +'
>> +
> I understand that test_i18ngrep is not necessary, because pack-objects.c
> is not internationalized. But wouldn't it make sense to use
> test_i18ngrep in preparation, so that tests don't have to be modified
> later on?

In this test, we are not interested in making sure the progress output is
properly localized.  I'd rather see it keep using grep and if you really
care, run "git push" under "LANG=C LC_ALL=C" or something reliable instead.

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Re: 1.7.10 doesn't show file pushstatus
  2012-05-01  8:40     ` Jeff King
                         ` (3 preceding siblings ...)
  2012-05-01  9:32       ` 1.7.10 doesn't show file pushstatus Clemens Buchacher
@ 2012-05-02  1:04       ` Johannes Schindelin
  4 siblings, 0 replies; 14+ messages in thread
From: Johannes Schindelin @ 2012-05-02  1:04 UTC (permalink / raw)
  To: Jeff King
  Cc: Clemens Buchacher, Junio C Hamano, dfowler, git, msysgit,
	Paul Betts, David Ebbo

Hi Peff,

On Tue, 1 May 2012, Jeff King wrote:

> On Tue, May 01, 2012 at 03:33:26AM -0400, Jeff King wrote:
> 
> > In this case, send pack gets its arguments from the command-line, not
> > from the options set at the transport layer. Remote-curl will pass along
> > "--quiet" if we get that from the transport layer, but it does not
> > otherwise pass along the "progress" flag. So there are two problems:
> > 
> >   1. send-pack defaults its progress boolean to 0. Before 01fdc21, that
> >      was OK, because it meant "don't explicitly ask for progress". But
> >      after 01fdc21 that now means "explicitly ask for no progress", and
> >      the direct-transport code paths were updated without updating
> >      cmd_send_pack.
> > 
> >   2. There's no way to tell send-pack explicitly "yes, I would like
> >      progress, no matter what isatty(2) says". I doubt anybody cares
> >      much, but it probably makes sense to handle that for the sake of
> >      completeness.
> 
> The following patch series fixes this:
> 
>   [1/3]: send-pack: show progress when isatty(2)
>   [2/3]: teach send-pack about --[no-]progress
>   [3/3]: t5541: test more combinations of --progress

Thanks. I applied and pushed it after testing on Linux.

David, if you want to relieve me of maintenance burden in the future, you

# apply the patches

# push them to a fork on GitHub

# run the tests

# report back to the mailing list (including the outcome of the test --
  detailed, if it fails)

Ciao,
Johannes

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2012-05-02  1:04 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20120501010609.GA14715@jupiter.local>
2012-05-01  6:58 ` 1.7.10 doesn't show file pushstatus Jeff King
2012-05-01  7:33   ` Jeff King
2012-05-01  8:40     ` Jeff King
2012-05-01  8:41       ` [PATCH 1/3] send-pack: show progress when isatty(2) Jeff King
2012-05-01  8:42       ` [PATCH 2/3] teach send-pack about --[no-]progress Jeff King
2012-05-01  8:43       ` [PATCH 3/3] t5541: test more combinations of --progress Jeff King
2012-05-01  9:35         ` Clemens Buchacher
2012-05-01  9:37           ` Jeff King
2012-05-01 17:53             ` David Ebbo
2012-05-01 20:45         ` Zbigniew Jędrzejewski-Szmek
2012-05-01 20:53           ` Junio C Hamano
2012-05-01  9:32       ` 1.7.10 doesn't show file pushstatus Clemens Buchacher
2012-05-01 17:33         ` Junio C Hamano
2012-05-02  1:04       ` Johannes Schindelin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).