{"thread":{"id":"57281","subject":"[GSoC] [PATCH 0/1] t0001: replace \"test [-d|-f]\" with test_path_is_* functions","startedAt":"2022-01-21T10:21:21Z","lastAt":"2022-02-14T08:53:56Z","messageCount":13,"participants":["Shaoxuan Yuan","Taylor Blau","Junio C Hamano","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"446617","messageId":"20220121102109.433457-1-shaoxuan.yuan02@gmail.com","threadId":"57281","inReplyTo":null,"subject":"[GSoC] [PATCH 0/1] t0001: replace \"test [-d|-f]\" with test_path_is_* functions","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-01-21T10:21:08Z","receivedAt":"2022-01-21T10:21:21Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"As a microproject, I found that the \"test [-d|-f]\" in t0001 test script\ncan be replaced by appropriate helper functions.\n\nShaoxuan Yuan (1):\n t0001-init.sh use test_path_is_* functions\n\n t/t0001-init.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\n-- \n2.25.1\n\n"},{"id":"446618","messageId":"20220121102109.433457-2-shaoxuan.yuan02@gmail.com","threadId":"57281","inReplyTo":"20220121102109.433457-1-shaoxuan.yuan02@gmail.com","subject":"[GSoC] [PATCH 1/1] t0001: replace \"test [-d|-f]\" with test_path_is_* functions","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-01-21T10:21:09Z","receivedAt":"2022-01-21T10:21:22Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Signed-off-by: Shaoxuan Yuan \n---\n t/t0001-init.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 3235ab4d53..c72a28d3a5 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -6,7 +6,7 @@ TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n check_config () {\n-\tif test -d \"$1\" && test -f \"$1/config\" && test -d \"$1/refs\"\n+\tif test_path_is_dir \"$1\" && test_path_is_file \"$1/config\" && test_path_is_dir \"$1/refs\"\n \tthen\n \t\t: happy\n \telse\n-- \n2.25.1\n\n"},{"id":"446639","messageId":"Yer/oEZK6TBFSsde@nand.local","threadId":"57281","inReplyTo":"20220121102109.433457-2-shaoxuan.yuan02@gmail.com","subject":"Re: [GSoC] [PATCH 1/1] t0001: replace \"test [-d|-f]\" with test_path_is_* functions","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-21T18:46:56Z","receivedAt":"2022-01-21T18:47:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Shaoxuan,\n\nOn Fri, Jan 21, 2022 at 06:21:09PM +0800, Shaoxuan Yuan wrote:\n> diff --git a/t/t0001-init.sh b/t/t0001-init.sh\n> index 3235ab4d53..c72a28d3a5 100755\n> --- a/t/t0001-init.sh\n> +++ b/t/t0001-init.sh\n> @@ -6,7 +6,7 @@ TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n>\n> check_config () {\n> -\tif test -d \"$1\" && test -f \"$1/config\" && test -d \"$1/refs\"\n> +\tif test_path_is_dir \"$1\" && test_path_is_file \"$1/config\" && test_path_is_dir \"$1/refs\"\n> \tthen\n> \t\t: happy\n> \telse\n\nLooks very reasonable to me. Indeed, this line comes from 6adcca3fe8\n(Fix initialization of a bare repository, 2007-08-27) which predates\n2caf20c52b (test-lib: user-friendly alternatives to test [-d|-f|-e],\n2010-08-10) when these helpers were originally introduced.\n\nI thought that we could probably just shorten this to calling\n\"test_path_is_file\" twice: once for \"$1/config\" and a second time for\n\"$1/refs\", but that assumes \"$1\" is non-empty. And to ensure that you'd\nneed another check, which amounts to the same amount of code overall.\n\nSo the fix here looks good to me, and thanks for your contribution!\n\nThanks,\nTaylor\n"},{"id":"446652","messageId":"xmqqr190g6gd.fsf@gitster.g","threadId":"57281","inReplyTo":"Yer/oEZK6TBFSsde@nand.local","subject":"Re: [GSoC] [PATCH 1/1] t0001: replace \"test [-d|-f]\" with test_path_is_* functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-21T20:49:22Z","receivedAt":"2022-01-21T20:49:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> -\tif test -d \"$1\" && test -f \"$1/config\" && test -d \"$1/refs\"\n>> +\tif test_path_is_dir \"$1\" && test_path_is_file \"$1/config\" && test_path_is_dir \"$1/refs\"\n>> \tthen\n>> \t\t: happy\n>> \telse\n>\n> Looks very reasonable to me. Indeed, this line comes from 6adcca3fe8\n> (Fix initialization of a bare repository, 2007-08-27) which predates\n> 2caf20c52b (test-lib: user-friendly alternatives to test [-d|-f|-e],\n> 2010-08-10) when these helpers were originally introduced.\n>\n> I thought that we could probably just shorten this to calling\n> \"test_path_is_file\" twice: once for \"$1/config\" and a second time for\n> \"$1/refs\", but that assumes \"$1\" is non-empty. And to ensure that you'd\n> need another check, which amounts to the same amount of code overall.\n\nI had the same thought.\n\nSince the first \"$GIT_DIR must be a directory\" matters only when the\ncaller is crazy enough to have a bare repository at the root of the\nfilesystem and to think that it is a good idea to say \"\" is the\n\"$GIT_DIR\" (in which case, \"test -d ''\" would fail, even though the\ntests for /config and /refs are checking the right thing), I do not\nsee much downside from omitting the first one, but I think that is\nsomething we need to do _outside_ the topic of this change, which is\npurely \"modernize, using the helpers we already have, without\nchanging what we do\".\n\n\n"},{"id":"446734","messageId":"CAJyCBORyjNaPXshQrGjbrUCUv832MJsNew6ez-EOWJo2od9a8g@mail.gmail.com","threadId":"57281","inReplyTo":"xmqqr190g6gd.fsf@gitster.g","subject":"Re: [GSoC] [PATCH 1/1] t0001: replace \"test [-d|-f]\" with test_path_is_* functions","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-01-24T05:56:55Z","receivedAt":"2022-01-24T05:57:09Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"On Sat, Jan 22, 2022 at 4:49 AM Junio C Hamano wrote:\n>\n> Taylor Blau writes:\n>\n> >> - if test -d \"$1\" && test -f \"$1/config\" && test -d \"$1/refs\"\n> >> + if test_path_is_dir \"$1\" && test_path_is_file \"$1/config\" && test_path_is_dir \"$1/refs\"\n> >> then\n> >> : happy\n> >> else\n> >\n> > Looks very reasonable to me. Indeed, this line comes from 6adcca3fe8\n> > (Fix initialization of a bare repository, 2007-08-27) which predates\n> > 2caf20c52b (test-lib: user-friendly alternatives to test [-d|-f|-e],\n> > 2010-08-10) when these helpers were originally introduced.\n> >\n> > I thought that we could probably just shorten this to calling\n> > \"test_path_is_file\" twice: once for \"$1/config\" and a second time for\n> > \"$1/refs\", but that assumes \"$1\" is non-empty. And to ensure that you'd\n> > need another check, which amounts to the same amount of code overall.\n>\n> I had the same thought.\n>\n> Since the first \"$GIT_DIR must be a directory\" matters only when the\n> caller is crazy enough to have a bare repository at the root of the\n> filesystem and to think that it is a good idea to say \"\" is the\n> \"$GIT_DIR\" (in which case, \"test -d ''\" would fail, even though the\n> tests for /config and /refs are checking the right thing), I do not\n> see much downside from omitting the first one, but I think that is\n> something we need to do _outside_ the topic of this change, which is\n> purely \"modernize, using the helpers we already have, without\n> changing what we do\"\n>\nYes I feel the same way, one patch for one thing :)\n"},{"id":"448085","messageId":"CAJyCBORHE2+UMKYFfKArgK68fAT=pKVabVmfifzg--rHAFkaqg@mail.gmail.com","threadId":"57281","inReplyTo":"xmqqr190g6gd.fsf@gitster.g","subject":"Re: [GSoC] [PATCH 1/1] t0001: replace \"test [-d|-f]\" with test_path_is_* functions","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-02-10T03:11:09Z","receivedAt":"2022-02-10T03:11:25Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Hi Junio,\n\nSince I didn't see this change in seen or next, do you plan to apply it?\n\n--\nThanks,\nShaoxuan\n\nOn Sat, Jan 22, 2022 at 4:49 AM Junio C Hamano wrote:\n>\n> Taylor Blau writes:\n>\n> >> - if test -d \"$1\" && test -f \"$1/config\" && test -d \"$1/refs\"\n> >> + if test_path_is_dir \"$1\" && test_path_is_file \"$1/config\" && test_path_is_dir \"$1/refs\"\n> >> then\n> >> : happy\n> >> else\n> >\n> > Looks very reasonable to me. Indeed, this line comes from 6adcca3fe8\n> > (Fix initialization of a bare repository, 2007-08-27) which predates\n> > 2caf20c52b (test-lib: user-friendly alternatives to test [-d|-f|-e],\n> > 2010-08-10) when these helpers were originally introduced.\n> >\n> > I thought that we could probably just shorten this to calling\n> > \"test_path_is_file\" twice: once for \"$1/config\" and a second time for\n> > \"$1/refs\", but that assumes \"$1\" is non-empty. And to ensure that you'd\n> > need another check, which amounts to the same amount of code overall.\n>\n> I had the same thought.\n>\n> Since the first \"$GIT_DIR must be a directory\" matters only when the\n> caller is crazy enough to have a bare repository at the root of the\n> filesystem and to think that it is a good idea to say \"\" is the\n> \"$GIT_DIR\" (in which case, \"test -d ''\" would fail, even though the\n> tests for /config and /refs are checking the right thing), I do not\n> see much downside from omitting the first one, but I think that is\n> something we need to do _outside_ the topic of this change, which is\n> purely \"modernize, using the helpers we already have, without\n> changing what we do\".\n>\n>\n"},{"id":"448104","messageId":"xmqqo83fw5yc.fsf@gitster.g","threadId":"57281","inReplyTo":"CAJyCBORHE2+UMKYFfKArgK68fAT=pKVabVmfifzg--rHAFkaqg@mail.gmail.com","subject":"Re: [GSoC] [PATCH 1/1] t0001: replace \"test [-d|-f]\" with test_path_is_* functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-10T07:12:27Z","receivedAt":"2022-02-10T07:12:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shaoxuan Yuan writes:\n\n> Since I didn't see this change in seen or next, do you plan to apply it?\n\nI actually wasn't, as my understanding of it was primarily your\npractice.\n"},{"id":"448105","messageId":"CAJyCBOTBcvgAcSaQ4xs-V=M-im_Umf3ymsWzCAGEmrj6b38pEA@mail.gmail.com","threadId":"57281","inReplyTo":"xmqqo83fw5yc.fsf@gitster.g","subject":"Re: [GSoC] [PATCH 1/1] t0001: replace \"test [-d|-f]\" with test_path_is_* functions","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-02-10T07:21:00Z","receivedAt":"2022-02-10T07:21:13Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"On Thu, Feb 10, 2022 at 3:12 PM Junio C Hamano wrote:\n>\n> Shaoxuan Yuan writes:\n>\n> > Since I didn't see this change in seen or next, do you plan to apply it?\n>\n> I actually wasn't, as my understanding of it was primarily your\n> practice.\n\nUnderstood, thanks for the reply.\n\n--\nThanks,\nShaoxuan\n"},{"id":"448174","messageId":"xmqqv8xmvdoa.fsf@gitster.g","threadId":"57281","inReplyTo":"CAJyCBOTBcvgAcSaQ4xs-V=M-im_Umf3ymsWzCAGEmrj6b38pEA@mail.gmail.com","subject":"Re: [GSoC] [PATCH 1/1] t0001: replace \"test [-d|-f]\" with test_path_is_* functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-10T17:23:17Z","receivedAt":"2022-02-10T17:23:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shaoxuan Yuan writes:\n\n> On Thu, Feb 10, 2022 at 3:12 PM Junio C Hamano wrote:\n>>\n>> Shaoxuan Yuan writes:\n>>\n>> > Since I didn't see this change in seen or next, do you plan to apply it?\n>>\n>> I actually wasn't, as my understanding of it was primarily your\n>> practice.\n>\n> Understood, thanks for the reply.\n\nFWIW, I have the posted patch plus the following SQUASH??? fix-up\nparked in the 'seen' branch. As the script is quiescent right now,\nI do not mind merging it down, now we spent more time on it ;-)\n\n t/t0001-init.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex c72a28d3a5..d479303efa 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -6,7 +6,8 @@ TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n check_config () {\n-\tif test_path_is_dir \"$1\" && test_path_is_file \"$1/config\" && test_path_is_dir \"$1/refs\"\n+\tif test_path_is_dir \"$1\" &&\n+\t test_path_is_file \"$1/config\" && test_path_is_dir \"$1/refs\"\n \tthen\n \t\t: happy\n \telse\n-- \n2.35.1-102-g2b9c120970\n\n"},{"id":"448214","messageId":"CAJyCBOR7KbRxuXJUJ2TrffAjj=rwB3H-Ys9tVUuWGjUKf_LHsw@mail.gmail.com","threadId":"57281","inReplyTo":"xmqqv8xmvdoa.fsf@gitster.g","subject":"Re: [GSoC] [PATCH 1/1] t0001: replace \"test [-d|-f]\" with test_path_is_* functions","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-02-11T09:56:21Z","receivedAt":"2022-02-11T09:56:37Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"Hi Junio,\n\nOn Fri, Feb 11, 2022 at 1:23 AM Junio C Hamano wrote:\n> FWIW, I have the posted patch plus the following SQUASH??? fix-up\n\nI'm not so sure what does \"SQUASH???\" mean especially the three\nquestion marks, i.e. is it just an incidental text or a commit message\nconvention?\nIs it for the convenience of grepping through the\n\"git log\" outputs (cause I found the commit 50d631c71c right away by\ngrepping through the \"git log\" output)?\n\n> parked in the 'seen' branch. As the script is quiescent right now,\n> I do not mind merging it down, now we spent more time on it ;-)\n>\n> t/t0001-init.sh | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t0001-init.sh b/t/t0001-init.sh\n> index c72a28d3a5..d479303efa 100755\n> --- a/t/t0001-init.sh\n> +++ b/t/t0001-init.sh\n> @@ -6,7 +6,8 @@ TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n>\n> check_config () {\n> - if test_path_is_dir \"$1\" && test_path_is_file \"$1/config\" && test_path_is_dir \"$1/refs\"\n> + if test_path_is_dir \"$1\" &&\n> + test_path_is_file \"$1/config\" && test_path_is_dir \"$1/refs\"\n> then\n> : happy\n> else\n\nYeah, I think wrapping it around is a good idea :-)\n\n> --\n> 2.35.1-102-g2b9c120970\n>\n\n-- \nThanks & Regards,\nShaoxuan\n"},{"id":"448228","messageId":"xmqqbkzde44v.fsf@gitster.g","threadId":"57281","inReplyTo":"CAJyCBOR7KbRxuXJUJ2TrffAjj=rwB3H-Ys9tVUuWGjUKf_LHsw@mail.gmail.com","subject":"Re: [GSoC] [PATCH 1/1] t0001: replace \"test [-d|-f]\" with test_path_is_* functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-11T16:53:36Z","receivedAt":"2022-02-11T16:53:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shaoxuan Yuan writes:\n\n> On Fri, Feb 11, 2022 at 1:23 AM Junio C Hamano wrote:\n>> FWIW, I have the posted patch plus the following SQUASH??? fix-up\n>\n> I'm not so sure what does \"SQUASH???\" mean especially the three\n> question marks, i.e. is it just an incidental text or a commit message\n> convention?\n> Is it for the convenience of grepping through the\n> \"git log\" outputs (cause I found the commit 50d631c71c right away by\n> grepping through the \"git log\" output)?\n\nIt is primarily to remind me not to merge the branch down to 'next'\nwithout dealing with it.\n\n> Yeah, I think wrapping it around is a good idea :-)\n\nThen will squash it in and merge it down.\n\nThanks.\n"},{"id":"448350","messageId":"CAP8UFD3=kd8ACA8PPHyV7fCYLuJSb4Qj2OSdBFJ3RHtnwbGO2A@mail.gmail.com","threadId":"57281","inReplyTo":"CAJyCBOR7KbRxuXJUJ2TrffAjj=rwB3H-Ys9tVUuWGjUKf_LHsw@mail.gmail.com","subject":"Re: [GSoC] [PATCH 1/1] t0001: replace \"test [-d|-f]\" with test_path_is_* functions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-14T08:45:25Z","receivedAt":"2022-02-14T08:46:05Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Feb 14, 2022 at 9:32 AM Shaoxuan Yuan wrote:\n> On Fri, Feb 11, 2022 at 1:23 AM Junio C Hamano wrote:\n> > FWIW, I have the posted patch plus the following SQUASH??? fix-up\n>\n> I'm not so sure what does \"SQUASH???\" mean especially the three\n> question marks, i.e. is it just an incidental text or a commit message\n> convention?\n\nIt means that you might want to squash the fix-up commit or a similar\ncommit into your commit (or one of your commits in case of a\ncommit/patch series), and then resubmit a new version.\n\n> Is it for the convenience of grepping through the\n> \"git log\" outputs (cause I found the commit 50d631c71c right away by\n> grepping through the \"git log\" output)?\n\nIt is for convenience that it's named \"SQUASH???\" as everyone (who is\nfamiliar with the mailing list) then knows what needs to be done on\nthe proposed commit(s).\n\n> > parked in the 'seen' branch. As the script is quiescent right now,\n> > I do not mind merging it down, now we spent more time on it ;-)\n\nAlternatively as Junio says he is ok with merging that down, you might\njust accept his offer and he will squash the \"SQUASH???\" commit for\nyou before merging the result into the \"next\" branch.\n"},{"id":"448351","messageId":"CAJyCBOTg+5m6s6Nw+ma4Y7RgMe_0GmjyN_f+BBeEc-BhzJi_Qw@mail.gmail.com","threadId":"57281","inReplyTo":"CAP8UFD3=kd8ACA8PPHyV7fCYLuJSb4Qj2OSdBFJ3RHtnwbGO2A@mail.gmail.com","subject":"Re: [GSoC] [PATCH 1/1] t0001: replace \"test [-d|-f]\" with test_path_is_* functions","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-02-14T08:53:41Z","receivedAt":"2022-02-14T08:53:56Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"On Mon, Feb 14, 2022 at 4:45 PM Christian Couder\n wrote:\n>\n> On Mon, Feb 14, 2022 at 9:32 AM Shaoxuan Yuan wrote:\n> > On Fri, Feb 11, 2022 at 1:23 AM Junio C Hamano wrote:\n> > > FWIW, I have the posted patch plus the following SQUASH??? fix-up\n> >\n> > I'm not so sure what does \"SQUASH???\" mean especially the three\n> > question marks, i.e. is it just an incidental text or a commit message\n> > convention?\n>\n> It means that you might want to squash the fix-up commit or a similar\n> commit into your commit (or one of your commits in case of a\n> commit/patch series), and then resubmit a new version.\n>\n> > Is it for the convenience of grepping through the\n> > \"git log\" outputs (cause I found the commit 50d631c71c right away by\n> > grepping through the \"git log\" output)?\n>\n> It is for convenience that it's named \"SQUASH???\" as everyone (who is\n> familiar with the mailing list) then knows what needs to be done on\n> the proposed commit(s).\n\nYeah, now I see :)\n\n> > > parked in the 'seen' branch. As the script is quiescent right now,\n> > > I do not mind merging it down, now we spent more time on it ;-)\n>\n> Alternatively as Junio says he is ok with merging that down, you might\n> just accept his offer and he will squash the \"SQUASH???\" commit for\n> you before merging the result into the \"next\" branch.\n\nYeah, I saw the squashed version in the latest \"What's cooking in git.git\".\nThanks for the help and suggestions :)\n\n-- \nThanks & Regards,\nShaoxuan\n"}]}