Re: [PATCH] submodule-parallel-fetch: make some file local symbols static

[Junio C Hamano]

Ramsay Jones  writes:

>> I thought I had this in yesterdays reroll (v6). Oh you're referring to
>> the version
>> from the 28th (I forgot to label them v5 I suppose).
>
> Ah! I thought I'd seen it on the list. (I thought I was going crazy) ;-)
> Sorry, my fault. I just assumed that today's pu branch would have your
> latest patches - I didn't actually check that.

Sometimes I take a one-day vacation ;-)

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] submodule-parallel-fetch: make some file local symbols static

[Ramsay Jones]

Commits 0fc1fdb0 ("fetch_populated_submodules: use new parallel job
processing", 28-09-2015) and 60f24f52 ("run-command: add an asynchronous
parallel child processor", 28-09-2015) both introduce external symbols
which only require file scope visibility. In order to reduce the
visibility, apply the static keyword to their declarations.

Signed-off-by: Ramsay Jones 
---

Hi Stefan,

No, despite the same subject, this is not the same patch that I sent
you last week! :-D

Could you please squash parts of this into the patches corresponding
to the above mentioned commits.

Thanks!

BTW, I would once again suggest that you could move the definition of
get_next_submodule() to be above/before fetch_populated_submodules()
so that you can remove the forward declaration.

ATB,
Ramsay Jones

 run-command.c | 2 +-
 submodule.c   | 8 
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/run-command.c b/run-command.c
index 341b23b..347d22e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -865,7 +865,7 @@ int capture_command(struct child_process *cmd, struct 
strbuf *buf, size_t hint)
return finish_command(cmd);
 }
 
-struct parallel_processes {
+static struct parallel_processes {
void *data;
 
int max_processes;
diff --git a/submodule.c b/submodule.c
index bd6e208..638efb5 100644
--- a/submodule.c
+++ b/submodule.c
@@ -622,8 +622,8 @@ struct submodule_parallel_fetch {
 };
 #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0}
 
-int get_next_submodule(void *data, struct child_process *cp,
-  struct strbuf *err);
+static int get_next_submodule(void *data, struct child_process *cp,
+ struct strbuf *err);
 
 static int fetch_start_failure(void *data, struct child_process *cp,
   struct strbuf *err)
@@ -682,8 +682,8 @@ out:
return spf.result;
 }
 
-int get_next_submodule(void *data, struct child_process *cp,
-  struct strbuf *err)
+static int get_next_submodule(void *data, struct child_process *cp,
+ struct strbuf *err)
 {
int ret = 0;
struct submodule_parallel_fetch *spf = data;
-- 
2.6.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] submodule-parallel-fetch: make some file local symbols static

[Stefan Beller]

On Thu, Oct 1, 2015 at 5:02 AM, Ramsay Jones
 wrote:
>
> Commits 0fc1fdb0 ("fetch_populated_submodules: use new parallel job
> processing", 28-09-2015) and 60f24f52 ("run-command: add an asynchronous
> parallel child processor", 28-09-2015) both introduce external symbols
> which only require file scope visibility. In order to reduce the
> visibility, apply the static keyword to their declarations.
>
> Signed-off-by: Ramsay Jones 
> ---
>
> Hi Stefan,
>
> No, despite the same subject, this is not the same patch that I sent
> you last week! :-D
>
> Could you please squash parts of this into the patches corresponding
> to the above mentioned commits.

I am sorry for the need to send this second patch. :(

>
> Thanks!
>
> BTW, I would once again suggest that you could move the definition of
> get_next_submodule() to be above/before fetch_populated_submodules()
> so that you can remove the forward declaration.
>
> ATB,
> Ramsay Jones
>
>  run-command.c | 2 +-
>  submodule.c   | 8 
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 341b23b..347d22e 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -865,7 +865,7 @@ int capture_command(struct child_process *cmd, struct 
> strbuf *buf, size_t hint)
> return finish_command(cmd);
>  }
>
> -struct parallel_processes {
> +static struct parallel_processes {

will pickup in a reroll

> void *data;
>
> int max_processes;
> diff --git a/submodule.c b/submodule.c
> index bd6e208..638efb5 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -622,8 +622,8 @@ struct submodule_parallel_fetch {
>  };
>  #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0}
>
> -int get_next_submodule(void *data, struct child_process *cp,
> -  struct strbuf *err);
> +static int get_next_submodule(void *data, struct child_process *cp,
> + struct strbuf *err);

I thought I had this in yesterdays reroll (v6). Oh you're referring to
the version
from the 28th (I forgot to label them v5 I suppose).

I will also get rid of the forward declaration.

>
>  static int fetch_start_failure(void *data, struct child_process *cp,
>struct strbuf *err)
> @@ -682,8 +682,8 @@ out:
> return spf.result;
>  }
>
> -int get_next_submodule(void *data, struct child_process *cp,
> -  struct strbuf *err)
> +static int get_next_submodule(void *data, struct child_process *cp,
> + struct strbuf *err)
>  {
> int ret = 0;
> struct submodule_parallel_fetch *spf = data;
> --
> 2.6.0
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] submodule-parallel-fetch: make some file local symbols static

[Ramsay Jones]

On 01/10/15 18:05, Stefan Beller wrote:
> On Thu, Oct 1, 2015 at 5:02 AM, Ramsay Jones
>  wrote:
>>
[snip]

>> diff --git a/run-command.c b/run-command.c
>> index 341b23b..347d22e 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -865,7 +865,7 @@ int capture_command(struct child_process *cmd, struct 
>> strbuf *buf, size_t hint)
>> return finish_command(cmd);
>>  }
>>
>> -struct parallel_processes {
>> +static struct parallel_processes {
> 
> will pickup in a reroll

Thanks

> 
>> void *data;
>>
>> int max_processes;
>> diff --git a/submodule.c b/submodule.c
>> index bd6e208..638efb5 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -622,8 +622,8 @@ struct submodule_parallel_fetch {
>>  };
>>  #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0}
>>
>> -int get_next_submodule(void *data, struct child_process *cp,
>> -  struct strbuf *err);
>> +static int get_next_submodule(void *data, struct child_process *cp,
>> + struct strbuf *err);
> 
> I thought I had this in yesterdays reroll (v6). Oh you're referring to
> the version
> from the 28th (I forgot to label them v5 I suppose).

Ah! I thought I'd seen it on the list. (I thought I was going crazy) ;-)
Sorry, my fault. I just assumed that today's pu branch would have your
latest patches - I didn't actually check that.

Note that the first hunk, above, is actually new (I hadn't noticed it
before).

> 
> I will also get rid of the forward declaration.

Thanks!

ATB,
Ramsay Jones

> 
>>
>>  static int fetch_start_failure(void *data, struct child_process *cp,
>>struct strbuf *err)
>> @@ -682,8 +682,8 @@ out:
>> return spf.result;
>>  }
>>
>> -int get_next_submodule(void *data, struct child_process *cp,
>> -  struct strbuf *err)
>> +static int get_next_submodule(void *data, struct child_process *cp,
>> + struct strbuf *err)
>>  {
>> int ret = 0;
>> struct submodule_parallel_fetch *spf = data;
>> --

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] submodule-parallel-fetch: make some file local symbols static

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Stefan,

When you next re-roll the patches for your
'sb/submodule-parallel-fetch' branch, could you please squash
parts of this into the relevant patches. [which would correspond
to commits a93fb7a ("run-command: add an asynchronous parallel
child processor", 22-09-2015) and 58713c8 ("fetch_populated_submodules:
use new parallel job processing", 22-09-2015).]

Thanks!

Also, you might consider renaming some (file local) functions with
really long names, to something a little shorter, like (say):

  handle_submodule_fetch_start_err -> fetch_start_failure
  handle_submodule_fetch_finish -> fetch_finish

(but, as I have said several times, I'm not good at naming things ... ;-)

Also, you could move the definition of get_next_submodule() to be
above/before fetch_populated_submodules() so that you can remove the
forward declaration.

[Note these issues were spotted by sparse and static-check.pl]

HTH

ATB,
Ramsay Jones


 run-command.c  | 12 ++--
 submodule.c| 12 ++--
 test-run-command.c |  6 +++---
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/run-command.c b/run-command.c
index 528a4fb..6ca0151 100644
--- a/run-command.c
+++ b/run-command.c
@@ -902,9 +902,9 @@ struct parallel_processes {
struct strbuf buffered_output; /* of finished children */
 };
 
-void default_start_failure(void *data,
-  struct child_process *cp,
-  struct strbuf *err)
+static void default_start_failure(void *data,
+ struct child_process *cp,
+ struct strbuf *err)
 {
int i;
struct strbuf sb = STRBUF_INIT;
@@ -915,9 +915,9 @@ void default_start_failure(void *data,
die_errno("Starting a child failed:%s", sb.buf);
 }
 
-void default_return_value(void *data,
- struct child_process *cp,
- int result)
+static void default_return_value(void *data,
+struct child_process *cp,
+int result)
 {
int i;
struct strbuf sb = STRBUF_INIT;
diff --git a/submodule.c b/submodule.c
index f362d6a..d786a76 100644
--- a/submodule.c
+++ b/submodule.c
@@ -620,16 +620,16 @@ struct submodule_parallel_fetch {
 };
 #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0}
 
-int get_next_submodule(void *data, struct child_process *cp,
-  struct strbuf *err);
+static int get_next_submodule(void *data, struct child_process *cp,
+ struct strbuf *err);
 
-void handle_submodule_fetch_start_err(void *data, struct child_process *cp, 
struct strbuf *err)
+static void handle_submodule_fetch_start_err(void *data, struct child_process 
*cp, struct strbuf *err)
 {
struct submodule_parallel_fetch *spf = data;
spf->result = 1;
 }
 
-void handle_submodule_fetch_finish( void *data, struct child_process *cp, int 
retvalue)
+static void handle_submodule_fetch_finish( void *data, struct child_process 
*cp, int retvalue)
 {
struct submodule_parallel_fetch *spf = data;
 
@@ -673,8 +673,8 @@ out:
return spf.result;
 }
 
-int get_next_submodule(void *data, struct child_process *cp,
-  struct strbuf *err)
+static int get_next_submodule(void *data, struct child_process *cp,
+ struct strbuf *err)
 {
int ret = 0;
struct submodule_parallel_fetch *spf = data;
diff --git a/test-run-command.c b/test-run-command.c
index 94c6eee..2555791 100644
--- a/test-run-command.c
+++ b/test-run-command.c
@@ -16,9 +16,9 @@
 #include 
 
 static int number_callbacks;
-int parallel_next(void *data,
- struct child_process *cp,
- struct strbuf *err)
+static int parallel_next(void *data,
+struct child_process *cp,
+struct strbuf *err)
 {
struct child_process *d = data;
if (number_callbacks >= 4)
-- 
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] submodule-parallel-fetch: make some file local symbols static

[Jacob Keller]

On Fri, Sep 25, 2015 at 8:15 AM, Ramsay Jones
 wrote:
> [Note these issues were spotted by sparse and static-check.pl]

Any chance you got a link to docs about static-check.pl? I've never
heard of this...

Regards,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html