Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode

Junio C Hamano [off-list ref] writes:

Note that this chooses to expose what comes out of the standard
output of the subprocess to the standard error to be shown to the
user sitting on the other end.  This is in line with what we do to
all of our hooks (Cf. cd83c74 (Redirect update hook stdout to
stderr., 2006-12-30)).

Ok, now a tested patch, on top of your 1/2

-- >8 --
Subject: [PATCH] receive-pack: do not leak output from auto-gc to standard output

The standard output channel of receive-pack is a structured protocol
channel, and subprocesses must never be allowed to leak anything
into it by writing to their standard output.

Use RUN_COMMAND_STDOUT_TO_STDERR option to run_command_v_opt() just
like we do when running hooks to prevent output from "gc" leaking to
the standard output.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin/receive-pack.c | 3 ++-
 t/t5400-send-pack.sh   | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0afb8b2..3f05d97 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c

@@ -977,7 +977,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 			const char *argv_gc_auto[] = {
 				"gc", "--auto", "--quiet", NULL,
 			};
-			run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
+			int opt = RUN_GIT_CMD | RUN_COMMAND_STDOUT_TO_STDERR;
+			run_command_v_opt(argv_gc_auto, opt);
 		}
 		if (auto_update_server_info)
 			update_server_info(0);

diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 04a8791..250c720 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh

@@ -145,7 +145,7 @@ test_expect_success 'push --all excludes remote-tracking hierarchy' '
 	)
 '
 
-test_expect_failure 'receive-pack runs auto-gc in remote repo' '
+test_expect_success 'receive-pack runs auto-gc in remote repo' '
 	rm -rf parent child &&
 	git init parent &&
 	(

-- 
1.7.12.rc1.93.g8914ab8

On Mon, Aug 6, 2012 at 10:34 PM, Junio C Hamano [off-list ref] wrote:

Junio C Hamano [off-list ref] writes:

Ok, now a tested patch, on top of your 1/2

On Mon, Aug 6, 2012 at 10:32 PM, Jeff King [off-list ref] wrote:

This seems like a band-aid. The real problem is that auto-gc can
interfere with the pack protocol, which it should not be allowed to do,
no matter what it produces.

We could fix that root cause with this patch (on top of your 1/2):

Anyone else? :)

Ah, I wasn't aware of that feature of run_command.  Both look obviously correct.

And the comment I made yesterday about leaking the full path to the
remote end can be disregarded, since prune will report the path
relative to the repository base.

Thanks,
-Brandon

On Mon, Aug 06, 2012 at 10:44:07PM -0700, Brandon Casey wrote:

On Mon, Aug 6, 2012 at 10:32 PM, Jeff King [off-list ref] wrote:

quoted

This seems like a band-aid. The real problem is that auto-gc can
interfere with the pack protocol, which it should not be allowed to do,
no matter what it produces.

We could fix that root cause with this patch (on top of your 1/2):

Anyone else? :)

Sorry to gang up on you. :)

I still think your 2/2 is worth doing independently, though. It is silly
that git-prune will not mention pruned objects without "-v", but will
mention temporary files. They should be in the same category.

-Peff

On Mon, Aug 6, 2012 at 11:03 PM, Jeff King [off-list ref] wrote:

On Mon, Aug 06, 2012 at 10:44:07PM -0700, Brandon Casey wrote:

quoted

Anyone else? :)

Sorry to gang up on you. :)

Heh. :b

I still think your 2/2 is worth doing independently, though. It is silly
that git-prune will not mention pruned objects without "-v", but will
mention temporary files. They should be in the same category.

As I mentioned in an earlier message, I think the original thinking
was that removing a temporary object should be an unusual occurrence
that indicates a failure of some sort, so you want to inform the user
who may want to investigate (of course the file's gone, so what's to
investigate).  Removing a stale object file on the other hand is just
part of the normal operation.  That is why the former is always
printed out and the latter only when -v is used.

That was the original thinking, but I don't think it matters very
much.  Printing both using the same conditions seems valid.  My commit
message should be scrapped and replaced with something like your
paragraph though..

-Brandon