git bisect visualize: find gitk on Windows again

Louis Strous reported a regression in git bisect visualize on Windows[1] that caused git bisect visualize to use git log instead of gitk unless explicitly called as git bisect visualize gitk.

This patch series fixes that regression.

[1]
https://lore.kernel.org/git/VI1PR10MB2462F7B52FF2E3F59AFE94A7F500A@VI1PR10MB2462.EURPRD10.PROD.OUTLOOK.COM/

Matthias Aßhauer (3):
compat: make path_lookup() available outside mingw.c
run-command: teach locate_in_PATH about Windows
docs: update when git bisect visualize uses gitk

 Documentation/git-bisect.txt |  6 +++---
 compat/mingw.c               | 20 ++++++++------------
 compat/mingw.h               |  6 ++++++
 run-command.c                |  8 ++++----
 4 files changed, 21 insertions(+), 19 deletions(-)

base-commit: fb7d80edcae482f4fa5d4be0227dc3054734e5f3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1560%2Frimrul%2Fwin-bisect-visualize-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1560/rimrul/win-bisect-visualize-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1560

run-command.c +1 -4

index 60c94198664..8f518e37e27 100644
@@ -182,13 +182,10 @@ int is_executable(const char *name)
  * Returns the path to the command, as found in $PATH or NULL if the
  * command could not be found.  The caller inherits ownership of the memory
  * used to store the resultant path.
- *
- * This should not be used on Windows, where the $PATH search rules
- * are more complicated (e.g., a search for "foo" should find
- * "foo.exe").
  */
 static char *locate_in_PATH(const char *file)
 {
+#ifndef GIT_WINDOWS_NATIVE
 	const char *p = getenv("PATH");
 	struct strbuf buf = STRBUF_INIT;
 

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <[email protected]>

This check has involved more environment variables than just DISPLAY since 508e84a790 (bisect view: check for MinGW32 and MacOSX in addition to X11, 2008-02-14), so let's update the documentation accordingly.

Signed-off-by Matthias Aßhauer <[email protected]>

---
 Documentation/git-bisect.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Documentation/git-bisect.txt +0 -0

index fbb39fbdf5d..82b1d5ac6c5 100644

"Matthias Aßhauer via GitGitGadget" <[email protected]>

Quoted 4 lines · writes:

Louis Strous reported a regression in git bisect visualize on Windows[1] that caused git bisect visualize to use git log instead of gitk unless explicitly called as git bisect visualize gitk.

Quoted 1 line

This patch series fixes that regression.

Wonderful. It would be nice to describe where in the release sequence the "regression" happened, if we know it. Perhaps you've written about it in one of the patches? We'll find out.

Thanks again.

Quoted 13 lines · Junio C Hamano

"Matthias Aßhauer via GitGitGadget" <[email protected]>

Quoted 5 lines · writes:

Louis Strous reported a regression in git bisect visualize on Windows[1] that caused git bisect visualize to use git log instead of gitk unless explicitly called as git bisect visualize gitk.

This patch series fixes that regression.

Wonderful. It would be nice to describe where in the release sequence the "regression" happened, if we know it. Perhaps you've written about it in one of the patches? We'll find out.

Quoted 1 line

Thanks again.

And the answer was in [2/3], if I am reading the proposed log messages correctly. When the "bisect visualize" was ported to C, we broke it.

Thanks. Will try to queue based on 'maint'.

Quoted 3 lines

And the answer was in [2/3], if I am reading the proposed log messages correctly. When the "bisect visualize" was ported to C, we broke it.

Yes, that's correct.

Quoted 3 lines

Thanks. Will try to queue based on 'maint'.

Please wait a moment, I'm currently preparing a V2 based on your feedback.

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <[email protected]>

This commit doesn't change any behaviour by itself, but allows us to easily define compat replacements for locate_in_PATH(). It prepares us for the next commit that adds a native Windows implementation of locate_in_PATH().

Signed-off-by Matthias Aßhauer <[email protected]>

---
 run-command.c | 2 ++
 1 file changed, 2 insertions(+)

run-command.c +0 -0

index 60c94198664..85fc1507288 100644

run-command.c +1 -0

@@ -218,6 +219,7 @@ static char *locate_in_PATH(const char *file)
 	strbuf_release(&buf);
 	return NULL;
 }
+#endif
 
 int exists_in_PATH(const char *command)
 {

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <[email protected]>

since 5e1f28d (bisect--helper: reimplement bisect_visualize() shell
function in C, 2021-09-13) git bisect visualize uses exists_in_PATH()
to check wether it should call gitk, but exists_in_PATH() relies on locate_in_PATH() which currently only understands POSIX-ish PATH variables (a list of paths, separated by colons) on native Windows executables we encounter Windows PATH variables (a list of paths that often contain drive letters (and thus colons), separated by semicolons). Luckily we do already have a function that can lookup executables on windows PATHs:
path_lookup(). Implement a small replacement for the existing locate_in_PATH() based on path_lookup().

Reported-by Louis Strous <[email protected]>

Signed-off-by Matthias Aßhauer <[email protected]>

---
 compat/mingw.c | 5 +++++
 compat/mingw.h | 3 +++
 2 files changed, 8 insertions(+)

compat/mingw.c +5 -0

index d06cdc6254f..bc3669d2986 100644
@@ -1347,6 +1347,11 @@ static char *path_lookup(const char *cmd, int exe_only)
 	return prog;
 }
 
+char *mingw_locate_in_PATH(const char *cmd)
+{
+	return path_lookup(cmd, 0);
+}
+
 static const wchar_t *wcschrnul(const wchar_t *s, wchar_t c)
 {
 	while (*s && *s != c)

compat/mingw.h +3 -0

index 209cf7cebad..b5262205965 100644
@@ -175,6 +175,9 @@ pid_t waitpid(pid_t pid, int *status, int options);
 #define kill mingw_kill
 int mingw_kill(pid_t pid, int sig);
 
+#define locate_in_PATH mingw_locate_in_PATH
+char *mingw_locate_in_PATH(const char *cmd);
+
 #ifndef NO_OPENSSL
 #include <openssl/ssl.h>
 static inline int mingw_SSL_set_fd(SSL *ssl, int fd)

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <[email protected]>

This check has involved more environment variables than just DISPLAY since 508e84a790 (bisect view: check for MinGW32 and MacOSX in addition to X11, 2008-02-14), so let's update the documentation accordingly.

Signed-off-by Matthias Aßhauer <[email protected]>

---
 Documentation/git-bisect.txt | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Documentation/git-bisect.txt +0 -0

index fbb39fbdf5d..bec8d2abb22 100644
@@ -204,9 +204,14 @@ as an alternative to `visualize`):
 $ git bisect visualize

This check has involved more environment variables than just DISPLAY since 508e84a790 (bisect view: check for MinGW32 and MacOSX in addition to X11, 2008-02-14), so let's update the documentation accordingly.

Signed-off-by Matthias Aßhauer <[email protected]>

---

"Matthias Aßhauer via GitGitGadget" <[email protected]>

Quoted 19 lines · writes:

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <[email protected]>

This commit doesn't change any behaviour by itself, but allows us to easily define compat replacements for locate_in_PATH(). It prepares us for the next commit that adds a native Windows implementation of locate_in_PATH().

Signed-off-by Matthias Aßhauer <[email protected]>

---
 run-command.c | 2 ++
 1 file changed, 2 insertions(+)

run-command.c +0 -0

index 60c94198664..85fc1507288 100644
@@ -170,6 +170,7 @@ int is_executable(const char *name)
 	return st.st_mode & S_IXUSR;
 }
 

run-command.c int is_executable(const char *name) · line 176

Quoted 4 lines

run-command.c +1 -0

+#ifndef locate_in_PATH
 /*
  * Search $PATH for a command.  This emulates the path search that
  * execvp would perform, without actually executing the command so it

Micronit. The comment should be shared across different platform implementations of this interface, so "#ifndef" would want to come immediately after this comment, not before, I would think.

It does not affect the correctness, of course ;-)

Quoted 8 lines

run-command.c +1 -0

@@ -218,6 +219,7 @@ static char *locate_in_PATH(const char *file)
 	strbuf_release(&buf);
 	return NULL;
 }
+#endif
 
 int exists_in_PATH(const char *command)
 {

Quoted 13 lines · Matthias Aßhauer via GitGitGadget

This check has involved more environment variables than just DISPLAY since 508e84a790 (bisect view: check for MinGW32 and MacOSX in addition to X11, 2008-02-14), so let's update the documentation accordingly.

Signed-off-by Matthias Aßhauer <[email protected]>

---

Documentation/git-bisect.txt +2 -3

@@ -204,9 +204,14 @@ as an alternative to `visualize`):
-If the `DISPLAY` environment variable is not set, 'git log' is used
-instead.  You can also give command-line options such as `-p` and
-`--stat`.
+Git detects a graphical environment through various environment variables:
+`DISPLAY`, which is set in X Window System environments on Unix systems.

Documentation/git-bisect.txt as an alternative to `visualize`): · line 211

Quoted 3 lines

Documentation/git-bisect.txt +3 -0

+`SESSIONNAME`, which is set under Cygwin in interactive desktop sessions.
+`MSYSTEM`, which is set under Msys2 and Git for Windows.
+`SECURITYSESSIONID`, which is set on macOS in interactive desktop sessions.

Micronit: SECURITYSESSIONID is not universal on macOS[1]; some people report its presence in iTerm2 and HyperTerm, and perhaps even Apple's own Terminal (although it's not defined for me in Terminal on High Sierra). Perhaps just say "may be set on macOS".

Probably not worth a reroll.

[1]: https://github.com/vercel/hyper/issues/482

Quoted 22 lines · Junio C Hamano

"Matthias Aßhauer via GitGitGadget" <[email protected]>

Quoted 19 lines · writes:

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <[email protected]>

This commit doesn't change any behaviour by itself, but allows us to easily define compat replacements for locate_in_PATH(). It prepares us for the next commit that adds a native Windows implementation of locate_in_PATH().

Signed-off-by Matthias Aßhauer <[email protected]>

---
 run-command.c | 2 ++
 1 file changed, 2 insertions(+)

run-command.c +0 -0

index 60c94198664..85fc1507288 100644
@@ -170,6 +170,7 @@ int is_executable(const char *name)
 	return st.st_mode & S_IXUSR;
 }
 

Quoted 8 lines

run-command.c int is_executable(const char *name) · line 176

Quoted 4 lines

run-command.c +1 -0

+#ifndef locate_in_PATH
 /*
  * Search $PATH for a command.  This emulates the path search that
  * execvp would perform, without actually executing the command so it

Micronit. The comment should be shared across different platform implementations of this interface, so "#ifndef" would want to come immediately after this comment, not before, I would think.

I can see the first part applying to all implementations, but the last part about it not working on windows is specific to this implementation.

I guess we could split the comment, if we wanted to make that clear.

Quoted 1 line

It does not affect the correctness, of course ;-)

Quoted 15 lines · Eric Sunshine

Quoted 13 lines · Matthias Aßhauer via GitGitGadget

This check has involved more environment variables than just DISPLAY since 508e84a790 (bisect view: check for MinGW32 and MacOSX in addition to X11, 2008-02-14), so let's update the documentation accordingly.

Signed-off-by Matthias Aßhauer <[email protected]>

---

Documentation/git-bisect.txt +2 -3

@@ -204,9 +204,14 @@ as an alternative to `visualize`):
-If the `DISPLAY` environment variable is not set, 'git log' is used
-instead.  You can also give command-line options such as `-p` and
-`--stat`.
+Git detects a graphical environment through various environment variables:
+`DISPLAY`, which is set in X Window System environments on Unix systems.

Quoted 8 lines

Documentation/git-bisect.txt as an alternative to `visualize`): · line 211

Quoted 3 lines

Documentation/git-bisect.txt +3 -0

+`SESSIONNAME`, which is set under Cygwin in interactive desktop sessions.
+`MSYSTEM`, which is set under Msys2 and Git for Windows.
+`SECURITYSESSIONID`, which is set on macOS in interactive desktop sessions.

Micronit: SECURITYSESSIONID is not universal on macOS[1]; some people report its presence in iTerm2 and HyperTerm, and perhaps even Apple's own Terminal (although it's not defined for me in Terminal on High Sierra). Perhaps just say "may be set on macOS".

I've just checked in Terminal on Ventura and it isn't set for me either. I'll reword it.

Quoted 14 lines · Matthias Aßhauer

Quoted 12 lines · Eric Sunshine

Quoted 5 lines · Matthias Aßhauer via GitGitGadget

+Git detects a graphical environment through various environment variables:
+DISPLAY, which is set in X Window System environments on Unix systems. +SESSIONNAME, which is set under Cygwin in interactive desktop sessions. +MSYSTEM, which is set under Msys2 and Git for Windows. +SECURITYSESSIONID, which is set on macOS in interactive desktop sessions.

Micronit: SECURITYSESSIONID is not universal on macOS[1]; some people report its presence in iTerm2 and HyperTerm, and perhaps even Apple's own Terminal (although it's not defined for me in Terminal on High Sierra). Perhaps just say "may be set on macOS".

Quoted 2 lines

I've just checked in Terminal on Ventura and it isn't set for me either. I'll reword it.

I'll locally tweak "which may be set on macOS" for now. It should be good enough for the upcoming release, right?