Re: [PATCH v4 2/2] commit: add --ignore-submodules[=<when>] parameter

From: Jens Lehmann <jens.lehmann@web.de>
Date: Apr 22, 2014, 19:14 UTC
Message-ID: <5356BF7C.1010200@web.de>
In-Reply-To: <535596D1.6070709@gmail.com>

Am 22.04.2014 00:08, schrieb Ronald Weiss:

Show 22 quoted lines

> On 18. 4. 2014 14:09, Jens Lehmann wrote:
>> Am 13.04.2014 00:49, schrieb Ronald Weiss:
>>> Allow ignoring submodules (or not) by command line switch, like diff
>>> and status do.
>>>
>>> Git commit honors the 'ignore' setting from .gitmodules or .git/config,
>>> but didn't allow to override it from command line.
>>>
>>> This patch depends on Jens Lehmann's patch "commit -m: commit staged
>>> submodules regardless of ignore config". Without it,
>>> "commit -m --ignore-submodules" would not work and tests introduced
>>> here would fail.
>>
>> Apart from the flags of the test (see below) I get three failures when
>> running t7513. And I'm wondering why you are using "test_might_fail"
>> there, shouldn't we know exactly if a commit should succeed or not
>> and test exactly for that?
> 
> I used "test_might_fail" instead of "test_must_fail" to denote that this
> test doesn't care whether "git commit" fails or not due to empty commit
> message. I found it more appropriate. However, if you disagree, I can
> change it to "test_must_fail", no problem for me.

I'd rather have them fail because nothing is to be committed (and not because the commit message is empty) and document we expect that to happen by using test_must_fail (and that for example will catch a later change which accidentally makes commit create empty commits here).

> Unfortunately I don't know why the test fails for you, they pass for me.
> Did you apply it on top of your own patch for "commit -m", which is
> a prerequisite?

Silly me, I forgot to do that (even though you explicitly mention this dependency in the commit message). Sorry for the noise, all tests run fine after rebasing your changes on top of mine.

Show 9 quoted lines

> I tried it again, on top of current master (cc29195 [tag: v2.0.0-rc0]).
> First, I have applied your patch from here:
> 
> http://article.gmane.org/gmane.comp.version-control.git/245783
> 
> On top of that, I applied my two patches, and the tests pass fine here.
> Until now I was testing on Windows, but now I tested on a Linux box,
> and that makes no difference.
>