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

Commit Message

Robin H. Johnson Nov. 10, 2019, 8:41 p.m. UTC

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 <[e-mail]>
---
 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

Comments

Junio C Hamano Nov. 11, 2019, 2:34 a.m. UTC | #1

"Robin H. Johnson" <[e-mail]> 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.

Jeff King Nov. 11, 2019, 3:54 a.m. UTC | #2

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

Johannes Schindelin Nov. 11, 2019, 8:46 a.m. UTC | #3

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 <[e-mail]>
> ---
>  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: <[e-mail]>
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: <[e-mail]>
In-Reply-To: <[e-mail]>
References: <[e-mail]>
From: ""Robin H. Johnson" via GitGitGadget" <[e-mail]>
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: [e-mail]
Cc: "Robin H. Johnson" <[e-mail]>,
    Junio C Hamano <[e-mail]>

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

Reference: https://www.mail-archive.com/[e-mail]/msg182844.html
Signed-off-by: Robin H. Johnson [e-mail] [[e-mail]]

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] <[e-mail]>
     -
     -    Reference: https://www.mail-archive.com/[e-mail]/msg182844.html
     +    Reference: https://www.mail-archive.com/[e-mail]/msg182844.html <[e-mail]>
          Signed-off-by: Robin H. Johnson <[e-mail]>

       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] <[e-mail]>
     -
     -    Reference: https://www.mail-archive.com/[e-mail]/msg182844.html
     +    Reference: https://www.mail-archive.com/[e-mail]/msg182844.html <[e-mail]>
          Signed-off-by: Robin H. Johnson <[e-mail]>

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

--
gitgitgadget

Junio C Hamano Nov. 11, 2019, 9 a.m. UTC | #4

Johannes Schindelin <[e-mail]> 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" <[e-mail]>

How does that doubled double quote work?

Johannes Schindelin Nov. 12, 2019, 3:09 p.m. UTC | #5

Hi Junio,

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

> Johannes Schindelin <[e-mail]> 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" <[e-mail]>
>
> 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

Patch

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;
 }