Re: [PATCH v8 04/15] bugreport: gather git version and build info

2 messages, 2 authors, 2020-02-20 · open the first message on its own page

Re: [PATCH v8 04/15] bugreport: gather git version and build info

From: Junio C Hamano <hidden>
Date: 2020-02-20 20:07:54

Emily Shaffer [off-list ref] 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.
quoted hunk
+}
...
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.
quoted hunk
 int cmd_version(int argc, const char **argv, const char *prefix)
 {
+	struct strbuf buf = STRBUF_INIT;
 	int build_options = 0;
 	const char * const usage[] = {
 		N_("git version [<options>]"),
@@ -637,26 +662,11 @@ int cmd_version(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, options, usage, 0);
 
-	/*
-	 * 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.
-	 */
-	printf("git version %s\n", git_version_string);
+	get_version_info(&buf, build_options);
+	printf("%s", buf.buf);
+
+	strbuf_release(&buf);
 
-	if (build_options) {
-		printf("cpu: %s\n", GIT_HOST_CPU);
-		if (git_built_from_commit_string[0])
-			printf("built from commit: %s\n",
-			       git_built_from_commit_string);
-		else
-			printf("no commit associated with this build\n");
-		printf("sizeof-long: %d\n", (int)sizeof(long));
-		printf("sizeof-size_t: %d\n", (int)sizeof(size_t));
-		printf("shell-path: %s\n", SHELL_PATH);
-		/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
-	}
 	return 0;
Makes sense.

Re: [PATCH v8 04/15] bugreport: gather git version and build info

From: Emily Shaffer <hidden>
Date: 2020-02-20 23:03:28

On Thu, Feb 20, 2020 at 12:07:46PM -0800, Junio C Hamano wrote:
Emily Shaffer [off-list ref] writes:
quoted
+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.
quoted
+}
...
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 ...
quoted
+	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.
quoted
+	}
+}
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help