[PATCH 0/6] Miscellaneous platform-independent patches from Git for Windows

[PATCH 0/6] Miscellaneous platform-independent patches from Git for Windows

[Johannes Schindelin]

While working on Git for Windows 2.x, a couple of fixes were necessary
that are not actually specific to Windows.

For example, when stuck behind a faulty Access Point that somehow worked
with an Android phone, but not with this developer's MacBook, it was
necessary to use a SOCKS proxy via the phone to be able to continue
developing Git for Windows.

This is the first patch series attempting to lift the patches from Git
for Windows' friendly fork into upstream Git.


Johannes Schindelin (4):
  Only use CURLOPT_LOGIN_OPTIONS if it is actually available
  Facilitate debugging Git executables in tests with gdb
  Squelch warning about an integer overflow
  Silence GCC's "cast of pointer to integer of a different size" warning

Pat Thoyts (1):
  remote-http(s): Support SOCKS proxies

Waldek Maleska (1):
  Correct fscanf formatting string for I64u values

 builtin/gc.c           |  2 +-
 compat/regex/regcomp.c |  6 ++++--
 git-compat-util.h      |  6 +++++-
 http.c                 | 11 +++++++++++
 imap-send.c            |  4 ++++
 pack-revindex.c        |  2 +-
 sha1_file.c            |  2 +-
 wrap-for-bin.sh        |  7 +++++++
 8 files changed, 34 insertions(+), 6 deletions(-)

-- 
2.1.4

[PATCH 1/6] Only use CURLOPT_LOGIN_OPTIONS if it is actually available

[Johannes Schindelin]

This fixes the compilation on an older Linux that was used to debug
test failures when upgrading Git for Windows to Git v2.3.0.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 imap-send.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/imap-send.c b/imap-send.c
index e9faaea..4d3b773 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1421,11 +1421,15 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
 	curl_easy_setopt(curl, CURLOPT_PORT, server.port);
 
 	if (server.auth_method) {
+#if LIBCURL_VERSION_NUM < 0x072200
+		warning("No LOGIN_OPTIONS support in this cURL version");
+#else
 		struct strbuf auth = STRBUF_INIT;
 		strbuf_addstr(&auth, "AUTH=");
 		strbuf_addstr(&auth, server.auth_method);
 		curl_easy_setopt(curl, CURLOPT_LOGIN_OPTIONS, auth.buf);
 		strbuf_release(&auth);
+#endif
 	}
 
 	if (!server.use_ssl)
-- 
2.1.4

Re: [PATCH 1/6] Only use CURLOPT_LOGIN_OPTIONS if it is actually available

[Junio C Hamano]

Thanks.  Makes sense.  It probably needs to have imap-send early on
the title, though (will locally amend, so no need to resend).

[PATCH 2/6] remote-http(s): Support SOCKS proxies

[Johannes Schindelin]

From: Pat Thoyts <patthoyts@users.sourceforge.net>

With this patch we properly support SOCKS proxies, configured e.g. like
this:

	git config http.proxy socks5://192.168.67.1:32767

Without this patch, Git mistakenly tries to use SOCKS proxies as if they
were HTTP proxies, resulting in a error message like:

	fatal: unable to access 'http://.../': Proxy CONNECT aborted

This patch was required to work behind a faulty AP and scraped from
http://stackoverflow.com/questions/15227130/#15228479 and guarded with
an appropriate cURL version check by Johannes Schindelin.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 http.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/http.c b/http.c
index 7da76ed..6b89dea 100644
--- a/http.c
+++ b/http.c
@@ -465,6 +465,17 @@ static CURL *get_curl_handle(void)
 
 	if (curl_http_proxy) {
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+#if LIBCURL_VERSION_NUM >= 0x071800
+		if (starts_with(curl_http_proxy, "socks5"))
+			curl_easy_setopt(result,
+				CURLOPT_PROXYTYPE, CURLPROXY_SOCKS5);
+		else if (starts_with(curl_http_proxy, "socks4a"))
+			curl_easy_setopt(result,
+				CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4A);
+		else if (starts_with(curl_http_proxy, "socks"))
+			curl_easy_setopt(result,
+				CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);
+#endif
 	}
 #if LIBCURL_VERSION_NUM >= 0x070a07
 	curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
-- 
2.1.4

Re: [PATCH 2/6] remote-http(s): Support SOCKS proxies

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> This patch was required to work behind a faulty AP and scraped from
> http://stackoverflow.com/questions/15227130/#15228479 and guarded with
> an appropriate cURL version check by Johannes Schindelin.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Thanks.

The code looks OK but the last paragraph makes _us_ worried.  What
is the licensing status of the original at SO?  I can see that you
are taking legal responsibility with the Signed-off-by: line; you
state that to the best of your knowledge the patch is covered under
an appropriate open source license and you ahve the right under that
license to submit it here to the project.

But it is my job to double check when in doubt, hence this question.

> ---
>  http.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/http.c b/http.c
> index 7da76ed..6b89dea 100644
> --- a/http.c
> +++ b/http.c
> @@ -465,6 +465,17 @@ static CURL *get_curl_handle(void)
>  
>  	if (curl_http_proxy) {
>  		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +#if LIBCURL_VERSION_NUM >= 0x071800
> +		if (starts_with(curl_http_proxy, "socks5"))
> +			curl_easy_setopt(result,
> +				CURLOPT_PROXYTYPE, CURLPROXY_SOCKS5);
> +		else if (starts_with(curl_http_proxy, "socks4a"))
> +			curl_easy_setopt(result,
> +				CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4A);
> +		else if (starts_with(curl_http_proxy, "socks"))
> +			curl_easy_setopt(result,
> +				CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);
> +#endif
>  	}
>  #if LIBCURL_VERSION_NUM >= 0x070a07
>  	curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);

Re: [PATCH 2/6] remote-http(s): Support SOCKS proxies

[James McCoy]

On Mon, Oct 26, 2015 at 01:15:17PM -0700, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > This patch was required to work behind a faulty AP and scraped from
> > http://stackoverflow.com/questions/15227130/#15228479 and guarded with
> > an appropriate cURL version check by Johannes Schindelin.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Thanks.
> 
> The code looks OK but the last paragraph makes _us_ worried.  What
> is the licensing status of the original at SO?

According to Stackoverflow[0],

  As noted in the Stack Exchange Terms of Service[1] and in the footer of
  every page, all user contributions are licensed under Creative Commons
  Attribution-Share Alike[2]. Proper attribution[3] is required if you
  republish any Stack Exchange content.

[0]: https://stackoverflow.com/help/licensing
[1]: http://stackexchange.com/legal
[2]: http://creativecommons.org/licenses/by-sa/3.0/
[3]: http://blog.stackoverflow.com/2009/06/attribution-required/

Cheers,
-- 
James
GPG Key: 4096R/331BA3DB 2011-12-05 James McCoy <vega.james@gmail.com>

Re: [PATCH 2/6] remote-http(s): Support SOCKS proxies

[Junio C Hamano]

James McCoy <vega.james@gmail.com> writes:

>> The code looks OK but the last paragraph makes _us_ worried.  What
>> is the licensing status of the original at SO?
>
> According to Stackoverflow[0],
>
>   As noted in the Stack Exchange Terms of Service[1] and in the footer of
>   every page, all user contributions are licensed under Creative Commons
>   Attribution-Share Alike[2]. Proper attribution[3] is required if you
>   republish any Stack Exchange content.
>
> [0]: https://stackoverflow.com/help/licensing

Yes, and (please correct me if I am wrong--this is one of the times
I hope I am wrong!) I thought BY-SA does not mesh well with GPLv2,
in which case we cannot use this patch (instead somebody has to
reimplement the same without copying).

Re: [PATCH 2/6] remote-http(s): Support SOCKS proxies

[Johannes Schindelin]

Hi,

On Mon, 26 Oct 2015, Junio C Hamano wrote:

> James McCoy <vega.james@gmail.com> writes:
> 
> >> The code looks OK but the last paragraph makes _us_ worried.  What
> >> is the licensing status of the original at SO?
> >
> > According to Stackoverflow[0],
> >
> >   As noted in the Stack Exchange Terms of Service[1] and in the footer of
> >   every page, all user contributions are licensed under Creative Commons
> >   Attribution-Share Alike[2]. Proper attribution[3] is required if you
> >   republish any Stack Exchange content.
> >
> > [0]: https://stackoverflow.com/help/licensing
> 
> Yes, and (please correct me if I am wrong--this is one of the times
> I hope I am wrong!) I thought BY-SA does not mesh well with GPLv2,
> in which case we cannot use this patch (instead somebody has to
> reimplement the same without copying).

Pat, could you please allow us to insert your SOB?

Thanks,
Dscho

Re: [PATCH 2/6] remote-http(s): Support SOCKS proxies

[Johannes Schindelin]

Hi,

On Tue, 27 Oct 2015, Johannes Schindelin wrote:

> On Mon, 26 Oct 2015, Junio C Hamano wrote:
> 
> > James McCoy <vega.james@gmail.com> writes:
> > 
> > >> The code looks OK but the last paragraph makes _us_ worried.  What
> > >> is the licensing status of the original at SO?
> > >
> > > According to Stackoverflow[0],
> > >
> > >   As noted in the Stack Exchange Terms of Service[1] and in the footer of
> > >   every page, all user contributions are licensed under Creative Commons
> > >   Attribution-Share Alike[2]. Proper attribution[3] is required if you
> > >   republish any Stack Exchange content.
> > >
> > > [0]: https://stackoverflow.com/help/licensing
> > 
> > Yes, and (please correct me if I am wrong--this is one of the times
> > I hope I am wrong!) I thought BY-SA does not mesh well with GPLv2,
> > in which case we cannot use this patch (instead somebody has to
> > reimplement the same without copying).
> 
> Pat, could you please allow us to insert your SOB?

On second thought... Junio, could you please sanity-check my claim that
this patch:

-- snip --
@@ -465,6 +465,17 @@ static CURL *get_curl_handle(void)
 
        if (curl_http_proxy) {
                curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+#if LIBCURL_VERSION_NUM >= 0x071800
+               if (starts_with(curl_http_proxy, "socks5"))
+                       curl_easy_setopt(result,
+                               CURLOPT_PROXYTYPE, CURLPROXY_SOCKS5);
+               else if (starts_with(curl_http_proxy, "socks4a"))
+                       curl_easy_setopt(result,
+                               CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4A);
+               else if (starts_with(curl_http_proxy, "socks"))
+                       curl_easy_setopt(result,
+                               CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);
+#endif
        }
 #if LIBCURL_VERSION_NUM >= 0x070a07
        curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
-- snap --

cannot be copyrighted because it is pretty much the only way to implement
said functionality?

Still, Pat, if you find the time, could you please simply relicense your
patch (I know that you are fine with it, but we need an explicit
statement)?

Ciao,
Johannes

Re: [PATCH 2/6] remote-http(s): Support SOCKS proxies

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On second thought... Junio, could you please sanity-check my claim that
> this patch:
>
> -- snip --
> ...
> -- snap --
>
> cannot be copyrighted because it is pretty much the only way to implement
> said functionality?

I am not a lawyer, so...


> Still, Pat, if you find the time, could you please simply relicense your
> patch (I know that you are fine with it, but we need an explicit
> statement)?
>
> Ciao,
> Johannes

Re: [PATCH 2/6] remote-http(s): Support SOCKS proxies

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> On second thought... Junio, could you please sanity-check my claim that
>> this patch:
>>
>> -- snip --
>> ...
>> -- snap --
>>
>> cannot be copyrighted because it is pretty much the only way to implement
>> said functionality?
>
> I am not a lawyer, so...
>
>
>> Still, Pat, if you find the time, could you please simply relicense your
>> patch (I know that you are fine with it, but we need an explicit
>> statement)?

So, I talked to our lawyer.

We are very lucky that the original was posted to SO by our friend
Pat, and you did the right thing to ask Pat to relicense.

Analyzing copyrightability is often more costly than the risk.  Even
if you believe it is not copyrightable, you are bearing the risk
that the court may disagree with you.  Finding a different way to
express the same idea, especially for a small patch like this, is
often cheaper than the cost of copyrightability analysis and the
risk of lawsuit.

If the original is from a friendly party, relicensing is clearly
cheaper and cleaner of the possible choices.

Re: [PATCH 2/6] remote-http(s): Support SOCKS proxies

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Analyzing copyrightability is often more costly than the risk.

Misspelled, obviously: "more costly than the other ways to mitigate
the risk" is what I meant.

Re: [PATCH 2/6] remote-http(s): Support SOCKS proxies

[Johannes Schindelin]

Hi Junio,

On Tue, 27 Oct 2015, Junio C Hamano wrote:

> We are very lucky that the original was posted to SO by our friend
> Pat, and you did the right thing to ask Pat to relicense.

I suspect Pat is on a six month trip around the globe or something,
judging from the feedback I got here:

	https://github.com/patthoyts/git-gui/pull/1

Ciao,
Dscho

Re: [PATCH 2/6] remote-http(s): Support SOCKS proxies

[Pat Thoyts]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>Hi,
>
>On Tue, 27 Oct 2015, Johannes Schindelin wrote:
>
>> On Mon, 26 Oct 2015, Junio C Hamano wrote:
>> 
>> > James McCoy <vega.james@gmail.com> writes:
>> > 
>> > >> The code looks OK but the last paragraph makes _us_ worried.  What
>> > >> is the licensing status of the original at SO?
>> > >
>> > > According to Stackoverflow[0],
>> > >
>> > >   As noted in the Stack Exchange Terms of Service[1] and in the footer of
>> > >   every page, all user contributions are licensed under Creative Commons
>> > >   Attribution-Share Alike[2]. Proper attribution[3] is required if you
>> > >   republish any Stack Exchange content.
>> > >
>> > > [0]: https://stackoverflow.com/help/licensing
>> > 
>> > Yes, and (please correct me if I am wrong--this is one of the times
>> > I hope I am wrong!) I thought BY-SA does not mesh well with GPLv2,
>> > in which case we cannot use this patch (instead somebody has to
>> > reimplement the same without copying).
>> 
>> Pat, could you please allow us to insert your SOB?
>
>On second thought... Junio, could you please sanity-check my claim that
>this patch:
>
>-- snip --
>@@ -465,6 +465,17 @@ static CURL *get_curl_handle(void)
> 
>        if (curl_http_proxy) {
>                curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>+#if LIBCURL_VERSION_NUM >= 0x071800
>+               if (starts_with(curl_http_proxy, "socks5"))
>+                       curl_easy_setopt(result,
>+                               CURLOPT_PROXYTYPE, CURLPROXY_SOCKS5);
>+               else if (starts_with(curl_http_proxy, "socks4a"))
>+                       curl_easy_setopt(result,
>+                               CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4A);
>+               else if (starts_with(curl_http_proxy, "socks"))
>+                       curl_easy_setopt(result,
>+                               CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);
>+#endif
>        }
> #if LIBCURL_VERSION_NUM >= 0x070a07
>        curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
>-- snap --
>
>cannot be copyrighted because it is pretty much the only way to implement
>said functionality?
>
>Still, Pat, if you find the time, could you please simply relicense your
>patch (I know that you are fine with it, but we need an explicit
>statement)?
>
>Ciao,
>Johannes

A bit late to the party but 'yes'. Frankly by posting something to SO I
rather consider it public domain but I hereby license this patch as
required for use by the Git project.

Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>

-- 
Pat Thoyts                            http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97  10 CE 11 E6 04 E0 B9 DD

Re: [PATCH 2/6] remote-http(s): Support SOCKS proxies

[Johannes Schindelin]

Hi Pat,

On Mon, 9 Nov 2015, Pat Thoyts wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >On Tue, 27 Oct 2015, Johannes Schindelin wrote:
> >
> >> On Mon, 26 Oct 2015, Junio C Hamano wrote:
> >> 
> >> > James McCoy <vega.james@gmail.com> writes:
> >> > 
> >> > >> The code looks OK but the last paragraph makes _us_ worried.  What
> >> > >> is the licensing status of the original at SO?
> >> > >
> >> > > According to Stackoverflow[0],
> >> > >
> >> > >   As noted in the Stack Exchange Terms of Service[1] and in the footer of
> >> > >   every page, all user contributions are licensed under Creative Commons
> >> > >   Attribution-Share Alike[2]. Proper attribution[3] is required if you
> >> > >   republish any Stack Exchange content.
> >> > >
> >> > > [0]: https://stackoverflow.com/help/licensing
> >> > 
> >> > Yes, and (please correct me if I am wrong--this is one of the times
> >> > I hope I am wrong!) I thought BY-SA does not mesh well with GPLv2,
> >> > in which case we cannot use this patch (instead somebody has to
> >> > reimplement the same without copying).
> >> 
> >> Pat, could you please allow us to insert your SOB?
> >
> >On second thought... Junio, could you please sanity-check my claim that
> >this patch:
> >
> >-- snip --
> >@@ -465,6 +465,17 @@ static CURL *get_curl_handle(void)
> > 
> >        if (curl_http_proxy) {
> >                curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> >+#if LIBCURL_VERSION_NUM >= 0x071800
> >+               if (starts_with(curl_http_proxy, "socks5"))
> >+                       curl_easy_setopt(result,
> >+                               CURLOPT_PROXYTYPE, CURLPROXY_SOCKS5);
> >+               else if (starts_with(curl_http_proxy, "socks4a"))
> >+                       curl_easy_setopt(result,
> >+                               CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4A);
> >+               else if (starts_with(curl_http_proxy, "socks"))
> >+                       curl_easy_setopt(result,
> >+                               CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);
> >+#endif
> >        }
> > #if LIBCURL_VERSION_NUM >= 0x070a07
> >        curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> >-- snap --
> >
> >cannot be copyrighted because it is pretty much the only way to implement
> >said functionality?
> >
> >Still, Pat, if you find the time, could you please simply relicense your
> >patch (I know that you are fine with it, but we need an explicit
> >statement)?
> >
> >Ciao,
> >Johannes
> 
> A bit late to the party but 'yes'. Frankly by posting something to SO I
> rather consider it public domain

Yeah, unfortunately it needs to be stated explicitly, though... ;-)

> but I hereby license this patch as required for use by the Git project.
> 
> Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>

Thanks!

Ciao,
Dscho

Re: [PATCH 2/6] remote-http(s): Support SOCKS proxies

[Junio C Hamano]

Pat Thoyts <patthoyts@users.sourceforge.net> writes:

> A bit late to the party but 'yes'. Frankly by posting something to SO I
> rather consider it public domain ...

Unless otherwise noted, material posted on stackoverflow by default
becomes CC-SA-BY (which may not be the best choice for open source
software).

Thanks for the patch.

[PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Johannes Schindelin]

When prefixing a Git call in the test suite with 'TEST_GDB_GIT=1 ', it
will now be run with GDB, allowing the developer to debug test failures
more conveniently.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wrap-for-bin.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/wrap-for-bin.sh b/wrap-for-bin.sh
index 701d233..a151c95 100644
--- a/wrap-for-bin.sh
+++ b/wrap-for-bin.sh
@@ -19,4 +19,11 @@ GIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale'
 PATH='@@BUILD_DIR@@/bin-wrappers:'"$PATH"
 export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR
 
+if test -n "$TEST_GDB_GIT"
+then
+	exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
+	echo "Could not run gdb -args ${GIT_EXEC_PATH}/@@PROG@@ $*" >&2
+	exit 1
+fi
+
 exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
-- 
2.1.4

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jonathan Nieder]

Johannes Schindelin wrote:

> When prefixing a Git call in the test suite with 'TEST_GDB_GIT=1 ', it
> will now be run with GDB, allowing the developer to debug test failures
> more conveniently.

Neat.

[...]
> --- a/wrap-for-bin.sh
> +++ b/wrap-for-bin.sh
> @@ -19,4 +19,11 @@ GIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale'
>  PATH='@@BUILD_DIR@@/bin-wrappers:'"$PATH"
>  export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR
>  
> +if test -n "$TEST_GDB_GIT"
> +then
> +	exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"

Most TEST_ environment variables that git respects are under
GIT_TEST_* --- e.g., GIT_TEST_OPTS.  Should this match that pattern
as well, for easier debugging with commands like 'env | grep GIT_'?

What happens if the child in turn calls git again?  Should this
unset TEST_GDB_GIT in gdb's environment?

The gdb manual and --help output advertise "--args".  Has "-args"
(with a single dash) always worked?

> +	echo "Could not run gdb -args ${GIT_EXEC_PATH}/@@PROG@@ $*" >&2
> +	exit 1

Does the 'exec' after the fi need this as well?  exec is supposed to
itself print a message and exit when it runs into an error.  Would
including an 'else' with the if make the control flow clearer?  E.g.

	if test -n "$TEST_GDB_GIT"
	then
		exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
	else
		exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
	fi

Thanks,
Jonathan

diff --git i/wrap-for-bin.sh w/wrap-for-bin.sh
index a151c95..db0ec6a 100644
--- i/wrap-for-bin.sh
+++ w/wrap-for-bin.sh
@@ -19,11 +19,10 @@ GIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale'
 PATH='@@BUILD_DIR@@/bin-wrappers:'"$PATH"
 export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR
 
-if test -n "$TEST_GDB_GIT"
+if test -n "$GIT_TEST_GDB"
 then
-	exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
-	echo "Could not run gdb -args ${GIT_EXEC_PATH}/@@PROG@@ $*" >&2
-	exit 1
+	unset GIT_TEST_GDB
+	exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
+else
+	exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
 fi
-
-exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Johannes Schindelin]

Hi Jonathan,

On Mon, 26 Oct 2015, Jonathan Nieder wrote:

> Johannes Schindelin wrote:
> 
> > --- a/wrap-for-bin.sh
> > +++ b/wrap-for-bin.sh
> > @@ -19,4 +19,11 @@ GIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale'
> >  PATH='@@BUILD_DIR@@/bin-wrappers:'"$PATH"
> >  export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR
> >  
> > +if test -n "$TEST_GDB_GIT"
> > +then
> > +	exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> 
> Most TEST_ environment variables that git respects are under
> GIT_TEST_* --- e.g., GIT_TEST_OPTS.  Should this match that pattern
> as well, for easier debugging with commands like 'env | grep GIT_'?

I dunno. This variable is most useful when inserted into the shell scripts
in t/ themselves, not when specified via the command line. For example, if
you have something like

	test_expect_success '123' '
		...
		# This Git call somehow fails and I have no clue why
		git push remote HEAD
		...
	'

then prefixing the `git push` command with `TEST_GDB_GIT=1` lets you use
`gdb` when running the test with the `-i` and `-v` flags.

Please note that `TEST_GDB_GIT` is already a major step up from my initial
`DDD`.

> What happens if the child in turn calls git again?  Should this
> unset TEST_GDB_GIT in gdb's environment?

It probably would call gdb again. Which is sometimes useful. But I have to
admit that I do not know whether that works.

> The gdb manual and --help output advertise "--args".  Has "-args"
> (with a single dash) always worked?

I always used it with a single dash... So I assume that it worked for a
long time (IIRC I used it first in 1994).

> > +	echo "Could not run gdb -args ${GIT_EXEC_PATH}/@@PROG@@ $*" >&2
> > +	exit 1
> 
> Does the 'exec' after the fi need this as well?  exec is supposed to
> itself print a message and exit when it runs into an error.  Would
> including an 'else' with the if make the control flow clearer?  E.g.
> 
> 	if test -n "$TEST_GDB_GIT"
> 	then
> 		exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> 	else
> 		exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> 	fi

I suppose you're right! The `exec` can fail easily, e.g. when `gdb` was
not found.

Ciao,
Johannes

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Most TEST_ environment variables that git respects are under
>> GIT_TEST_* --- e.g., GIT_TEST_OPTS.  Should this match that pattern
>> as well, for easier debugging with commands like 'env | grep GIT_'?
>
> I dunno. This variable is most useful when inserted into the shell scripts
> in t/ themselves, not when specified via the command line. For example, if
> you have something like
>
> 	test_expect_success '123' '
> 		...
> 		# This Git call somehow fails and I have no clue why
> 		git push remote HEAD
> 		...
> 	'
>
> then prefixing the `git push` command with `TEST_GDB_GIT=1` lets you use
> `gdb` when running the test with the `-i` and `-v` flags.
>
> Please note that `TEST_GDB_GIT` is already a major step up from my initial
> `DDD`.

Yeah, that was my first reaction when I saw this patch.  Instead of
having to munge that line to "gdb -whatever-args git", you can do a
single-shot debugging in a convenient way.  And quite honestly,
because nobody sane will run:

     $ cd t && TEST_GDB_GIT=1 sh ./t1234-frotz.sh

and can drive all the "git" running under gdb at the same time, I
think what you showed would be the _only_ practical use case.  I
would have thought that TEST_GDB_GIT was way too long (and so is
GIT_TEST_GDB) and was about to suggest using something short and
sweet, even shorter than DDD, that you can easily add and remove.

It can be called GDB=1, perhaps?

I agree with all other points Jonathan made in his review, including
"Neat." part ;-)

Thanks.

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jeff King]

On Tue, Oct 27, 2015 at 09:34:48AM -0700, Junio C Hamano wrote:

> Yeah, that was my first reaction when I saw this patch.  Instead of
> having to munge that line to "gdb -whatever-args git", you can do a
> single-shot debugging in a convenient way.  And quite honestly,
> because nobody sane will run:
> 
>      $ cd t && TEST_GDB_GIT=1 sh ./t1234-frotz.sh
> 
> and can drive all the "git" running under gdb at the same time, I
> think what you showed would be the _only_ practical use case.

I agree doing so would be crazy. But would:

  ./t1234-frotz.sh --gdb=17

be sane to run gdb only inside test 17?

I suspect it would work about half the time. Many tests will call git
only once per snippet, but many make multiple git calls, and we are only
interested in debugging one.

I dunno. Maybe that is making things more complicated than they need to
be. I usually use the "tweak the test script" approach, but I have
always found it annoying to have to untweak it later.

-Peff

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Stefan Beller]

On Tue, Oct 27, 2015 at 4:28 PM, Jeff King <peff@peff.net> wrote:
> I agree doing so would be crazy. But would:
>
>   ./t1234-frotz.sh --gdb=17
>
> be sane to run gdb only inside test 17?

OT:
We have two ways of addressing tests, by number and by name.
Usually when a test fails ("Foo gobbles the bar correctly" failed),
I want to run tests 1,17 (1 is the correct setup and 17 is the failing test)
But coming up with that tuple is hard.
  * How do I know we need to run 1 as the setup ? (usually we do,
    sometimes we don't and other times we also need 2,3 to completely
setup the tests)
  * How do I know it's test 17 which is failing? My workflow up to now
    I just searched the test title in the file, such that I'd be there anyway
    to inspect it further. But still I found it inconvenient to
mentally map between
    17 and the test title.

Stefan

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jeff King]

On Tue, Oct 27, 2015 at 04:39:37PM -0700, Stefan Beller wrote:

> On Tue, Oct 27, 2015 at 4:28 PM, Jeff King <peff@peff.net> wrote:
> > I agree doing so would be crazy. But would:
> >
> >   ./t1234-frotz.sh --gdb=17
> >
> > be sane to run gdb only inside test 17?
> 
> OT:
> We have two ways of addressing tests, by number and by name.

Yeah. The numbers are not stable if the script gets new test, but they
are usually fine for within a debugging session. Names are annoying to
type (and also not guaranteed unique).

> Usually when a test fails ("Foo gobbles the bar correctly" failed),
> I want to run tests 1,17 (1 is the correct setup and 17 is the failing test)
> But coming up with that tuple is hard.
>   * How do I know we need to run 1 as the setup ? (usually we do,
>     sometimes we don't and other times we also need 2,3 to completely
> setup the tests)

I think trying to deduce that tuple is a fool's errand. It takes a lot
of manual work, and even if you _think_ you have it, sometimes state
left from earlier tests is accidentally important. But it's usually not
that expensive to run earlier tests at all; it's just expensive to run
them with extra debugging. That's why we have options like
"--valgrind-only=17". We still _run_ tests 1..16, but we do it quickly,
and then execute the expensive and slow valgrind git only on the
suspicious one.

And I'd propose --gdb to work the same way (run all the other tests, but
only kick in gdb for the suspicious one).

If you had multiple "git" invocations inside test 17, you could even do
something like "--gdb=17:4" to kick in only for the 4th git invocation
or something. But counting up git invocations is probably too irritating
to be worth doing manually.

>   * How do I know it's test 17 which is failing? My workflow up to now
>     I just searched the test title in the file, such that I'd be there anyway
>     to inspect it further. But still I found it inconvenient to
> mentally map between
>     17 and the test title.

I usually just run the test script and look at the output. Here's a
failure (which I obviously induced with an extra line):

  $ ./t4103-apply-binary.sh -v -i
  [...]
  ok 5 - check binary diff -- should fail.
  
  expecting success: 
          git checkout master &&
          echo whoops, we fail here && false &&
          test_must_fail git apply --check C.diff
  
  Already on 'master'
  whoops, we fail here
  not ok 6 - check binary diff (copy) -- should fail.
  #
  #               git checkout master &&
  #               echo whoops, we fail here && false &&
  #               test_must_fail git apply --check C.diff
  #

I'd pull the test number from the "not ok" above (it's actually even
easier to see if you drop the "-v", but I usually start my debugging
with "-v" anyway, since error messages often make the problem obvious).

-Peff

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Johannes Schindelin]

Hi Peff,

On Tue, 27 Oct 2015, Jeff King wrote:

> On Tue, Oct 27, 2015 at 09:34:48AM -0700, Junio C Hamano wrote:
> 
> > Yeah, that was my first reaction when I saw this patch.  Instead of
> > having to munge that line to "gdb -whatever-args git", you can do a
> > single-shot debugging in a convenient way.  And quite honestly,
> > because nobody sane will run:
> > 
> >      $ cd t && TEST_GDB_GIT=1 sh ./t1234-frotz.sh
> > 
> > and can drive all the "git" running under gdb at the same time, I
> > think what you showed would be the _only_ practical use case.
> 
> I agree doing so would be crazy. But would:
> 
>   ./t1234-frotz.sh --gdb=17
> 
> be sane to run gdb only inside test 17?

It would probably be sane, but I never encountered the need for something
like that. It was always much easier to run the test using `sh -x t... -i
-v` to find out what command was behaving funnily (mind you, that can be a
pretty hard thing todo, we have some quite convoluted test scripts in our
code base) and then edit the test.

I would expect that `--gdb=<n>` thing to drive me crazy: first, I would
choose the wrong number. Next, I would probably forget that test_commit
and other commands *also* calls Git.

Ciao,
Dscho

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jeff King]

On Fri, Oct 30, 2015 at 07:25:24PM +0100, Johannes Schindelin wrote:

> > I agree doing so would be crazy. But would:
> > 
> >   ./t1234-frotz.sh --gdb=17
> > 
> > be sane to run gdb only inside test 17?
> 
> It would probably be sane, but I never encountered the need for something
> like that. It was always much easier to run the test using `sh -x t... -i
> -v` to find out what command was behaving funnily (mind you, that can be a
> pretty hard thing todo, we have some quite convoluted test scripts in our
> code base) and then edit the test.
> 
> I would expect that `--gdb=<n>` thing to drive me crazy: first, I would
> choose the wrong number. Next, I would probably forget that test_commit
> and other commands *also* calls Git.

Yeah, good points. You somehow have to say "debug _this_ git
invocation", and there is probably not a more precise way to do that
than sticking something in the code on the right line.

I do think I like Junio's "debug git foo" rather than setting the
environment variable, as its syntactically a little simpler to type (and
of course it would probably be implemented with an environment variable,
so one could whichever style they prefer).

-Peff

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Johannes Schindelin]

Hi Junio,

On Tue, 27 Oct 2015, Junio C Hamano wrote:

> It can be called GDB=1, perhaps?

No, this is way too generic. As I only test that the environment
variable's existence, even something like GDB=/usr/opt/gdb/bin/gdb would
trigger it.

I could be talked into GDB_GIT=1, though.

Ciao,
Dscho

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Tue, 27 Oct 2015, Junio C Hamano wrote:
>
>> It can be called GDB=1, perhaps?
>
> No, this is way too generic. As I only test that the environment
> variable's existence, even something like GDB=/usr/opt/gdb/bin/gdb would
> trigger it.
>
> I could be talked into GDB_GIT=1, though.

As I said in another message, I have no preference myself over the
name of this variable (or making it a shell function like Duy
mentioned, which incidentally may give us more visual pleasantness
by losing '=').

I'd just be happy as long as the feature becomes available, and I'd
leave the choice of consistent and convenient naming to others who
have stronger opinions ;-)

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jonathan Nieder]

Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> On Tue, 27 Oct 2015, Junio C Hamano wrote:

>>> It can be called GDB=1, perhaps?
>>
>> No, this is way too generic. As I only test that the environment
>> variable's existence, even something like GDB=/usr/opt/gdb/bin/gdb would
>> trigger it.
>>
>> I could be talked into GDB_GIT=1, though.
>
> As I said in another message, I have no preference myself over the
> name of this variable (or making it a shell function like Duy
> mentioned, which incidentally may give us more visual pleasantness
> by losing '=').
>
> I'd just be happy as long as the feature becomes available, and I'd
> leave the choice of consistent and convenient naming to others who
> have stronger opinions ;-)

Here's a suggested patch.

-- >8 --
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Facilitate debugging Git executables in tests with gdb

When prefixing a Git call in the test suite with 'debug ', it will now
be run with GDB, allowing the developer to debug test failures more
conveniently.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/README                | 5 +++++
 t/test-lib-functions.sh | 8 ++++++++
 wrap-for-bin.sh         | 8 +++++++-
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/t/README b/t/README
index 35438bc..1dc908e 100644
--- a/t/README
+++ b/t/README
@@ -563,6 +563,11 @@ library for your script to use.
    argument.  This is primarily meant for use during the
    development of a new test script.
 
+ - debug <git-command>
+
+   Run a git command inside a debugger. This is primarily meant for
+   use when debugging a failing test script.
+
  - test_done
 
    Your test script must have test_done at the end.  Its purpose
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6dffb8b..73e37a1 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -145,6 +145,14 @@ test_pause () {
 	fi
 }
 
+# Wrap git in gdb. Adding this to a command can make it easier to
+# understand what is going on in a failing test.
+#
+# Example: "debug git checkout master".
+debug () {
+	 GIT_TEST_GDB=1 "$@"
+}
+
 # Call test_commit with the arguments "<message> [<file> [<contents> [<tag>]]]"
 #
 # This will commit a file with the given contents and the given commit
diff --git a/wrap-for-bin.sh b/wrap-for-bin.sh
index 701d233..db0ec6a 100644
--- a/wrap-for-bin.sh
+++ b/wrap-for-bin.sh
@@ -19,4 +19,10 @@ GIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale'
 PATH='@@BUILD_DIR@@/bin-wrappers:'"$PATH"
 export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR
 
-exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
+if test -n "$GIT_TEST_GDB"
+then
+	unset GIT_TEST_GDB
+	exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
+else
+	exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
+fi
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Johannes Schindelin]

Hi Jonathan,

On Fri, 30 Oct 2015, Jonathan Nieder wrote:

> Junio C Hamano wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> On Tue, 27 Oct 2015, Junio C Hamano wrote:
> 
> >>> It can be called GDB=1, perhaps?
> >>
> >> No, this is way too generic. As I only test that the environment
> >> variable's existence, even something like GDB=/usr/opt/gdb/bin/gdb would
> >> trigger it.
> >>
> >> I could be talked into GDB_GIT=1, though.
> >
> > As I said in another message, I have no preference myself over the
> > name of this variable (or making it a shell function like Duy
> > mentioned, which incidentally may give us more visual pleasantness
> > by losing '=').
> >
> > I'd just be happy as long as the feature becomes available, and I'd
> > leave the choice of consistent and convenient naming to others who
> > have stronger opinions ;-)
> 
> Here's a suggested patch.

I am fine with this patch as a replacement for my original version.

Thanks,
Dscho

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jeff King]

On Fri, Oct 30, 2015 at 12:02:56PM -0700, Jonathan Nieder wrote:

> > I'd just be happy as long as the feature becomes available, and I'd
> > leave the choice of consistent and convenient naming to others who
> > have stronger opinions ;-)
> 
> Here's a suggested patch.
> 
> -- >8 --
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Subject: Facilitate debugging Git executables in tests with gdb
> 
> When prefixing a Git call in the test suite with 'debug ', it will now
> be run with GDB, allowing the developer to debug test failures more
> conveniently.

At the risk of repeating what I just said elsewhere in the thread, I
think this patch is the best of the proposed solutions.

> --- a/wrap-for-bin.sh
> +++ b/wrap-for-bin.sh
> @@ -19,4 +19,10 @@ GIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale'
>  PATH='@@BUILD_DIR@@/bin-wrappers:'"$PATH"
>  export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR
>  
> -exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> +if test -n "$GIT_TEST_GDB"
> +then
> +	unset GIT_TEST_GDB
> +	exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> +else
> +	exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> +fi

Somebody suggested elsewhere that the name "gdb" be configurable. We
could stick that in the same variable, like:

  test "$GIT_TEST_GDB" = 1 && GIT_TEST_GDB=gdb
  exec ${GIT_TEST_GDB} --args ...

but that does not play well with the "debug" function, which does not
know which value to set it to. I guess we would need GIT_TEST_GDB_PATH
or something.

I am happy to let that get added later by interested parties (I am happy
with "gdb" myself). I just wanted to mention it to make sure we are not
painting ourselves into any corners.

-Peff

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jonathan Nieder]

Jeff King wrote:

> Somebody suggested elsewhere that the name "gdb" be configurable. We
> could stick that in the same variable, like:
>
>   test "$GIT_TEST_GDB" = 1 && GIT_TEST_GDB=gdb
>   exec ${GIT_TEST_GDB} --args ...
>
> but that does not play well with the "debug" function, which does not
> know which value to set it to. I guess we would need GIT_TEST_GDB_PATH
> or something.

*nod* I think having a separate variable like GIT_TEST_GDB_COMMAND
would be fine.

An interested person could also replace 'gdb --args' with 'lldb --' if
they want to.

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> At the risk of repeating what I just said elsewhere in the thread, I
> think this patch is the best of the proposed solutions.

OK, will queue.  I agree that more could be built on top, instead of
polishing this further in place.

Thanks.

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Johannes Schindelin]

Hi Jonathan,

On Tue, 27 Oct 2015, Johannes Schindelin wrote:

> On Mon, 26 Oct 2015, Jonathan Nieder wrote:
> 
> > Does the 'exec' after the fi need this as well?  exec is supposed to
> > itself print a message and exit when it runs into an error.  Would
> > including an 'else' with the if make the control flow clearer?  E.g.
> > 
> > 	if test -n "$TEST_GDB_GIT"
> > 	then
> > 		exec gdb --args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> > 	else
> > 		exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> > 	fi
> 
> I suppose you're right! The `exec` can fail easily, e.g. when `gdb` was
> not found.

Actually, after reading the patch again, I think it is better to be less
intrusive and add the error message *just* for the gdb case, as it is
right now:

-- snipsnap --
 export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR
 
+if test -n "$TEST_GDB_GIT"
+then
+       exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
+       echo "Could not run gdb -args ${GIT_EXEC_PATH}/@@PROG@@ $*" >&2
+       exit 1
+fi
+
 exec "${GIT_EXEC_PATH}/@@PROG@@" "$@"

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Jonathan Nieder]

Johannes Schindelin wrote:
> On Tue, 27 Oct 2015, Johannes Schindelin wrote:
>> On Mon, 26 Oct 2015, Jonathan Nieder wrote:

>>> Does the 'exec' after the fi need this as well?  exec is supposed to
>>> itself print a message and exit when it runs into an error.
[...]
> Actually, after reading the patch again, I think it is better to be less
> intrusive and add the error message *just* for the gdb case, as it is
> right now:

Why?  Unlike the C library function of the same name, the shell
builtin 'exec' prints an error message and exits on error.

Sorry for the lack of clarity,
Jonathan

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Duy Nguyen]

On Mon, Oct 26, 2015 at 2:15 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> When prefixing a Git call in the test suite with 'TEST_GDB_GIT=1 ', it
> will now be run with GDB, allowing the developer to debug test failures
> more conveniently.

I'm very slowly catching up with git traffic. Apologies if it's
already mentioned elsewhere since I have only read this mail thread.

Is it more convenient to add a sh function "gdb" instead? Most of the
time I only want to stop one command, and I put "gdb /path/..../" in
front of "git ...". This gdb function could just expand to that THis
would make it a lot more convenient to debug (single command, not full
.sh file).

We can even go further with supporting gdbserver function, to launch
gdbserver, then I can debug from outside, works even without -v -i.
-- 
Duy

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Mon, Oct 26, 2015 at 2:15 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>> When prefixing a Git call in the test suite with 'TEST_GDB_GIT=1 ', it
>> will now be run with GDB, allowing the developer to debug test failures
>> more conveniently.
>
> I'm very slowly catching up with git traffic. Apologies if it's
> already mentioned elsewhere since I have only read this mail thread.
>
> Is it more convenient to add a sh function "gdb" instead?

Changing a line of git invocation you want to debug from

	git frotz &&

to

	debug git frotz &&

indeed is slightly more pleasing to the eye than

	TEST_GDB_GIT=1 git frotz &&

I do not terribly care either way, as long as that feature is
availble ;-)

Either way these tweaks are temporary changes we make while figuring
out where things go wrong, and from that point of view, (1) the
longer and more cumbersome to type, the more cumbersome to use, but
(2) the longer and more visually identifiable, the easier to spot in
"diff" a tweak you forgot to revert before committing.

> We can even go further with supporting gdbserver function, to launch
> gdbserver, then I can debug from outside, works even without -v -i.

Yes, that may be useful, but you can do so whether you use your
shell function or TEST_GDB_GIT=1 that trigeers inside the "git"
wrapper in bin-wrappers, I would think.

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Victor Leschuk]

>   
> +if test -n "$TEST_GDB_GIT"
> +then
> +	exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
Maybe we could make $TEST_GDB_GIT not just a boolean flag? It would be 
useful to contain "gdb" executable name. It would allow to set path to 
GDB when it is not in $PATH, set different debuggers (for example, I 
usually use cgdb), or even set it to /path/to/gdb_wrapper.sh which could 
contain different gdb options and tunings.

--
Victor

Re: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Johannes Schindelin]

Hi Victor,

On Thu, 29 Oct 2015, Victor Leschuk wrote:

> >   +if test -n "$TEST_GDB_GIT"
> > +then
> > +	exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> Maybe we could make $TEST_GDB_GIT not just a boolean flag? It would be useful
> to contain "gdb" executable name. It would allow to set path to GDB when it
> is not in $PATH, set different debuggers (for example, I usually use cgdb),
> or even set it to /path/to/gdb_wrapper.sh which could contain different gdb
> options and tunings.

Sure, as long as TEST_GDB_GIT=1 still works. Why don't you make an add-on
patch and submit it?

Ciao,
Johannes

RE: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Victor Leschuk]

> >   +if test -n "$TEST_GDB_GIT"
> > +then
> > +   exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> Maybe we could make $TEST_GDB_GIT not just a boolean flag? It would be useful
> to contain "gdb" executable name. It would allow to set path to GDB when it
> is not in $PATH, set different debuggers (for example, I usually use cgdb),
> or even set it to /path/to/gdb_wrapper.sh which could contain different gdb
> options and tunings.

> Sure, as long as TEST_GDB_GIT=1 still works. Why don't you make an add-on
> patch and submit it?

Hello Johannes,

Sure, I will prepare the patch as soon as this one is included in master.
--
Victor

RE: [PATCH 3/6] Facilitate debugging Git executables in tests with gdb

[Johannes Schindelin]

Hi Victor,

On Sat, 31 Oct 2015, Victor Leschuk wrote:

> > >   +if test -n "$TEST_GDB_GIT"
> > > +then
> > > +   exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
> > Maybe we could make $TEST_GDB_GIT not just a boolean flag? It would be useful
> > to contain "gdb" executable name. It would allow to set path to GDB when it
> > is not in $PATH, set different debuggers (for example, I usually use cgdb),
> > or even set it to /path/to/gdb_wrapper.sh which could contain different gdb
> > options and tunings.
> 
> > Sure, as long as TEST_GDB_GIT=1 still works. Why don't you make an add-on
> > patch and submit it?
> 
> Sure, I will prepare the patch as soon as this one is included in master.

Excuse my asking: why do you want to wait?

Ciao,
Johannes

[PATCH 4/6] Squelch warning about an integer overflow

[Johannes Schindelin]

We cannot rely on long integers to have more than 32 bits...

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-compat-util.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 805d0e2..610e8a5 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -568,7 +568,7 @@ extern int git_lstat(const char *, struct stat *);
 #endif
 
 #define DEFAULT_PACKED_GIT_LIMIT \
-	((1024L * 1024L) * (sizeof(void*) >= 8 ? 8192 : 256))
+	((1024L * 1024L) * (size_t)(sizeof(void*) >= 8 ? 8192 : 256))
 
 #ifdef NO_PREAD
 #define pread git_pread
-- 
2.1.4

Re: [PATCH 4/6] Squelch warning about an integer overflow

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> We cannot rely on long integers to have more than 32 bits...
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Interesting.  8192 * 1024 * 1024 does not fit within 32-bit long, of
course.  Perhaps we can lose L after 1024 if we are explicitly
saying that the result ought to be size_t (which may be larger than
long)?

>  git-compat-util.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 805d0e2..610e8a5 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -568,7 +568,7 @@ extern int git_lstat(const char *, struct stat *);
>  #endif
>  
>  #define DEFAULT_PACKED_GIT_LIMIT \
> -	((1024L * 1024L) * (sizeof(void*) >= 8 ? 8192 : 256))
> +	((1024L * 1024L) * (size_t)(sizeof(void*) >= 8 ? 8192 : 256))
>  
>  #ifdef NO_PREAD
>  #define pread git_pread

Re: [PATCH 4/6] Squelch warning about an integer overflow

[Johannes Schindelin]

Hi Junio,

On Mon, 26 Oct 2015, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > We cannot rely on long integers to have more than 32 bits...
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> 
> Interesting.  8192 * 1024 * 1024 does not fit within 32-bit long, of
> course.  Perhaps we can lose L after 1024 if we are explicitly
> saying that the result ought to be size_t (which may be larger than
> long)?

Sure. But it would make the patch harder to read.

Do you insist?

Ciao,
Dscho

Re: [PATCH 4/6] Squelch warning about an integer overflow

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Mon, 26 Oct 2015, Junio C Hamano wrote:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> 
>> > We cannot rely on long integers to have more than 32 bits...
>> >
>> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> > ---
>> 
>> Interesting.  8192 * 1024 * 1024 does not fit within 32-bit long, of
>> course.  Perhaps we can lose L after 1024 if we are explicitly
>> saying that the result ought to be size_t (which may be larger than
>> long)?
>
> Sure. But it would make the patch harder to read.
>
> Do you insist?

Not at all.  I think the series is already in 'next', and if I
haven't merged it yet, I should.

[PATCH 5/6] Silence GCC's "cast of pointer to integer of a different size" warning

[Johannes Schindelin]

When calculating hashes from pointers, it actually makes sense to cut
off the most significant bits. In that case, said warning does not make
a whole lot of sense.

So let's just work around it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/regex/regcomp.c | 6 ++++--
 pack-revindex.c        | 2 +-
 sha1_file.c            | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
index 06f3088..fba5986 100644
--- a/compat/regex/regcomp.c
+++ b/compat/regex/regcomp.c
@@ -18,6 +18,8 @@
    Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
    02110-1301 USA.  */
 
+#include <stdint.h>
+
 static reg_errcode_t re_compile_internal (regex_t *preg, const char * pattern,
 					  size_t length, reg_syntax_t syntax);
 static void re_compile_fastmap_iter (regex_t *bufp,
@@ -2577,7 +2579,7 @@ parse_dup_op (bin_tree_t *elem, re_string_t *regexp, re_dfa_t *dfa,
     old_tree = NULL;
 
   if (elem->token.type == SUBEXP)
-    postorder (elem, mark_opt_subexp, (void *) (long) elem->token.opr.idx);
+    postorder (elem, mark_opt_subexp, (void *) (intptr_t) elem->token.opr.idx);
 
   tree = create_tree (dfa, elem, NULL, (end == -1 ? OP_DUP_ASTERISK : OP_ALT));
   if (BE (tree == NULL, 0))
@@ -3806,7 +3808,7 @@ create_token_tree (re_dfa_t *dfa, bin_tree_t *left, bin_tree_t *right,
 static reg_errcode_t
 mark_opt_subexp (void *extra, bin_tree_t *node)
 {
-  int idx = (int) (long) extra;
+  int idx = (int) (intptr_t) extra;
   if (node->token.type == SUBEXP && node->token.opr.idx == idx)
     node->token.opt_subexp = 1;
 
diff --git a/pack-revindex.c b/pack-revindex.c
index 5c8376e..e542ea7 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -21,7 +21,7 @@ static int pack_revindex_hashsz;
 
 static int pack_revindex_ix(struct packed_git *p)
 {
-	unsigned long ui = (unsigned long)p;
+	unsigned long ui = (unsigned long)(intptr_t)p;
 	int i;
 
 	ui = ui ^ (ui >> 16); /* defeat structure alignment */
diff --git a/sha1_file.c b/sha1_file.c
index 50896ff..c5b31de 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2126,7 +2126,7 @@ static unsigned long pack_entry_hash(struct packed_git *p, off_t base_offset)
 {
 	unsigned long hash;
 
-	hash = (unsigned long)p + (unsigned long)base_offset;
+	hash = (unsigned long)(intptr_t)p + (unsigned long)base_offset;
 	hash += (hash >> 8) + (hash >> 16);
 	return hash % MAX_DELTA_CACHE;
 }
-- 
2.1.4

Re: [PATCH 5/6] Silence GCC's "cast of pointer to integer of a different size" warning

[Junio C Hamano]

OK.  Will queue.  Thanks.

[PATCH 6/6] Correct fscanf formatting string for I64u values

[Johannes Schindelin]

From: Waldek Maleska <w.maleska@gmail.com>

This fix is probably purely cosmetic because PRIuMAX is likely identical
to SCNuMAX. Nevertheless, when using a function of the scanf() family,
the correct interpolation to use is the latter, not the former.

Signed-off-by: Waldek Maleska <w.maleska@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/gc.c      | 2 +-
 git-compat-util.h | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index b677923..df3e454 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -240,7 +240,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 			 * running.
 			 */
 			time(NULL) - st.st_mtime <= 12 * 3600 &&
-			fscanf(fp, "%"PRIuMAX" %127c", &pid, locking_host) == 2 &&
+			fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 &&
 			/* be gentle to concurrent "gc" on remote hosts */
 			(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);
 		if (fp != NULL)
diff --git a/git-compat-util.h b/git-compat-util.h
index 610e8a5..87456a3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -296,6 +296,10 @@ extern char *gitbasename(char *);
 #define PRIuMAX "llu"
 #endif
 
+#ifndef SCNuMAX
+#define SCNuMAX PRIuMAX
+#endif
+
 #ifndef PRIu32
 #define PRIu32 "u"
 #endif
-- 
2.1.4

Re: [PATCH 6/6] Correct fscanf formatting string for I64u values

[Junio C Hamano]

Thanks; will queue.