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

From: Sean Allred <allred.sean@gmail.com>
Date: Nov 26, 2022, 13:54 UTC
Message-ID: <87fse5ssyo.fsf@gmail.com>
In-Reply-To: <221125.86pmdamyv5.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 9 quoted lines

> 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.

Show 24 quoted lines

>> +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