| Message ID | 5c9217ad8fc594fbff46507c4be7961eb5a478e2.1587601501.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | shallow.c: reset shallow-ness after updating | expand |
Hi, Taylor Blau wrote: > A number of spots in t5537 use the non-indented heredoc '<<EOF' when > they would benefit from instead using '<<-EOF' or simply > test_write_lines. > > In preparation for adding new tests in a good style and being consistent > with the surrounding code, update the existing tests to improve their > readability. > > Suggested-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > t/t5537-fetch-shallow.sh | 70 +++++++++++----------------------------- > 1 file changed, 18 insertions(+), 52 deletions(-) Sounds like a good idea. Some nitpicks --- please don't act on them all, but only the ones that seem appropriate to you: [...] > +++ b/t/t5537-fetch-shallow.sh > @@ -25,10 +25,7 @@ test_expect_success 'setup' ' > test_expect_success 'setup shallow clone' ' > git clone --no-local --depth=2 .git shallow && > git --git-dir=shallow/.git log --format=%s >actual && > - cat <<EOF >expect && > -4 > -3 > -EOF > + test_write_lines 4 3 >expect && > test_cmp expect actual > ' Nice. [...] > @@ -133,14 +110,12 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' ' [...] > - cat <<EOF >expect && > -no-shallow > -EOF > + cat <<-EOF >expect && > + no-shallow > + EOF Can this use "echo"? Or if using cat, please quote the EOF in <<-EOF so the reader doesn't have to check for $substitutions in the body: cat >expect <<-\EOF && [...] > @@ -158,21 +133,15 @@ test_expect_success 'fetch --update-shallow' ' > git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* && > git fsck && > git for-each-ref --sort=refname --format="%(refname)" >actual.refs && > - cat <<EOF >expect.refs && > -refs/remotes/shallow/master > -refs/remotes/shallow/no-shallow > -refs/tags/heavy-tag > -refs/tags/light-tag > -EOF > + cat <<-EOF >expect.refs && Likewise (missing \ before EOF). A few more nits, that probably don't belong in the same patch: - the code in subshells would be more readable if indented - existing <<-EOF here blocks should \quote the EOF - the resulting history would be more realistic if it uses test_tick before running "git commit". Or perhaps this can use the test_commit helper to handle that - should use test_must_fail in preference to ! git - might be simpler if http tests go in a different file Thanks, Jonathan
On Wed, Apr 22, 2020 at 06:14:44PM -0700, Jonathan Nieder wrote: > Hi, > > Taylor Blau wrote: > > > A number of spots in t5537 use the non-indented heredoc '<<EOF' when > > they would benefit from instead using '<<-EOF' or simply > > test_write_lines. > > > > In preparation for adding new tests in a good style and being consistent > > with the surrounding code, update the existing tests to improve their > > readability. > > > > Suggested-by: Junio C Hamano <gitster@pobox.com> > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > > --- > > t/t5537-fetch-shallow.sh | 70 +++++++++++----------------------------- > > 1 file changed, 18 insertions(+), 52 deletions(-) > > Sounds like a good idea. Some nitpicks --- please don't act on them > all, but only the ones that seem appropriate to you: Thanks -- here's an updated venison of this patch with some of the more minor points addressed. I agree that there is room for cleaning up these tests more substantially, but I think that those can be done in other patches. Junio -- would you please used the patch below as PATCH 1/2 instead of the original here? Thanks. -- 8< -- Subject: [PATCH] t5537: use test_write_lines, indented heredocs for readability A number of spots in t5537 use the non-indented heredoc '<<EOF' when they would benefit from instead using '<<-EOF' or simply test_write_lines. In preparation for adding new tests in a good style and being consistent with the surrounding code, update the existing tests to improve their readability. Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t5537-fetch-shallow.sh | 70 ++++++++++------------------------------ 1 file changed, 17 insertions(+), 53 deletions(-) diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index 4f681dbbe1..d02f9b5ae8 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -16,7 +16,7 @@ test_expect_success 'setup' ' commit 3 && commit 4 && git config --global transfer.fsckObjects true && - test_oid_cache <<-EOF + test_oid_cache <<-\EOF perl sha1:s/0034shallow %s/0036unshallow %s/ perl sha256:s/004cshallow %s/004eunshallow %s/ EOF @@ -25,10 +25,7 @@ test_expect_success 'setup' ' test_expect_success 'setup shallow clone' ' git clone --no-local --depth=2 .git shallow && git --git-dir=shallow/.git log --format=%s >actual && - cat <<EOF >expect && -4 -3 -EOF + test_write_lines 4 3 >expect && test_cmp expect actual ' @@ -38,10 +35,7 @@ test_expect_success 'clone from shallow clone' ' cd shallow2 && git fsck && git log --format=%s >actual && - cat <<EOF >expect && -4 -3 -EOF + test_write_lines 4 3 >expect && test_cmp expect actual ) ' @@ -56,11 +50,7 @@ test_expect_success 'fetch from shallow clone' ' git fetch && git fsck && git log --format=%s origin/master >actual && - cat <<EOF >expect && -5 -4 -3 -EOF + test_write_lines 5 4 3 >expect && test_cmp expect actual ) ' @@ -75,10 +65,7 @@ test_expect_success 'fetch --depth from shallow clone' ' git fetch --depth=2 && git fsck && git log --format=%s origin/master >actual && - cat <<EOF >expect && -6 -5 -EOF + test_write_lines 6 5 >expect && test_cmp expect actual ) ' @@ -89,12 +76,7 @@ test_expect_success 'fetch --unshallow from shallow clone' ' git fetch --unshallow && git fsck && git log --format=%s origin/master >actual && - cat <<EOF >expect && -6 -5 -4 -3 -EOF + test_write_lines 6 5 4 3 >expect && test_cmp expect actual ) ' @@ -111,15 +93,10 @@ test_expect_success 'fetch something upstream has but hidden by clients shallow git fetch ../.git +refs/heads/master:refs/remotes/top/master && git fsck && git log --format=%s top/master >actual && - cat <<EOF >expect && -add-1-back -4 -3 -EOF + test_write_lines add-1-back 4 3 >expect && test_cmp expect actual ) && git --git-dir=shallow2/.git cat-file blob $(echo 1|git hash-object --stdin) >/dev/null - ' test_expect_success 'fetch that requires changes in .git/shallow is filtered' ' @@ -133,14 +110,10 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' ' cd notshallow && git fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/*&& git for-each-ref --format="%(refname)" >actual.refs && - cat <<EOF >expect.refs && -refs/remotes/shallow/no-shallow -EOF + echo refs/remotes/shallow/no-shallow >expect.refs && test_cmp expect.refs actual.refs && git log --format=%s shallow/no-shallow >actual && - cat <<EOF >expect && -no-shallow -EOF + echo no-shallow >expect && test_cmp expect actual ) ' @@ -158,21 +131,15 @@ test_expect_success 'fetch --update-shallow' ' git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* && git fsck && git for-each-ref --sort=refname --format="%(refname)" >actual.refs && - cat <<EOF >expect.refs && -refs/remotes/shallow/master -refs/remotes/shallow/no-shallow -refs/tags/heavy-tag -refs/tags/light-tag -EOF + cat <<-\EOF >expect.refs && + refs/remotes/shallow/master + refs/remotes/shallow/no-shallow + refs/tags/heavy-tag + refs/tags/light-tag + EOF test_cmp expect.refs actual.refs && git log --format=%s shallow/master >actual && - cat <<EOF >expect && -7 -6 -5 -4 -3 -EOF + test_write_lines 7 6 5 4 3 >expect && test_cmp expect actual ) ' @@ -183,10 +150,7 @@ test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' ' find read-only.git -print | xargs chmod -w && git clone --no-local --depth=2 read-only.git from-read-only && git --git-dir=from-read-only/.git log --format=%s >actual && - cat >expect <<EOF && -add-1-back -4 -EOF + test_write_lines add-1-back 4 >expect && test_cmp expect actual ' -- 2.26.0.113.ge9739cdccc
Taylor Blau wrote: > Subject: [PATCH] t5537: use test_write_lines, indented heredocs for readability > > A number of spots in t5537 use the non-indented heredoc '<<EOF' when > they would benefit from instead using '<<-EOF' or simply > test_write_lines. > > In preparation for adding new tests in a good style and being consistent > with the surrounding code, update the existing tests to improve their > readability. > > Suggested-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > t/t5537-fetch-shallow.sh | 70 ++++++++++------------------------------ > 1 file changed, 17 insertions(+), 53 deletions(-) The commit message is missing the mention of \EOF quoting. See "git log --grep='modernize style'" for some examples about how to write this kind of commit message without it getting tedious. That said, I think it's fine --- just mentioning that for the future. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Taylor Blau <me@ttaylorr.com> writes: > Subject: [PATCH] t5537: use test_write_lines, indented heredocs for readability Assuming that "indented" is not a verb in the past tense, but adjective on the noun "heredocs", this reads well, but "s/,/ and/" would make it more clear in that case. Or "for readability, use X and indent Y" would work equally well. But the original is probably OK as-is. > - cat <<EOF >expect && > -4 > -3 > -EOF > + test_write_lines 4 3 >expect && Yeah, becomes much nicer with test_write_lines. Thanks.
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index 4f681dbbe1..d66b3656c0 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -25,10 +25,7 @@ test_expect_success 'setup' ' test_expect_success 'setup shallow clone' ' git clone --no-local --depth=2 .git shallow && git --git-dir=shallow/.git log --format=%s >actual && - cat <<EOF >expect && -4 -3 -EOF + test_write_lines 4 3 >expect && test_cmp expect actual ' @@ -38,10 +35,7 @@ test_expect_success 'clone from shallow clone' ' cd shallow2 && git fsck && git log --format=%s >actual && - cat <<EOF >expect && -4 -3 -EOF + test_write_lines 4 3 >expect && test_cmp expect actual ) ' @@ -56,11 +50,7 @@ test_expect_success 'fetch from shallow clone' ' git fetch && git fsck && git log --format=%s origin/master >actual && - cat <<EOF >expect && -5 -4 -3 -EOF + test_write_lines 5 4 3 >expect && test_cmp expect actual ) ' @@ -75,10 +65,7 @@ test_expect_success 'fetch --depth from shallow clone' ' git fetch --depth=2 && git fsck && git log --format=%s origin/master >actual && - cat <<EOF >expect && -6 -5 -EOF + test_write_lines 6 5 >expect && test_cmp expect actual ) ' @@ -89,12 +76,7 @@ test_expect_success 'fetch --unshallow from shallow clone' ' git fetch --unshallow && git fsck && git log --format=%s origin/master >actual && - cat <<EOF >expect && -6 -5 -4 -3 -EOF + test_write_lines 6 5 4 3 >expect && test_cmp expect actual ) ' @@ -111,15 +93,10 @@ test_expect_success 'fetch something upstream has but hidden by clients shallow git fetch ../.git +refs/heads/master:refs/remotes/top/master && git fsck && git log --format=%s top/master >actual && - cat <<EOF >expect && -add-1-back -4 -3 -EOF + test_write_lines add-1-back 4 3 >expect && test_cmp expect actual ) && git --git-dir=shallow2/.git cat-file blob $(echo 1|git hash-object --stdin) >/dev/null - ' test_expect_success 'fetch that requires changes in .git/shallow is filtered' ' @@ -133,14 +110,12 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' ' cd notshallow && git fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/*&& git for-each-ref --format="%(refname)" >actual.refs && - cat <<EOF >expect.refs && -refs/remotes/shallow/no-shallow -EOF + echo refs/remotes/shallow/no-shallow >expect.refs && test_cmp expect.refs actual.refs && git log --format=%s shallow/no-shallow >actual && - cat <<EOF >expect && -no-shallow -EOF + cat <<-EOF >expect && + no-shallow + EOF test_cmp expect actual ) ' @@ -158,21 +133,15 @@ test_expect_success 'fetch --update-shallow' ' git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* && git fsck && git for-each-ref --sort=refname --format="%(refname)" >actual.refs && - cat <<EOF >expect.refs && -refs/remotes/shallow/master -refs/remotes/shallow/no-shallow -refs/tags/heavy-tag -refs/tags/light-tag -EOF + cat <<-EOF >expect.refs && + refs/remotes/shallow/master + refs/remotes/shallow/no-shallow + refs/tags/heavy-tag + refs/tags/light-tag + EOF test_cmp expect.refs actual.refs && git log --format=%s shallow/master >actual && - cat <<EOF >expect && -7 -6 -5 -4 -3 -EOF + test_write_lines 7 6 5 4 3 >expect && test_cmp expect actual ) ' @@ -183,10 +152,7 @@ test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' ' find read-only.git -print | xargs chmod -w && git clone --no-local --depth=2 read-only.git from-read-only && git --git-dir=from-read-only/.git log --format=%s >actual && - cat >expect <<EOF && -add-1-back -4 -EOF + test_write_lines add-1-back 4 >expect && test_cmp expect actual '
A number of spots in t5537 use the non-indented heredoc '<<EOF' when they would benefit from instead using '<<-EOF' or simply test_write_lines. In preparation for adding new tests in a good style and being consistent with the surrounding code, update the existing tests to improve their readability. Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t5537-fetch-shallow.sh | 70 +++++++++++----------------------------- 1 file changed, 18 insertions(+), 52 deletions(-)