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

> @@ -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 [this message]
2018-05-01 20:48           ` Casey Fitzpatrick
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=CAGZ79kbmVMgLWg8nW9Jnwie0vbfov7PaujMPeVr1oq5o24hcDw@mail.gmail.com \
    --to=sbeller@google.com \
    --cc=git@vger.kernel.org \
    --cc=kcghost@gmail.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