| Message ID | 20230206211823.8651-4-cheskaqiqi@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t4113: modernize style | expand |
On Mon, Feb 06 2023, Shuqi Liang wrote: . ./test-lib.sh > > -# setup > - > -cat >test-patch <<\EOF > -diff --git a/file b/file > ---- a/file > -+++ b/file > -@@ -1,2 +1,3 @@ > - a > - b > -+c > -EOF > - > -echo 'a' >file > -echo 'b' >>file > -echo 'c' >>file > - > test_expect_success setup ' > + cat >test-patch <<-\EOF > + diff --git a/file b/file > + --- a/file > + +++ b/file > + @@ -1,2 +1,3 @@ > + a > + b > + +c > + EOF > + > + echo 'a' >file > + echo 'b' >>file > + echo 'c' >>file I have not read the rest here, but this immediately fails with a very large error from chain-lint by default, and even if you manually disable it (which I assume you're doing, or just not testing these at all before submission), you'll get: $ ./t4113-apply-ending.sh --no-chain-lint ok 1 - setup ok 2 - apply at the end ok 3 - apply at the beginning ./t4113-apply-ending.sh: 44: b: not found ./t4113-apply-ending.sh: 48: c git update-index file test_must_fail git apply --index test-patch : not found # passed all 3 test(s) 1..3 Which shows that even with the &&-chaining fixed you have quoting issues here, you're trying to execute 'b' etc. I didn't read the rest of this topic, but please test with chain-lint, see if there's any unexpected new output from the tests etc. before a v5 re-roll.
Shuqi Liang <cheskaqiqi@gmail.com> writes: > -cat >test-patch <<\EOF > -diff --git a/file b/file > ---- a/file > -+++ b/file > -@@ -1,2 +1,3 @@ > - a > - b > -+c > -EOF > - > -echo 'a' >file > -echo 'b' >>file > -echo 'c' >>file Have you run the resulting test? > test_expect_success setup ' > + cat >test-patch <<-\EOF > + diff --git a/file b/file > + --- a/file > + +++ b/file > + @@ -1,2 +1,3 @@ > + a > + b > + +c > + EOF This creates a "test-patch" file with lines 'a' and 'b' that are common context lines without any whitespace before them, no? The original left the necessary single space in front of them (see the line removed above).
On Mon, Feb 6, 2023 at 5:44 PM Junio C Hamano <gitster@pobox.com> wrote: > Have you run the resulting test? My apologies for not testing after V1. That was a major oversight on my part. I'll make sure to thoroughly test before each submission to avoid any issues with the code in the future. > This creates a "test-patch" file with lines 'a' and 'b' that are > common context lines without any whitespace before them, no? The > original left the necessary single space in front of them (see the > line removed above). I try to change the code to(left the necessary single space in front of 'a' and 'b': diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh index ab5ecaab7f..ef61a3187c 100755 --- a/t/t4113-apply-ending.sh +++ b/t/t4113-apply-ending.sh @@ -14,8 +14,8 @@ test_expect_success setup ' --- a/file +++ b/file @@ -1,2 +1,3 @@ - a - b + a + b +c EOF Here I only show one part ,but I fix two same issue in the V4 patch and it still can not pass the test . It say : Test Summary Report ------------------- t4113-apply-ending.sh (Wstat: 256 Tests: 0 Failed: 0) Non-zero exit status: 1 Parse errors: No plan found in TAP output Files=1, Tests=0, 0 wallclock secs ( 0.01 usr 0.01 sys + 0.05 cusr 0.02 csys = 0.09 CPU) Result: FAIL. I'm stumped as to why it's still failing. I've tried searching for answers on StackOverflow, but I still can't figure it out. ---------------- Thanks, Shuqi
On Mon, Feb 06 2023, Shuqi Liang wrote: > On Mon, Feb 6, 2023 at 5:44 PM Junio C Hamano <gitster@pobox.com> wrote: > >> Have you run the resulting test? > > My apologies for not testing after V1. That was a major oversight on > my part. I'll make sure to thoroughly test before each submission to > avoid any issues with the code in the future. > > >> This creates a "test-patch" file with lines 'a' and 'b' that are >> common context lines without any whitespace before them, no? The >> original left the necessary single space in front of them (see the >> line removed above). > > I try to change the code to(left the necessary single space in front > of 'a' and 'b': > > diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh > index ab5ecaab7f..ef61a3187c 100755 > --- a/t/t4113-apply-ending.sh > +++ b/t/t4113-apply-ending.sh > @@ -14,8 +14,8 @@ test_expect_success setup ' > --- a/file > +++ b/file > @@ -1,2 +1,3 @@ > - a > - b > + a > + b > +c > EOF > > Here I only show one part ,but I fix two same issue in the V4 patch > and it still can not pass the test . > It say : > > Test Summary Report > > ------------------- > > t4113-apply-ending.sh (Wstat: 256 Tests: 0 Failed: 0) > > Non-zero exit status: 1 > > Parse errors: No plan found in TAP output > > Files=1, Tests=0, 0 wallclock secs ( 0.01 usr 0.01 sys + 0.05 cusr > 0.02 csys = 0.09 CPU) > > Result: FAIL. > > I'm stumped as to why it's still failing. I've tried searching for > answers on StackOverflow, but I still can't figure it out. > ---------------- > Thanks, > Shuqi The error doesn't tell us much, instead of "make prove" or "prove <name>" running e.g.: ./t4113-apply-ending.sh -vixd Gives you better output. But this is almost certainly that you're trying to insert leading whitespace into a line that's in a <<-EOF here-doc, the "-" part of that means that your leading whitespace is being stripped. A typical idiom for that is have a marker for the start of line, and strip the whitespace with "sed". See this for existing examples: git grep 'sed.*\^.*>.*EOF'
Hi Ævar, On Tue, Feb 7, 2023 at 3:12 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > The error doesn't tell us much, instead of "make prove" or "prove > <name>" running e.g.: > > ./t4113-apply-ending.sh -vixd > > Gives you better output. Thanks, this is really helpful. > But this is almost certainly that you're trying to insert leading > whitespace into a line that's in a <<-EOF here-doc, the "-" part of that > means that your leading whitespace is being stripped. > > A typical idiom for that is have a marker for the start of line, and > strip the whitespace with "sed". See this for existing examples: > > git grep 'sed.*\^.*>.*EOF' Thank you for the tip! I'll try to fix the problem as soon as possible. --------- Thanks, Shuqi
Hi Ævar, On Tue, Feb 7, 2023 at 3:12 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > But this is almost certainly that you're trying to insert leading > whitespace into a line that's in a <<-EOF here-doc, the "-" part of that > means that your leading whitespace is being stripped. > > A typical idiom for that is have a marker for the start of line, and > strip the whitespace with "sed". See this for existing examples: > > git grep 'sed.*\^.*>.*EOF' I try to use Z as the marker in front of 'a' and 'b' and use sed -e "s/Z/ /g" in order to replace Z with white space but it still can not pass the test. Then I realize even if I don't add tab in front of the line but with space in front of 'a' and 'b' like the original test script. It still says it can't read "b" and "c” ： test_expect_success 'apply at the beginning' ' cat >test-patch<<\EOF && diff a/file b/file --- a/file +++ b/file @@ -1,2 +1,3 @@ +a b c EOF echo >file 'a b c'&& git update-index file&& test_must_fail git apply --index test-patch ' Maybe the error is not caused by whitespace? Then I try to change: echo >file 'a b c' To: echo >file "a b c" Then everything passes the test. I think double quotes allow for variable substitution and command substitution, while single quotes preserve the literal value of all characters within the quotes. In this case, the string contains no variables or commands, so either type of quote would work. Is there something wrong with my idea? Is it good to modify code like that? Looking forward to your reply! ------ Shuqi
On Wed, Feb 08 2023, Shuqi Liang wrote: > On Tue, Feb 7, 2023 at 3:12 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > >> But this is almost certainly that you're trying to insert leading >> whitespace into a line that's in a <<-EOF here-doc, the "-" part of that >> means that your leading whitespace is being stripped. >> >> A typical idiom for that is have a marker for the start of line, and >> strip the whitespace with "sed". See this for existing examples: >> >> git grep 'sed.*\^.*>.*EOF' > > > I try to use Z as the marker in front of 'a' and 'b' and use sed -e > "s/Z/ /g" in order to replace Z with white space but it still can not > pass the test. > > Then I realize even if I don't add tab in front of the line but with > space in front of 'a' and 'b' like the original test script. It still > says it can't read "b" and "c” ： > > test_expect_success 'apply at the beginning' ' > cat >test-patch<<\EOF && > diff a/file b/file > --- a/file > +++ b/file > @@ -1,2 +1,3 @@ > +a > b > c > EOF > > echo >file 'a > b > c'&& > git update-index file&& > test_must_fail git apply --index test-patch > ' > Maybe the error is not caused by whitespace? > > Then I try to change: > > echo >file 'a > b > c' > > To: > echo >file "a > b > c" > > Then everything passes the test. I think double quotes allow for > variable substitution and command substitution, while single quotes > preserve the literal value of all characters within the quotes. In > this case, the string contains no variables or commands, so either > type of quote would work. Is there something wrong with my idea? Is it > good to modify code like that? > > Looking forward to your reply! I'm not sure because you're just posting snippets, if you have problems in the future it would be best to post the full diff to "master" that you're having issues with, e.g. an RFC per Documentation/SubmittingPatches. But I think this is because the test itself is using '-quotes, so you need to use '\'' if you want to single quote, and " for double quotes, and \" if the test were in double quotes. But the issues you're having here aren't with Git, but the very basics of POSIX shell syntax. I think it would be good for you to read some basic documentation on POSIX shells, their syntax, common POSIX commands etc. Your local "man sh" is probably a good place to start, but there's also books, online tutorials etc. In this case the syntax you're trying to get working is something we usually try to avoid in either case, i.e. even if it involves an external process we usually do: cat >out <<-\EOF a b c EOF Rather than: echo "a b c" >out If you are using "echo" I saw another change of yours had e.g.: echo x >f && echo y >>f && echo z >>f It's better to e.g. (assuming use of "echo", or other built-ins or commands): { echo x && echo y && echo z } >f
Hi Ævar , Sorry if I sent this email twice. I forget to CC git@vger.kernel.org. On Wed, Feb 8, 2023 at 2:50 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > I'm not sure because you're just posting snippets, if you have problems > in the future it would be best to post the full diff to "master" that > you're having issues with, e.g. an RFC per Documentation/SubmittingPatches. Yeah, posting snippets doesn't describe the problem very well. Thanks for the advice. > But I think this is because the test itself is using '-quotes, so you > need to use '\'' if you want to single quote, and " for double quotes, > and \" if the test were in double quotes. > But the issues you're having here aren't with Git, but the very basics > of POSIX shell syntax. > I think it would be good for you to read some basic documentation on > POSIX shells, their syntax, common POSIX commands etc. Your local "man > sh" is probably a good place to start, but there's also books, online > tutorials etc. Thanks, will do . I'll try to avoid making mistakes on POSIX shells questions and really learn the basic POSIX shells syntax. > In this case the syntax you're trying to get working is something we > usually try to avoid in either case, i.e. even if it involves an > external process we usually do: > cat >out <<-\EOF > a > b > c > EOF > > Rather than: > > echo "a > b > c" >out > > If you are using "echo" I saw another change of yours had e.g.: > > echo x >f && > echo y >>f && > echo z >>f > > It's better to e.g. (assuming use of "echo", or other built-ins or > commands): > > { > echo x && > echo y && > echo z > } >f Seen I pass the test s, I'v a submit V5 patch instead of RFC, Would you mind taking a look at this for me? Looking forward to reply. ----------------------------- Thanks Shuqi On Wed, Feb 8, 2023 at 2:50 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Wed, Feb 08 2023, Shuqi Liang wrote: > > > On Tue, Feb 7, 2023 at 3:12 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > >> But this is almost certainly that you're trying to insert leading > >> whitespace into a line that's in a <<-EOF here-doc, the "-" part of that > >> means that your leading whitespace is being stripped. > >> > >> A typical idiom for that is have a marker for the start of line, and > >> strip the whitespace with "sed". See this for existing examples: > >> > >> git grep 'sed.*\^.*>.*EOF' > > > > > > I try to use Z as the marker in front of 'a' and 'b' and use sed -e > > "s/Z/ /g" in order to replace Z with white space but it still can not > > pass the test. > > > > Then I realize even if I don't add tab in front of the line but with > > space in front of 'a' and 'b' like the original test script. It still > > says it can't read "b" and "c” ： > > > > test_expect_success 'apply at the beginning' ' > > cat >test-patch<<\EOF && > > diff a/file b/file > > --- a/file > > +++ b/file > > @@ -1,2 +1,3 @@ > > +a > > b > > c > > EOF > > > > echo >file 'a > > b > > c'&& > > git update-index file&& > > test_must_fail git apply --index test-patch > > ' > > Maybe the error is not caused by whitespace? > > > > Then I try to change: > > > > echo >file 'a > > b > > c' > > > > To: > > echo >file "a > > b > > c" > > > > Then everything passes the test. I think double quotes allow for > > variable substitution and command substitution, while single quotes > > preserve the literal value of all characters within the quotes. In > > this case, the string contains no variables or commands, so either > > type of quote would work. Is there something wrong with my idea? Is it > > good to modify code like that? > > > > Looking forward to your reply! > > I'm not sure because you're just posting snippets, if you have problems > in the future it would be best to post the full diff to "master" that > you're having issues with, e.g. an RFC per Documentation/SubmittingPatches. > > But I think this is because the test itself is using '-quotes, so you > need to use '\'' if you want to single quote, and " for double quotes, > and \" if the test were in double quotes. > > But the issues you're having here aren't with Git, but the very basics > of POSIX shell syntax. > > I think it would be good for you to read some basic documentation on > POSIX shells, their syntax, common POSIX commands etc. Your local "man > sh" is probably a good place to start, but there's also books, online > tutorials etc. > > In this case the syntax you're trying to get working is something we > usually try to avoid in either case, i.e. even if it involves an > external process we usually do: > > cat >out <<-\EOF > a > b > c > EOF > > Rather than: > > echo "a > b > c" >out > > If you are using "echo" I saw another change of yours had e.g.: > > echo x >f && > echo y >>f && > echo z >>f > > It's better to e.g. (assuming use of "echo", or other built-ins or > commands): > > { > echo x && > echo y && > echo z > } >f
Hi Junio, I didn't see this change in " What's cooking in git.git". I'm not sure if the V5 patch is overlooked. I didn't receive any review after V5. Is there anything wrong in V5 that needs to be corrected? Thanks, Shuqi
Shuqi Liang <cheskaqiqi@gmail.com> writes: > I didn't see this change in " What's cooking in git.git". I'm not sure > if the V5 patch is overlooked. I didn't receive any review after V5. > Is there anything wrong in V5 that needs to be corrected? I dunno (yet). These days a day did not have enough time to be looking at all the patches on the list, and patches that are more about practice than fixing real bugs or adding real features tend to be placed on the back burner. THanks for pinging.
On Tue, Feb 7, 2023 at 3:19 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Mon, Feb 06 2023, Shuqi Liang wrote: > > On Mon, Feb 6, 2023 at 5:44 PM Junio C Hamano <gitster@pobox.com> wrote: > >> This creates a "test-patch" file with lines 'a' and 'b' that are > >> common context lines without any whitespace before them, no? The > >> original left the necessary single space in front of them (see the > >> line removed above). > > > > I try to change the code to(left the necessary single space in front > > of 'a' and 'b': > > > > @@ -1,2 +1,3 @@ > > - a > > - b > > + a > > + b > > +c > > EOF > > > > t4113-apply-ending.sh (Wstat: 256 Tests: 0 Failed: 0) > > Result: FAIL. > > > > I'm stumped as to why it's still failing. I've tried searching for > > answers on StackOverflow, but I still can't figure it out. > > But this is almost certainly that you're trying to insert leading > whitespace into a line that's in a <<-EOF here-doc, the "-" part of that > means that your leading whitespace is being stripped. Almost. The `<<-` operator actually only strips leading TABs; other whitespace following the TABs is left intact.
On Wed, Feb 8, 2023 at 2:57 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > In this case the syntax you're trying to get working is something we > usually try to avoid in either case, i.e. even if it involves an > external process we usually do: > > cat >out <<-\EOF > a > b > c > EOF > > Rather than: > > echo "a > b > c" >out The here-doc (<<-\EOF) form is definitely a good idea when the code is part of an indented test body, whereas the multi-line double-quoted string will be problematic since lines "b" and "c" will be indented with TAB, which is undesirable here. Even better for such a simple case would be: test_write_lines a b c >out && In fact, Junio made this suggestion in the form of a code snipped much earlier in this thread. > If you are using "echo" I saw another change of yours had e.g.: > > echo x >f && > echo y >>f && > echo z >>f > > It's better to e.g. (assuming use of "echo", or other built-ins or > commands): > > { > echo x && > echo y && > echo z > } >f This is also an improvement, though test_write_lines would (again) be even better for such a simple case: test_write_lines x y z >f &&
diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh index 5ee177e8eb..ab5ecaab7f 100755 --- a/t/t4113-apply-ending.sh +++ b/t/t4113-apply-ending.sh @@ -8,47 +8,42 @@ test_description='git apply trying to add an ending line. ' . ./test-lib.sh -# setup - -cat >test-patch <<\EOF -diff --git a/file b/file ---- a/file -+++ b/file -@@ -1,2 +1,3 @@ - a - b -+c -EOF - -echo 'a' >file -echo 'b' >>file -echo 'c' >>file - test_expect_success setup ' + cat >test-patch <<-\EOF + diff --git a/file b/file + --- a/file + +++ b/file + @@ -1,2 +1,3 @@ + a + b + +c + EOF + + echo 'a' >file + echo 'b' >>file + echo 'c' >>file git update-index --add file ' -# test test_expect_success 'apply at the end' ' test_must_fail git apply --index test-patch ' -cat >test-patch <<\EOF -diff a/file b/file ---- a/file -+++ b/file -@@ -1,2 +1,3 @@ -+a - b - c -EOF - -echo >file 'a -b -c' -git update-index file - test_expect_success 'apply at the beginning' ' + cat >test-patch <<-\EOF + diff a/file b/file + --- a/file + +++ b/file + @@ -1,2 +1,3 @@ + +a + b + c + EOF + + echo >file 'a + b + c' + git update-index file test_must_fail git apply --index test-patch '
This script is written in old style,where there are some executable lines outside test_expect_success. Put the executable lines inside the test_expect_success. As t/README says,use "<<-" instead of "<<" to strip leading TABs used for indentation. Change the "<<" to "<<-" for example: -cat >test-patch <<\EOF -diff a/file b/file test_expect_success 'apply at the beginning' ' + cat >test-patch <<-\EOF + diff a/file b/file + --- a/file Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com> --- t/t4113-apply-ending.sh | 59 +++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 32 deletions(-)