| Message ID | 427cb7b55ac3fead1651cbad7318b9c0bb454b08.1669395151.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve consistency of git-var | expand |
On Fri, Nov 25 2022, Sean Allred via GitGitGadget wrote: > From: Sean Allred <allred.sean@gmail.com> > +test_expect_success 'get GIT_EDITOR without configuration' ' > + ( > + sane_unset GIT_EDITOR && > + sane_unset VISUAL && > + sane_unset EDITOR && > + >expect && > + ! git var GIT_EDITOR >actual && 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 > +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)
Æ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
diff --git a/builtin/var.c b/builtin/var.c index e215cd3b0c0..5678ec68bfe 100644 --- a/builtin/var.c +++ b/builtin/var.c @@ -11,12 +11,7 @@ static const char var_usage[] = "git var (-l | <variable>)"; static const char *editor(int flag) { - const char *pgm = git_editor(); - - if (!pgm && flag & IDENT_STRICT) - die("Terminal is dumb, but EDITOR unset"); - - return pgm; + return git_editor(); } static const char *pager(int flag) diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh index e56f4b9ac59..bdef271c92a 100755 --- a/t/t0007-git-var.sh +++ b/t/t0007-git-var.sh @@ -47,6 +47,75 @@ test_expect_success 'get GIT_DEFAULT_BRANCH with configuration' ' ) ' +test_expect_success 'get GIT_EDITOR without configuration' ' + ( + sane_unset GIT_EDITOR && + sane_unset VISUAL && + sane_unset EDITOR && + >expect && + ! git var GIT_EDITOR >actual && + test_cmp expect actual + ) +' + +test_expect_success 'get GIT_EDITOR with configuration' ' + test_config core.editor foo && + ( + sane_unset GIT_EDITOR && + sane_unset VISUAL && + sane_unset EDITOR && + echo foo >expect && + git var GIT_EDITOR >actual && + test_cmp expect actual + ) +' + +test_expect_success 'get GIT_EDITOR with environment variable GIT_EDITOR' ' + ( + sane_unset GIT_EDITOR && + sane_unset VISUAL && + sane_unset EDITOR && + echo bar >expect && + GIT_EDITOR=bar git var GIT_EDITOR >actual && + test_cmp expect actual + ) +' + +test_expect_success 'get GIT_EDITOR with environment variable EDITOR' ' + ( + sane_unset GIT_EDITOR && + sane_unset VISUAL && + sane_unset EDITOR && + echo bar >expect && + EDITOR=bar git var GIT_EDITOR >actual && + test_cmp expect actual + ) +' + +test_expect_success 'get GIT_EDITOR with configuration and environment variable GIT_EDITOR' ' + test_config core.editor foo && + ( + sane_unset GIT_EDITOR && + sane_unset VISUAL && + sane_unset EDITOR && + echo bar >expect && + GIT_EDITOR=bar git var GIT_EDITOR >actual && + test_cmp expect actual + ) +' + +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 + ) +' + # For git var -l, we check only a representative variable; # testing the whole output would make our test too brittle with # respect to unrelated changes in the test suite's environment.