{"thread":{"id":"55364","subject":"Bug report: 'filtering content' delayed progress message does not respect --quiet","startedAt":"2021-03-21T20:54:05Z","lastAt":"2021-08-27T02:26:38Z","messageCount":7,"participants":["Sean Allred","Jeff King","Matheus Tavares","Ævar Arnfjörð Bjarmason","Matheus Tavares Bernardino"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"419887","messageId":"CABceR4ZFVW=zeSwef7_dP+TWZ29J7BUkmMEB1CzCz=et_yYS9w@mail.gmail.com","threadId":"55364","inReplyTo":null,"subject":"Bug report: 'filtering content' delayed progress message does not respect --quiet","fromName":"Sean Allred","fromEmail":"allred.sean@gmail.com","sentAt":"2021-03-21T20:53:07Z","receivedAt":"2021-03-21T20:54:05Z","isPatch":false,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"Hi folks,\n\nSubmitting the filled-out template from git-bugreport. Let me know if\nI can be of any assistance.\n\nThanks!\n-Sean\n\nThank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\n  Called `git clone --quiet git://path/to/private/repo`\n\nWhat did you expect to happen? (Expected behavior)\n\n  Expected git to be quiet :-)  Did not expect writes to stderr/stdout.\n\nWhat happened instead? (Actual behavior)\n\n  Received output that looked like\n\n      Filtering content:  --% (--/--), --.-- MiB | --.-- MiB/s\n\nWhat's different between what you expected and what actually happened?\n\n  `--quiet` should suppress all output, but in actuality, that line\n  was still output.\n\nAnything else you want to add:\n\n  Submitting from https://github.com/git-lfs/git-lfs/issues/1270 (re\n  https://github.com/git-lfs/git-lfs/issues/1270#issuecomment-461176647).\n  Other details may be available there.\n\n  I've verified I can still reproduce on the current version of git\n  (listed below) with a private repo (note to self:\n  gid://gitlab/Project/2458).\n\n  It looks like we need similar handling from\n  index_pack.c:resolve_deltas at entry.c:finish_delayed_checkout.  It\n  looks like the necessary state may not be available where it's\n  needed, but this is based on about two minutes of reading :-)\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.31.0.windows.1\ncpu: x86_64\nbuilt from commit: 959b2f0c6fb12f8ba13f9015c4482fa8133b7f9c\nsizeof-long: 4\nsizeof-size_t: 8\nshell-path: /bin/sh\nfeature: fsmonitor--daemon\nuname: Windows 10.0 17763\ncompiler info: gnuc: 10.2\nlibc info: no libc information available\n$SHELL (typically, interactive shell): <unset>\n\n\n[Enabled Hooks]\nnot run from a git repository - no hooks to show\n\n\n-- \n-Sean\n"},{"id":"420243","messageId":"YF2b8LLhE0vjc7mg@coredump.intra.peff.net","threadId":"55364","inReplyTo":"CABceR4ZFVW=zeSwef7_dP+TWZ29J7BUkmMEB1CzCz=et_yYS9w@mail.gmail.com","subject":"Re: Bug report: 'filtering content' delayed progress message does not respect --quiet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-26T08:31:44Z","receivedAt":"2021-03-26T08:32:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 21, 2021 at 03:53:07PM -0500, Sean Allred wrote:\n\n> What did you do before the bug happened? (Steps to reproduce your issue)\n> \n>   Called `git clone --quiet git://path/to/private/repo`\n> \n> What did you expect to happen? (Expected behavior)\n> \n>   Expected git to be quiet :-)  Did not expect writes to stderr/stdout.\n> \n> What happened instead? (Actual behavior)\n> \n>   Received output that looked like\n> \n>       Filtering content:  --% (--/--), --.-- MiB | --.-- MiB/s\n\n+cc Lars, who added this in 52f1d62eb4 (convert: display progress for\nfiltered objects that have been delayed, 2017-08-20).\n\nThe message is in finish_delayed_checkout(), which gets only a \"struct\ncheckout\" to carry the state. That has a \"quiet\" field, but I'm not sure\nit is set appropriately. E.g., builtin/checkout.c's checkout_worktree()\ndoes not set it at all, and it is unconditionally set in unpack-trees.c's\ncheck_updates().\n\nWe should obviously be respecting --quiet, but also checking isatty(2)\nbefore auto-enabling. Probably we need a separate show_progress field.\nFor unpack-trees, I think it would get set from o->verbose_update, which\nis what controls the existing \"Updating files\" meter. For checkout.c, it\nprobably comes from checkout_opts.show_progress.\n\n-Peff\n"},{"id":"433759","messageId":"d1405b781915c085ac8a8965dadf3efbe1b0f6aa.1629915330.git.matheus.bernardino@usp.br","threadId":"55364","inReplyTo":"YF2b8LLhE0vjc7mg@coredump.intra.peff.net","subject":"[PATCH] checkout: make delayed checkout respect --quiet and --no-progress","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-08-25T18:15:55Z","receivedAt":"2021-08-25T18:16:05Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The 'Filtering contents...' progress report from delayed checkout is\ndisplayed even when checkout and clone are invoked with --quiet or\n--no-progress. Furthermore, it is displayed unconditionally, without\nfirst checking whether stdout is a tty. Let's fix these issues and also\nadd some regression tests for the two code paths that currently use\ndelayed checkout: unpack_trees.c:check_updates() and\nbuiltin/checkout.c:checkout_worktree().\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n\nThe test section of the patch is a bit long because it checks all the\nverbosity options related to the progress report, and it also tests both\nthe check_updates() and checkout_worktree() code paths. If that is\noverkill, I can remove some tests.\n\n builtin/checkout.c    |  2 +-\n entry.c               |  7 +++--\n entry.h               |  3 ++-\n t/t0021-conversion.sh | 63 +++++++++++++++++++++++++++++++++++++++++++\n unpack-trees.c        |  2 +-\n 5 files changed, 72 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex b5d477919a..b23bc149d1 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -404,7 +404,7 @@ static int checkout_worktree(const struct checkout_opts *opts,\n \tmem_pool_discard(&ce_mem_pool, should_validate_cache_entries());\n \tremove_marked_cache_entries(&the_index, 1);\n \tremove_scheduled_dirs();\n-\terrs |= finish_delayed_checkout(&state, &nr_checkouts);\n+\terrs |= finish_delayed_checkout(&state, &nr_checkouts, opts->show_progress);\n \n \tif (opts->count_checkout_paths) {\n \t\tif (nr_unmerged)\ndiff --git a/entry.c b/entry.c\nindex 125fabdbd5..044e8ec92c 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -159,7 +159,8 @@ static int remove_available_paths(struct string_list_item *item, void *cb_data)\n \treturn !available;\n }\n \n-int finish_delayed_checkout(struct checkout *state, int *nr_checkouts)\n+int finish_delayed_checkout(struct checkout *state, int *nr_checkouts,\n+\t\t\t    int show_progress)\n {\n \tint errs = 0;\n \tunsigned delayed_object_count;\n@@ -173,7 +174,9 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts)\n \n \tdco->state = CE_RETRY;\n \tdelayed_object_count = dco->paths.nr;\n-\tprogress = start_delayed_progress(_(\"Filtering content\"), delayed_object_count);\n+\tprogress = show_progress\n+\t\t? start_delayed_progress(_(\"Filtering content\"), delayed_object_count)\n+\t\t: NULL;\n \twhile (dco->filters.nr > 0) {\n \t\tfor_each_string_list_item(filter, &dco->filters) {\n \t\t\tstruct string_list available_paths = STRING_LIST_INIT_NODUP;\ndiff --git a/entry.h b/entry.h\nindex b8c0e170dc..7c889e58fd 100644\n--- a/entry.h\n+++ b/entry.h\n@@ -43,7 +43,8 @@ static inline int checkout_entry(struct cache_entry *ce,\n }\n \n void enable_delayed_checkout(struct checkout *state);\n-int finish_delayed_checkout(struct checkout *state, int *nr_checkouts);\n+int finish_delayed_checkout(struct checkout *state, int *nr_checkouts,\n+\t\t\t    int show_progress);\n \n /*\n  * Unlink the last component and schedule the leading directories for\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex b5749f327d..be12b92f21 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -6,6 +6,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n TEST_ROOT=\"$PWD\"\n PATH=$TEST_ROOT:$PATH\n@@ -1061,4 +1062,66 @@ test_expect_success PERL,SYMLINKS,CASE_INSENSITIVE_FS \\\n \t)\n '\n \n+test_expect_success PERL 'setup for progress tests' '\n+\tgit init progress &&\n+\t(\n+\t\tcd progress &&\n+\t\tgit config filter.delay.process \"rot13-filter.pl delay-progress.log clean smudge delay\" &&\n+\t\tgit config filter.delay.required true &&\n+\n+\t\techo \"*.a filter=delay\" >.gitattributes &&\n+\t\ttouch test-delay10.a &&\n+\t\tgit add . &&\n+\t\tgit commit -m files\n+\t)\n+'\n+\n+for mode in pathspec branch\n+do\n+\tcase \"$mode\" in\n+\tpathspec) opt='.' ;;\n+\tbranch) opt='-f HEAD' ;;\n+\tesac\n+\n+\ttest_expect_success PERL,TTY \"delayed checkout shows progress by default only on tty ($mode checkout)\" '\n+\t\t(\n+\t\t\tcd progress &&\n+\t\t\trm -f *.a delay-progress.log &&\n+\t\t\ttest_terminal env GIT_PROGRESS_DELAY=0 git checkout $opt 2>err &&\n+\t\t\tgrep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n+\t\t\tgrep \"Filtering content\" err &&\n+\n+\t\t\trm -f *.a delay-progress.log &&\n+\t\t\tGIT_PROGRESS_DELAY=0 git checkout $opt 2>err &&\n+\t\t\tgrep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n+\t\t\t! grep \"Filtering content\" err\n+\t\t)\n+\t'\n+\n+\ttest_expect_success PERL,TTY \"delayed checkout ommits progress with --quiet ($mode checkout)\" '\n+\t\t(\n+\t\t\tcd progress &&\n+\t\t\trm -f *.a delay-progress.log &&\n+\t\t\ttest_terminal env GIT_PROGRESS_DELAY=0 git checkout --quiet $opt 2>err &&\n+\t\t\tgrep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n+\t\t\t! grep \"Filtering content\" err\n+\t\t)\n+\t'\n+\n+\ttest_expect_success PERL,TTY \"delayed checkout honors --[no]-progress ($mode checkout)\" '\n+\t\t(\n+\t\t\tcd progress &&\n+\t\t\trm -f *.a delay-progress.log &&\n+\t\t\ttest_terminal env GIT_PROGRESS_DELAY=0 git checkout --no-progress $opt 2>err &&\n+\t\t\tgrep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n+\t\t\t! grep \"Filtering content\" err &&\n+\n+\t\t\trm -f *.a delay-progress.log &&\n+\t\t\ttest_terminal env GIT_PROGRESS_DELAY=0 git checkout --quiet --progress $opt 2>err &&\n+\t\t\tgrep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n+\t\t\tgrep \"Filtering content\" err\n+\t\t)\n+\t'\n+done\n+\n test_done\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 5786645f31..f07304f1b7 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -479,7 +479,7 @@ static int check_updates(struct unpack_trees_options *o,\n \t\terrs |= run_parallel_checkout(&state, pc_workers, pc_threshold,\n \t\t\t\t\t      progress, &cnt);\n \tstop_progress(&progress);\n-\terrs |= finish_delayed_checkout(&state, NULL);\n+\terrs |= finish_delayed_checkout(&state, NULL, o->verbose_update);\n \tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n \n \tif (o->clone)\n-- \n2.32.0\n\n"},{"id":"433786","messageId":"87bl5lccx0.fsf@evledraar.gmail.com","threadId":"55364","inReplyTo":"d1405b781915c085ac8a8965dadf3efbe1b0f6aa.1629915330.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] checkout: make delayed checkout respect --quiet and --no-progress","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-25T23:35:20Z","receivedAt":"2021-08-25T23:39:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Aug 25 2021, Matheus Tavares wrote:\n\n> The test section of the patch is a bit long because it checks all the\n> verbosity options related to the progress report, and it also tests both\n> the check_updates() and checkout_worktree() code paths. If that is\n> overkill, I can remove some tests.\n\nMore exhaustive tests are generally nice..\n\n> +test_expect_success PERL 'setup for progress tests' '\n> +\tgit init progress &&\n> +\t(\n> +\t\tcd progress &&\n> +\t\tgit config filter.delay.process \"rot13-filter.pl delay-progress.log clean smudge delay\" &&\n> +\t\tgit config filter.delay.required true &&\n> +\n> +\t\techo \"*.a filter=delay\" >.gitattributes &&\n> +\t\ttouch test-delay10.a &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m files\n> +\t)\n> +'\n\nThis doesn't seem to depend on PERL, should this really be a skip_all at\nthe top if we don't have the TTY prereq, i.e. we shouldn't bother?\n\n> +\n> +for mode in pathspec branch\n> +do\n> +\tcase \"$mode\" in\n> +\tpathspec) opt='.' ;;\n> +\tbranch) opt='-f HEAD' ;;\n> +\tesac\n> +\n> +\ttest_expect_success PERL,TTY \"delayed checkout shows progress by default only on tty ($mode checkout)\" '\n\nAll of the PERL,TTY can just be TTY, since TTY itself checks PERL.\n\n> +\t\t(\n> +\t\t\tcd progress &&\n> +\t\t\trm -f *.a delay-progress.log &&\n> +\t\t\ttest_terminal env GIT_PROGRESS_DELAY=0 git checkout $opt 2>err &&\n> +\t\t\tgrep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n> +\t\t\tgrep \"Filtering content\" err &&\n\nThis seems to need TTY...\n\n> +\t\t\trm -f *.a delay-progress.log &&\n> +\t\t\tGIT_PROGRESS_DELAY=0 git checkout $opt 2>err &&\n> +\t\t\tgrep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n> +\t\t\t! grep \"Filtering content\" err\n\nBut this one doesn't, perhaps it could be a non-TTY test?\n\n> +\t\t)\n> +\t'\n> +\n> +\ttest_expect_success PERL,TTY \"delayed checkout ommits progress with --quiet ($mode checkout)\" '\n> +\t\t(\n> +\t\t\tcd progress &&\n> +\t\t\trm -f *.a delay-progress.log &&\n> +\t\t\ttest_terminal env GIT_PROGRESS_DELAY=0 git checkout --quiet $opt 2>err &&\n> +\t\t\tgrep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n> +\t\t\t! grep \"Filtering content\" err\n> +\t\t)\n> +\t'\n> +\n> +\ttest_expect_success PERL,TTY \"delayed checkout honors --[no]-progress ($mode checkout)\" '\n> +\t\t(\n> +\t\t\tcd progress &&\n> +\t\t\trm -f *.a delay-progress.log &&\n> +\t\t\ttest_terminal env GIT_PROGRESS_DELAY=0 git checkout --no-progress $opt 2>err &&\n> +\t\t\tgrep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n> +\t\t\t! grep \"Filtering content\" err &&\n> +\n> +\t\t\trm -f *.a delay-progress.log &&\n> +\t\t\ttest_terminal env GIT_PROGRESS_DELAY=0 git checkout --quiet --progress $opt 2>err &&\n> +\t\t\tgrep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n> +\t\t\tgrep \"Filtering content\" err\n> +\t\t)\n> +\t'\n\nIt looks like these tests could be split into one helper function which\njust passed params for e.g. whether the \"Filtering content\" grep was\nnegated, and what command should be run.\n\nAlso if possible the two sections of the test could be split up, and\nthen the \"rm -rf\" could just be a \"test_when_finished\" at the top...\n"},{"id":"433838","messageId":"CAHd-oW7Z8TXZTRmSN0FkCpqEzz7-chJwYbDqyJaQ_ETW8xoG+Q@mail.gmail.com","threadId":"55364","inReplyTo":"87bl5lccx0.fsf@evledraar.gmail.com","subject":"Re: [PATCH] checkout: make delayed checkout respect --quiet and --no-progress","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-08-26T14:26:46Z","receivedAt":"2021-08-26T14:27:01Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Ævar\n\nThanks for the comments!\n\nOn Wed, Aug 25, 2021 at 8:39 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> On Wed, Aug 25 2021, Matheus Tavares wrote:\n>\n> > +test_expect_success PERL 'setup for progress tests' '\n> > +     git init progress &&\n> > +     (\n> > +             cd progress &&\n> > +             git config filter.delay.process \"rot13-filter.pl delay-progress.log clean smudge delay\" &&\n> > +             git config filter.delay.required true &&\n> > +\n> > +             echo \"*.a filter=delay\" >.gitattributes &&\n> > +             touch test-delay10.a &&\n> > +             git add . &&\n> > +             git commit -m files\n> > +     )\n> > +'\n>\n> This doesn't seem to depend on PERL,\n\nIt actually depends on PERL because `git add .` will run the clean\nfilter for `test-delay10.a`.\n\n> should this really be a skip_all at\n> the top if we don't have the TTY prereq, i.e. we shouldn't bother?\n\nYeah, I think it could be a skip_all. But as you pointed out below,\none of the tests doesn't really depend on TTY, so I guess we could\nleave the independent prereqs for each test.\n\n> > +\n> > +for mode in pathspec branch\n> > +do\n> > +     case \"$mode\" in\n> > +     pathspec) opt='.' ;;\n> > +     branch) opt='-f HEAD' ;;\n> > +     esac\n> > +\n> > +     test_expect_success PERL,TTY \"delayed checkout shows progress by default only on tty ($mode checkout)\" '\n>\n> All of the PERL,TTY can just be TTY, since TTY itself checks PERL.\n\nI don't mind changing that, but isn't it a bit clearer for readers to\nhave both dependencies explicitly?\n\n> > +             (\n> > +                     cd progress &&\n> > +                     rm -f *.a delay-progress.log &&\n> > +                     test_terminal env GIT_PROGRESS_DELAY=0 git checkout $opt 2>err &&\n> > +                     grep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n> > +                     grep \"Filtering content\" err &&\n>\n> This seems to need TTY...\n>\n> > +                     rm -f *.a delay-progress.log &&\n> > +                     GIT_PROGRESS_DELAY=0 git checkout $opt 2>err &&\n> > +                     grep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n> > +                     ! grep \"Filtering content\" err\n>\n> But this one doesn't, perhaps it could be a non-TTY test?\n\nGood catch, I'll split this test in two.\n\n> > +             )\n> > +     '\n> > +\n> > +     test_expect_success PERL,TTY \"delayed checkout ommits progress with --quiet ($mode checkout)\" '\n> > +             (\n> > +                     cd progress &&\n> > +                     rm -f *.a delay-progress.log &&\n> > +                     test_terminal env GIT_PROGRESS_DELAY=0 git checkout --quiet $opt 2>err &&\n> > +                     grep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n> > +                     ! grep \"Filtering content\" err\n> > +             )\n> > +     '\n> > +\n> > +     test_expect_success PERL,TTY \"delayed checkout honors --[no]-progress ($mode checkout)\" '\n> > +             (\n> > +                     cd progress &&\n> > +                     rm -f *.a delay-progress.log &&\n> > +                     test_terminal env GIT_PROGRESS_DELAY=0 git checkout --no-progress $opt 2>err &&\n> > +                     grep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n> > +                     ! grep \"Filtering content\" err &&\n> > +\n> > +                     rm -f *.a delay-progress.log &&\n> > +                     test_terminal env GIT_PROGRESS_DELAY=0 git checkout --quiet --progress $opt 2>err &&\n> > +                     grep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n> > +                     grep \"Filtering content\" err\n> > +             )\n> > +     '\n>\n> It looks like these tests could be split into one helper function which\n> just passed params for e.g. whether the \"Filtering content\" grep was\n> negated, and what command should be run.\n\nMakes sense, I'll do that.\n\n> Also if possible the two sections of the test could be split up, and\n> then the \"rm -rf\" could just be a \"test_when_finished\" at the top...\n\nHmm, as we are removing the `test-delay10.a` file in order to check it\nout again with custom options, I think it's a bit clearer to remove it\nright before the actual git checkout invocation.\n"},{"id":"433861","messageId":"f3ac3246254c99e6ecb4a4578022d04324691c63.1630004263.git.matheus.bernardino@usp.br","threadId":"55364","inReplyTo":"d1405b781915c085ac8a8965dadf3efbe1b0f6aa.1629915330.git.matheus.bernardino@usp.br","subject":"[PATCH v2] checkout: make delayed checkout respect --quiet and --no-progress","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-08-26T19:10:06Z","receivedAt":"2021-08-26T19:10:17Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The 'Filtering contents...' progress report from delayed checkout is\ndisplayed even when checkout and clone are invoked with --quiet or\n--no-progress. Furthermore, it is displayed unconditionally, without\nfirst checking whether stdout is a tty. Let's fix these issues and also\nadd some regression tests for the two code paths that currently use\ndelayed checkout: unpack_trees.c:check_updates() and\nbuiltin/checkout.c:checkout_worktree().\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n\nChanges since v1:\n\n- Exctract duplicated code from the different test cases into an\n  auxiliary function.\n- Split test that do not depend on TTY. \n\n builtin/checkout.c    |  2 +-\n entry.c               |  7 +++--\n entry.h               |  3 +-\n t/t0021-conversion.sh | 71 +++++++++++++++++++++++++++++++++++++++++++\n unpack-trees.c        |  2 +-\n 5 files changed, 80 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex b5d477919a..b23bc149d1 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -404,7 +404,7 @@ static int checkout_worktree(const struct checkout_opts *opts,\n \tmem_pool_discard(&ce_mem_pool, should_validate_cache_entries());\n \tremove_marked_cache_entries(&the_index, 1);\n \tremove_scheduled_dirs();\n-\terrs |= finish_delayed_checkout(&state, &nr_checkouts);\n+\terrs |= finish_delayed_checkout(&state, &nr_checkouts, opts->show_progress);\n \n \tif (opts->count_checkout_paths) {\n \t\tif (nr_unmerged)\ndiff --git a/entry.c b/entry.c\nindex 125fabdbd5..044e8ec92c 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -159,7 +159,8 @@ static int remove_available_paths(struct string_list_item *item, void *cb_data)\n \treturn !available;\n }\n \n-int finish_delayed_checkout(struct checkout *state, int *nr_checkouts)\n+int finish_delayed_checkout(struct checkout *state, int *nr_checkouts,\n+\t\t\t    int show_progress)\n {\n \tint errs = 0;\n \tunsigned delayed_object_count;\n@@ -173,7 +174,9 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts)\n \n \tdco->state = CE_RETRY;\n \tdelayed_object_count = dco->paths.nr;\n-\tprogress = start_delayed_progress(_(\"Filtering content\"), delayed_object_count);\n+\tprogress = show_progress\n+\t\t? start_delayed_progress(_(\"Filtering content\"), delayed_object_count)\n+\t\t: NULL;\n \twhile (dco->filters.nr > 0) {\n \t\tfor_each_string_list_item(filter, &dco->filters) {\n \t\t\tstruct string_list available_paths = STRING_LIST_INIT_NODUP;\ndiff --git a/entry.h b/entry.h\nindex b8c0e170dc..7c889e58fd 100644\n--- a/entry.h\n+++ b/entry.h\n@@ -43,7 +43,8 @@ static inline int checkout_entry(struct cache_entry *ce,\n }\n \n void enable_delayed_checkout(struct checkout *state);\n-int finish_delayed_checkout(struct checkout *state, int *nr_checkouts);\n+int finish_delayed_checkout(struct checkout *state, int *nr_checkouts,\n+\t\t\t    int show_progress);\n \n /*\n  * Unlink the last component and schedule the leading directories for\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex b5749f327d..33dfc9cd56 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -6,6 +6,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n TEST_ROOT=\"$PWD\"\n PATH=$TEST_ROOT:$PATH\n@@ -1061,4 +1062,74 @@ test_expect_success PERL,SYMLINKS,CASE_INSENSITIVE_FS \\\n \t)\n '\n \n+test_expect_success PERL 'setup for progress tests' '\n+\tgit init progress &&\n+\t(\n+\t\tcd progress &&\n+\t\tgit config filter.delay.process \"rot13-filter.pl delay-progress.log clean smudge delay\" &&\n+\t\tgit config filter.delay.required true &&\n+\n+\t\techo \"*.a filter=delay\" >.gitattributes &&\n+\t\ttouch test-delay10.a &&\n+\t\tgit add . &&\n+\t\tgit commit -m files\n+\t)\n+'\n+\n+test_delayed_checkout_progress () {\n+\tif test \"$1\" = \"!\"\n+\tthen\n+\t\tlocal expect_progress=N &&\n+\t\tshift\n+\telse\n+\t\tlocal expect_progress=\n+\tfi &&\n+\n+\tif test $# -lt 1\n+\tthen\n+\t\tBUG \"no command given to test_delayed_checkout_progress\"\n+\tfi &&\n+\n+\t(\n+\t\tcd progress &&\n+\t\tGIT_PROGRESS_DELAY=0 &&\n+\t\texport GIT_PROGRESS_DELAY &&\n+\t\trm -f *.a delay-progress.log &&\n+\n+\t\t\"$@\" 2>err &&\n+\t\tgrep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delay-progress.log &&\n+\t\tif test \"$expect_progress\" = N\n+\t\tthen\n+\t\t\t! grep \"Filtering content\" err\n+\t\telse\n+\t\t\tgrep \"Filtering content\" err\n+\t\tfi\n+\t)\n+}\n+\n+for mode in pathspec branch\n+do\n+\tcase \"$mode\" in\n+\tpathspec) opt='.' ;;\n+\tbranch) opt='-f HEAD' ;;\n+\tesac\n+\n+\ttest_expect_success PERL,TTY \"delayed checkout shows progress by default on tty ($mode checkout)\" '\n+\t\ttest_delayed_checkout_progress test_terminal git checkout $opt\n+\t'\n+\n+\ttest_expect_success PERL \"delayed checkout ommits progress on non-tty ($mode checkout)\" '\n+\t\ttest_delayed_checkout_progress ! git checkout $opt\n+\t'\n+\n+\ttest_expect_success PERL,TTY \"delayed checkout ommits progress with --quiet ($mode checkout)\" '\n+\t\ttest_delayed_checkout_progress ! test_terminal git checkout --quiet $opt\n+\t'\n+\n+\ttest_expect_success PERL,TTY \"delayed checkout honors --[no]-progress ($mode checkout)\" '\n+\t\ttest_delayed_checkout_progress ! test_terminal git checkout --no-progress $opt &&\n+\t\ttest_delayed_checkout_progress test_terminal git checkout --quiet --progress $opt\n+\t'\n+done\n+\n test_done\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 5786645f31..f07304f1b7 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -479,7 +479,7 @@ static int check_updates(struct unpack_trees_options *o,\n \t\terrs |= run_parallel_checkout(&state, pc_workers, pc_threshold,\n \t\t\t\t\t      progress, &cnt);\n \tstop_progress(&progress);\n-\terrs |= finish_delayed_checkout(&state, NULL);\n+\terrs |= finish_delayed_checkout(&state, NULL, o->verbose_update);\n \tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n \n \tif (o->clone)\n-- \n2.32.0\n\n"},{"id":"433898","messageId":"YShNV1isGfxO1QZn@coredump.intra.peff.net","threadId":"55364","inReplyTo":"CAHd-oW7Z8TXZTRmSN0FkCpqEzz7-chJwYbDqyJaQ_ETW8xoG+Q@mail.gmail.com","subject":"Re: [PATCH] checkout: make delayed checkout respect --quiet and --no-progress","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-27T02:26:31Z","receivedAt":"2021-08-27T02:26:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 26, 2021 at 11:26:46AM -0300, Matheus Tavares Bernardino wrote:\n\n> > > +for mode in pathspec branch\n> > > +do\n> > > +     case \"$mode\" in\n> > > +     pathspec) opt='.' ;;\n> > > +     branch) opt='-f HEAD' ;;\n> > > +     esac\n> > > +\n> > > +     test_expect_success PERL,TTY \"delayed checkout shows progress by default only on tty ($mode checkout)\" '\n> >\n> > All of the PERL,TTY can just be TTY, since TTY itself checks PERL.\n> \n> I don't mind changing that, but isn't it a bit clearer for readers to\n> have both dependencies explicitly?\n\nNo just clearer, but the perl dependency of TTY is an implementation\ndetail. It's conceivable that we could end up converting it to another\nlanguage (e.g., I recall there are at least some races with the stdin\nmechanism, according to [0]).\n\n-Peff\n\n[0] https://lore.kernel.org/git/20190520125016.GA13474@sigill.intra.peff.net/\n"}]}