git-bundle --quiet support #435

robbat2 · 2019-10-29T19:08:29Z

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

gitgitgadget · 2019-10-29T19:09:27Z

Welcome to GitGitGadget

Hi @robbat2, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that this Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

the lines should not exceed 76 columns,
the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and
the commit messages' body should be describing the "why?" of the change.
Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the FreeNode IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions.

If you want to see what email(s) would be sent for a submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via:

curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

dscho · 2019-10-29T19:20:27Z

/allow

gitgitgadget · 2019-10-29T19:21:31Z

User robbat2 is now allowed to use GitGitGadget.

dscho

Looks pretty good already!

builtin/bundle.c

robbat2 · 2019-10-29T22:45:41Z

@dscho This shows a failure at https://github.com/gitgitgadget/git/pull/435/checks?check_run_id=280279153 that I cannot reproduce anywhere else. Got any suggestions?

dscho · 2019-10-30T13:24:56Z

/azp run

azure-pipelines · 2019-10-30T13:25:07Z

Azure Pipelines successfully started running 2 pipeline(s).

dscho · 2019-10-30T13:28:32Z

@robbat2 unfortunately, t5516 is flaky. Part of it will be fixed via #428, another part (which triggered here) will hopefully be fixed soon, see e.g. https://public-inbox.org/git/20190830121005.GI8571@szeder.dev/

robbat2 · 2019-11-06T19:17:05Z

/submit

gitgitgadget · 2019-11-06T19:18:18Z

Submitted as pull.435.git.1573067879.gitgitgadget@gmail.com

robbat2 · 2019-11-08T20:32:26Z

/submit

gitgitgadget · 2019-11-08T20:33:20Z

Error: 0131780 was already submitted

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>

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>

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>

robbat2 · 2019-11-08T21:29:44Z

/submit

gitgitgadget · 2019-11-08T21:30:59Z

Submitted as pull.435.v2.git.1573248640.gitgitgadget@gmail.com

dscho · 2019-11-09T10:40:12Z

Hrm. It looks as if something is universally unliked in this PR... The log does not shed any light into it... It is strange that already the cover letter does not make it to the mailing list...

Maybe it is the PR description that essentially consists of a hyperlink? Maybe vger labels this as likely spam?

gitgitgadget · 2019-11-10T20:46:54Z

builtin/bundle.c

@@ -1,4 +1,5 @@
 #include "builtin.h"


On the Git mailing list, "Robin H. Johnson" wrote (reply to this):

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

gitgitgadget · 2019-11-10T20:46:56Z

builtin/bundle.c

@@ -1,4 +1,5 @@
 #include "builtin.h"


On the Git mailing list, "Robin H. Johnson" wrote (reply to this):

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

gitgitgadget · 2019-11-10T20:46:57Z

builtin/bundle.c

@@ -1,4 +1,5 @@
 #include "builtin.h"


On the Git mailing list, "Robin H. Johnson" wrote (reply to this):

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

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

On the Git mailing list, Jeff King wrote (reply to this):

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

On the Git mailing list, Jeff King wrote (reply to this):

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

On the Git mailing list, Jeff King wrote (reply to this):

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

On the Git mailing list, Jeff King wrote (reply to this):

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

On the Git mailing list, Johannes Schindelin wrote (reply to this):

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?): =2D- 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=3DTLS1_2 cipher=3DECDHE-RSA-AES128-GCM-SHA256 bits=3D128/= 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=3DUTF-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 veri= fy 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%2Fro= bbat2%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 =3D 1: 1f7f0aa1e8 bundle: framework for options before bu= ndle file 2: 5678de06f3 ! 2: 468922581b bundle-create: progress output control @@ -6,9 +6,7 @@ create subcommand. Most notably, this provides --quiet as reques= ted on the git mailing list per [1] - [1] <robbat2-20190806T191156-796782357Z@orbis-terrarum.net> - - Reference: https://www.mail-archive.com/git@vger.kernel.org/msg1= 82844.html + Reference: https://www.mail-archive.com/git@vger.kernel.org/msg1= 82844.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-bundl= e.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/msg1= 82844.html + Reference: https://www.mail-archive.com/git@vger.kernel.org/msg1= 82844.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-bundl= e.txt =2D- gitgitgadget

On the Git mailing list, Junio C Hamano wrote (reply to this):

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?

On the Git mailing list, Junio C Hamano wrote (reply to this):

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.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

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 th= e > > mbox of the cover letter reads like this (maybe anybody else has a clu= e > > 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

gitgitgadget · 2019-11-11T04:51:36Z

This patch series was integrated into pu via git@97ae8c8.

gitgitgadget · 2019-11-11T11:49:52Z

This branch is now known as rj/bundle-ui-updates.

gitgitgadget · 2019-11-12T08:45:17Z

This patch series was integrated into pu via git@7709260.

We correctly enclose "<author> via GitGitGadget" in double quotes, but if <author> already contained double quotes (e.g. because it contains dots), we failed to strip/escape them. This prevented gitgitgadget/git#435 from being sent correctly (it was sent all right, but the Git mailing list dropped the mails unceremoniously). Let's handle this correctly. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho · 2019-11-12T15:26:06Z

@robbat2 I think Junio identified the culprit correctly, and I just opened a PR that will hopefully prevent this problem in the future. Again, very sorry for this!

gitgitgadget · 2019-11-19T07:12:46Z

This patch series was integrated into pu via git@2dcaccb.

gitgitgadget · 2019-11-19T07:12:48Z

This patch series was integrated into next via git@d88f626.

gitgitgadget · 2019-11-20T05:51:17Z

This patch series was integrated into pu via git@4fd5707.

gitgitgadget · 2019-11-21T07:42:03Z

This patch series was integrated into pu via git@317c36a.

gitgitgadget · 2019-11-22T08:36:27Z

This patch series was integrated into pu via git@f6db88d.

gitgitgadget · 2019-11-25T08:55:31Z

This patch series was integrated into pu via git@2ed36d1.

gitgitgadget · 2019-11-27T10:13:04Z

This patch series was integrated into pu via git@604ce00.

gitgitgadget · 2019-12-02T16:16:46Z

This patch series was integrated into pu via git@ca5c8aa.

gitgitgadget · 2019-12-02T16:16:47Z

This patch series was integrated into next via git@ca5c8aa.

gitgitgadget · 2019-12-02T16:16:48Z

This patch series was integrated into master via git@ca5c8aa.

gitgitgadget · 2019-12-02T16:16:51Z

Closed via ca5c8aa.

robbat2 force-pushed the silent-bundle branch from c3a15c2 to 3977ea7 Compare October 29, 2019 19:15

robbat2 force-pushed the silent-bundle branch from 3977ea7 to 096da9a Compare October 29, 2019 19:26

dscho approved these changes Oct 29, 2019

View reviewed changes

builtin/bundle.c Outdated Show resolved Hide resolved

builtin/bundle.c Show resolved Hide resolved

builtin/bundle.c Outdated Show resolved Hide resolved

robbat2 force-pushed the silent-bundle branch from 096da9a to 0131780 Compare October 29, 2019 20:44

robbat2 added 3 commits November 8, 2019 12:52

bundle-verify: add --quiet

26c7b28

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>

robbat2 force-pushed the silent-bundle branch from 0131780 to 26c7b28 Compare November 8, 2019 20:53

gitgitgadget bot reviewed Nov 10, 2019

View reviewed changes

gitgitgadget bot added the pu label Nov 11, 2019

dscho mentioned this pull request Nov 12, 2019

Fix the "double double quotes" problem in the "<author> via GitGitGadget" From: lines gitgitgadget/gitgitgadget#142

Merged

gitgitgadget bot added the next label Nov 19, 2019

gitgitgadget bot added the master label Dec 2, 2019

gitgitgadget bot closed this Dec 2, 2019

git-bundle --quiet support #435

git-bundle --quiet support #435

Conversation

robbat2 commented Oct 29, 2019

gitgitgadget bot commented Oct 29, 2019

Welcome to GitGitGadget

Contributing the patches

dscho commented Oct 29, 2019

gitgitgadget bot commented Oct 29, 2019

dscho left a comment

Choose a reason for hiding this comment

robbat2 commented Oct 29, 2019

dscho commented Oct 30, 2019

azure-pipelines bot commented Oct 30, 2019

dscho commented Oct 30, 2019

robbat2 commented Nov 6, 2019

gitgitgadget bot commented Nov 6, 2019

robbat2 commented Nov 8, 2019

gitgitgadget bot commented Nov 8, 2019

robbat2 commented Nov 8, 2019

gitgitgadget bot commented Nov 8, 2019

dscho commented Nov 9, 2019

gitgitgadget bot Nov 10, 2019

Choose a reason for hiding this comment

gitgitgadget bot Nov 10, 2019

Choose a reason for hiding this comment

gitgitgadget bot Nov 10, 2019

Choose a reason for hiding this comment

gitgitgadget bot Nov 11, 2019

Choose a reason for hiding this comment

gitgitgadget bot Nov 11, 2019

Choose a reason for hiding this comment

gitgitgadget bot Nov 11, 2019

Choose a reason for hiding this comment

gitgitgadget bot Nov 11, 2019

Choose a reason for hiding this comment

gitgitgadget bot Nov 11, 2019

Choose a reason for hiding this comment

gitgitgadget bot Nov 11, 2019

Choose a reason for hiding this comment

gitgitgadget bot Nov 11, 2019

Choose a reason for hiding this comment

gitgitgadget bot Nov 11, 2019

Choose a reason for hiding this comment

gitgitgadget bot Nov 12, 2019

Choose a reason for hiding this comment

gitgitgadget bot commented Nov 11, 2019

gitgitgadget bot commented Nov 11, 2019

gitgitgadget bot commented Nov 12, 2019

dscho commented Nov 12, 2019

gitgitgadget bot commented Nov 19, 2019

gitgitgadget bot commented Nov 19, 2019

gitgitgadget bot commented Nov 20, 2019

gitgitgadget bot commented Nov 21, 2019

gitgitgadget bot commented Nov 22, 2019

gitgitgadget bot commented Nov 25, 2019

gitgitgadget bot commented Nov 27, 2019

gitgitgadget bot commented Dec 2, 2019

gitgitgadget bot commented Dec 2, 2019

gitgitgadget bot commented Dec 2, 2019

gitgitgadget bot commented Dec 2, 2019