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

[Casey Fitzpatrick <kcghost@gmail.com>]

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@google.com> wrote:
> 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.
>
>> @@ -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