threads / patch / 42989

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

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

## tl;dr

6 messages between Aug 2, 2016 and Aug 7, 2016. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Ramsay Jones· Aug 2, 2016, 22:33 UTC · lore

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

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(-)
Show changes to 2 files +13 −27

apply.c, apply.h

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
Junio C Hamano· Aug 2, 2016, 22:44 UTC · re: Ramsay Jones · lore

On Tue, Aug 2, 2016 at 3:33 PM, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:

Show 16 quoted lines
>
> 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.

Christian Couder· Aug 3, 2016, 09:47 UTC · re: Junio C Hamano · lore
Hi Ramsay,
On Wed, Aug 3, 2016 at 12:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> 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.

Ramsay Jones· Aug 3, 2016, 13:47 UTC · re: Christian Couder · lore
On 03/08/16 10:47, Christian Couder wrote:
Show 11 quoted lines
> 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]
Show 10 quoted lines
>>> 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]
Show 12 quoted lines
> 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

Johannes Schindelin· Aug 3, 2016, 14:37 UTC · re: Christian Couder · lore
Hi Christian,
On Wed, 3 Aug 2016, Christian Couder wrote:
Show 8 quoted lines
> 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

Christian Couder· Aug 7, 2016, 16:37 UTC · re: Johannes Schindelin · lore
Hi Dscho,

On Wed, Aug 3, 2016 at 4:37 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 17 quoted lines
> 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.

← back to recent threads