{"thread":{"id":"53036","subject":"[GSOC][PATCH 1/2] t1300: replace \"test -f\" into \"test_path_is_file\"","startedAt":"2020-03-19T23:47:36Z","lastAt":"2020-03-20T18:08:44Z","messageCount":8,"participants":["Adrian Wijaya","Jeff King","adrian wijaya","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"393496","messageId":"20200319234723.6504-1-adrianwijaya100@gmail.com","threadId":"53036","inReplyTo":null,"subject":"[GSOC][PATCH 1/2] t1300: replace \"test -f\" into \"test_path_is_file\"","fromName":"Adrian Wijaya","fromEmail":"adrianwijaya100@gmail.com","sentAt":"2020-03-19T23:47:22Z","receivedAt":"2020-03-19T23:47:36Z","isPatch":true,"sender":{"key":"adrianwijaya100@gmail.com","avatar":null},"body":"Dear all,\n\nIn this year, I intend to apply in git for GSoC 2020. This is actually my \nfirst contribution to the git community. I am a sophomore Computer Science \nstudent at University of Indonesia.\n\nIn this patch, I have replaced 'test -f' into 'test_path_is_file' function \n(can be found in test-lib-functions).\n\nSuggestions and advice are very welcome. \n\nRegards,\nAdrian Wijaya\n"},{"id":"393497","messageId":"20200319234723.6504-2-adrianwijaya100@gmail.com","threadId":"53036","inReplyTo":"20200319234723.6504-1-adrianwijaya100@gmail.com","subject":"[GSOC][PATCH 2/2] t1300: replace \"test -f\" into \"test_path_is_file\"","fromName":"Adrian Wijaya","fromEmail":"adrianwijaya100@gmail.com","sentAt":"2020-03-19T23:47:23Z","receivedAt":"2020-03-19T23:47:41Z","isPatch":true,"sender":{"key":"adrianwijaya100@gmail.com","avatar":null},"body":"Replace \"test -f\" into \"test_path_is_file\" to give more verbose\ntest output.\n\nSigned-off-by: Adrian Wijaya <adrianwijaya100@gmail.com>\n---\n t/t1300-config.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 97ebfe1f9d..d74554fc09 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1020,11 +1020,11 @@ test_expect_success SYMLINKS 'symlinked configuration' '\n \tln -s notyet myconfig &&\n \tgit config --file=myconfig test.frotz nitfol &&\n \ttest -h myconfig &&\n-\ttest -f notyet &&\n+\ttest_path_is_file notyet &&\n \ttest \"z$(git config --file=notyet test.frotz)\" = znitfol &&\n \tgit config --file=myconfig test.xyzzy rezrov &&\n \ttest -h myconfig &&\n-\ttest -f notyet &&\n+\ttest_path_is_file notyet &&\n \tcat >expect <<-\\EOF &&\n \tnitfol\n \trezrov\n-- \n2.26.0.rc1.11.g30e9940356\n\n"},{"id":"393518","messageId":"20200320055225.GG499858@coredump.intra.peff.net","threadId":"53036","inReplyTo":"20200319234723.6504-2-adrianwijaya100@gmail.com","subject":"Re: [GSOC][PATCH 2/2] t1300: replace \"test -f\" into \"test_path_is_file\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-20T05:52:25Z","receivedAt":"2020-03-20T05:52:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 20, 2020 at 06:47:23AM +0700, Adrian Wijaya wrote:\n\n> [...]\n\nThanks, and welcome to the Git community. The patch looks pretty good to\nme. A few minor nits:\n\n> Subject: Re: [GSOC][PATCH 2/2] t1300: replace \"test -f\" into \"test_path_is_file\"\n\nThe subject says 2/2, but I think there is only one patch. :) Looks like\nyou used send-email; the --cover-letter option is probably what you\nwanted to generate the first message. Though for a single-patch series,\nI'd generally suggest just sending one email total, and putting any\ncomments below the \"---\" line (which would then not be included in the\ncommit message).\n\nThe general form of the subject line looks good, and follows our\nconventions.\n\nI'd suggest s/into/with/ in the subject line as a minor English fixup.\nWe'd often assume the maintainer will just fix up something small like\nthat while applying (or if he doesn't, that it's not too big a deal).\nBut since the point of the microproject is to get comfortable with the\npatch submission process, maybe it would be good practice for you to fix\nit up yourself (using \"commit --amend\" or \"rebase -i\") and re-send (try\ngit-send-email's \"-v\" option).\n\n> Replace \"test -f\" into \"test_path_is_file\" to give more verbose\n> test output.\n\nSame s/into/with/ here, too (or perhaps s/Replace/Convert/).\n\nMaybe worth saying \"to give more verbose test output on failure\", though\nnow I am really nit-picking (sorry, you avoided so many of the usual\nfirst-time-patch pitfalls I have to stretch :) ).\n\n> Signed-off-by: Adrian Wijaya <adrianwijaya100@gmail.com>\n> ---\n\nYou remembered your signoff. Good.\n\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index 97ebfe1f9d..d74554fc09 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -1020,11 +1020,11 @@ test_expect_success SYMLINKS 'symlinked configuration' '\n>  \tln -s notyet myconfig &&\n>  \tgit config --file=myconfig test.frotz nitfol &&\n>  \ttest -h myconfig &&\n> -\ttest -f notyet &&\n> +\ttest_path_is_file notyet &&\n\nAnd the patch itself looks obviously correct.\n\nThe \"test -h\" in the context sticks out now, but we don't have a\ntest_path_is_symlink(). I think adding it goes beyond the scope of this\npatch, and beyond what's needed for a microproject. But if you or\nanybody wants to add it (modeled after test_path_is_file), it seems like\na reasonable thing for us to have.\n\n-Peff\n"},{"id":"393530","messageId":"CA+0Uiy-EkT1BHSgL4R783Qh=c4-W_FZwB=LwnQG7OLx92YN=ww@mail.gmail.com","threadId":"53036","inReplyTo":"20200320055225.GG499858@coredump.intra.peff.net","subject":"Re: [GSOC][PATCH 2/2] t1300: replace \"test -f\" into \"test_path_is_file\"","fromName":"adrian wijaya","fromEmail":"adrianwijaya100@gmail.com","sentAt":"2020-03-20T15:22:56Z","receivedAt":"2020-03-20T15:23:14Z","isPatch":true,"sender":{"key":"adrianwijaya100@gmail.com","avatar":null},"body":"On Fri, Mar 20, 2020 at 12:52 PM Jeff King <peff@peff.net> wrote:\n>\n> On Fri, Mar 20, 2020 at 06:47:23AM +0700, Adrian Wijaya wrote:\n>\n> > [...]\n>\n> Thanks, and welcome to the Git community. The patch looks pretty good to\n> me. A few minor nits:\n>\n> > Subject: Re: [GSOC][PATCH 2/2] t1300: replace \"test -f\" into \"test_path_is_file\"\n>\n> The subject says 2/2, but I think there is only one patch. :) Looks like\n> you used send-email; the --cover-letter option is probably what you\n> wanted to generate the first message. Though for a single-patch series,\n\nThanks for letting me know. Hmm, looks like I didn't get to see that part\nwhen I looked at the documentation.\n\n> I'd generally suggest just sending one email total, and putting any\n> comments below the \"---\" line (which would then not be included in the\n> commit message).\n\nGot it.\n\n>\n> The general form of the subject line looks good, and follows our\n> conventions.\n>\n> I'd suggest s/into/with/ in the subject line as a minor English fixup.\n> We'd often assume the maintainer will just fix up something small like\n> that while applying (or if he doesn't, that it's not too big a deal).\n> But since the point of the microproject is to get comfortable with the\n> patch submission process, maybe it would be good practice for you to fix\n> it up yourself (using \"commit --amend\" or \"rebase -i\") and re-send (try\n> git-send-email's \"-v\" option).\n>\n> > Replace \"test -f\" into \"test_path_is_file\" to give more verbose\n> > test output.\n>\n> Same s/into/with/ here, too (or perhaps s/Replace/Convert/).\n>\n\nSounds good. I will make a second version of this patch.\n\n>\n> Maybe worth saying \"to give more verbose test output on failure\", though\n> now I am really nit-picking (sorry, you avoided so many of the usual\n> first-time-patch pitfalls I have to stretch :) ).\n>\n\nNo worries. Actually, I can learn something that will be useful for my next\ncontribution.\n\n>\n> > Signed-off-by: Adrian Wijaya <adrianwijaya100@gmail.com>\n> > ---\n>\n> You remembered your signoff. Good.\n>\n> > diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> > index 97ebfe1f9d..d74554fc09 100755\n> > --- a/t/t1300-config.sh\n> > +++ b/t/t1300-config.sh\n> > @@ -1020,11 +1020,11 @@ test_expect_success SYMLINKS 'symlinked configuration' '\n> >       ln -s notyet myconfig &&\n> >       git config --file=myconfig test.frotz nitfol &&\n> >       test -h myconfig &&\n> > -     test -f notyet &&\n> > +     test_path_is_file notyet &&\n>\n> And the patch itself looks obviously correct.\n\nThanks :)\n\n>\n> The \"test -h\" in the context sticks out now, but we don't have a\n> test_path_is_symlink(). I think adding it goes beyond the scope of this\n> patch, and beyond what's needed for a microproject. But if you or\n> anybody wants to add it (modeled after test_path_is_file), it seems like\n> a reasonable thing for us to have.\n>\n> -Peff\n\n\nNever thought of that. I think I will make a feature request about it when\nI have enough time.\n"},{"id":"393535","messageId":"CA+0Uiy-gtdn07=QOx3JcOH-zhMz5yAKOW6=9mNc8Zi+RdfWGyQ@mail.gmail.com","threadId":"53036","inReplyTo":"20200319234723.6504-1-adrianwijaya100@gmail.com","subject":"Re: [GSOC][PATCH 1/2] t1300: replace \"test -f\" into \"test_path_is_file\"","fromName":"adrian wijaya","fromEmail":"adrianwijaya100@gmail.com","sentAt":"2020-03-20T15:45:53Z","receivedAt":"2020-03-20T15:46:09Z","isPatch":true,"sender":{"key":"adrianwijaya100@gmail.com","avatar":null},"body":"From 5fb689207ebe06b5f4c506b0b184c95a8a2e529d Mon Sep 17 00:00:00 2001\nFrom: Adrian Wijaya <adrianwijaya100@gmail.com>\nDate: Thu, 19 Mar 2020 21:13:44 +0700\nSubject: [GSOC][PATCH v2] t1300: convert \"test -f\" with \"test_path_is_file\"\n\nConvert \"test -f\" with \"test_path_is_file\" to give more verbose\ntest output on failure.\n\nSigned-off-by: Adrian Wijaya <adrianwijaya100@gmail.com>\n---\n t/t1300-config.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 97ebfe1f9d..d74554fc09 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1020,11 +1020,11 @@ test_expect_success SYMLINKS 'symlinked configuration' '\n     ln -s notyet myconfig &&\n     git config --file=myconfig test.frotz nitfol &&\n     test -h myconfig &&\n-    test -f notyet &&\n+    test_path_is_file notyet &&\n     test \"z$(git config --file=notyet test.frotz)\" = znitfol &&\n     git config --file=myconfig test.xyzzy rezrov &&\n     test -h myconfig &&\n-    test -f notyet &&\n+    test_path_is_file notyet &&\n     cat >expect <<-\\EOF &&\n     nitfol\n     rezrov\n-- \n2.26.0.rc1.11.g30e9940356\n\nOn Fri, Mar 20, 2020 at 6:47 AM Adrian Wijaya <adrianwijaya100@gmail.com> wrote:\n>\n> Dear all,\n>\n> In this year, I intend to apply in git for GSoC 2020. This is actually my\n> first contribution to the git community. I am a sophomore Computer Science\n> student at University of Indonesia.\n>\n> In this patch, I have replaced 'test -f' into 'test_path_is_file' function\n> (can be found in test-lib-functions).\n>\n> Suggestions and advice are very welcome.\n>\n> Regards,\n> Adrian Wijaya\n"},{"id":"393538","messageId":"20200320160723.15190-1-adrianwijaya100@gmail.com","threadId":"53036","inReplyTo":"CA+0Uiy-gtdn07=QOx3JcOH-zhMz5yAKOW6=9mNc8Zi+RdfWGyQ@mail.gmail.com","subject":"[GSOC][PATCH v2] t1300: convert \"test -f\" with \"test_path_is_file\"","fromName":"Adrian Wijaya","fromEmail":"adrianwijaya100@gmail.com","sentAt":"2020-03-20T16:07:23Z","receivedAt":"2020-03-20T16:07:43Z","isPatch":true,"sender":{"key":"adrianwijaya100@gmail.com","avatar":null},"body":"Convert \"test -f\" with \"test_path_is_file\" to give more verbose\ntest output on failure.\n\nSigned-off-by: Adrian Wijaya <adrianwijaya100@gmail.com>\n---\n t/t1300-config.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 97ebfe1f9d..d74554fc09 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1020,11 +1020,11 @@ test_expect_success SYMLINKS 'symlinked configuration' '\n \tln -s notyet myconfig &&\n \tgit config --file=myconfig test.frotz nitfol &&\n \ttest -h myconfig &&\n-\ttest -f notyet &&\n+\ttest_path_is_file notyet &&\n \ttest \"z$(git config --file=notyet test.frotz)\" = znitfol &&\n \tgit config --file=myconfig test.xyzzy rezrov &&\n \ttest -h myconfig &&\n-\ttest -f notyet &&\n+\ttest_path_is_file notyet &&\n \tcat >expect <<-\\EOF &&\n \tnitfol\n \trezrov\n-- \n2.26.0.rc1.11.g30e9940356\n\n"},{"id":"393543","messageId":"xmqq4kuidhs7.fsf@gitster.c.googlers.com","threadId":"53036","inReplyTo":"20200320160723.15190-1-adrianwijaya100@gmail.com","subject":"Re: [GSOC][PATCH v2] t1300: convert \"test -f\" with \"test_path_is_file\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-20T18:04:40Z","receivedAt":"2020-03-20T18:04:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adrian Wijaya <adrianwijaya100@gmail.com> writes:\n\n> Subject: Re: [GSOC][PATCH v2] t1300: convert \"test -f\" with \"test_path_is_file\"\n\nI thought the suggestion was either \"replace with\" or \"convert\ninto\", but the above looks a bit funny cross between them.  I'd\nprobably go with \"replace\".\n\n> Convert \"test -f\" with \"test_path_is_file\" to give more verbose\n> test output on failure.\n\nLikewise, but the first part is already said in the title.  So\n\n\tSubject: t1300: replace \"test -f\" with \"test_path_is_file\"\n\n\tThis way, the test will not fail silently, making it easier\n\tto diagnose.\n\nor something like that, perhaps.\n\n> Signed-off-by: Adrian Wijaya <adrianwijaya100@gmail.com>\n> ---\n>  t/t1300-config.sh | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index 97ebfe1f9d..d74554fc09 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -1020,11 +1020,11 @@ test_expect_success SYMLINKS 'symlinked configuration' '\n>  \tln -s notyet myconfig &&\n>  \tgit config --file=myconfig test.frotz nitfol &&\n>  \ttest -h myconfig &&\n> -\ttest -f notyet &&\n> +\ttest_path_is_file notyet &&\n>  \ttest \"z$(git config --file=notyet test.frotz)\" = znitfol &&\n>  \tgit config --file=myconfig test.xyzzy rezrov &&\n>  \ttest -h myconfig &&\n> -\ttest -f notyet &&\n> +\ttest_path_is_file notyet &&\n>  \tcat >expect <<-\\EOF &&\n>  \tnitfol\n>  \trezrov\n"},{"id":"393544","messageId":"xmqqzhcac313.fsf@gitster.c.googlers.com","threadId":"53036","inReplyTo":"20200320160723.15190-1-adrianwijaya100@gmail.com","subject":"Re: [GSOC][PATCH v2] t1300: convert \"test -f\" with \"test_path_is_file\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-20T18:08:40Z","receivedAt":"2020-03-20T18:08:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adrian Wijaya <adrianwijaya100@gmail.com> writes:\n\n>  \ttest \"z$(git config --file=notyet test.frotz)\" = znitfol &&\n\nWe see 130+ instances of this rather antiquated construct that\npresumably avoids to compare an empty string with something else\nusing \"test X = Y\" tool.  It might have been a good idea in 2005 (I\ndo not remember why we started doing this), but perhaps it is time\nto clean them up by listing it as another microproject idea for the\nnext year?\n\n"}]}