run-command: trigger PATH lookup properly on Cygwin

[r0mai]

On Cygwin, the codepath for POSIX-like systems is taken in
run-command.c::start_command(). The prepare_cmd() helper
function is called to decide if the command needs to be looked
up in the PATH. The logic there is to do the PATH-lookup if
and only if it does not have any slash '/' in it. If this test
passes we end up attempting to run the command by appending the
string after each colon-separated component of PATH.

The Cygwin environment supports both Windows and POSIX style
paths, so both forwardslahes '/' and back slashes '' can be
used as directory separators for any external program the user
supplies.

Examples for path strings which are being incorrectly searched
for in the PATH instead of being executed as is:

• "C:\Program Files\some-program.exe"
• "a\b\c.exe"

To handle these, the PATH lookup detection logic in prepare_cmd()
is taught to know about this Cygwin quirk, by introducing
has_dir_sep(path) helper function to abstract away the difference
between true POSIX and Cygwin systems.

Signed-off-by: Andras Kucsma r0maikx02b@gmail.com

Changes since v1:

• Avoid scanning the whole path for a directory separator even if one is found earlier as suggested by Junio C Hamano.
  Changes since v2:
• Rephrased the commit message based on Junio's suggestion.

CC: Torsten Bögershausen tboegi@web.de, Junio C Hamano gitster@pobox.com

[gitgitgadget]

Welcome to GitGitGadget

Hi @r0mai, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

• the lines should not exceed 76 columns,
• the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and
• the commit messages' body should be describing the "why?" of the change.
• Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

• Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the FreeNode IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Freenode. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

[dscho]

/allow

[gitgitgadget]

User r0mai is now allowed to use GitGitGadget.

WARNING: r0mai has no public email address set on GitHub

[r0mai]

/submit

[gitgitgadget]

Submitted as pull.587.git.1584997990694.gitgitgadget@gmail.com

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"András Kucsma via GitGitGadget"  <gitgitgadget@gmail.com> writes:

> From: Andras Kucsma <r0maikx02b@gmail.com>
>
> On Windows with git installed through cygwin, GIT_ASKPASS failed to run
> for relative and absolute paths containing only backslashes as directory
> separators.
>
> The reason was that git assumed that if there are no forward slashes in
> the executable path, it has to search for the executable on the PATH.

Also if I were reading the discussion correctly, there was a doubt
about locate_in_PATH() that may not work on Windows for at least two
reasons.  Is it OK to ignore these issues, and if so why?

I know if you have a full path, a broken locate_in_PATH() would be
skipped and won't cause an immediate issue, but this change to make
the code realize that "a\\b" is not asking to search in %PATH% feels
just a beginning of a fix, not the whole fix, at least to me.

> The fix is to look for OS specific directory separators, not just
> forward slashes.

Yes, but it is quite unfortunate that you would use a function that
has to scan the string to the end because it asks for the last one.

Perhaps introduce 

--------------------------------------------------
#ifndef has_dir_sep
static inline int git_has_dir_sep(const char *path)
{
	return !!strchr(path, '/');
}
#define has_dir_sep(path) git_has_dir_sep(path)
#endif
--------------------------------------------------

in <git-compat-util.h>, with a replacement definition in
<compat/win32/path-utils.h> that may read

--------------------------------------------------
#define has_dir_sep(path) win32_has_dir_sep(path)
static inline int has_dir_sep(const char *path)
{
        /* 
         * See how long the non-separator part of the given path is, and
         * if and only if it covers the whole path (i.e. path[len] is NUL),
         * there is no separator in the path---otherwise there is a separaptor.
         */
        size_t len = strcspn(path, "/\\");
        return !!path[len];
}
--------------------------------------------------

and use that instead?


> diff --git a/run-command.c b/run-command.c
> index f5e1149f9b3..9fcc12ebf9c 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -421,12 +421,12 @@ static int prepare_cmd(struct argv_array *out, const struct child_process *cmd)
>  	}
>  
>  	/*
> -	 * If there are no '/' characters in the command then perform a path
> -	 * lookup and use the resolved path as the command to exec.  If there
> -	 * are '/' characters, we have exec attempt to invoke the command
> -	 * directly.
> +	 * If there are no dir separator characters in the command then perform
> +	 * a path lookup and use the resolved path as the command to exec. If
> +	 * there are dir separator characters, we have exec attempt to invoke
> +	 * the command directly.
>  	 */
> -	if (!strchr(out->argv[1], '/')) {
> +	if (find_last_dir_sep(out->argv[1]) == NULL) {
>  		char *program = locate_in_PATH(out->argv[1]);
>  		if (program) {
>  			free((char *)out->argv[1]);
>
> base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925

[r0mai]

/submit

[gitgitgadget]

Submitted as pull.587.v2.git.1585143910604.gitgitgadget@gmail.com

[gitgitgadget]

On the Git mailing list, Torsten Bögershausen wrote (reply to this):

Thanks for working on this. I have 1 or 2 nits/questions, please see below.

On Wed, Mar 25, 2020 at 01:45:10PM +0000, András Kucsma via GitGitGadget wrote:
> From: Andras Kucsma <r0maikx02b@gmail.com>
>
> On Windows with git installed through cygwin, GIT_ASKPASS failed to run

My understanding is, that git under cygwin needs this patch (so to say),
but isn't it so, that even Git for Windows has the same issue ?
The headline of the patch and the indicate so.
How about the following ?

On Windows GIT_ASKPASS failed to run for relative and absolute paths
containing only backslashes as directory separators.
The reason was that Git assumed that if there are no forward slashes in
the executable path, it has to search for the executable on the PATH.

The fix is to look for OS specific directory separators, not just
forward slashes, so introduce a helper function has_dir_sep() and use it
in run-command.

[gitgitgadget]

On the Git mailing list, András Kucsma wrote (reply to this):

On Wed, Mar 25, 2020 at 5:35 PM Torsten Bögershausen <tboegi@web.de> wrote:
>
> Thanks for working on this. I have 1 or 2 nits/questions, please see below.
>
> On Wed, Mar 25, 2020 at 01:45:10PM +0000, András Kucsma via GitGitGadget wrote:
> > From: Andras Kucsma <r0maikx02b@gmail.com>
> >
> > On Windows with git installed through cygwin, GIT_ASKPASS failed to run
>
> My understanding is, that git under cygwin needs this patch (so to say),
> but isn't it so, that even Git for Windows has the same issue ?
> The headline of the patch and the indicate so.

Git for Windows does not have this issue, because there
GIT_WINDOWS_NATIVE is defined, which is not true under Cygwin:
https://github.com/git/git/blob/274b9cc2/git-compat-util.h#L157-L165

You can see in start_command() that there are separate implementations
based on GIT_WINDOWS_NATIVE. The problematic code is in prepare_cmd,
which is not called in the branch where GIT_WINDOWS_NATIVE is defined:
https://github.com/git/git/blob/274b9cc2/run-command.c#L740

This means, that cygwin is running in the Unix-like branch of the
code, even though it supports backslashes in its paths.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"András Kucsma via GitGitGadget"  <gitgitgadget@gmail.com> writes:

> From: Andras Kucsma <r0maikx02b@gmail.com>
>
> On Windows with git installed through cygwin, GIT_ASKPASS failed to run
> for relative and absolute paths containing only backslashes as directory
> separators.

> Subject: [PATCH v2] Fix dir sep handling of GIT_ASKPASS on Windows

Isn't it curious that there is nothing in the code that was touched
that is specific to GIT_ASKPASS?  We shouldn't have to see that in
the title.

Perhaps

    Subject: run-command: notice needs for PATH-lookup correctly on Cygwin

    On Cygwin, the codepath for POSIX-like systems is taken in
    run-command.c::start_command().  The prepare_cmd() helper
    function is called to decide if the command needs to be looked
    up in the $PATH, and the logic there is to do the PATH-lookup if
    and only if it does not have any slash '/' in it.

    Unfortunately, a end-user can give "c:\program files\askpass" or
    "a\b\c" to be absolute or relative path to the command, but in
    these strings there is no '/'.  We end up attempting to run the
    command by appending the absoluter or relative path after each
    colon-separated component of $PATH.

    Instead, introduce a has_dir_sep(path) helper function to
    abstract away the difference between true POSIX and Cygwin, and
    use it to make the decision for PATH-lookup.

Having said all that, I am not sure if we need to change anything.

As Cygwin is about trying to mimicking UNIXy environment as much as
possible, shouldn't "GIT_ASKPASS=//c/program files/askpass" the way
end-users would expect to work, not the one that uses backslashes?

And if the user pretends to be on UNIXy system by using Cygwin by
using slashes when specifying these commands run via the run_command
API, the code makes the decision for PATH-lookup quite correctly,
no?

So...

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Torsten Bögershausen <tboegi@web.de> writes:

>> On Windows with git installed through cygwin, GIT_ASKPASS failed to run
>
> My understanding is, that git under cygwin needs this patch (so to say),
> but isn't it so, that even Git for Windows has the same issue ?
> The headline of the patch and the indicate so.

Yes, I agree that the commit is mistitled.  It is not specific to
Windows (it only is Cygwin, as the support for native Windows goes a
separate codepath), and it is not specific to GIT_ASKPASS, either.

[gitgitgadget]

On the Git mailing list, András Kucsma wrote (reply to this):

On Thu, Mar 26, 2020 at 10:14 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "András Kucsma via GitGitGadget"  <gitgitgadget@gmail.com> writes:
>
> > From: Andras Kucsma <r0maikx02b@gmail.com>
> >
> > On Windows with git installed through cygwin, GIT_ASKPASS failed to run
> > for relative and absolute paths containing only backslashes as directory
> > separators.
>
> > Subject: [PATCH v2] Fix dir sep handling of GIT_ASKPASS on Windows
>
> Isn't it curious that there is nothing in the code that was touched
> that is specific to GIT_ASKPASS?  We shouldn't have to see that in
> the title.

You're completely right, I'll rephrase the commit message based on
your suggestion and resubmit.

> Having said all that, I am not sure if we need to change anything.
>
> As Cygwin is about trying to mimicking UNIXy environment as much as
> possible, shouldn't "GIT_ASKPASS=//c/program files/askpass" the way
> end-users would expect to work, not the one that uses backslashes?
>
> And if the user pretends to be on UNIXy system by using Cygwin by
> using slashes when specifying these commands run via the run_command
> API, the code makes the decision for PATH-lookup quite correctly,
> no?
>
> So...

Cygwin provides a Unix like environment, while also maintaining
Windows compatibility, at least as far as path handling is concerned.
As a quick test, fopen can handle forward slashes, backslashes too.
These four all work under cygwin:

fopen("C:\\file.txt", "r");
fopen("C:/file.txt", "r");
fopen("/cygdrive/c/file.txt", "r");
fopen("/cygdrive\\c\\file.txt", "r");

There seems to be a precedent to support Cygwin as a kind of "hybrid"
platform in the git codebase. In git-compat-util.h, the
compat/win32/path-utils.h header is included, but GIT_WINDOWS_NATIVE
is not defined.

https://github.com/git/git/blob/a7d14a442/git-compat-util.h#L204-L206
https://github.com/git/git/blob/a7d14a442/git-compat-util.h#L157-L165

The compat/win32/path-utils.h header mostly provides utilities dealing
with directory separator related logic on Windows, but these utilities
are being used in the Unixy code paths on Cygwin.

The current version of the patch fits into this pattern. It only
changes behaviour under Cygwin, not touching pure Windows and
non-Cygwin Unix variants at all.

[r0mai]

/submit

[gitgitgadget]

Submitted as pull.587.v3.git.1585269403947.gitgitgadget@gmail.com

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"András Kucsma via GitGitGadget"  <gitgitgadget@gmail.com> writes:

> Subject: Re: [PATCH v3] run-command: trigger PATH lookup properly on Cygwin

You phrased it much better than my earlier attempt.  Succinct,
accurate and to the point.  Good.

>  compat/win32/path-utils.h | 11 +++++++++++
>  git-compat-util.h         |  8 ++++++++
>  run-command.c             | 10 +++++-----
>  3 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/compat/win32/path-utils.h b/compat/win32/path-utils.h
> index f2e70872cd2..18eff7899e9 100644
> --- a/compat/win32/path-utils.h
> +++ b/compat/win32/path-utils.h
> @@ -20,6 +20,17 @@ static inline char *win32_find_last_dir_sep(const char *path)
>  	return ret;
>  }
>  #define find_last_dir_sep win32_find_last_dir_sep
> +static inline int win32_has_dir_sep(const char *path)
> +{
> +	/*
> +	 * See how long the non-separator part of the given path is, and
> +	 * if and only if it covers the whole path (i.e. path[len] is NULL),

The name of the ASCII character '\0' is NUL, not NULL (I'll fix it
while applying, so no need to resend if you do not have anything
else that needs updating).

Otherwise, the patch looks good. 

Thanks.

[gitgitgadget]

On the Git mailing list, András Kucsma wrote (reply to this):

On Fri, Mar 27, 2020 at 7:04 PM Junio C Hamano <gitster@pobox.com> wrote:
> The name of the ASCII character '\0' is NUL, not NULL (I'll fix it
> while applying, so no need to resend if you do not have anything
> else that needs updating).

Right, sorry! I have no other updates.

> Otherwise, the patch looks good.
>
> Thanks.

Thanks for the help!

[gitgitgadget]

On the Git mailing list, Andreas Schwab wrote (reply to this):

On Mär 27 2020, Junio C Hamano wrote:

> The name of the ASCII character '\0' is NUL, not NULL

NUL is not a name, it is an abbreviation or acronym.  Its name is the
Null character.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Mär 27 2020, Junio C Hamano wrote:
>
>> The name of the ASCII character '\0' is NUL, not NULL
>
> NUL is not a name, it is an abbreviation or acronym.  Its name is the
> Null character.

OK, let's put it differently.

> +	 * See how long the non-separator part of the given path is, and
> +	 * if and only if it covers the whole path (i.e. path[len] is NULL),

When referring to character '\0' like so, write "NUL", not "NULL",
as the latter is how you write a null pointer.

[gitgitgadget]

This branch is now known as ak/run-command-on-cygwin-fix.

[gitgitgadget]

This patch series was integrated into pu via git@b676b84.

[gitgitgadget]

This patch series was integrated into pu via git@e2fc54e.

[gitgitgadget]

This patch series was integrated into pu via git@4b747bb.

[gitgitgadget]

This patch series was integrated into pu via git@1231740.

[gitgitgadget]

This patch series was integrated into pu via git@1d782b4.

[gitgitgadget]

This patch series was integrated into pu via git@78aec63.

[gitgitgadget]

This patch series was integrated into pu via git@02e342b.

[gitgitgadget]

This patch series was integrated into pu via git@e72bf9e.

[gitgitgadget]

This patch series was integrated into next via git@9e98b82.

[gitgitgadget]

This patch series was integrated into pu via git@7931f45.

[gitgitgadget]

This patch series was integrated into pu via git@bbc93cd.

[gitgitgadget]

This patch series was integrated into pu via git@d01b722.

[gitgitgadget]

This patch series was integrated into next via git@d01b722.

[gitgitgadget]

This patch series was integrated into master via git@d01b722.

[gitgitgadget]

Closed via d01b722.