Re: [PATCH 1/2] submodule: Add --progress option to add command

From: Stefan Beller <sbeller@google.com>
Date: May 1, 2018, 18:41 UTC
Message-ID: <CAGZ79kbmVMgLWg8nW9Jnwie0vbfov7PaujMPeVr1oq5o24hcDw@mail.gmail.com>
In-Reply-To: <20180501180908.17443-2-kcghost@gmail.com>

On Tue, May 1, 2018 at 11:09 AM, Casey Fitzpatrick <kcghost@gmail.com> 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.

Show 7 quoted lines

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

Show 6 quoted lines

> +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' ?

Show 8 quoted lines

> +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