bundle: turn on --all-progress-implied by default

In 79862b6b77c (bundle-create: progress output control, 2019-11-10),
"bundle create" learned about the --all-progress and
--all-progress-implied options, which were copied from pack-objects.
I think these were a mistake.

In pack-objects, "all-progress-implied" is about switching the behavior
between a regular on-disk "git repack" and the use of pack-objects for
push/fetch (where a fetch does not want progress from the server during
the write stage; the client will print progress as it receives the
data). But there's no such distinction for bundles. Prior to
79862b6b77c, we always printed the write stage. Afterwards, a vanilla:

  git bundle create foo.bundle

omits the write progress, appearing to hang (especially if your
repository is large or your disk is slow). That seems like a regression.

It's possible that the flexibility to disable the write-phase progress
_could_ be useful for bundle. E.g., if you did something like:

  ssh some-host git bundle create foo.bundle |
  git bundle unbundle

But if you are running both in real-time, why are you using bundles in
the first place? You're better off doing a real fetch.

But even if we did want to support that, it should be the exception, and
vanilla "bundle create" should display the full progress. So we'd want
to name the option "--no-write-progress" or something.

The "--all-progress" option itself is even worse. It exists in
pack-objects only for historical reasons. It's a mistake because it
implies "--progress", and we added "--all-progress-implied" to fix that.
There is no reason to propagate that mistake to new commands.

Likewise, the documentation for these options was pulled from
pack-objects. But it doesn't make any sense in this context. It talks
about "--stdout", but that is not even an option that git-bundle
supports.

This patch flips the default for "--all-progress-implied" back to
"true", fixing the regression in 79862b6b77c. This turns that option
into a noop, and means that "--all-progress" is really the same as
"--progress". We _could_ drop them completely, but since they've been
shipped with Git since v2.25.0, it's polite to continue accepting them.

I didn't implement any sort of "--no-write-progress" here. I'm not at
all convinced it's necessary, and the discussion from the original
thread:

  https://lore.kernel.org/git/20191110204126.30553-2-robbat2@gentoo.org/

shows that that the main focus was on getting --progress and --quiet
support, and not any kind of clever "real-time bundle over the network"
feature. But technically this patch is making it impossible to do
something that you _could_ do post-79862b6b77c.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
index 18a022b4b40c345040c23e83bed1fd1983bd725e..a4227b6561d2f98f00b5104a80c4ff62b89a5883 100644 (file)
--- 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 [-q | --quiet | --progress | --all-progress] [--all-progress-implied]
+'git bundle' create [-q | --quiet | --progress]
                    [--version=<version>] <file> <git-rev-list-args>
 'git bundle' verify [-q | --quiet] <file>
 'git bundle' list-heads <file> [<refname>...]
@@ -115,22 +115,6 @@ unbundle <file>::
        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.
-
 --version=<version>::
        Specify the bundle version.  Version 2 is the older format and can only be
        used with SHA-1 repositories; the newer version 3 contains capabilities that

diff --git a/builtin/bundle.c b/builtin/bundle.c
index acceef6200172cf7a44de45bfa6f3e940b6444c9..c2b916e027a1a68d8b9d2b6607c5b093541547d9 100644 (file)
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -12,7 +12,7 @@
  */
 
 #define BUILTIN_BUNDLE_CREATE_USAGE \
-       N_("git bundle create [-q | --quiet | --progress | --all-progress] [--all-progress-implied]\n" \
+       N_("git bundle create [-q | --quiet | --progress]\n" \
           "                  [--version=<version>] <file> <git-rev-list-args>")
 #define BUILTIN_BUNDLE_VERIFY_USAGE \
        N_("git bundle verify [-q | --quiet] <file>")
@@ -64,7 +64,7 @@ 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 all_progress_implied = 1;
        int progress = isatty(STDERR_FILENO);
        struct strvec pack_opts;
        int version = -1;
@@ -74,11 +74,12 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
                            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_SET_INT_F(0, "all-progress", &progress,
+                             N_("historical; same as --progress"), 2,
+                             PARSE_OPT_HIDDEN),
+               OPT_HIDDEN_BOOL(0, "all-progress-implied",
+                               &all_progress_implied,
+                               N_("historical; does nothing")),
                OPT_INTEGER(0, "version", &version,
                            N_("specify bundle format version")),
                OPT_END()

diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
index 3a1cf30b1d737e3bc5cb34039822b7d45ab99445..6b905e1b836d113fe03b6801b708d3e18db48eb9 100755 (executable)
--- a/t/t6020-bundle-misc.sh
+++ b/t/t6020-bundle-misc.sh
@@ -10,6 +10,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-bundle.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 for cmd in create verify list-heads unbundle
 do
@@ -566,4 +567,16 @@ test_expect_success 'cloning from filtered bundle has useful error' '
        grep "cannot clone from filtered bundle" err
 '
 
+test_expect_success 'bundle progress includes write phase' '
+       GIT_PROGRESS_DELAY=0 \
+               git bundle create --progress out.bundle --all 2>err &&
+       grep 'Writing' err
+'
+
+test_expect_success TTY 'create --quiet disables all bundle progress' '
+       test_terminal env GIT_PROGRESS_DELAY=0 \
+               git bundle create --quiet out.bundle --all 2>err &&
+       test_must_be_empty err
+'
+
 test_done