Re: [PATCH] sequencer: detect author name errors in read_author_script()
- From
Jeff King <peff@peff.net>
- Date
- Oct 3, 2022, 17:27 UTC
- Message-ID
- <YzsbiRcCEaJUGwdp@coredump.intra.peff.net>
- In-Reply-To
- <221003.86k05htf84.gmgdl@evledraar.gmail.com>
On Mon, Oct 03, 2022 at 11:40:52AM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 11 quoted lines
> > +check_broken_author 'unknown key in author-script' '
> > + echo "GIT_AUTHOR_BOGUS=${SQ}whatever${SQ}" \
> > + >>.git/rebase-merge/author-script'
> > +
> > +
> > +test_done
>
> Maybe I just have too much PTSD from dealing with shell quoting issues
> with this '"$script"" pattern when you need to pass arguments with
> spaces in it, or even quotes. Although it probably won't ever be an
> issue here.Quoting-wise, it's the exact same as passing the snippet to test_expect_success that we already do. Which, granted, is full of horrors, but is well understood within our code base. The "$script" is evaluated when we pass the snippet to test_expect_success, which sees the expansion along with the rest of the script. The double-quotes around it ensure that whitespace is retained.
There is one extra quirk here, which is that it must not end with a newline, since we stick "&&" on it.
Show 13 quoted lines
> + # Avoid quoting issues > + write_script script.sh && > + > + test_expect_success "$title of '$script'" ' > # create conflicted state > test_when_finished "git rebase --abort" && > git checkout -B tmp branch2 && > test_must_fail git rebase branch1 && > > - # break author-script > - '"$script"' && > + ./script.sh >tmp && > + mv tmp .git/rebase-merge/author-script &&
One of my goals was that the script be expanded inline so that verbose-mode showed what the test was actually doing. But this hides it behind a vague "script.sh". Maybe that's not important enough to merit the admitted ugliness of the loop+function we have.
If we give that up, then a better option is probably to put the before/after parts in functions, like:
diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh index e68aac4b36..b92a3ce46b 100755 --- a/t/t3438-rebase-broken-files.sh +++ b/t/t3438-rebase-broken-files.sh @@ -11,43 +11,49 @@ test_expect_success 'set up conflicting branches' ' test_commit two file ' -check_broken_author () { - title=$1; shift - script=$1; shift - - test_expect_success "$title" ' - # create conflicted state - test_when_finished "git rebase --abort" && - git checkout -B tmp branch2 && - test_must_fail git rebase branch1 && - - # break author-script - '"$script"' && - - # resolving notices broken author-script - echo resolved >file && - git add file && - test_must_fail git rebase --continue - ' +create_conflict () { + test_when_finished "git rebase --abort" && + git checkout -B tmp branch2 && + test_must_fail git rebase branch1 +} + +check_resolve_fails () { + echo resolved >file && + git add file && + test_must_fail git rebase --continue } for item in NAME EMAIL DATE do - check_broken_author "detect missing GIT_AUTHOR_$item" ' + test_expect_success "detect missing GIT_AUTHOR_$item" ' + create_conflict && + grep -v $item .git/rebase-merge/author-script >tmp && - mv tmp .git/rebase-merge/author-script' + mv tmp .git/rebase-merge/author-script && + + check_resolve_fails + ' done for item in NAME EMAIL DATE do - check_broken_author "detect duplicate GIT_AUTHOR_$item" ' + test_expect_success "detect duplicate GIT_AUTHOR_$item" ' + create_conflict && + grep -i $item .git/rebase-merge/author-script >tmp && - cat tmp >>.git/rebase-merge/author-script' + cat tmp >>.git/rebase-merge/author-script && + + check_resolve_fails + ' done -check_broken_author 'unknown key in author-script' ' +test_expect_success 'unknown key in author-script' ' + create_conflict && + echo "GIT_AUTHOR_BOGUS=${SQ}whatever${SQ}" \ - >>.git/rebase-merge/author-script' + >>.git/rebase-merge/author-script && + check_resolve_fails +' test_done That makes the boilerplate shorter in the "-v" output but focuses on the actual modification that breaks the author-script. -Peff