[3/4] diff: change scope of the function count_lines()
diff mbox series

Message ID 20200702192409.21865-4-shouryashukla.oo@gmail.com
State New
Headers show
Series
  • submodule: port 'summary' from Shell to C
Related show

Commit Message

Shourya Shukla July 2, 2020, 7:24 p.m. UTC
From: Prathamesh Chavan <pc44800@gmail.com>

Change the scope of function count_lines for allowing the function
to be reused in other parts of the code as well.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 diff.c | 2 +-
 diff.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Johannes Schindelin July 3, 2020, 3:07 p.m. UTC | #1
Hi Shourya,

On Fri, 3 Jul 2020, Shourya Shukla wrote:

> From: Prathamesh Chavan <pc44800@gmail.com>
>
> Change the scope of function count_lines for allowing the function
> to be reused in other parts of the code as well.

It may be just me, but I'd rather see the word "visibility" instead of
"scope" here. I mistook the subject line to indicate that the function is
changed to serve an (at least slightly) different purpose than before,
which is not actually the case.

Another alternative to "visibility" might be to imitate existing commit
messages, such as e4cb659ebdd (diff: export diffstat interface,
2019-11-13), 22184497a36 (factor out refresh_and_write_cache function,
2019-09-11) or ef283b3699f (apply: make parse_git_diff_header public,
2019-07-11).

In addition, as with all such changes, we need to be careful to consider
whether unrelated function names coming in from system headers might
clash. In this case, I think `count_lines()` is a bit too generic, but
will probably not clash. Personally, I would probably have opted for
`count_lines_in_string()`.

Ciao,
Johannes

>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
>  diff.c | 2 +-
>  diff.h | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/diff.c b/diff.c
> index 4a2c631c37..39ddbcf451 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -547,7 +547,7 @@ struct emit_callback {
>  	struct strbuf *header;
>  };
>
> -static int count_lines(const char *data, int size)
> +int count_lines(const char *data, int size)
>  {
>  	int count, ch, completely_empty = 1, nl_just_seen = 0;
>  	count = 0;
> diff --git a/diff.h b/diff.h
> index 9443dc1b00..bd96c8c434 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -495,6 +495,7 @@ void free_diffstat_info(struct diffstat_t *diffstat);
>  int parse_long_opt(const char *opt, const char **argv,
>  		   const char **optarg);
>
> +int count_lines(const char *data, int size);
>  int git_diff_basic_config(const char *var, const char *value, void *cb);
>  int git_diff_heuristic_config(const char *var, const char *value, void *cb);
>  void init_diff_ui_defaults(void);
> --
> 2.27.0
>
>
Shourya Shukla July 4, 2020, 3:28 p.m. UTC | #2
On 03/07 05:07, Johannes Schindelin wrote:
> Hi Shourya,
> 
> On Fri, 3 Jul 2020, Shourya Shukla wrote:
> 
> > From: Prathamesh Chavan <pc44800@gmail.com>
> >
> > Change the scope of function count_lines for allowing the function
> > to be reused in other parts of the code as well.
> 
> It may be just me, but I'd rather see the word "visibility" instead of
> "scope" here. I mistook the subject line to indicate that the function is
> changed to serve an (at least slightly) different purpose than before,
> which is not actually the case.
> 
> Another alternative to "visibility" might be to imitate existing commit
> messages, such as e4cb659ebdd (diff: export diffstat interface,
> 2019-11-13), 22184497a36 (factor out refresh_and_write_cache function,
> 2019-09-11) or ef283b3699f (apply: make parse_git_diff_header public,
> 2019-07-11).

These are some excellent commit messages! I will take inspiration from
these. Thanks Dscho! :)

> In addition, as with all such changes, we need to be careful to consider
> whether unrelated function names coming in from system headers might
> clash. In this case, I think `count_lines()` is a bit too generic, but
> will probably not clash. Personally, I would probably have opted for
> `count_lines_in_string()`.

I do not see it clashing with any other functions. Do we need to change
the name still (maybe to make the functions purpose even more clear)?
Christian Couder July 4, 2020, 9:46 p.m. UTC | #3
On Sat, Jul 4, 2020 at 5:28 PM Shourya Shukla
<shouryashukla.oo@gmail.com> wrote:
>
> On 03/07 05:07, Johannes Schindelin wrote:

> > In addition, as with all such changes, we need to be careful to consider
> > whether unrelated function names coming in from system headers might
> > clash. In this case, I think `count_lines()` is a bit too generic, but
> > will probably not clash. Personally, I would probably have opted for
> > `count_lines_in_string()`.
>
> I do not see it clashing with any other functions. Do we need to change
> the name still (maybe to make the functions purpose even more clear)?

This patch might not be needed at all according to Dscho's review of
another patch in this series.

Patch
diff mbox series

diff --git a/diff.c b/diff.c
index 4a2c631c37..39ddbcf451 100644
--- a/diff.c
+++ b/diff.c
@@ -547,7 +547,7 @@  struct emit_callback {
 	struct strbuf *header;
 };
 
-static int count_lines(const char *data, int size)
+int count_lines(const char *data, int size)
 {
 	int count, ch, completely_empty = 1, nl_just_seen = 0;
 	count = 0;
diff --git a/diff.h b/diff.h
index 9443dc1b00..bd96c8c434 100644
--- a/diff.h
+++ b/diff.h
@@ -495,6 +495,7 @@  void free_diffstat_info(struct diffstat_t *diffstat);
 int parse_long_opt(const char *opt, const char **argv,
 		   const char **optarg);
 
+int count_lines(const char *data, int size);
 int git_diff_basic_config(const char *var, const char *value, void *cb);
 int git_diff_heuristic_config(const char *var, const char *value, void *cb);
 void init_diff_ui_defaults(void);