git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Casey Fitzpatrick <kcghost@gmail.com>
To: Stefan Beller <sbeller@google.com>
Cc: git <git@vger.kernel.org>
Subject: Re: [PATCH 1/2] submodule: Add --progress option to add command
Date: Tue, 1 May 2018 16:48:49 -0400
Message-ID: <CAEp-SHXURpp3XuyHv-pNERRE8WK+o5ao8E2htDA3gdYZGAz5vA@mail.gmail.com> (raw)
In-Reply-To: <CAGZ79kbmVMgLWg8nW9Jnwie0vbfov7PaujMPeVr1oq5o24hcDw@mail.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

  reply index

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-30  8:29 git-submodule is missing --dissociate option Casey Fitzpatrick
2018-04-30 11:30 ` Casey Fitzpatrick
2018-04-30 13:16   ` Ævar Arnfjörð Bjarmason
2018-05-02  4:32     ` Junio C Hamano
2018-04-30 18:19 ` Stefan Beller
2018-04-30 21:39   ` Casey Fitzpatrick
2018-05-01 18:09     ` [PATCH 0/2] Add --progress and --dissociate to git submodule Casey Fitzpatrick
2018-05-01 18:09       ` [PATCH 1/2] submodule: Add --progress option to add command Casey Fitzpatrick
2018-05-01 18:41         ` Stefan Beller
2018-05-01 20:48           ` Casey Fitzpatrick [this message]
2018-05-01 18:09       ` [PATCH 2/2] submodule: Add --dissociate option to add/update commands Casey Fitzpatrick
2018-05-01 20:23         ` Stefan Beller
2018-05-01 20:25         ` Eric Sunshine
2018-05-01 21:21           ` Casey Fitzpatrick
2018-05-01 18:20       ` [PATCH 0/2] Add --progress and --dissociate to git submodule Stefan Beller
2018-05-02  0:27     ` [PATCH 0/3] " Casey Fitzpatrick
2018-05-02  0:27       ` [PATCH 1/3] submodule: clean up subsititions in script Casey Fitzpatrick
2018-05-02  0:27       ` [PATCH 2/3] submodule: add --progress option to add command Casey Fitzpatrick
2018-05-02  0:27       ` [PATCH 3/3] submodule: add --dissociate option to add/update commands Casey Fitzpatrick
2018-05-02  0:40         ` Casey Fitzpatrick
2018-05-02  0:55     ` [PATCH 0/3] Add --progress and --dissociate to git submodule Casey Fitzpatrick
2018-05-02  0:55       ` [PATCH 1/3] submodule: clean up subsititions in script Casey Fitzpatrick
2018-05-02  5:59         ` Junio C Hamano
2018-05-03 10:46           ` Casey Fitzpatrick
2018-05-02  0:55       ` [PATCH 2/3] submodule: add --progress option to add command Casey Fitzpatrick
2018-05-02  0:55       ` [PATCH 3/3] submodule: add --dissociate option to add/update commands Casey Fitzpatrick
2018-05-02  4:37       ` [PATCH 0/3] Add --progress and --dissociate to git submodule Junio C Hamano
2018-05-02  8:54         ` Casey Fitzpatrick
2018-05-02  4:34   ` git-submodule is missing --dissociate option Junio C Hamano

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAEp-SHXURpp3XuyHv-pNERRE8WK+o5ao8E2htDA3gdYZGAz5vA@mail.gmail.com \
    --to=kcghost@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox