[PATCH] apply: mark some file-local symbols static

[PATCH] apply: mark some file-local symbols static

[Ramsay Jones]

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---

Hi Christian,

I had intended to ask you to squash this into your 'cc/apply-am'
branch, specifically commit 4d18b33a (apply: move libified code
from builtin/apply.c to apply.{c,h}, 30-07-2016).

However, having read that commit a little closer, it seems that
you deliberately made these symbols public. The commit message
does not mention this issue at all, and it is not clear to me
why these symbols should be public.

What am I missing?

ATB,
Ramsay Jones


 apply.c | 26 +++++++++++++-------------
 apply.h | 14 --------------
 2 files changed, 13 insertions(+), 27 deletions(-)

diff --git a/apply.c b/apply.c
index 96f02fa..ec545f6 100644
--- a/apply.c
+++ b/apply.c
@@ -4743,16 +4743,16 @@ static int apply_patch(struct apply_state *state,
 	return res;
 }
 
-int apply_option_parse_exclude(const struct option *opt,
-			       const char *arg, int unset)
+static int apply_option_parse_exclude(const struct option *opt,
+				      const char *arg, int unset)
 {
 	struct apply_state *state = opt->value;
 	add_name_limit(state, arg, 1);
 	return 0;
 }
 
-int apply_option_parse_include(const struct option *opt,
-			       const char *arg, int unset)
+static int apply_option_parse_include(const struct option *opt,
+				      const char *arg, int unset)
 {
 	struct apply_state *state = opt->value;
 	add_name_limit(state, arg, 0);
@@ -4760,9 +4760,9 @@ int apply_option_parse_include(const struct option *opt,
 	return 0;
 }
 
-int apply_option_parse_p(const struct option *opt,
-			 const char *arg,
-			 int unset)
+static int apply_option_parse_p(const struct option *opt,
+				const char *arg,
+				int unset)
 {
 	struct apply_state *state = opt->value;
 	state->p_value = atoi(arg);
@@ -4770,8 +4770,8 @@ int apply_option_parse_p(const struct option *opt,
 	return 0;
 }
 
-int apply_option_parse_space_change(const struct option *opt,
-				    const char *arg, int unset)
+static int apply_option_parse_space_change(const struct option *opt,
+					   const char *arg, int unset)
 {
 	struct apply_state *state = opt->value;
 	if (unset)
@@ -4781,8 +4781,8 @@ int apply_option_parse_space_change(const struct option *opt,
 	return 0;
 }
 
-int apply_option_parse_whitespace(const struct option *opt,
-				  const char *arg, int unset)
+static int apply_option_parse_whitespace(const struct option *opt,
+					 const char *arg, int unset)
 {
 	struct apply_state *state = opt->value;
 	state->whitespace_option = arg;
@@ -4791,8 +4791,8 @@ int apply_option_parse_whitespace(const struct option *opt,
 	return 0;
 }
 
-int apply_option_parse_directory(const struct option *opt,
-				 const char *arg, int unset)
+static int apply_option_parse_directory(const struct option *opt,
+					const char *arg, int unset)
 {
 	struct apply_state *state = opt->value;
 	strbuf_reset(&state->root);
diff --git a/apply.h b/apply.h
index 66ed0c5..010726e 100644
--- a/apply.h
+++ b/apply.h
@@ -107,20 +107,6 @@ struct apply_state {
 	int applied_after_fixing_ws;
 };
 
-extern int apply_option_parse_exclude(const struct option *opt,
-				      const char *arg, int unset);
-extern int apply_option_parse_include(const struct option *opt,
-				      const char *arg, int unset);
-extern int apply_option_parse_p(const struct option *opt,
-				const char *arg,
-				int unset);
-extern int apply_option_parse_whitespace(const struct option *opt,
-					 const char *arg, int unset);
-extern int apply_option_parse_directory(const struct option *opt,
-					const char *arg, int unset);
-extern int apply_option_parse_space_change(const struct option *opt,
-					   const char *arg, int unset);
-
 extern int apply_parse_options(int argc, const char **argv,
 			       struct apply_state *state,
 			       int *force_apply, int *options,
-- 
2.9.0

Re: [PATCH] apply: mark some file-local symbols static

[Junio C Hamano]

On Tue, Aug 2, 2016 at 3:33 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>
> Hi Christian,
>
> I had intended to ask you to squash this into your 'cc/apply-am'
> branch, specifically commit 4d18b33a (apply: move libified code
> from builtin/apply.c to apply.{c,h}, 30-07-2016).
>
> However, having read that commit a little closer, it seems that
> you deliberately made these symbols public. The commit message
> does not mention this issue at all, and it is not clear to me
> why these symbols should be public.
>
> What am I missing?

Their exports have been made obsolete by the reroll we have
in 'pu' when "builtin/am: use apply api in run_apply()" was
redone in a way not to duplicate the argument parsing.

They should have been cleaned with 4820e13, but I think
Christian did not carefully review the whole series before
sending it out and did not notice that they no longer need
to be extern.

Re: [PATCH] apply: mark some file-local symbols static

[Christian Couder]

Hi Ramsay,

On Wed, Aug 3, 2016 at 12:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> On Tue, Aug 2, 2016 at 3:33 PM, Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
>>
>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> ---
>>
>> Hi Christian,
>>
>> I had intended to ask you to squash this into your 'cc/apply-am'
>> branch, specifically commit 4d18b33a (apply: move libified code
>> from builtin/apply.c to apply.{c,h}, 30-07-2016).
>>
>> However, having read that commit a little closer, it seems that
>> you deliberately made these symbols public. The commit message
>> does not mention this issue at all, and it is not clear to me
>> why these symbols should be public.
>>
>> What am I missing?

These symbols are still used in builtin/apply.c until 9f87c22 ("apply:
refactor `git apply` option parsing") at the end of the series, for
example:

$ git checkout 4d18b33a
$ git grep -n apply_option_parse_directory builtin/apply.c
builtin/apply.c:86:                     0, apply_option_parse_directory },

> Their exports have been made obsolete by the reroll we have
> in 'pu' when "builtin/am: use apply api in run_apply()" was
> redone in a way not to duplicate the argument parsing.

Yeah.

> They should have been cleaned with 4820e13,

4820e13 (apply: make some parsing functions static again) is too early
in the series for cleaning this.
At that point the symbols are still used in builtin/apply.c.

> but I think
> Christian did not carefully review the whole series before
> sending it out and did not notice that they no longer need
> to be extern.

Yeah, I did not notice that they no longer need to be extern.
Now there are different options to fix this:

1) remove the symbols in 9f87c22 ("apply: refactor `git apply` option
parsing") at the end of the series, or
2) move 4820e13 (apply: make some parsing functions static again) at
the end of the series and make it also remove them, or:
3) add another patch to remove them after 9f87c22 ("apply: refactor
`git apply` option parsing")

My preference is to do 1). This way, or if I do 3), I would not need
to resend the first 31 patches in the series.

Thanks,
Christian.

Re: [PATCH] apply: mark some file-local symbols static

[Ramsay Jones]

On 03/08/16 10:47, Christian Couder wrote:
> Hi Ramsay,
> 
> On Wed, Aug 3, 2016 at 12:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> On Tue, Aug 2, 2016 at 3:33 PM, Ramsay Jones
>> <ramsay@ramsayjones.plus.com> wrote:
>>>
>>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>>> ---
>>>
>>> Hi Christian,
>>>
[snip]

>>> What am I missing?
> 
> These symbols are still used in builtin/apply.c until 9f87c22 ("apply:
> refactor `git apply` option parsing") at the end of the series, for
> example:
> 
> $ git checkout 4d18b33a
> $ git grep -n apply_option_parse_directory builtin/apply.c
> builtin/apply.c:86:                     0, apply_option_parse_directory },
> 

Heh, thanks. I thought I had done exactly this, but I obviously
messed up!

[snip]

> Yeah, I did not notice that they no longer need to be extern.
> Now there are different options to fix this:
> 
> 1) remove the symbols in 9f87c22 ("apply: refactor `git apply` option
> parsing") at the end of the series, or
> 2) move 4820e13 (apply: make some parsing functions static again) at
> the end of the series and make it also remove them, or:
> 3) add another patch to remove them after 9f87c22 ("apply: refactor
> `git apply` option parsing")
> 
> My preference is to do 1). This way, or if I do 3), I would not need
> to resend the first 31 patches in the series.

FWIW, I would go with option #1.

ATB,
Ramsay Jones

Re: [PATCH] apply: mark some file-local symbols static

[Johannes Schindelin]

Hi Christian,

On Wed, 3 Aug 2016, Christian Couder wrote:

> Now there are different options to fix this:
> 
> 1) remove the symbols in 9f87c22 ("apply: refactor `git apply` option
> parsing") at the end of the series, or
> 2) move 4820e13 (apply: make some parsing functions static again) at
> the end of the series and make it also remove them, or:
> 3) add another patch to remove them after 9f87c22 ("apply: refactor
> `git apply` option parsing")

You forgot 4) provide fixup patches that fix the patch series.

And 5) fix the patch series, push the branch to GitHub and provide a
pointer, but not sending a new iteration unless needed otherwise.

Ciao,
Johannes

Re: [PATCH] apply: mark some file-local symbols static

[Christian Couder]

Hi Dscho,

On Wed, Aug 3, 2016 at 4:37 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Christian,
>
> On Wed, 3 Aug 2016, Christian Couder wrote:
>
>> Now there are different options to fix this:
>>
>> 1) remove the symbols in 9f87c22 ("apply: refactor `git apply` option
>> parsing") at the end of the series, or
>> 2) move 4820e13 (apply: make some parsing functions static again) at
>> the end of the series and make it also remove them, or:
>> 3) add another patch to remove them after 9f87c22 ("apply: refactor
>> `git apply` option parsing")
>
> You forgot 4) provide fixup patches that fix the patch series.
>
> And 5) fix the patch series, push the branch to GitHub and provide a
> pointer, but not sending a new iteration unless needed otherwise.

Unfortunately there are other changes, especially those suggested by
Peff, I have to make in the patch series, so I will resend everything.

Thanks,
Christian.