Re: [PATCH v2] pull: handle --log=<n>

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> On Tue, May 19, 2015 at 3:30 PM, Dennis Kaarsemaker
> <dennis@kaarsemaker.net> wrote:
>> On di, 2015-05-19 at 15:10 -0700, Junio C Hamano wrote:
>>> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>>>
>>> > I've actually done it differently while implementing:
>>> >
>>> > 1) Make test_commit recognize --tags and stop creating tags unless
>>> >    specified
>>> > 2) while ! prove --state=save,failed {
>>> >        Find and fix tests that now need --tags
>>> >    }
>>>
>>> That was what I feared.  The result of that process is the hardest
>>> to reason about and review.
>>
>> I'm not quite sure I understand what you're trying to say. You seem to
>> be worried that there will be silent successes for tests that should
>> fail after N/N if I take the proposed approach. I have no idea how that
>> could happen though.
>
> You are placing too much faith in the tests. We would want to see
> a conversion that is reasonable and indeed reasoned by eyeballing
> the change for correctness.

To put it in another way, imagine this scenario.

Six months ago, you found a bug in "git frotz" command.  The bug
triggered only when a commit had an extra tag pointing at it.  You
fixed the bug, and in order to protect your fix from getting broken
by other people, added a new test that would have failed with the
bug present, perhaps like this:

    test_expect_success 'commit with an extra tag' '
	test_commit sample &&
        git frotz HEAD
    '

Fast forward to today.  You changed "test_commit" and it no longer
creates a tag by default with your step 1.

You run this test again.

It will pass.

Why?  "git frotz" command used to break when you had tag, but did
not break if you didn't have the tag even without your bugfix six
months ago.

Next week, somebody will accidentally break the bugfix you did six
months ago.  "git frotz" again does not work correctly when the
commit is pointed at by a tag.  But with the test will not catch
this breakage.

The problem was that your step 1 broke this test, and your step 2
of running the test did not catch the fact that you broke it.

Hopefully now you have some idea how your approach is problematic.