[v7,07/15] bugreport: add git-remote-https version
diff mbox series

Message ID 20200214015343.201946-8-emilyshaffer@google.com
State New
Headers show
Series
  • [v7,01/15] help: move list_config_help to builtin/help
Related show

Commit Message

Emily Shaffer Feb. 14, 2020, 1:53 a.m. UTC
It's possible for git-remote-curl to be built separately from git; in that
case we want to know what version of cURL is used by git-remote-curl, not
necessarily which version was present at git-bugreport's build time.
So instead, ask git-remote-curl for the version information it knows
about.

Today, "git-remote-http" and "git-remote-https" are aliased to
"git-remote-curl"; but in case we rely on a different library than cURL
in the future, let's not explicitly reference cURL from bugreport.

For longevity purposes, invoke the alias "git-remote-https" instead of
"git-remote-http".

Since it could have been built at a different time, also report the
version and built-from commit of git-remote-curl alongside the cURL info.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/git-bugreport.txt |  1 +
 bugreport.c                     | 16 ++++++++++++++++
 remote-curl.c                   |  8 ++++++++
 3 files changed, 25 insertions(+)

Comments

Johannes Schindelin Feb. 19, 2020, 2:28 p.m. UTC | #1
Hi Emily,

On Thu, 13 Feb 2020, Emily Shaffer wrote:

> diff --git a/bugreport.c b/bugreport.c
> index 4f9101caeb..bfdff33368 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -5,6 +5,18 @@
>  #include "time.h"
>  #include "help.h"
>  #include "compat/compiler.h"
> +#include "run-command.h"
> +
> +static void get_git_remote_https_version_info(struct strbuf *version_info)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +
> +	argv_array_push(&cp.args, "git");
> +	argv_array_push(&cp.args, "remote-https");
> +	argv_array_push(&cp.args, "--build-info");
> +	if (capture_command(&cp, version_info, 0))

Let's use RUN_GIT_CMD instead of adding `"git"` explicitly; It documents
that we're interested in a Git command, and if we ever build a
single-binary version of Git (as some Git for Windows users already asked
for), it will make things easier.

> +	    strbuf_addstr(version_info, "'git-remote-https --build-info' not supported\n");
> +}
>
>  static void get_system_info(struct strbuf *sys_info)
>  {
> diff --git a/remote-curl.c b/remote-curl.c
> index 350d92a074..c590fbfae3 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -1374,6 +1375,13 @@ int cmd_main(int argc, const char **argv)
>  	string_list_init(&options.deepen_not, 1);
>  	string_list_init(&options.push_options, 1);
>
> +	if (!strcmp("--build-info", argv[1])) {

The context does not say this, but at this point, we already verified that
`argc` is larger than 1. Good.

Also, in keeping with the existing code, we would need to use
`--build-options` here (this is what `git version` calls the equivalent
mode).

_However_.

I like your `--build-info` a lot more than `--build-options` (because the
latter is very misleading: the commit and the date of the build are not
"options" at all).

Thanks,
Dscho

> +		printf("git-http-fetch version: %s\n", git_version_string);
> +		printf("built from commit: %s\n", git_built_from_commit_string);
> +		printf("curl version: %s\n", curl_version());
> +		return 0;
> +	}
> +
>  	/*
>  	 * Just report "remote-curl" here (folding all the various aliases
>  	 * ("git-remote-http", "git-remote-https", and etc.) here since they
> --
> 2.25.0.265.gbab2e86ba0-goog
>
>
Emily Shaffer Feb. 19, 2020, 10:28 p.m. UTC | #2
On Wed, Feb 19, 2020 at 03:28:35PM +0100, Johannes Schindelin wrote:
> Hi Emily,
> 
> On Thu, 13 Feb 2020, Emily Shaffer wrote:
> 
> > diff --git a/bugreport.c b/bugreport.c
> > index 4f9101caeb..bfdff33368 100644
> > --- a/bugreport.c
> > +++ b/bugreport.c
> > @@ -5,6 +5,18 @@
> >  #include "time.h"
> >  #include "help.h"
> >  #include "compat/compiler.h"
> > +#include "run-command.h"
> > +
> > +static void get_git_remote_https_version_info(struct strbuf *version_info)
> > +{
> > +	struct child_process cp = CHILD_PROCESS_INIT;
> > +
> > +	argv_array_push(&cp.args, "git");
> > +	argv_array_push(&cp.args, "remote-https");
> > +	argv_array_push(&cp.args, "--build-info");
> > +	if (capture_command(&cp, version_info, 0))
> 
> Let's use RUN_GIT_CMD instead of adding `"git"` explicitly; It documents
> that we're interested in a Git command, and if we ever build a
> single-binary version of Git (as some Git for Windows users already asked
> for), it will make things easier.

Hm. RUN_GIT_CMD is an argument used for the run_command_v_opt* family of
calls, but it seems that setting child_process.git_cmd has the same
effect. Done.

> 
> > +	    strbuf_addstr(version_info, "'git-remote-https --build-info' not supported\n");
> > +}
> >
> >  static void get_system_info(struct strbuf *sys_info)
> >  {
> > diff --git a/remote-curl.c b/remote-curl.c
> > index 350d92a074..c590fbfae3 100644
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> > @@ -1374,6 +1375,13 @@ int cmd_main(int argc, const char **argv)
> >  	string_list_init(&options.deepen_not, 1);
> >  	string_list_init(&options.push_options, 1);
> >
> > +	if (!strcmp("--build-info", argv[1])) {
> 
> The context does not say this, but at this point, we already verified that
> `argc` is larger than 1. Good.
> 
> Also, in keeping with the existing code, we would need to use
> `--build-options` here (this is what `git version` calls the equivalent
> mode).
> 
> _However_.
> 
> I like your `--build-info` a lot more than `--build-options` (because the
> latter is very misleading: the commit and the date of the build are not
> "options" at all).

Sure. Thanks for saying so.

 - Emily
Junio C Hamano Feb. 19, 2020, 10:33 p.m. UTC | #3
Emily Shaffer <emilyshaffer@google.com> writes:

>> Also, in keeping with the existing code, we would need to use
>> `--build-options` here (this is what `git version` calls the equivalent
>> mode).
>> 
>> _However_.
>> 
>> I like your `--build-info` a lot more than `--build-options` (because the
>> latter is very misleading: the commit and the date of the build are not
>> "options" at all).
>
> Sure. Thanks for saying so.

I don't think anybody would mind introducing --build-info to "git
version" as a synonym and deprecate --build-options from it ;-)
Johannes Schindelin Feb. 20, 2020, 10:33 p.m. UTC | #4
Hi Junio,

On Wed, 19 Feb 2020, Junio C Hamano wrote:

> Emily Shaffer <emilyshaffer@google.com> writes:
>
> >> Also, in keeping with the existing code, we would need to use
> >> `--build-options` here (this is what `git version` calls the equivalent
> >> mode).
> >>
> >> _However_.
> >>
> >> I like your `--build-info` a lot more than `--build-options` (because the
> >> latter is very misleading: the commit and the date of the build are not
> >> "options" at all).
> >
> > Sure. Thanks for saying so.
>
> I don't think anybody would mind introducing --build-info to "git
> version" as a synonym and deprecate --build-options from it ;-)

Heh. I almost suggested this. Almost.

Ciao,
Dscho

Patch
diff mbox series

diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
index 8bbc4c960c..33df4dec7f 100644
--- a/Documentation/git-bugreport.txt
+++ b/Documentation/git-bugreport.txt
@@ -28,6 +28,7 @@  The following information is captured automatically:
  - 'git version --build-options'
  - uname sysname, release, version, and machine strings
  - Compiler-specific info string
+ - 'git remote-https --build-info'
 
 OPTIONS
 -------
diff --git a/bugreport.c b/bugreport.c
index 4f9101caeb..bfdff33368 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -5,6 +5,18 @@ 
 #include "time.h"
 #include "help.h"
 #include "compat/compiler.h"
+#include "run-command.h"
+
+static void get_git_remote_https_version_info(struct strbuf *version_info)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	argv_array_push(&cp.args, "git");
+	argv_array_push(&cp.args, "remote-https");
+	argv_array_push(&cp.args, "--build-info");
+	if (capture_command(&cp, version_info, 0))
+	    strbuf_addstr(version_info, "'git-remote-https --build-info' not supported\n");
+}
 
 static void get_system_info(struct strbuf *sys_info)
 {
@@ -29,6 +41,10 @@  static void get_system_info(struct strbuf *sys_info)
 	strbuf_addstr(sys_info, "compiler info: ");
 	get_compiler_info(sys_info);
 	strbuf_complete_line(sys_info);
+
+	strbuf_addstr(sys_info, "git-remote-https --build-info:\n");
+	get_git_remote_https_version_info(sys_info);
+	strbuf_complete_line(sys_info);
 }
 
 static const char * const bugreport_usage[] = {
diff --git a/remote-curl.c b/remote-curl.c
index 350d92a074..c590fbfae3 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -17,6 +17,7 @@ 
 #include "protocol.h"
 #include "quote.h"
 #include "transport.h"
+#include "version.h"
 
 static struct remote *remote;
 /* always ends with a trailing slash */
@@ -1374,6 +1375,13 @@  int cmd_main(int argc, const char **argv)
 	string_list_init(&options.deepen_not, 1);
 	string_list_init(&options.push_options, 1);
 
+	if (!strcmp("--build-info", argv[1])) {
+		printf("git-http-fetch version: %s\n", git_version_string);
+		printf("built from commit: %s\n", git_built_from_commit_string);
+		printf("curl version: %s\n", curl_version());
+		return 0;
+	}
+
 	/*
 	 * Just report "remote-curl" here (folding all the various aliases
 	 * ("git-remote-http", "git-remote-https", and etc.) here since they