{"thread":{"id":"42989","subject":"Re: [PATCH] apply: mark some file-local symbols static","startedAt":"2016-08-02T22:45:56Z","lastAt":"2016-08-07T16:37:10Z","messageCount":6,"participants":["Junio C Hamano","Ramsay Jones","Christian Couder","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"292877","messageId":"CAPc5daVo2o7zxTGVARoZc8AmX2WAJC1cWB4R=9sG8TH8ddKkuA@mail.gmail.com","threadId":"42989","inReplyTo":"a2b34209-2244-d498-6ed2-4f9fbf9f7ed1@ramsayjones.plus.com","subject":"Re: [PATCH] apply: mark some file-local symbols static","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-02T22:44:45Z","receivedAt":"2016-08-02T22:45:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, Aug 2, 2016 at 3:33 PM, Ramsay Jones\n wrote:\n>\n> Signed-off-by: Ramsay Jones \n> ---\n>\n> Hi Christian,\n>\n> I had intended to ask you to squash this into your 'cc/apply-am'\n> branch, specifically commit 4d18b33a (apply: move libified code\n> from builtin/apply.c to apply.{c,h}, 30-07-2016).\n>\n> However, having read that commit a little closer, it seems that\n> you deliberately made these symbols public. The commit message\n> does not mention this issue at all, and it is not clear to me\n> why these symbols should be public.\n>\n> What am I missing?\n\nTheir exports have been made obsolete by the reroll we have\nin 'pu' when \"builtin/am: use apply api in run_apply()\" was\nredone in a way not to duplicate the argument parsing.\n\nThey should have been cleaned with 4820e13, but I think\nChristian did not carefully review the whole series before\nsending it out and did not notice that they no longer need\nto be extern.\n"},{"id":"292878","messageId":"a2b34209-2244-d498-6ed2-4f9fbf9f7ed1@ramsayjones.plus.com","threadId":"42989","inReplyTo":null,"subject":"[PATCH] apply: mark some file-local symbols static","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-08-02T22:33:11Z","receivedAt":"2016-08-02T22:46:08Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSigned-off-by: Ramsay Jones \n---\n\nHi Christian,\n\nI had intended to ask you to squash this into your 'cc/apply-am'\nbranch, specifically commit 4d18b33a (apply: move libified code\nfrom builtin/apply.c to apply.{c,h}, 30-07-2016).\n\nHowever, having read that commit a little closer, it seems that\nyou deliberately made these symbols public. The commit message\ndoes not mention this issue at all, and it is not clear to me\nwhy these symbols should be public.\n\nWhat am I missing?\n\nATB,\nRamsay Jones\n\n\n apply.c | 26 +++++++++++++-------------\n apply.h | 14 --------------\n 2 files changed, 13 insertions(+), 27 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 96f02fa..ec545f6 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4743,16 +4743,16 @@ static int apply_patch(struct apply_state *state,\n \treturn res;\n }\n \n-int apply_option_parse_exclude(const struct option *opt,\n-\t\t\t const char *arg, int unset)\n+static int apply_option_parse_exclude(const struct option *opt,\n+\t\t\t\t const char *arg, int unset)\n {\n \tstruct apply_state *state = opt->value;\n \tadd_name_limit(state, arg, 1);\n \treturn 0;\n }\n \n-int apply_option_parse_include(const struct option *opt,\n-\t\t\t const char *arg, int unset)\n+static int apply_option_parse_include(const struct option *opt,\n+\t\t\t\t const char *arg, int unset)\n {\n \tstruct apply_state *state = opt->value;\n \tadd_name_limit(state, arg, 0);\n@@ -4760,9 +4760,9 @@ int apply_option_parse_include(const struct option *opt,\n \treturn 0;\n }\n \n-int apply_option_parse_p(const struct option *opt,\n-\t\t\t const char *arg,\n-\t\t\t int unset)\n+static int apply_option_parse_p(const struct option *opt,\n+\t\t\t\tconst char *arg,\n+\t\t\t\tint unset)\n {\n \tstruct apply_state *state = opt->value;\n \tstate->p_value = atoi(arg);\n@@ -4770,8 +4770,8 @@ int apply_option_parse_p(const struct option *opt,\n \treturn 0;\n }\n \n-int apply_option_parse_space_change(const struct option *opt,\n-\t\t\t\t const char *arg, int unset)\n+static int apply_option_parse_space_change(const struct option *opt,\n+\t\t\t\t\t const char *arg, int unset)\n {\n \tstruct apply_state *state = opt->value;\n \tif (unset)\n@@ -4781,8 +4781,8 @@ int apply_option_parse_space_change(const struct option *opt,\n \treturn 0;\n }\n \n-int apply_option_parse_whitespace(const struct option *opt,\n-\t\t\t\t const char *arg, int unset)\n+static int apply_option_parse_whitespace(const struct option *opt,\n+\t\t\t\t\t const char *arg, int unset)\n {\n \tstruct apply_state *state = opt->value;\n \tstate->whitespace_option = arg;\n@@ -4791,8 +4791,8 @@ int apply_option_parse_whitespace(const struct option *opt,\n \treturn 0;\n }\n \n-int apply_option_parse_directory(const struct option *opt,\n-\t\t\t\t const char *arg, int unset)\n+static int apply_option_parse_directory(const struct option *opt,\n+\t\t\t\t\tconst char *arg, int unset)\n {\n \tstruct apply_state *state = opt->value;\n \tstrbuf_reset(&state->root);\ndiff --git a/apply.h b/apply.h\nindex 66ed0c5..010726e 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -107,20 +107,6 @@ struct apply_state {\n \tint applied_after_fixing_ws;\n };\n \n-extern int apply_option_parse_exclude(const struct option *opt,\n-\t\t\t\t const char *arg, int unset);\n-extern int apply_option_parse_include(const struct option *opt,\n-\t\t\t\t const char *arg, int unset);\n-extern int apply_option_parse_p(const struct option *opt,\n-\t\t\t\tconst char *arg,\n-\t\t\t\tint unset);\n-extern int apply_option_parse_whitespace(const struct option *opt,\n-\t\t\t\t\t const char *arg, int unset);\n-extern int apply_option_parse_directory(const struct option *opt,\n-\t\t\t\t\tconst char *arg, int unset);\n-extern int apply_option_parse_space_change(const struct option *opt,\n-\t\t\t\t\t const char *arg, int unset);\n-\n extern int apply_parse_options(int argc, const char **argv,\n \t\t\t struct apply_state *state,\n \t\t\t int *force_apply, int *options,\n-- \n2.9.0\n\n"},{"id":"292891","messageId":"CAP8UFD2NFZCAF=ZWNf4YF+F2L7A4ZM0XS98o-cR_MjuLLR8pfg@mail.gmail.com","threadId":"42989","inReplyTo":"CAPc5daVo2o7zxTGVARoZc8AmX2WAJC1cWB4R=9sG8TH8ddKkuA@mail.gmail.com","subject":"Re: [PATCH] apply: mark some file-local symbols static","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-08-03T09:47:14Z","receivedAt":"2016-08-03T10:47:04Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Ramsay,\n\nOn Wed, Aug 3, 2016 at 12:44 AM, Junio C Hamano wrote:\n> On Tue, Aug 2, 2016 at 3:33 PM, Ramsay Jones\n> wrote:\n>>\n>> Signed-off-by: Ramsay Jones \n>> ---\n>>\n>> Hi Christian,\n>>\n>> I had intended to ask you to squash this into your 'cc/apply-am'\n>> branch, specifically commit 4d18b33a (apply: move libified code\n>> from builtin/apply.c to apply.{c,h}, 30-07-2016).\n>>\n>> However, having read that commit a little closer, it seems that\n>> you deliberately made these symbols public. The commit message\n>> does not mention this issue at all, and it is not clear to me\n>> why these symbols should be public.\n>>\n>> What am I missing?\n\nThese symbols are still used in builtin/apply.c until 9f87c22 (\"apply:\nrefactor `git apply` option parsing\") at the end of the series, for\nexample:\n\n$ git checkout 4d18b33a\n$ git grep -n apply_option_parse_directory builtin/apply.c\nbuiltin/apply.c:86: 0, apply_option_parse_directory },\n\n> Their exports have been made obsolete by the reroll we have\n> in 'pu' when \"builtin/am: use apply api in run_apply()\" was\n> redone in a way not to duplicate the argument parsing.\n\nYeah.\n\n> They should have been cleaned with 4820e13,\n\n4820e13 (apply: make some parsing functions static again) is too early\nin the series for cleaning this.\nAt that point the symbols are still used in builtin/apply.c.\n\n> but I think\n> Christian did not carefully review the whole series before\n> sending it out and did not notice that they no longer need\n> to be extern.\n\nYeah, I did not notice that they no longer need to be extern.\nNow there are different options to fix this:\n\n1) remove the symbols in 9f87c22 (\"apply: refactor `git apply` option\nparsing\") at the end of the series, or\n2) move 4820e13 (apply: make some parsing functions static again) at\nthe end of the series and make it also remove them, or:\n3) add another patch to remove them after 9f87c22 (\"apply: refactor\n`git apply` option parsing\")\n\nMy preference is to do 1). This way, or if I do 3), I would not need\nto resend the first 31 patches in the series.\n\nThanks,\nChristian.\n"},{"id":"292898","messageId":"2e615a57-cf15-9deb-0a35-d6bbe175edfa@ramsayjones.plus.com","threadId":"42989","inReplyTo":"CAP8UFD2NFZCAF=ZWNf4YF+F2L7A4ZM0XS98o-cR_MjuLLR8pfg@mail.gmail.com","subject":"Re: [PATCH] apply: mark some file-local symbols static","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-08-03T13:47:41Z","receivedAt":"2016-08-03T13:47:49Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 03/08/16 10:47, Christian Couder wrote:\n> Hi Ramsay,\n> \n> On Wed, Aug 3, 2016 at 12:44 AM, Junio C Hamano wrote:\n>> On Tue, Aug 2, 2016 at 3:33 PM, Ramsay Jones\n>> wrote:\n>>>\n>>> Signed-off-by: Ramsay Jones \n>>> ---\n>>>\n>>> Hi Christian,\n>>>\n[snip]\n\n>>> What am I missing?\n> \n> These symbols are still used in builtin/apply.c until 9f87c22 (\"apply:\n> refactor `git apply` option parsing\") at the end of the series, for\n> example:\n> \n> $ git checkout 4d18b33a\n> $ git grep -n apply_option_parse_directory builtin/apply.c\n> builtin/apply.c:86: 0, apply_option_parse_directory },\n> \n\nHeh, thanks. I thought I had done exactly this, but I obviously\nmessed up!\n\n[snip]\n\n> Yeah, I did not notice that they no longer need to be extern.\n> Now there are different options to fix this:\n> \n> 1) remove the symbols in 9f87c22 (\"apply: refactor `git apply` option\n> parsing\") at the end of the series, or\n> 2) move 4820e13 (apply: make some parsing functions static again) at\n> the end of the series and make it also remove them, or:\n> 3) add another patch to remove them after 9f87c22 (\"apply: refactor\n> `git apply` option parsing\")\n> \n> My preference is to do 1). This way, or if I do 3), I would not need\n> to resend the first 31 patches in the series.\n\nFWIW, I would go with option #1.\n\nATB,\nRamsay Jones\n\n\n"},{"id":"292931","messageId":"alpine.DEB.2.20.1608031635570.107993@virtualbox","threadId":"42989","inReplyTo":"CAP8UFD2NFZCAF=ZWNf4YF+F2L7A4ZM0XS98o-cR_MjuLLR8pfg@mail.gmail.com","subject":"Re: [PATCH] apply: mark some file-local symbols static","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-03T14:37:35Z","receivedAt":"2016-08-03T17:05:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Christian,\n\nOn Wed, 3 Aug 2016, Christian Couder wrote:\n\n> Now there are different options to fix this:\n> \n> 1) remove the symbols in 9f87c22 (\"apply: refactor `git apply` option\n> parsing\") at the end of the series, or\n> 2) move 4820e13 (apply: make some parsing functions static again) at\n> the end of the series and make it also remove them, or:\n> 3) add another patch to remove them after 9f87c22 (\"apply: refactor\n> `git apply` option parsing\")\n\nYou forgot 4) provide fixup patches that fix the patch series.\n\nAnd 5) fix the patch series, push the branch to GitHub and provide a\npointer, but not sending a new iteration unless needed otherwise.\n\nCiao,\nJohannes\n"},{"id":"293325","messageId":"CAP8UFD3MN0-Eq+BRrJaFBWX7YjgVPiEmPMU=BL0K_FVtaZppTw@mail.gmail.com","threadId":"42989","inReplyTo":"alpine.DEB.2.20.1608031635570.107993@virtualbox","subject":"Re: [PATCH] apply: mark some file-local symbols static","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-08-07T16:37:03Z","receivedAt":"2016-08-07T16:37:10Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Dscho,\n\nOn Wed, Aug 3, 2016 at 4:37 PM, Johannes Schindelin\n wrote:\n> Hi Christian,\n>\n> On Wed, 3 Aug 2016, Christian Couder wrote:\n>\n>> Now there are different options to fix this:\n>>\n>> 1) remove the symbols in 9f87c22 (\"apply: refactor `git apply` option\n>> parsing\") at the end of the series, or\n>> 2) move 4820e13 (apply: make some parsing functions static again) at\n>> the end of the series and make it also remove them, or:\n>> 3) add another patch to remove them after 9f87c22 (\"apply: refactor\n>> `git apply` option parsing\")\n>\n> You forgot 4) provide fixup patches that fix the patch series.\n>\n> And 5) fix the patch series, push the branch to GitHub and provide a\n> pointer, but not sending a new iteration unless needed otherwise.\n\nUnfortunately there are other changes, especially those suggested by\nPeff, I have to make in the patch series, so I will resend everything.\n\nThanks,\nChristian.\n"}]}