Hi Takashi, 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: > > > > Hey Corinna, > > > > > > > > [Sorry for top-posting] > > > > > > /*rolling eyes*/ > > > > I wanted to reply quickly, which precluded me from using a mailer that > > allows inlined responses, sorry. > > > > > > 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. > 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? 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. Ciao, Johannes > > What do you think? > > -- > Takashi Yano <[email protected]> >
