Thanks, I'll clean it up based on your comments. I based the tests from t5606-clone-options.sh; I'm not sure why now but I thought I needed that clone -o thing from there, but it appears useless. On Tue, May 1, 2018 at 2:41 PM, Stefan Beller <sbeller@xxxxxxxxxx> wrote: > On Tue, May 1, 2018 at 11:09 AM, Casey Fitzpatrick <kcghost@xxxxxxxxx> wrote: >> --progress was missing from add command, was only in update. >> Add --progress where it makes sense (both clone helper commands). >> Add missing documentation entry. >> Add test. > > Maybe: > The '--progress' was introduced in 72c5f88311d (clone: pass --progress > decision to recursive submodules, 2016-09-22) to fix the progress reporting > of the clone command. Also add the progress option to the 'submodule add' > command. The update command already support the progress flag, but it > is not documented. > >> @@ -117,6 +117,9 @@ cmd_add() >> -q|--quiet) >> GIT_QUIET=1 >> ;; >> + --progress) >> + progress="--progress" >> + ;; > > The code looks good, though unlike the other commands progress is a > boolean decision. > > If we want to support --no-progress as well, we can do so by adding > --no-progress) > progress="--no-progress" > ;; > and then the submodule helper ought to cope with it. > > >> +test_expect_success 'setup test repo' ' >> + mkdir parent && >> + (cd parent && git init && >> + echo one >file && git add file && >> + git commit -m one) >> +' > > Coding style: > ( > parens open on its own line, and its contents > are indented by a tab. > > Instead of coding this yourself, you could write the > test as: > > test_create_repo parent && > test_create_commit -C parent one > >> +test_expect_success 'clone -o' ' > > What are we testing here? I do not quite see the connection to > testing --progress here. > >> + git clone -o foo parent clone-o && >> + (cd clone-o && git rev-parse --verify refs/remotes/foo/master) > > >> +test_expect_success 'redirected submodule add does not show progress' ' >> + ( >> + cd addtest && > > > >> + git submodule add "file://$submodurl/parent" submod-redirected \ >> + >out 2>err && >> + ! grep % err && > > We're grepping for percent to see that there is no progress. At first I thought > the percent sign might be a special character, had to research it to know it's > meant literally. TiL! > >> + test_i18ngrep ! "Checking connectivity" err > > Makes sense. > >> + ) > > We could omit the extra shell by using > > git -C addtest submodule add "file://$... \ > >../out 2>../err && > > Why do we need 'out' ? > >> +test_expect_success 'redirected submodule add --progress does show progress' ' >> + ( >> + cd addtest && >> + git submodule add --progress "file://$submodurl/parent" \ >> + submod-redirected-progress >out 2>err && \ >> + grep % err >> + ) >> +' > > Thanks for writing these tests! > Stefan
