{"thread":{"id":"59184","subject":"[GSoC][PATCH] t/t5000-tar-tree: add helper function","startedAt":"2023-02-02T20:27:07Z","lastAt":"2023-02-05T17:59:18Z","messageCount":8,"participants":["Kostya Farber","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"471388","messageId":"20230202202557.19297-1-kostya.farber@gmail.com","threadId":"59184","inReplyTo":null,"subject":"[GSoC][PATCH] t/t5000-tar-tree: add helper function","fromName":"Kostya Farber","fromEmail":"kostya.farber@gmail.com","sentAt":"2023-02-02T20:25:57Z","receivedAt":"2023-02-02T20:27:07Z","isPatch":true,"sender":{"key":"kostya.farber@gmail.com","avatar":"https://avatars.githubusercontent.com/u/73378227?v=4"},"body":"Add the helper function test_file_path_exists to the\ninterpret pax header test. This change makes it clearer\nas to what the test is trying to check, in this case whether\na file path exists.\n\nSigned-off-by: Kostya Farber <kostya.farber@gmail.com>\n---\nHello all. I'd like to introduce myself. My name is \nKostya, I am new to the git open source community.\n\nI am a data engineer by trade, but have recently gotten\ninto (and am really enjoying) the open source world.\nMy main contributions have been in the scientific \npython space (https://github.com/kostyafarber).\n\nI have read MyFirstContribution, documentation in \nt/README and the general GSoC page. I have really \nstarted to enjoy low-level programming and have\nbeen going through The C Programming Language (Second\nEdition) and the Linux Programming Interface.\n\nI am planning to submit a proposal for GSoC (hopefully) and \nwant to try help out and contribute to a tool I've been using\nfor a long time and have love for.\n\nKeen to speak to you all soon. \n\n t/t5000-tar-tree.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex d473048138..19d5bd0c04 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -73,7 +73,7 @@ check_tar() {\n \t\t\tfor header in *.paxheader\n \t\t\tdo\n \t\t\t\tdata=${header%.paxheader}.data &&\n-\t\t\t\tif test -h $data || test -e $data\n+\t\t\t\tif test -h $data || test_file_path_exists $data\n \t\t\t\tthen\n \t\t\t\t\tpath=$(get_pax_header $header path) &&\n \t\t\t\t\tif test -n \"$path\"\n-- \n2.39.0\n\n"},{"id":"471401","messageId":"xmqqa61vsniy.fsf@gitster.g","threadId":"59184","inReplyTo":"20230202202557.19297-1-kostya.farber@gmail.com","subject":"Re: [GSoC][PATCH] t/t5000-tar-tree: add helper function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-02T22:36:21Z","receivedAt":"2023-02-02T22:36:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kostya Farber <kostya.farber@gmail.com> writes:\n\n> Add the helper function test_file_path_exists to the\n> interpret pax header test. This change makes it clearer\n> as to what the test is trying to check, in this case whether\n> a file path exists.\n\nReally?  \n\nThe code with \"test -e\" is already clear that it is checking if the\npath $data exists.  This change does not make it any clearer.  What\nit helps is that it gives a message upon failure, when the test is\nrun with the \"-v\" option.\n\n> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\n> index d473048138..19d5bd0c04 100755\n> --- a/t/t5000-tar-tree.sh\n> +++ b/t/t5000-tar-tree.sh\n> @@ -73,7 +73,7 @@ check_tar() {\n>  \t\t\tfor header in *.paxheader\n>  \t\t\tdo\n>  \t\t\t\tdata=${header%.paxheader}.data &&\n> -\t\t\t\tif test -h $data || test -e $data\n> +\t\t\t\tif test -h $data || test_file_path_exists $data\n>  \t\t\t\tthen\n>  \t\t\t\t\tpath=$(get_pax_header $header path) &&\n>  \t\t\t\t\tif test -n \"$path\"\n\nNothing seems to be adding a new helper whose name is\ntest_file_path_exists; the patch expects such a helper already\nexists and uses it in place for existing \"test -e\".\n\nPerhaps you meant to say \"use test_path_exists\" not \"add helper\" on\nthe title, and use that function in the patch instead?\n\nThanks.\n"},{"id":"471404","messageId":"CAPig+cTC4TUK2NXFmP6ymEYYjYAmCSd53dzFhKn0ido8x6uh1Q@mail.gmail.com","threadId":"59184","inReplyTo":"xmqqa61vsniy.fsf@gitster.g","subject":"Re: [GSoC][PATCH] t/t5000-tar-tree: add helper function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-02-02T23:09:16Z","receivedAt":"2023-02-02T23:09:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 2, 2023 at 5:37 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Kostya Farber <kostya.farber@gmail.com> writes:\n> > Add the helper function test_file_path_exists to the\n> > interpret pax header test. This change makes it clearer\n> > as to what the test is trying to check, in this case whether\n> > a file path exists.\n> >\n> > -                             if test -h $data || test -e $data\n> > +                             if test -h $data || test_file_path_exists $data\n>\n> Nothing seems to be adding a new helper whose name is\n> test_file_path_exists; the patch expects such a helper already\n> exists and uses it in place for existing \"test -e\".\n>\n> Perhaps you meant to say \"use test_path_exists\" not \"add helper\" on\n> the title, and use that function in the patch instead?\n\nA couple comments...\n\nThe test framework does not define a function named\n\"test_file_path_exists\". Probably \"test_path_exists\" was intended.\n\nDelving more deeply, though, this change seems undesirable from a\nclarity viewpoint. The function \"test_path_exists\" is an assertion;\nits purpose is to make the test fail if the path is expected to exist\nbut doesn't. However, in the original code from t5000:\n\n    if test -h $data || test -e $data\n    then\n        path=$(get_pax_header $header path) &&\n        if test -n \"$path\"\n        then\n            mv \"$data\" \"$path\" || exit 1\n        fi\n    fi\n\nit is perfectly fine if the path is neither a symbolic link nor an\nactual file; that's not considered an error. Therefore, using an\nassertion function -- which suggests test failure -- muddles the\nintent of the code rather than clarifying it.\n\nAdditionally, t/test-lib-functions.sh also defines the function\n\"test_path_is_symlink\" which would seem to be the obvious complement\nto \"test_path_exists\", thus one might have expected the patch to\nchange the code to:\n\n    if test_path_is_symlink $data || test_path_exists $data\n    then\n        ...\n\nhowever, \"test_path_is_symlink\" is also an assertion, thus not really\nsuitable for this case in which it is acceptable (not an error) if\nneither condition holds true.\n\nSo, t5000 seems to be one of those relatively rare cases in which the\nraw \"test\" command is more correct than the higher-level helper\nfunctions.\n"},{"id":"471406","messageId":"CAPig+cR+1bxgitSmJ47PxD=CPxJjMh5Vm+zRdVU8iEo9pvtutw@mail.gmail.com","threadId":"59184","inReplyTo":"CAPig+cTC4TUK2NXFmP6ymEYYjYAmCSd53dzFhKn0ido8x6uh1Q@mail.gmail.com","subject":"Re: [GSoC][PATCH] t/t5000-tar-tree: add helper function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-02-02T23:19:14Z","receivedAt":"2023-02-02T23:19:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 2, 2023 at 6:09 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> So, t5000 seems to be one of those relatively rare cases in which the\n> raw \"test\" command is more correct than the higher-level helper\n> functions.\n\nBy the way, although the change made by this patch is probably\nundesirable, if you would like to try a different submission, there is\na bit of modernization that could be applied to t5000. In particular,\nthe \"archive and :(glob)\" test does not match present-day style\nguidelines:\n\n        git archive -v HEAD -- \":(glob)**/sh\" >/dev/null 2>actual &&\n        cat >expect <<EOF &&\n    a/\n    a/bin/\n    a/bin/sh\n    EOF\n        test_cmp expect actual\n\nThese days, we would use `<<-EOF` rather than `<<EOF` to allow the\nhere-doc body to be indented to the same depth as the `cat` command\nitself. Furthermore, we would use `\\EOF` to indicate to the reader\nthat no interpolation is expected within the body. Taken together, the\nfinal result would be:\n\n    git archive -v HEAD -- \":(glob)**/sh\" >/dev/null 2>actual &&\n    cat >expect <<-\\EOF &&\n    a/\n    a/bin/\n    a/bin/sh\n    EOF\n    test_cmp expect actual\n\nBoth cleanups can be done via a single patch; the commit message\nshould mention both modernizations.\n"},{"id":"471489","messageId":"CAPRQfCZK+zAtT-4mQMAvGwNhB5O3sr3YecSvb0WHTxnC3A5r+A@mail.gmail.com","threadId":"59184","inReplyTo":"xmqqa61vsniy.fsf@gitster.g","subject":"Re: [GSoC][PATCH] t/t5000-tar-tree: add helper function","fromName":"Kostya Farber","fromEmail":"kostya.farber@gmail.com","sentAt":"2023-02-04T15:04:04Z","receivedAt":"2023-02-04T15:04:21Z","isPatch":true,"sender":{"key":"kostya.farber@gmail.com","avatar":"https://avatars.githubusercontent.com/u/73378227?v=4"},"body":"On Thu, Feb 2, 2023 at 10:36 PM Junio C Hamano <gitster@pobox.com> wrote:\n\n> Kostya Farber <kostya.farber@gmail.com> writes:\n>\n> > Add the helper function test_file_path_exists to the\n> > interpret pax header test. This change makes it clearer\n> > as to what the test is trying to check, in this case whether\n> > a file path exists.\n>\n> Really?\n>\n> The code with \"test -e\" is already clear that it is checking if the\n> path $data exists.  This change does not make it any clearer.  What\n> it helps is that it gives a message upon failure, when the test is\n> run with the \"-v\" option.\n\nOkay, noted. I will look into the helper functions more closely to\nunderstand why and how they useful compared to other methods (i.e test\n-e in this instance)\n\n> > diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\n> > index d473048138..19d5bd0c04 100755\n> > --- a/t/t5000-tar-tree.sh\n> > +++ b/t/t5000-tar-tree.sh\n> > @@ -73,7 +73,7 @@ check_tar() {\n> >                       for header in *.paxheader\n> >                       do\n> >                               data=${header%.paxheader}.data &&\n> > -                             if test -h $data || test -e $data\n> > +                             if test -h $data || test_file_path_exists $data\n> >                               then\n> >                                       path=$(get_pax_header $header path) &&\n> >                                       if test -n \"$path\"\n>\n> Nothing seems to be adding a new helper whose name is\n> test_file_path_exists; the patch expects such a helper already\n> exists and uses it in place for existing \"test -e\".\n>\n> Perhaps you meant to say \"use test_path_exists\" not \"add helper\" on\n> the title, and use that function in the patch instead?\n\nYes you are right. I made a mistake by using the wrong function name\nand I think \"use test_path_exists\" was my intention as a title name.\n\n> Thanks.\n"},{"id":"471490","messageId":"CAPRQfCYxdkTqqBuN3qY1_UsXYk59ocxNd6D31rO+MoGf_4fsUw@mail.gmail.com","threadId":"59184","inReplyTo":"CAPig+cTC4TUK2NXFmP6ymEYYjYAmCSd53dzFhKn0ido8x6uh1Q@mail.gmail.com","subject":"Re: [GSoC][PATCH] t/t5000-tar-tree: add helper function","fromName":"Kostya Farber","fromEmail":"kostya.farber@gmail.com","sentAt":"2023-02-04T15:12:14Z","receivedAt":"2023-02-04T15:12:34Z","isPatch":true,"sender":{"key":"kostya.farber@gmail.com","avatar":"https://avatars.githubusercontent.com/u/73378227?v=4"},"body":"On Thu, Feb 2, 2023 at 11:09 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Thu, Feb 2, 2023 at 5:37 PM Junio C Hamano <gitster@pobox.com> wrote:\n> > Kostya Farber <kostya.farber@gmail.com> writes:\n> > > Add the helper function test_file_path_exists to the\n> > > interpret pax header test. This change makes it clearer\n> > > as to what the test is trying to check, in this case whether\n> > > a file path exists.\n> > >\n> > > -                             if test -h $data || test -e $data\n> > > +                             if test -h $data || test_file_path_exists $data\n> >\n> > Nothing seems to be adding a new helper whose name is\n> > test_file_path_exists; the patch expects such a helper already\n> > exists and uses it in place for existing \"test -e\".\n> >\n> > Perhaps you meant to say \"use test_path_exists\" not \"add helper\" on\n> > the title, and use that function in the patch instead?\n>\n> A couple comments...\n>\n> The test framework does not define a function named\n> \"test_file_path_exists\". Probably \"test_path_exists\" was intended.\n>\n> Delving more deeply, though, this change seems undesirable from a\n> clarity viewpoint. The function \"test_path_exists\" is an assertion;\n> its purpose is to make the test fail if the path is expected to exist\n> but doesn't. However, in the original code from t5000:\n>\n>     if test -h $data || test -e $data\n>     then\n>         path=$(get_pax_header $header path) &&\n>         if test -n \"$path\"\n>         then\n>             mv \"$data\" \"$path\" || exit 1\n>         fi\n>     fi\n>\n> it is perfectly fine if the path is neither a symbolic link nor an\n> actual file; that's not considered an error. Therefore, using an\n> assertion function -- which suggests test failure -- muddles the\n> intent of the code rather than clarifying it.\n\nOkay, that makes sense. Thanks for explaining why this function isn't\ndesirable in this instance, and why it actually reduces the clarity of\nthe test.\n\n> Additionally, t/test-lib-functions.sh also defines the function\n> \"test_path_is_symlink\" which would seem to be the obvious complement\n> to \"test_path_exists\", thus one might have expected the patch to\n> change the code to:\n>\n>     if test_path_is_symlink $data || test_path_exists $data\n>     then\n>         ...\n>\n> however, \"test_path_is_symlink\" is also an assertion, thus not really\n> suitable for this case in which it is acceptable (not an error) if\n> neither condition holds true.\n>\n> So, t5000 seems to be one of those relatively rare cases in which the\n> raw \"test\" command is more correct than the higher-level helper\n> functions.\n\nYes on first glance it would look like test_path_is_symlink would be\nthe obvious complement, but as you say I guess it doesn't make sense\nin this case for the same reasons as using test_path_exists.\nIt's a good learning experience for me to understand when, and in\nwhich contexts, to use the helper functions.\n\nThanks\n"},{"id":"471491","messageId":"CAPRQfCZUwaE4mko46n0wQbkKjpg4Tqxbg6eKLm9rFrBRj6sVLg@mail.gmail.com","threadId":"59184","inReplyTo":"CAPig+cR+1bxgitSmJ47PxD=CPxJjMh5Vm+zRdVU8iEo9pvtutw@mail.gmail.com","subject":"Re: [GSoC][PATCH] t/t5000-tar-tree: add helper function","fromName":"Kostya Farber","fromEmail":"kostya.farber@gmail.com","sentAt":"2023-02-04T15:16:48Z","receivedAt":"2023-02-04T15:17:14Z","isPatch":true,"sender":{"key":"kostya.farber@gmail.com","avatar":"https://avatars.githubusercontent.com/u/73378227?v=4"},"body":"On Thu, Feb 2, 2023 at 11:19 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Thu, Feb 2, 2023 at 6:09 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > So, t5000 seems to be one of those relatively rare cases in which the\n> > raw \"test\" command is more correct than the higher-level helper\n> > functions.\n>\n> By the way, although the change made by this patch is probably\n> undesirable, if you would like to try a different submission, there is\n> a bit of modernization that could be applied to t5000. In particular,\n> the \"archive and :(glob)\" test does not match present-day style\n> guidelines:\n>\n>         git archive -v HEAD -- \":(glob)**/sh\" >/dev/null 2>actual &&\n>         cat >expect <<EOF &&\n>     a/\n>     a/bin/\n>     a/bin/sh\n>     EOF\n>         test_cmp expect actual\n>\n> These days, we would use `<<-EOF` rather than `<<EOF` to allow the\n> here-doc body to be indented to the same depth as the `cat` command\n> itself. Furthermore, we would use `\\EOF` to indicate to the reader\n> that no interpolation is expected within the body. Taken together, the\n> final result would be:\n>\n>     git archive -v HEAD -- \":(glob)**/sh\" >/dev/null 2>actual &&\n>     cat >expect <<-\\EOF &&\n>     a/\n>     a/bin/\n>     a/bin/sh\n>     EOF\n>     test_cmp expect actual\n>\n> Both cleanups can be done via a single patch; the commit message\n> should mention both modernizations.\n\nI would be happy to help and submit another patch for this test based\non your observations above. Thanks for the suggestion. I am trying to\nget used to the development workflow of emailing patches and generally\ngetting familiar with the code base and this seems like a small but\nimportant step in the right direction.\n\nThanks\n"},{"id":"471533","messageId":"CAPig+cT-3GuGg91WDeMC__iQ7db0-1-q7-4z5jBFb19GC3E9mQ@mail.gmail.com","threadId":"59184","inReplyTo":"CAPRQfCZUwaE4mko46n0wQbkKjpg4Tqxbg6eKLm9rFrBRj6sVLg@mail.gmail.com","subject":"Re: [GSoC][PATCH] t/t5000-tar-tree: add helper function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-02-05T17:59:03Z","receivedAt":"2023-02-05T17:59:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Feb 4, 2023 at 10:17 AM Kostya Farber <kostya.farber@gmail.com> wrote:\n> On Thu, Feb 2, 2023 at 11:19 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > On Thu, Feb 2, 2023 at 6:09 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > > So, t5000 seems to be one of those relatively rare cases in which the\n> > > raw \"test\" command is more correct than the higher-level helper\n> > > functions.\n> >\n> > By the way, although the change made by this patch is probably\n> > undesirable, if you would like to try a different submission, there is\n> > a bit of modernization that could be applied to t5000. [...]\n>\n> I would be happy to help and submit another patch for this test based\n> on your observations above. Thanks for the suggestion. I am trying to\n> get used to the development workflow of emailing patches and generally\n> getting familiar with the code base and this seems like a small but\n> important step in the right direction.\n\nThe goal of the microproject isn't so much to get a change accepted\ninto the project, but rather to get experience with the workflow and\nreview process. Responding to reviewer comments, as you did, is part\nof that process, so you're doing fine.\n"}]}