[v3,2/3] bundle-create: progress output control
diff mbox series

Message ID 20191110204126.30553-2-robbat2@gentoo.org
State New
Headers show
Series
  • [v3,1/3] bundle: framework for options before bundle file
Related show

Commit Message

Robin H. Johnson Nov. 10, 2019, 8:41 p.m. UTC
Support the progress output options from pack-objects in git-bundle's
create subcommand. Most notably, this provides --quiet as requested on
the git mailing list per [1]

Reference: https://www.mail-archive.com/git@vger.kernel.org/msg182844.html <robbat2-20190806T191156-796782357Z@orbis-terrarum.net>
Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
---
 Documentation/git-bundle.txt | 33 +++++++++++++++++++++++++++++++--
 builtin/bundle.c             | 30 +++++++++++++++++++++++++++---
 bundle.c                     |  9 +++++----
 bundle.h                     |  3 ++-
 4 files changed, 65 insertions(+), 10 deletions(-)

Comments

Jeff King Nov. 11, 2019, 4:07 a.m. UTC | #1
On Sun, Nov 10, 2019 at 12:41:25PM -0800, Robin H. Johnson wrote:

> Support the progress output options from pack-objects in git-bundle's
> create subcommand. Most notably, this provides --quiet as requested on
> the git mailing list per [1]
> 
> Reference: https://www.mail-archive.com/git@vger.kernel.org/msg182844.html <robbat2-20190806T191156-796782357Z@orbis-terrarum.net>

I'm glad you included the message-id here, since "182844" is useless if
mail-archive.com ever goes away. We usually just cite public-inbox for
that reason, since its URLs just use the message-id anyway:

  https://public-inbox.org/git/robbat2-20190806T191156-796782357Z@orbis-terrarum.net

> +--progress::
> +	Progress status is reported on the standard error stream
> +	by default when it is attached to a terminal, unless -q
> +	is specified. This flag forces progress status even if
> +	the standard error stream is not directed to a terminal.
> +
> +--all-progress::
> +	When --stdout is specified then progress report is
> +	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
> +	status of its own as it processes incoming pack data.
> +	This flag is like --progress except that it forces progress
> +	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::
> +--quiet::
> +	This flag makes the command not to report its progress
> +	on the standard error stream.

Do we need all four of these?

Just saying "--no-progress" would do what you want right now. I could
understand the desire for a general "--quiet" flag that implies
"--no-progress", and shuts off any other non-progress chatter as well.
There isn't any now, but it could be a future proofing thing (plus
having a "-q" option is standard). But I think we should document it
that way from the outset (though I notice you probably just lifted this
from pack-objects, IMHO it should be more clear, too).

The "all-progress" thing doesn't seem useful at this level. pack-objects
needs it so that it can do the right thing when being driven by
upload-pack versus send-pack. But for a bundle, we're always writing to
a file. We'd always want "all-progress" (and that's what the current
code does).

Likewise, "all-progress-implied" is about setting the "all-progress" bit
but still letting pack-objects decide whether to show progress based on
isatty(2). I don't think we'd need that here at all (we check isatty
ourselves, and we'd always want all-progress).

So could we perhaps simplify this to:

  1. Set show_progress to isatty(2).

  2. Make --progress a parseopt bool, setting show_progress to 1 (or if
     we see "--no-progress").

  3. Pass "--no-progress" or "--all-progress" to pack-objects, based on
     show_progress.

  4. (Optional) Make "--quiet" a synonym for "--no-progress", with the
     documentation that it may later encompass other messages.

-Peff
Robin H. Johnson Nov. 11, 2019, 7:28 a.m. UTC | #2
On Sun, Nov 10, 2019 at 11:07:50PM -0500, Jeff King wrote:
> On Sun, Nov 10, 2019 at 12:41:25PM -0800, Robin H. Johnson wrote:
> 
> > Support the progress output options from pack-objects in git-bundle's
> > create subcommand. Most notably, this provides --quiet as requested on
> > the git mailing list per [1]
> > 
> > Reference: https://www.mail-archive.com/git@vger.kernel.org/msg182844.html <robbat2-20190806T191156-796782357Z@orbis-terrarum.net>
> 
> I'm glad you included the message-id here, since "182844" is useless if
> mail-archive.com ever goes away. We usually just cite public-inbox for
> that reason, since its URLs just use the message-id anyway:
> 
>   https://public-inbox.org/git/robbat2-20190806T191156-796782357Z@orbis-terrarum.net
> 
> > +--progress::
> > +	Progress status is reported on the standard error stream
> > +	by default when it is attached to a terminal, unless -q
> > +	is specified. This flag forces progress status even if
> > +	the standard error stream is not directed to a terminal.
> > +
> > +--all-progress::
> > +	When --stdout is specified then progress report is
> > +	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
> > +	status of its own as it processes incoming pack data.
> > +	This flag is like --progress except that it forces progress
> > +	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::
> > +--quiet::
> > +	This flag makes the command not to report its progress
> > +	on the standard error stream.
> 
> Do we need all four of these?
I copied the exact set of messages from git-pack-objects, and I do think
the same set makes sense specifically to mirror pack-objects for the
moment.

stderr is a tty:
A/(no options) - shorter output
B/--quiet = no output
C/--progress - shorter output
D/--all-progress - longer output
E/--all-progress-implied - longer output

stderr is not a tty:
A/(no options) - no output
B/--quiet = no output
C/--progress - shorter output
D/--all-progress - longer output
E/--all-progress-implied - no output

Mapping this to a table for a moment:
  1 2
A s n
B n n
C s s
D l l
E l n

1 = stderr is a tty
2 = stderr is not a tty

s = short output
l = long output (includes "Delta compression...", "Writing objects: ..")
n = no output

I think there is a lot of room to improve the behavior here, but at the
risk of breaking backwards compatibility on the existing options, I
think this older set of options should consistent between this and
pack-objects.

--pack-progress-output=[never|short|long] 
--pack-progress-conditional-on-stderr-tty
(horrible names, but I wanted to convey the intent)

> Just saying "--no-progress" would do what you want right now. I could
> understand the desire for a general "--quiet" flag that implies
> "--no-progress", and shuts off any other non-progress chatter as well.
> There isn't any now, but it could be a future proofing thing (plus
> having a "-q" option is standard). But I think we should document it
> that way from the outset (though I notice you probably just lifted this
> from pack-objects, IMHO it should be more clear, too).
Willing to do later series to add --no-progress to this &
pack-objects as consistency improvement if you'd like for future
proofing (specifically --quiet would be all output whereas --no-progress
would only cut out progress output).
Jeff King Nov. 11, 2019, 8:10 a.m. UTC | #3
On Mon, Nov 11, 2019 at 07:28:55AM +0000, Robin H. Johnson wrote:

> > Do we need all four of these?
> I copied the exact set of messages from git-pack-objects, and I do think
> the same set makes sense specifically to mirror pack-objects for the
> moment.

I'm not sure I agree. In what situation would anybody use "git bundle
create --all-progress-implied", for example? Literally no other Git
command except pack-objects has "--all-progress" or
"--all-progress-implied" (even ones which call pack-objects under the
hood to print the progress!), and the presence of the latter in
pack-objects is due to a backwards-compatibility thing in the early days
(where --all-progress did too many things, but we could no longer change
it). I think it would be a mistake to spread it further.

> I think there is a lot of room to improve the behavior here, but at the
> risk of breaking backwards compatibility on the existing options, I
> think this older set of options should consistent between this and
> pack-objects.

But now is the moment where we can do what we want without breaking
compatibility (since there aren't any progress options for git-bundle at
all yet).

I guess another way of thinking about it: why is "pack-objects" the
model for how its progress options should work, and not "send-pack"?
git-bundle is much closer to the latter in how users will invoke it.

> > Just saying "--no-progress" would do what you want right now. I could
> > understand the desire for a general "--quiet" flag that implies
> > "--no-progress", and shuts off any other non-progress chatter as well.
> > There isn't any now, but it could be a future proofing thing (plus
> > having a "-q" option is standard). But I think we should document it
> > that way from the outset (though I notice you probably just lifted this
> > from pack-objects, IMHO it should be more clear, too).
> Willing to do later series to add --no-progress to this &

You already added --no-progress (and it's already there in
pack-objects). It comes for free with OPT_SET_INT("progress").

-Peff
Junio C Hamano Nov. 11, 2019, 9:01 a.m. UTC | #4
Jeff King <peff@peff.net> writes:

> On Mon, Nov 11, 2019 at 07:28:55AM +0000, Robin H. Johnson wrote:
>
>> > Do we need all four of these?
>> I copied the exact set of messages from git-pack-objects, and I do think
>> the same set makes sense specifically to mirror pack-objects for the
>> moment.
>
> I'm not sure I agree. In what situation would anybody use "git bundle
> create --all-progress-implied", for example? Literally no other Git
> command except pack-objects has "--all-progress" or
> "--all-progress-implied" (even ones which call pack-objects under the
> hood to print the progress!), and the presence of the latter in
> pack-objects is due to a backwards-compatibility thing in the early days
> (where --all-progress did too many things, but we could no longer change
> it). I think it would be a mistake to spread it further.

I am quite cure I agree with your reasoning that we would want to
limit the "--all-progress-implied" craziness from spreading ;-)

Thanks.

Patch
diff mbox series

diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
index 7d6c9dcd17..96bb94df7b 100644
--- a/Documentation/git-bundle.txt
+++ b/Documentation/git-bundle.txt
@@ -9,7 +9,7 @@  git-bundle - Move objects and refs by archive
 SYNOPSIS
 --------
 [verse]
-'git bundle' create <file> <git-rev-list-args>
+'git bundle' create [-q | --quiet | --progress | --all-progress] [--all-progress-implied] <file> <git-rev-list-args>
 'git bundle' verify <file>
 'git bundle' list-heads <file> [<refname>...]
 'git bundle' unbundle <file> [<refname>...]
@@ -33,9 +33,11 @@  destination repository.
 OPTIONS
 -------
 
-create <file>::
+create [options] <file> <git-rev-list-args>::
 	Used to create a bundle named 'file'.  This requires the
 	'git-rev-list-args' arguments to define the bundle contents.
+	'options' contains the options specific to the 'git bundle create'
+	subcommand.
 
 verify <file>::
 	Used to check that a bundle file is valid and will apply
@@ -75,6 +77,33 @@  unbundle <file>::
 	necessarily everything in the pack (in this case, 'git bundle' acts
 	like 'git fetch-pack').
 
+--progress::
+	Progress status is reported on the standard error stream
+	by default when it is attached to a terminal, unless -q
+	is specified. This flag forces progress status even if
+	the standard error stream is not directed to a terminal.
+
+--all-progress::
+	When --stdout is specified then progress report is
+	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
+	status of its own as it processes incoming pack data.
+	This flag is like --progress except that it forces progress
+	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::
+--quiet::
+	This flag makes the command not to report its progress
+	on the standard error stream.
+
 SPECIFYING REFERENCES
 ---------------------
 
diff --git a/builtin/bundle.c b/builtin/bundle.c
index 09b989cfc0..39b3e88d40 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -1,4 +1,5 @@ 
 #include "builtin.h"
+#include "argv-array.h"
 #include "parse-options.h"
 #include "cache.h"
 #include "bundle.h"
@@ -11,7 +12,7 @@ 
  */
 
 static const char * const builtin_bundle_usage[] = {
-  N_("git bundle create <file> <git-rev-list args>"),
+  N_("git bundle create [<options>] <file> <git-rev-list args>"),
   N_("git bundle verify <file>"),
   N_("git bundle list-heads <file> [<refname>...]"),
   N_("git bundle unbundle <file> [<refname>...]"),
@@ -19,7 +20,7 @@  static const char * const builtin_bundle_usage[] = {
 };
 
 static const char * const builtin_bundle_create_usage[] = {
-  N_("git bundle create <file> <git-rev-list args>"),
+  N_("git bundle create [<options>] <file> <git-rev-list args>"),
   NULL
 };
 
@@ -56,7 +57,20 @@  static int parse_options_cmd_bundle(int argc,
 }
 
 static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
+	int all_progress_implied = 0;
+	int progress = isatty(STDERR_FILENO);
+	struct argv_array pack_opts;
+
 	struct option options[] = {
+		OPT_SET_INT('q', "quiet", &progress,
+			    N_("do not show progress meter"), 0),
+		OPT_SET_INT(0, "progress", &progress,
+			    N_("show progress meter"), 1),
+		OPT_SET_INT(0, "all-progress", &progress,
+			    N_("show progress meter during object writing phase"), 2),
+		OPT_BOOL(0, "all-progress-implied",
+			 &all_progress_implied,
+			 N_("similar to --all-progress when progress meter is shown")),
 		OPT_END()
 	};
 	const char* bundle_file;
@@ -65,9 +79,19 @@  static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 			builtin_bundle_create_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
+	argv_array_init(&pack_opts);
+	if (progress == 0)
+		argv_array_push(&pack_opts, "--quiet");
+	else if (progress == 1)
+		argv_array_push(&pack_opts, "--progress");
+	else if (progress == 2)
+		argv_array_push(&pack_opts, "--all-progress");
+	if (progress && all_progress_implied)
+		argv_array_push(&pack_opts, "--all-progress-implied");
+
 	if (!startup_info->have_repository)
 		die(_("Need a repository to create a bundle."));
-	return !!create_bundle(the_repository, bundle_file, argc, argv);
+	return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts);
 }
 
 static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
diff --git a/bundle.c b/bundle.c
index a85ed3f7bc..99439e07a1 100644
--- a/bundle.c
+++ b/bundle.c
@@ -249,15 +249,16 @@  static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
 
 
 /* Write the pack data to bundle_fd */
-static int write_pack_data(int bundle_fd, struct rev_info *revs)
+static int write_pack_data(int bundle_fd, struct rev_info *revs, struct argv_array *pack_options)
 {
 	struct child_process pack_objects = CHILD_PROCESS_INIT;
 	int i;
 
 	argv_array_pushl(&pack_objects.args,
-			 "pack-objects", "--all-progress-implied",
+			 "pack-objects",
 			 "--stdout", "--thin", "--delta-base-offset",
 			 NULL);
+	argv_array_pushv(&pack_objects.args, pack_options->argv);
 	pack_objects.in = -1;
 	pack_objects.out = bundle_fd;
 	pack_objects.git_cmd = 1;
@@ -428,7 +429,7 @@  static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
 }
 
 int create_bundle(struct repository *r, const char *path,
-		  int argc, const char **argv)
+		  int argc, const char **argv, struct argv_array *pack_options)
 {
 	struct lock_file lock = LOCK_INIT;
 	int bundle_fd = -1;
@@ -470,7 +471,7 @@  int create_bundle(struct repository *r, const char *path,
 		goto err;
 
 	/* write pack */
-	if (write_pack_data(bundle_fd, &revs))
+	if (write_pack_data(bundle_fd, &revs, pack_options))
 		goto err;
 
 	if (!bundle_to_stdout) {
diff --git a/bundle.h b/bundle.h
index 37c37d7f65..ceab0c7475 100644
--- a/bundle.h
+++ b/bundle.h
@@ -1,6 +1,7 @@ 
 #ifndef BUNDLE_H
 #define BUNDLE_H
 
+#include "argv-array.h"
 #include "cache.h"
 
 struct ref_list {
@@ -19,7 +20,7 @@  struct bundle_header {
 int is_bundle(const char *path, int quiet);
 int read_bundle_header(const char *path, struct bundle_header *header);
 int create_bundle(struct repository *r, const char *path,
-		  int argc, const char **argv);
+		  int argc, const char **argv, struct argv_array *pack_options);
 int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
 #define BUNDLE_VERBOSE 1
 int unbundle(struct repository *r, struct bundle_header *header,