| Message ID | 20191110204126.30553-2-robbat2@gentoo.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] bundle: framework for options before bundle file | expand |
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 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).
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
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.
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,
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(-)