| Message ID | 20200702192409.21865-4-shouryashukla.oo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | submodule: port 'summary' from Shell to C | expand |
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 > >
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)?
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.
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);