On Thu, Feb 20, 2020 at 12:07:46PM -0800, Junio C Hamano wrote:
> Emily Shaffer <emilyshaffer@xxxxxxxxxx> writes:
>
> > +static void get_system_info(struct strbuf *sys_info)
> > +{
> > + /* get git version from native cmd */
> > + strbuf_addstr(sys_info, "git version:\n");
> > + get_version_info(sys_info, 1);
> > + strbuf_complete_line(sys_info);
>
> It is a bit curious use of "don't do anything if sys_info ends with
> a complete line, but complete it if it ends with an imcomplete
> line". That tells the readers that we do not know what
> get_version_info() will do (now or in the future) to its output
> buffer.
>
> > +}
> > ...
> > diff --git a/help.c b/help.c
> > index 190722fb0a..44cee69c11 100644
> > --- a/help.c
> > +++ b/help.c
> > @@ -622,8 +622,33 @@ const char *help_unknown_cmd(const char *cmd)
> > exit(1);
> > }
> >
> > +void get_version_info(struct strbuf *buf, int show_build_options)
> > +{
> > + /*
> > + * The format of this string should be kept stable for compatibility
> > + * with external projects that rely on the output of "git version".
> > + *
> > + * Always show the version, even if other options are given.
> > + */
> > + strbuf_addf(buf, "git version %s\n", git_version_string);
>
> This ends the output with a complete line when !show_build_options ...
>
> > + if (show_build_options) {
> > + strbuf_addf(buf, "cpu: %s\n", GIT_HOST_CPU);
> > + if (git_built_from_commit_string[0])
> > + strbuf_addf(buf, "built from commit: %s\n",
> > + git_built_from_commit_string);
> > + else
> > + strbuf_addstr(buf, "no commit associated with this build\n");
> > + strbuf_addf(buf, "sizeof-long: %d\n", (int)sizeof(long));
> > + strbuf_addf(buf, "sizeof-size_t: %d\n", (int)sizeof(size_t));
> > + strbuf_addf(buf, "shell-path: %s\n", SHELL_PATH);
> > + /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
>
> ... and the pattern indicates the output will end with a complete
> line when !!show_build_options, too.
>
> > + }
> > +}
>
> So, was the strbuf_complete_line() merely defensive programming? It
> may deserve a comment if it will stay there.
It was meant defensively, here and elsewhere in the series. I figured
that for something like this, which is mostly bounded by human writing
in an editor and then by file IO, spurious string-checking was not such
a big deal.
Are you suggesting to comment around the strbuf_complete_line() calls,
or to comment around get_version_info() that it should end in newline?
- Emily
