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

Junio C Hamano [off-list ref] writes:

On Tue, May 19, 2015 at 3:30 PM, Dennis Kaarsemaker
[off-list ref] wrote:

quoted

On di, 2015-05-19 at 15:10 -0700, Junio C Hamano wrote:

quoted

Dennis Kaarsemaker [off-list ref] writes:

quoted

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.

On di, 2015-05-19 at 19:19 -0700, Junio C Hamano wrote:

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

Yes, thanks for the thorough explanation!

Two more bad sideeffects, so two more reasons not to take this approach:

- test_must_fail tests might now fail for the wrong reason, undetectedly
  ("ref does not exist" instead of "can't handle ref")
- same for TODO tests

-- 
Dennis Kaarsemaker
www.kaarsemaker.net