Re: [PATCH v2 2/2] var: allow GIT_EDITOR to return null

[Sean Allred <allred.sean@gmail.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Negate git with "test_must_fail", not "!", this would e.g. hide
> segfaults. See t/README's discussion about it.
>
>> +		test_cmp expect actual
>
> Looks like this should be:
>
> 	test_must_fail git ... >out &&
> 	test_must_be_empty out

Nice! I don't know why I didn't look for t/README, but I also found
test_expect_code, which seems to be even more specific as to what is
being expected. I assume it has the same segfault detection.

This has now been incorporated in my branch; I'll submit it in v3 later
today.

>> +test_expect_success 'get GIT_EDITOR with configuration and environment variable EDITOR' '
>> +	test_config core.editor foo &&
>> +	(
>> +		sane_unset GIT_EDITOR &&
>> +		sane_unset VISUAL &&
>> +		sane_unset EDITOR &&
>> +		echo foo >expect &&
>> +		EDITOR=bar git var GIT_EDITOR >actual &&
>> +		test_cmp expect actual
>> +	)
>
> Perhaps these can all be factored into a helper to hide this repetition
> in a function, but maybe not. E.g:
>
> 	test_git_var () {
> 		cat >expect &&
> 		(
> 			[...common part of subshell ...]
> 		        "$@" >actual &&
> 			test_cmp expect actual
> 		)
> 	}
>
> (untested)

In all honesty, I think too much abstraction would do more harm than
good here. I definitely share the instinct to factor out the common
pieces, but in other codebases I've worked in, that tends to stifle
future changes in the tests themselves.

That said, I can't realistically imagine a world where a
'sane_unset_all_editors' would stifle code changes -- and I think that
accounts for the lion's share of the repetition. I've incorporated such
a helper in my branch now.

If you're not convinced there should be further abstraction, I'd rather
leave things 'stupid simple' -- but if you think this would block merge,
I'd be happy to take a crack at further factoring out what I can.


--
Sean Allred