Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode
On Mon, Aug 6, 2012 at 11:03 PM, Jeff King p...@peff.net wrote: On Mon, Aug 06, 2012 at 10:44:07PM -0700, Brandon Casey wrote: 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode
Brandon Casey draf...@gmail.com writes: On Mon, Aug 6, 2012 at 11:03 PM, Jeff King p...@peff.net wrote: On Mon, Aug 06, 2012 at 10:44:07PM -0700, Brandon Casey wrote: 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 matches my understanding, modulo may want to investigate is probably more like may want to be reminded of an earlier repack that was aborted. That was the original thinking, but I don't think it matters very much. Printing both using the same conditions seems valid. Yeah, I agree that it does not make much difference either way and both ways of thinking feel equally valid. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode
Jeff King p...@peff.net writes: 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. Ok, so I'll queue it as a separate topic with a different justification. -- 8 -- From: Brandon Casey draf...@gmail.com Date: Mon, 6 Aug 2012 22:01:49 -0700 Subject: [PATCH] prune.c: only print informational message in show_only or verbose mode git prune reports removal of loose object files that are no longer necessary only under the -v option, but unconditionally reports removal of temporary files that are no longer needed. The original thinking was that presence of a leftover temporary file should be an unusual occurrence that may indicate an earlier failure of some sort, and the user may want to be reminded of it. Removing an unnecessary loose 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. But neither report is particularly useful. Hide both of these behind the -v option for consistency. Signed-off-by: Brandon Casey draf...@gmail.com Signed-off-by: Junio C Hamano gits...@pobox.com --- builtin/prune.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/prune.c b/builtin/prune.c index b99b635..6cb9944 100644 --- a/builtin/prune.c +++ b/builtin/prune.c @@ -25,7 +25,8 @@ static int prune_tmp_object(const char *path, const char *filename) return error(Could not stat '%s', fullpath); if (st.st_mtime expire) return 0; - printf(Removing stale temporary file %s\n, fullpath); + if (show_only || verbose) + printf(Removing stale temporary file %s\n, fullpath); if (!show_only) unlink_or_warn(fullpath); return 0; -- 1.7.12.rc2.53.g9ec2ef6 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode
On Tue, Aug 07, 2012 at 02:44:51PM -0700, Junio C Hamano wrote: Ok, so I'll queue it as a separate topic with a different justification. -- 8 -- From: Brandon Casey draf...@gmail.com Date: Mon, 6 Aug 2012 22:01:49 -0700 Subject: [PATCH] prune.c: only print informational message in show_only or verbose mode git prune reports removal of loose object files that are no longer necessary only under the -v option, but unconditionally reports removal of temporary files that are no longer needed. The original thinking was that presence of a leftover temporary file s/presence/the / should be an unusual occurrence that may indicate an earlier failure of some sort, and the user may want to be reminded of it. Removing an unnecessary loose 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. But neither report is particularly useful. Hide both of these behind the -v option for consistency. Signed-off-by: Brandon Casey draf...@gmail.com Signed-off-by: Junio C Hamano gits...@pobox.com --- Looks fine to me. I think tmpfile removal is also not that interesting in general. A stale file can happen any time the user aborts an operation via ^C. But I think your justification is sufficient as-is (and this topic is not worth spending too much more time on). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode
On Tue, Aug 7, 2012 at 2:44 PM, Junio C Hamano gits...@pobox.com wrote: Jeff King p...@peff.net writes: 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. Ok, so I'll queue it as a separate topic with a different justification. Looks fine to me. Thanks. -Brandon -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode
On Mon, Aug 06, 2012 at 10:01:49PM -0700, Brandon Casey wrote:
This informational message can cause a problem if 'git prune' is spawned
from an auto-gc during receive-pack. In this case, the informational
message will be sent back over the wire to the git client and the client
will try to interpret it as part of the pack protocol and will produce an
error.
So let's refrain from producing this message unless show_only or verbose
is enabled.
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):
-- 8 --
Subject: [PATCH] receive-pack: redirect auto-gc stdout to stderr
In some cases, git-gc may produce informational messages to
stdout, rather than stderr. This is bad for receive-pack,
because its stdout (and therefore that of its child) is
connected to a git client and speaking pack protocol.
Instead, let's redirect these messages to stderr to avoid
interference and let the client see them.
Signed-off-by: Jeff King p...@peff.net
---
We already do the same thing for all of the hooks we run. With this
change, all sub-processes have their stdout redirected (either to a
pipe, or to stderr) except git-unpack-objects.
Looking at unpack-objects, it should not write anything to stdout under
normal circumstances. However, if it is fed more bytes than the
pack data claims (e.g., extra entries beyond what the header claims), it
will send them to stdout. I've never heard of that happening, but
probably it should go to /dev/null, and/or flag an error.
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..e0b9f2e 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);
+ run_command_v_opt(argv_gc_auto,
+ RUN_GIT_CMD |
RUN_COMMAND_STDOUT_TO_STDERR);
}
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.12.g6d3a2d7
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode
Junio C Hamano gits...@pobox.com 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 gits...@pobox.com
---
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
