Re: [PATCH 01/16] submodule.h: add extern keyword to functions, break line before 80

From: Stefan Beller <sbeller@google.com>
Date: Nov 17, 2016, 18:29 UTC
Message-ID: <CAGZ79kbVq0SrD8u+GrGAVYpvNeirDqnAU9jJCbSLoEvqSgB4yg@mail.gmail.com>
In-Reply-To: <xmqqshqrp7i8.fsf@gitster.mtv.corp.google.com>

On Wed, Nov 16, 2016 at 11:08 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> submodule.h: add extern keyword to functions, break line before 80
>
> The former is probably a good change for consistency.  As the latter
> change breaks a workflow around quickly checking the output from
> "git grep funcname \*.h", I am not sure if it is a good idea.
>
> Especially things like this look like a usability regression:
>
> -void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
> +extern void handle_ignore_submodules_arg(struct diff_options *diffopt,
> +                                        const char *);
>
> Perhaps the name "diffopt" can be dropped from there if we want it
> to be shorter.

Does the Git community have an opinion on dropping the name? Usually the meaning can be inferred from the type such as in this example. "diffopt" doesn't add information to my understanding when looking at the header file.

One (cherry-picked) counter example is:

    /**
    * Like `strbuf_getwholeline`, but operates on a file descriptor.
    * It reads one character at a time, so it is very slow.  Do not
    * use it unless you need the correct position in the file
    * descriptor.
    */
    extern int strbuf_getwholeline_fd(struct strbuf *, int, int);

where I'd need a bit of time to figure out which of the 2 ints is the fd and which is the line termination character.

So I'd rather have a broken line than dropping names, as when grepping for names I'd look them up most of the time anyway (to e.g. read additional documentation or just reading the source)

> Names in prototypes are valuable for parameters of
> more generic types like "char *", especially when there are more
> than one parameters of the same type, but in this case the typename
> is specific enough for readers to tell what it is.

Ok, that seems a reasonable to me.

I'll squash the changes below, with an updated commit message:

submodule.h: add extern keyword to functions

As the upcoming series will add a lot of functions to the submodule header, let's first make the header consistent to the rest of the project by adding the extern keyword to functions.

As per the CodingGuidelines we try to stay below 80 characters per line, so adapt all those functions to stay below 80 characters that are already using more than one line. Those function using just one line are better kept in one line than breaking them up into multiple lines just for the goal of staying below the character limit as it makes grepping for functions easier if they are one liners.

(diff on top of the patch under discussion)

diff --git a/submodule.h b/submodule.h
index afc58d0..2082847 100644
--- a/submodule.h
+++ b/submodule.h
@@ -33,17 +33,14 @@ extern int is_staging_gitmodules_ok(void);
 extern int update_path_in_gitmodules(const char *oldpath, const char *newpath);
 extern int remove_path_from_gitmodules(const char *path);
 extern void stage_updated_gitmodules(void);
-extern void set_diffopt_flags_from_submodule_config(
-               struct diff_options *diffopt,
+extern void set_diffopt_flags_from_submodule_config(struct diff_options *,
                const char *path);
 extern int submodule_config(const char *var, const char *value, void *cb);
 extern void gitmodules_config(void);
 extern int parse_submodule_update_strategy(const char *value,
                struct submodule_update_strategy *dst);
-extern const char *submodule_strategy_to_string(
-               const struct submodule_update_strategy *s);
-extern void handle_ignore_submodules_arg(struct diff_options *diffopt,
-                                        const char *);
+extern const char *submodule_strategy_to_string(const struct
submodule_update_strategy *s);
+extern void handle_ignore_submodules_arg(struct diff_options *, const char *);
 extern void show_submodule_summary(FILE *f, const char *path,
                const char *line_prefix,
                struct object_id *one, struct object_id *two,
@@ -72,8 +69,7 @@ extern int find_unpushed_submodules(unsigned char
new_sha1[20],
                                    struct string_list *needs_pushing);
 extern int push_unpushed_submodules(unsigned char new_sha1[20],
                                    const char *remotes_name);
-extern void connect_work_tree_and_git_dir(const char *work_tree,
-                                         const char *git_dir);
+extern void connect_work_tree_and_git_dir(const char *work_tree,
const char *git_dir);
 extern int parallel_submodules(void);

 /*