how to suppress progress percentage in git-push

how to suppress progress percentage in git-push

[bill lam]

I set crontab to push to another computer for backup. It sent
confirmation email after finished.  It looked like

Counting objects: 1
Counting objects: 9, done.
Delta compression using up to 2 threads.
Compressing objects:  20% (1/5)
Compressing objects:  40% (2/5)
Compressing objects:  60% (3/5)
Compressing objects:  80% (4/5)
Compressing objects: 100% (5/5)
Compressing objects: 100% (5/5), done.
Writing objects:  20% (1/5)
Writing objects:  40% (2/5)
Writing objects:  60% (3/5)
Writing objects:  80% (4/5)
Writing objects: 100% (5/5)
Writing objects: 100% (5/5), 549 bytes, done.
Total 5 (delta 3), reused 0 (delta 0)

Often the list of progress % can be a page long.  I want output but
not those percentage progress status.  Will that be possible?

-- 
regards,
====================================================
GPG key 1024D/4434BAB3 2008-08-24
gpg --keyserver subkeys.pgp.net --recv-keys 4434BAB3

Re: how to suppress progress percentage in git-push

[Jeff King]

On Sun, Nov 22, 2009 at 10:53:53PM +0800, bill lam wrote:

> I set crontab to push to another computer for backup. It sent
> confirmation email after finished.  It looked like
> 
> Counting objects: 1
> Counting objects: 9, done.
> Delta compression using up to 2 threads.
> Compressing objects:  20% (1/5)
> Compressing objects:  40% (2/5)
> Compressing objects:  60% (3/5)
> Compressing objects:  80% (4/5)
> Compressing objects: 100% (5/5)
> Compressing objects: 100% (5/5), done.
> Writing objects:  20% (1/5)
> Writing objects:  40% (2/5)
> Writing objects:  60% (3/5)
> Writing objects:  80% (4/5)
> Writing objects: 100% (5/5)
> Writing objects: 100% (5/5), 549 bytes, done.
> Total 5 (delta 3), reused 0 (delta 0)
> 
> Often the list of progress % can be a page long.  I want output but
> not those percentage progress status.  Will that be possible?

Hmm. There seems to be a bug. pack-objects is supposed to see that
stderr is not a tty and suppress the progress messages. But it doesn't,
because send-pack gives it the --all-progress flag, which
unconditionally tells it to display progress, when the desired impact is
actually to just make the progress more verbose.

We need to do one of:

  1. make --all-progress imply "if we are using progress, then make it
     more verbose. Otherwise, ignore."

  2. fix all callers to check isatty(2) before unconditionally passing
     the option

The patch for (1) would look something like what's below.  It's simpler,
but it does change the semantics; anyone who was relying on
--all-progress to turn on progress unconditionally would need to now
also use --progress. However, turning on progress unconditionally is
usually an error (the except is if you are piping output in real-time to
the user and need to overcome the isatty check).

Nicolas, this is your code. Which do you prefer?

-Peff

---
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 4c91e94..50dd429 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -75,6 +75,7 @@ static int ignore_packed_keep;
 static int allow_ofs_delta;
 static const char *base_name;
 static int progress = 1;
+static int all_progress;
 static int window = 10;
 static uint32_t pack_size_limit, pack_size_limit_cfg;
 static int depth = 50;
@@ -2220,7 +2221,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 		if (!strcmp("--all-progress", arg)) {
-			progress = 2;
+			all_progress = 1;
 			continue;
 		}
 		if (!strcmp("-q", arg)) {
@@ -2295,6 +2296,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		usage(pack_usage);
 	}
 
+	if (progress && all_progress)
+		progress = 2;
+
 	/* Traditionally "pack-objects [options] base extra" failed;
 	 * we would however want to take refs parameter that would
 	 * have been given to upstream rev-list ourselves, which means

Re: how to suppress progress percentage in git-push

[Petr Baudis]

On Mon, Nov 23, 2009 at 10:00:00AM -0500, Jeff King wrote:
> The patch for (1) would look something like what's below.  It's simpler,
> but it does change the semantics; anyone who was relying on
> --all-progress to turn on progress unconditionally would need to now
> also use --progress. However, turning on progress unconditionally is
> usually an error (the except is if you are piping output in real-time to
> the user and need to overcome the isatty check).

I'm actually doing exactly that in the mirrorproj.cgi of Girocco, so I
would be unhappy if I would have to go through creating ptys or whatever
now. Maybe conditioning this by an environment variable?

				Petr "Pasky" Baudis

Re: how to suppress progress percentage in git-push

[Jeff King]

On Mon, Nov 23, 2009 at 04:50:43PM +0100, Petr Baudis wrote:

> On Mon, Nov 23, 2009 at 10:00:00AM -0500, Jeff King wrote:
> > The patch for (1) would look something like what's below.  It's simpler,
> > but it does change the semantics; anyone who was relying on
> > --all-progress to turn on progress unconditionally would need to now
> > also use --progress. However, turning on progress unconditionally is
> > usually an error (the except is if you are piping output in real-time to
> > the user and need to overcome the isatty check).
> 
> I'm actually doing exactly that in the mirrorproj.cgi of Girocco, so I
> would be unhappy if I would have to go through creating ptys or whatever
> now. Maybe conditioning this by an environment variable?

You wouldn't need to do anything that drastic. You would just need to
pass "--progress --all-progress" instead of only --all-progress. But you
have provided the data point that such a change would break at least one
user.

We could also leave --all-progress as-is and add new option to mean "if
you are already doing progress, do all progress".

-Peff

Re: how to suppress progress percentage in git-push

[Nicolas Pitre]

On Mon, 23 Nov 2009, Jeff King wrote:

> On Sun, Nov 22, 2009 at 10:53:53PM +0800, bill lam wrote:
> 
> > I set crontab to push to another computer for backup. It sent
> > confirmation email after finished.  It looked like
> > 
> > Counting objects: 1
> > Counting objects: 9, done.
> > Delta compression using up to 2 threads.
> > Compressing objects:  20% (1/5)
> > Compressing objects:  40% (2/5)
> > Compressing objects:  60% (3/5)
> > Compressing objects:  80% (4/5)
> > Compressing objects: 100% (5/5)
> > Compressing objects: 100% (5/5), done.
> > Writing objects:  20% (1/5)
> > Writing objects:  40% (2/5)
> > Writing objects:  60% (3/5)
> > Writing objects:  80% (4/5)
> > Writing objects: 100% (5/5)
> > Writing objects: 100% (5/5), 549 bytes, done.
> > Total 5 (delta 3), reused 0 (delta 0)
> > 
> > Often the list of progress % can be a page long.  I want output but
> > not those percentage progress status.  Will that be possible?
> 
> Hmm. There seems to be a bug. pack-objects is supposed to see that
> stderr is not a tty and suppress the progress messages. But it doesn't,
> because send-pack gives it the --all-progress flag, which
> unconditionally tells it to display progress, when the desired impact is
> actually to just make the progress more verbose.

Not exactly.

First, the progress variable is initialized with the result of isatty(2) 
by default.  The --progress argument is there to override that since 
pack-objects is often executed on the sending end of a fetch operation 
where stderr is not a terminal.

Then, during the pack-objects process, there are 3 phases: counting 
objects, compressing objects, and writing objects.  However in the fetch 
case we prefer to let the receiving end (index-pack or unpack-objects) 
take care of the progress display during the third phase.  This is why 
by default pack-objects doesn't display any "writing objects" progress 
when the generated pack is sent to stdout.  The --all-progress argument 
is there to override that, namely for a push.  The fact that 
--all-progress implies --progress is a bad side effect which wouldn't 
need to exist, but I don't think this is the cause of the issue here.

> We need to do one of:
> 
>   1. make --all-progress imply "if we are using progress, then make it
>      more verbose. Otherwise, ignore."
> 
>   2. fix all callers to check isatty(2) before unconditionally passing
>      the option

None of the above would fix the issue as this only affects progress 
display for phase #3.  You'd still get progress display for the counting 
phase and the compressing phase.

That doesn't mean it is OK for send-pack to unconditionally use 
--all-progress though, although it does provide the -q argument to 
pack-objects when push -q is used which inhibits any progress display 
already.


Nicolas

Re: how to suppress progress percentage in git-push

[Petr Baudis]

On Mon, Nov 23, 2009 at 11:43:19AM -0500, Jeff King wrote:
> On Mon, Nov 23, 2009 at 04:50:43PM +0100, Petr Baudis wrote:
> 
> > On Mon, Nov 23, 2009 at 10:00:00AM -0500, Jeff King wrote:
> > > The patch for (1) would look something like what's below.  It's simpler,
> > > but it does change the semantics; anyone who was relying on
> > > --all-progress to turn on progress unconditionally would need to now
> > > also use --progress. However, turning on progress unconditionally is
> > > usually an error (the except is if you are piping output in real-time to
> > > the user and need to overcome the isatty check).
> > 
> > I'm actually doing exactly that in the mirrorproj.cgi of Girocco, so I
> > would be unhappy if I would have to go through creating ptys or whatever
> > now. Maybe conditioning this by an environment variable?
> 
> You wouldn't need to do anything that drastic. You would just need to
> pass "--progress --all-progress" instead of only --all-progress. But you
> have provided the data point that such a change would break at least one
> user.
> 
> We could also leave --all-progress as-is and add new option to mean "if
> you are already doing progress, do all progress".

Hmm, maybe I'm confused - I just call

	git remote update

and don't pass any progress switches - would your change still affect
me? Can I pass --progress to `git remote update`?

-- 
				Petr "Pasky" Baudis
A lot of people have my books on their bookshelves.
That's the problem, they need to read them. -- Don Knuth

[PATCH] pack-objects: split implications of --all-progress from progress activation

[Nicolas Pitre]

Currently the --all-progress flag is used to use force progress display 
during the writing object phase even if output goes to stdout which is 
primarily the case during a push operation.  This has the unfortunate 
side effect of forcing progress display even if stderr is not a 
terminal.

Let's introduce the --all-progress-implied argument which has the same 
intent except for actually forcing the activation of any progress 
display.  With this, progress display will be automatically inhibited 
whenever stderr is not a terminal, or full progress display will be 
included otherwise.  This should let people use 'git push' within a cron 
job without filling their logs with useless percentage displays.

Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
---

On Mon, 23 Nov 2009, Jeff King wrote:

> We could also leave --all-progress as-is and add new option to mean "if
> you are already doing progress, do all progress".

Actually, that's what I was working on when I saw the above.

diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index 2e49929..f54d433 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -9,8 +9,9 @@ git-pack-objects - Create a packed archive of objects
 SYNOPSIS
 --------
 [verse]
-'git pack-objects' [-q] [--no-reuse-delta] [--delta-base-offset] [--non-empty]
-	[--local] [--incremental] [--window=N] [--depth=N] [--all-progress]
+'git pack-objects' [-q | --progress | --all-progress] [--all-progress-implied]
+	[--no-reuse-delta] [--delta-base-offset] [--non-empty]
+	[--local] [--incremental] [--window=N] [--depth=N]
 	[--revs [--unpacked | --all]*] [--stdout | base-name]
 	[--keep-true-parents] < object-list
 
@@ -137,7 +138,7 @@ base-name::
 
 --all-progress::
 	When --stdout is specified then progress report is
-	displayed during the object count and deltification phases
+	displayed during the object count and compression phases
 	but inhibited during the write-out phase. The reason is
 	that in some cases the output stream is directly linked
 	to another command which may wish to display progress
@@ -146,6 +147,11 @@ base-name::
 	report for the write-out phase as well even if --stdout is
 	used.
 
+--all-progress-implied::
+	This is used to imply --all-progress whenever progress display
+	is activated.  Unlike --all-progress this flag doesn't actually
+	force any progress display by itself.
+
 -q::
 	This flag makes the command not to report its progress
 	on the standard error stream.
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 4c91e94..4429d53 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -24,6 +24,7 @@
 
 static const char pack_usage[] =
   "git pack-objects [{ -q | --progress | --all-progress }]\n"
+  "        [--all-progress-implied]\n"
   "        [--max-pack-size=N] [--local] [--incremental]\n"
   "        [--window=N] [--window-memory=N] [--depth=N]\n"
   "        [--no-reuse-delta] [--no-reuse-object] [--delta-base-offset]\n"
@@ -2124,6 +2125,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 {
 	int use_internal_rev_list = 0;
 	int thin = 0;
+	int all_progress_implied = 0;
 	uint32_t i;
 	const char **rp_av;
 	int rp_ac_alloc = 64;
@@ -2223,6 +2225,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			progress = 2;
 			continue;
 		}
+		if (!strcmp("--all-progress-implied", arg)) {
+			all_progress_implied = 1;
+			continue;
+		}
 		if (!strcmp("-q", arg)) {
 			progress = 0;
 			continue;
@@ -2326,6 +2332,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	if (keep_unreachable && unpack_unreachable)
 		die("--keep-unreachable and --unpack-unreachable are incompatible.");
 
+	if (progress && all_progress_implied)
+		progress = 2;
+
 	prepare_packed_git();
 
 	if (progress)
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index d26997b..8fffdbf 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -40,7 +40,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	 */
 	const char *argv[] = {
 		"pack-objects",
-		"--all-progress",
+		"--all-progress-implied",
 		"--revs",
 		"--stdout",
 		NULL,
diff --git a/bundle.c b/bundle.c
index e04ab49..ff97adc 100644
--- a/bundle.c
+++ b/bundle.c
@@ -343,7 +343,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 
 	/* write pack */
 	argv_pack[0] = "pack-objects";
-	argv_pack[1] = "--all-progress";
+	argv_pack[1] = "--all-progress-implied";
 	argv_pack[2] = "--stdout";
 	argv_pack[3] = "--thin";
 	argv_pack[4] = NULL;

Re: [PATCH] pack-objects: split implications of --all-progress from progress activation

[Petr Baudis]

On Mon, Nov 23, 2009 at 12:43:50PM -0500, Nicolas Pitre wrote:
> Currently the --all-progress flag is used to use force progress display 
> during the writing object phase even if output goes to stdout which is 
> primarily the case during a push operation.  This has the unfortunate 
> side effect of forcing progress display even if stderr is not a 
> terminal.
> 
> Let's introduce the --all-progress-implied argument which has the same 
> intent except for actually forcing the activation of any progress 
> display.  With this, progress display will be automatically inhibited 
> whenever stderr is not a terminal, or full progress display will be 
> included otherwise.  This should let people use 'git push' within a cron 
> job without filling their logs with useless percentage displays.
> 
> Signed-off-by: Nicolas Pitre <nico@fluxnic.net>

Ok, but what is currently the way to force the old behaviour? I believe
that should be also part of the commit message.

Naive deduction fails:

	$ git remote update --progress
	error: unknown option `progress'

Thanks,

-- 
				Petr "Pasky" Baudis
A lot of people have my books on their bookshelves.
That's the problem, they need to read them. -- Don Knuth

Re: [PATCH] pack-objects: split implications of --all-progress from progress activation

[Nicolas Pitre]

On Mon, 23 Nov 2009, Petr Baudis wrote:

> On Mon, Nov 23, 2009 at 12:43:50PM -0500, Nicolas Pitre wrote:
> > Currently the --all-progress flag is used to use force progress display 
> > during the writing object phase even if output goes to stdout which is 
> > primarily the case during a push operation.  This has the unfortunate 
> > side effect of forcing progress display even if stderr is not a 
> > terminal.
> > 
> > Let's introduce the --all-progress-implied argument which has the same 
> > intent except for actually forcing the activation of any progress 
> > display.  With this, progress display will be automatically inhibited 
> > whenever stderr is not a terminal, or full progress display will be 
> > included otherwise.  This should let people use 'git push' within a cron 
> > job without filling their logs with useless percentage displays.
> > 
> > Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
> 
> Ok, but what is currently the way to force the old behaviour?

If any existing out-of-tree users of pack-objects were using 
--all-progress then nothing has changed for them.

> I believe that should be also part of the commit message.
> 
> Naive deduction fails:
> 
> 	$ git remote update --progress
> 	error: unknown option `progress'

Usage of 'git remote" is about fetching and not pushing, right? My patch 
only affects pushes.  So I don't know what old behavior you're after.


Nicolas

Re: [PATCH] pack-objects: split implications of --all-progress from progress activation

[Petr Baudis]

On Mon, Nov 23, 2009 at 01:27:14PM -0500, Nicolas Pitre wrote:
> On Mon, 23 Nov 2009, Petr Baudis wrote:
> > Naive deduction fails:
> > 
> > 	$ git remote update --progress
> > 	error: unknown option `progress'
> 
> Usage of 'git remote" is about fetching and not pushing, right? My patch 
> only affects pushes.  So I don't know what old behavior you're after.

Oh, I'm sorry, in that case I was confused from the very beginning;
somehow I saw pull instead of push in the initial mail.

				Petr "Pasky" Baudis

Re: how to suppress progress percentage in git-push

[Jeff King]

On Mon, Nov 23, 2009 at 11:56:43AM -0500, Nicolas Pitre wrote:

> > We need to do one of:
> > 
> >   1. make --all-progress imply "if we are using progress, then make it
> >      more verbose. Otherwise, ignore."
> > 
> >   2. fix all callers to check isatty(2) before unconditionally passing
> >      the option
> 
> None of the above would fix the issue as this only affects progress 
> display for phase #3.  You'd still get progress display for the counting 
> phase and the compressing phase.

I think it does fix the issue, as it is exactly the side effect that we
are concerned about (and I tested my patch for 1, which squelched it).
But your --all-progress-implied is a better fix, so I think we should go
with that.

> That doesn't mean it is OK for send-pack to unconditionally use 
> --all-progress though, although it does provide the -q argument to 
> pack-objects when push -q is used which inhibits any progress display 
> already.

It does, and I almost suggested that (although -q is new as of 1.6.5, so
it may not help the original poster). But "-q" actually does more than
that; it also silences any output for a successful push, which may not
be desired.

-Peff

Re: how to suppress progress percentage in git-push

[Jeff King]

On Mon, Nov 23, 2009 at 06:05:47PM +0100, Petr Baudis wrote:

> > You wouldn't need to do anything that drastic. You would just need to
> > pass "--progress --all-progress" instead of only --all-progress. But you
> > have provided the data point that such a change would break at least one
> > user.
> > 
> > We could also leave --all-progress as-is and add new option to mean "if
> > you are already doing progress, do all progress".
> 
> Hmm, maybe I'm confused - I just call
> 
> 	git remote update
> 
> and don't pass any progress switches - would your change still affect
> me? Can I pass --progress to `git remote update`?

Oh, I misunderstood; I thought you were calling pack-objects directly.
So you are actually relying on the "even though isatty(2) is not true,
we always print progress messages" behavior? I think that behavior is
buggy. It hurts everybody pushing via cron, and it violates the usual
rule for when we show progress messages.

I don't think you can get a --progress pushed all the way down to the
pack-objects in this case; we would need to add code to override the
isatty check.

That being said, your example of "remote update" means you are dealing
with fetch, and we are not touching the fetch code path at all.

-Peff

Re: [PATCH] pack-objects: split implications of --all-progress from progress activation

[Jeff King]

On Mon, Nov 23, 2009 at 12:43:50PM -0500, Nicolas Pitre wrote:

> Currently the --all-progress flag is used to use force progress display 
> during the writing object phase even if output goes to stdout which is 
> primarily the case during a push operation.  This has the unfortunate 
> side effect of forcing progress display even if stderr is not a 
> terminal.
> 
> Let's introduce the --all-progress-implied argument which has the same 
> intent except for actually forcing the activation of any progress 
> display.  With this, progress display will be automatically inhibited 
> whenever stderr is not a terminal, or full progress display will be 
> included otherwise.  This should let people use 'git push' within a cron 
> job without filling their logs with useless percentage displays.
> 
> Signed-off-by: Nicolas Pitre <nico@fluxnic.net>

Thanks,

Tested-by: Jeff King <peff@peff.net>

-Peff

Re: how to suppress progress percentage in git-push

[Nicolas Pitre]

On Mon, 23 Nov 2009, Jeff King wrote:

> On Mon, Nov 23, 2009 at 11:56:43AM -0500, Nicolas Pitre wrote:
> 
> > > We need to do one of:
> > > 
> > >   1. make --all-progress imply "if we are using progress, then make it
> > >      more verbose. Otherwise, ignore."
> > > 
> > >   2. fix all callers to check isatty(2) before unconditionally passing
> > >      the option
> > 
> > None of the above would fix the issue as this only affects progress 
> > display for phase #3.  You'd still get progress display for the counting 
> > phase and the compressing phase.
> 
> I think it does fix the issue, as it is exactly the side effect that we
> are concerned about (and I tested my patch for 1, which squelched it).

Sorry, you were right.  I somehow understood something else initially.

> But your --all-progress-implied is a better fix, so I think we should go
> with that.

OK.


Nicolas

Re: how to suppress progress percentage in git-push

[bill lam]

On Mon, 23 Nov 2009, Nicolas Pitre wrote:
> Then, during the pack-objects process, there are 3 phases: counting 
> objects, compressing objects, and writing objects.  However in the fetch 

during git-gc it shows yet another progress 

Removing duplicate objects: 100% (256/256), done.

-- 
regards,
====================================================
GPG key 1024D/4434BAB3 2008-08-24
gpg --keyserver subkeys.pgp.net --recv-keys 4434BAB3

Re: how to suppress progress percentage in git-push

[Jeff King]

On Tue, Nov 24, 2009 at 09:13:39AM +0800, bill lam wrote:

> On Mon, 23 Nov 2009, Nicolas Pitre wrote:
> > Then, during the pack-objects process, there are 3 phases: counting 
> > objects, compressing objects, and writing objects.  However in the fetch 
> 
> during git-gc it shows yet another progress 
> 
> Removing duplicate objects: 100% (256/256), done.

Thanks, this doesn't seem to have been guarded at all (but since it is
on a 2-second delay, you have to have quite a lot of loose objects or a
slow disk to trigger it).

We should apply the patch below to keep things consistent.

I also checked every other call to start_progress; everything else seems
to be guarded. Most of them were easy to trace to an isatty check,
though the one in unpack-trees is influenced by o->verbose_update. That
in turn usually corresponds to a quiet option, though merge does seem to
use it unconditionally. Maybe that should be tweaked, too?

-- >8 --
Subject: [PATCH] prune-packed: only show progress when stderr is a tty

This matches the behavior of other git programs, and helps
keep cruft out of things like cron job output.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-prune-packed.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-prune-packed.c b/builtin-prune-packed.c
index be99eb0..f9463de 100644
--- a/builtin-prune-packed.c
+++ b/builtin-prune-packed.c
@@ -71,7 +71,7 @@ void prune_packed_objects(int opts)
 
 int cmd_prune_packed(int argc, const char **argv, const char *prefix)
 {
-	int opts = VERBOSE;
+	int opts = isatty(2) ? VERBOSE : 0;
 	const struct option prune_packed_options[] = {
 		OPT_BIT('n', "dry-run", &opts, "dry run", DRY_RUN),
 		OPT_NEGBIT('q', "quiet", &opts, "be quiet", VERBOSE),
-- 
1.6.6.rc0.249.g9b4cf.dirty