* [PATCH] apply: mark some file-local symbols static
@ 2016-08-02 22:33 Ramsay Jones
2016-08-02 22:44 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Ramsay Jones @ 2016-08-02 22:33 UTC (permalink / raw)
To: Christian Couder; +Cc: Junio C Hamano, GIT Mailing-list
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] apply: mark some file-local symbols static
2016-08-02 22:33 [PATCH] apply: mark some file-local symbols static Ramsay Jones
@ 2016-08-02 22:44 ` Junio C Hamano
2016-08-03 9:47 ` Christian Couder
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2016-08-02 22:44 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Christian Couder, GIT Mailing-list
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] apply: mark some file-local symbols static
2016-08-02 22:44 ` Junio C Hamano
@ 2016-08-03 9:47 ` Christian Couder
2016-08-03 13:47 ` Ramsay Jones
2016-08-03 14:37 ` Johannes Schindelin
0 siblings, 2 replies; 6+ messages in thread
From: Christian Couder @ 2016-08-03 9:47 UTC (permalink / raw)
To: Junio C Hamano, Ramsay Jones; +Cc: GIT Mailing-list
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] apply: mark some file-local symbols static
2016-08-03 9:47 ` Christian Couder
@ 2016-08-03 13:47 ` Ramsay Jones
2016-08-03 14:37 ` Johannes Schindelin
1 sibling, 0 replies; 6+ messages in thread
From: Ramsay Jones @ 2016-08-03 13:47 UTC (permalink / raw)
To: Christian Couder, Junio C Hamano; +Cc: GIT Mailing-list
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] apply: mark some file-local symbols static
2016-08-03 9:47 ` Christian Couder
2016-08-03 13:47 ` Ramsay Jones
@ 2016-08-03 14:37 ` Johannes Schindelin
2016-08-07 16:37 ` Christian Couder
1 sibling, 1 reply; 6+ messages in thread
From: Johannes Schindelin @ 2016-08-03 14:37 UTC (permalink / raw)
To: Christian Couder; +Cc: Junio C Hamano, Ramsay Jones, GIT Mailing-list
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] apply: mark some file-local symbols static
2016-08-03 14:37 ` Johannes Schindelin
@ 2016-08-07 16:37 ` Christian Couder
0 siblings, 0 replies; 6+ messages in thread
From: Christian Couder @ 2016-08-07 16:37 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Ramsay Jones, GIT Mailing-list
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-08-07 16:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-02 22:33 [PATCH] apply: mark some file-local symbols static Ramsay Jones
2016-08-02 22:44 ` Junio C Hamano
2016-08-03 9:47 ` Christian Couder
2016-08-03 13:47 ` Ramsay Jones
2016-08-03 14:37 ` Johannes Schindelin
2016-08-07 16:37 ` Christian Couder
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).