Re: [PATCH v2 1/6] submodules: add helper functions to determine presence of submodules

Stefan Beller [off-list ref] writes:

Overall the suggestions from you in this review is good and please
consider anything I did not mention I agree with you.  Thanks.

quoted

+extern int is_submodule_initialized(const char *path);
+extern int is_submodule_checked_out(const char *path);

no need to put extern for function names. (no other functions in this
header are extern. so local consistency maybe? I'd also claim that
all other extern functions in headers ought to be declared without
being extern)

Maybe I am old fashioned, but I'd feel better to see these with
explicit "extern" in front (check the older header files like
cache.h when you are in doubt what the project convention has been).

On 11/01, Junio C Hamano wrote:

Stefan Beller [off-list ref] writes:

Overall the suggestions from you in this review is good and please
consider anything I did not mention I agree with you.  Thanks.

quoted

quoted

+extern int is_submodule_initialized(const char *path);
+extern int is_submodule_checked_out(const char *path);

no need to put extern for function names. (no other functions in this
header are extern. so local consistency maybe? I'd also claim that
all other extern functions in headers ought to be declared without
being extern)

Maybe I am old fashioned, but I'd feel better to see these with
explicit "extern" in front (check the older header files like
cache.h when you are in doubt what the project convention has been).

I wouldn't consider that old fashion as I'm fairly new to all this and
I also prefer the explicit "extern" :P

-- 
Brandon Williams

On Tue, Nov 1, 2016 at 10:20 AM, Junio C Hamano [off-list ref] wrote:

Maybe I am old fashioned, but I'd feel better to see these with
explicit "extern" in front (check the older header files like
cache.h when you are in doubt what the project convention has been).

I did check the other files and saw them, so I was very unsure what to
suggest here. I only saw the extern keyword used in headers that were
there when Git was really young, so I assumed it's a style nit by kernel
developers. Thanks for clarifying!

I think we'll want to have some consistency though, so we
maybe want to coordinate a cleanup of submodule.h as well as
submodule-config.h to mark all the functions extern.

This doesn't need to be a all-at-once thing, but we'd keep it in mind
for future declarations in the header.

Thanks,
Stefan

On Tue, Nov 1, 2016 at 10:31 AM, Stefan Beller [off-list ref] wrote:

On Tue, Nov 1, 2016 at 10:20 AM, Junio C Hamano [off-list ref] wrote:

quoted

Maybe I am old fashioned, but I'd feel better to see these with
explicit "extern" in front (check the older header files like
cache.h when you are in doubt what the project convention has been).

I did check the other files and saw them, so I was very unsure what to
suggest here. I only saw the extern keyword used in headers that were
there when Git was really young, so I assumed it's a style nit by kernel
developers. Thanks for clarifying!

I think we'll want to have some consistency though, so we
maybe want to coordinate a cleanup of submodule.h as well as
submodule-config.h to mark all the functions extern.

This doesn't need to be a all-at-once thing, but we'd keep it in mind
for future declarations in the header.

Thanks,
Stefan

Extern is generally used when you want to declare a header for a
function that's in a different object file. I'm not sure if we
actually need it or not though.

Thanks,
Jake