Re: [PATCH v2 4/4] lib-submodule-update: pass OVERWRITING_FAIL

From: Denton Liu <liu.denton@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>, Taylor Blau <me@ttaylorr.com>,
	Johannes Sixt <j6t@kdbg.org>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH v2 4/4] lib-submodule-update: pass OVERWRITING_FAIL
Date: Thu, 21 May 2020 14:55:14 -0400	[thread overview]
Message-ID: <20200521185514.GA615266@generichostname> (raw)
In-Reply-To: <20200521182928.GA1308647@coredump.intra.peff.net>

Hi Peff,

On Thu, May 21, 2020 at 02:29:28PM -0400, Jeff King wrote:
> > This is useful because currently, when we run a test helper function, we
> > just mark the whole thing as `test_must_fail`. However, it's possible
> > that the helper function might fail earlier or later than expected due
> > to an introduced bug. If this happens, then the test case will still
> > report as passing but it should really be marked as failing since it
> > didn't actually display the intended behaviour.
> 
> Now this second concern I think is much more interesting, because it
> impacts the test results. And it's really not even about test_must_fail
> in particular, but is a general problem with checking failure in any
> compound operation. We may expect the early parts to succeed, but the
> later parts to fail, and we want to tell the difference. And that's true
> whether you're using "!", test_must_fail, etc.
> 
> You solve it here by passing OVERWRITING_FAIL down into the callback
> functions. And that does work. But I think it may be easier to
> understand if we invert the responsibility. Let the outer caller specify
> two callbacks: one for setup/prep that must succeed, and one for a
> single operation where we might expect success or failure.

I believe that we'll need a third optional argument. For example, in
t3906, we have the following diff

	diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
	index 6a7e801ca0..6e8dac9669 100755
	--- a/t/t3906-stash-submodule.sh
	+++ b/t/t3906-stash-submodule.sh
	@@ -8,7 +8,11 @@ test_description='stash can handle submodules'
	 git_stash () {
		git status -su >expect &&
		ls -1pR * >>expect &&
	-	git read-tree -u -m "$1" &&
	+	$OVERWRITING_FAIL git read-tree -u -m "$1" &&
	+	if test -n "$OVERWRITING_FAIL"
	+	then
	+		return
	+	fi &&
		git stash &&
		git status -su >actual &&
		ls -1pR * >>actual &&

which means, when we're doing test_must_fail, we have to skip the
remainder of the function otherwise, if the first command succeeds but
it fails later in the test, then we report success even though it didn't
fail at the point where we were expecting. I think that we'll have to
have an optional third arg for $cleanup or something. The only thing
that I'm worried about is that having three separate functions being
passed in may be too messy.

Aside from that, I like this approach much more than mine.

Thanks,

Denton