| Message ID | xmqqwoeprsp1.fsf@gitster-ct.c.googlers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t: use LF variable defined in the test harness | expand |
Hi Junio, On Tue, Sep 03, 2019 at 02:11:22PM -0700, Junio C Hamano wrote: > A few test scripts assign a single LF to $LF, but that is already > given by test-lib.sh to everybody. I didn't know that 't/test-lib.sh' provided '$LF' (as I'm sure was the case for the respective authors of those tests below ;-)), so removing the bespoke definitions and relying on the one already defined makes good sense. > Remove the unnecessary reassignment. I did find a couple of extra spots when I went looking through 't' myself. I ran: $ git grep "='$" -- t which in addition to the three spots below turned up a couple more interesting locations, as well as a few false positives. They are: - t/t3005: this script calls the variable '$new_line', but could be renamed to LF and then removed in a second patch - t/valgrind/analyze.sh: this script also calls the variable '$new_line', but does not ever source test-lib.sh, so I think this spot can be ignored. So I think that my recommendation is to either: * introduce a prepratory patch that updates t3005 to rename '$new_line' to '$LF', and then amend the patch below to also blow away t3005's use of '$LF', or * do both steps in one larger patch > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > t/t3404-rebase-interactive.sh | 2 -- > t/t4013-diff-various.sh | 3 --- > t/t5515-fetch-merge-logic.sh | 3 --- > 3 files changed, 8 deletions(-) > > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 461dd539ff..31114d0bbb 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -155,8 +155,6 @@ test_expect_success 'rebase -x with empty command fails' ' > test_i18ncmp expected actual > ' > > -LF=' > -' > test_expect_success 'rebase -x with newline in command fails' ' > test_when_finished "git rebase --abort ||:" && > test_must_fail env git rebase -x "a${LF}b" @ 2>actual && > diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh > index a9054d2db1..5ac94b390d 100755 > --- a/t/t4013-diff-various.sh > +++ b/t/t4013-diff-various.sh > @@ -7,9 +7,6 @@ test_description='Various diff formatting options' > > . ./test-lib.sh > > -LF=' > -' > - > test_expect_success setup ' > > GIT_AUTHOR_DATE="2006-06-26 00:00:00 +0000" && > diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh > index e55d8474ef..961eb35c99 100755 > --- a/t/t5515-fetch-merge-logic.sh > +++ b/t/t5515-fetch-merge-logic.sh > @@ -12,9 +12,6 @@ GIT_TEST_PROTOCOL_VERSION= > > . ./test-lib.sh > > -LF=' > -' > - > test_expect_success setup ' > GIT_AUTHOR_DATE="2006-06-26 00:00:00 +0000" && > GIT_COMMITTER_DATE="2006-06-26 00:00:00 +0000" && The rest of the patch here looks obviously good. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > - t/t3005: this script calls the variable '$new_line', but could be > renamed to LF and then removed in a second patch It is worse than that, isn't it? If it used $new_line, then it would probably have been a good idea to somehow make a separate patch related to this one and make a series about "use $LF from test-lib", but ever since its beginning at 0f64bfa9 ("ls-files: fix pathspec display on error", 2011-08-01), $new_line is assigned once but never used in the script. Somebody may want to go clean-up the use of various $sq and $SQ locally defined by giving a unified $SQ in test-lib.sh, by the way. -- >8 -- Subject: t3005: remove unused variable Since the beginning of the script, $new_line variable was never used. Remove it. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- t/t3005-ls-files-relative.sh | 2 -- 1 file changed, 2 deletions(-) diff --git a/t/t3005-ls-files-relative.sh b/t/t3005-ls-files-relative.sh index 209b4c7cd8..583e467683 100755 --- a/t/t3005-ls-files-relative.sh +++ b/t/t3005-ls-files-relative.sh @@ -7,8 +7,6 @@ This test runs git ls-files with various relative path arguments. . ./test-lib.sh -new_line=' -' sq=\' test_expect_success 'prepare' '
On Thu, Sep 05, 2019 at 11:17:57AM -0700, Junio C Hamano wrote: > Somebody may want to go clean-up the use of various $sq and $SQ > locally defined by giving a unified $SQ in test-lib.sh, by the way. Maybe good #leftoverbits material, since we may have Outreachy applications coming up soon. -Peff
On Thu, Sep 05, 2019 at 11:17:57AM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > - t/t3005: this script calls the variable '$new_line', but could be > > renamed to LF and then removed in a second patch > > It is worse than that, isn't it? > > If it used $new_line, then it would probably have been a good idea > to somehow make a separate patch related to this one and make a > series about "use $LF from test-lib", but ever since its beginning > at 0f64bfa9 ("ls-files: fix pathspec display on error", 2011-08-01), > $new_line is assigned once but never used in the script. Ah, thanks for catching it ;-). I think the patch below the scissors is totally right; there's obviously no need for a preparatory patch here other than the one you provided. > Somebody may want to go clean-up the use of various $sq and $SQ > locally defined by giving a unified $SQ in test-lib.sh, by the way. > > -- >8 -- > Subject: t3005: remove unused variable > > Since the beginning of the script, $new_line variable was never used. > Remove it. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > t/t3005-ls-files-relative.sh | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/t/t3005-ls-files-relative.sh b/t/t3005-ls-files-relative.sh > index 209b4c7cd8..583e467683 100755 > --- a/t/t3005-ls-files-relative.sh > +++ b/t/t3005-ls-files-relative.sh > @@ -7,8 +7,6 @@ This test runs git ls-files with various relative path arguments. > > . ./test-lib.sh > > -new_line=' > -' > sq=\' > > test_expect_success 'prepare' ' Thanks, Taylor
Jeff King <peff@peff.net> writes: > On Thu, Sep 05, 2019 at 11:17:57AM -0700, Junio C Hamano wrote: > >> Somebody may want to go clean-up the use of various $sq and $SQ >> locally defined by giving a unified $SQ in test-lib.sh, by the way. > > Maybe good #leftoverbits material, since we may have Outreachy > applications coming up soon. OK, then I'd refrain from doing it as a lunchtime hack myself ;-) * Find sq=, $sq and ${sq} case insensitively in t/. If there is any use of $SQ that does not want a single quote in it, abort the whole thing. Otherwise proceed. * Introduce an assignment SQ=\' in t/test-lib.sh, next to where LF is assigned to. Replace all uses you found in #1 with reference to $SQ. #leftoverbits.
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 461dd539ff..31114d0bbb 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -155,8 +155,6 @@ test_expect_success 'rebase -x with empty command fails' ' test_i18ncmp expected actual ' -LF=' -' test_expect_success 'rebase -x with newline in command fails' ' test_when_finished "git rebase --abort ||:" && test_must_fail env git rebase -x "a${LF}b" @ 2>actual && diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index a9054d2db1..5ac94b390d 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -7,9 +7,6 @@ test_description='Various diff formatting options' . ./test-lib.sh -LF=' -' - test_expect_success setup ' GIT_AUTHOR_DATE="2006-06-26 00:00:00 +0000" && diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh index e55d8474ef..961eb35c99 100755 --- a/t/t5515-fetch-merge-logic.sh +++ b/t/t5515-fetch-merge-logic.sh @@ -12,9 +12,6 @@ GIT_TEST_PROTOCOL_VERSION= . ./test-lib.sh -LF=' -' - test_expect_success setup ' GIT_AUTHOR_DATE="2006-06-26 00:00:00 +0000" && GIT_COMMITTER_DATE="2006-06-26 00:00:00 +0000" &&
A few test scripts assign a single LF to $LF, but that is already given by test-lib.sh to everybody. Remove the unnecessary reassignment. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- t/t3404-rebase-interactive.sh | 2 -- t/t4013-diff-various.sh | 3 --- t/t5515-fetch-merge-logic.sh | 3 --- 3 files changed, 8 deletions(-)