From: Eric Sunshine <sunshine@sunshineco.com>
To: Philippe Blain via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Michael J Gruber <git@grubix.eu>,
Matthieu Moy <git@matthieu-moy.fr>,
John Keeping <john@keeping.me.uk>,
Karthik Nayak <karthik.188@gmail.com>, Jeff King <peff@peff.net>,
Alex Henrie <alexhenrie24@gmail.com>,
Philippe Blain <levraiphilippeblain@gmail.com>
Subject: Re: [PATCH v3 1/3] t: add lib-crlf-messages.sh for messages containing CRLF
Date: Mon, 12 Oct 2020 18:47:06 -0400 [thread overview]
Message-ID: <20201012224706.GA4318@flurp.local> (raw)
In-Reply-To: <f17d182c3bf5e758490441801423cdb0da17060d.1602526169.git.gitgitgadget@gmail.com>
On Mon, Oct 12, 2020 at 06:09:27PM +0000, Philippe Blain via GitGitGadget wrote:
> Add a test library (t/lib-crlf-messages.sh) that creates refs with such
> commit messages, so that we can easily test that this bug does not
> appear in other commands in the future.
In addition to Junio's review comments...
>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> diff --git a/t/lib-crlf-messages.sh b/t/lib-crlf-messages.sh
> @@ -0,0 +1,90 @@
> +create_crlf_ref () {
> + message="$1" &&
> + subject="$2" &&
> + body="$3" &&
> + branch="$4" &&
> + printf "${message}" >.crlf-message-${branch}.txt &&
> + printf "${subject}" >.crlf-subject-${branch}.txt &&
> + printf "${body}" >.crlf-body-${branch}.txt &&
> + LIB_CRLF_BRANCHES="${LIB_CRLF_BRANCHES} ${branch}"
Broken &&-chain.
> + test_tick &&
> + hash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&
> + git branch ${branch} ${hash} &&
> + git tag tag-${branch} ${branch} -F .crlf-message-${branch}.txt --cleanup=verbatim
> +}
> +
> +create_crlf_refs () {
> + message="Subject first line\r\n\r\nBody first line\r\nBody second line\r\n" &&
> + body="Body first line\r\nBody second line\r\n" &&
> + subject="Subject first line" &&
> + branch="crlf" &&
> + create_crlf_ref "${message}" "${subject}" "${body}" "${branch}" &&
This is somewhat onerous to digest and compose. Have you considered
making it more automated and easier to read? Perhaps something like
this:
create_crlf_ref () {
branch=$1
cat >.crlf-message-$branch.txt &&
sed -n "1,/^$/p" <.crlf-message-$branch.txt | sed "/^$/d" | append_cr >.crlf-subject-$branch.txt &&
sed -n "/^$/,\$p" <.crlf-message-$branch.txt | sed "1d" | append_cr >.crlf-body-$branch.txt &&
...
}
create_crlf_refs () {
create_crlf_ref crlf <<-\EOF
Subject first line
Body first line
Body second line
EOF
...
}
> +test_create_crlf_refs () {
> + test_expect_success 'setup refs with CRLF commit messages' '
> + create_crlf_refs
> + '
> +}
This almost seems like an unnecessary indirection since callers could
just as easily do this on their own, like this:
test_expect_success 'setup refs with CRLF commit messages' '
create_crlf_refs
'
which isn't very burdensome. However, I suppose doing it this way
gives consistent test titles between scripts, so not necessarily a
strong objection on my part.
> +cleanup_crlf_refs () {
> + for branch in ${LIB_CRLF_BRANCHES}; do
Our style is to place 'do' on its own line:
for branch in $LIB_CRLF_BRANCHES
do
...
This would be a syntax error if LIB_CRLF_BRANCHES is empty for some
reason, but I suppose we don't really have to worry about it here(?).
> + git branch -D ${branch} &&
> + git tag -d tag-${branch} &&
> + rm .crlf-message-${branch}.txt &&
> + rm .crlf-subject-${branch}.txt &&
> + rm .crlf-body-${branch}.txt
> + done
> +}
> +
> +test_cleanup_crlf_refs () {
> + test_expect_success 'cleanup refs with CRLF commit messages' '
> + cleanup_crlf_refs
> + '
> +}
> +
> +test_crlf_subject_body_and_contents() {
> + command_and_args="$@" &&
> + command=$1 &&
> + if [ ${command} = "branch" ] || [ ${command} = "for-each-ref" ] || [ ${command} = "tag" ]; then
> + atoms="(contents:subject) (contents:body) (contents)"
> + elif [ ${command} = "log" ] || [ ${command} = "show" ]; then
> + atoms="s b B"
> + fi &&
Style:
if test "$command" = "branch" || test ...
then
...
elif test ...
then
...
fi &&
> + files="subject body message" &&
> + while [ -n "${atoms}" ]; do
Too many spaces after 'while'.
Style:
while tests -n "..."
do
...
> + set ${atoms} && atom=$1 && shift && atoms="$*" &&
> + set ${files} && file=$1 && shift && files="$*" &&
> + test_expect_success "${command}: --format='%${atom}' works with CRLF input" "
> + rm -f expect &&
> + for ref in ${LIB_CRLF_BRANCHES}; do
Style.
> + cat .crlf-${file}-\"\${ref}\".txt >>expect &&
> + printf \"\n\" >>expect
> + done &&
> + git $command_and_args --format=\"%${atom}\" >actual &&
> + test_cmp expect actual
> + "
> + done
> +}
next prev parent reply other threads:[~2020-10-12 22:47 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-08 18:29 [PATCH 0/3] Teach ref-filter API to correctly handle CRLF in messages Philippe Blain via GitGitGadget
2020-03-08 18:29 ` [PATCH 1/3] t: add lib-crlf-messages.sh for messages containing CRLF Philippe Blain via GitGitGadget
2020-03-08 18:29 ` [PATCH 2/3] ref-filter: teach the API to correctly handle CRLF Philippe Blain via GitGitGadget
2020-03-08 18:29 ` [PATCH 3/3] log: add tests for messages containing CRLF to t4202 Philippe Blain via GitGitGadget
2020-03-09 15:14 ` [PATCH 0/3] Teach ref-filter API to correctly handle CRLF in messages Junio C Hamano
2020-03-10 2:19 ` Philippe Blain
2020-03-10 2:24 ` [PATCH v2 " Philippe Blain via GitGitGadget
2020-03-10 2:24 ` [PATCH v2 1/3] t: add lib-crlf-messages.sh for messages containing CRLF Philippe Blain via GitGitGadget
2020-03-10 2:24 ` [PATCH v2 2/3] ref-filter: fix the API to correctly handle CRLF Philippe Blain via GitGitGadget
2020-03-10 17:50 ` Junio C Hamano
2020-03-10 2:24 ` [PATCH v2 3/3] log: add tests for messages containing CRLF to t4202 Philippe Blain via GitGitGadget
2020-03-10 3:31 ` [PATCH v2 0/3] Teach ref-filter API to correctly handle CRLF in messages Junio C Hamano
2020-03-10 17:24 ` Junio C Hamano
2020-10-12 18:09 ` [PATCH v3 0/3] ref-filter: handle CRLF at end-of-line more gracefully Philippe Blain via GitGitGadget
2020-10-12 18:09 ` [PATCH v3 1/3] t: add lib-crlf-messages.sh for messages containing CRLF Philippe Blain via GitGitGadget
2020-10-12 22:22 ` Junio C Hamano
2020-10-14 13:22 ` Philippe Blain
2020-10-12 22:47 ` Eric Sunshine [this message]
2020-10-14 13:20 ` Philippe Blain
2020-10-14 13:45 ` Eric Sunshine
2020-10-14 13:52 ` Philippe Blain
2020-10-14 23:01 ` Eric Sunshine
2020-10-22 3:09 ` Philippe Blain
2020-10-12 18:09 ` [PATCH v3 2/3] ref-filter: handle CRLF at end-of-line more gracefully Philippe Blain via GitGitGadget
2020-10-12 22:24 ` Junio C Hamano
2020-10-14 13:09 ` Philippe Blain
2020-10-12 18:09 ` [PATCH v3 3/3] log, show: add tests for messages containing CRLF Philippe Blain via GitGitGadget
2020-10-22 3:01 ` [PATCH v4 0/2] ref-filter: handle CRLF at end-of-line more gracefully Philippe Blain via GitGitGadget
2020-10-22 3:01 ` [PATCH v4 1/2] " Philippe Blain via GitGitGadget
2020-10-23 0:52 ` Junio C Hamano
2020-10-23 1:46 ` Philippe Blain
2020-10-28 20:24 ` Junio C Hamano
2020-10-29 1:29 ` Philippe Blain
2020-10-22 3:01 ` [PATCH v4 2/2] log, show: add tests for messages containing CRLF Philippe Blain via GitGitGadget
2020-10-22 19:24 ` Philippe Blain
2020-10-29 12:48 ` [PATCH v5 0/2] ref-filter: handle CRLF at end-of-line more gracefully Philippe Blain via GitGitGadget
2020-10-29 12:48 ` [PATCH v5 1/2] " Philippe Blain via GitGitGadget
2020-10-29 12:48 ` [PATCH v5 2/2] log, show: add tests for messages containing CRLF Philippe Blain via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201012224706.GA4318@flurp.local \
--to=sunshine@sunshineco.com \
--cc=alexhenrie24@gmail.com \
--cc=git@grubix.eu \
--cc=git@matthieu-moy.fr \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=john@keeping.me.uk \
--cc=karthik.188@gmail.com \
--cc=levraiphilippeblain@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.