| Message ID | 20240304095436.56399-2-shejialuo@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0332e813d6b4a9413684830f3ac02715633dc544 |
| Headers | show |
| Series | Change commit message | expand |
On Mon, Mar 04, 2024 at 05:54:36PM +0800, shejialuo wrote: > test -(e|d) does not provide a nice error message when we hit test > failures, so use test_path_exists, test_path_is_dir instead. > > Signed-off-by: shejialuo <shejialuo@gmail.com> This version looks good to me, thanks! One suggestion for potential future contributions by you: it's always helpful to create a "range-diff" of what has changed between the previous version of your patch series and the next one. Like this, reviewers can immediately see what the difference is between the two versions, which helps them to get the review done faster. Assuming you use git-format-patch(1) you can generate such a range diff with the `--range-diff=` parameter. Patrick > --- > t/t9117-git-svn-init-clone.sh | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh > index 62de819a44..3b038c338f 100755 > --- a/t/t9117-git-svn-init-clone.sh > +++ b/t/t9117-git-svn-init-clone.sh > @@ -17,32 +17,32 @@ test_expect_success 'setup svnrepo' ' > test_expect_success 'basic clone' ' > test ! -d trunk && > git svn clone "$svnrepo"/project/trunk && > - test -d trunk/.git/svn && > - test -e trunk/foo && > + test_path_is_dir trunk/.git/svn && > + test_path_exists trunk/foo && > rm -rf trunk > ' > > test_expect_success 'clone to target directory' ' > test ! -d target && > git svn clone "$svnrepo"/project/trunk target && > - test -d target/.git/svn && > - test -e target/foo && > + test_path_is_dir target/.git/svn && > + test_path_exists target/foo && > rm -rf target > ' > > test_expect_success 'clone with --stdlayout' ' > test ! -d project && > git svn clone -s "$svnrepo"/project && > - test -d project/.git/svn && > - test -e project/foo && > + test_path_is_dir project/.git/svn && > + test_path_exists project/foo && > rm -rf project > ' > > test_expect_success 'clone to target directory with --stdlayout' ' > test ! -d target && > git svn clone -s "$svnrepo"/project target && > - test -d target/.git/svn && > - test -e target/foo && > + test_path_is_dir target/.git/svn && > + test_path_exists target/foo && > rm -rf target > ' > > -- > 2.44.0 >
Thanks for your advice. I will remember this suggestion.
shejialuo <shejialuo@gmail.com> writes: > test -(e|d) does not provide a nice error message when we hit test > failures, so use test_path_exists, test_path_is_dir instead. OK. > > Signed-off-by: shejialuo <shejialuo@gmail.com> > --- Just for the next single-patch topic you'd work on, here below the three-dash line is where you may mention what's different between the previous iteration and this one, if you wanted to, instead of having a separate cover-letter message. > t/t9117-git-svn-init-clone.sh | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) The patch looks good to me. Thanks (and thanks for all the reviewers of the previous rounds).
Patrick Steinhardt <ps@pks.im> writes: > This version looks good to me, thanks! > > One suggestion for potential future contributions by you: it's always > helpful to create a "range-diff" of what has changed between the > previous version of your patch series and the next one. Like this, > reviewers can immediately see what the difference is between the two > versions, which helps them to get the review done faster. > > Assuming you use git-format-patch(1) you can generate such a range diff > with the `--range-diff=` parameter. Thanks for a review.
> Just for the next single-patch topic you'd work on, here below the > three-dash line is where you may mention what's different between > the previous iteration and this one, if you wanted to, instead of > having a separate cover-letter message. Thanks for your suggestions. At last, Thank every reviewer for your dedicated comments which make me learn a lot.
diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh index 62de819a44..3b038c338f 100755 --- a/t/t9117-git-svn-init-clone.sh +++ b/t/t9117-git-svn-init-clone.sh @@ -17,32 +17,32 @@ test_expect_success 'setup svnrepo' ' test_expect_success 'basic clone' ' test ! -d trunk && git svn clone "$svnrepo"/project/trunk && - test -d trunk/.git/svn && - test -e trunk/foo && + test_path_is_dir trunk/.git/svn && + test_path_exists trunk/foo && rm -rf trunk ' test_expect_success 'clone to target directory' ' test ! -d target && git svn clone "$svnrepo"/project/trunk target && - test -d target/.git/svn && - test -e target/foo && + test_path_is_dir target/.git/svn && + test_path_exists target/foo && rm -rf target ' test_expect_success 'clone with --stdlayout' ' test ! -d project && git svn clone -s "$svnrepo"/project && - test -d project/.git/svn && - test -e project/foo && + test_path_is_dir project/.git/svn && + test_path_exists project/foo && rm -rf project ' test_expect_success 'clone to target directory with --stdlayout' ' test ! -d target && git svn clone -s "$svnrepo"/project target && - test -d target/.git/svn && - test -e target/foo && + test_path_is_dir target/.git/svn && + test_path_exists target/foo && rm -rf target '
test -(e|d) does not provide a nice error message when we hit test failures, so use test_path_exists, test_path_is_dir instead. Signed-off-by: shejialuo <shejialuo@gmail.com> --- t/t9117-git-svn-init-clone.sh | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)