[PATCH v3 1/3] bundle: framework for options before bundle file

[PATCH v3 1/3] bundle: framework for options before bundle file

[Robin H. Johnson]

Make it possible for any of the git-bundle subcommands to include
options:
- before the sub-command
- after the sub-command, before the bundle filename

There is an immediate gain in support for help with all of the
sub-commands, where 'git bundle list-heads -h' previously returned an
error.

Downside here is an increase in code duplication that cannot be
trivially avoided short of shared global static options.

Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
---
 builtin/bundle.c | 190 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 145 insertions(+), 45 deletions(-)

I tried doing this via GitGitGadget initially as a test of that process,
as well as the CI integration side; however as noted in #git-devel and
elsewhere on the list, vger seems to swallow the mail to /dev/null

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 1ea4bfdfc1..09b989cfc0 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -1,4 +1,5 @@
 #include "builtin.h"
+#include "parse-options.h"
 #include "cache.h"
 #include "bundle.h"
 
@@ -9,59 +10,158 @@
  * bundle supporting "fetch", "pull", and "ls-remote".
  */
 
-static const char builtin_bundle_usage[] =
-  "git bundle create <file> <git-rev-list args>\n"
-  "   or: git bundle verify <file>\n"
-  "   or: git bundle list-heads <file> [<refname>...]\n"
-  "   or: git bundle unbundle <file> [<refname>...]";
+static const char * const builtin_bundle_usage[] = {
+  N_("git bundle create <file> <git-rev-list args>"),
+  N_("git bundle verify <file>"),
+  N_("git bundle list-heads <file> [<refname>...]"),
+  N_("git bundle unbundle <file> [<refname>...]"),
+  NULL
+};
 
-int cmd_bundle(int argc, const char **argv, const char *prefix)
-{
+static const char * const builtin_bundle_create_usage[] = {
+  N_("git bundle create <file> <git-rev-list args>"),
+  NULL
+};
+
+static const char * const builtin_bundle_verify_usage[] = {
+  N_("git bundle verify <file>"),
+  NULL
+};
+
+static const char * const builtin_bundle_list_heads_usage[] = {
+  N_("git bundle list-heads <file> [<refname>...]"),
+  NULL
+};
+
+static const char * const builtin_bundle_unbundle_usage[] = {
+  N_("git bundle unbundle <file> [<refname>...]"),
+  NULL
+};
+
+static int verbose;
+
+static int parse_options_cmd_bundle(int argc,
+		const char **argv,
+		const char* prefix,
+		const char * const usagestr[],
+		const struct option options[],
+		const char **bundle_file) {
+	int newargc;
+	newargc = parse_options(argc, argv, NULL, options, usagestr,
+			     PARSE_OPT_STOP_AT_NON_OPTION);
+	if (argc < 1)
+		usage_with_options(usagestr, options);
+	*bundle_file = prefix_filename(prefix, argv[0]);
+	return newargc;
+}
+
+static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
+	struct option options[] = {
+		OPT_END()
+	};
+	const char* bundle_file;
+
+	argc = parse_options_cmd_bundle(argc, argv, prefix,
+			builtin_bundle_create_usage, options, &bundle_file);
+	/* bundle internals use argv[1] as further parameters */
+
+	if (!startup_info->have_repository)
+		die(_("Need a repository to create a bundle."));
+	return !!create_bundle(the_repository, bundle_file, argc, argv);
+}
+
+static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
-	const char *cmd, *bundle_file;
 	int bundle_fd = -1;
 
-	if (argc < 3)
-		usage(builtin_bundle_usage);
+	struct option options[] = {
+		OPT_END()
+	};
+	const char* bundle_file;
 
-	cmd = argv[1];
-	bundle_file = prefix_filename(prefix, argv[2]);
-	argc -= 2;
-	argv += 2;
+	argc = parse_options_cmd_bundle(argc, argv, prefix,
+			builtin_bundle_verify_usage, options, &bundle_file);
+	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if (strcmp(cmd, "create") && (bundle_fd =
-				read_bundle_header(bundle_file, &header)) < 0)
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
 		return 1;
+	close(bundle_fd);
+	if (verify_bundle(the_repository, &header, 1))
+		return 1;
+	fprintf(stderr, _("%s is okay\n"), bundle_file);
+	return 0;
+}
 
-	if (!strcmp(cmd, "verify")) {
-		close(bundle_fd);
-		if (argc != 1) {
-			usage(builtin_bundle_usage);
-			return 1;
-		}
-		if (verify_bundle(the_repository, &header, 1))
-			return 1;
-		fprintf(stderr, _("%s is okay\n"), bundle_file);
-		return 0;
-	}
-	if (!strcmp(cmd, "list-heads")) {
-		close(bundle_fd);
-		return !!list_bundle_refs(&header, argc, argv);
+static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {
+	struct bundle_header header;
+	int bundle_fd = -1;
+
+	struct option options[] = {
+		OPT_END()
+	};
+	const char* bundle_file;
+
+	argc = parse_options_cmd_bundle(argc, argv, prefix,
+			builtin_bundle_list_heads_usage, options, &bundle_file);
+	/* bundle internals use argv[1] as further parameters */
+
+	memset(&header, 0, sizeof(header));
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
+		return 1;
+	close(bundle_fd);
+	return !!list_bundle_refs(&header, argc, argv);
+}
+
+static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {
+	struct bundle_header header;
+	int bundle_fd = -1;
+
+	struct option options[] = {
+		OPT_END()
+	};
+	const char* bundle_file;
+
+	argc = parse_options_cmd_bundle(argc, argv, prefix,
+			builtin_bundle_unbundle_usage, options, &bundle_file);
+	/* bundle internals use argv[1] as further parameters */
+
+	memset(&header, 0, sizeof(header));
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
+		return 1;
+	if (!startup_info->have_repository)
+		die(_("Need a repository to unbundle."));
+	return !!unbundle(the_repository, &header, bundle_fd, 0) ||
+		list_bundle_refs(&header, argc, argv);
+}
+
+int cmd_bundle(int argc, const char **argv, const char *prefix)
+{
+	struct option options[] = {
+		OPT__VERBOSE(&verbose, N_("be verbose; must be placed before a subcommand")),
+		OPT_END()
+	};
+	int result;
+
+	argc = parse_options(argc, argv, prefix, options, builtin_bundle_usage,
+		PARSE_OPT_STOP_AT_NON_OPTION);
+
+	packet_trace_identity("bundle");
+
+	if (argc < 2)
+		usage_with_options(builtin_bundle_usage, options);
+
+	else if (!strcmp(argv[0], "create"))
+		result = cmd_bundle_create(argc, argv, prefix);
+	else if (!strcmp(argv[0], "verify"))
+		result = cmd_bundle_verify(argc, argv, prefix);
+	else if (!strcmp(argv[0], "list-heads"))
+		result = cmd_bundle_list_heads(argc, argv, prefix);
+	else if (!strcmp(argv[0], "unbundle"))
+		result = cmd_bundle_unbundle(argc, argv, prefix);
+	else {
+		error(_("Unknown subcommand: %s"), argv[0]);
+		usage_with_options(builtin_bundle_usage, options);
 	}
-	if (!strcmp(cmd, "create")) {
-		if (argc < 2) {
-			usage(builtin_bundle_usage);
-			return 1;
-		}
-		if (!startup_info->have_repository)
-			die(_("Need a repository to create a bundle."));
-		return !!create_bundle(the_repository, bundle_file, argc, argv);
-	} else if (!strcmp(cmd, "unbundle")) {
-		if (!startup_info->have_repository)
-			die(_("Need a repository to unbundle."));
-		return !!unbundle(the_repository, &header, bundle_fd, 0) ||
-			list_bundle_refs(&header, argc, argv);
-	} else
-		usage(builtin_bundle_usage);
+	return result ? 1 : 0;
 }
-- 
2.23.0

[PATCH v3 2/3] bundle-create: progress output control

[Robin H. Johnson]

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(-)

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,
-- 
2.23.0

[PATCH v3 3/3] bundle-verify: add --quiet

[Robin H. Johnson]

Add --quiet to git-bundle verify as proposed on the mailing list [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 | 2 +-
 builtin/bundle.c             | 9 ++++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
index 96bb94df7b..ccada80a4a 100644
--- a/Documentation/git-bundle.txt
+++ b/Documentation/git-bundle.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git bundle' create [-q | --quiet | --progress | --all-progress] [--all-progress-implied] <file> <git-rev-list-args>
-'git bundle' verify <file>
+'git bundle' verify [-q | --quiet] <file>
 'git bundle' list-heads <file> [<refname>...]
 'git bundle' unbundle <file> [<refname>...]
 
diff --git a/builtin/bundle.c b/builtin/bundle.c
index 39b3e88d40..f049d27a14 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -13,7 +13,7 @@
 
 static const char * const builtin_bundle_usage[] = {
   N_("git bundle create [<options>] <file> <git-rev-list args>"),
-  N_("git bundle verify <file>"),
+  N_("git bundle verify [<options>] <file>"),
   N_("git bundle list-heads <file> [<refname>...]"),
   N_("git bundle unbundle <file> [<refname>...]"),
   NULL
@@ -25,7 +25,7 @@ static const char * const builtin_bundle_create_usage[] = {
 };
 
 static const char * const builtin_bundle_verify_usage[] = {
-  N_("git bundle verify <file>"),
+  N_("git bundle verify [<options>] <file>"),
   NULL
 };
 
@@ -97,8 +97,11 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
+	int quiet = 0;
 
 	struct option options[] = {
+		OPT_BOOL('q', "quiet", &quiet,
+			    N_("do not show bundle details")),
 		OPT_END()
 	};
 	const char* bundle_file;
@@ -111,7 +114,7 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
 		return 1;
 	close(bundle_fd);
-	if (verify_bundle(the_repository, &header, 1))
+	if (verify_bundle(the_repository, &header, !quiet))
 		return 1;
 	fprintf(stderr, _("%s is okay\n"), bundle_file);
 	return 0;
-- 
2.23.0

Re: [PATCH v3 1/3] bundle: framework for options before bundle file

[Junio C Hamano]

"Robin H. Johnson" <robbat2@gentoo.org> writes:

> +static int verbose;
> +
> +static int parse_options_cmd_bundle(int argc,
> +		const char **argv,
> +		const char* prefix,
> +		const char * const usagestr[],
> +		const struct option options[],
> +		const char **bundle_file) {
> +	int newargc;
> +	newargc = parse_options(argc, argv, NULL, options, usagestr,
> +			     PARSE_OPT_STOP_AT_NON_OPTION);
> +	if (argc < 1)
> +		usage_with_options(usagestr, options);
> +	*bundle_file = prefix_filename(prefix, argv[0]);
> +	return newargc;
> +}

Looks like a useful helper to be shared among subcommands.

> +static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	const char* bundle_file;
> ...
> +int cmd_bundle(int argc, const char **argv, const char *prefix)
> +{
> +	struct option options[] = {
> +		OPT__VERBOSE(&verbose, N_("be verbose; must be placed before a subcommand")),
> +		OPT_END()
> +	};
> +	int result;
> +
> +	argc = parse_options(argc, argv, prefix, options, builtin_bundle_usage,
> +		PARSE_OPT_STOP_AT_NON_OPTION);

Looks like a reasonable arrangement for two-level option parser.

Re: [PATCH v3 1/3] bundle: framework for options before bundle file

[Jeff King]

On Sun, Nov 10, 2019 at 12:41:24PM -0800, Robin H. Johnson wrote:

> Make it possible for any of the git-bundle subcommands to include
> options:
> - before the sub-command
> - after the sub-command, before the bundle filename

On reading your subject and the start of the commit message, I thought
you mean that you meant to allow both:

  git bundle --foo create

and

  git bundle create --foo

But looking at the patch, this is about creating two separate sets of
options, one for all sub-commands, and one for each individual
sub-command. That makes sense to me. I don't know if it's worth trying
to spell that out more explicitly in the commit message.

-Peff

Re: [PATCH v3 2/3] bundle-create: progress output control

[Jeff King]

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

Re: [PATCH v3 3/3] bundle-verify: add --quiet

[Jeff King]

On Sun, Nov 10, 2019 at 12:41:26PM -0800, Robin H. Johnson wrote:

> @@ -97,8 +97,11 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
>  static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
>  	struct bundle_header header;
>  	int bundle_fd = -1;
> +	int quiet = 0;
>  
>  	struct option options[] = {
> +		OPT_BOOL('q', "quiet", &quiet,
> +			    N_("do not show bundle details")),
>  		OPT_END()
>  	};

This --quiet makes much more sense to me (compared to the last patch) as
distinct from "--no-progress", because it is about quieting non-progress
chatter.

There's an OPT__QUIET() macro; should we be using that here?

-Peff

Re: [PATCH v3 2/3] bundle-create: progress output control

[Robin H. Johnson]

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

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).

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1113 bytes --]

Re: [PATCH v3 2/3] bundle-create: progress output control

[Jeff King]

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

Re: [PATCH v3 1/3] bundle: framework for options before bundle file

[Johannes Schindelin]

Hi Robin,

On Sun, 10 Nov 2019, Robin H. Johnson wrote:

> Make it possible for any of the git-bundle subcommands to include
> options:
> - before the sub-command
> - after the sub-command, before the bundle filename
>
> There is an immediate gain in support for help with all of the
> sub-commands, where 'git bundle list-heads -h' previously returned an
> error.
>
> Downside here is an increase in code duplication that cannot be
> trivially avoided short of shared global static options.
>
> Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
> ---
>  builtin/bundle.c | 190 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 145 insertions(+), 45 deletions(-)
>
> I tried doing this via GitGitGadget initially as a test of that process,
> as well as the CI integration side; however as noted in #git-devel and
> elsewhere on the list, vger seems to swallow the mail to /dev/null

I am very sorry for the woes, and I have to admit that I _still_ have no
clue what is going wrong there.

The mail was sent correctly by GMail, at least it reports that, and the
mbox of the cover letter reads like this (maybe anybody else has a clue
why vger thinks it okay to just drop the mail without further notice?):

-- snipsnap --
Return-Path: <gitgitgadget@gmail.com>
Received: from [127.0.0.1] ([13.74.141.28])
        by smtp.gmail.com with ESMTPSA id t1sm9512770wrn.81.2019.11.08.13.30.41
        (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);
        Fri, 08 Nov 2019 13:30:41 -0800 (PST)
Message-Id: <pull.435.v2.git.1573248640.gitgitgadget@gmail.com>
In-Reply-To: <pull.435.git.1573067879.gitgitgadget@gmail.com>
References: <pull.435.git.1573067879.gitgitgadget@gmail.com>
From: ""Robin H. Johnson" via GitGitGadget" <gitgitgadget@gmail.com>
Date: Fri, 08 Nov 2019 21:30:37 +0000
Subject: [PATCH v2 0/3] git-bundle --quiet support
Fcc: Sent
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
To: git@vger.kernel.org
Cc: "Robin H. Johnson" <robbat2@orbis-terrarum.net>,
    Junio C Hamano <gitster@pobox.com>

Implement --quiet support for some git-bundle subcommands: create and verify

Reference: https://www.mail-archive.com/git@vger.kernel.org/msg182844.html
Signed-off-by: Robin H. Johnson robbat2@gentoo.org [robbat2@gentoo.org]

Robin H. Johnson (3):
  bundle: framework for options before bundle file
  bundle-create: progress output control
  bundle-verify: add --quiet

 Documentation/git-bundle.txt |  35 +++++-
 builtin/bundle.c             | 217 +++++++++++++++++++++++++++--------
 bundle.c                     |   9 +-
 bundle.h                     |   3 +-
 4 files changed, 211 insertions(+), 53 deletions(-)


base-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-435%2Frobbat2%2Fsilent-bundle-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-435/robbat2/silent-bundle-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/435

Range-diff vs v1:

 1:  c48ff9adbb = 1:  1f7f0aa1e8 bundle: framework for options before bundle file
 2:  5678de06f3 ! 2:  468922581b bundle-create: progress output control
     @@ -6,9 +6,7 @@
          create subcommand. Most notably, this provides --quiet as requested on
          the git mailing list per [1]

     -    [1] <robbat2-20190806T191156-796782357Z@orbis-terrarum.net>
     -
     -    Reference: https://www.mail-archive.com/git@vger.kernel.org/msg182844.html
     +    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>

       diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
 3:  01317800c5 ! 3:  26c7b28309 bundle-verify: add --quiet
     @@ -2,11 +2,9 @@

          bundle-verify: add --quiet

     -    Add --quiet to git-bundle verify as proposed per [1]
     +    Add --quiet to git-bundle verify as proposed on the mailing list [1].

     -    [1] <robbat2-20190806T191156-796782357Z@orbis-terrarum.net>
     -
     -    Reference: https://www.mail-archive.com/git@vger.kernel.org/msg182844.html
     +    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>

       diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt

--
gitgitgadget

Re: [PATCH v3 1/3] bundle: framework for options before bundle file

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The mail was sent correctly by GMail, at least it reports that, and the
> mbox of the cover letter reads like this (maybe anybody else has a clue
> why vger thinks it okay to just drop the mail without further notice?):

> From: ""Robin H. Johnson" via GitGitGadget" <gitgitgadget@gmail.com>

How does that doubled double quote work?

Re: [PATCH v3 2/3] bundle-create: progress output control

[Junio C Hamano]

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.

Re: [PATCH v3 1/3] bundle: framework for options before bundle file

[Johannes Schindelin]

Hi Junio,

On Mon, 11 Nov 2019, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > The mail was sent correctly by GMail, at least it reports that, and the
> > mbox of the cover letter reads like this (maybe anybody else has a clue
> > why vger thinks it okay to just drop the mail without further notice?):
>
> > From: ""Robin H. Johnson" via GitGitGadget" <gitgitgadget@gmail.com>
>
> How does that doubled double quote work?

Wow. I looked through the mbox three times, and I managed to overlook
this, still. Talk about bias.

Thanks,
Dscho