Hi Takashi, On Wed, 17 Dec 2025, Takashi Yano wrote:
> On Tue, 16 Dec 2025 10:31:17 +0100 (CET) > Johannes Schindelin wrote: > > > On Tue, 16 Dec 2025, Takashi Yano wrote: > > > > > On Mon, 15 Dec 2025 18:15:10 +0100 (CET) > > > Johannes Schindelin wrote: > > > > > > > > On Mon, 15 Dec 2025, Corinna Vinschen wrote: > > > > > > > > > On Dec 15 16:40, Johannes Schindelin wrote: > > > > > > > > > > > Also, it looks as if that other proposed patch will always add > > > > > > overhead, not only when the reparse point needs to be handled in a > > > > > > special way. Given that this code path imposes already quite a bit > > > > > > of > > > > > > overhead, overhead that delays execution noticeably and makes > > > > > > debugging less delightful than I'd like, I would much prefer to do > > > > > > it > > > > > > in the way that I proposed, where the extra time penalty is imposed > > > > > > _only_ in case the special handling is actually needed. > > > > > > > > > > You may want to discuss this with Takashi. Simplicity vs. Speed ;) > > > > > > > > With that little rationale, the patch to always follow symlinks does not > > > > exactly look simple to me, but complex and requiring some > > > > head-scratching... > > > > > > The overhead of path_conv with PC_SYM_FOLLOW is small, however, > > > it may be a waste of process time to call it always indeed. > > > > When I debugged this problem, I introduced debug statements that show how > > often that code path is hit. In a simple rebuild of the Cygwin runtime, it > > is hit _very often_, and it is vexing how slow the rebuild is. > > > > I don't want to pile onto the damage by adding overhead that is totally > > unnecessary in the common case (most times processes are _not_ spawned via > > app execution aliases), even if it is small. If nothing else, it > > encourages more of that undesirable code pattern to add more and more > > stuff that is not even needed in the vast majority of calls. > > I believe path_conv::is_app_execution_alias() can minimize the overhead. I see that you dislike the idea of working with me, and want to go with your approach instead. I also see that you're not necessarily interested in conversing with me, otherwise you would spend many more words on talking to me rather than less so. That's something I can accept, although I would have preferred it to be spelled out clearly. > > > However, IMHO, calling CreateFileW() twice is not what we want to do. > > > I've just submitted v2 patch. In v2 patch, use extra path_conv only > > > when the path is a symlink. Usually, simple symlink is already followed > > > in spawn.cc: > > > https://cygwin.com/git/?p=newlib-cygwin.git;a=blob;f=winsup/cygwin/spawn.cc;h=71add8755cabf4cc0113bf9f00924fddb8ddc5b7;hb=HEAD#l46 > > > > Okay. However, then I don't understand how: > > > > 1. The patch in question is even necessary, as it would appear that it > > introduces a _second time_ where the symlink is followed? > > App execution alias can be executed by CreateProcess() while it cannot > be opened by CreateFile(). However, other windows reparse points can be > opened by CreateFile(). So, as your patch title saids, extra path_conv > is necessary only for app execution aliases. > > Therefore, I proposed path_conv::is_app_execution_alias() in v3 patch. So let me translate that into a form that would have actually not required several minutes of looking through code paths to reconstruct the thinking: While the "symlink" target of app execution aliases _was_ resolved when 2533912fc76c (Allow executing Windows Store's "app execution aliases", 2021-03-12) started allowing Microsoft Store apps to be executed, as of f74dc93c63 (fix native symlink spawn passing wrong arg0, 2025-03-10) they were _no longer_ resolved. Hence they are now only resolved _once_, namely in `is_console_app()`. > > 2. What purpose is the name `perhaps_suffix()` possibly trying to convey? > > I know naming is hard, but... `perhaps_suffix()`? Really? > > > > > The code is simpler than your patch 3/3 and my previous patch > > > and intent of the code is clearer. > > > > The intent of that previous patch is a far cry from clear without a > > much-improved commit message, I'd think. It talks about symlinks in > > general, but then uses the app execution alias `debian.exe` as example > > (when a simple test shows that regular symlinks do not need that fix at > > all), and the patch treats it as an "all symlinks" problem, too. Honestly, > > I am quite surprised to read this claim. > > Thanks. > Please recheck the commit message in v3 patch. 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. - 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. - 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 - 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. - 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). - 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. - 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. - 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. 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. You have push permission, so you can just push it as-is, of course. Obviously, I'd rather you improve the commit message (and the patch), but there is nothing I could do to enforce that. Ciao, Johannes
