[PATCH 0/3] git bisect visualize: find gitk on Windows again

[PATCH 0/3] git bisect visualize: find gitk on Windows again

[Matthias Aßhauer via GitGitGadget]

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
-- 
gitgitgadget

[PATCH 1/3] compat: make path_lookup() available outside mingw.c

[Matthias Aßhauer via GitGitGadget]

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>

Rename it to mingw_path_lookup() to avoid leading future contributors
to believe this would be portable.

This is in preparation for a patch to teach locate_in_PATH() and
exists_in_PATH() in run-command.c to work on windows.

Signed-off-by: Matthias Aßhauer <mha1993@live.de>
---
 compat/mingw.c | 20 ++++++++------------
 compat/mingw.h |  6 ++++++
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index d06cdc6254f..5d3368b1705 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1316,11 +1316,7 @@ static char *lookup_prog(const char *dir, int dirlen, const char *cmd,
 	return NULL;
 }
 
-/*
- * Determines the absolute path of cmd using the split path in path.
- * If cmd contains a slash or backslash, no lookup is performed.
- */
-static char *path_lookup(const char *cmd, int exe_only)
+char *mingw_path_lookup(const char *cmd, int exe_only)
 {
 	const char *path;
 	char *prog = NULL;
@@ -1515,7 +1511,7 @@ static int is_msys2_sh(const char *cmd)
 		if (ret >= 0)
 			return ret;
 
-		p = path_lookup(cmd, 0);
+		p = mingw_path_lookup(cmd, 0);
 		if (!p)
 			ret = 0;
 		else {
@@ -1533,7 +1529,7 @@ static int is_msys2_sh(const char *cmd)
 		static char *sh;
 
 		if (!sh)
-			sh = path_lookup("sh", 0);
+			sh = mingw_path_lookup("sh", 0);
 
 		return !fspathcmp(cmd, sh);
 	}
@@ -1646,7 +1642,7 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
 
 	strace_env = getenv("GIT_STRACE_COMMANDS");
 	if (strace_env) {
-		char *p = path_lookup("strace.exe", 1);
+		char *p = mingw_path_lookup("strace.exe", 1);
 		if (!p)
 			return error("strace not found!");
 		if (xutftowcs_path(wcmd, p) < 0) {
@@ -1801,7 +1797,7 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **deltaenv,
 		     int fhin, int fhout, int fherr)
 {
 	pid_t pid;
-	char *prog = path_lookup(cmd, 0);
+	char *prog = mingw_path_lookup(cmd, 0);
 
 	if (!prog) {
 		errno = ENOENT;
@@ -1812,7 +1808,7 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **deltaenv,
 
 		if (interpr) {
 			const char *argv0 = argv[0];
-			char *iprog = path_lookup(interpr, 1);
+			char *iprog = mingw_path_lookup(interpr, 1);
 			argv[0] = prog;
 			if (!iprog) {
 				errno = ENOENT;
@@ -1841,7 +1837,7 @@ static int try_shell_exec(const char *cmd, char *const *argv)
 
 	if (!interpr)
 		return 0;
-	prog = path_lookup(interpr, 1);
+	prog = mingw_path_lookup(interpr, 1);
 	if (prog) {
 		int exec_id;
 		int argc = 0;
@@ -1890,7 +1886,7 @@ int mingw_execv(const char *cmd, char *const *argv)
 
 int mingw_execvp(const char *cmd, char *const *argv)
 {
-	char *prog = path_lookup(cmd, 0);
+	char *prog = mingw_path_lookup(cmd, 0);
 
 	if (prog) {
 		mingw_execv(prog, argv);
diff --git a/compat/mingw.h b/compat/mingw.h
index 209cf7cebad..af1ff4be320 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -626,3 +626,9 @@ void open_in_gdb(void);
  * Used by Pthread API implementation for Windows
  */
 int err_win_to_posix(DWORD winerr);
+
+/*
+ * Determines the absolute path of cmd using the split path in path.
+ * If cmd contains a slash or backslash, no lookup is performed.
+ */
+char *mingw_path_lookup(const char *cmd, int exe_only);
-- 
gitgitgadget

[PATCH 2/3] run-command: teach locate_in_PATH about Windows

[Matthias Aßhauer via GitGitGadget]

From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>

since 5e1f28d206 (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:
mingw_path_lookup(). Teach locate_in_PATH() to use mingw_path_lookup()
on Windows.

Reported-by: Louis Strous <Louis.Strous@intellimagic.com>
Signed-off-by: Matthias Aßhauer <mha1993@live.de>
---
 run-command.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

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
 }
 
 int exists_in_PATH(const char *command)
-- 
gitgitgadget

Re: [PATCH 2/3] run-command: teach locate_in_PATH about Windows

[Junio C Hamano]

"Matthias Aßhauer via GitGitGadget"  <gitgitgadget@gmail.com>
writes:

> 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
>  }

It may be cleaner to make the above more like

	#ifndef locate_in_PATH
	static char *locate_in_PATH(const char *file)
	{
	    ... original implementation without any #ifdef ...
	}
	#endif

and redo the [1/3] patch so that it does not rename or otherwise
touch path_lookup() in any way, and instead implements a
mingw_locate_in_PATH() in terms of path_lookup() and make it public,
declare it in <compat/mingw.h>, together with #define
locate_in_PATH(), i.e. [1/3] will essentially become something like:

    (add to compat/mingw.c)
    char *mingw_locate_in_PATH(const char *file)
    {
	return path_lookup(file, 0);
    }

    (add to compat/mingw.h)
    extern char *mingw_locate_in_PATH(const char *);
    #define locate_in_PATH(file) mingw_locate_in_PATH(file)

That way, the second non-UNIXy system can add its own way to locate
an executable in PATH without having to touch the main part of the
system, right?

[PATCH 3/3] docs: update when `git bisect visualize` uses `gitk`

[Matthias Aßhauer via GitGitGadget]

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`.
 
 ------------
 $ git bisect visualize --stat
-- 
gitgitgadget

Re: [PATCH 3/3] docs: update when `git bisect visualize` uses `gitk`

[Junio C Hamano]

"Matthias Aßhauer via GitGitGadget"  <gitgitgadget@gmail.com>
writes:

> 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`.

Good.  Would a casual (read: not working on) Git for Windows user
know if their environment has MSYSTEM variable?  The same question
applies to the other variables with relevant platforms.  I think
folks working in GUI environment of X Window pedigree may be
familiar enough with DISPLAY (but of course I am biased as I have
used such an envionrment in the past) that the original description
is permissible without extra explanation, but among these new ones
some may deserve an additional short description in parentheses,
e.g.

	..., `MSYSTEM` (always set in Git for Windows) and ...

Other than that, this is a very welcome addition.

Thanks.

Re: [PATCH 0/3] git bisect visualize: find gitk on Windows again

[Junio C Hamano]

"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.


> [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

Re: [PATCH 0/3] git bisect visualize: find gitk on Windows again

[Junio C Hamano]

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.

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

Re: [PATCH 0/3] git bisect visualize: find gitk on Windows again

[Matthias Aßhauer]

On Thu, 3 Aug 2023, Junio C Hamano wrote:

> 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.

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.

Re: [PATCH 0/3] git bisect visualize: find gitk on Windows again

[Junio C Hamano]

Matthias Aßhauer <mha1993@live.de> writes:

>> 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.

OK.  Thanks.

[PATCH v2 0/3] git bisect visualize: find gitk on Windows again

[Matthias Aßhauer via GitGitGadget]

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. It was introduced during the
conversion of git bisect visualize to a builtin.

This patch series fixes that regression.

Changes since v1:

 * simplified patches 1 and 2 based on Junios feedback.
 * expanded the new wording in the documentation to clarify which variables
   users might encounter

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

Matthias Aßhauer (3):
  run-command: conditionally define locate_in_PATH()
  compat/mingw: implement a native locate_in_PATH()
  docs: update when `git bisect visualize` uses `gitk`

 Documentation/git-bisect.txt | 11 ++++++++---
 compat/mingw.c               |  5 +++++
 compat/mingw.h               |  3 +++
 run-command.c                |  2 ++
 4 files changed, 18 insertions(+), 3 deletions(-)


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

Range-diff vs v1:

 1:  6ed968e1288 < -:  ----------- compat: make path_lookup() available outside mingw.c
 2:  bf8b34aaef3 ! 1:  dc9c0812d20 run-command: teach locate_in_PATH about Windows
     @@ Metadata
      Author: Matthias Aßhauer <mha1993@live.de>
      
       ## Commit message ##
     -    run-command: teach locate_in_PATH about Windows
     +    run-command: conditionally define locate_in_PATH()
      
     -    since 5e1f28d206 (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:
     -    mingw_path_lookup(). Teach locate_in_PATH() to use mingw_path_lookup()
     -    on Windows.
     +    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().
      
     -    Reported-by: Louis Strous <Louis.Strous@intellimagic.com>
          Signed-off-by: Matthias Aßhauer <mha1993@live.de>
      
       ## run-command.c ##
      @@ run-command.c: 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;
     + 	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
      @@ run-command.c: static char *locate_in_PATH(const char *file)
     - 
       	strbuf_release(&buf);
       	return NULL;
     -+#else
     -+	return mingw_path_lookup(file,0);
     -+#endif
       }
     ++#endif
       
       int exists_in_PATH(const char *command)
     + {
 -:  ----------- > 2:  8b8c8c3f70a compat/mingw: implement a native locate_in_PATH()
 3:  c872431b608 ! 3:  04227199089 docs: update when `git bisect visualize` uses `gitk`
     @@ Documentation/git-bisect.txt: 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`.
     -+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`.
     ++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.
     ++
     ++If none of these environment variables is set, 'git log' is used instead.
     ++You can also give command-line options such as `-p` and `--stat`.
       
       ------------
       $ git bisect visualize --stat

-- 
gitgitgadget

[PATCH v2 1/3] run-command: conditionally define locate_in_PATH()

[Matthias Aßhauer via GitGitGadget]

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

Re: [PATCH v2 1/3] run-command: conditionally define locate_in_PATH()

[Junio C Hamano]

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

Re: [PATCH v2 1/3] run-command: conditionally define locate_in_PATH()

[Matthias Aßhauer]

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

Re: [PATCH v2 1/3] run-command: conditionally define locate_in_PATH()

[Junio C Hamano]

Matthias Aßhauer <mha1993@live.de> writes:

> 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 ;-)

Let's not bother immediately before -rc0; letting people use "gitk"
during "git bisect" without having to type 5 extra keystrokes in the
upcoming release is a good outcome and we can touch up the in-code
comment later.

Thanks.

[PATCH v2 2/3] compat/mingw: implement a native locate_in_PATH()

[Matthias Aßhauer via GitGitGadget]

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)
-- 
gitgitgadget

Re: [PATCH v2 2/3] compat/mingw: implement a native locate_in_PATH()

[Junio C Hamano]

"Matthias Aßhauer via GitGitGadget"  <gitgitgadget@gmail.com>
writes:

> 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(+)

Makes perfect sense ;-)

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

[PATCH v2 3/3] docs: update when `git bisect visualize` uses `gitk`

[Matthias Aßhauer via GitGitGadget]

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.
+
+If none of these environment variables is set, 'git log' is used instead.
+You can also give command-line options such as `-p` and `--stat`.
 
 ------------
 $ git bisect visualize --stat
-- 
gitgitgadget

Re: [PATCH v2 3/3] docs: update when `git bisect visualize` uses `gitk`

[Junio C Hamano]

"Matthias Aßhauer via GitGitGadget"  <gitgitgadget@gmail.com>
writes:

> 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.

Great.

> +If none of these environment variables is set, 'git log' is used instead.
> +You can also give command-line options such as `-p` and `--stat`.

Micronit.  I think "is set" want to be "are set", as "none" in "none
of these" is used for "not any" [*].

>  
>  ------------
>  $ git bisect visualize --stat


[Reference]

* https://www.merriam-webster.com/video/is-none-singular-or-plural

Re: [PATCH v2 3/3] docs: update when `git bisect visualize` uses `gitk`

[Eric Sunshine]

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".

Probably not worth a reroll.

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

Re: [PATCH v2 3/3] docs: update when `git bisect visualize` uses `gitk`

[Matthias Aßhauer]

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.

> Probably not worth a reroll.
>
> [1]: https://github.com/vercel/hyper/issues/482
>

Re: [PATCH v2 3/3] docs: update when `git bisect visualize` uses `gitk`

[Junio C Hamano]

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:
>>> 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.

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

Re: [PATCH v2 3/3] docs: update when `git bisect visualize` uses `gitk`

[Eric Sunshine]

On Fri, Aug 4, 2023 at 12:45 PM Junio C Hamano <gitster@pobox.com> wrote:
> 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?

I would think so.