Hi Johannes,
I'll respond to your comment based on v4 patch series, in which my patches
are intended to be merged into your patch series. The changes to your
patches are:
- is_console_app() returns true rather than false on errors. (The reason
is described bellow.)
- Replace your patch 2/2 with my v3 patch.
If you think your solution (call CreateFile() twice) is still better than
mine, please feel free to say that.
On Wed, 17 Dec 2025 16:50:28 +0100 (CET)
Johannes Schindelin wrote:
> I did. It still leaves a lot to be desired from my side:
>
> - It does not start with a clear statement of what is broken.
"After the commit f74dc93c6359, WSL cannot start by distribution name
such as debian.exe, which has '.exe' extention but actually is an app
execution alias."
> - It leaves a huge gap between mentioning the added `PC_SYM_NOFOLLOW_REP`
> flag and the `is_console_app()` function, leaving it as a lengthy
> homework assignment to each and every reader to figure out what possible
> connection there is between those two: At first sight they seem rather
> unrelated.
"This is because the commit f74dc93c6359 disabled to
follow windows reparse point by adding PC_SYM_NOFOLLOW_REP flag in
spawn.cc, that path is used for sapwning a process. As a result, the
path, that is_console_app () received, had been the reparse point of
app execution alias, then it returned false for the the path due to
open-failure because CreateFileW() cannot open an app execution alias,
while it can open normal reparse point. If is_console_app() returns
false, standard handles for console app (such as WSL) would not be
setup. This causes that the console input cannot be transfered to the
non-cygwin app."
> - Saying "This patch fixes the issue by converting the path again" cannot
> do anything but cause utter confusion because the path "conversion"
> happens at a totally different place than it used to happen before, and
> there is not the slightest assistance in that commit message to help
> anybody understand
"This patch fixes the issue by locally converting the path, which is
a path to the app execution alias, once again using PC_SYM_FOLLOW
(without PC_SYM_NOFOLLOW_REP) option path_conv for using inside of
is_console_app() to resolve the reparse point here, if the path is
an app execution alias."
> - how the code path is getting from that `perhaps_suffix()` function
> (which is not even mentioned _once_ in that commit message), where
> the `PC_SYM_NOFOLLOW_REP` flag is newly set, to the
> `is_console_app()` function, which is in a totally different file.
"This is because the commit f74dc93c6359 disabled to
follow windows reparse point by adding PC_SYM_NOFOLLOW_REP flag in
spawn.cc, that path is used for sapwning a process. As a result, the
path, that is_console_app () received, had been the reparse point of
app execution alias, then it returned false for the the path due to
open-failure because CreateFileW() cannot open an app execution alias,
while it can open normal reparse point."
> - what guarantee this patch makes that the touched code doesn't miss
> anything else that was broken by the "fix native symlink spawn"
> commit (or for that matter, whether there even has been given _any_
> thought to unintended side effects or unwanted gaps in the fix).
"This patch fixes the issue by locally converting the path, which is
a path to the app execution alias, once again using PC_SYM_FOLLOW
(without PC_SYM_NOFOLLOW_REP) option path_conv for using inside of
is_console_app() to resolve the reparse point here, if the path is
an app execution alias."
> - The commit message freely admits that the `is_console_app()` code
> blatantly ignores errors when calling `CreateFileW()`, and leaves
> things at that. The missing error checks (also for `ReadFile()`) are
> still as missing as before.
Because I assumed that my v3 patch would be mearged with your patch series.
> - The commit message says that the fix is to use `PC_SYM_FOLLOW` again,
> instead of `PC_SYM_NOFOLLOW_REP`. But the diff mentions neither of those
> constants. I don't know which reader would find this helpful, as I
> don't.
+ if (pc.is_app_execution_alias ())
+ {
+ UNICODE_STRING upath;
+ RtlInitUnicodeString (&upath, native_path);
+ path_conv target (&upath, PC_SYM_FOLLOW); <<<============
+ target.get_wide_win32_path (native_path);
+ }
> - One particularly irritating gap is the question why app execution
> aliases aren't simply special-cased such that their `argv[0]` is set to
> the symlink _target_, as it used to be when app execution alias support
> was introduced to `spawn()`. That would be quite an interesting
> discussion, in particular when the somewhat surprising fact is conveyed
> that app execution aliases are tied to a package identity, and executing
> them instead of the reparse point target path quite potentially equips
> the spawned process with permissions it would not otherwise have.
In v4 patch series, is_console_app() returns true when an error occurs.
This is for fail-safe if the app is actually a non-cygwin console app.
In this case, standard handles is not setup for non-cygwin app if it
returns false. Therefore, it is safer to return true for unknown case.
Setting-up standard handles for GUI apps by an error due to permission
issue, is poinless, but not unsafe.
> This is not the first time I have pointed out this class of problem in
> commit messages. When a bug fix is quite involved, it pays a disservice to
> any reader when the commit message just rushes through the words without
> even trying to do a good job of explaining the problem, the context, the
> approach taken to address the problem, and considerations what
> (potantially unintended) consequences the patch might lead to.
>
> I don't actually know how I can impress on you how crucial the skill of
> commit message writing is, how essential it is to practice it well until
> you do a better job. I have tried several times, and I am approaching my
> wits' end.
>
> If the author of a commit message would have trouble, after reading it as
> little as half a year in the future, to understand the reasoning behind
> their own patch (and there is not the slightest doubt in my mind that v3's
> commit message would fall into that exact category), then that commit
> message is in need of some work.
>
> Writing commit messages is a craft as much as writing code, and if you
> love your craft, you devote passion to honing that craft. It shows in the
> results whether you do that.
Noted. When I read the commit message I wrote several years ago,
I often do not remember how the problem was solved. Obviously,
it is due to unappropriate commit message. I've been trying to be
careful in recent years, but it seems I'm still not quite there.
--
Takashi Yano <[email protected]>
