| Message ID | ca869cef57bcf620a7b5d0519d362dcd9a27eae6.1573094789.git.congdanhqx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Correct internal working and output encoding | expand |
On Thu, Nov 07, 2019 at 09:56:14AM +0700, Doan Tran Cong Danh wrote: > We're using fixup!/squash! <subject> to mark if current commit will be > used to be fixed up or squashed to a previous commit. > > However, if we're changing i18n.commitencoding after making the > original commit but before making the fixing up, we couldn't find the > original commit to do the fixup/squash. > > Add a test to demonstrate that problem. OK, this makes sense to do. I'm not sure if we need to test so many combinations, as the problem is apparent even on some vanilla ones. But I guess this is just following the lead of the rest of the script. > +test_commit_autosquash_multi_encoding () { > + flag=$1 > + old=$2 > + new=$3 > + msg=$4 > + test_expect_failure "commit --$flag into $old from $new" ' > + git checkout -b '$flag-$old-$new' C0 && These single quotes are funny; they close the test-snippet string, so these variables are outside of any quoting (and thus subject to whitespace splitting). The test snippets are run as an eval, so they have access to the variables you set above. I.e., just: git checkout -b $flag-$old-$new C0 would work. Or: git checkout -b "$flag-$old-$new" C0 if you wanted to be more careful inside the snippet. > + git config i18n.commitencoding '$old' && > + echo '$old' >>F && > + git commit -a -F "$TEST_DIRECTORY/t3900/'$msg'" && Likewise for all these other bits of the script. -Peff
On 2019-11-07 01:02:33 -0500, Jeff King wrote: > On Thu, Nov 07, 2019 at 09:56:14AM +0700, Doan Tran Cong Danh wrote: > > > +test_commit_autosquash_multi_encoding () { > > + flag=$1 > > + old=$2 > > + new=$3 > > + msg=$4 > > + test_expect_failure "commit --$flag into $old from $new" ' > > + git checkout -b '$flag-$old-$new' C0 && > > These single quotes are funny; they close the test-snippet string, so > these variables are outside of any quoting (and thus subject to > whitespace splitting). Yes, those quotes are funny, and I'm also aware that they will be subjected to whitespace spliting. But those quotes were intentional, they're there in order to have better log with: ./t3900-i18n-commit.sh -v With those funny quotes, we will see this (verbose) log: expecting success of 3900.38 'commit --fixup into ISO-2022-JP from UTF-8': git checkout -b fixup-ISO-2022-JP-UTF-8 C0 && git config i18n.commitencoding ISO-2022-JP && echo ISO-2022-JP >>F && git commit -a -F "$TEST_DIRECTORY/t3900/ISO-2022-JP.txt" && test_tick && echo intermediate stuff >>G && git add G && git commit -a -m "intermediate commit" && test_tick && git config i18n.commitencoding UTF-8 && echo UTF-8-fixup >>F && git commit -a --fixup HEAD^ && git rebase --autosquash -i HEAD^^^ && git rev-list HEAD >actual && test_line_count = 3 actual && iconv -f ISO-2022-JP -t utf-8 "$TEST_DIRECTORY/t3900/ISO-2022-JP.txt" >expect && git cat-file commit HEAD^ >raw && (sed "1,/^$/d" raw | iconv -f UTF-8 -t utf-8) >actual && test_cmp expect actual I think it's easier to manual run the test step with this log.
On Thu, Nov 07, 2019 at 01:48:54PM +0700, Danh Doan wrote: > On 2019-11-07 01:02:33 -0500, Jeff King wrote: > > On Thu, Nov 07, 2019 at 09:56:14AM +0700, Doan Tran Cong Danh wrote: > > > > > +test_commit_autosquash_multi_encoding () { > > > + flag=$1 > > > + old=$2 > > > + new=$3 > > > + msg=$4 > > > + test_expect_failure "commit --$flag into $old from $new" ' > > > + git checkout -b '$flag-$old-$new' C0 && > > > > These single quotes are funny; they close the test-snippet string, so > > these variables are outside of any quoting (and thus subject to > > whitespace splitting). > > Yes, those quotes are funny, and I'm also aware that > they will be subjected to whitespace spliting. > But those quotes were intentional, they're there in order to have > better log with: > > ./t3900-i18n-commit.sh -v > > With those funny quotes, we will see this (verbose) log: > > expecting success of 3900.38 'commit --fixup into ISO-2022-JP from UTF-8': > git checkout -b fixup-ISO-2022-JP-UTF-8 C0 && Yes, it's true you get the expanded version here, but... > git config i18n.commitencoding ISO-2022-JP && > echo ISO-2022-JP >>F && > git commit -a -F "$TEST_DIRECTORY/t3900/ISO-2022-JP.txt" && ...you still can't just run this manually because of other lines like this one. It's also weirdly unlike all of the other tests, which creates confusion for people reading the code. IMHO the tradeoff isn't worth it. -Peff
On 2019-11-07 03:02:18 -0500, Jeff King wrote: > > git config i18n.commitencoding ISO-2022-JP && > > echo ISO-2022-JP >>F && > > git commit -a -F "$TEST_DIRECTORY/t3900/ISO-2022-JP.txt" && > > ...you still can't just run this manually because of other lines like > this one. Except we can with a little effort: export TEST_DIRECTORY=.. > It's also weirdly unlike all of the other tests, which creates confusion > for people reading the code. IMHO the tradeoff isn't worth it. Hm, I think it's the test_commit_autosquash_flag is the one that is weird in this file. Most of other sets of tests (line 89-176) use the same quote. test_commit_autosquash_flag is the inconsistent one, for the most part, it doesn't employ the funny quote, but it uses the funny one in the `git cat-file` line.
On Thu, Nov 07, 2019 at 05:51:09PM +0700, Danh Doan wrote: > On 2019-11-07 03:02:18 -0500, Jeff King wrote: > > > git config i18n.commitencoding ISO-2022-JP && > > > echo ISO-2022-JP >>F && > > > git commit -a -F "$TEST_DIRECTORY/t3900/ISO-2022-JP.txt" && > > > > ...you still can't just run this manually because of other lines like > > this one. > > Except we can with a little effort: > > export TEST_DIRECTORY=.. Sure, but if you allow setting variables, you could do the same for "$msg", etc. :) > > It's also weirdly unlike all of the other tests, which creates confusion > > for people reading the code. IMHO the tradeoff isn't worth it. > > Hm, I think it's the test_commit_autosquash_flag is the one that is weird > in this file. Most of other sets of tests (line 89-176) use the same quote. Yeah, you're right. I did look at the other tests to see if it was an existing style, but of course that was the exact one I looked at. ;) IMHO it's still a bad style (and is unlike most of the rest of our tests), but as it's following the existing style in the file, I can live with it (and we can think about changing it all as a separate step later). -Peff
diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh index b92ff95977..0f978bfde1 100755 --- a/t/t3900-i18n-commit.sh +++ b/t/t3900-i18n-commit.sh @@ -204,4 +204,33 @@ test_commit_autosquash_flags eucJP fixup test_commit_autosquash_flags ISO-2022-JP squash +test_commit_autosquash_multi_encoding () { + flag=$1 + old=$2 + new=$3 + msg=$4 + test_expect_failure "commit --$flag into $old from $new" ' + git checkout -b '$flag-$old-$new' C0 && + git config i18n.commitencoding '$old' && + echo '$old' >>F && + git commit -a -F "$TEST_DIRECTORY/t3900/'$msg'" && + test_tick && + echo intermediate stuff >>G && + git add G && + git commit -a -m "intermediate commit" && + test_tick && + git config i18n.commitencoding '$new' && + echo '$new-$flag' >>F && + git commit -a --'$flag' HEAD^ && + git rebase --autosquash -i HEAD^^^ && + git rev-list HEAD >actual && + test_line_count = 3 actual + ' +} + +test_commit_autosquash_multi_encoding fixup UTF-8 ISO-8859-1 1-UTF-8.txt +test_commit_autosquash_multi_encoding squash ISO-8859-1 UTF-8 ISO8859-1.txt +test_commit_autosquash_multi_encoding squash eucJP ISO-2022-JP eucJP.txt +test_commit_autosquash_multi_encoding fixup ISO-2022-JP UTF-8 ISO-2022-JP.txt + test_done
We're using fixup!/squash! <subject> to mark if current commit will be used to be fixed up or squashed to a previous commit. However, if we're changing i18n.commitencoding after making the original commit but before making the fixing up, we couldn't find the original commit to do the fixup/squash. Add a test to demonstrate that problem. Helped-by: Jeff King <peff@peff.net> Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com> --- t/t3900-i18n-commit.sh | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)