| Message ID | patch-08.16-352eeff41c9-20210412T110456Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib.sh: new test_commit args, simplification & fixes | expand |
On Mon, Apr 12, 2021 at 7:09 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > Add a --printf option to test_commit to allow writing to the file with > "printf" instead of "echo". > > This is useful for writing "\n", "\0" etc., in particular in > combination with the --append option added in 3373518cc8 (test-lib > functions: add an --append option to test_commit, 2021-01-12). > [...] > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- Just a bit of pure bikeshedding... > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > @@ -173,6 +173,10 @@ debug () { > +# --printf > +# Use "printf" instead of "echo" when writing "<contents>" to > +# "<file>". You will need to provide your own trailing "\n". You > +# can only supply the FORMAT for the printf(1), not its ARGUMENT(s). The name "printf" has such strong association in programmer's minds with "%" and argument consumption that the name of this option alone almost begs people to take advantage of argument interpolation even though it's documented here as not allowing it. Taking into consideration that people often do not read documentation, `--printf` as the name of the option may be an unfortunate one. Perhaps it could be called `--raw` or something less likely to suggest argument interpolation. > @@ -192,6 +196,7 @@ debug () { > > test_commit () { > notick= && > + echo=echo && This could be slightly confusing. I wonder if naming this variable `emit` would be clearer.
Eric Sunshine <sunshine@sunshineco.com> writes: > The name "printf" has such strong association in programmer's minds > with "%" and argument consumption that the name of this option alone > almost begs people to take advantage of argument interpolation even > though it's documented here as not allowing it. Taking into > consideration that people often do not read documentation, `--printf` > as the name of the option may be an unfortunate one. Perhaps it could > be called `--raw` or something less likely to suggest argument > interpolation. The reason we want to use 'printf' instead of 'echo' is because only some implementations of 'echo' honors '\t\n\r' etc., and 'echo' by others show these literally. Using printf(1) allows us to write these backslashed special characters universally. So, I find 'raw' equally confusing, if not more. >> @@ -192,6 +196,7 @@ debug () { >> >> test_commit () { >> notick= && >> + echo=echo && > > This could be slightly confusing. I wonder if naming this variable > `emit` would be clearer. Perhaps.
On Mon, Apr 12, 2021 at 5:27 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > The name "printf" has such strong association in programmer's minds > > with "%" and argument consumption that the name of this option alone > > almost begs people to take advantage of argument interpolation even > > though it's documented here as not allowing it. Taking into > > consideration that people often do not read documentation, `--printf` > > as the name of the option may be an unfortunate one. Perhaps it could > > be called `--raw` or something less likely to suggest argument > > interpolation. > > The reason we want to use 'printf' instead of 'echo' is because only > some implementations of 'echo' honors '\t\n\r' etc., and 'echo' by > others show these literally. Using printf(1) allows us to write > these backslashed special characters universally. > > So, I find 'raw' equally confusing, if not more. I don't care for `--raw` either but couldn't think of anything better at the time. But perhaps a name such as `--allow-escapes` would be clearer, or perhaps not. `--c-style-escapes`? Anyhow, it's pure bikeshedding as I mentioned in my original email, so not a big deal. I brought it up only because the very first thought that popped into my head when reading the commit message saying it was adding `--printf` was "oh, interesting; how do I specify the arguments to interpolate into the `printf` format string?".
Eric Sunshine <sunshine@sunshineco.com> writes: > I don't care for `--raw` either but couldn't think of anything better > at the time. But perhaps a name such as `--allow-escapes` would be > clearer, or perhaps not. `--c-style-escapes`? It's printf(1) style escapes ;-) > Anyhow, it's pure bikeshedding as I mentioned in my original email, so > not a big deal. I brought it up only because the very first thought > that popped into my head when reading the commit message saying it was > adding `--printf` was "oh, interesting; how do I specify the arguments > to interpolate into the `printf` format string?".
On Tue, Apr 13 2021, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > >> I don't care for `--raw` either but couldn't think of anything better >> at the time. But perhaps a name such as `--allow-escapes` would be >> clearer, or perhaps not. `--c-style-escapes`? > > It's printf(1) style escapes ;-) > >> Anyhow, it's pure bikeshedding as I mentioned in my original email, so >> not a big deal. I brought it up only because the very first thought >> that popped into my head when reading the commit message saying it was >> adding `--printf` was "oh, interesting; how do I specify the arguments >> to interpolate into the `printf` format string?". So, the conclusion of this thread is let's keep it as --printf?
On Thu, Apr 15, 2021 at 7:33 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Tue, Apr 13 2021, Junio C Hamano wrote: > > Eric Sunshine <sunshine@sunshineco.com> writes: > >> I don't care for `--raw` either but couldn't think of anything better > >> at the time. But perhaps a name such as `--allow-escapes` would be > >> clearer, or perhaps not. `--c-style-escapes`? > > > > It's printf(1) style escapes ;-) > > > So, the conclusion of this thread is let's keep it as --printf? It was bikeshedding on my part, so I don't feel strongly. As mentioned, I only brought it up because my first thought was to wonder how interpolation would work. One might suggest --printf-escapes or --string-escapes but the audience for this is so narrow (Git developers) that the short and concise --printf is probably preferable.
Eric Sunshine <sunshine@sunshineco.com> writes: > On Thu, Apr 15, 2021 at 7:33 AM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> On Tue, Apr 13 2021, Junio C Hamano wrote: >> > Eric Sunshine <sunshine@sunshineco.com> writes: >> >> I don't care for `--raw` either but couldn't think of anything better >> >> at the time. But perhaps a name such as `--allow-escapes` would be >> >> clearer, or perhaps not. `--c-style-escapes`? >> > >> > It's printf(1) style escapes ;-) >> > >> So, the conclusion of this thread is let's keep it as --printf? > > It was bikeshedding on my part, so I don't feel strongly. As > mentioned, I only brought it up because my first thought was to wonder > how interpolation would work. One might suggest --printf-escapes or > --string-escapes but the audience for this is so narrow (Git > developers) that the short and concise --printf is probably > preferable. After seeing you raised the issue, I wish we had a better option name than --printf to be even more clear, but the wish is not strong enough to make me say "let's stop until we come up with the perfect name". I am OK with --printf Thanks.
diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh index 002e6d3388e..930dce06f0f 100755 --- a/t/t1307-config-blob.sh +++ b/t/t1307-config-blob.sh @@ -65,9 +65,7 @@ test_expect_success 'parse errors in blobs are properly attributed' ' ' test_expect_success 'can parse blob ending with CR' ' - printf "[some]key = value\\r" >config && - git add config && - git commit -m CR && + test_commit --printf CR config "[some]key = value\\r" && echo value >expect && git config --blob=HEAD:config some.key >actual && test_cmp expect actual diff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh index be6c84c52a2..f691e6d9032 100755 --- a/t/t2030-unresolve-info.sh +++ b/t/t2030-unresolve-info.sh @@ -179,8 +179,7 @@ test_expect_success 'rerere and rerere forget (subdirectory)' ' test_expect_success 'rerere forget (binary)' ' git checkout -f side && - printf "a\0c" >binary && - git commit -a -m binary && + test_commit --printf binary binary "a\0c" && test_must_fail git merge second && git rerere forget binary ' diff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh index 275ce5fa15b..6cdee2a2164 100755 --- a/t/t4006-diff-mode.sh +++ b/t/t4006-diff-mode.sh @@ -26,10 +26,8 @@ test_expect_success 'chmod' ' ' test_expect_success 'prepare binary file' ' - git commit -m rezrov && - printf "\00\01\02\03\04\05\06" >binbin && - git add binbin && - git commit -m binbin + git commit -m one && + test_commit --printf two binbin "\00\01\02\03\04\05\06" ' test_expect_success '--stat output after text chmod' ' diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh index c906320b60d..a39a626664d 100755 --- a/t/t4030-diff-textconv.sh +++ b/t/t4030-diff-textconv.sh @@ -26,12 +26,8 @@ EOF chmod +x hexdump test_expect_success 'setup binary file with history' ' - printf "\\0\\n" >file && - git add file && - git commit -m one && - printf "\\01\\n" >>file && - git add file && - git commit -m two + test_commit --printf one file "\\0\\n" && + test_commit --printf --append two file "\\01\\n" ' test_expect_success 'file is considered binary by porcelain' ' diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index a09411327f9..e2c0c510222 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -746,14 +746,8 @@ test_expect_success 'pull --rebase fails on corrupt HEAD' ' ' test_expect_success 'setup for detecting upstreamed changes' ' - mkdir src && - ( - cd src && - git init && - printf "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n" > stuff && - git add stuff && - git commit -m "Initial revision" - ) && + test_create_repo src && + test_commit -C src --printf one stuff "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n" && git clone src dst && ( cd src && diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index a0fcc383d0b..a90a6b2cc27 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -173,6 +173,10 @@ debug () { # Do not call test_tick before making a commit # --append # Use ">>" instead of ">" when writing "<contents>" to "<file>" +# --printf +# Use "printf" instead of "echo" when writing "<contents>" to +# "<file>". You will need to provide your own trailing "\n". You +# can only supply the FORMAT for the printf(1), not its ARGUMENT(s). # --signoff # Invoke "git commit" with --signoff # --author <author> @@ -192,6 +196,7 @@ debug () { test_commit () { notick= && + echo=echo && append= && author= && signoff= && @@ -203,6 +208,9 @@ test_commit () { --notick) notick=yes ;; + --printf) + echo=printf + ;; --append) append=yes ;; @@ -239,9 +247,9 @@ test_commit () { file=${2:-"$1.t"} && if test -n "$append" then - echo "${3-$1}" >>"$indir$file" + $echo "${3-$1}" >>"$indir$file" else - echo "${3-$1}" >"$indir$file" + $echo "${3-$1}" >"$indir$file" fi && git ${indir:+ -C "$indir"} add "$file" && if test -z "$notick"
Add a --printf option to test_commit to allow writing to the file with "printf" instead of "echo". This is useful for writing "\n", "\0" etc., in particular in combination with the --append option added in 3373518cc8 (test-lib functions: add an --append option to test_commit, 2021-01-12). I'm converting a few tests to use the new option rather than a manual printf/add/commit combination to demonstrate its usefulness. While I'm at it use "test_create_repo" where appropriate, and give the first/second commit a meaningful/more conventional log message in cases where no test cared about that message. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t1307-config-blob.sh | 4 +--- t/t2030-unresolve-info.sh | 3 +-- t/t4006-diff-mode.sh | 6 ++---- t/t4030-diff-textconv.sh | 8 ++------ t/t5520-pull.sh | 10 ++-------- t/test-lib-functions.sh | 12 ++++++++++-- 6 files changed, 18 insertions(+), 25 deletions(-)