patch, 3 partsgit bisect visualize: find gitk on Windows again

> From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>
>
> 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 <mha1993@live.de>
> ---
>  Documentation/git-bisect.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
> index fbb39fbdf5d..82b1d5ac6c5 100644
> --- a/Documentation/git-bisect.txt
> +++ b/Documentation/git-bisect.txt
> @@ -204,9 +204,9 @@ as an alternative to `visualize`):
>  $ git bisect 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`.
> +If none of the environment variables `DISPLAY`, `SESSIONNAME`, `MSYSTEM` and
> +`SECURITYSESSIONID` is set, 'git log' is used instead.  You can also give
> +command-line options such as `-p` and `--stat`.

> diff --git a/run-command.c b/run-command.c
> index 60c94198664..8f518e37e27 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -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;
>  
> @@ -217,6 +214,9 @@ static char *locate_in_PATH(const char *file)
>  
>  	strbuf_release(&buf);
>  	return NULL;
> +#else
> +	return mingw_path_lookup(file,0);
> +#endif
>  }

> 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

> "Matthias Aßhauer via GitGitGadget"  <gitgitgadget@gmail.com>
> 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.
>
> Thanks again.

> Junio C Hamano <gitster@pobox.com> writes:
>
>> "Matthias Aßhauer via GitGitGadget"  <gitgitgadget@gmail.com>
>> 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.
>>
>> 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.

>> 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.
>
>>
>> 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?= <mha1993@live.de>
>
> 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 <mha1993@live.de>
> ---
>  run-command.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/run-command.c b/run-command.c
> index 60c94198664..85fc1507288 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -170,6 +170,7 @@ int is_executable(const char *name)
>  	return st.st_mode & S_IXUSR;
>  }
>  
> +#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

> @@ -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)
>  {

> "Matthias Aßhauer via GitGitGadget"  <gitgitgadget@gmail.com>
> writes:
>
>> From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>
>>
>> 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 <mha1993@live.de>
>> ---
>>  run-command.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 60c94198664..85fc1507288 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -170,6 +170,7 @@ int is_executable(const char *name)
>>  	return st.st_mode & S_IXUSR;
>>  }
>>
>> +#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 ;-)
>
>> @@ -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)
>>  {
>

> On Thu, 3 Aug 2023, Junio C Hamano wrote:
>
>> "Matthias Aßhauer via GitGitGadget"  <gitgitgadget@gmail.com>
>> writes:
>>
>>> From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>
>>>
>>> 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 <mha1993@live.de>
>>> ---
>>>  run-command.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/run-command.c b/run-command.c
>>> index 60c94198664..85fc1507288 100644
>>> --- a/run-command.c
>>> +++ b/run-command.c
>>> @@ -170,6 +170,7 @@ int is_executable(const char *name)
>>>  	return st.st_mode & S_IXUSR;
>>>  }
>>>
>>> +#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.
>
>> It does not affect the correctness, of course ;-)

> From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>
>
> 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 <mha1993@live.de>
> ---
>  Documentation/git-bisect.txt | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
> index fbb39fbdf5d..bec8d2abb22 100644
> --- a/Documentation/git-bisect.txt
> +++ b/Documentation/git-bisect.txt
> @@ -204,9 +204,14 @@ as an alternative to `visualize`):
>  $ git bisect 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.
> +`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.

> 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 <mha1993@live.de>
> ---
> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
> @@ -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.
> +`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.

> On Fri, Aug 4, 2023 at 1:22 AM Matthias Aßhauer via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> 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 <mha1993@live.de>
>> ---
>> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
>> @@ -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.
>> +`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".

> On Fri, 4 Aug 2023, Eric Sunshine wrote:
>
>> On Fri, Aug 4, 2023 at 1:22 AM Matthias Aßhauer via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>>> 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 <mha1993@live.de>
>>> ---
>>> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
>>> @@ -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.
>>> +`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.

> Matthias Aßhauer <mha1993@live.de> writes:
> > On Fri, 4 Aug 2023, Eric Sunshine wrote:
> >> On Fri, Aug 4, 2023 at 1:22 AM Matthias Aßhauer via GitGitGadget
> >> <gitgitgadget@gmail.com> wrote:
> >>> +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".
> >
> > 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?

> From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>
>
> 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 <Louis.Strous@intellimagic.com>
> Signed-off-by: Matthias Aßhauer <mha1993@live.de>
> ---
>  compat/mingw.c | 5 +++++
>  compat/mingw.h | 3 +++
>  2 files changed, 8 insertions(+)

> diff --git a/compat/mingw.c b/compat/mingw.c
> index d06cdc6254f..bc3669d2986 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -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)
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 209cf7cebad..b5262205965 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -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)