git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>, Brandon Williams <bmwill@google.com>
Subject: Re: [PATCHv2 2/6] submodule test invocation: only pass additional arguments
Date: Tue, 23 May 2017 11:29:12 -0700
Message-ID: <CAGZ79kb2iu1D1hRbGNx9aP_ebCyXzrCZQorbop7BG11vSUNzoA@mail.gmail.com> (raw)
In-Reply-To: <xmqqefvgks0g.fsf@gitster.mtv.corp.google.com>

On Mon, May 22, 2017 at 11:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
>> index e8f70b806f..2672f104cf 100755
>> --- a/t/t2013-checkout-submodule.sh
>> +++ b/t/t2013-checkout-submodule.sh
>> @@ -65,9 +65,9 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/
>>
>>  KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
>>  KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1
>> -test_submodule_switch_recursing "git checkout --recurse-submodules"
>> +test_submodule_switch_recursing "checkout"
>>
>> -test_submodule_forced_switch_recursing "git checkout -f --recurse-submodules"
>> +test_submodule_forced_switch_recursing "checkout -f"
>>
>>  test_submodule_switch "git checkout"
>
> Doesn't the above look crazy to you?

Oh well. The commit message doesn't explain why the craziness is
required here (really!).

    submodule test invocation: only pass additional arguments

    In a later patch we want to introduce a config option to trigger
    the submodule recursing by default. As this option should be
    available and uniform across all commands that deal with submodules
    we'd want to test for this option in the submodule update library.

    So instead of calling the whole test set again for
    "git -c submodule.recurse foo" instead of
    "git foo --recurse-submodules", we'd only want to introduce one
    basic test that tests if the option is recognized and respected
    to not overload the test suite.

>
> It is hostile to other people (and those who need to make merges)
> who have to work with test_submodule_switch_recursing that older one
> used to take the full command but its definition suddenly changes so
> that the caller now must omit the leading "git".

I am not aware of other people (or other series in flight by myself) that use
one of the switches currently.

>  Even worse,
> another helper with a similar-sounding name, test_submodule_switch,
> still must be called with the leading "git".

Oh, yeah that is a real issue. I will migrate all of them.

>
> The same comment applies to the one we can see below.

An alternative would be to come up with a slightly different name
to ensure we do not have issues with other series in flight. The function
name is already pretty long, so encoding even more information in it
may be not a good idea. But the argument is shorter, so maybe:

- test_submodule_switch_recursing "git reset --hard --recurse-submodules"
+ test_submodule_switch_recursing_args_only  "reset --hard"

Thanks,
Stefan

  reply index

Thread overview: 11+ messages in thread (expand / mbox.gz / Atom feed / [top])
2017-05-22 19:48 [PATCHv2 0/6] Add option to recurse into submodules Stefan Beller
2017-05-22 19:48 ` [PATCHv2 1/6] submodule.c: add has_submodules to check if we have any submodules Stefan Beller
2017-05-23 18:40   ` Brandon Williams
2017-05-23 18:52     ` Stefan Beller
2017-05-22 19:48 ` Stefan Beller
2017-05-23  6:26   ` Junio C Hamano
2017-05-23 18:29     ` Stefan Beller [this message]
2017-05-22 19:48 ` [PATCHv2 3/6] Introduce submodule.recurse option for worktree manipulators Stefan Beller
2017-05-22 19:48 ` [PATCHv2 4/6] builtin/fetch.c: respect 'submodule.recurse' option Stefan Beller
2017-05-22 19:48 ` [PATCHv2 5/6] builtin/grep.c: respect 'submodule.recurse' option Stefan Beller
2017-05-22 19:48 ` [PATCHv2 6/6] builtin/push.c: respect 'submodule.recurse' option Stefan Beller

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 to all the recipients using the --to, --cc,
  and --in-reply-to switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGZ79kb2iu1D1hRbGNx9aP_ebCyXzrCZQorbop7BG11vSUNzoA@mail.gmail.com \
    --to=sbeller@google.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.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