v2, 2 partsTeach git version --build-options about zlib+libcurl

> This change uses the libcurl LIBCURL_VERSION #define text macro. No
> stringification is required for the variable's use. If the #define
> is not present, that version is not reported.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  help.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/help.c b/help.c
> index 1d057aa607..bf74e935b9 100644
> --- a/help.c
> +++ b/help.c
> @@ -1,4 +1,5 @@
>  #include "git-compat-util.h"
> +#include "git-curl-compat.h" /* For LIBCURL_VERSION only */
>  #include "config.h"
>  #include "builtin.h"
>  #include "exec-cmd.h"
> @@ -757,6 +758,9 @@ void get_version_info(struct strbuf *buf, int show_build_options)
>
>  		if (fsmonitor_ipc__is_supported())
>  			strbuf_addstr(buf, "feature: fsmonitor--daemon\n");
> +#if defined LIBCURL_VERSION
> +		strbuf_addf(buf, "libcurl: %s\n", LIBCURL_VERSION);

> +#endif
>  	}
>  }
>
> --
> 2.43.0
>
>
>

>On Fri, 21 Jun 2024, Randall S. Becker wrote:
>
>> This change uses the libcurl LIBCURL_VERSION #define text macro. No
>> stringification is required for the variable's use. If the #define is
>> not present, that version is not reported.
>>
>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> ---
>>  help.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/help.c b/help.c
>> index 1d057aa607..bf74e935b9 100644
>> --- a/help.c
>> +++ b/help.c
>> @@ -1,4 +1,5 @@
>>  #include "git-compat-util.h"
>> +#include "git-curl-compat.h" /* For LIBCURL_VERSION only */
>>  #include "config.h"
>>  #include "builtin.h"
>>  #include "exec-cmd.h"
>> @@ -757,6 +758,9 @@ void get_version_info(struct strbuf *buf, int
>> show_build_options)
>>
>>  		if (fsmonitor_ipc__is_supported())
>>  			strbuf_addstr(buf, "feature: fsmonitor--daemon\n");
>> +#if defined LIBCURL_VERSION
>> +		strbuf_addf(buf, "libcurl: %s\n", LIBCURL_VERSION);
>
>I am not sure that this is the most helpful information Git can provide:
>It reports the version against which Git was _compiled_, whereas the version it is
>_running against_ might be quite different.
>
>Wouldn't calling `curl_version()` make more sense here?

>Ciao,
>Johannes
>
>> +#endif
>>  	}
>>  }
>>
>> --
>> 2.43.0

> On Monday, June 24, 2024 10:13 AM, Johannes Schindelin wrote:
>> I am not sure that this is the most helpful information Git can 
>> provide:
>> It reports the version against which Git was _compiled_, whereas the 
>> version it is
>> _running against_ might be quite different.
>> 
>> Wouldn't calling `curl_version()` make more sense here?
> 
> I think the more important information is the build used. My reasoning
> is that one can call run curl --version to see the current curl
> install. However, different versions of curl have potential API
> changes - same argument with OpenSSL. What initiated this for me (the
> use case) started with a customer who incorrectly installed a git
> build for OpenSSL 3.2 (and its libcurl friend). Git would then get a
> compatibility issue when attempting to use either library. The
> customer did not know (!) they had the git for OpenSSL 3.2 version and
> I had no way to determine which one they had without seeing their path
> - hard in an email support situation. Having git version
> --build-options report what was used for the build *at a compatibility
> level* would have easily shown that the available library (after
> running openssl version or curl --version) reported different values.
> Otherwise, we are back to guessing what they installed. The goal is to
> compare what git expects with what git has available. The above series
> makes this comparative information available.

>On 2024-06-24 16:33, Randall Becker wrote:
>> On Monday, June 24, 2024 10:13 AM, Johannes Schindelin wrote:
>>> I am not sure that this is the most helpful information Git can
>>> provide:
>>> It reports the version against which Git was _compiled_, whereas the
>>> version it is _running against_ might be quite different.
>>>
>>> Wouldn't calling `curl_version()` make more sense here?
>>
>> I think the more important information is the build used. My reasoning
>> is that one can call run curl --version to see the current curl
>> install. However, different versions of curl have potential API
>> changes - same argument with OpenSSL. What initiated this for me (the
>> use case) started with a customer who incorrectly installed a git
>> build for OpenSSL 3.2 (and its libcurl friend). Git would then get a
>> compatibility issue when attempting to use either library. The
>> customer did not know (!) they had the git for OpenSSL 3.2 version and
>> I had no way to determine which one they had without seeing their path
>> - hard in an email support situation. Having git version
>> --build-options report what was used for the build *at a compatibility
>> level* would have easily shown that the available library (after
>> running openssl version or curl --version) reported different values.
>> Otherwise, we are back to guessing what they installed. The goal is to
>> compare what git expects with what git has available. The above series
>> makes this comparative information available.
>
>How about announcing both versions of the library if they differ, and only

> On Monday, June 24, 2024 1:09 PM, Dragan Simic wrote:
>> On 2024-06-24 16:33, Randall Becker wrote:
>>> On Monday, June 24, 2024 10:13 AM, Johannes Schindelin wrote:
>>>> I am not sure that this is the most helpful information Git can
>>>> provide:
>>>> It reports the version against which Git was _compiled_, whereas the
>>>> version it is _running against_ might be quite different.
>>>> 
>>>> Wouldn't calling `curl_version()` make more sense here?
>>> 
>>> I think the more important information is the build used. My 
>>> reasoning
>>> is that one can call run curl --version to see the current curl
>>> install. However, different versions of curl have potential API
>>> changes - same argument with OpenSSL. What initiated this for me (the
>>> use case) started with a customer who incorrectly installed a git
>>> build for OpenSSL 3.2 (and its libcurl friend). Git would then get a
>>> compatibility issue when attempting to use either library. The
>>> customer did not know (!) they had the git for OpenSSL 3.2 version 
>>> and
>>> I had no way to determine which one they had without seeing their 
>>> path
>>> - hard in an email support situation. Having git version
>>> --build-options report what was used for the build *at a 
>>> compatibility
>>> level* would have easily shown that the available library (after
>>> running openssl version or curl --version) reported different values.
>>> Otherwise, we are back to guessing what they installed. The goal is 
>>> to
>>> compare what git expects with what git has available. The above 
>>> series
>>> makes this comparative information available.
>> 
>> How about announcing both versions of the library if they differ, and 
>> only
> one
>> version if they're the same?  We're building this to serve as a way 
>> for
> debugging
>> various issues, having that information available could only be 
>> helpful.
> 
> I don't have a huge problem with that except it will significantly 
> decrease
> performance. We do not currently have to load libcurl/openssl to obtain 
> the
> build version (it is the --build-options flag), so adding additional 
> load on
> this command is not really what the series is about. Doing this 
> run-time
> check might be something someone else may want to take on separately, 
> but
> from a support use-case standpoint, it should be covered as is. Doing a
> comparison is a separate use case.

>On 2024-06-24 23:15, rsbecker@nexbridge.com wrote:
>> On Monday, June 24, 2024 1:09 PM, Dragan Simic wrote:
>>> On 2024-06-24 16:33, Randall Becker wrote:
>>>> On Monday, June 24, 2024 10:13 AM, Johannes Schindelin wrote:
>>>>> I am not sure that this is the most helpful information Git can
>>>>> provide:
>>>>> It reports the version against which Git was _compiled_, whereas
>>>>> the version it is _running against_ might be quite different.
>>>>>
>>>>> Wouldn't calling `curl_version()` make more sense here?
>>>>
>>>> I think the more important information is the build used. My
>>>> reasoning is that one can call run curl --version to see the current
>>>> curl install. However, different versions of curl have potential API
>>>> changes - same argument with OpenSSL. What initiated this for me
>>>> (the use case) started with a customer who incorrectly installed a
>>>> git build for OpenSSL 3.2 (and its libcurl friend). Git would then
>>>> get a compatibility issue when attempting to use either library. The
>>>> customer did not know (!) they had the git for OpenSSL 3.2 version
>>>> and I had no way to determine which one they had without seeing
>>>> their path
>>>> - hard in an email support situation. Having git version
>>>> --build-options report what was used for the build *at a
>>>> compatibility
>>>> level* would have easily shown that the available library (after
>>>> running openssl version or curl --version) reported different values.
>>>> Otherwise, we are back to guessing what they installed. The goal is
>>>> to compare what git expects with what git has available. The above
>>>> series makes this comparative information available.
>>>
>>> How about announcing both versions of the library if they differ, and
>>> only
>> one
>>> version if they're the same?  We're building this to serve as a way
>>> for
>> debugging
>>> various issues, having that information available could only be
>>> helpful.
>>
>> I don't have a huge problem with that except it will significantly
>> decrease performance. We do not currently have to load libcurl/openssl
>> to obtain the build version (it is the --build-options flag), so
>> adding additional load on this command is not really what the series
>> is about. Doing this run-time check might be something someone else
>> may want to take on separately, but from a support use-case
>> standpoint, it should be covered as is. Doing a comparison is a
>> separate use case.
>
>Yes, the additional load is actually a bit concerning.  Perhaps we could wrap the
>current series up as-is and leave the possible improvements to the follow-up
>patches?

> On Monday, June 24, 2024 10:13 AM, Johannes Schindelin wrote:
> >On Fri, 21 Jun 2024, Randall S. Becker wrote:
> >
> >> This change uses the libcurl LIBCURL_VERSION #define text macro. No
> >> stringification is required for the variable's use. If the #define is
> >> not present, that version is not reported.
> >>
> >> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> >> ---
> >>  help.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/help.c b/help.c
> >> index 1d057aa607..bf74e935b9 100644
> >> --- a/help.c
> >> +++ b/help.c
> >> @@ -1,4 +1,5 @@
> >>  #include "git-compat-util.h"
> >> +#include "git-curl-compat.h" /* For LIBCURL_VERSION only */
> >>  #include "config.h"
> >>  #include "builtin.h"
> >>  #include "exec-cmd.h"
> >> @@ -757,6 +758,9 @@ void get_version_info(struct strbuf *buf, int
> >> show_build_options)
> >>
> >>  		if (fsmonitor_ipc__is_supported())
> >>  			strbuf_addstr(buf, "feature: fsmonitor--daemon\n");
> >> +#if defined LIBCURL_VERSION
> >> +		strbuf_addf(buf, "libcurl: %s\n", LIBCURL_VERSION);
> >
> >I am not sure that this is the most helpful information Git can provide:
> >It reports the version against which Git was _compiled_, whereas the version it is
> >_running against_ might be quite different.
> >
> >Wouldn't calling `curl_version()` make more sense here?
>
> I think the more important information is the build used.

> > @@ -757,6 +758,9 @@ void get_version_info(struct strbuf *buf, int show_build_options)
> >
> >  		if (fsmonitor_ipc__is_supported())
> >  			strbuf_addstr(buf, "feature: fsmonitor--daemon\n");
> > +#if defined LIBCURL_VERSION
> > +		strbuf_addf(buf, "libcurl: %s\n", LIBCURL_VERSION);
> 
> I am not sure that this is the most helpful information Git can provide:
> It reports the version against which Git was _compiled_, whereas the
> version it is _running against_ might be quite different.
> 
> Wouldn't calling `curl_version()` make more sense here?

>> +#if defined LIBCURL_VERSION
>> +		strbuf_addf(buf, "libcurl: %s\n", LIBCURL_VERSION);
>
> I am not sure that this is the most helpful information Git can provide:
> It reports the version against which Git was _compiled_, whereas the
> version it is _running against_ might be quite different.

> > Wouldn't calling `curl_version()` make more sense here?
> 
> I wouldn't give that question an outright "no", but unless "git
> version" is split out of the builtin suite of commands and made into
> a standalone binary, I would *not* be able to give an unconditional
> "yes".
> 
> For now, let's stop at the simplest solution---if the library
> project gives us a CPP macro to use for _this exact purpose_, let's
> take the offer.

> On Mon, Jun 24, 2024 at 09:06:03AM -0700, Junio C Hamano wrote:
>
> > > Wouldn't calling `curl_version()` make more sense here?
> >
> > I wouldn't give that question an outright "no", but unless "git
> > version" is split out of the builtin suite of commands and made into
> > a standalone binary, I would *not* be able to give an unconditional
> > "yes".
> >
> > For now, let's stop at the simplest solution---if the library
> > project gives us a CPP macro to use for _this exact purpose_, let's
> > take the offer.
>
> Here's another point of view: libcurl is not a dependency of the git
> binary at all! It is a dependency of the "curl" remote helper. Would it
> make sense for "git remote-https --build-options" to exist?

> I'm not sure. It resolves the linking problem and matches how the actual
> programs are structured. But it is also not something that normal users
> would tend to think about (even if you are having trouble with https
> remotes, you might not know that is implemented as a remote helper).
>
> But we could also have "git version --build-options" call "remote-https
> --build-options" automatically, and just let it dump to the shared
> stdout stream.

> > But we could also have "git version --build-options" call "remote-https
> > --build-options" automatically, and just let it dump to the shared
> > stdout stream.
> 
> Teaching `git version` to show the cURL version may not be the best idea,
> especially when it comes to the version used at runtime and using the
> command-line option `--build-options` (with the option being specifically
> about the build, not the runtime, version that was used).
> 
> Wouldn't it be better to go with Emily's approach to surface this
> information via `git bugreport` instead of `git version`, potentially
> enhanced to show both build-time and runtime version of libcurl?

> On Wed, Jul 24, 2024 at 12:48:20PM +0200, Johannes Schindelin wrote:
>
> > > But we could also have "git version --build-options" call "remote-https
> > > --build-options" automatically, and just let it dump to the shared
> > > stdout stream.
> >
> > Teaching `git version` to show the cURL version may not be the best idea,
> > especially when it comes to the version used at runtime and using the
> > command-line option `--build-options` (with the option being specifically
> > about the build, not the runtime, version that was used).
> >
> > Wouldn't it be better to go with Emily's approach to surface this
> > information via `git bugreport` instead of `git version`, potentially
> > enhanced to show both build-time and runtime version of libcurl?
>
> I don't have a strong preference either way. I naturally turned towards
> "git version" because that's what this thread was about, and also
> because it predates git-bugreport.

> It feels like there may be some duplication / overlap between what those
> two things inspect, and we should perhaps unify them. One thing I
> notice about bugreport is that it doesn't have a way to just dump
> information without trying to start a bugreport. I'd be very unlikely to
> use it myself for reporting a bug, but I may want to dump information
> about git while debugging.
>
> So whether that is in the form of "git bugreport --dump", or if all of
> the collection is moved to "git version --build-info" and then bugreport
> uses that to fill out its template, I don't care.

> All true, but the name `git version` also sets some expectations. Users
> who run `<command> version` will expect to see the version of the command
> they are actually using currently.
> 
> For example, `curl -V` will list something like:
> 
> 	curl 7.81.0 (x86_64-pc-linux-gnu) libcurl/7.81.0 OpenSSL/3.0.2
> 	zlib/1.2.11 brotli/1.0.9 zstd/1.4.8 libidn2/2.3.2 libpsl/0.21.0
> 	(+libidn2/2.3.2) libssh/0.9.6/openssl/zlib nghttp2/1.43.0
> 	librtmp/2.3 OpenLDAP/2.5.18
> 
> Those are the versions of the components that are actually used when
> invoking `curl` commands, not versions that were present on the machine
> that built the `curl` package.
> 
> Compare that to what we're experiencing with Git for Windows v2.46.0-rc2:
> `git version --build-options` lists `libcurl: 8.8.0`. But running `git
> fetch` will actually use libcurl v8.9.0, not v8.8.0. And the output does
> not mention that this is the compile-time version. It lists only one
> version, as if it was the one that the Git executable were using.

> > So whether that is in the form of "git bugreport --dump", or if all of
> > the collection is moved to "git version --build-info" and then bugreport
> > uses that to fill out its template, I don't care.
> 
> I feel that we may need a different command for that than `bugreport
> --dump`, something that reflects that the user wants to gather data to
> investigate an issue, but not necessarily report a bug to the Git project,
> and that we should guide users to use that command instead of `git
> version` when investigating such issues.
> 
> A command with a name along the lines of `git diagnose`, I'd say.

>> A command with a name along the lines of `git diagnose`, I'd say.
>
> OK. I don't really care much either way how it is spelled, though my
> inclination is that we already have a confusing number of commands and
> should avoid adding more.

> Jeff King <peff@peff.net> writes:
> 
> >> A command with a name along the lines of `git diagnose`, I'd say.
> >
> > OK. I don't really care much either way how it is spelled, though my
> > inclination is that we already have a confusing number of commands and
> > should avoid adding more.
> 
> I think what you are responding to is an oblique reference to a
> command that already exists, and there is no need to worry about
> adding more ;-).  If "bugreport" is not farming out the task of
> collecting the information to it, "bugreport" need to be corrected
> to do so (while "diagnose" may have to learn to collect more, if
> "bugreport" collects things that "diagnose" does not (yet)).

> This change uses the zlib ZLIB_VERSION #define text macro. No
> stringification is required for the variable's use. If the #define
> is not present, that version is not reported.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  help.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/help.c b/help.c
> index bf74e935b9..f378750af4 100644
> --- a/help.c
> +++ b/help.c
> @@ -760,6 +760,9 @@ void get_version_info(struct strbuf *buf, int show_build_options)
>  			strbuf_addstr(buf, "feature: fsmonitor--daemon\n");
>  #if defined LIBCURL_VERSION
>  		strbuf_addf(buf, "libcurl: %s\n", LIBCURL_VERSION);
> +#endif
> +#if defined ZLIB_VERSION
> +		strbuf_addf(buf, "zlib: %s\n", ZLIB_VERSION);

>  #endif
>  	}
>  }
> --
> 2.43.0
>
>
>

>On Fri, 21 Jun 2024, Randall S. Becker wrote:
>
>> This change uses the zlib ZLIB_VERSION #define text macro. No
>> stringification is required for the variable's use. If the #define is
>> not present, that version is not reported.
>>
>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> ---
>>  help.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/help.c b/help.c
>> index bf74e935b9..f378750af4 100644
>> --- a/help.c
>> +++ b/help.c
>> @@ -760,6 +760,9 @@ void get_version_info(struct strbuf *buf, int
>show_build_options)
>>  			strbuf_addstr(buf, "feature: fsmonitor--daemon\n");  #if
>defined
>> LIBCURL_VERSION
>>  		strbuf_addf(buf, "libcurl: %s\n", LIBCURL_VERSION);
>> +#endif
>> +#if defined ZLIB_VERSION
>> +		strbuf_addf(buf, "zlib: %s\n", ZLIB_VERSION);
>
>This reports what zlib version Git was linked against, at compile time.
>That may be misleading e.g. when running with a different version that has a bug.
>Would `zlibVersion()` be more useful here?

>
>Ciao,
>Johannes
>
>>  #endif
>>  	}
>>  }
>> --
>> 2.43.0

> On Monday, June 24, 2024 10:16 AM, Johannes Schindelin wrote:
> >On Fri, 21 Jun 2024, Randall S. Becker wrote:
> >
> >> This change uses the zlib ZLIB_VERSION #define text macro. No
> >> stringification is required for the variable's use. If the #define is
> >> not present, that version is not reported.
> >>
> >> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> >> ---
> >>  help.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/help.c b/help.c
> >> index bf74e935b9..f378750af4 100644
> >> --- a/help.c
> >> +++ b/help.c
> >> @@ -760,6 +760,9 @@ void get_version_info(struct strbuf *buf, int
> >show_build_options)
> >>  			strbuf_addstr(buf, "feature: fsmonitor--daemon\n");  #if
> >defined
> >> LIBCURL_VERSION
> >>  		strbuf_addf(buf, "libcurl: %s\n", LIBCURL_VERSION);
> >> +#endif
> >> +#if defined ZLIB_VERSION
> >> +		strbuf_addf(buf, "zlib: %s\n", ZLIB_VERSION);
> >
> >This reports what zlib version Git was linked against, at compile time.
> >That may be misleading e.g. when running with a different version that has a bug.
> >Would `zlibVersion()` be more useful here?
>
> Please see my comments on the libcurl sub-thread. Same logic applies here.

>On Mon, 24 Jun 2024, Randall Becker wrote:
>
>> On Monday, June 24, 2024 10:16 AM, Johannes Schindelin wrote:
>> >On Fri, 21 Jun 2024, Randall S. Becker wrote:
>> >
>> >> This change uses the zlib ZLIB_VERSION #define text macro. No
>> >> stringification is required for the variable's use. If the #define
>> >> is not present, that version is not reported.
>> >>
>> >> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> >> ---
>> >>  help.c | 3 +++
>> >>  1 file changed, 3 insertions(+)
>> >>
>> >> diff --git a/help.c b/help.c
>> >> index bf74e935b9..f378750af4 100644
>> >> --- a/help.c
>> >> +++ b/help.c
>> >> @@ -760,6 +760,9 @@ void get_version_info(struct strbuf *buf, int
>> >show_build_options)
>> >>  			strbuf_addstr(buf, "feature: fsmonitor--daemon\n");  #if
>> >defined
>> >> LIBCURL_VERSION
>> >>  		strbuf_addf(buf, "libcurl: %s\n", LIBCURL_VERSION);
>> >> +#endif
>> >> +#if defined ZLIB_VERSION
>> >> +		strbuf_addf(buf, "zlib: %s\n", ZLIB_VERSION);
>> >
>> >This reports what zlib version Git was linked against, at compile time.
>> >That may be misleading e.g. when running with a different version that has a
>bug.
>> >Would `zlibVersion()` be more useful here?
>>
>> Please see my comments on the libcurl sub-thread. Same logic applies here.
>
>Let's summarize the part of your comments in that sub-thread that is actually
>relevant here, okay? Here goes my attempt:
>
>	The scenario of concern involved a customer having installed a Git
>	build targeting the wrong library version.
>
>Does that sound like a valid summary?
>
>I am quite puzzled what exactly your answer is meant to tell me here, though. Is it
>meant to say:
>
>- Yes, it should be `zlibVersion()`, it is the version that is used and
>  that might cause troubles after all,
>
>- No, you want to continue showing the compile-time version, even if the
>  user might actually use a different version depending what is installed
>  in their setup,
>
>- Yes _and_ no: both versions need to be displayed, as a discrepancy
>  there might explain reported problems and could therefore be quite
>  useful when handling bug reports.
>
>In other words: Please understand that your answer to my question left me
>wanting for an answer.

> What I actually proposed was splitting --build-options with
> runtime (some representative argument). This would allow the
> headers used at build time (--build-options) to be reported *and*
> the runtime (probably) DLL versions (but would also report static
> linked library versions) to be reported. Both are useful from a
> support standpoint.

>Randall Becker <randall.becker@nexbridge.ca> writes:
>
>> What I actually proposed was splitting --build-options with runtime
>> (some representative argument). This would allow the headers used at
>> build time (--build-options) to be reported *and* the runtime
>> (probably) DLL versions (but would also report static linked library
>> versions) to be reported. Both are useful from a support standpoint.
>
>That certainly is a reasonable future plan.
>
>> However, the --build-options argument was intended to report an
>> invariant set of values used during the build, so I would rather not
>> conflate the two distinctly different semantic values.
>
>This reasoning makes sense to me, too.
>
>Please wrap overly long lines to reasonable width, by the way.